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

Merged
merged 65 commits into from
Nov 19, 2024
Merged

add key attestations #389

merged 65 commits into from
Nov 19, 2024

Conversation

paulbastian
Copy link
Contributor

@paulbastian paulbastian commented Sep 6, 2024

Closes #355
Closes #368
Closes #215
Closes #150

  • 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
  • relax proof/proofs (doesn't have to be a Pop)

@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 <[email protected]>
@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.

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.

I have provided additional comments, some editorial and others requesting further clarification for the readers. I can approve this regardless, as it appears to be very well done to me.

Copy link

@bj-ms bj-ms left a comment

Choose a reason for hiding this comment

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

Just added a comment about identifiers in key attestation JWTs, otherwise I find it to be a good extension to the specification!

Comment on lines +2330 to +2331
"iss": "<identifier of the issuer of this key attestation>",
"iat": 1516247022,
Copy link

Choose a reason for hiding this comment

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

Isn't that an information that we could get from the key attestation certificate from the key storage/WSCD, if necessary?

I would expect a wallet to already have an internal mapping with identifiers between key attestation jwts and the location where the key resides.

Co-authored-by: Giuseppe De Marco <[email protected]>
Comment on lines 2292 to 2294
There are two ways to convey key attestations during Credential issuance:
- The Wallet uses the `jwt` proof type in the Credential Request to create a proof of possession of the key and adds the key attestation in the JOSE header as specified in (#jwt-proof-type).
- The Wallet uses the `attestation` proof type in the Credential Request with the key attestation without a proof of possession of the key itself as specified in (#attestation-proof-type).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

todo: move this part to the proof type section

matzf added a commit to UbiqueInnovation/openid4vc that referenced this pull request Nov 18, 2024
Add key attestation metadata as described in
openid/OpenID4VCI#389, at commit
openid/OpenID4VCI@7130e4f.

As an aside, also add the optional nonce endpoint to the issuer metadata
which has been added recently.
Fix building a bunch of tests (but not all).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet