openid / OpenID4VCI

68 stars 20 forks source link

add key attestations #389

Closed paulbastian closed 2 days ago

paulbastian commented 2 months ago

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

Sakurann commented 2 months ago

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

andprian commented 2 months ago

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.

Sakurann commented 1 month ago

discussed on a WG call:

c2bo commented 1 month ago

This PR is not a 100% done, but the core parts should be here and we need a bit more discussion/attention so I'll convert to ready for review.

My take on the open points:

discuss if we need cnf claim --> I'd say the attested_keys array is fine and replaces cnf

discuss whether expiration of key attestation and expiration of key is the same or different --> I could see value in having the expiration time of the key as an optional value. These are different things imho (key attestation expiration and key expiration).

Sakurann commented 1 month ago

also worth clarifying that since nonce is mandatory in key attestation, without nonce endpoint, wallets cannot use key attestation proof type.

paulbastian commented 1 month ago

I see these scenarios:

paulbastian commented 1 month ago

@c2bo and I feel somewhat confident with the current state of the PR and are looking for more feedback! Open points:

c2bo commented 1 month ago

Some eyes especially on the possible values for user_authentication and key_type would be really helpful imho

Sakurann commented 1 month ago

from https://github.com/openid/OpenID4VCI/pull/389#issuecomment-2405477640

use 1 jwt proof type and key attestation with 10 keys

I think we should decide explicitly what should be the behavior in this case: A) Issuer returning only 1 credential for the key that has a PoP, or B) Issuer returning 10 credentials for all of the keys in the key attestation. following the logic of use attestation proof type with 10 keys 👍, I think B is ok?

antonwiens commented 1 month ago

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.

paulbastian commented 1 month ago

from #389 (comment)

use 1 jwt proof type and key attestation with 10 keys

I think we should decide explicitly what should be the behavior in this case: A) Issuer returning only 1 credential for the key that has a PoP, or B) Issuer returning 10 credentials for all of the keys in the key attestation. following the logic of use attestation proof type with 10 keys 👍, I think B is ok?

Should we add the following text to jwt proof typ:?

If an attestation is provided, the Credential Issuer SHOULD return a Credential for each of the keys provided in the attested_keys claim of the attestation.

paulbastian commented 1 month ago

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.

Sakurann commented 3 weeks ago

discussion during the WG

direction:

next steps:

c2bo commented 2 weeks ago

I would prefer to completely remove key_storage_type and user_authentication and directly go to apr only which should then become a mandatory claim. It feels like we need the complexity of a somewhat complete APR description anyway in the near future and this would reduce complexity. This would also reduce the amount of information that is leaked a bit - issuers will (hopefully?) be interested what level of assurance a key storage can provide and most likely not how exactly it is achieved.

The resulting key attestation JWT payload would then look like this:

{
  "iss": "<identifier of the issuer of this key attestation>",
  "iat": 1516247022,
  "exp": 1541493724,
  "apr" : [ "https://trust-list.eu/apr/high", "SESIP_HIGH" ],
  "attested_keys": [
    {
      "kty": "EC",
      "crv": "P-256",
      "x": "TCAER19Zvu3OHF4j4W4vfSVoHIP1ILilDls7vCeGemc",
      "y": "ZxjiWWbZMQGHVWKVQ4hbSIirsVfuecCE6t4jT9F2HZQ"
    }
  ]
}

In this case I would also believe that keeping the current (flat) structure instead of nested objects would be fine.

paulbastian commented 2 weeks ago

I would prefer to completely remove key_storage_type and user_authentication and directly go to apr only which should then become a mandatory claim. It feels like we need the complexity of a somewhat complete APR description anyway in the near future and this would reduce complexity. This would also reduce the amount of information that is leaked a bit - issuers will (hopefully?) be interested what level of assurance a key storage can provide and most likely not how exactly it is achieved.

The resulting key attestation JWT payload would then look like this:

{
  "iss": "<identifier of the issuer of this key attestation>",
  "iat": 1516247022,
  "exp": 1541493724,
  "apr" : [ "https://trust-list.eu/apr/high", "SESIP_HIGH" ],
  "attested_keys": [
    {
      "kty": "EC",
      "crv": "P-256",
      "x": "TCAER19Zvu3OHF4j4W4vfSVoHIP1ILilDls7vCeGemc",
      "y": "ZxjiWWbZMQGHVWKVQ4hbSIirsVfuecCE6t4jT9F2HZQ"
    }
  ]
}

In this case I would also believe that keeping the current (flat) structure instead of nested objects would be fine.

This is some feedback that I got from security advisors:

"We think the key_storage_type and user_authentication are not needed, but the APR should be split into two values: APR for authentication mechanism and apr for the key store itself (e.g. resistance against extraction/duplication). Additionally each APR value should come with a proof in form of either a CC certificate (including the EAL) or some other custom evaluation scheme (including some kind of value showing the depth of evaluation) attached."

c2bo commented 1 week ago

This is some feedback that I got from security advisors:

"We think the key_storage_type and user_authentication are not needed, but the APR should be split into two values: APR for authentication mechanism and apr for the key store itself (e.g. resistance against extraction/duplication). Additionally each APR value should come with a proof in form of either a CC certificate (including the EAL) or some other custom evaluation scheme (including some kind of value showing the depth of evaluation) attached."

Two separate APR values sound like a good idea, but I am not sure it makes sense to always deliver the proofs for those? Couldn't we just link to some (online) resource with the detailed information and have some reference between the detailed certification and the system attesting the keys, but I guess those are details. Do we have any example of expected contents of those 2 apr values?

peppelinux commented 1 week ago

I would prefer to completely remove key_storage_type and user_authentication and directly go to apr only which should then become a mandatory claim. It

This Is exactly the approach we have in Italy, where we don't Need and don't want that kind of detail. APR Is enough, we should map which storage/auth types would be included in which APR type/level in a common trust framework