Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add specific requirements for response encryption #155

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

bc-pi
Copy link
Member

@bc-pi bc-pi commented Jan 10, 2025

Comment on lines +234 to +236
* Response encryption MUST be performed as specified in [@!OIDF.OID4VP, section 7.3]. The JWE `alg` (algorithm) header parameter (see [@!RFC7516, section 4.1.1])
value MUST be `ECDH-ES` (as defined in [@!RFC7518, section 4.6]), with key agreement utilizing keys on the `P-256` curve (see [@!RFC7518, section 6.2.1.1]).
The JWE `enc` (encryption algorithm) header parameter (see [@!RFC7516, section 4.1.2]) value MUST be `A128GCM` (as defined in [@!RFC7518, section 5.3]).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we might want to put it in a separate algorithms section later on, but this should be a good start

Copy link
Member Author

@bc-pi bc-pi Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was trying to do only what had been decided on about encryption by the WG to meet the stated requirements from ISO. I am not convinced this is a good idea or it actually fulfills the requirements. Getting to "all the details required to have interoperability need to be specified" involves more than just some encryption algs. And something like an MTI set of algs across the whole ting rather than mandating specific ones for one piece would probably be much better. But after circling around and around the HPKE discussions and being told that JARM and JWE were not implementable or interoperable, I am trying to move things forward with this as per the WG decision as I understood it to be.

@@ -535,6 +537,7 @@ The technology described in this specification was made available from contribut
* Require encrypted responses
* Remove reference to `client_id_scheme` parameter that no longer exists in OpenID4VP
* Refresh tokens are now optional
* Add specific requirements for response encryption
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be in -02 I do believe?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct. I only went so far as to look at the draft version in the head of the source, which was -01, but didn't look at https://openid.net/specs/openid4vc-high-assurance-interoperability-profile-1_0-01.html

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and 45c5a6e fixes that in this PR

* Specific requirements for the response encryption are tbd (https://github.com/openid/oid4vc-haip/issues/131).
* Response encryption MUST be performed as specified in [@!OIDF.OID4VP, section 7.3]. The JWE `alg` (algorithm) header parameter (see [@!RFC7516, section 4.1.1])
value MUST be `ECDH-ES` (as defined in [@!RFC7518, section 4.6]), with key agreement utilizing keys on the `P-256` curve (see [@!RFC7518, section 6.2.1.1]).
The JWE `enc` (encryption algorithm) header parameter (see [@!RFC7516, section 4.1.2]) value MUST be `A128GCM` (as defined in [@!RFC7518, section 5.3]).
Copy link
Collaborator

@awoie awoie Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a specific reason to use 128-bit keys, otherwise I'd use 256-bit keys for stronger security/compliance reasons.

Note that 128-bit keys are sufficiently secure to protect sensitive information in general. However, ISO 18013-5 uses 256-bit keys in their protocol. So, in case, this is required to meet ISO requirements, then it is probably better to use A256GCM instead of A128GCM.

Suggested change
The JWE `enc` (encryption algorithm) header parameter (see [@!RFC7516, section 4.1.2]) value MUST be `A128GCM` (as defined in [@!RFC7518, section 5.3]).
The JWE `enc` (encryption algorithm) header parameter (see [@!RFC7516, section 4.1.2]) value MUST be `A256GCM` (as defined in [@!RFC7518, section 5.3]).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. good catch Oliver

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going for bit strength parody with P-256 ECDH. Also some ISO document that I probably shouldn't have but was somehow on my hard drive was using AES-128-GCM as the AEAD part of HPKE. So I figured if it was good enough for use with the magnificence of HPKE, then it was probably good enough for this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, ISO 18013-7 Annex C uses AES 128 GCM while ISO 18013-5 uses AES 256 GCM. I guess this is fine then.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong preference, to be honest, I was mostly just giving rational for the initial choice.

Comment on lines +233 to +235
* Response encryption MUST be performed as specified in [@!OIDF.OID4VP, section 7.3]. The JWE `alg` (algorithm) header parameter (see [@!RFC7516, section 4.1.1])
value MUST be `ECDH-ES` (as defined in [@!RFC7518, section 4.6]), with key agreement utilizing keys on the `P-256` curve (see [@!RFC7518, section 6.2.1.1]).
The JWE `enc` (encryption algorithm) header parameter (see [@!RFC7516, section 4.1.2]) value MUST be `A128GCM` (as defined in [@!RFC7518, section 5.3]).
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this text would be better? It wouldn't preclude the use of the great and powerful HPKE should the https://datatracker.ietf.org/doc/draft-ietf-jose-hpke-encrypt/ endeavor pan out into something reasonable. But gives an MTI now that encourages interop.

Suggested change
* Response encryption MUST be performed as specified in [@!OIDF.OID4VP, section 7.3]. The JWE `alg` (algorithm) header parameter (see [@!RFC7516, section 4.1.1])
value MUST be `ECDH-ES` (as defined in [@!RFC7518, section 4.6]), with key agreement utilizing keys on the `P-256` curve (see [@!RFC7518, section 6.2.1.1]).
The JWE `enc` (encryption algorithm) header parameter (see [@!RFC7516, section 4.1.2]) value MUST be `A128GCM` (as defined in [@!RFC7518, section 5.3]).
* Response encryption MUST be performed as specified in [@!OIDF.OID4VP, section 7.3]. The JWE `alg` (algorithm) header parameter (see [@!RFC7516, section 4.1.1])
value `ECDH-ES` (as defined in [@!RFC7518, section 4.6]), with key agreement utilizing keys on the `P-256` curve (see [@!RFC7518, section 6.2.1.1]) MUST be supported.
The JWE `enc` (encryption algorithm) header parameter (see [@!RFC7516, section 4.1.2]) value `A128GCM` (as defined in [@!RFC7518, section 5.3]) MUST be supported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants