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.

Sign up for GitHub

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

Jump to bottom

added wallet attestation scheme #52

Merged
merged 20 commits into from
Oct 5, 2023
Merged

added wallet attestation scheme #52

Changes from 4 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
9b5d2c2
added wallet attestation scheme
tlodderstedt Jul 7, 2023
74b182a
run fix-lint
tlodderstedt Jul 7, 2023
20adc96
added values as suggested by Paul
tlodderstedt Jul 12, 2023
2d10a1b
lint again
tlodderstedt Jul 12, 2023
dfbe1e4
Update draft-oid4vc-haip-sd-jwt-vc.md
tlodderstedt Jul 13, 2023
c5214b7
Update draft-oid4vc-haip-sd-jwt-vc.md
tlodderstedt Jul 13, 2023
1f479dc
Merge branch 'main' into wallet-attestation
tlodderstedt Jul 20, 2023
27450a8
fixed references to client attestation draft
tlodderstedt Jul 20, 2023
72ed205
Merge branch 'wallet-attestation' of https://github.com/vcstuff/oid4v…
tlodderstedt Jul 20, 2023
78ba68c
changed the PR to match the evolved concepts
tlodderstedt Sep 18, 2023
1ca5b6d
expanded key type values
tlodderstedt Sep 18, 2023
c2def75
itemized key type values, changed ref to ietf-oauth-attestation-based…
tlodderstedt Sep 18, 2023
794a339
make fix-lint
tlodderstedt Sep 18, 2023
c0a7c6a
profile is defined in HAIP
tlodderstedt Sep 18, 2023
8cb4f35
corrected key type values
tlodderstedt Sep 20, 2023
d9e263c
Apply suggestions from Giuseppe's code review
Sakurann Sep 25, 2023
1ae5b33
Update draft-oid4vc-haip-sd-jwt-vc.md
tlodderstedt Oct 4, 2023
e778c55
Update draft-oid4vc-haip-sd-jwt-vc.md
tlodderstedt Oct 5, 2023
8095800
Merge branch 'main' into wallet-attestation
tlodderstedt Oct 5, 2023
4f88057
Merge branch 'wallet-attestation' of https://github.com/vcstuff/oid4v…
tlodderstedt Oct 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 36 additions & 1 deletion draft-oid4vc-haip-sd-jwt-vc.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,42 @@ Note: Issuers should be mindful of how long the usage of the refresh token is al

### Wallet Attestation Schema {#wallet-attestation-schema}

[Section 3.1 of wallet attestation draft would define the basics, and this profile will define the details.]
Wallets MUST use attestations following the definition given in [!I-D.ietf-looker-key-attestation-client-authentication].

In addition to the claims, the Wallet Attestation MUST contain the following claims:
Copy link
Contributor

@Sakurann Sakurann Sep 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
In addition to the claims, the Wallet Attestation MUST contain the following claims:
In addition to the claims defined in in [@!I-D.looker-oauth-attestation-based-client-auth], the Wallet Attestation SHOULD contain the following claims defined by this profile:

made optional following the conversation below.


* `key_type`: this claim asserts the security mechanism the wallet can use to manage private keys. This capability is based on the capabilities of the execution environent of the wallet, this might be a secure element (in case of a wallet residing on a smartphone) or a Cloud-HSM (in case of a cloud wallet). This specification defines the following values for `key_type`: `Software`, `TEE`, `Strongbox`, `Secure Enclave`, and `Secure Element`.
tlodderstedt marked this conversation as resolved.
Show resolved Hide resolved
* `user_authentication`: this claim asserts the security mechanism the wallet can use to authenticate access to private keys. This specification defines the following values for `user_authentication`: `System-Biometry`, `System-PIN`, `Internal-Biometry`, `Internal-PIN`, and `SecureElement-PIN`.
Copy link
Contributor

@Sakurann Sakurann Sep 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
* `user_authentication`: this claim asserts the security mechanism the wallet can use to authenticate access to private keys. This specification defines the following values for `user_authentication`: `System-Biometry`, `System-PIN`, `Internal-Biometry`, `Internal-PIN`, and `SecureElement-PIN`.
* `user_authentication`: JSON string of a claim that asserts the security mechanism the wallet can use to authenticate access to private keys.

Is it a string or array of strings??

Copy link
Contributor

Choose a reason for hiding this comment

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

I honestly do not know what any of the following values for user_authentication mean: System-Biometry, System-PIN, Internal-Biometry, Internal-PIN, and SecureElement-PIN." suggest we open an issue for now.


These additional claims inform the issuer about the security capabilities of the wallet and allows the issuer to refuse credential issuance if the achievble security level of a certain wallet does not fulfil the issuer's requirements.
Copy link
Member

Choose a reason for hiding this comment

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

Do we have to defined error codes and messages for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean in the credential issuance response?

Copy link
Member

Choose a reason for hiding this comment

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

yes, do we have to give a ref to openid4vc?

Copy link
Contributor

@Sakurann Sakurann Sep 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

once this PR goes in (https://github.com/openid/OpenID4VCI/pull/64/files), we should probably add insufficient_wallet_security or something as an error code? for now, let's open an issue on this.

Copy link
Member

Choose a reason for hiding this comment

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

#62


Wallet attestions MUST support web-based key resolution as defined in Section 5 of [@!I-D.terbu-sd-jwt-vc]. The JOSE header `kid` MUST be used to identify the respective key.
tlodderstedt marked this conversation as resolved.
Show resolved Hide resolved

This is an example of a wallet attestation:
Sakurann marked this conversation as resolved.
Show resolved Hide resolved

```json
{
"typ": "wallet-attestation+jwt",
"alg": "ES256",
"kid": "1"
}
.
{
"iss": "https://wallet.example.com",
"sub": "https://wallet.example.com",
Copy link
Member

Choose a reason for hiding this comment

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

I'm not in favor of duplicating iss to sub

I have the strong assumption that the wallet provider and the wallet instance are different entities

In my current implementation we have decided to use the jwk thumbprint to identify the wallet instance and then we use this value in the sub

there's a good agreement to simply remove the sub claim, WDYT about this?

Copy link
Contributor Author

@tlodderstedt tlodderstedt Jul 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It's not a duplication. It represents the fact that the wallet provider issues an attestation about itself. Whatever component uses this attestation with a valid proof of possession of the key in cnf can be considered a valid "representative" of the wallet provider. That is sufficient for authentication and authorization.
This value is especially used a client_id. If you want to make the client id instance specific, you can do so. I would prefer to have a client_id identifying the wallet provider. This information can be used in self service and customer care and makes sense to the user.

Copy link
Collaborator

Choose a reason for hiding this comment

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

With regard to eIDAS terms: iss describes the Wallet Solution Provider and sub descibes the specific Wallet Solution of a Wallet Instance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sub descibes the specific Wallet Solution of a Wallet Instance
which includes the option to leave instances anonymous, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, anonymous instances should be RECOMMENDED in my opinion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the underlying draft-looker-oauth-attestation-based-client-auth, the iss is the entity the AS (here the wallet) trusts for the purpose of the issuance of attestations and the sub is the client id. The wallet can establish the trust be looking the iss value up in a trusted list, for example. The actual wallet product could be the sub value. The attributes (including capabilities and perhaps even authorization) need to be conveyed through claims in the attestation.
The difference between attester and wallet provider would be that if the wallet provider is in the iss, it needs to be entitled by the trusted list to issue it's own attestations whereas in the case of the attester, it's the attester who has that entitlement.

Does this sound reasonable?

Copy link
Member

Choose a reason for hiding this comment

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

I'm in favor of having sub as optional and without any constraint about its value

the current proposal is concerning to me

Copy link
Collaborator

Choose a reason for hiding this comment

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

in the call yesterday we came to the conclusion that the fundamental question is:
"Is the Wallet Provider always the Attester of the Wallet Instance?"

Indeed, we have already seen the reality that wallet providers may not have the capacity/willingness to run an attestation service on their own. And nobody will force an wallet provider to use a different attestation service. But we should keep this option open. Therefore I am voting to use:

  • iss as the issuer of the wallet attestation
  • sub as the client id
    The version should not be referenced in there as this is responsibility of the wallet provider and not a burden to the issuer. If the Wallet Provider runs the attestation service on his own, we have two possibilities:
  • a) iss equals sub
  • b) sub may be omitted
    I'm in favor of a)

Copy link
Contributor

Choose a reason for hiding this comment

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

@peppelinux making the sub optional cannot be done in HAIP and needs to be discussed in the IETF on the client attestation draft. For HAIP, I suggest we remove the concrete values and put in letter what the values are expected to be: code suggestion below.

Copy link
Member

Choose a reason for hiding this comment

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

in the implementation I'm working on the sub value is the thumbprint of the cnf.jwk since this latter uniquely identify, anyway, the wallet instance

Sakurann marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"iss": "https://wallet.example.com",
"sub": "https://wallet.example.com",
"iss": "<identifier of the issuer of this wallet attestation>",
"sub": "<`client_id` of the OAuth client>",

@peppelinux

Copy link
Member

Choose a reason for hiding this comment

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

I think that you found the best editorial compromise for this annoying issue of the sub

Copy link
Member

Choose a reason for hiding this comment

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

I would also like to add to this discussion that we agreed that the claim client_id represents the unique client identifier, even if there are divisions about who is the real client: the wallet solution or the wallet instance. I'm in favour of the latter, by having the distinction between iss and sub when the wallet instance is installed on a personal mobile device and all the respective crytpographic keys are stored by each material owner (wallet provider has its own, then the wallet instance has its own).

However, "client_id" is not always used to specify a client identifier since there are many way in OAuth2 to uniquely identify a client within a flow, below I mention for instance methods like:

  • client_secret_basic
  • client_secret_jwt
  • private_key_jwt

even if these are not relevant for the wallet solution, I'd try to not mandate too many details within the specs at this current phase, by giving more flexibility in the material implementations on the way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

incorporated Kristina's suggestion

"exp": 1516247022,
"key_type": "STRONGBOX",
"user_authentication": "APP_PIN_6_DIGITS",
"cnf": {
"jwk": {
"kty": "EC",
"crv": "P-256",
"x": "TCAER19Zvu3OHF4j4W4vfSVoHIP1ILilDls7vCeGemc",
"y": "ZxjiWWbZMQGHVWKVQ4hbSIirsVfuecCE6t4jT9F2HZQ"
}
}
}
```

## Credential Endpoint

Expand Down