-
Notifications
You must be signed in to change notification settings - Fork 27
Add support for request encryption to credential endpoint and request/response encryption to deferred credential endpoint #505
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
base: main
Are you sure you want to change the base?
Conversation
The PR indeed, adds the I have some questions. Assume the following:
In the above scenario, wallet has to place a deferred request. a) Should this differed request include also I am trying to understand, whether |
a) Yes, it must, will update to add text to that effect. Architecturally, the only downside to this approach is that the issuer would need to store the pending credential somewhere securely (whereas if the key is managed, they could encrypt it and store it anywhere). IMO avoiding key management is more important. I'll update the text a bit. |
* `enc`: REQUIRED. JWE [@!RFC7516] `enc` algorithm [@!RFC7518] for encrypting Credential Responses. | ||
* `zip`: OPTIONAL. JWE [@!RFC7516] `zip` algorithm [@!RFC7518] for compressing Credential Responses prior to encryption. If absent then compression MUST not be used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Someone will probably mention (or maybe not, I don't know) that the JWT BCP recommends against compressing prior to encryption: https://datatracker.ietf.org/doc/html/rfc8725#section-3.6
As I understand things, the BCP's guidance is well-intentioned but somewhat overreaching and lacking in nuance. The kind of nuance that would suggest it's fine in this kind of usage.
That's not really actionable, sorry, but seemed worth mentioning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know. Is there anything we can/should mention here why this sort of usage is fine? (Or why it is advised against normally)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have some security & privacy considerations added too, albeit I don't mind if we open an issue to do that in another PR so long as it gets into 1.0 - the effectiveness of this mechanism seems to be highly dependent on the wallet frontend fetching the issuer metadata directly rather than through the backend.
I think it would also be worthwhile to actually state the intended advantages of the encryption mechanism in the specification itself.
## Deferred Credential Response {#deferred-credential-response} | ||
|
||
The Deferred Credential Response uses the `credentials` and `notification_id` parameters as defined in (#credential-response). | ||
|
||
Additional Deferred Credential Response parameters MAY be defined and used. | ||
The Wallet MUST ignore any unrecognized parameters. | ||
|
||
The Deferred Credential Response MUST be sent using the `application/json` media type. | ||
If the Client requested an encrypted response by including the `credential_response_encryption` object in the request, the Credential Issuer MUST encode the information in the Deferred Credential Response as specified by [#encrypted-messages], using the parameters from the `credential_response_encryption` object. The `credential_response_encryption` object may be different from the one included in the initial Credential Request so the Credential Issuer MUST use the newly provided one. This is to simplify key management in the case of longer deferred issuance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this has possibly semantically clashed with https://github.com/openid/OpenID4VCI/pull/492/files and it's worth merging main into this branch so it's clear what the behaviour now is.
I think after #492 this text will now also result in the "credentials are not ready" request being encrypted. I don't know if that's good or bad but I think it's different to how you originally wrote this PR :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GarethCOliver can you please resolve conflicts by pulling the main as joseph suggests? (i also want to see html generated once the pipeline runs to confirm that [#---] reference is not compiling correctly)
Co-authored-by: Joseph Heenan <[email protected]>
Co-authored-by: Joseph Heenan <[email protected]>
Co-authored-by: Joseph Heenan <[email protected]>
Makes sense. Probably two sections: One for what encryption can protect against and what it doesn't (particular in regard to TLS already existing). The second is the one christian is starting for wallet backend considerations. The problem you are describing is definitely true for retrieving the keys (you have to have some way, on the device, to trust them (the most obvious of which is fetching them directly). I think you would have the same issue with any other client metadata you are trusting (i.e. if it isn't fetched directly you can't trust it, e.g. if I pulled the endpoint from a wallet backend, even if I directly connected to it, it'd be a problem). Filed two issues on these. |
@@ -829,6 +829,12 @@ The `c_nonce` value can be retrieved from the Nonce Endpoint as defined in (#non | |||
Additional Credential Request parameters MAY be defined and used. | |||
The Credential Issuer MUST ignore any unrecognized parameters. | |||
|
|||
The Credential Issuer indicates support for encrypted requests by including the `credential_request_encryption` parameter in the Credential Issuer Metadata. The Client MAY encrypt the request when `encryption_required` is `false` and MUST do so when `encryption_required` is `true`. | |||
|
|||
When performing Credential Request encryption, the Client MUST encode the information in the Credential Request in a JWT as specified by [#encrypted-messages], using the parameters in from `credential_request_encryption` object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When performing Credential Request encryption, the Client MUST encode the information in the Credential Request in a JWT as specified by [#encrypted-messages], using the parameters in from `credential_request_encryption` object. | |
When performing Credential Request encryption, the Client MUST encode the information in the Credential Request in a JWT as specified by (#encrypted-messages), using the parameters in from `credential_request_encryption` object. |
|
||
When performing Credential Request encryption, the Client MUST encode the information in the Credential Request in a JWT as specified by [#encrypted-messages], using the parameters in from `credential_request_encryption` object. | ||
|
||
If the Credential Request is not encrypted, the media type of the request MUST be set to `application/json`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's kind of weird having media type for non-encrypted case all across the spec and for encrypted case in one section...
|
||
* `credential_identifier`: REQUIRED when an Authorization Details of type `openid_credential` was returned from the Token Response. It MUST NOT be used otherwise. A string that identifies a Credential Dataset that is requested for issuance. When this parameter is used, the `credential_configuration_id` MUST NOT be present. | ||
* `credential_configuration_id`: REQUIRED if a `credential_identifiers` parameter was not returned from the Token Response as part of the `authorization_details` parameter. It MUST NOT be used otherwise. String that uniquely identifies one of the keys in the name/value pairs stored in the `credential_configurations_supported` Credential Issuer metadata. The corresponding object in the `credential_configurations_supported` map MUST contain one of the value(s) used in the `scope` parameter in the Authorization Request. When this parameter is used, the `credential_identifier` MUST NOT be present. | ||
* `proofs`: OPTIONAL. Object providing one or more proof of possessions of the cryptographic key material to which the issued Credential instances will be bound to. The `proofs` parameter contains exactly one parameter named as the proof type in (#proof-types), the value set for this parameter is a non-empty array containing parameters as defined by the corresponding proof type. | ||
* `credential_response_encryption`: OPTIONAL. Object containing information for encrypting the Credential Response. If this request element is not present, the corresponding credential response returned is not encrypted. | ||
* `jwk`: REQUIRED. Object containing a single public key as a JWK used for encrypting the Credential Response. | ||
* `alg`: REQUIRED. JWE [@!RFC7516] `alg` algorithm [@!RFC7518] for encrypting Credential Responses. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is alg being removed..? alg and zip are not mutually exclusive, no?
@@ -1089,7 +1093,7 @@ Credential Response can be immediate or deferred and can contain one or more Cre | |||
|
|||
The HTTP status code MUST be 202 (see Section 15.3.3 of [@!RFC9110]). | |||
|
|||
If the Client requested an encrypted response by including the `credential_response_encryption` object in the request, the Credential Issuer MUST encode the information in the Credential Response as a JWT using the parameters from the `credential_response_encryption` object. If the Credential Response is encrypted, the media type of the response MUST be set to `application/jwt`. If encryption was requested in the Credential Request and the Credential Response is not encrypted, the Client SHOULD reject the Credential Response. | |||
If the Client requested an encrypted response by including the `credential_response_encryption` object in the request, the Credential Issuer MUST encode the information in the Credential Response as specified by [#encrypted-messages], using the parameters from the `credential_response_encryption` object. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the Client requested an encrypted response by including the `credential_response_encryption` object in the request, the Credential Issuer MUST encode the information in the Credential Response as specified by [#encrypted-messages], using the parameters from the `credential_response_encryption` object. | |
If the Client requested an encrypted response by including the `credential_response_encryption` object in the request, the Credential Issuer MUST encode the information in the Credential Response as specified by (#encrypted-messages), using the parameters from the `credential_response_encryption` object. |
Resolves #339
Resolves #286
Resolves #506
resolves #480
Resolves adding support for
zip
to support #480The adds in a new credential issuer metadata parameter to support Credential Request encryption. Additionally it updates Deferred Credential Endpoint to support encryption in the same manner as Credential Endpoint.
Merging this with the existing credential response encryption was considered, but given we allow all permutations of mandatory/optional/not supported for both request and response independently, there isn't a great way to do this.
If we required issuers to be consistent on support or not then these would be cleaner as single parameter (but perhaps something we should leave to HAIP).
To be consistent with HAIP,
alg
is now required in thejwk
, a common section on how to perform selection is included andkid
must be included if available.The removal of alg is a breaking change but IMO is worth while to be consistent across the specs.