openid / OpenID4VP

56 stars 20 forks source link

Address client_id_scheme problems (fixes #124) #237

Closed awoie closed 1 month ago

awoie commented 2 months ago

This PR is still WIP.

This PR removes client_id_scheme by trying to crystalize the common features of existing client ID schemes (authentication, and validating redirect/response_uris) on top of existing client metadata retrieval mechanisms such as DCR, OpenID Federation or the new client_metadata parameter.

I figured that an activity diagram might help framing the discussion on Thursday's DCP WG call (thanks @javereec for this idea).

NOTE: a wallet does not have to support each method. I will make this more clear.

javereec commented 2 months ago

Below is a diagram to potentially facilitate the discussion on how to verify these requests.

flowchart TD

    RS{Request Signed?} -->|Yes| URI{Client ID URI?}

    URI --> |Yes| X509
    URI --> |No| PRE

    X509{x5c,x5t header?} --> |Yes| X509A[X.509 Certificates]
    X509{x5c,x5t header?} --> |No| FED

    FED{Client ID URL?} --> |Yes| FEDA[OpenID Federation]
    FED{Client ID URL?} --> |No| DID

    DID{Client ID DID?} --> |Yes| DIDA[DID Resolution]
    DID{Client ID DID?} --> |No| PRE

    RS{Request Signed?} -->|No| PRE
    PRE{Client ID Pre-Registered?} --> |No| POL[Redirect URI = Client ID<br>Wallet Policy decision]
    PRE{Client ID Pre-Registered?} --> |Yes| PREA[Pre Registered <br>RFC6749]
danielfett commented 2 months ago

For completeness, can you please expand the X.509 Certificates item?

tlodderstedt commented 2 months ago

@awoie can you please remove all occurrences of client_id_scheme? It feels like not all parts of the processing rules are adopted yet.

danielfett commented 2 months ago

As discussed on the call, the PR in its current form does not address how to process a request (if more than one method is supported). Clearly defined processing rules that ensure that a request can never be modified by a third party such that the signature verification can be skipped or a signature can be replaced are the core of the problem in my opinion.

awoie commented 2 months ago

As discussed on the call, the PR in its current form does not address how to process a request (if more than one method is supported). Clearly defined processing rules that ensure that a request can never be modified by a third party such that the signature verification can be skipped or a signature can be replaced are the core of the problem in my opinion.

@danielfett Could you provide an example of a downgrade attack based on the existing language?

For signed requests, the request parameters are integrity protected:

For unsigned requests, the request parameters are either fetched from an authoritative source (pre-registered, perhaps with require_signed_request_object not enabled), or the client_id must match the redirect_uri or response_uri for simpler requests. In the latter case, a downgrade attack wouldn't make sense. To downgrade signed to unsigned, we could even require that for these in-band registrations (DID, X509, verifier attestation), client_id != redirect_uri.

Browser API is a different conversation.

A wallet would only enforce one of these methods if they were considered to be secure or even equally secure. The same applies to the verifier.

nemqe commented 2 months ago

Just a note regarding DIDs and the comment about linkage to redirect_uri/response_uri.

Signed by DIDs that match the client_id, though there's no linkage to the redirect_uri or response_uri (which I believe is a potential issue).

For this I think it could be a mandate that linkage credentials need to be used (linked domains) so you have a direct link between the did-diddoc-domain.

awoie commented 2 months ago

Just a note regarding DIDs and the comment about linkage to redirect_uri/response_uri.

Signed by DIDs that match the client_id, though there's no linkage to the redirect_uri or response_uri (which I believe is a potential issue).

For this I think it could be a mandate that linkage credentials need to be used (linked domains) so you have a direct link between the did-diddoc-domain.

IMO, it is also questionable if this linkage is required for in-band request object authentication, i.e., where the client_id is authenticated by things included in the request object. Because if the request object is trusted to originate from the client_id, the redirect_uri/response_uri might NOT have to be linked to other things such as X.509 SAN(s), DID linked domain, or redirect_uris in the verifier attestation but this is separate discussion.

awoie commented 2 months ago

Perhaps a better visualization of the PR which shows that all of the request object authentication methods don't have to be applied in a particular order. The diagram below does not include redirect/response_uri validation yet but the bullet points above show how this can be done. Perhaps in-band registration is the better term for x.509/DID/verifier attestation where the request object is authenticated against the client ID using parameters that are provided in band and additional client metadata is typically provided using the client_metadata parameter.

flowchart TD
    start --> pre
    start --> fed
    start --> inband

    pre[Pre-registered] -->|client_id is pre-registered| prev
    fed[OpenID Federation] -->|client_id is OpenID Federation Entity ID,<br>i.e., URI with OpenID Fed well-known| fedv[Verify signature using<br>resolved OpenID Fed metadata] --> con[Continue]

    inband[Inband registration] -->IS{is request signed?}-->|No| usv
    IS -->|Yes|II{client_id matches<br>redirect/response_uri?} -->|Yes| rej[Reject]
    II -->|No| inbandss[Determine verification key]
    inbandss -->|x5c, x5t in JAR| x509v[Verify signature using<br>corresponding certificate] --> con
    inbandss -->|jwt/verifier attestation in JAR| vav[Verify signature using<br>cnf claim] --> con
    inbandss -->|client_id is a DID| didv[Verify signature<br>using key from DID Document] --> con

    prev[Fetch client metadata] --> B{Is request<br>signed?} -->|Yes| C[Verify signature using<br>jwks from client metadata] --> con
    B -->|No| D{Is require<br>request object<br>signing set?} -->|Yes| con
    D -->|No| rej

    usv{client_id matches<br>redirect/response_uri?} -->|Yes| con
javereec commented 2 months ago

@awoie regarding the diagram.

When the request is not signed and client_id = redirect_uri/response_uri I think we must continue, not reject.

When a request comes in, I think we can be more clear about what we need to evaluate first. Is this the client_id or wether the request is signed or not. The entry point is not quite clear.

awoie commented 2 months ago

@awoie regarding the diagram.

When the request is not signed and client_id = redirect_uri/response_uri I think we must continue, not reject.

Indeed, that was just a bug in the diagram. I fixed it.

awoie commented 2 months ago

@awoie regarding the diagram.

When a request comes in, I think we can be more clear about what we need to evaluate first. Is this the client_id or wether the request is signed or not. The entry point is not quite clear.

IMO, this is a wallet policy since it is also a wallet policy which methods to support at all. It should make no difference if we prescribed a specific order or not unless I'm missing something.

danielfett commented 2 months ago

To reiterate what I said on the call, the current PR does not ensure that

This is an extremely critical part of the spec and therefore we need to ensure to get it right.

awoie commented 2 months ago

To reiterate what I said on the call, the current PR does not ensure that

  • the verifier's expectation on what was actually checked when it receives a presentation with its client id in the "aud" claim matches what the wallet checked, and

This can only be guaranteed if the public verification key is included in the VP, e.g., aud=<jwk-thumbprint>:<client_id>. This would also mitigate the concern that even within an existing client ID scheme, e.g., x509, verifier attestation etc., and if the client ID scheme was included in the response, the verifier does not know which credential (certificate, or verifier attestation etc.) was used to authenticate the client ID if a verifier supports multiple in-band client credentials of the same type but adhering to different security levels or ecosystem requirements. Note that it is possible that a verifier obtains multiple credentials for the same client ID.

  • two implementations of this draft that, for example, allow x509 and pre-registered clients, behave the same way and do the same checks.

This is a performance concern, not a security concern, if we consider my comment above. We can suggest an order in the PR to guide wallets.

awoie commented 2 months ago

This is a performance concern, not a security concern, if we consider my comment above. We can suggest an order in the PR to guide wallets.

For example, we could suggest the following order if performance is a concern.

If the request is signed:

If the request is not signed:

If the request was received via the browser API:

jogu commented 1 month ago

I think we've achieved consensus around https://github.com/openid/OpenID4VP/pull/266 so I think this PR is no longer needed and hence closing it to clean up the PR list - many thanks go to Oliver/Tobias/etc for help moving the conversation along!