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

fix: [RP] same device uri and cross device security and implementation considerations #87

Closed
wants to merge 1 commit into from

Conversation

peppelinux
Copy link
Member

@peppelinux peppelinux commented Jul 31, 2023

this PR brings the URI used in the Cross Device flow also in the Same Device flow

it also appends in the non-normative example of the request URI a unique-session-id webpath

In a Cross Device Flow the user-agent uses the JS (with WS or polling) made available in the QRCode Page to check if the authentication is successfully. For doing that it has to request to a specific URL (and also be authenticated with a session cookie that binds the unique-session-identifier, to avoid adversary to be able to made hijacks).

This opens a interesting scenario where the cross device flow is "animated" within the desktop user-agent with three different steps

  1. The RP shows the QRcode (and periodically checks the authentication status in a time window of maximum 5 minutes)
    • the user frames the qrcode using their wallet instance
    • the wallet instance extract the request_uri and issues the HTTP request to obtain the signed request
    • The RP gives the signed_request (and update the session status)
  2. The JS executed by the desktop user-agent receives a response containing a status code 201(Content Created) and optionally displaying a message to the user like "The Wallet Instance is processing the request" . Then continue polling to the unique-session-id endpoint
    • The User accepts the request using their wallet instance, selects the required data to be released, then issue the presentation response to the RP
    • The RP authenticates the user (and update the session status)
  3. The JS executed by the desktop user-agent receives a response with a JSON containing a status code 200 (OK), having requested with the session cookie, the session bound within it is then updated, allowing the RP (JS) to redirect the User to the previous resource requested, before the authentication (next= parameter or whatever)

In absence of a session-cookie linked to a cross device session-id the flow may be hijacked by an adversary that by stolen the sole session-id could authenticate using another user-agent

@peppelinux
Copy link
Member Author

Another important consideration is that the request_uri MUST NOT then be included in the metadata, since it may be extended with unique session identifiers

Is there the same assumption for the redirect_uri?
Let's see ...

@balanza
Copy link
Contributor

balanza commented Aug 1, 2023

authentication status

Which authentication are you referring to?

Copy link
Contributor

@balanza balanza left a comment

Choose a reason for hiding this comment

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

For the sake of taking a thoughtful decision, I'd highlight:

  • what are the security benefit of this choice?
  • What are the implications?

Below follows its Base64 decoded content:

.. code-block:: text
eudiw://authorize?client_id=https://verifier.example.org&request_uri=https://verifier.example.org/request_uri
eudiw://authorize?client_id=https://verifier.example.org&request_uri=https://verifier.example.org/request_uri/$unique-session-identifier
Copy link
Contributor

Choose a reason for hiding this comment

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

As I'm working on this every day, I understood what $unique-session-identifier represents and where such value comes from. However, from the perspective of a new reader, I'm afraid it might look like a reference to an actual value mentioned somewhere else.

Would we spend a couple of lines on this value, explaining the meaning and the expected usage from the RP?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can do that once the discussion in the introduction of this PR find an agreement

once we decide that's the way to go, this PR will have a section with the security and implementation considerations

@peppelinux
Copy link
Member Author

authentication status

Which authentication are you referring to?

the User authentication via Wallet

what are the security benefit of this choice?

the user-agent needs to be redirected on the protected resource after the authentication happens. Otherwise the wallet made the authentication and the web page remains stucked on the qrcode page. So we have a UX requirement more than others.

What are the implications?

request_uri cannot be used in the metadata, since every qrcode and every request_uri will be unique and one-time-use.

@balanza
Copy link
Contributor

balanza commented Aug 1, 2023

Thanks for the explanation, I think I better understood what is this PR purpose. As far as my concerns, I see the following open points:

  1. (nitpick): As the PR addresses a UX issue, I'd change the security mention in the title (and the label)
  2. (issue): Being request_uri non-static so that it cannot be included in the entity configuration as it is, we either:
    a. remove request_uri from RP's entity configuration, as not needed anymore
    b. explain how both request_uri attributes from entity configuration and QR code payload are related if they are.
  3. (issue): Using deep-linking for same-device interaction is problematic as pointed out in the issue [RP] Same Device Flow concerns #81; I advocate we first resolve that conversation so that our options and alternatives are analyzed, and we can come up with a solid solution and documented trade-offs.

What do you think?

@peppelinux
Copy link
Member Author

@balanza

  1. added labels
  2. I'd like to discuss if we want to define a new endpoint for the request status instead of using the request_uri for multiple scopes. This allows us to keep request_uri in the metadata and a specialized (and more secure and scalable) endpoint for the status checking and UX (this would make normative the QRCode implementation in our profile with conseguences in EUDI Wallet)
  3. I fully agree with you, while for the scope of the PoC we really need to find a compromise even if temporary

@balanza
Copy link
Contributor

balanza commented Aug 1, 2023

  1. I'm sorry, English is not my native language and I may have explained myself poorly. I try to express myself better.
    As this PR doesn't address a security issue but a UX one, I think is not appropriate to name it cross device security and to add the label security; I'd use the UX word instead.

  2. I think your initial solution is fine and it solves a real problem; I just noted that we should make the whole documentation consistent so that we don't have a page that tells something and another that tells the opposite. I thought options a or b could be sufficient for that, but any other idea can be fine as well.

  3. I'm very keen on compromises as long as they are motivated and documented. What are actually these compromises for? What does the current definition impede the PoC to do? How those impediments are solved by the proposed changes?
    For the sake of the compromise we should be able to explain the taken trade-offs, and how/if they are temporary; I think we can discuss them in this PR.
    More general problems about Wallet deep-linking, that do not impact the PoC, can be discussed in the dedicated issue.

