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

added wallet attestation scheme #52

Merged
merged 20 commits into from
Oct 5, 2023
Merged

added wallet attestation scheme #52

merged 20 commits into from
Oct 5, 2023

Conversation

tlodderstedt
Copy link
Contributor

@tlodderstedt tlodderstedt commented Jul 7, 2023

Closes # 51

📑 Description

This PR adds a Wallet Attestation scheme to the spec.

Preview Link

click here for rendered preview of PR

@tlodderstedt
Copy link
Contributor Author

The PR is not complete yet. It lacks the list of values allowed (or at least ore-defined) for user authentication and key type. I would also like to suggest to adopt the WebAuthn attestation claims instead of defining new claims and values.

key-protection-types

user-verification-methods

@paulbastian
Copy link
Collaborator

For a start I would recommend to limit the possible values as follows.
Key protection Types:

  • Software
  • TEE
  • Strongbox
  • Secure Enclave
  • Secure Element

User Verification Methods:

  • System-Biometry
  • System-PIN
  • Internal-Biometry
  • Internal-PIN
  • SecureElement-PIN

[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, the Wallet Attestation MUST contain the following claims:
Copy link
Contributor

Choose a reason for hiding this comment

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

In addition to what? To the claims defined in draft-ietf-looker?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In addition to "the definition given in [!I-D.ietf-looker-key-attestation-client-authentication]"


In addition, the Wallet Attestation MUST contain the following claims:

* `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`: `STRONGBOX`, `TEE`, `SecureEnclave`, `Software`.
Copy link
Contributor

Choose a reason for hiding this comment

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

are the values taken from somewhere or defined in HAIP? If the latter, why not use snake case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We define it in HAIP. I'm neutral on the style. Snake case would be consistent with https://fidoalliance.org/specs/common-specs/fido-registry-v2.2-rd-20210525.html#key-protection-types.

In addition, the Wallet Attestation MUST contain the following claims:

* `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`: `STRONGBOX`, `TEE`, `SecureEnclave`, `Software`.
* `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`: `APP_PIN_6_DIGITS`, ...
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, where are the values being defined..? If not in HAIP should it point somewhere?

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 prefer to reuse WebAuthn values. Let's discuss.

Copy link
Member

Choose a reason for hiding this comment

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

if WebAuthn values are used I'm ok with that, we then need a ref to them

draft-oid4vc-haip-sd-jwt-vc.md Outdated Show resolved Hide resolved
@tlodderstedt
Copy link
Contributor Author

@paulbastian can you please provide descriptions for the different values of key type and user authentication?

In addition, the Wallet Attestation MUST contain the following claims:

* `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`: `STRONGBOX`, `TEE`, `SecureEnclave`, `Software`.
* `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`: `APP_PIN_6_DIGITS`, ...
Copy link
Member

Choose a reason for hiding this comment

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

if WebAuthn values are used I'm ok with that, we then need a ref to them

* `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`: `STRONGBOX`, `TEE`, `SecureEnclave`, `Software`.
* `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`: `APP_PIN_6_DIGITS`, ...

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

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

.
{
"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

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

@tlodderstedt
Copy link
Contributor Author

feedback from the GAIN call: would it make sense to make user_authentication a multi value to carry all capabilities? WebAuthn does it this way for user authentication (userVerificationDetails) and key types (keyProtection).
https://fidoalliance.org/specs/fido-v2.0-rd-20180702/fido-metadata-statement-v2.0-rd-20180702.html#widl-MetadataStatement-userVerificationDetails

.
{
"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.

do we assume that a single wallet provider may own multiple wallet solution?

I assume that a single wallet provider owns a single wallet solution.
If we don't have agreement on how the sub value should be valued, considering that it may have the same value of iss and that's misleading since provider and instance are different entities, I suggest to get sub out by simply removing it from the required attributes

@TakahikoKawasaki
Copy link

In the discussions in this PR,

  1. the Wallet Provider and the Attester are competing for the iss claim, and
  2. the Per-Application identifier, the Per-Version identifier and the Per-Device identifer are competing for the sub claim.

Each of the concepts should have a separate claim in order to avoid conflicts. Otherwise, we would encounter technically troublesome issues in the future.

Comment on lines 146 to 147
* `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`, `Secure Element` and `External-HSM`.
* `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
Member

Choose a reason for hiding this comment

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

these information increases the responsability to the issuer that should evaluate mixed variables to an qualitative index

I'm not in favor of this, since, as already made with ACR values (LoA) in SAML2 and OIDC, a trust framework just have to define the properties within which some attestation levels are achieved

that's why we're working with attested_security_context

the regulation should map key types and user auth types, by quality, inside different level (low, middle, high?)

very often the technology changes, with upgrades and deprecation, using a normalized and indexed approach is better than expose issuer to internal evaluation of the security properties that, not at least, then exposes also personal tastes like the user auth type or differently, the key_Type that in some cases are mobile-brand specific -> then discloses the hardware type

Copy link
Collaborator

Choose a reason for hiding this comment

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

as discussed in the call yesterday, as long as we don't have a complete framework on attested security context and the mappings of key type and userauthentication to asc, it makes sense to offer key_type and user_authentication as this is a mechanism that is already working today and has been implemented.
Once we have a complete trust framework with asc mappings, we can add them to the client attestations

Therefore, I'm voting to keep them in for now and offer both possiblities

Copy link
Contributor

Choose a reason for hiding this comment

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

In general I agree with Giuseppe on the direction, but what Paul is saying that we are lacking those "normalized values" is also true.. so if we foresee that concrete key_type and user_authentication will be replaced by "normalized approach", I think at least we should not mandate key_type and user_authentication?

Copy link
Contributor

Choose a reason for hiding this comment

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

JohnB suggested external-multifactor

.
{
"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 in favor of having sub as optional and without any constraint about its value

the current proposal is concerning to me

draft-oid4vc-haip-sd-jwt-vc.md Outdated Show resolved Hide resolved
@peppelinux
Copy link
Member

peppelinux commented Jul 20, 2023

I have a good opinion about the possibility to use the WIA to give more guidance about how a wallet instance metadata discovery can be made, thanks to it

that's why I'd propose the following OPTIONAL claims to attests the WI capabilities, exemplified below in a non normative way

  "attested_security_context": "https://trust-framework.example.org/LoA/basic",
  "authorization_endpoint": "haip:",
  "response_types_supported": [
    "vp_token"
  ],
  "vp_formats_supported": {
    "jwt_vp_json": {
      "alg_values_supported": ["ES256"]
    },
    "jwt_vc_json": {
      "alg_values_supported": ["ES256"]
    }
  },
  "request_object_signing_alg_values_supported": [
    "ES256"
  ],
  "presentation_definition_uri_supported": false,

@@ -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.looker-oauth-attestation-based-client-auth].
Copy link
Member

Choose a reason for hiding this comment

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

Now I-D.ietf-oauth-attestation-based-client-auth


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

* `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`, `Secure Element` and `External-HSM`.
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't include spaces in the values. They can cause quirky problems, in practice.

Copy link
Member

@peppelinux peppelinux Sep 14, 2023

Choose a reason for hiding this comment

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

I would propose to use snake_case for the key values, then

  • software
  • tee
  • strongbox
  • secure_enclave
  • secure_element
  • external_hsm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

modified

@tlodderstedt
Copy link
Contributor Author

In the discussions in this PR,

the Wallet Provider and the Attester are competing for the iss claim, and
the Per-Application identifier, the Per-Version identifier and the Per-Device identifer are competing for the sub claim.
Each of the concepts should have a separate claim in order to avoid conflicts. Otherwise, we would encounter technically troublesome issues in the future.

Looking onto it from an OAuth AS perspective, I think is important to define a) what kind of assertions the AS should process and b) how the client id is determined.

For a) all what matters is that the AS can look that value up and knows how to access the signing key. Whether that is a 3rd party or the wallet provider itself, can be determined by the deployment.

For b) I would stick to the definition of RFC 752 "For client authentication, the subject MUST be the "client_id" of the OAuth client." That basically means client id values are at the discretion of the wallet assertion issuer. If the issuer decides to use ephemeral values, that's fine, too.


* `key_type`: OPTIONAL. JSON String that asserts the security mechanism the wallet uses to manage the private key associated with the public key given in the `cnf` claim. This mechanism 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`: It MUST be used when the wallet uses software-based key management.
* `hardware`: It MUST be used when the wallet uses software-based key management.
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
* `hardware`: It MUST be used when the wallet uses software-based key management.
* `hardware`: It MUST be used when the wallet uses hardware-based key management.

draft-oid4vc-haip-sd-jwt-vc.md Outdated Show resolved Hide resolved
draft-oid4vc-haip-sd-jwt-vc.md Outdated Show resolved Hide resolved
draft-oid4vc-haip-sd-jwt-vc.md Outdated Show resolved Hide resolved

* `key_type`: OPTIONAL. JSON String that asserts the security mechanism the wallet uses to manage the private key associated with the public key given in the `cnf` claim. This mechanism 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`: It MUST be used when the wallet uses software-based key management.
* `hardware`: It MUST be used when the wallet uses software-based key management.
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
* `hardware`: It MUST be used when the wallet uses software-based key management.
* `hardware`: It MUST be used when the Wallet uses software-based key management.

draft-oid4vc-haip-sd-jwt-vc.md Outdated Show resolved Hide resolved
* `tee`: It SHOULD be used when the wallet uses the Trusted Execution Environment for key management.
* `secure_enclave`: It SHOULD be used when the Wallet uses the Secure Enclave for key management.
* `strong_box`: It SHOULD be used when the Wallet uses the Strongbox for key management.
* `secure_element`: It SHOULD be used when the Wallet uses the Trusted Execution Environment for key management.
Copy link
Member

Choose a reason for hiding this comment

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

definition about Secure Enclave, Strongbox and TEE are required if any external reference is given to the reader

what they are or where they are defined is an editorial requirement

draft-oid4vc-haip-sd-jwt-vc.md Outdated Show resolved Hide resolved
draft-oid4vc-haip-sd-jwt-vc.md Outdated Show resolved Hide resolved
draft-oid4vc-haip-sd-jwt-vc.md Outdated Show resolved Hide resolved
draft-oid4vc-haip-sd-jwt-vc.md Outdated Show resolved Hide resolved
* `strong_box`: It SHOULD be used when the Wallet uses the Strongbox for key management.
* `secure_element`: It SHOULD be used when the Wallet uses a Secure Element for key management.
* `hsm`: It SHOULD be used when the Wallet uses Hardware Security Module (HSM).
* `user_authentication`: OPTIONAL. JSON String that asserts the security mechanism the Wallet uses to authenticate access to the private key associated with the public key given in the `cnf` claim. This specification defines the following values for `user_authentication`: `system_biometry`, `system_pin`, `internal_biometry`, `internal_pin`, and `secureelement_pin`.
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
* `user_authentication`: OPTIONAL. JSON String that asserts the security mechanism the Wallet uses to authenticate access to the private key associated with the public key given in the `cnf` claim. This specification defines the following values for `user_authentication`: `system_biometry`, `system_pin`, `internal_biometry`, `internal_pin`, and `secureelement_pin`.
* `user_authentication`: OPTIONAL. JSON String that asserts the security mechanism the Wallet uses to authenticate access to the private key associated with the public key given in the `cnf` claim. This specification plans to define the following values for `user_authentication`: `system_biometry`, `system_pin`, `internal_biometry`, `internal_pin`, and `secureelement_pin`.

we need to define each of these values like for key_type. can be another PR, but would like to be clear we will keep working on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paulbastian can you please provide definitions of the user authentication values?
@Sakurann I think we either don't define the claim at all or we fully define it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Sakurann Paul updated the PR with definitions. Please check.

Copy link
Contributor

@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.

happy to approve once my one suggestion is incorporated

@tlodderstedt tlodderstedt merged commit 38753fc into main Oct 5, 2023
2 checks passed
"y": "ZxjiWWbZMQGHVWKVQ4hbSIirsVfuecCE6t4jT9F2HZQ"
},
"key_type": "STRONGBOX",
"user_authentication": "SYSTEM_PIN",
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't these values be lower cased to match the definitions above?

@Sakurann
Copy link
Contributor

Sakurann commented Oct 5, 2023

why was it merged? i still had request for changes

@advatar
Copy link

advatar commented Sep 12, 2024

Sorry for being a bit late to the party.

What do you mean by internal_biometry? That the wallet has some kind internal implementation of face verification at some level of assurance? Then the level of the assurance is supplied by aal.

Is this correct?

I think that it might be the case that we will see an ecosystem of apps that, given a portrait, only verifies the presence (liveness) of the user. That would be more of external_biometry but maybe this distinction does not matter? What is important is that it is biometric authentication that is not by the system but by the portrait from the PID(passport/national id).

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.

8 participants