w3c-fedid / FedCM

A privacy preserving identity exchange Web API
https://w3c-fedid.github.io/FedCM/
Other
375 stars 72 forks source link

Need to define token validation steps for the RP #318

Closed kdenhartog closed 1 year ago

kdenhartog commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

kdenhartog commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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