-
Notifications
You must be signed in to change notification settings - Fork 162
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
Draft OAuth Spec #326
Draft OAuth Spec #326
Conversation
Thanks so much hailey! Co-authored-by: Hailey <[email protected]>
On return URLs and desktop applications Limiting localhost to "development scenarios" rules out console applications or an application plugin architecture where you cannot control the host and cannot register your own protocol url to receive callbacks. (And protocol URLs are ridiculously hard to make cross platform) The localhost approach is commonly used for plugins, for example, authentication within Visual Studio Code where a plugin runs under the context of another app. If you, for example, attempt to auth with GitHub or Azure AD to sync your plugins VS will pop up a browser, with a return URL of localhost and a random port. The plugin itself has opened a simple listener on localhost and the port and waits to get the token from the auth server. This is also how I (and google) would approach auth in console applications, and to get extremely specific it's how I'd want doing it inside a Windows Widget. This, of course, presents an interesting drawback, if someone writes an abusive command line utilty, or plug-in, you can't ban the client from a server because it's the hardcoded localhost client without banning every other native app using this approach. Expecting each command line utility to have its own domain name, and client json is a heavy burden (for example, as I continue to mess around writing a library I have multiple sample apps, all command line apps, so one domain per sample is both silly and expensive). TLDR - conisder extending native client support to use http://localhost/_randomPort_ as a callback URL, and don't limit that to just development environments. (I realise us desktop app people are old now, but we still exist 😈) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I learnt a lot from reading this, I hope my suggestions are helpful :)
Even if it is outside the development scenario, it seems that it is not prohibited to use According to my understanding, only Either way, I or you are misunderstanding, so it would be worthwhile to clarify the specifications. |
@yamarten It's probably my reading, but it's implied by these (my emphasis added)
Also, that supporting it is optional for servers. |
@blowdart Well, to me, neither of them only describes |
I'll admit I'm incredibly fussy when it comes to authentication and authorization docs. Anywhere they don't have clarity is an area things could be mistaken or misused :) It gets even more complicated if you want to use a native client with a client ID and localhost. As I read it, a native client must have the client metadata file on the internet. Which is fine, except the return URL is going to be static, and for localhost based return URIs the port cannot ever be fixed. |
Indeed, I understand that if the range of the port cannot be determined in advance, there will be problems. Since the port number is finite, it seems that it can be solved by listing it all, but it is not a practical method. |
A potential solution might be special casing native apps.
For example, a return_url of "http://localhost:*/oauth/" would allow http://localhost:12345/oauth, http://localhost:12346/oauth etc. But a specific port, "http://localhost:8080/oauth/" would enforce an exact match on a return_url of http://localhost:8080/oauth/ This would allow specific native clients, like my Windows widget, to support the metadata file, have a client ID, name and get the loopback support. Of course any malicous actor is just going to use the same client ID and name in their own native app, but that'd be the same for any native app, regardless of return protocol. An aside @bnewbold it may be worth specifying whether values in the meta data file are case sensitive or not. |
Thanks so much @surferdude29! Co-authored-by: surfdude29 <[email protected]> Co-authored-by: Hailey <[email protected]>
@surfdude29 thanks so much for the copy-editing! |
@blowdart I think we'd like to end up with robust support for all sorts of native apps, but we might have a great solution for your use-case in this phase of OAuth development. It might be good to move this discussion to a separate github discussion thread so this conversation can continue after this draft gets merged... sorry for the run-around, I think this conversation has already bounced between a few venues which is probably frustrating. My current thoughts and recommendations on native apps:
If none of those can be made to work, we could consider making the localhost exception more formalized for native apps. But i'd want to take this back and discuss with the whole team and understand whether that really solves the issue in a robust long-term way for a large number of native app use-cases (aka, across multiple platforms). |
content/specs/auth.md
Outdated
- **web clients** include web services and browser apps. Redirect URLs are regular web URLs which open in a browser. | ||
- **native clients** include some mobile and desktop native clients. Redirect URLs may use platform-specific app callback schemes to open in the app itself. | ||
|
||
Authorization Servers may maintain a set of "trusted" clients, identified by `client_id`. Because any client could use unverified client metadata to impersonate a better-known app or brand, Authorization Servers should not display such metadata to end users in the Authorization Interface by default. Trusted clients can have additional metadata shown, such as a readable name (`client_name`), project URI (`client_uri`, which may have a different domain/origin than `client_id`) and logo (`logo_uri`). See the "Security Considerations" section for more details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
client_uri
, which may have a different domain/origin thanclient_id
I think we should not allow client_uri
, tos_uri
, or policy_uri
to be on distinct origins. Similarly, we should enforce that these endpoints serve text/html
content with a 200 response and open CORS, in order to potentially loading their content in the auth interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to disagree on CORS here. You don't want the auth endpoint loading policies or TOSes, as you shouldn't be trusting them to render it. You want those links to open in new windows, which show the source URI, so an observant user will see where they come from.
(Not that most people will of course, but some of us might)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are totally right, I meant CSP, not CORS, in order to load in an iframe. loading arbitrary html in an auth interface is a very bad idea indeed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably don't need this (^) for now though.
I would still want to restrict all the uris of the document to the same origin as the client_id
(so that we can relax this later).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tracking here for follow-up: #339
@bnewbold All of which is fair, if one can assume username / password / app password isn't going away, and it will keep up with scopes. But the reason I want to head down the oauth route is I've spent way too long in security and auth to like usernames anymore, plus someone is going to want a C# sample at some point, so why don't I just write it now :) Add to that you're already describing it as legacy, who wants to code against legacy that is going to vanish at some point? The mediating service is fine, but honestly the idea of having a man in the middle worries me just a little. It's just too tempting a target to get popped. |
content/specs/auth.md
Outdated
|
||
#### Request Fields | ||
|
||
A summary of fields relevant to authorization requests with the atproto OAuth profile: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specific case sensitivity for values here. Presumably they're all case sensitive, but best to call it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we refer to / defer to an upstream OAuth RFC on this? there are probably many such details we aren't re-specifying in this document. would like to clarify if we are adding an atproto-specific requirement or not.
@blowdart loopback redirect uris are allowed for all native clients as described in RFC8252 - OAuth 2.0 for Native Apps. Note that there are some caveats (e.g. |
content/specs/auth.md
Outdated
|
||
Authorization Servers may maintain a set of "trusted" clients, identified by `client_id`. Because any client could use unverified client metadata to impersonate a better-known app or brand, Authorization Servers should not display such metadata to end users in the Authorization Interface by default. Trusted clients can have additional metadata shown, such as a readable name (`client_name`), project URI (`client_uri`, which may have a different domain/origin than `client_id`) and logo (`logo_uri`). See the "Security Considerations" section for more details. | ||
|
||
Clients which are only using atproto OAuth for account authentication (without authorization to access PDS resources) should request minimal scopes (see “Scopes” section), but still need to implement most of the authorization flow. In particular, it is critical that they check the `sub` field in a token response to verify the account identity (this is an atproto-specific detail). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how to phrase this, and i know that this is also specified later in this document, but this is an important security measure so I think that for devs skimming through the doc while implementing, it is important to repeat this.
Clients which are only using atproto OAuth for account authentication (without authorization to access PDS resources) should request minimal scopes (see “Scopes” section), but still need to implement most of the authorization flow. In particular, it is critical that they check the `sub` field in a token response to verify the account identity (this is an atproto-specific detail). | |
Clients which are only using atproto OAuth for account authentication (without authorization to access PDS resources) should request minimal scopes (see “Scopes” section), but still need to implement most of the authorization flow. In particular, it is critical that they check the `sub` field in a token response to verify the account identity (this is an atproto-specific detail). Clients must also ensure that the `iss` callback query parameter matches the Authorization Server's (derived from the `sub`) `issuer` metadata. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tracking: #339
content/specs/auth.md
Outdated
|
||
The Authorization Server uses the `request_uri` to look up the earlier Authorization Request parameters, authenticates the user (which might include sign-in or account selection), and prompts the user with the Authorization Interface. The user might refine any granular requested scopes, then approves or rejects the request. The Authorization Server redirects the user back to the `redirect_uri`, which might be a web callback URL (for web clients), or a native app URI. | ||
|
||
The client uses URL query parameters (`state` and `iss`) to look up and verify session information. Using the `code` query parameter, the client then makes an initial token request to the Authorization Server’s token endpoint. The client completes the PKCE flow by including the earlier value in the `code_verifier` field. Confidential clients need to include a client assertion JWT in the token request; see the "Confidential Client" section. The Authorization Server validates the request and returns a set of tokens, as well as a `sub` field indicating the account identifier (DID) for this session, and the `scope` that is covered by the issued access token. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should explain that replayed the code
must be rejected by the AS, and must cause the token previously issued to that code
to be revoked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The later part does make HTTP glitches/retries brittle during the initial token request, which is too bad. And is it really strictly necessary (mandatory, the sort of thing we will eventually write a compliance test for), or just recommended / best practice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only MAY
s and SHOULD
s in various RFCs (see bellow).
The point of this protection is to ensure that the user realizes it is under attack when it's session gets invalidated. Sadly, there is no way to distinguish an attack from a network error during the token exchange...
When a client fails to exchange a code for a token, it typically will implement recovery measures. In case of network error, the recovery method is to initiate a new auth flow, which may be silently accepted by the AS (resulting in no UX degradation). Worst case scenario, the user will have to "accept" again.
https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics#section-4.3.1
When a browser navigates to client.example/redirection_endpoint?code=abcd as a result of a redirect from a provider's authorization endpoint, the URL including the authorization code may end up in the browser's history. An attacker with access to the device could obtain the code and try to replay it.
https://www.rfc-editor.org/rfc/rfc6819.html#section-4.4.1.1
If an authorization server observes multiple attempts to redeem an authorization "code", the authorization server may want to revoke all tokens granted based on the authorization "code" (see Section 5.2.1.1).
The authorization server should enforce a one-time usage restriction (see Section 5.1.5.4).
https://www.rfc-editor.org/rfc/rfc6819.html#section-5.1.5.4
For example, if an authorization server observes more than one attempt to redeem an authorization "code", the authorization server may want to revoke all access tokens granted based on the authorization "code" as well as reject the current request.
https://www.rfc-editor.org/rfc/rfc6819.html#section-5.2.3.6
An authorization server may revoke a client's secret in order to prevent abuse of a revealed secret.
Note: This measure will immediately invalidate any authorization "code" or refresh token issued to the respective client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The real issue here is with refresh_token rotation, as replaying those would result in the whole session to get invalidated. This could happen "randomly" from the user's POV.
One mitigation here, for clients, is to proactively refresh (without waiting the access token to be expired) at a moment when having to login back in is less inconvenient for the user (for example when reactivating the app).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated text to add this as a "should" in the PKCE session (this is about codes).
tracking discussion of whether this is a "must": #339
content/specs/auth.md
Outdated
|
||
At this point it is critical (mandatory) for all clients to verify that the account identified by the `sub` field is consistent with the Authorization Server "issuer" (present in the `iss` query string), either by validating against the originally-supplied account DID, or by resolving the accounts DID to confirm the PDS is consistent with the Authorization Server. See “Identity Authentication” section. The Authorization always returns the scopes approved for the session in the `scopes` field (even if they are the same as the request, as an atproto OAuth profile requirement), which may reflect partial authorization by the user. Clients must reject the session if the response does not include `atproto` in the returned scopes. | ||
|
||
Authentication-only clients can end the flow here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should encourage clients to revoke
credentials they no longer use. The reason being to prevent an attacker from using those credentials without the user noticing.
This requires that we spec that AS must implement a revocation_endpoint
(which is an OIDC feature)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bigger change, tracking here: #339
Co-authored-by: Matthieu Sieben <[email protected]>
I'm generally inclined to keep the summary section at the top as short as possible, as a true summary, and not digress too much on defining terms/specs or enumerating too many security aspects. The main text should be the actual authority. I don't want to distract devs when they are getting the big picture. But maybe a couple small tweaks are ok, and turning terms in to hyperlinks or mouse-over tool tips shouldn't impact readability. I only skimmed the other stuff so far, it seems mostly fine, maybe with some wording tweaks. We might want to give examples of good/bad |
Co-authored-by: Matthieu Sieben <[email protected]>
BTW when this is ready to merge ping me and I'll update it for the new website |
As an update for external folks: please don't comment here on this PR any more, open a new issue on this repo instead. We are moving to merge this PR soon and don't want to lose additional conversation threads or comments. |
Making our OAuth spec draft public, for visibility and feedback.
A couple things we are still cooking on:
As with all atproto specs, this was a big team effort (with matthieu leading the charge). And we are particularly thankful for IETF folks and atproto dev community for early feedback on this work.