OpenID4VCI: Same scope values #462
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| val issuerStateCorrect = issuerState?.let { | ||
| if (codeService.verifyAndRemove(it)) true | ||
| else if (issuerStateToCredentialOffer.remove(it) != null) true | ||
| else false | ||
| } ?: true |
There was a problem hiding this comment.
The new issuer_state validation accepts a state if codeService.verifyAndRemove returns true or if issuerStateToCredentialOffer.remove returns a value, but the credential-offer mapping is only removed in the second branch. A first valid request therefore leaves the issuer_state entry in issuerStateToCredentialOffer, and a second request using the same issuer_state bypasses replay protection via the remove branch and is accepted. The previous code removed both entries on first use, so this change introduces a one-time replay window for issuer_state-protected authorization.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I am not sure whether this is actually an issue in this specific case, because issuerStateToCredentialOffer will only contain an entry if a state was explicitly set via credentialOfferWithAuthorizationCode(...), which maybe is not be done in any flow.
However, the complete set of state maps defined as
private val issuerStateToCredentialOffer: MapStore<String, CredentialOffer> = DefaultMapStore(),
/** Associates issued codes with the auth request from the client. */
private val codeToClientAuthRequest: MapStore<String, ClientAuthRequest> = DefaultMapStore(),
/** Associates issued refresh tokens with the auth request from the client. *Refresh tokens are usually long-lived!* */
private val refreshTokenToAuthRequest: MapStore<String, ClientAuthRequest> = DefaultMapStore(),
/** Associates the issued request_uri to the auth request from the client. */
private val requestUriToPushedAuthorizationRequest: MapStore<String, AuthenticationRequestParameters> = DefaultMapStore(),
poses a general risk of unbounded growth. If processing breaks, a client never responds, or a flow is aborted midway, these entries may never be cleaned up.
Introducing a DefaultMapStore variant with automatic cleanup (e.g. time-based expiration or a fixed maximum size with eviction) could mitigate this risk and make the implementation more robust.
tlenz
left a comment
tlenz
left a comment
There was a problem hiding this comment.
Contains a general remark, but that is not specific for this pull request
| val issuerStateCorrect = issuerState?.let { | ||
| if (codeService.verifyAndRemove(it)) true | ||
| else if (issuerStateToCredentialOffer.remove(it) != null) true | ||
| else false | ||
| } ?: true |
There was a problem hiding this comment.
I am not sure whether this is actually an issue in this specific case, because issuerStateToCredentialOffer will only contain an entry if a state was explicitly set via credentialOfferWithAuthorizationCode(...), which maybe is not be done in any flow.
However, the complete set of state maps defined as
private val issuerStateToCredentialOffer: MapStore<String, CredentialOffer> = DefaultMapStore(),
/** Associates issued codes with the auth request from the client. */
private val codeToClientAuthRequest: MapStore<String, ClientAuthRequest> = DefaultMapStore(),
/** Associates issued refresh tokens with the auth request from the client. *Refresh tokens are usually long-lived!* */
private val refreshTokenToAuthRequest: MapStore<String, ClientAuthRequest> = DefaultMapStore(),
/** Associates the issued request_uri to the auth request from the client. */
private val requestUriToPushedAuthorizationRequest: MapStore<String, AuthenticationRequestParameters> = DefaultMapStore(),
poses a general risk of unbounded growth. If processing breaks, a client never responds, or a flow is aborted midway, these entries may never be cleaned up.
Introducing a DefaultMapStore variant with automatic cleanup (e.g. time-based expiration or a fixed maximum size with eviction) could mitigate this risk and make the implementation more robust.
| throw InvalidDpopProof("invalid type: ${dpopProof.header.type}") | ||
| } | ||
| if (dpopProof.payload.nonce == null || !dpopNonceService.verifyAndRemoveNonce(dpopProof.payload.nonce!!)) { | ||
| if (dpopProof.payload.nonce == null) { |
There was a problem hiding this comment.
cant this be pulled out as val nonce = dpopProof.payload.nonce?: throw throw UseDpopNonce(dpopNonceService.provideNonce(), "DPoP JWT nonce is null") so this check and the check in line 188 can be merged into this single line?
There was a problem hiding this comment.
same goes for the verifyAndRemoveNonce, (where you would not need a !! any more)
nodh
force-pushed
the
feature/openid4vci-scopes
branch
from
646ad5b to
e0c22ff
Compare
Handles the legitimate case of issuers advertising same scope values for different credential configurations (distinguished by their credential representation). Had to do some refactoring in our code to cleanly test this scenario.