API client authentication and token granting #1194

plotnick · 2022-06-10T20:24:25Z

Support OAuth 2.0 Device Authorization Grant for client (e.g., CLI) authentication. Remaining TODOs:

Tests
Better error handling (see Fix device auth error handling #1286)
Datastore authz (see DB authz for client authentication & token records #1255)
Dropshot#363 (application/x-www-form-urlencoded bodies)
Finish RFD 275

Dependents:

CLI#118 (login via token grant)
Console#929 (client authentication verification page)

Depends on dropshot commit c903eb8def228d4597b379b67c4ecc82164a11f8

Needed for "application/x-www-form-urlencoded" body support.

davepacheco

This is an impressive piece of work! I have a bunch of comments below but most are aimed at clarity and error handling. Thanks for doing this!

davepacheco · 2022-06-24T22:09:26Z

common/src/sql/dbinit.sql

+-- a token is granted.
+-- TODO-security: We should not grant a token more than once per record.
+CREATE TABLE omicron.public.client_authentication (
+    client_id UUID NOT NULL,


It would be great to have a Big Theory Statement-style comment about the client id, device code, user code, etc. It doesn't have to be here -- it could be in the Rust code somewhere. (edit: I suggested putting it in nexus/src/app/client_api.rs)

If there's something like the Beer Drinker's Guide to SAML for the Oauth device flow, that might work too.

Ok, basic flow outline added to app/device_auth.rs in c9839ba.

davepacheco · 2022-06-24T22:11:51Z

common/src/sql/dbinit.sql

+    time_created TIMESTAMPTZ NOT NULL
+);
+
+-- Matches the primary key on client authentication records.


Can you add: "this is critical for ensuring that no more than one token is ever created for a client authentication attempt"? Like we talked about my fear here is that someone won't realize what's load-bearing here (e.g., that if they change this index in some ways, they might break that at-most-one-for-the-other-table property)

Yes, fixed in 139c47f. Further schema revisions to be taken as a follow-up (#1285).

davepacheco · 2022-06-24T22:14:04Z

nexus/src/app/client_api.rs

@@ -0,0 +1,75 @@
+// This Source Code Form is subject to the terms of the Mozilla Public


I think we talked about this a bit but I do find the naming here a little confusing. We have lots of clients and lots of tokens, and browsers are clients that authenticate with tokens too. Might this be better called "oauth_device.rs" with oauth_device_authenticate() etc? I might apply that to the tables too.

I realize this great fodder for bikeshedding and we can always change it later instead but I figured I'd ask.

edit: see also the comment in datastore.rs

Agreed. c9839ba renames client_api → device_auth, which I think is clearer. Function and table names updated as well.

davepacheco · 2022-06-24T22:15:35Z

nexus/src/app/client_api.rs

+use omicron_common::api::external::{CreateResult, LookupResult};
+use uuid::Uuid;
+
+impl super::Nexus {


Above this block (or part of the module-level doc comment) might be a good place to give an overview of the oauth device flow and how these functions are used. It may also be nice to have a sentence or two for each function saying which part of the flow it's used in.

Yep, done in c9839ba.

davepacheco · 2022-06-24T22:21:36Z

nexus/src/authn/external/mod.rs

@@ -111,6 +113,12 @@ pub enum SchemeResult {
    Failed(Reason),
 }

+/// A context that can look up a Silo user's Silo.


What do you think of renaming this to SiloUserGetSilo or something? (I know I overuse "context" but this feels like less of a "context" than it was when it was in spoof.rs.)

Done in bd8da5f (SiloUserSilo).

davepacheco · 2022-06-24T22:57:14Z

nexus/tests/integration_tests/client_api.rs

+        .expect_status(Some(StatusCode::BAD_REQUEST))
+        .execute()
+        .await
+        .expect("client_id required to start client authentication flow");


I'm not sure if you're trying to check the error message here, but this won't do that.

No, it was just poorly worded. Fixed in bbab057.

davepacheco · 2022-06-24T23:02:35Z

nexus/src/external_api/client_api.rs

+            },
+        };
+
+        let model = nexus.client_authenticate(&opctx, params.client_id).await?;


Is this going to do the right thing in terms of the error response form? Say the database is down. It seems like this will generate an Err(Error::ServiceUnavailable), which will get turned into a dropshot::HttpError, which I think we're not supposed to be returning from this function?

If that's true, even if we fix it here, it seems easy to accidentally re-introduce that bug. I wonder if we should have the body of these handlers call a Rust function that returns only Response<Body> (no Result). We could document why -- we need to format all the responses ourselves to ensure compliance.

Excellent catch, thank you. I'd like to take this as a follow-up, recorded as #1286.

davepacheco · 2022-06-24T23:03:38Z

nexus/src/external_api/client_api.rs

+            &ClientAuthentication::from_model(model, host),
+        )
+    };
+    // TODO: instrumentation doesn't work because we use `Response<Body>`


Why doesn't the instrumentation work with that?

Excellent question; I don't actually know! I copied the comment from console_api.rs, but will investigate as part of #1286.

nexus/src/db/model/client_api.rs

nexus/src/external_api/client_api.rs

Currently used only by test_device_auth_flow integration test.

ahl

a couple of questions as I try to integrate these changes into the generated client.

ahl · 2022-07-03T04:59:38Z

nexus/src/external_api/device_auth.rs

