-
Notifications
You must be signed in to change notification settings - Fork 27
fix signing alg values supported for ISO mdos in metadata #495
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
base: main
Are you sure you want to change the base?
Conversation
Cryptographic algorithm names used in the `credential_signing_alg_values_supported` parameter correspond to the algorithm identifiers used to secure the COSE `IssuerAuth` structure, as defined in [@!ISO.18013-5]. The values SHOULD be chosen from the following: | ||
|
||
* The value exactly matches the `alg` value in the IssuerAuth COSE header. | ||
* The value is a fully specified algorithm, as defined in [I-D.ietf-jose-fully-specified-algorithms], and the combination of the `alg` value and the signing key's curve in the `IssuerAuth` COSE structure matches the combination specified by the fully specified algorithm. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I stated in this PR openid/OpenID4VP#553 im concerned about the complexity of this logic, rather than just having a credential_signing_crv_values_supported
parameter which would mean the wallet nor the issuer would need to maintain mapping logic here as there would always be an explicit match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I agree that a _crv
parameter would be slightly less complex, the DCP WG decided not to introduce a _crv
parameter for OID4VP, and opted for consistency by making the same decision for OID4VCI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should actually require fully specified algorithm for this and remove the other option, otherwise the wallet does not know whether the curve is supported before getting the credential.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed in the WG and it makes no sense to require fully specified algorithm only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not an objection to this PR outright but I've tried to express my concerns in openid/OpenID4VP#553 which is related and I've not had any clarification to that, so until we do I'm going to request changes here.
Co-authored-by: Tobias Looker <[email protected]>
… format specific; change algorithm identifiers for credential_signing_alg_values_supported to COSE algorithm values for mdocs
…e proof type specific
@GarethCOliver @martijnharing could you please review. |
Cryptographic algorithm identifiers used in the `credential_signing_alg_values_supported` parameter correspond to the numeric COSE algorithm identifiers used to secure the `IssuerAuth` COSE structure, as defined in [@!ISO.18013-5]. The values SHOULD be chosen from the following: | ||
|
||
* The value exactly matches the `alg` value in the `IssuerAuth` COSE header; | ||
* Or, the value is a fully specified algorithm, as defined in [@!I-D.ietf-jose-fully-specified-algorithms], and the combination of the `alg` value and the signing key's curve in the `IssuerAuth` COSE structure matches the combination specified by the fully specified algorithm. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still concerned (and this applies to OpenID4VP too) that we are undermining referencing the COSE IANA registry with this language, which should be the primary source of algorithm identifiers. For example if guidance were to change with the IANA registry again in future, which technically any RFC can do, this language would make it super complicated to understand what an implementation should do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to agree, (especially in VCI) I think there is a reasonable chance that we are aligning things at the cost of ambiguity. An alternative would be to use arrays of crv/alg parameters, which doesn't have any risk of being misinterpreted.
…tation proof since we now have a separate PR
* Or, the value is a fully specified algorithm, as defined in [@!I-D.ietf-jose-fully-specified-algorithms], and the combination of the `alg` value and the signing key's curve in the `IssuerAuth` COSE structure matches the combination specified by the fully specified algorithm. | ||
|
||
Example: | ||
If the `IssuerAuth` structure contains an `alg` header value of `-7` (ECDSA with SHA-256, per [@!IANA.COSE]) and is signed using a P-256 key, it matches both `-7` and `-9` in `credential_signing_alg_values_supported`. The latter (`-9`) corresponds to ECDSA with P-256 and SHA-256, as defined in [@!I-D.ietf-jose-fully-specified-algorithms]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this means that there is no mechanism for the issuer to say:
" I support issuance of mdocs with -9, but not issuance of -7"
This can be important to know, since receiving an mdoc with -9 may not work for the wallet, while -7 does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martijnharing Yes, that is not possible today. I would argue that even if we split the parameter into dedicated alg/crv parameters, the wallet would either receive -7 or -9 depending on the issuer's choice. What solution would you suggest instead?
This PR fixes signing alg values supported for ISO mdos in metadata. Aligned with OID4VP language.
Fixes #303; fixes #354