Skip to content

Defined APV values for ECDH-ES#628

Closed
GarethCOliver wants to merge 3 commits intomainfrom
apv
Closed

Defined APV values for ECDH-ES#628
GarethCOliver wants to merge 3 commits intomainfrom
apv

Conversation

@GarethCOliver
Copy link
Contributor

Solves topic from #597

This defines the value for ECDH-ES, as the base64url encoding of the sha-256 hash of concatted values.

The values to be concatted are defined based on mode per the request, with reasoning AIUI.

TODO to add two illustrative examples. examples (and update the above examples.

Comment on lines +1416 to +1417
- When response mode is `direct_post.jwt` the clientId, nonce, jwkThumbprint and responseUri
- When response mode is `dc_api.jwt` the origin, nonce and jwkThumbprint
Copy link
Member

Choose a reason for hiding this comment

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

Per https://www.rfc-editor.org/rfc/rfc7638.html#section-3, the JWK Thumbprint is the hash of the bytes of a canonical representation of the JWK. When SHA-256 is the hash function, this is 32 bytes of binary data. Is that what you intend to include in the concatenation? Or do you intend to include the base64url-encoding of the JWK Thumbprint in the concatenation?

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.

When this was proposed on the last call, I had asked that a clear description of the problem being solved with this proposed addition be provided in the PR description. Specifically, what is the attack that is possible without this addition and how does this addition prevent it? How realizable is the attack is in practice and what are its consequences? What parties must perform what malicious actions for the attack to succeed, and what are the preconditions for them to be able to do so?

This information isn't provided in the PR or in the predecessor PR or issue.

Given we're 6 weeks into the 60-day review for the specification to become final, I'm quite skeptical of making this breaking and complicating change without a clear and compelling description of why we MUST do this now. That description should include an analysis of why including these specific parameters in the computation will prevent the attack and why this is the right set of parameters to do so.

@bc-pi
Copy link
Member

bc-pi commented Jun 5, 2025

I am likewise skeptical of making a complicating and breaking change without a providing a clearly articulated and compelling rationale for doing so. In general but especially at the given stage of the process.

GarethCOliver and others added 2 commits June 5, 2025 08:04
Co-authored-by: Christian Bormann <8774236+c2bo@users.noreply.github.com>
Co-authored-by: Christian Bormann <8774236+c2bo@users.noreply.github.com>
@Sakurann
Copy link
Collaborator

Sakurann commented Jun 5, 2025

WG discussion about defining apu/apv values for ECDH-ES JWE..

  • on one hand...
    • adds marginal security benefit (majority agrees with @selfissued and @bc-pi disagreeing)
    • would not be the most complex part of the OpenID4VP implementation and can be implemented using existing JOSE libraries
    • there is implementation experience of similar mechanisms in 18013-7 annex B
  • on the other hand...
    • there are strong concerns that adding this mechanism last minute would result in it being underspecified leading to interoperability problems
    • extension mechanism to define apu/apv values could potentially be added in 1.1 (verifiers that support only that extension will potentially reject responses from wallets that do not support it)
    • wg seems to agree to define the content of a detached payload for JOSE-HPKE in OpenID4VP 1.1 or HAIP

wg agreed to discuss and tackle #347 during wglc/review period and as an outcome this PR and PR #597 will be closed for the above reasons since there is no wg rough consensus to proceed with either of them.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants