-
Notifications
You must be signed in to change notification settings - Fork 19
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
add key attestations #389
base: main
Are you sure you want to change the base?
add key attestations #389
Conversation
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 like it as a first draft and added some general comments.
General question: The plan would be to describe the overall mechanism in an Appendix and reference in Credential Endpoint (additional parameter for a Credential Request) and in Credential Issuer Metadata (to signal this is required for specific credential configurations)?
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.
good one @paulbastian , I support and follow this work, thank you
Co-authored-by: Giuseppe De Marco <[email protected]>
Is it possible to add description to the PR what this PR does and which issues it touches upon? thank you |
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.
my big question to this PR is where in the request do I put this key attestation needs to be defined, no?
I had the same question as @Sakurann as to where to put the key attestation. Moreover, if one attestation contains a list of keys, how can we provide one PoP for each key, and how to figure out which PoP corresponds to which key in the keys array. |
Co-authored-by: Christian Bormann <[email protected]>
HAIP will be aligned with the changes made herein after this is merged. |
* `nonce`: OPTIONAL. String that represents a nonce provided by the Issuer to proof that a key attestation was freshly generated. | ||
* `status`: OPTIONAL. JSON Object representing the supported revocation check mechanisms, such as the one defined in [@!I-D.ietf-oauth-status-list] | ||
|
||
The Credential Issuer MUST validate that the JWT used as a proof is actually signed by a key identified in the JOSE Header. |
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.
Is this object also used for proof type "attestation"? If so, this statement should be conditional.
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 you are correct. @c2bo did you add this text?
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.
This is from from your initial commit, but it seems I forgot to change it when introducing the second proof type. We should move this into the jwt proof type text imho.
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.
The Credential Issuer MUST validate that the JWT used as a proof is actually signed by a key identified in the JOSE Header. | |
If used with the `jwt` proof type, the Credential Issuer MUST validate that the JWT used as a proof is signed by a key contained in the attestation in the JOSE Header. |
Co-authored-by: Kristina <[email protected]>
* `iat`: REQUIRED (number). Integer for the time at which the key attestation was issued using the syntax defined in [@!RFC7519]. | ||
* `exp`: REQUIRED (number). Integer for the time at which the key attestation and the key(s) it is attesting expire, using the syntax defined in [@!RFC7519]. | ||
* `attested_keys` : REQUIRED. Array of attested keys from the same key storage component using the syntax of JWK as defined in [@!RFC7517]. | ||
* `key_type` : OPTIONAL. Case sensitive string that asserts the key storage component of the keys attested in the `attested_keys` parameter. This specification defines initial values in (#keyattestation-keytypes). |
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.
Editors Call:
- Mike says key_type is kind of used in JWK land, try to rename
|
||
A key attestation defined by this specification is an interoperable, verifiable statement that provides evidence of authenticity and security properties of a key and its storage component to the Credential Issuer. Keys can be stored in various key storage components, which differ in their ability to protect the private key against extraction and duplication, as well as in the methods used for End-User authentication to unlock key operations. These key storage components may be software-based or hardware-based, and can be located on the same device as the Wallet, on external security tokens, or on remote services that enable cryptographic key operations. Key attestations are issued by the Wallet's key storage component itself or by the Wallet Provider. When the Wallet Provider creates the key attestation, it needs to verify the authenticity of the claims it is making about the keys (e.g., by using the platform-specific key attestations). | ||
|
||
A Wallet MAY provide key attestations to inform the Credential Issuer about the properties of the provided cryptographic public keys, e.g. for proof types sent in the Credential Request. Credential Issuers may want to evaluate these key attestations to determine whether the keys meet their own security requirements, based on the trust framework in use, regulatory requirements, laws, or internal design decisions. A Credential Issuer SHOULD communicate this requirement to evaluate key attestations through its metadata or using some sort of out-of-band mechanism. |
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.
Given that the description of metadata here: https://github.com/openid/OpenID4VCI/pull/389/files#diff-1f424614b35a9899813079f1b1f6218631a2aedd993368ccb89bb81a9eda0289R1308 says
If the Credential Issuer does not expect a key attestation, this object is absent.
Is this sentence here valid
A Credential Issuer SHOULD communicate this requirement to evaluate key attestations through its metadata or using some sort of out-of-band mechanism.
It reads to me that a Credential Issuer can decide to require attestations but not put it in the metadata, but maybe that is the goal.
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.
Do you mean SHOULD -> MAY ?
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.
Justto clarify key attestations are completly optional, but if an Issuer wants them, the typical way in OAuth is to include the needs in the metadata, therefore we added such entries to the Credential Issuer metadata
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.
Do you mean SHOULD -> MAY ?
I am not qualified to have this conversations. :) I will try to explain In layman's terms the way I read this
The issuer medatada section says:
If the Credential Issuer does not expect a key attestation, this object is absent.
So my expectation here would be that if I read the metadata, don't find this object, i don't send an attestation as it is not required/expected -- but I think my expectation would be wrong because there is this sentence:
A Credential Issuer SHOULD communicate this requirement to evaluate key attestations through its metadata or using some sort of out-of-band mechanism.
This one reads, for me at least, like "Issuer may choose to require the attestation but the exact way this requirement is communicated is not enforced" i.e. if I go to metadata and see that there is no key_attestations_required
it could just mean that this expectation/requirement is defined elsewhere (other metadata, or using the mentioned out-of-band mechanisms like policy or interop profile).
I would potentially just remove the sentence I mentioned above from the issuer metadata section and add a link to the attestation section where one can read that this object can be used to advertise attestation requirement but that the usage of this filed is not mandated for such a purpose and that one needs to consult the issuer's/ecosystem's documentation on whether the wallet must include it or not. Or maybe even not add a link.
Co-authored-by: Nemanja Patrnogic <[email protected]>
{ | ||
"typ": "keyattestation+jwt", | ||
"alg": "ES256", | ||
"kid": "1" |
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.
Stupid comment, but this kid
is a bit confusing for me here in this form, is this referring to a particular key from the attestation or is it just a random value for the sake of the example?
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.
This jwt would be signed by the Wallet Backend or the key storage directly - it cannot be referring to a key from the attestation. But it might make sense to change it to x5c for the example to avoid confusion?
"kid": "1" | |
"x5c": ["MIIDQjCCA..."] |
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.
Would just point out this thing if it makes sense.
Object providing a single proof of possession of the cryptographic key material to which the issued Credential instance will be bound to...
Not sure if this will be in conflict because now credentials are not then bound to the key doing the proof or possession
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.
Good catch! We probably need to adapt those lines describing proofs and proof a bit. We still get a proof that the wallet is in possession of the key(s), it just doesn't have to be a proof of possession.
@@ -824,6 +828,8 @@ The JWT MUST contain the following elements: | |||
|
|||
The Credential Issuer MUST validate that the JWT used as a proof is actually signed by a key identified in the JOSE Header. | |||
|
|||
If an `attestation` is provided and successfully validated by the Credential Issuer, it SHOULD return a Credential for each of the keys provided in the `attested_keys` claim of the attestation. |
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.
Is there a requirement that the key doing the signing of the proof must be an attested key from the set of attested_keys
, and is that what the "kid"
is used below in the example?
I am having trouble finding where this is defined.
kind of a copy comment of: https://github.com/openid/OpenID4VCI/pull/389/files#r1811469557
* `exp`: REQUIRED (number). Integer for the time at which the key attestation and the key(s) it is attesting expire, using the syntax defined in [@!RFC7519]. | ||
* `attested_keys` : REQUIRED. Array of attested keys from the same key storage component using the syntax of JWK as defined in [@!RFC7517]. | ||
* `key_type` : OPTIONAL. Case sensitive string that asserts the key storage component of the keys attested in the `attested_keys` parameter. This specification defines initial values in (#keyattestation-keytypes). | ||
* `user_authentication` : OPTIONAL. Array of case sensitive strings that assert the authentication methods allowed to access the private keys from the `attested_keys` parameter. This specification defines initial values in (#keyattestation-auth). |
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.
Are the methods defined in user_authentication
evaluated as OR or AND? If they are OR, then how would you encode MFA?
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.
It was meant as logical OR, does this text fit better or do you have better words?
* `user_authentication` : OPTIONAL. Array of case sensitive strings that assert the authentication methods allowed to access the private keys from the `attested_keys` parameter. This specification defines initial values in (#keyattestation-auth). | |
* `user_authentication` : OPTIONAL. Array of case sensitive strings that assert the authentication methods allowed to access the private keys from the `attested_keys` parameter. The values given it this array are interpreted as a logical OR. This specification defines initial values in (#keyattestation-auth). |
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.
to clarify: the key given from key_storage_type
forms the possession factor. So whatever is in user_authentication
already creates MFA. If you want 3FA, then we would need to create a user_authentication
value like pin_and_biometry
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 not sure if this would meet NIST 2FA requirements for all listed cases since both factors have to be independent, i.e., especially not sure if the key is stored by the app and the device biometrics to unlock the same app are used would meet the 2FA requirement.
However, I'm fine with the suggested text to resolve this issue.
|
||
Since key attestations may be used for different Credential Issuers from different trust frameworks and varying in their requirements, it is necessary to use a common approach to facilitate interoperability. Therefore, key attestations SHOULD use a common format, allowing Credential Issuers to develop consistent evaluation processes, reducing complexity and potential errors. Common formats make it easy for Credential Issuers to demonstrate compliance with regulatory requirements across different jurisdictions and facilitate the development of shared best practices and security benchmarks. | ||
|
||
There are two ways to convey key attestations during Credential issuance: |
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.
Wouldn't it make sense to allow to convey the key attestations also in the in the Pushed Authorization and/or Token Request, i.e., using OAuth2 attestation-based client authentication? This way, the AS could also make a decision if they want to issue an access/refresh token to the wallet? Or is this mechanism out of scope of OID4VCI while still being possible because an AS can use the OAuth2 client authentication extension mechanism to support this use case anyways?
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.
If it is out of scope, perhaps adding a note would still make sense?
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 would say in an ideal world, key attestations would be its own draft and could be applied to OpenID4VCI credential request and attestation-based client authentication. If you would want it for access/refresh tokens, it would only require to add attestation
claim to the DPoP proof, which I also proposed (see checklist in PR description).
|
||
A Wallet MAY provide key attestations to inform the Credential Issuer about the properties of the provided cryptographic public keys, e.g. for proof types sent in the Credential Request. Credential Issuers may want to evaluate these key attestations to determine whether the keys meet their own security requirements, based on the trust framework in use, regulatory requirements, laws, or internal design decisions. A Credential Issuer SHOULD communicate this requirement to evaluate key attestations through its metadata or using some sort of out-of-band mechanism. | ||
|
||
Since key attestations may be used for different Credential Issuers from different trust frameworks and varying in their requirements, it is necessary to use a common approach to facilitate interoperability. Therefore, key attestations SHOULD use a common format, allowing Credential Issuers to develop consistent evaluation processes, reducing complexity and potential errors. Common formats make it easy for Credential Issuers to demonstrate compliance with regulatory requirements across different jurisdictions and facilitate the development of shared best practices and security benchmarks. |
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.
Should OID4VCI also say something about how these key attestations might relate to encryption? We had the discussion in the past that the encryption is not adding a lot on top of TLS because the wallet keys are not trusted necessarily. If the key attestations would also contain keys the wallet provides for encryption, wouldn't this solve this specific problem we had with encryption?
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 like the idea in general, but I think the keys from the wallet attestation (attestation-based client authenticaiton) may be a better fit, as the wallet attestation generates trust in who the wallet is, key attestation only says this is some valid key from hardware
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.
Wallet attestations are currently defined in HAIP. Probably a separate issue, but would it make sense to create an issue to add a note to the spec to explain how wallet attestation and encryption can relate to each other?
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've tried to compile the different discussions into proposed changes. In general I think this PR will not be able to properly/fully define the security properties (APR, storage types, etc), but we should aim to get to a starting point that can be properly extended.
|
||
* in the JWT body, | ||
* `iat`: REQUIRED (number). Integer for the time at which the key attestation was issued using the syntax defined in [@!RFC7519]. | ||
* `exp`: REQUIRED (number). Integer for the time at which the key attestation and the key(s) it is attesting expire, using the syntax defined in [@!RFC7519]. |
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.
Following the discussion, would this be acceptable for everyone?
* `exp`: REQUIRED (number). Integer for the time at which the key attestation and the key(s) it is attesting expire, using the syntax defined in [@!RFC7519]. | |
* `exp`: OPTIONAL (number). Integer for the time at which the key attestation and the key(s) it is attesting expire, using the syntax defined in [@!RFC7519]. MUST be present if the attestation is used with the attestation proof type. |
* `nonce`: OPTIONAL. String that represents a nonce provided by the Issuer to proof that a key attestation was freshly generated. | ||
* `status`: OPTIONAL. JSON Object representing the supported revocation check mechanisms, such as the one defined in [@!I-D.ietf-oauth-status-list] | ||
|
||
The Credential Issuer MUST validate that the JWT used as a proof is actually signed by a key identified in the JOSE Header. |
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.
The Credential Issuer MUST validate that the JWT used as a proof is actually signed by a key identified in the JOSE Header. | |
If used with the `jwt` proof type, the Credential Issuer MUST validate that the JWT used as a proof is signed by a key contained in the attestation in the JOSE Header. |
{ | ||
"typ": "keyattestation+jwt", | ||
"alg": "ES256", | ||
"kid": "1" |
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.
This jwt would be signed by the Wallet Backend or the key storage directly - it cannot be referring to a key from the attestation. But it might make sense to change it to x5c for the example to avoid confusion?
"kid": "1" | |
"x5c": ["MIIDQjCCA..."] |
Co-authored-by: Christian Bormann <[email protected]>
Co-authored-by: Christian Bormann <[email protected]> Co-authored-by: Oliver Terbu <[email protected]>
…e collision-resistant. Co-authored-by: Oliver Terbu <[email protected]>
Co-authored-by: Nemanja Patrnogic <[email protected]>
* `key_attestations_required`: OPTIONAL. Object that describes the requirement for key attestations as described in (#keyattestation), which the Credential Issuer expects the Wallet to send within the proof of the Credential Request. If the Credential Issuer does not expect a key attestation, this object is absent. If neither of the `key_storage_type`, `user_authentication` and `apr` parameters are present, this object may be empty, indicating that a key attestation without further constraints is required. | ||
* `key_storage_type`: OPTIONAL. Array defining values specified in (#keyattestation-keytypes) accepted by the Credential Issuer. | ||
* `user_authentication`: OPTIONAL. Array defining values specified in (#keyattestation-keytypes) accepted by the Credential Issuer. | ||
* `apr`: OPTIONAL. Array defining values specified in (#keyattestation-apr) accepted by the Credential Issuer. |
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.
feedback from a Funke team: it would be much more actionable if this information is communicated per credential type, and not in general, so "i need HSM for PID, but for diploma strongbox is ok" as opposed to "I am ok with HSM or strongbox"
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.
also, would be more helpful if this is designed as a combination: HSM and PIN, or strongbox with biometrics vs HSM or strongbox and PIN or biometrics
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.
If you have a look at the metadata it is associated to a particular credential configuration id, so that is already possible?
Closes #355
Closes #368