Skip to content
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

Open
wants to merge 48 commits into
base: main
Choose a base branch
from
Open

add key attestations #389

wants to merge 48 commits into from

Conversation

paulbastian
Copy link
Contributor

@paulbastian paulbastian commented Sep 6, 2024

Closes #355
Closes #368

  • link to the point in the spec where this is being used
  • add metadata
  • discuss if we need cnf claim
  • discuss whether OpenID4VCI wants to specify how to use DPoP with key attestation
  • update Security Consideration section for key attestation
  • discuss whether expiration of key attestation and expiration of key is the same or different
  • add text about Level of assurance and attack potential resistance
  • explain difference between two usages
  • new proof type that contains mandatory keyattestation with nonce
  • https://github.com/openid/OpenID4VCI/pull/389/files#r1797063233
  • tuning key_type and user_authentication values
  • provide initial values for apr and improve the description
  • provide better explanation/references for key_type

@paulbastian paulbastian marked this pull request as draft September 6, 2024 16:53
Copy link
Member

@c2bo c2bo left a 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)?

openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved
openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved
openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved
Copy link
Member

@peppelinux peppelinux left a 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

openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved
openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved
openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved
openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved
openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved
openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved
Co-authored-by: Giuseppe De Marco <demarcog83@gmail.com>
@Sakurann
Copy link
Collaborator

Is it possible to add description to the PR what this PR does and which issues it touches upon? thank you

Copy link
Collaborator

@Sakurann Sakurann left a 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?

@andprian
Copy link

andprian commented Sep 20, 2024

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 <8774236+c2bo@users.noreply.github.com>
Co-authored-by: Daniel Fett <fett@danielfett.de>
@paulbastian
Copy link
Contributor Author

Could you please align the HAIP profile wallet attestation with the key attestation. Also the ability to reuse the wallet/key attestation on all endpoints (PAR, Token and credential request) endpoints would be a nice possibility.

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.
Copy link
Collaborator

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.

Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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 <52878547+Sakurann@users.noreply.github.com>
* `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).
Copy link
Contributor Author

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.
Copy link
Contributor

@nemqe nemqe Oct 22, 2024

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.

Copy link
Contributor Author

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 ?

Copy link
Contributor Author

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

Copy link
Contributor

@nemqe nemqe Oct 24, 2024

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 <3052405+nemqe@users.noreply.github.com>
{
"typ": "keyattestation+jwt",
"alg": "ES256",
"kid": "1"
Copy link
Contributor

@nemqe nemqe Oct 22, 2024

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?

Copy link
Member

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?

Suggested change
"kid": "1"
"x5c": ["MIIDQjCCA..."]

Copy link
Contributor

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.

https://openid.github.io/OpenID4VCI/openid-4-verifiable-credential-issuance-wg-draft.html#section-8.2-2.3.1 says:

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

Copy link
Member

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.
Copy link
Contributor

@nemqe nemqe Oct 22, 2024

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).
Copy link
Contributor

@awoie awoie Oct 23, 2024

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?

Copy link
Contributor Author

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?

Suggested change
* `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).

Copy link
Contributor Author

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

Copy link
Contributor

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:
Copy link
Contributor

@awoie awoie Oct 23, 2024

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?

Copy link
Contributor

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?

Copy link
Contributor Author

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.
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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?

Copy link
Member

@c2bo c2bo left a 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.

openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved
openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved

* 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].
Copy link
Member

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?

Suggested change
* `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.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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"
Copy link
Member

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?

Suggested change
"kid": "1"
"x5c": ["MIIDQjCCA..."]

Co-authored-by: Christian Bormann <8774236+c2bo@users.noreply.github.com>
Co-authored-by: Christian Bormann <8774236+c2bo@users.noreply.github.com>
Co-authored-by: Oliver Terbu <oliver.terbu@mattr.global>
paulbastian and others added 2 commits October 24, 2024 14:03
…e collision-resistant.

Co-authored-by: Oliver Terbu <oliver.terbu@mattr.global>
Co-authored-by: Nemanja Patrnogic <3052405+nemqe@users.noreply.github.com>
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.

Optimization for Key Attestations for Multiple Keys Issuer Trust Evidence / key attestations for OpenID4VCI