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

Need to define token validation steps for the RP #318

Closed
kdenhartog opened this issue Jul 21, 2022 · 13 comments · Fixed by #391
Closed

Need to define token validation steps for the RP #318

kdenhartog opened this issue Jul 21, 2022 · 13 comments · Fixed by #391

Comments

@kdenhartog
Copy link
Contributor

Right now the spec doesn't specifically mention the token validation steps for the RP very well. This could be very small and reference just a single set of claims that the spec defines and highly reference the JWT spec. Alternatively, this could be defined in a way that focuses primarily on the baseline requirements now and allow for an extension point that leaves space for the VC Data model to slot in as an extension to the token data model defined.

@npm1
Copy link
Collaborator

npm1 commented Jul 21, 2022

Those steps are not mentioned because that is not something that this API provides. How validation would work probably varies a bit depending on the IDP, but in any case that seems a bit out of scope of the spec. We could add a Note saying that the RP should perform some validation once it receives the token, WDYT?

@kdenhartog
Copy link
Contributor Author

So is section 5 and section 6 non-normative definitions? There's attributes in there that read as if they're being normatively required at the moment which doesn't seem to align with what the CG is expecting vs what the spec is saying. I think that's where the confusion comes for me when reading the spec is that the spec reads as if there's an intention to define RP/IDP behavior. I think it's fine to leave this out of scope, but then it should read more as if it's extending OIDC/SAML and adhering strongly to their token structures to render the necessary browser UI rather than at this point where it reads as if a new protocol is being defined.

We could add a Note saying that the RP should perform some validation once it receives the token

That seems like something that needs to be normatively defined either here or referenced in another spec. Otherwise it's likely going to lead to authorization bypass vulnerabilities.

@npm1
Copy link
Collaborator

npm1 commented Jul 22, 2022

I think this could be clearer, but sections 5 and 6 introduce endpoints which are actually used by the APIs defined in section 7. The section 5 endpoints are used when the RP calls get() whereas the section 6 endpoint is used within logoutRPs(). I could even see us removing section 6, as it is not really necessary: it just explains that there is an endpoint that the IDP can use to logout from the RP, and this endpoint is explained there only because it is used on section 7. Hope that makes sense?

@kdenhartog
Copy link
Contributor Author

Ahh ok, that makes more sense so these are underlying APIs that need to be called inside the logic of the browser. I'll spend a bit more time thinking about this to see if I can help make this a bit more clearer now to help clear up how an implementer of an RP/IDP should handle these since it's something they still want to consider, but isn't necessarily something that fits within the scope of this document now that I understand the intent here better.

@kdenhartog
Copy link
Contributor Author

Coming back to this issue to see if I can clean it up and drive it to completion. Can someone clarify if the intent is to keep the tokens opaque browser side still? That was my understanding last time I was looking through it and would help me to figure out what action (if any) would be necessary to close this one out.

cc @timcappalli who I believe has been involved in this specific discussion about how tokens should be treated by the browser

@achimschloss
Copy link
Contributor

This is also my understanding and stated in the spec https://fedidcg.github.io/FedCM/#idp-api-id-assertion-endpoint

The content of the token is opaque to the user agent and can contain anything that the Identity Provider would like to pass to the Relying Party to facilitate the login

@npm1
Copy link
Collaborator

npm1 commented Jan 10, 2023

Yep, that is correct. The user agent only passes the token (it does not try to parse it in any way).

@kdenhartog
Copy link
Contributor Author

kdenhartog commented Jan 11, 2023

Ok, in that case I just opened a PR to address this one. Thank you for clarifying and pointing me to the right place @asr-enid and @npm1!

@pradtke
Copy link

pradtke commented Jan 12, 2023

There is another section mentioning the token is a jwt. https://fedidcg.github.io/FedCM/#browser-api

If all goes well, the Relying Party receives back an IdentityCredential which contains a token in the form of a signed JWT which it can use to authenticate the user.

@kdenhartog
Copy link
Contributor Author

My understanding is that this will not always be the case though @pradtke. While JWTs are more likely to be the most common form of token used, OAuth intentionally doesn't specify token formats which allows it to be a variety of different formats. In the case of OIDC, they do specify it will be a JWT.

I believe this API is intentionally designed so that things like SAML is still usable in which case I believe the SAML assertion would be the opaque token used. In this case, we wouldn't want to specify that the browser should be validating the token then before it stores or forwards it to the RP because we can't be certain of the format. That's why I wanted to verify we were still making the assumption of having opaque tokens.

@pradtke
Copy link

pradtke commented Jan 12, 2023

@kdenhartog Sorry, my initial comment was poorly worded. I agree with 100% and am looking through the spec from the SAML perspective. What I was trying to convey with my comment, assuming this issue was to clarify the documentation, was that section 7 should remove mention of the JWT. E.g. change from which contains a in the form of a signed JWT to which contains an opaque (to the browser) token. You mentioned working on a documentation PR, so I was lazily and passively suggesting a second place to change :) . I can of course make a separate PR if you think it's distinct (e.g what an RP should do vs what the browser should ignore)

@kdenhartog
Copy link
Contributor Author

Ahh ok yeah that makes sense what you're requesting now. Hmm WDYT @samuelgoto @npm1 and @asr-enid? I believe you've done the most work on the spec to date.

@npm1
Copy link
Collaborator

npm1 commented Jan 13, 2023

Personally I'm fine changing the wording, yea. We can remove the JWT mention altogether or mention it as an example.

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

Successfully merging a pull request may close this issue.

4 participants