@peppelinux peppelinux marked this pull request as draft August 1, 2023 21:03
@peppelinux
Copy link
Member Author

@balanza the PR is now converted to a draft, since the decisions on this PR will produce the security/implementation considerations.

I'm changing my mind day after day:
We may leave the static request_uri as it was and add a RP custom endpoint that's called from the qrcode page (internal API within the RP architecture)

I've realized that because:

  1. the request_uri shold have a single and specific scope -> giving a signed request in the form or JWS
  2. the request status should be served in a specialized endpoint that's out from the specs but since it is not normative, however, at the same time, the specs gives those consideration about implementation an security, using a non normative example

the security consideration is that:
we can't rely just on a nonce/state lookup parameter for get a user-agent authenticated, we need to do a cross check with those nonce/state with a session cookie, so that just the user-agent that has obtained that request_uri is enabled to check the request status using as parameter the nonce/state (or just the cookie?)

with this nth proposal we can get back the static request_uri (but not a static signed request object, since the state and the nonce must change)

@balanza
Copy link
Contributor

balanza commented Aug 2, 2023

@peppelinux let's break the proposal:

The problem is well-defined: the RP must be aware of the status of each user authentication request, so that it can "refresh" the access page.

I like your proposal of differentiating the QR for each interaction (session?). I think we can agree on that.

What does the RP need? A value that is the same both on its frontend and in the QR payload so that RP's backend can correlate an instance of access page with an incoming API user request from the Wallet Instance. How RP handles such correlation (cookies, browser local storage, whatever) is a detail that I think is far beyond our scope of the protocol definition, and we must make no assumptions about that.

Are you fine with this statement?

Now let's find a form that is both elegant, safe, and easy to implement. Some options are:

  1. in the QR payload, use a request_uri specific for each request (your initial proposal)
    a. 🚧 we have to define a fixed format like $RP_Entity_Configuration.request_uri/$unique-session-identifier, so the Wallet Instance can evaluate trust by comparing QR's request_uri with RP Entity Configuration's request_uri (it's just a sub-path, in this example)
    b. ✅ RP will receive the same value $unique-session-identifier both from the page that rendered the QR code, and the incoming request from the Wallet Instance. It can correlate them

  2. in the QR payload, add a unique-session-identifier field with such value
    a. ✅ both QR's request_uri and RP Entity Configuration's request_uri are the same, so we do not need to review our security decisions on the trust model
    b. 🚧 we have to require such a field in the endpoint for obtaining the Request Object
    c. ✅ RP will receive the same value $unique-session-identifier both from the page that rendered the QR code, and the incoming request from the Wallet Instance. It can correlate them

(🚧=decision to take)

@grausof
Copy link
Collaborator

grausof commented Aug 2, 2023

Regarding the $unique-session-identifier I agree with @balanza on option 1 which was the original one. The verification of the endpoint can always be done via the entity configuration, just go and verify the base path without the $unique-session-identifier.

I'll give an example, if the request_uri is like this:
https://verifier.example.org/request_uri/AAABBBBBCCC0001

within the metadata it is however possible to include a request_uri of the type:
https://verifier.example.org/request_uri

It will then be specified in the technical rules that $unique-session-identifier must be replaced with the unique id.

@peppelinux
Copy link
Member Author

@grausof unfortunately your proposal is out of standard, since a value in the metadata doesn't have placeholder or other elements that can be replaced

please take my last proposal, where redirect_uri remains as it is in the current specs without any additional prefix (even if recommended by JAR)

the RP internal architecture then has a custom endpoint that is used by the JS executed in the QRCode page to check the status of the request

this is a security and implementative consideration, not normative, since every RP can implement it's internal api and resource whatever they wants, they just have to use the minimum requirements for interop. The security consideration is then needed, but a consideration is not normative.

@balanza
Copy link
Contributor

balanza commented Aug 2, 2023

@grausof unfortunately your proposal is out of standard, since a value in the metadata doesn't have placeholder or other elements that can be replaced

This is option 1 on my last comment and it doesn't imply placeholders in metadata. Here is an example:

  • in the Entity Configuration metadata:
...
"request_uri": "https://verifier.example.com/request_uri"
...
  • in the QR payload:
eudiw://authorize?client_id=https://verifier.example.org&request_uri=https://verifier.example.org/request_uri/abcd1234mysessionid

All you need to do is confirm that the URL in the QR is a subpath of the URL in metadata.


Have you considered option 2? At the extent of an extra query string field, we keep adherence to OIDC Federation standards regarding request_uri.

@peppelinux
Copy link
Member Author

peppelinux commented Aug 2, 2023

@balanza

request_uri=https://verifier.example.org/request_uri/abcd1234mysessionid != request_uri=https://verifier.example.org/request_uri

while the one contained in the metadata must be respected. We can take a decision looking also to JAR recommendation but for now I'm saying that we just need a specialized endpoint to check the session status and then authenticate the user-agent (cookie) and give the redirect to the protected resource

for this reason, I'm going to remove the $session-unique-id from this PR and add a security/implementation consideration note once @fmarino-ipzs give review

regarding the JAR recommandation to randomize the request_uri this makes sense if we remove the request_uri from the metadata and add normative language about the mandate to check that the FQDN contained in the URI MUST be the same of the one contained in the client_id

if ok for you I'll proceed, I look forward for your kind reply

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants