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

Add tokens and scopes. #208

Closed
wants to merge 1 commit into from
Closed

Add tokens and scopes. #208

wants to merge 1 commit into from

Conversation

dj2
Copy link
Collaborator

@dj2 dj2 commented Feb 23, 2022

This PR adds the concept of tokens and scopes into the login request.


Preview | Diff

This PR adds the concept of tokens and scopes into the `login` request.
@dj2 dj2 added the design label Feb 23, 2022
@dj2 dj2 requested a review from samuelgoto February 23, 2022 20:15
@dj2 dj2 self-assigned this Feb 23, 2022
@@ -608,8 +608,8 @@ The file is parsed expecting the following properties:
:: A URL that points to an HTTP API that complies with the [[#idp-api-accounts-endpoint]] API.
: <dfn>client_metadata_endpoint</dfn> (required)
:: A URL that points to an HTTP API that complies with the [[#idp-api-client-id-metadata-endpoint]] API.
: <dfn>id_token_endpoint</dfn> (required)
:: A URL that points to an HTTP API that complies with the [[#idp-api-id-token-endpoint]] API.
: <dfn>token_endpoint</dfn> (required)
Copy link
Collaborator

Choose a reason for hiding this comment

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

A couple of points:

(a) this is going to be backwards incompatible ... which isn't great and
(b) it would be useful to somewhat get inspiration on terminology / requests / responses / etc from the OIDC token endpoint

It would be interesting to find a formulation in which (b) is 100% backwards compatible to the OIDC endpoint, because then we could re-use their existing deployments. I don't think it is a mandatory / hard requirement, but if everything fits into that model, it wouldn't be bad to reuse the existing deployment.

Also, would be useful to anticipate the kinds of security affordances that were added to that endpoint so that we don't run into the same walls.

There is also the authorization endpoint which I don't think I quite understand the distinction between that and the token endpoint, but worth pointing out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Backwards incompatible with what? FedCM or OIDC?

Copy link

Choose a reason for hiding this comment

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

The authorization endpoint in OAuth https://www.rfc-editor.org/rfc/rfc6749.html#section-3.1 is where the client sends the user for interactive authentication and approval of the requested access.

The token endpoint in OAuth https://www.rfc-editor.org/rfc/rfc6749.html#section-3.2 is used directly by the client to get tokens - typically by presenting an authorization code obtained from the authorization endpoint or a refresh token.

Copy link

Choose a reason for hiding this comment

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

Which is to say that, while I don't know that backwards compatible is even possible here, it would be nice if different terms were used for things which are different than established things in these established protocols.

Copy link

@will-bartlett will-bartlett Mar 15, 2022

Choose a reason for hiding this comment

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

The authorization endpoint is used for cases where the user needs to interact with the IDP (in a browser window). If I understand your scoping correctly (https://github.com/fedidcg/FedCM/blob/main/explainer.md), you want to exclude such interactive requests currently (under "out of scope", "Sign into the IDP"). There are two main cases where the user might need to interact with the IDP: 1) initial sign-in to the IDP 2) additional authentication required (e.g. TFA). As a concrete example of additional authentication required, Microsoft requires any sign-in (e.g. password) to sync the browser history in Microsoft Edge. However, to access the OneDrive personal vault, the sign-in must include TFA. I think excluding interactive sign-in (and thus the authorization endpoint) is reasonable - you may want to add "additional authentication required" as an out of scope case in the explainer as well.

Connecting to OAuth's token endpoint. Microsoft defines an artifact called the primary refresh token. It's similar to an OAuth refresh token, but useable for all clients, not just one clients, because it is owned by the IDP. Here, you've effectively done the same thing, but with the IDP cookies.

Consider if you start from an OAuth refresh token request that would be valid for a public client under RFC 6749 Section 6:

  POST /token HTTP/1.1
  Host: server.example.com
  Content-Type: application/x-www-form-urlencoded

  grant_type=refresh_token&refresh_token=tGzv3JOkF0XG5Qx2TlKWIA
  &client_id=s6BhdRkqt3&scope=profile_short

You move the refresh_token from form data to cookie, and treat the cookie as if it is valid for all clients, not just one client. That gets you to something like:

  POST /token HTTP/1.1
  Host: server.example.com
  Content-Type: application/x-www-form-urlencoded
  **Cookie: tGzv3JOkF0XG5Qx2TlKWIA**

  grant_type=**cookie**&client_id=s6BhdRkqt3&scope=profile_short

If you drop grant_type and switch from /token to /tokens.php, and add the security header, you have a FedCM request:

  POST **/tokens.php** HTTP/1.1
  Host: server.example.com
  Content-Type: application/x-www-form-urlencoded
  **Cookie: tGzv3JOkF0XG5Qx2TlKWIA**
  Sec-FedCM-CSRF: ?1

  client_id=s6BhdRkqt3&scope=profile_short

So, to the extent that you want to make this more OAuth-y (which I tend to think is a good thing), I think the only change you would really need to make is to add a grant_type parameter and to ensure you carefully follow OAuth semantics (e.g. returning unknown response parameters, not interpreting request scope parameter, etc.).

@will-bartlett
Copy link

will-bartlett commented Mar 15, 2022

I'm a bit unsure of the need to even enumerate access_token and refresh_token explicitly - these are just names, and in the context of FedCM, they seem to carry little semantics. Which is to say - if an IDP were to return an access_token as a refresh_token or a refresh_token as an access_token, or an access_token inside the id_token - the API would function the same. The names are semantic-less.

It seems like the spec is looking for the following attributes:

  1. An id_token is mandatory, a JWT, and contains the OpenID Connect minimal claimset for an id_token.
  2. The browser can distinguish a minimally-privacy-revealing request from an unknown-privacy-revealing request - and show a scarier warning in the later case. "The |tokens| and |scopes| MAY affect the dialog presented to the user to makes
    clear that more power is being granted in the communication."

The first makes sense for interoperability.

The second, I have doubts about, captured in #83 . Effectively, if the IDP can hide 128 bits of data anywhere, the IDP can be perfectly privacy non-preserving. I believe the IDP can already hide 128 bits of data in an id_token and so distinguishing requests as "less-privacy-preserving" because they use access_token or refresh_token is mostly hurting well-behaved companies, while not impeding bad actors.

There is also a body of existing practice of the scope parameter indicating the response token type. In particular, OpenID Connect defines that the "openid" scope requests that an additional token (the id_token) be returned and that the "offline_access" scope requests that an additional token (the refresh_token) be returned.

With all that in mind, my recommendation would be to drop the "tokens" parameter, drop the accessToken and refreshToken members from FederatedTokens response object, rename "FederatedTokens" to "FederatedTokenResponse" (to indicate that it can contain either token or non-token content) and simply state that the entire JSON response (including unknown parameters) is serialized into a javascript object that must match the FederatedTokenResponse interface. Relying parties can continue to indicate additional tokens with the scope parameter, as per their existing practice. And, if the browser wants to make distinctions about the privacy or requests, the browser can distinguish id_token vs id_token+opaquedata - which I think is the only meaningful technical distinction anyway.

I think recommendation this follows the OAuth pattern. Across the ecosystem, there are various extensions where IDPs can indicate the lifetime of a refresh token - some IDPs (Microsoft) use "refresh_token_expires_in":7776000, others use "refresh_token_expires_on", "rt_expiry", "rt_expires", "rt_expires_in", etc. OAuth made access_token mandatory and basically made everything else optional and out-of-scope - which gave the Identity ecosystem freedom to grow and experiment. Taking a similar choice here (making id_token mandatory and making everything else optional and out-of-scope) might make sense. Just scanning Microsoft's OAuth implementation, we have optional response parameters for geographic affinity, optional response parameters for extended duration resiliency during outages, optional response parameters for cache keys to improve client caching - it'd be nice to strike a balance where this stuff "just works", but doesn't need to be covered in the spec.

Edit: as an aside - OAuth says that request scopes != response scopes. Specifically, it says:

The authorization server MAY fully or partially ignore the scope requested by the client, based on the authorization server policy or the resource owner's instructions. If the issued access token scope is different from the one requested by the client, the authorization server MUST include the "scope" response parameter to inform the client of the actual scope granted."

Along the lines of what @dj2 says in #154 , IDPs generally need the ability to add extra scopes that aren't present in the request and for any prompt to the user that mentions scopes to show the same e.g. because governments pass new laws that require additional or more explicit approvals. In such cases, IDPs want to add the new prompts to the consent interaction, but not require the RP to make updates. So I'm dubious of the browser interpreting the scope request param in this way.

@will-bartlett
Copy link

will-bartlett commented Mar 15, 2022

A bit of a nit, but scope should be used in the singular here, consistent with RFC6749. The use of scope is similar to "scope of work" for a project. You might say "the scope of the project that we will complete in the first quarter includes text, audio, and video", but you wouldn't say "the scopes of the project that we will complete in the first quarter are text, audio, and video" - similarly, we write scope=text+audio+video, not scopes=text+audio+video. The individual items that are space delimited in the scope parameter are called "permissions" by Microsoft and called "strings" and/or "values" by RFC 6749.

@dj2 dj2 closed this May 11, 2022
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.

4 participants