implement OAuth 2.0 Authorization Code Grant #205

Johann150 · 2022-10-16T19:24:35Z

Johann150 commented

2022-10-16 19:24:35 +00:00

This adds the possibility to use the OAuth 2.0 Authorization Code Grant type to obtain an API token.

The token can then be used - as was already implemented - as an OAuth 2.0 Bearer Token.

Maybe also interesting: https://qwertqwefsday.eu/oauth2-0-foundkey.html

scopes (a.k.a. permissions)
I think we could expand the current API and data model insofar as making it possible to set per token permissions.
The idea would be that when registering an app, you have to give what are the most wide permissions the app is ever going to use. Then when you want to get the token you can give a subset of those permissions only.
(Just as a sanity check that is is not completely crazy: Mastodon seems to do this too)
properly check redirect URIs

The complexity of implementing and managing pattern matching correctly obviously causes security issues. This document therefore advises to simplify the required logic and configuration by using exact redirect URI matching. This means the authorization server MUST compare the two URIs using simple string comparison as defined in [RFC3986], Section 6.2.1. The only exception are native apps using a localhost URI: In this case, the AS MUST allow variable port numbers as described in [RFC8252], Section 7.3.
-- OAuth 2.0 Security Best Current Practice (draft 21) § 4.1.3

Proof Key for Code Exchange (PKCE, RFC 7636)
OAuth discovery (RFC 8414)
better documentation

Recommend merge with merge commit: There are separate changelog entries, but no overall entry.

Changelog: Added

This adds the possibility to use the OAuth 2.0 Authorization Code Grant type to obtain an API token. The token can then be used - as was already implemented - as an OAuth 2.0 Bearer Token. <small>Maybe also interesting: <https://qwertqwefsday.eu/oauth2-0-foundkey.html></small> - [x] scopes (a.k.a. permissions) I think we could expand the current API and data model insofar as making it possible to set per token permissions. The idea would be that when registering an app, you have to give what are the most wide permissions the app is ever going to use. Then when you want to get the token you can give a subset of those permissions only. (Just as a sanity check that is is not completely crazy: [Mastodon seems to do this too](https://docs.joinmastodon.org/spec/oauth/)) - [x] properly check redirect URIs > The complexity of implementing and managing pattern matching correctly obviously causes security issues. This document therefore advises to simplify the required logic and configuration by using exact redirect URI matching. This means the authorization server MUST compare the two URIs using simple string comparison as defined in [RFC3986], Section 6.2.1. The only exception are native apps using a localhost URI: In this case, the AS MUST allow variable port numbers as described in [RFC8252], Section 7.3. -- [OAuth 2.0 Security Best Current Practice (draft 21) § 4.1.3](https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics#section-4.1.3) - [x] Proof Key for Code Exchange (PKCE, RFC 7636) - [x] OAuth discovery (RFC 8414) - [x] better documentation - - - Recommend merge with merge commit: There are separate changelog entries, but no overall entry. ``` Changelog: Added ```

Johann150 added this to the (deleted) project 2022-10-16 19:26:20 +00:00

Johann150 commented

2022-10-18 15:37:16 +00:00

~~Another question is: do we want to implement any of the following?~~ Since nobody stopped me I added some of them as todo items.

