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

Mandate the use of apu/apv in the JWE header of OpenID4VP encrypted responses #380

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

Copy link
Member

@selfissued selfissued left a comment

Choose a reason for hiding this comment

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

Given that the nonce is part of the encrypted payload, it already contributes to the cryptographic output. Therefore also using it as the "apu" value is redundant.

Likewise, the Client ID is part of the encrypted content, and so need not also be in "apv".

@bc-pi
Copy link
Member Author

bc-pi commented Jan 10, 2025

Given that the nonce is part of the encrypted payload, it already contributes to the cryptographic output. Therefore also using it as the "apu" value is redundant.

Likewise, the Client ID is part of the encrypted content, and so need not also be in "apv".

Arguably none of this is needed or useful but this is my attempt to carry out the will of the WG in a compromise that isn't a complete departure from the specification stack this all builds on and hopefully put an end to months of confusing and confused but very heated discussion on the topic.

@tlodderstedt tlodderstedt added the ISO_VirtualMeeting relevant for ISO OID4VP mdoc profile over DC API label Jan 13, 2025
Copy link
Contributor

@awoie awoie left a comment

Choose a reason for hiding this comment

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

Approving this change since it implements what was agreed in the DCP WG call. I want to note that this will not work with the Multi RP PR #308 since the response might be encrypted to different client IDs. After the Multi RP RP got merged, this text will need to be revised accordingly.

@Sakurann
Copy link
Collaborator

@selfissued

Given that the nonce is part of the encrypted payload, it already contributes to the cryptographic output. Therefore also using it as the "apu" value is redundant.

Likewise, the Client ID is part of the encrypted content, and so need not also be in "apv".

the purpose of using nonce/client_id as apu/apv values is to ensure interoperability so that the verifier can decrypt, and NOT audience restriction or session binding

@bc-pi
Copy link
Member Author

bc-pi commented Jan 13, 2025

Approving this change since it implements what was agreed in the DCP WG call. I want to note that this will not work with the Multi RP PR #308 since the response might be encrypted to different client IDs. After the Multi RP RP got merged, this text will need to be revised accordingly.

Noted. Though I'm not sure what one response encrypted to different client IDs would look like...

Comment on lines +1133 to +1134
- The `apu` (Agreement PartyUInfo) header parameter MUST be set to the value of the `nonce` from the Authorization Request.
- The `apv` (Agreement PartyVInfo) header parameter MUST be set to the value of the `client_id` of the Verifier.
Copy link
Contributor

Choose a reason for hiding this comment

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

In ISO18013-7 the apv is the nonce instead of apu, does that make more or less sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

