From a9e20c57c8585f9028e8def46b2458a8485ea01b Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Fri, 1 Jul 2022 09:15:40 -0600 Subject: [PATCH 01/10] Refactor device auth schema Requests are now short-lived so that we can use `user_code` as a primary key. Authz is implemented and working for all but access token lookup, which needs to be looked up both by `token` (primary key) and the unique combination of `client_id` and `device_code`. --- common/src/api/external/mod.rs | 2 + common/src/sql/dbinit.sql | 28 ++++--- nexus/src/app/device_auth.rs | 102 ++++++++++++++++++++------ nexus/src/authz/api_resources.rs | 72 ++++++++++++++++++ nexus/src/authz/omicron.polar | 25 +++++++ nexus/src/authz/oso_generic.rs | 3 + nexus/src/db/datastore.rs | 96 ++++++++++-------------- nexus/src/db/lookup.rs | 56 ++++++++++++++ nexus/src/db/model/device_auth.rs | 15 ++++ nexus/src/db/schema.rs | 5 +- nexus/src/external_api/device_auth.rs | 11 ++- 11 files changed, 314 insertions(+), 101 deletions(-) diff --git a/common/src/api/external/mod.rs b/common/src/api/external/mod.rs index c0e92841ed7..83bb373b2a7 100644 --- a/common/src/api/external/mod.rs +++ b/common/src/api/external/mod.rs @@ -518,6 +518,8 @@ pub enum ResourceType { SamlIdentityProvider, SshKey, ConsoleSession, + DeviceAuthRequest, + DeviceAccessToken, GlobalImage, Organization, Project, diff --git a/common/src/sql/dbinit.sql b/common/src/sql/dbinit.sql index 83958afa9fa..0fc94e977a6 100644 --- a/common/src/sql/dbinit.sql +++ b/common/src/sql/dbinit.sql @@ -1252,23 +1252,19 @@ INSERT INTO omicron.public.user_builtin ( * OAuth 2.0 Device Authorization Grant (RFC 8628) */ --- Device authorization requests. In theory these records could --- (and probably should) be short-lived, and removed as soon as --- a token is granted. --- TODO-security: We should not grant a token more than once per record. +-- Device authorization requests. These records are short-lived, +-- and removed as soon as a token is granted. This allows us to +-- use the `user_code` as primary key, despite it not having very +-- much entropy. +-- TODO: A background task should remove unused expired records. CREATE TABLE omicron.public.device_auth_request ( + user_code STRING(20) PRIMARY KEY, client_id UUID NOT NULL, device_code STRING(40) NOT NULL, - user_code STRING(63) NOT NULL, time_created TIMESTAMPTZ NOT NULL, - time_expires TIMESTAMPTZ NOT NULL, - - PRIMARY KEY (client_id, device_code) + time_expires TIMESTAMPTZ NOT NULL ); --- Fast lookup by user_code for verification -CREATE INDEX ON omicron.public.device_auth_request (user_code); - -- Access tokens granted in response to successful device authorization flows. -- TODO-security: expire tokens. CREATE TABLE omicron.public.device_access_token ( @@ -1276,13 +1272,15 @@ CREATE TABLE omicron.public.device_access_token ( client_id UUID NOT NULL, device_code STRING(40) NOT NULL, silo_user_id UUID NOT NULL, - time_created TIMESTAMPTZ NOT NULL + time_created TIMESTAMPTZ NOT NULL, + time_expires TIMESTAMPTZ ); --- Matches the primary key on device authorization records. --- The UNIQUE constraint is critical for ensuring that at most +-- This UNIQUE constraint is critical for ensuring that at most -- one token is ever created for a given device authorization flow. -CREATE UNIQUE INDEX ON omicron.public.device_access_token (client_id, device_code); +CREATE UNIQUE INDEX ON omicron.public.device_access_token ( + client_id, device_code +); /* * Roles built into the system diff --git a/nexus/src/app/device_auth.rs b/nexus/src/app/device_auth.rs index 932a247b728..958f07ec821 100644 --- a/nexus/src/app/device_auth.rs +++ b/nexus/src/app/device_auth.rs @@ -29,8 +29,8 @@ //! automatic case, it may supply the `user_code` as a query parameter. //! 5. The user logs in using their configured IdP, then enters or verifies //! the `user_code`. -//! 6. On successful login, the server is notified and responds to the -//! poll started in step 3 with a freshly granted access token. +//! 6. On successful login, the server responds to the poll started +//! in step 3 with a freshly granted access token. //! //! Note that in this flow, there are actually two distinct sets of //! connections made to the server: by the client itself, and by the @@ -45,44 +45,88 @@ //! but that may change in the future. use crate::authn::{Actor, Reason}; +use crate::authz; use crate::context::OpContext; +use crate::db::lookup::LookupPath; use crate::db::model::{DeviceAccessToken, DeviceAuthRequest}; use crate::external_api::device_auth::DeviceAccessTokenResponse; -use omicron_common::api::external::CreateResult; + +use omicron_common::api::external::{CreateResult, Error}; + +use chrono::Utc; use uuid::Uuid; impl super::Nexus { /// Start a device authorization grant flow. /// Corresponds to steps 1 & 2 in the flow description above. - pub async fn device_auth_request( + pub async fn device_auth_request_create( &self, opctx: &OpContext, client_id: Uuid, ) -> CreateResult { + // TODO-correctness: the `user_code` generated for a new request + // is used as a primary key, but may potentially collide with an + // existing outstanding request. So we should retry some (small) + // number of times if inserting the new request fails. let auth_request = DeviceAuthRequest::new(client_id); - self.db_datastore.device_auth_start(opctx, auth_request).await + self.db_datastore.device_auth_request_create(opctx, auth_request).await } - /// Verify a device authorization grant. + /// Verify a device authorization grant, and delete the authorization + /// request so that at most one token will be granted per request. + /// Invoked in response to a request from the browser, not the client. /// Corresponds to step 5 in the flow description above. - pub async fn device_auth_verify( + pub async fn device_auth_request_verify( &self, opctx: &OpContext, user_code: String, silo_user_id: Uuid, ) -> CreateResult { - let auth_request = - self.db_datastore.device_auth_get_request(opctx, user_code).await?; - let client_id = auth_request.client_id; - let device_code = auth_request.device_code; - let token = - DeviceAccessToken::new(client_id, device_code, silo_user_id); - self.db_datastore.device_auth_grant(opctx, token).await + let (.., authz_request, db_request) = + LookupPath::new(opctx, &self.db_datastore) + .device_auth_request(&user_code) + .fetch() + .await?; + + let (.., authz_user) = LookupPath::new(opctx, &self.datastore()) + .silo_user_id(silo_user_id) + .lookup_for(authz::Action::CreateChild) + .await?; + assert_eq!(authz_user.id(), silo_user_id); + + // Delete the request regardless of whether it's still valid. + self.db_datastore + .device_auth_request_hard_delete(opctx, &authz_request) + .await?; + + // Create an access token record. + let token = DeviceAccessToken::new( + db_request.client_id, + db_request.device_code, + silo_user_id, + ); + + if db_request.time_expires < Utc::now() { + // Store the expired token anyway so that the client + // can get a proper "denied" message on its next poll. + let token = token.expires(db_request.time_expires); + self.db_datastore + .device_access_token_create(opctx, &authz_user, token) + .await?; + Err(Error::InvalidRequest { + message: "device authorization request expired".to_string(), + }) + } else { + // TODO-security: set an expiration time for the valid token. + self.db_datastore + .device_access_token_create(opctx, &authz_user, token) + .await + } } /// Look up a possibly-not-yet-granted device access token. /// Corresponds to steps 3 & 6 in the flow description above. - pub async fn device_access_token_lookup( + pub async fn device_access_token_fetch( &self, opctx: &OpContext, client_id: Uuid, @@ -91,7 +135,7 @@ impl super::Nexus { use DeviceAccessTokenResponse::*; match self .db_datastore - .device_auth_get_token(opctx, client_id, device_code) + .device_access_token_fetch(opctx, client_id, device_code) .await { Ok(token) => Ok(Granted(token)), @@ -107,12 +151,24 @@ impl super::Nexus { opctx: &OpContext, token: String, ) -> Result { - match self.db_datastore.device_access_token_actor(opctx, token).await { - Ok(actor) => Ok(actor), - // TODO: better error handling - Err(_) => Err(Reason::UnknownActor { - actor: String::from("from bearer token"), - }), - } + let (.., db_access_token) = LookupPath::new(opctx, &self.db_datastore) + .device_access_token(&token) + .fetch() + .await + .map_err(|_| Reason::UnknownActor { + actor: "from bearer token".to_string(), + })?; + + let silo_user_id = db_access_token.silo_user_id; + let (.., db_silo_user) = LookupPath::new(opctx, &self.db_datastore) + .silo_user_id(silo_user_id) + .fetch() + .await + .map_err(|_| Reason::UnknownActor { + actor: silo_user_id.to_string(), + })?; + let silo_id = db_silo_user.silo_id; + + Ok(Actor::SiloUser { silo_user_id, silo_id }) } } diff --git a/nexus/src/authz/api_resources.rs b/nexus/src/authz/api_resources.rs index a17568a5b79..b684a608a53 100644 --- a/nexus/src/authz/api_resources.rs +++ b/nexus/src/authz/api_resources.rs @@ -239,6 +239,8 @@ impl db::model::DatabaseString for FleetRole { } } +// TODO: refactor synthetic resources below + /// ConsoleSessionList is a synthetic resource used for modeling who has access /// to create sessions. #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -421,6 +423,60 @@ impl AuthorizedResource for IpPoolList { } } +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub struct DeviceAuthRequestList; +/// Singleton representing the [`DeviceAuthRequestList`] itself for authz purposes +pub const DEVICE_AUTH_REQUEST_LIST: DeviceAuthRequestList = + DeviceAuthRequestList; + +impl oso::PolarClass for DeviceAuthRequestList { + fn get_polar_class_builder() -> oso::ClassBuilder { + oso::Class::builder() + .with_equality_check() + .add_attribute_getter("fleet", |_| FLEET) + } +} + +impl AuthorizedResource for DeviceAuthRequestList { + fn load_roles<'a, 'b, 'c, 'd, 'e, 'f>( + &'a self, + opctx: &'b OpContext, + datastore: &'c DataStore, + authn: &'d authn::Context, + roleset: &'e mut RoleSet, + ) -> futures::future::BoxFuture<'f, Result<(), Error>> + where + 'a: 'f, + 'b: 'f, + 'c: 'f, + 'd: 'f, + 'e: 'f, + { + // There are no roles on the DeviceAuthRequestList, only permissions. But we + // still need to load the Fleet-related roles to verify that the actor has the + // "admin" role on the Fleet. + load_roles_for_resource( + opctx, + datastore, + authn, + ResourceType::Fleet, + *FLEET_ID, + roleset, + ) + .boxed() + } + + fn on_unauthorized( + &self, + _: &Authz, + error: Error, + _: AnyActor, + _: Action, + ) -> Error { + error + } +} + // Main resource hierarchy: Organizations, Projects, and their resources authz_resource! { @@ -602,6 +658,22 @@ authz_resource! { polar_snippet = FleetChild, } +authz_resource! { + name = "DeviceAuthRequest", + parent = "Fleet", + primary_key = String, // user_code + roles_allowed = false, + polar_snippet = FleetChild, +} + +authz_resource! { + name = "DeviceAccessToken", + parent = "Fleet", + primary_key = String, // token + roles_allowed = false, + polar_snippet = FleetChild, +} + authz_resource! { name = "RoleBuiltin", parent = "Fleet", diff --git a/nexus/src/authz/omicron.polar b/nexus/src/authz/omicron.polar index 2e39545a5ca..112dd1e7cb6 100644 --- a/nexus/src/authz/omicron.polar +++ b/nexus/src/authz/omicron.polar @@ -355,6 +355,25 @@ resource ConsoleSessionList { has_relation(fleet: Fleet, "parent_fleet", collection: ConsoleSessionList) if collection.fleet = fleet; +# Describes the policy for creating and managing device authorization requests. +resource DeviceAuthRequestList { + permissions = [ "create_child" ]; + relations = { parent_fleet: Fleet }; + "create_child" if "external-authenticator" on "parent_fleet"; +} +has_relation(fleet: Fleet, "parent_fleet", collection: DeviceAuthRequestList) + if collection.fleet = fleet; + +# Describes the policy for creating and managing device access tokens. +resource DeviceAccessToken { + permissions = [ "read" ]; + relations = { silo_user: SiloUser }; + + "read" if "read" on "silo_user"; +} +has_relation(user: SiloUser, "silo_user", access_token: DeviceAccessToken) + if access_token.silo_user = user; + # These rules grants the external authenticator role the permissions it needs to # read silo users and modify their sessions. This is necessary for login to # work. @@ -362,11 +381,17 @@ has_permission(actor: AuthenticatedActor, "read", silo: Silo) if has_role(actor, "external-authenticator", silo.fleet); has_permission(actor: AuthenticatedActor, "read", user: SiloUser) if has_role(actor, "external-authenticator", user.silo.fleet); + has_permission(actor: AuthenticatedActor, "read", session: ConsoleSession) if has_role(actor, "external-authenticator", session.fleet); has_permission(actor: AuthenticatedActor, "modify", session: ConsoleSession) if has_role(actor, "external-authenticator", session.fleet); +has_permission(actor: AuthenticatedActor, "read", device_auth: DeviceAuthRequest) + if has_role(actor, "external-authenticator", device_auth.fleet); +has_permission(actor: AuthenticatedActor, "read", device_token: DeviceAccessToken) + if has_role(actor, "external-authenticator", device_token.fleet); + has_permission(actor: AuthenticatedActor, "read", identity_provider: IdentityProvider) if has_role(actor, "external-authenticator", identity_provider.silo.fleet); has_permission(actor: AuthenticatedActor, "list_identity_providers", identity_provider: IdentityProvider) diff --git a/nexus/src/authz/oso_generic.rs b/nexus/src/authz/oso_generic.rs index 9f5cda930f8..80cad8b4a1a 100644 --- a/nexus/src/authz/oso_generic.rs +++ b/nexus/src/authz/oso_generic.rs @@ -46,6 +46,7 @@ pub fn make_omicron_oso(log: &slog::Logger) -> Result { IpPoolList::get_polar_class(), GlobalImageList::get_polar_class(), ConsoleSessionList::get_polar_class(), + DeviceAuthRequestList::get_polar_class(), ]; for c in classes { info!(log, "registering Oso class"; "class" => &c.name); @@ -66,6 +67,8 @@ pub fn make_omicron_oso(log: &slog::Logger) -> Result { VpcSubnet::init(), // Fleet-level resources ConsoleSession::init(), + DeviceAuthRequest::init(), + DeviceAccessToken::init(), Rack::init(), RoleBuiltin::init(), SshKey::init(), diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index c53b84c7a45..c73a1f49034 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -25,7 +25,7 @@ use super::error::diesel_pool_result_optional; use super::identity::{Asset, Resource}; use super::pool::DbConnection; use super::Pool; -use crate::authn::{self, Actor}; +use crate::authn; use crate::authz::{self, ApiResource}; use crate::context::OpContext; use crate::db::collection_attach::{AttachError, DatastoreAttachTarget}; @@ -4279,12 +4279,18 @@ impl DataStore { /// Start a device authorization grant flow by recording the request /// and initial response parameters. - // TODO-security: authz - pub async fn device_auth_start( + pub async fn device_auth_request_create( &self, opctx: &OpContext, auth_request: DeviceAuthRequest, ) -> CreateResult { + opctx + .authorize( + authz::Action::CreateChild, + &authz::DEVICE_AUTH_REQUEST_LIST, + ) + .await?; + use db::schema::device_auth_request::dsl; diesel::insert_into(dsl::device_auth_request) .values(auth_request) @@ -4294,33 +4300,34 @@ impl DataStore { .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) } - /// Look up a device authorization request by `user_code`. - // TODO-security: authz - pub async fn device_auth_get_request( + /// Delete a device authorization request. + pub async fn device_auth_request_hard_delete( &self, opctx: &OpContext, - user_code: String, - ) -> LookupResult { + authz_request: &authz::DeviceAuthRequest, + ) -> DeleteResult { + opctx.authorize(authz::Action::Delete, authz_request).await?; + use db::schema::device_auth_request::dsl; - dsl::device_auth_request - .filter(dsl::user_code.eq(user_code)) - .filter(dsl::time_expires.gt(Utc::now())) - .select(DeviceAuthRequest::as_select()) - .get_result_async(self.pool_authorized(opctx).await?) + diesel::delete(dsl::device_auth_request) + .filter(dsl::user_code.eq(authz_request.id())) + .execute_async(self.pool_authorized(opctx).await?) .await - .map_err(|e| { - // TODO-correctness: better error (not found) - public_error_from_diesel_pool(e, ErrorHandler::Server) - }) + .map(|rows_deleted| assert_eq!(rows_deleted, 1)) + .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) } - /// Grant a device authorization token. - // TODO-security: authz - pub async fn device_auth_grant( + /// Create a device access token record. The token may already + /// be expired if the device auth flow was not completed in time. + pub async fn device_access_token_create( &self, opctx: &OpContext, + authz_user: &authz::SiloUser, access_token: DeviceAccessToken, ) -> CreateResult { + assert_eq!(authz_user.id(), access_token.silo_user_id); + opctx.authorize(authz::Action::CreateChild, authz_user).await?; + use db::schema::device_access_token::dsl; diesel::insert_into(dsl::device_access_token) .values(access_token) @@ -4330,9 +4337,9 @@ impl DataStore { .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) } - /// Look up a granted device authorization token. + /// Look up a granted device access token. // TODO-security: authz - pub async fn device_auth_get_token( + pub async fn device_access_token_fetch( &self, opctx: &OpContext, client_id: Uuid, @@ -4346,45 +4353,18 @@ impl DataStore { .get_result_async(self.pool_authorized(opctx).await?) .await .map_err(|e| { - // TODO-correctness: better error (not found) - public_error_from_diesel_pool(e, ErrorHandler::Server) + public_error_from_diesel_pool( + e, + ErrorHandler::NotFoundByLookup( + ResourceType::DeviceAccessToken, + LookupType::ByCompositeId( + "client_id, device_code".to_string(), + ), + ), + ) }) } - /// Look up the actor (a Silo user) for whom a token was granted. - // TODO-security: authz - pub async fn device_access_token_actor( - &self, - opctx: &OpContext, - token: String, - ) -> LookupResult { - use db::schema::device_access_token::dsl; - use db::schema::silo_user::dsl as silo_user_dsl; - - let pool = self.pool_authorized(opctx).await?; - let token = dsl::device_access_token - .filter(dsl::token.eq(token)) - .select(DeviceAccessToken::as_select()) - .get_result_async(pool) - .await - .map_err(|e| { - // TODO-correctness: better error (not found) - public_error_from_diesel_pool(e, ErrorHandler::Server) - })?; - let silo_user_id = token.silo_user_id; - let silo_id = silo_user_dsl::silo_user - .filter(silo_user_dsl::id.eq(silo_user_id)) - .select(SiloUser::as_select()) - .get_result_async(pool) - .await - .map_err(|e| { - // TODO-correctness: better error (not found) - public_error_from_diesel_pool(e, ErrorHandler::Server) - })? - .silo_id; - Ok(Actor::SiloUser { silo_user_id, silo_id }) - } - // Test interfaces #[cfg(test)] diff --git a/nexus/src/db/lookup.rs b/nexus/src/db/lookup.rs index 7b6df46900a..45306acba0f 100644 --- a/nexus/src/db/lookup.rs +++ b/nexus/src/db/lookup.rs @@ -290,6 +290,40 @@ impl<'a> LookupPath<'a> { } } + /// Select a resource of type DeviceAuthRequest, identified by its `user_code` + pub fn device_auth_request<'b, 'c>( + self, + user_code: &'b str, + ) -> DeviceAuthRequest<'c> + where + 'a: 'c, + 'b: 'c, + { + DeviceAuthRequest { + key: DeviceAuthRequestKey::PrimaryKey( + Root { lookup_root: self }, + user_code.to_string(), + ), + } + } + + /// Select a resource of type DeviceAccessToken, identified by its `token` + pub fn device_access_token<'b, 'c>( + self, + token: &'b str, + ) -> DeviceAccessToken<'c> + where + 'a: 'c, + 'b: 'c, + { + DeviceAccessToken { + key: DeviceAccessTokenKey::PrimaryKey( + Root { lookup_root: self }, + token.to_string(), + ), + } + } + /// Select a resource of type RoleBuiltin, identified by its `name` pub fn role_builtin_name(self, name: &str) -> RoleBuiltin<'a> { let parts = name.split_once('.'); @@ -566,6 +600,28 @@ lookup_resource! { ] } +lookup_resource! { + name = "DeviceAuthRequest", + ancestors = [], + children = [], + lookup_by_name = false, + soft_deletes = false, + primary_key_columns = [ + { column_name = "user_code", rust_type = String }, + ] +} + +lookup_resource! { + name = "DeviceAccessToken", + ancestors = [], + children = [], + lookup_by_name = false, + soft_deletes = false, + primary_key_columns = [ + { column_name = "token", rust_type = String }, + ] +} + lookup_resource! { name = "RoleBuiltin", ancestors = [], diff --git a/nexus/src/db/model/device_auth.rs b/nexus/src/db/model/device_auth.rs index ca9fd0a699a..8913e52ca5a 100644 --- a/nexus/src/db/model/device_auth.rs +++ b/nexus/src/db/model/device_auth.rs @@ -85,6 +85,10 @@ impl DeviceAuthRequest { + Duration::seconds(CLIENT_AUTHENTICATION_TIMEOUT), } } + + pub fn id(&self) -> String { + self.user_code.clone() + } } /// An access token granted in response to a successful device authorization flow. @@ -97,6 +101,7 @@ pub struct DeviceAccessToken { pub device_code: String, pub silo_user_id: Uuid, pub time_created: DateTime, + pub time_expires: Option>, } impl DeviceAccessToken { @@ -111,8 +116,18 @@ impl DeviceAccessToken { device_code, silo_user_id, time_created: Utc::now(), + time_expires: None, } } + + pub fn id(&self) -> String { + self.token.clone() + } + + pub fn expires(mut self, time: DateTime) -> Self { + self.time_expires = Some(time); + self + } } #[cfg(test)] diff --git a/nexus/src/db/schema.rs b/nexus/src/db/schema.rs index 06a5504aedb..53c94d1f5cc 100644 --- a/nexus/src/db/schema.rs +++ b/nexus/src/db/schema.rs @@ -514,10 +514,10 @@ table! { } table! { - device_auth_request (client_id, device_code) { + device_auth_request (user_code) { + user_code -> Text, client_id -> Uuid, device_code -> Text, - user_code -> Text, time_created -> Timestamptz, time_expires -> Timestamptz, } @@ -530,6 +530,7 @@ table! { device_code -> Text, silo_user_id -> Uuid, time_created -> Timestamptz, + time_expires -> Nullable, } } diff --git a/nexus/src/external_api/device_auth.rs b/nexus/src/external_api/device_auth.rs index 00132cc078c..ce72a28ec71 100644 --- a/nexus/src/external_api/device_auth.rs +++ b/nexus/src/external_api/device_auth.rs @@ -95,7 +95,8 @@ pub async fn device_auth_request( }, }; - let model = nexus.device_auth_request(&opctx, params.client_id).await?; + let model = + nexus.device_auth_request_create(&opctx, params.client_id).await?; build_oauth_response( StatusCode::OK, &DeviceAuthResponse::from_model(model, host), @@ -162,7 +163,11 @@ pub async fn device_auth_confirm( let opctx = OpContext::for_external_api(&rqctx).await?; let &actor = opctx.authn.actor_required()?; let _token = nexus - .device_auth_verify(&opctx, params.user_code, actor.actor_id()) + .device_auth_request_verify( + &opctx, + params.user_code, + actor.actor_id(), + ) .await?; Ok(HttpResponseOk(())) }; @@ -215,7 +220,7 @@ pub async fn device_access_token( let opctx = nexus.opctx_external_authn(); use DeviceAccessTokenResponse::*; match nexus - .device_access_token_lookup( + .device_access_token_fetch( &opctx, params.client_id, params.device_code, From a195c9e002bd1d367172d1baa716956321f4354b Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Tue, 5 Jul 2022 11:20:40 -0600 Subject: [PATCH 02/10] Trim superfluous policy resource, comment lack of authz on device_access_token_fetch --- nexus/src/authz/omicron.polar | 10 ---------- nexus/src/db/datastore.rs | 7 ++++++- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/nexus/src/authz/omicron.polar b/nexus/src/authz/omicron.polar index 112dd1e7cb6..59e0952b96f 100644 --- a/nexus/src/authz/omicron.polar +++ b/nexus/src/authz/omicron.polar @@ -364,16 +364,6 @@ resource DeviceAuthRequestList { has_relation(fleet: Fleet, "parent_fleet", collection: DeviceAuthRequestList) if collection.fleet = fleet; -# Describes the policy for creating and managing device access tokens. -resource DeviceAccessToken { - permissions = [ "read" ]; - relations = { silo_user: SiloUser }; - - "read" if "read" on "silo_user"; -} -has_relation(user: SiloUser, "silo_user", access_token: DeviceAccessToken) - if access_token.silo_user = user; - # These rules grants the external authenticator role the permissions it needs to # read silo users and modify their sessions. This is necessary for login to # work. diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index c73a1f49034..c3962e9e8a4 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -4338,7 +4338,12 @@ impl DataStore { } /// Look up a granted device access token. - // TODO-security: authz + /// Note: since this lookup is not by a primary key or name, + /// (though it does use a unique index), it does not fit the + /// usual lookup machinery pattern. It therefore does include + /// any authz checks. However, the device code is a single-use + /// high-entropy random token, and so should not be guessable + /// by an attacker. pub async fn device_access_token_fetch( &self, opctx: &OpContext, From 0af0e2705c466a67b08a878fcc940aa9ab4598e6 Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Wed, 6 Jul 2022 09:06:02 -0600 Subject: [PATCH 03/10] Store request time in device_access_token records --- common/src/sql/dbinit.sql | 1 + nexus/src/app/device_auth.rs | 1 + nexus/src/db/model/device_auth.rs | 7 ++++++- nexus/src/db/schema.rs | 1 + 4 files changed, 9 insertions(+), 1 deletion(-) diff --git a/common/src/sql/dbinit.sql b/common/src/sql/dbinit.sql index 0fc94e977a6..c500848123c 100644 --- a/common/src/sql/dbinit.sql +++ b/common/src/sql/dbinit.sql @@ -1272,6 +1272,7 @@ CREATE TABLE omicron.public.device_access_token ( client_id UUID NOT NULL, device_code STRING(40) NOT NULL, silo_user_id UUID NOT NULL, + time_requested TIMESTAMPTZ NOT NULL, time_created TIMESTAMPTZ NOT NULL, time_expires TIMESTAMPTZ ); diff --git a/nexus/src/app/device_auth.rs b/nexus/src/app/device_auth.rs index 958f07ec821..7e138bb2b2e 100644 --- a/nexus/src/app/device_auth.rs +++ b/nexus/src/app/device_auth.rs @@ -103,6 +103,7 @@ impl super::Nexus { let token = DeviceAccessToken::new( db_request.client_id, db_request.device_code, + db_request.time_created, silo_user_id, ); diff --git a/nexus/src/db/model/device_auth.rs b/nexus/src/db/model/device_auth.rs index 8913e52ca5a..aeb37faa65e 100644 --- a/nexus/src/db/model/device_auth.rs +++ b/nexus/src/db/model/device_auth.rs @@ -100,6 +100,7 @@ pub struct DeviceAccessToken { pub client_id: Uuid, pub device_code: String, pub silo_user_id: Uuid, + pub time_requested: DateTime, pub time_created: DateTime, pub time_expires: Option>, } @@ -108,14 +109,18 @@ impl DeviceAccessToken { pub fn new( client_id: Uuid, device_code: String, + time_requested: DateTime, silo_user_id: Uuid, ) -> Self { + let now = Utc::now(); + assert!(time_requested < now); Self { token: generate_token(), client_id, device_code, silo_user_id, - time_created: Utc::now(), + time_requested, + time_created: now, time_expires: None, } } diff --git a/nexus/src/db/schema.rs b/nexus/src/db/schema.rs index 53c94d1f5cc..8a8cd00b90c 100644 --- a/nexus/src/db/schema.rs +++ b/nexus/src/db/schema.rs @@ -529,6 +529,7 @@ table! { client_id -> Uuid, device_code -> Text, silo_user_id -> Uuid, + time_requested -> Timestamptz, time_created -> Timestamptz, time_expires -> Nullable, } From 027d517c843b06dd7ade82c902880b30c58efea0 Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Wed, 6 Jul 2022 09:37:49 -0600 Subject: [PATCH 04/10] Delete device auth request and insert access token in transaction --- nexus/src/app/device_auth.rs | 9 ++------ nexus/src/db/datastore.rs | 42 ++++++++++++++++-------------------- 2 files changed, 21 insertions(+), 30 deletions(-) diff --git a/nexus/src/app/device_auth.rs b/nexus/src/app/device_auth.rs index 7e138bb2b2e..97a709423ca 100644 --- a/nexus/src/app/device_auth.rs +++ b/nexus/src/app/device_auth.rs @@ -94,11 +94,6 @@ impl super::Nexus { .await?; assert_eq!(authz_user.id(), silo_user_id); - // Delete the request regardless of whether it's still valid. - self.db_datastore - .device_auth_request_hard_delete(opctx, &authz_request) - .await?; - // Create an access token record. let token = DeviceAccessToken::new( db_request.client_id, @@ -112,7 +107,7 @@ impl super::Nexus { // can get a proper "denied" message on its next poll. let token = token.expires(db_request.time_expires); self.db_datastore - .device_access_token_create(opctx, &authz_user, token) + .device_access_token_create(opctx, &authz_request, &authz_user, token) .await?; Err(Error::InvalidRequest { message: "device authorization request expired".to_string(), @@ -120,7 +115,7 @@ impl super::Nexus { } else { // TODO-security: set an expiration time for the valid token. self.db_datastore - .device_access_token_create(opctx, &authz_user, token) + .device_access_token_create(opctx, &authz_request, &authz_user, token) .await } } diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index c3962e9e8a4..6935ee62e3c 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -4300,39 +4300,35 @@ impl DataStore { .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) } - /// Delete a device authorization request. - pub async fn device_auth_request_hard_delete( - &self, - opctx: &OpContext, - authz_request: &authz::DeviceAuthRequest, - ) -> DeleteResult { - opctx.authorize(authz::Action::Delete, authz_request).await?; - - use db::schema::device_auth_request::dsl; - diesel::delete(dsl::device_auth_request) - .filter(dsl::user_code.eq(authz_request.id())) - .execute_async(self.pool_authorized(opctx).await?) - .await - .map(|rows_deleted| assert_eq!(rows_deleted, 1)) - .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) - } - - /// Create a device access token record. The token may already - /// be expired if the device auth flow was not completed in time. + /// Remove the device authorization request and create a new device + /// access token record. The token may already be expired if the flow + /// was not completed in time. pub async fn device_access_token_create( &self, opctx: &OpContext, + authz_request: &authz::DeviceAuthRequest, authz_user: &authz::SiloUser, access_token: DeviceAccessToken, ) -> CreateResult { assert_eq!(authz_user.id(), access_token.silo_user_id); + opctx.authorize(authz::Action::Delete, authz_request).await?; opctx.authorize(authz::Action::CreateChild, authz_user).await?; - use db::schema::device_access_token::dsl; - diesel::insert_into(dsl::device_access_token) + use db::schema::device_auth_request::dsl as request_dsl; + let delete_request = diesel::delete(request_dsl::device_auth_request) + .filter(request_dsl::user_code.eq(authz_request.id())); + + use db::schema::device_access_token::dsl as token_dsl; + let insert_token = diesel::insert_into(token_dsl::device_access_token) .values(access_token) - .returning(DeviceAccessToken::as_returning()) - .get_result_async(self.pool_authorized(opctx).await?) + .returning(DeviceAccessToken::as_returning()); + + self.pool_authorized(opctx) + .await? + .transaction(move |conn| { + delete_request.execute(conn)?; + Ok(insert_token.get_result(conn)?) + }) .await .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) } From c4f4d01d809d43002bf691f763ee1fcf589a989b Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Wed, 6 Jul 2022 09:53:26 -0600 Subject: [PATCH 05/10] fmt --- nexus/src/app/device_auth.rs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/nexus/src/app/device_auth.rs b/nexus/src/app/device_auth.rs index 97a709423ca..122b95c7edd 100644 --- a/nexus/src/app/device_auth.rs +++ b/nexus/src/app/device_auth.rs @@ -107,7 +107,12 @@ impl super::Nexus { // can get a proper "denied" message on its next poll. let token = token.expires(db_request.time_expires); self.db_datastore - .device_access_token_create(opctx, &authz_request, &authz_user, token) + .device_access_token_create( + opctx, + &authz_request, + &authz_user, + token, + ) .await?; Err(Error::InvalidRequest { message: "device authorization request expired".to_string(), @@ -115,7 +120,12 @@ impl super::Nexus { } else { // TODO-security: set an expiration time for the valid token. self.db_datastore - .device_access_token_create(opctx, &authz_request, &authz_user, token) + .device_access_token_create( + opctx, + &authz_request, + &authz_user, + token, + ) .await } } From 77a0d8cbfe6dbd80ca2e3e6a7c5a6bb97e471736 Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Wed, 6 Jul 2022 09:53:31 -0600 Subject: [PATCH 06/10] Refine error handling in device_access_token_actor --- nexus/src/app/device_auth.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/nexus/src/app/device_auth.rs b/nexus/src/app/device_auth.rs index 122b95c7edd..368d866c7dc 100644 --- a/nexus/src/app/device_auth.rs +++ b/nexus/src/app/device_auth.rs @@ -161,8 +161,11 @@ impl super::Nexus { .device_access_token(&token) .fetch() .await - .map_err(|_| Reason::UnknownActor { - actor: "from bearer token".to_string(), + .map_err(|e| match e { + Error::ObjectNotFound { .. } => Reason::UnknownActor { + actor: "from device access token".to_string(), + }, + e => Reason::UnknownError { source: e }, })?; let silo_user_id = db_access_token.silo_user_id; @@ -170,8 +173,11 @@ impl super::Nexus { .silo_user_id(silo_user_id) .fetch() .await - .map_err(|_| Reason::UnknownActor { - actor: silo_user_id.to_string(), + .map_err(|e| match e { + Error::ObjectNotFound { .. } => { + Reason::UnknownActor { actor: silo_user_id.to_string() } + } + e => Reason::UnknownError { source: e }, })?; let silo_id = db_silo_user.silo_id; From 2b504ca34b627c8e91b8cc13d2e9c9eb11719399 Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Wed, 6 Jul 2022 11:02:32 -0600 Subject: [PATCH 07/10] Allow for low-resolution clocks --- nexus/src/db/model/device_auth.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nexus/src/db/model/device_auth.rs b/nexus/src/db/model/device_auth.rs index aeb37faa65e..b0a0703c5ea 100644 --- a/nexus/src/db/model/device_auth.rs +++ b/nexus/src/db/model/device_auth.rs @@ -113,7 +113,7 @@ impl DeviceAccessToken { silo_user_id: Uuid, ) -> Self { let now = Utc::now(); - assert!(time_requested < now); + assert!(time_requested <= now); Self { token: generate_token(), client_id, From 152fd2a2bf149c1606bbf7bcccbf15ed4cf8cfb6 Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Wed, 6 Jul 2022 11:32:16 -0600 Subject: [PATCH 08/10] Handle concurrent token grants in transaction --- nexus/src/db/datastore.rs | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 6935ee62e3c..af8bcf706d5 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -4323,14 +4323,34 @@ impl DataStore { .values(access_token) .returning(DeviceAccessToken::as_returning()); + #[derive(Debug)] + enum TokenGrantError { + ConcurrentRequests, + } + type TxnError = TransactionError; + self.pool_authorized(opctx) .await? .transaction(move |conn| { - delete_request.execute(conn)?; - Ok(insert_token.get_result(conn)?) + if delete_request.execute(conn)? == 1 { + Ok(insert_token.get_result(conn)?) + } else { + Err(TxnError::CustomError( + TokenGrantError::ConcurrentRequests, + )) + } }) .await - .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) + .map_err(|e| match e { + TxnError::CustomError(TokenGrantError::ConcurrentRequests) => { + Error::invalid_request( + "token grant failed due to concurrent requests", + ) + } + TxnError::Pool(e) => { + public_error_from_diesel_pool(e, ErrorHandler::Server) + } + }) } /// Look up a granted device access token. From bf53d38f023d589dfb1f960739797328e47bb798 Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Wed, 6 Jul 2022 17:14:06 -0600 Subject: [PATCH 09/10] Even more refined error handling in device_access_token_create --- nexus/src/db/datastore.rs | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index af8bcf706d5..6aaff854959 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -4325,23 +4325,32 @@ impl DataStore { #[derive(Debug)] enum TokenGrantError { + RequestNotFound, ConcurrentRequests, } type TxnError = TransactionError; self.pool_authorized(opctx) .await? - .transaction(move |conn| { - if delete_request.execute(conn)? == 1 { - Ok(insert_token.get_result(conn)?) - } else { - Err(TxnError::CustomError( - TokenGrantError::ConcurrentRequests, - )) + .transaction(move |conn| match delete_request.execute(conn)? { + 0 => { + Err(TxnError::CustomError(TokenGrantError::RequestNotFound)) } + 1 => Ok(insert_token.get_result(conn)?), + _ => Err(TxnError::CustomError( + TokenGrantError::ConcurrentRequests, + )), }) .await .map_err(|e| match e { + TxnError::CustomError(TokenGrantError::RequestNotFound) => { + Error::ObjectNotFound { + type_name: ResourceType::DeviceAuthRequest, + lookup_type: LookupType::ByCompositeId( + authz_request.id(), + ), + } + } TxnError::CustomError(TokenGrantError::ConcurrentRequests) => { Error::invalid_request( "token grant failed due to concurrent requests", From 8ce7e42758cd05106db9f7ef13354f6f37490909 Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Thu, 7 Jul 2022 10:47:34 -0600 Subject: [PATCH 10/10] Refine error handling harder --- nexus/src/db/datastore.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 6aaff854959..f8266c33999 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -4326,7 +4326,7 @@ impl DataStore { #[derive(Debug)] enum TokenGrantError { RequestNotFound, - ConcurrentRequests, + TooManyRequests, } type TxnError = TransactionError; @@ -4338,7 +4338,7 @@ impl DataStore { } 1 => Ok(insert_token.get_result(conn)?), _ => Err(TxnError::CustomError( - TokenGrantError::ConcurrentRequests, + TokenGrantError::TooManyRequests, )), }) .await @@ -4351,10 +4351,8 @@ impl DataStore { ), } } - TxnError::CustomError(TokenGrantError::ConcurrentRequests) => { - Error::invalid_request( - "token grant failed due to concurrent requests", - ) + TxnError::CustomError(TokenGrantError::TooManyRequests) => { + Error::internal_error("unexpectedly found multiple device auth requests for the same user code") } TxnError::Pool(e) => { public_error_from_diesel_pool(e, ErrorHandler::Server)