OID4VCI: Rework attestations#549
OID4VCI: Rework attestations#549nodh wants to merge 2 commits intofeature/deprecated-code-5.12.0from
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.
Reviewed commit: cd9f00c210
ℹ️ 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".
| supportedAlgorithms = listOf() // TODO | ||
| ) | ||
| )?.getOrElse { throw IllegalArgumentException("Key attestation required, none provided", it) } | ||
| ?.header?.keyAttestation |
There was a problem hiding this comment.
In createCredentialRequestProofAttestation, the code now reads loadKeyAttestation(...).header.keyAttestation as the proof value. loadKeyAttestation is typed to return a signed KeyAttestationJwt itself, so in the normal case its JOSE header does not contain another nested keyAttestation field. This makes attestation-proof issuance fail with "Key attestation required, none provided" for wallets that return a standard key-attestation JWT. Use the returned JWS serialization as the attestation proof payload.
Useful? React with 👍 / 👎.
| clientNonce = clientNonce, | ||
| supportedAlgorithms = listOf() // TODO | ||
| ) | ||
| )?.getOrNull() |
There was a problem hiding this comment.
When keyAttestationRequired is present, this path only checks whether the callback exists, then calls getOrNull() and silently drops callback failures. That allows building a JWT proof without key_attestation even though issuer metadata requires it, causing late issuer-side rejection and hiding the local root cause. The required-attestation path should propagate callback errors instead of degrading to null.
Useful? React with 👍 / 👎.
| loadKeyAttestation?.invoke( | ||
| KeyAttestationInput( | ||
| clientNonce = clientNonce, | ||
| supportedAlgorithms = listOf() // TODO |
There was a problem hiding this comment.
This change hardcodes supportedAlgorithms = listOf() in the attestation proof path, while the previous implementation forwarded issuer-declared proof_signing_alg_values_supported. If the wallet provider uses this input to choose/sign attestations, it can now select an algorithm the issuer does not support, causing avoidable issuance failures against stricter issuers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: db3afe6881
ℹ️ 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".
| if (keyAttestationRequired != null && loadKeyAttestation == null) { | ||
| throw IllegalArgumentException("Key attestation required, none provided") |
There was a problem hiding this comment.
When keyAttestationRequired is advertised, this check only considers loadKeyAttestation and throws immediately if it is null, even though the class still accepts deprecated loadUnitAttestationPop. That regresses existing integrations that have not migrated yet: they now fail with "Key attestation required, none provided" despite supplying a valid attestation callback via loadUnitAttestationPop, whereas the previous implementation used that callback to build the proof.
Useful? React with 👍 / 👎.
1 participant
Requires a-sit-plus/signum#433