bluesky-social / atproto-website

https://atproto.com
Other
258 stars 158 forks source link

Draft OAuth Spec #326

Closed bnewbold closed 2 months ago

bnewbold commented 3 months ago

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.

blowdart commented 3 months ago

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 😈)

yamarten commented 3 months ago

@blowdart

Even if it is outside the development scenario, it seems that it is not prohibited to use localhost for redirect_uris. Which description is specifically the problem?

According to my understanding, only client_id mentions localhost as an exception of restriction that it must be a uniquely and fetchable URL. On the other hand, redirect_uris does not have such restrictions.

Either way, I or you are misunderstanding, so it would be worthwhile to clarify the specifications.

blowdart commented 3 months ago

@yamarten It's probably my reading, but it's implied by these (my emphasis added)

There is a special exception for the localhost development workflow to use http://

To make development workflows easier, a special exception is made for clients with client_id having origin http://localhost (with no port number specified). Authorization Servers are encouraged to support this exception - including in production environments - but it is optional.

Also, that supporting it is optional for servers.

yamarten commented 3 months ago

@blowdart Well, to me, neither of them only describes client_uri and cannot be read as if the callback URI is restricted.

blowdart commented 3 months ago

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.

yamarten commented 3 months ago

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.

blowdart commented 3 months ago

A potential solution might be special casing native apps.

if application_type == "native" AND
   return_url STARTS WITH "http://localhost" or "http://127.0.0.1" or "http://[::1]" AND
   the port is * 
only match on protocol, host and path, and allow **any** specified port, 

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.

bnewbold commented 3 months ago

@surfdude29 thanks so much for the copy-editing!

bnewbold commented 3 months ago

@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).

blowdart commented 3 months ago

@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.

matthieusieben commented 3 months ago

@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. http://localhost is not allowed, only http://127.0.0.1 and http://[::1] are).

cloudflare-workers-and-pages[bot] commented 2 months ago

Deploying atproto-website with  Cloudflare Pages  Cloudflare Pages

Latest commit: 74e4f8e
Status:🚫  Build failed.

View logs

bnewbold commented 2 months ago

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 client_id values (as we do in, eg, the handle spec document), though overall I think we should bias towards getting this doc merged ASAP and do a follow-up PR with any tweaks, instead of getting the initial version perfect.

pfrazee commented 2 months ago

BTW when this is ready to merge ping me and I'll update it for the new website

bnewbold commented 2 months ago

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.