None of this makes much sense to me, to be honest. Per JWA - apu is information about the producer and apv is information about the recipient. ISO18013-7 has (or so I'm told) mdocGeneratedNonce for apu and nonce for apv which makes a certain amount of sense if you squint at it right. The wallet creates the mdocGeneratedNonce/apu so I guess it's information about the producer while the nonce/apv is generated by the verifier so I guess it's information about the recipient. If you squint at it a different way, it doesn't make any sense at all. I went with nonce as apu because it's about the the transaction or session the producer (Wallet) has with the Verifier and so seemed close enough to being about the producer and there are only two KDF contributing headers and client_id in apv made sense to me as information about the recipient because it's an identifier for the recipient.

That's a lot of words that probably don't make much sense either.

The upside though, as I said in #380 (comment), is that I don't think any of this is needed or useful.

@tlodderstedt
Copy link
Collaborator

Discussion in the WG call on the 14th of Jan 2025:

  • With the latest change in Add Multi RP Credentials/Authentication capability #308, we can no longer assume the encryption key to be related to a certain Client Identifier. We might change this into the origin. @paulbastian raised the question how the RP would determine the correct origin if the request contains multiple expected_origins.
  • @mike pointed out use of apv/apu would preclude algorithms that do not have those features.

@paulbastian
Copy link
Contributor

I think I agree with Mike, that defining new top-level parameters beside the JWE may be more future-proof than using apu and apv, as long as those things are not directly involved in the decryption itself

@awoie
Copy link
Contributor

awoie commented Jan 14, 2025

I think I agree with Mike, that defining new top-level parameters beside the JWE may be more future-proof than using apu and apv, as long as those things are not directly involved in the decryption itself

Tend to agree as well since all protected headers are part of the AAD which would include these newly defined headers. This would also meet the ISO requirement:

Response encryption authentication must be bound to the origin, e.g. RP URL.

@Sakurann
Copy link
Collaborator

so am I getting it right that the proposed solution is to add origin to the protected header?
since the requirement is Response encryption authentication must be bound to the origin, e.g. RP URL. I assume there is no need to add any other information to the protected header?

Does it also mean that we do not use apu and apv values? if we do not define them, there will be no way for both parties to be able to use the same values.

@bc-pi bc-pi closed this Jan 15, 2025
@bc-pi bc-pi reopened this Jan 15, 2025
@bc-pi
Copy link
Member Author

bc-pi commented Jan 15, 2025

the purpose of using nonce/client_id as apu/apv values is to ensure interoperability so that the verifier can decrypt, and NOT audience restriction or session binding

that was not my understanding but perhaps my not understanding is part of the problem

@bc-pi
Copy link
Member Author

bc-pi commented Jan 15, 2025

Discussion in the WG call on the 14th of Jan 2025:

* With the latest change in [Add Multi RP Credentials/Authentication capability #308](https://github.com/openid/OpenID4VP/pull/308), we can no longer assume the encryption key to be related to a certain Client Identifier. We might change this into the origin. @paulbastian raised the question how the RP would determine the correct origin if the request contains multiple `expected_origins`.

The rational for multiple expected_origins was questioned from it's introduction but it was explained that at IIW it was determined as needed because the backend infrastructure thing that signs these requests might not know the exact origin the whole page or whatever the script invoking the API is being served on. Which I can kinda see. But it'll have to know the set of possible origins in order to make the expected_origins list. I think an RP/Verifer would just need to ensure that the origin in the JWE header is one of those that it expects.

* @mike pointed out use of apv/apu would preclude algorithms that do not have those features.

I don't understand what that means exactly but what it seems to say on the face of things isn't correct.

@bc-pi
Copy link
Member Author

bc-pi commented Jan 15, 2025

I think I agree with Mike, that defining new top-level parameters beside the JWE may be more future-proof than using apu and apv, as long as those things are not directly involved in the decryption itself

I apologize for missing the call where this all was discussed and for my not being able to understand much of this but I don't understand how or why defining new top-level parameters beside the JWE would contribute anything to anything.

@bc-pi
Copy link
Member Author

bc-pi commented Jan 15, 2025

I think I agree with Mike, that defining new top-level parameters beside the JWE may be more future-proof than using apu and apv, as long as those things are not directly involved in the decryption itself

Tend to agree as well since all protected headers are part of the AAD which would include these newly defined headers. This would also meet the ISO requirement:

Response encryption authentication must be bound to the origin, e.g. RP URL.

"protected headers" are different than "top-level parameters beside the JWE" and maybe I'm being too literal but it's a meaningful distinction. Also apu and apv are protected headers. All headers are in the compact serialization (but please please don't take that as any kind of support for the JSON serialization). So yeah, the AAD would include these newly defined headers but it also includes the apu and apv headers (which also contribute to the KDF in ECDH). The text in the PR even tries to say this. So I am indeed sorry but I am pretty confused about what was discussed yesterday and what the concerns are and what to do with any of it.

@bc-pi
Copy link
Member Author

bc-pi commented Jan 15, 2025

so am I getting it right that the proposed solution is to add origin to the protected header? since the requirement is Response encryption authentication must be bound to the origin, e.g. RP URL. I assume there is no need to add any other information to the protected header?

Is this where the WG wants to take this? Asking 'cause I honestly don't know.

Does it also mean that we do not use apu and apv values? if we do not define them, there will be no way for both parties to be able to use the same values.

We don't define them now, they don't need to be used and default behavior when they aren't there is specified. So I'm not sure why it would be a problem. But I've very likely misunderstood something.

@Sakurann
Copy link
Collaborator

Sakurann commented Jan 16, 2025

since this is about origin, this has to be defined only for browser API.

we discussed two options:
option 1: set apv or apu as origin value.
option 2: define a new protected header origin. do not define apv/apu values?
-> Brian thinks this might be less of an evil. should work with some of the JWE libraries Brian is intimately aware of.
-> this might help to future proof for the algorithms that do not use apu/apv ie HPKE. but will prevent the goal of detached AAD because protected header is not detached?

apv and apu are protected to the same extent as other protected headers are protected.

the goal: fail early if the wrong origin was used? what does "Response encryption authentication must be bound to the origin, e.g. RP URL." mean..?

current situation: if the purpose of the requirement is replay prevention, there is already audience restriction via origin and client_id and nonce being present in the session transcript. if this is for HPKE and potentially detached ECDH-ES then DCP WG will tackle it later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ISO_VirtualMeeting relevant for ISO OID4VP mdoc profile over DC API priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Encryption details for OpenID4VP over the digital credentials API
6 participants