-
Notifications
You must be signed in to change notification settings - Fork 25
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 Multi RP Credentials/Authentication capability #308
base: main
Are you sure you want to change the base?
Conversation
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.
The JWS Compact Serialization is supported by essentially all implementations, whereas many don't support the JSON Serialization, in part, because JWTs don't use it.
I believe switching to the JSON Serialization would present an unnecessary burden to developers and harm interoperability.
(For what it's worth, I believe that adding the JSON Serialization a dozen or so years ago at all was a mistake, but I realize that that's water under the bridge.)
As I pointed out in the issue that this PR purports to resolve: when there is a trust infrastructure in place the request does not need to be signed as the wallet can determine the authenticity of the RP using the trust infrastructure. So we do not need multiple signatures and JSON serialisation. |
@David-Chadwick well... the wallet determines |
Co-authored-by: Stefan Charsley <[email protected]>
Co-authored-by: Christian Bormann <[email protected]>
Co-authored-by: Christian Bormann <[email protected]>
The profile supports unsigned requests. Would that be appropriate for your use cases? |
@Sakurann What you say is only one way of evaluating trust. The trust infrastructure can equally well decide if the RP is trustworthy or not based on its asserted name or ID. No keys are needed for this. If the asserted name/ID is deemed to be untrustworthy by the trust infrastructure, then the wallet knows not to trust the RP. If the name/ID is deemed to be trustworthy then there are two cases. 1. The RP is giving its true name, or 2. the RP is masquerading as a trustworthy RP. The latter can be protected against by the trust infrastructure providing the return URI of the RP for the wallet's response, and then the masquerading RP will not glean any information from the wallet. This is what we implemented. To prevent a DOS attack on the real trustworthy RP, we could add an extra protection feature, namely, the RP provides a random number on its request, which forms the query parameter on the wallet's reply to the return URI. The RP checks this parameter and if it is not recognised throws the wallet's response away. |
@tlodderstedt Yes, not requiring the RP's request to be signed solves the use case, and it also solves the problem of requiring multiple signatures to be on the request, because no signatures are actually needed. The RP can provide a set of names/IDs, one for each trust infrastructure that it is a member of. Then the wallet can match the presented trust infrastructures to the ones it is a member of, and where there is an intersection find out if the RP is trustworthy or not. |
@selfissued The requirement we are asked to fulfill is to allow for multiple RP credentials being proven (through signatures) in a single request. We have extensively discussed this requirement in the hybrid workshop and agreed to support it. The design in the PR is, in my opinion, the simplest and most robust solution we can do it (alternative proposals involved canonicalization). So from my standpoint, what we can do is to also continue to support compact serialization. That's why I asked that question further up. |
Co-authored-by: Kristina <[email protected]>
} | ||
}, | ||
"presentation_definition": {...} | ||
"dcql_query": {...} |
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 were the client_id
, client_metadata
, and jwks
deleted? This change seems unrelated to the purpose of 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.
This is indeed related to this PR, as this data elements are RP specific. Consequently, the PR moves them into the RP credential specific elements of the request.
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.
We should discuss where the client_id
, client_metadata
, and jwks
claims should go when using the Compact Serialization. These would normally be claims - not header parameters.
I understand the reasoning for making them header parameters when using the JSON Serialization, but in my view, that's not the normal case, and we should make the normal case natural.
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.
client_id
, client_metadata
, and jwks
are request parameters when using the Compact Serialization. see examples/digital_credentials_api/signed_request_payload_compact.json
The following request parameters MUST be present in the protected header of the respective signature object: | ||
|
||
* `client_id` | ||
* `client_metadata` |
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.
These aren't defined header parameters, so this is weird. Why aren't these being sent as claims in the request?
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.
because they are specific to a certain RP credential
"payload": "eyAiaXNzIjogImh0dHBzOi8...NzY4Mzc4MzYiIF0gfQ", | ||
"signatures": [ | ||
{ | ||
"protected": "eyJhbGciOiJFUzI1NiJ9", |
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 would have expected a "kid" parameter - not just "alg".
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.
not sure what you mean, see an header example (with kid and alg) below
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.
eyJhbGciOiJFUzI1NiJ9
decodes to {"alg":"ES256"}
, which has no kid
.
Co-authored-by: Christian Bormann <[email protected]>
…penID4VP into multiple-rp-credentials
Discussion in the WG meeting on the 14th of Jan:
|
I don't think this is possible. I'm leaning towards removing the |
Can you please explain why this is not possible? We can add a "credentials" array to every client identifier specific "signature" object and use that to link the Client Identifier. |
That would mean the audience restriction would differ depending on the "transport". I would assume that makes implementations (at least in transition from traditional protocol to DC) harder. It also changes the level on which the restriction needs to be checked (client id/application vs. origin/transport). |
That's already the reality for ISO mdoc format. I don't think it makes it much more complicated, because the input for credential formats, so mdoc and SD-JWT implementations don't need to change. The protocol implementations change anyway when people add DC-API, the change is how to build session_transcript/aud and that's what you stuff into the credential format library. |
Doesn't the device response contain the client id? |
Not sure what you mean with device response. client_id is present at two places in ISO18013-7
The first is removed with DC-API and the second may be as well following my discussion line here in this thread. and imo that makes sense, the client_id is kind of an origin equivalent, because OAuth couldn't achieve this without OS support. |
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 can approve this PR because it enables an important flexibility in how a wallet evaluates the trust with an RP.
I have read and I found valuable the comments of other contributors, I have decided to approve it in the current status anyway to facilitate the process.
Regarding the question: would we be able to support both compact and json serialization, I want to answer that it would be in the scope of any good library detect and support both the formats, in full support of RFC 7515. I would not use openid4vp to limit the use of the serialization types (impl profile could decide this, if/when required).
Not sure what you mean, the client identifier is either present in the request (signed request) or determined from the origin (unsigned request).
They are different (in case of signed request) and the client identifier is more of a logical identifier that could be used from different origins. |
Co-authored-by: Giuseppe De Marco <[email protected]>
Co-authored-by: Brian Campbell <[email protected]>
Co-authored-by: Kristina <[email protected]>
Co-authored-by: Kristina <[email protected]>
Co-authored-by: Kristina <[email protected]>
### client_id | ||
|
||
* Header Parameter Name: `client_id` | ||
* Header Parameter Description: This header contains a Client Identifier. |
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.
* Header Parameter Description: This header contains a Client Identifier. | |
* Header Parameter Description: This header contains a Client Identifier. A Client Identifier is used in OAuth to identify a certain client. It is defined in [@!RFC6749], section 2.2. |
{ | ||
"alg": "ES256", | ||
"x5c": [ | ||
"MIICOjCCAeG...djzH7lA==", | ||
"MIICLTCCAdS...koAmhWVKe" | ||
], | ||
"client_id": "x509_san_dns:rp.example.com" | ||
} |
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.
specific proposal for disambiguating credential requested and credential_id
{ | |
"alg": "ES256", | |
"x5c": [ | |
"MIICOjCCAeG...djzH7lA==", | |
"MIICLTCCAdS...koAmhWVKe" | |
], | |
"client_id": "x509_san_dns:rp.example.com" | |
} | |
{ | |
"alg": "ES256", | |
"x5c": [ | |
"MIICOjCCAeG...djzH7lA==", | |
"MIICLTCCAdS...koAmhWVKe" | |
], | |
"client_id": "x509_san_dns:rp.example.com", | |
"credential_ids": [ <Array of strings each referencing a Credential requested by the Verifier> ] | |
} |
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.
credential_ids: REQUIRED. Array of strings each referencing a Credential requested by the Verifier that can be used in session transcript. In [DIF.PresentationExchange], the string matches the id field in the Input Descriptor. In the Digital Credentials Query Language, the string matches the id field in the Credential Query. If there is more than one element in the array, the Wallet MUST use only one of the referenced Credentials for session transcript
credential_id and client_id has to be 1:1 mapping. so that RP can determine a client_id based on the credential_id
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.
alternative proposal:
wallet puts used client_id in the response next to the credential:
{
"my_credential": {
“payload”: “eyJhbGci...QMA”,
“client_id”: “asdf”
}
}
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.
what prevents MITM replaying the request RP sent through the DC API:
- for unsigned requests, origin is in the returned credential because origin == client_id
- for signed request, expected_origins is in the signed request that MITM cannot modify
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.
what prevents MITM replaying the request RP sent through the DC API:
for unsigned requests, origin is in the returned credential because origin == client_id
for signed request, expected_origins is in the signed request that MITM cannot modify
Thank you for this summary and apologies for struggling with the concept during the most recent DCP call. One nit just for posterity is that for unsigned request the client_id contains the origin and is prefixed (it doesn't strictly equal origin).
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.
How about adding the client_id
as an optional parameter (array of strings) to the Credential Query? That would simplify things quite a bit imho for the request:
- If client_id is not present in the Credential Query: you use only one client_id in the request and nothing changes to the status quo (without multi-rp)
- If client_id is present, the wallet can directly match which client ids are possible for each requested credential and choose accordingly
The DCQL vp_token
gets an optional parameter client_id
as well that is only included if more than one client_id was provided in the request (the DCQL query) and not present otherwise --> nothing changes for normal flows.
That way nothing changes for simple flows, but mult-rp flows extend the normal data structures without any unnecessary duplication unless I am missing something.
WG discussion notes
arguments to go the second way:
proposal and further discussion documented https://github.com/openid/OpenID4VP/pull/308/files#r1927297499 |
We don't have another DCP WG mtg before ISO virtual mtg next week. Having heard DCP WG discussion and having re-read ISO requirement, what I would like to offer as a way forward is to explain at the ISO virtual mtg that we are addressing ISO requirement of “mdoc authentication must be bound to the origin” not by including origin in the session transcript but by what is described below. If this gets accepted, we can remove origin from session transcript as an option, and conclude this circular discussion we have been having:
|
Would it be better to rephrase the first bullet point to for unsigned requests, the resulting credential is returned to the origin, because origin == client_id |
Use Case: As a Verifier I want to request mDLs to verify KYC, and I accept mDLs issued in the EU, US. US uses web PKI This seems like it would be a problem, unless we allow a mechanism to know to use the origin for particular returned credentials. (A US wallet may not be able to validate an EU signature, and so can't verify the signature over expected_origins) |
…penID4VP into multiple-rp-credentials
The question is whether the RP should be able to explicitly send an unsigned message or whether there should be a special logic to always try Web PKI in addition to the signed request. |
This PR changes the way requests are signed to the Digital Credentials API from compact serialization (single RP) to JWS JSON Serialization (Multiple RPs).
Questions to the WG:
resolves #248