Proof Key for Code Exchange (PKCE, RFC 7636)
This is an extension of the Authorization Code Grant implemented here that should make it even more secure. (Question: How secure does it have to be if we retain the other auth methods...)
~~OAuth discovery (RFC 8414)~~
dynamic client registration (RFC 7591)
This would be a standardized version of the api/app/* endpoints, although I haven't checked if they would be compatible
token revocation (RFC 7009)

~~Another question is: do we want to implement any of the following?~~ Since nobody stopped me I added some of them as todo items. - ~~Proof Key for Code Exchange (PKCE, RFC 7636) This is an extension of the Authorization Code Grant implemented here that should make it even more secure. (Question: How secure does it have to be if we retain the other auth methods...)~~ - ~~OAuth discovery (RFC 8414)~~ - dynamic client registration (RFC 7591) This would be a standardized version of the `api/app/*` endpoints, although I haven't checked if they would be compatible - token revocation (RFC 7009)

Johann150 force-pushed oauth from badb94bd6f to 3c9422dcb0

2022-10-19 13:31:25 +00:00

Compare

Johann150 force-pushed oauth from 3c9422dcb0 to 3e9217dcd1

2022-11-03 19:18:53 +00:00

Compare

Johann150 force-pushed oauth from 3e9217dcd1 to 1365419a43

2022-11-05 21:47:18 +00:00

Compare

Johann150 force-pushed oauth from 3d2924f900 to 1313c2e3ee

2022-11-06 19:21:13 +00:00

Compare

Johann150 force-pushed oauth from 1313c2e3ee to ffaff820d9

2022-11-06 23:13:48 +00:00

Compare

Johann150 force-pushed oauth from ffaff820d9 to b7d2e9b11a

2022-11-10 20:32:03 +00:00

Compare

Johann150 force-pushed oauth from b7d2e9b11a to 4912fb286c

2022-11-10 22:07:12 +00:00

Compare

Johann150 commented

2022-11-10 22:16:42 +00:00

From my testing this seems to work now. Review and more testing by others appreciated.

Johann150 changed title from ~~WIP: implement OAuth 2.0 Authorization Code Grant~~ to implement OAuth 2.0 Authorization Code Grant

2022-11-10 22:16:45 +00:00

sn0w approved these changes 2022-11-23 20:17:29 +00:00

sn0w left a comment

First-time contributor

I'm SO sorry for taking so long with this. There's been .. stuff going on.
I'll buy you a nice tea or cofe at the next meetup to make up for it :)

From my side this looks good. I tested it with a little cli app on top of golang.org/x/oauth2 and it worked fine. One thing that would be cool to have is support for a urn:ietf:wg:oauth:2.0:oob redirect URI, though I can see that this is likely an edge case (and at any rate perfectly fine as a later addition).

I'm SO sorry for taking so long with this. There's been .. stuff going on. I'll buy you a nice tea or cofe at the next meetup to make up for it :) From my side this looks good. I tested it with a little cli app on top of `golang.org/x/oauth2` and it worked fine. One thing that would be cool to have is support for a `urn:ietf:wg:oauth:2.0:oob` redirect URI, though I can see that this is likely an edge case (and at any rate perfectly fine as a later addition).

❤️ 1

packages/backend/src/server/well-known.ts Outdated

					
				@ -69,0 +66,4 @@

				function oauth(ctx) {

					ctx.body = oauthMeta;

					ctx.type = 'application/json';

					ctx.set('Cache-Control', 'max-age=31536000, immutable');

sn0w commented

2022-11-11 19:31:50 +00:00

First-time contributor

Does it really make sense to cache this immutably for a year?

I know most of this endpoint's content is rather static, but it may lead to problems when an update is published that shifts URLs around or introduces new scopes for some fancy new feature with clients still using the old cached value, especially since immutable turns off revalidation 🤔

There isn't any guidance in the RFCs on this as far as I could tell. I did cross-check what my organization's okta server does though and it seems they limit this endpoint to Cache-Control: max-age=86400, must-revalidate which looks like a reasonable tradeoff.

Maybe some food for thought.

Does it really make sense to cache this immutably for a year? I know most of this endpoint's content is rather static, but it may lead to problems when an update is published that shifts URLs around or introduces new scopes for some fancy new feature with clients still using the old cached value, especially since `immutable` turns off revalidation 🤔 There isn't any guidance in the RFCs on this as far as I could tell. I did cross-check what my organization's okta server does though and it seems they limit this endpoint to `Cache-Control: max-age=86400, must-revalidate` which looks like a reasonable tradeoff. Maybe some food for thought.

Johann150 force-pushed oauth from 4912fb286c to 51f1bbf54c

2022-11-26 15:45:31 +00:00

Compare

sn0w reviewed 2022-11-29 16:54:39 +00:00

toast force-pushed oauth from 6583c3a2be to 7924d5d01b

2022-12-03 10:38:34 +00:00

Compare

toast approved these changes 2022-12-03 12:20:04 +00:00

toast left a comment

Can't see anything wrong with it, but I'm not in a position to test right now.

Johann150 referenced this pull request from a commit

2022-12-04 13:07:28 +00:00

server: implement OAuth 2.0 Authorization Code grant

Johann150 manually merged commit 946e862ecd into main

2022-12-04 13:08:27 +00:00

Johann150 deleted branch oauth

2022-12-04 13:08:42 +00:00

dwarf commented

2022-12-05 11:14:48 +00:00

This seems to break compatibility with apps like SubwayTooter. Reading works, but trying to toot produces "This operation requires privileges which this token does not grant.". Trying to log out and back in results in the browser loading the auth page but it remains blank with a loading animation.

Johann150 commented

2022-12-06 16:12:42 +00:00

Do you have a database backup before the migration?

I'm guessing the migration tokenPermissions1667653936442 is incorrect but I am not sure what is wrong.

Do you have a database backup before the migration? I'm guessing the migration `tokenPermissions1667653936442` is incorrect but I am not sure what is wrong.

dwarf commented

2022-12-06 16:14:12 +00:00

Sadly no. Since I tried it on a mobile device it's quite difficult debugging what goes on in the browser console but I'm sure something shows up there.

Johann150 commented

2022-12-06 19:58:35 +00:00

Hmm I've downloaded subway tooter and authorized it for my account on my instance which does not have this merged yet. I then copied the app and acces_token tables to a testing database and performed the migration there, but everything seems fine. I'm not sure if there was maybe a previous bug in subway tooter that may have cause this. But then I think it should not have worked before.

Anyway, to be sure that it is not the database, can you check the permissions that are currently given to subway tooter in your database are correct?

Database querying instructions

To do this, first run this query:

SELECT id, permission FROM app WHERE name = 'SubwayTooter';

There should be one result. Then run this query, replacing <app id> with the id from the request before.

SELECT permission FROM access_token WHERE "appId" = '<app id>';

Also for this query there should be one result. Both permission values should be the same.

For cross-checking, this is what permissions I found in my database after authorizing subway tooter:

{read:account,write:account,read:blocks,write:blocks,read:drive,write:drive,read:favorites,write:favorites,read:following,write:following,read:messaging,write:messaging,read:mutes,write:mutes,write:notes,read:notifications,write:notifications,read:reactions,write:reactions,write:votes}

Hmm I've downloaded subway tooter and authorized it for my account on my instance which does not have this merged yet. I then copied the `app` and `acces_token` tables to a testing database and performed the migration there, but everything seems fine. I'm not sure if there was maybe a previous bug in subway tooter that may have cause this. But then I think it should not have worked before. Anyway, to be sure that it is not the database, can you check the permissions that are currently given to subway tooter in your database are correct? <details><summary>Database querying instructions</summary> To do this, first run this query: ```sql SELECT id, permission FROM app WHERE name = 'SubwayTooter'; ``` There should be one result. Then run this query, replacing `<app id>` with the id from the request before. ```sql SELECT permission FROM access_token WHERE "appId" = '<app id>'; ``` Also for this query there should be one result. Both `permission` values should be the same. </details> For cross-checking, this is what permissions I found in my database after authorizing subway tooter: ```text {read:account,write:account,read:blocks,write:blocks,read:drive,write:drive,read:favorites,write:favorites,read:following,write:following,read:messaging,write:messaging,read:mutes,write:mutes,write:notes,read:notifications,write:notifications,read:reactions,write:reactions,write:votes} ```

dwarf commented

2022-12-06 20:20:11 +00:00

I authorized the app back when I was using Misskey, so perhaps that could be why this is happening. The first query does return one entry, but the second query gets me 3 results. Two from 2022-12-05, and the original from 2022-05-01. All matching the permissions from your comment, oddly.

Worth noting that I created an access token by hand to use within SubwayTooter so I can continue tooting for now, but I'm not sure if that would be listed by searching for SubwayTooter?

I authorized the app back when I was using Misskey, so perhaps that could be why this is happening. The first query does return one entry, but the second query gets me 3 results. Two from 2022-12-05, and the original from 2022-05-01. All matching the permissions from your comment, oddly. Worth noting that I created an access token by hand to use within SubwayTooter so I can continue tooting for now, but I'm not sure if that would be listed by searching for SubwayTooter?

Johann150 commented

2022-12-06 20:27:13 +00:00

In the meantime I have tried upgrading my instance to include these changes while having subway tooter registered. I can still post after upgrading so I can't reproduce your problem. Maybe it really was because of an older subway tooter version.

Johann150 added the

feature

label 2022-12-23 10:16:27 +00:00

Johann150 removed this from the (deleted) project 2022-12-23 10:16:33 +00:00

Sign in to join this conversation.

No reviewers