+#[endpoint {
+    method = POST,
+    path = "/device/token",
+    content_type = "application/x-www-form-urlencoded",


I'm late to this (sorry) but I thought endpoints using application/x-www-form-urlencoded were not going to appear in the OpenAPI spec for omicron. Did I misunderstand?

If our client is the consumer of this, why isn't it using application/json?

I don't have strong opinions on whether these should appear in the spec or not; we can set unpublished = true if you think that's a good idea. Personally I think they should appear, but don't necessarily need full body descriptions if that's not easy. The TS generator, for instance, just skips non-application/json endpoints, which I think is reasonable, since they'll be used only by OAuth client libraries, not directly by our client.

The reason for not using application/json is that the OAuth protocol requires application/x-www-form-urlencoded request bodies, and I would like us not to reinvent that particular wheel. OAuth clients exist for every popular language & framework, and the idea is that we can use off-the-shelf, well-tested client implementations instead of rolling our own n times. That is, we could implement and document our own not-quite-compatible protocol, but I don't see any concrete advantage to doing so. OAuth is not a great protocol, but it's been widely analyzed and implemented, and I think if we're not going to use it, we should have good reasons why we think our own version would be better, more secure, etc. This PR implements only the Device Authorization Grant flow, but we can layer more things on it, such as short-lived access tokens with long-lived refresh tokens. RFD 275 attempts to provide some of this rationale, and is probably the right place for a serious discussion of this issue.

Finally, I suppose it's fair to ask why we should write our own OAuth server but not the client libraries. To this I would respond that (1) we only need one server implementation, whereas for the clients we'd need n clients with specialized token, error, retry, and timeout handling, and (2) to the best of my understanding, we want there to be a single source of truth server-side (CockroachDB) and a single piece of software (Nexus) that handles all client requests and talks to that database, provides authz for it, etc. I did look into off-the-shelf OAuth servers, but none of them seemed suitable for our needs. OTOH, the client libraries are readily available and easy to integrate.

ahl · 2022-07-03T05:01:11Z

nexus/src/external_api/device_auth.rs

+    method = POST,
+    path = "/device/auth",
+    content_type = "application/x-www-form-urlencoded",
+    tags = ["hidden"], // "token"


This seems important for OAuth, but can we set unpublished = true since I don't think we expect other consumers to use this?

plotnick added 2 commits June 10, 2022 14:14

Client authentication and token granting

57d47df

Client token authentication scheme

33bbd46

This was referenced Jun 10, 2022

Add client authn verification page oxidecomputer/console#929

Merged

Login with OAuth 2.0 Device Authorization Grant oxidecomputer/cli-old#188

Merged

plotnick added 3 commits June 19, 2022 12:33

Fix authn integration test code

04f8a4a

Specify content type for OAuth endpoints

afc1fd2

Depends on dropshot commit c903eb8def228d4597b379b67c4ecc82164a11f8

Bump async-trait version to match dropshot

80d0c68

plotnick force-pushed the client-authn branch from d01c5ee to 0acdf5d Compare June 23, 2022 15:27

plotnick added 3 commits June 23, 2022 09:31

Drop Actor::ApiClient and Kind::Authenticating enum variants

0f6041e

Use build_oauth_response in client_authenticate

f6852a7

Add integration test for client token granting

6640c12

plotnick force-pushed the client-authn branch from 0acdf5d to 6640c12 Compare June 23, 2022 15:32

plotnick added 4 commits June 23, 2022 09:46

Add token authorization header tests

a72d8b9

Update to latest dropshot

55f8b49

Needed for "application/x-www-form-urlencoded" body support.

Merge branch 'main' into client-authn

b27a45e

Add TODOs for DB authz, make index on client tokens unique

81e907c

plotnick mentioned this pull request Jun 23, 2022

DB authz for client authentication & token records #1255

Closed

plotnick marked this pull request as ready for review June 23, 2022 19:31

plotnick requested review from davepacheco and smklein June 23, 2022 19:32

davepacheco mentioned this pull request Jun 23, 2022

tracking issue for MVP IAM work #849

Closed

69 tasks

davepacheco reviewed Jun 24, 2022

View reviewed changes

plotnick added 8 commits June 27, 2022 14:24

Rename client_api → device_auth

c9839ba

Update device auth schema comments

139c47f

Rename SiloContext → SiloUserSilo

bd8da5f

Revert wildcard match on authn::Kind

f63ab0c

Use ErrorHandler::Server

09e4eff

Add allow_non_dropshot_errors flag to test RequestBuilder

bbab057

Currently used only by test_device_auth_flow integration test.

Merge branch 'main' into client-authn

542c5ff

Fix uncovered-authz-endpoints ordering

377cf6d

davepacheco approved these changes Jun 28, 2022

View reviewed changes

plotnick merged commit e4126a1 into main Jun 29, 2022

plotnick deleted the client-authn branch June 29, 2022 00:17

ahl reviewed Jul 3, 2022

View reviewed changes

plotnick mentioned this pull request Jul 13, 2022

device auth flow #595

Closed

		@@ -0,0 +1,75 @@
		// This Source Code Form is subject to the terms of the Mozilla Public

API client authentication and token granting #1194

API client authentication and token granting #1194

Uh oh!

Conversation

plotnick commented Jun 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davepacheco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ahl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

plotnick commented Jun 10, 2022 •

edited

Loading