From e0b39594fc300ccad4755f42125975f0e94285f7 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 9 May 2025 10:34:08 -0500 Subject: [PATCH 1/8] add IDs to tokens and sessions --- nexus/db-model/src/console_session.rs | 9 ++++++++- nexus/db-model/src/device_auth.rs | 3 +++ nexus/db-model/src/schema_versions.rs | 3 ++- nexus/db-queries/src/db/datastore/mod.rs | 1 + nexus/db-schema/src/schema.rs | 2 ++ nexus/tests/integration_tests/schema.rs | 14 ++++++++++++++ schema/crdb/dbinit.sql | 21 ++++++++++++++++++--- schema/crdb/token-and-session-ids/up01.sql | 2 ++ schema/crdb/token-and-session-ids/up02.sql | 5 +++++ schema/crdb/token-and-session-ids/up03.sql | 2 ++ schema/crdb/token-and-session-ids/up04.sql | 14 ++++++++++++++ schema/crdb/token-and-session-ids/up05.sql | 2 ++ schema/crdb/token-and-session-ids/up06.sql | 2 ++ schema/crdb/token-and-session-ids/up07.sql | 2 ++ schema/crdb/token-and-session-ids/up08.sql | 5 +++++ schema/crdb/token-and-session-ids/up09.sql | 2 ++ schema/crdb/token-and-session-ids/up10.sql | 14 ++++++++++++++ schema/crdb/token-and-session-ids/up11.sql | 2 ++ schema/crdb/token-and-session-ids/up12.sql | 2 ++ 19 files changed, 102 insertions(+), 5 deletions(-) create mode 100644 schema/crdb/token-and-session-ids/up01.sql create mode 100644 schema/crdb/token-and-session-ids/up02.sql create mode 100644 schema/crdb/token-and-session-ids/up03.sql create mode 100644 schema/crdb/token-and-session-ids/up04.sql create mode 100644 schema/crdb/token-and-session-ids/up05.sql create mode 100644 schema/crdb/token-and-session-ids/up06.sql create mode 100644 schema/crdb/token-and-session-ids/up07.sql create mode 100644 schema/crdb/token-and-session-ids/up08.sql create mode 100644 schema/crdb/token-and-session-ids/up09.sql create mode 100644 schema/crdb/token-and-session-ids/up10.sql create mode 100644 schema/crdb/token-and-session-ids/up11.sql create mode 100644 schema/crdb/token-and-session-ids/up12.sql diff --git a/nexus/db-model/src/console_session.rs b/nexus/db-model/src/console_session.rs index eb3111841af..cec2d6a8ea3 100644 --- a/nexus/db-model/src/console_session.rs +++ b/nexus/db-model/src/console_session.rs @@ -11,6 +11,7 @@ use uuid::Uuid; #[derive(Queryable, Insertable, Clone, Debug, Selectable)] #[diesel(table_name = console_session)] pub struct ConsoleSession { + pub id: Uuid, pub token: String, pub time_created: DateTime, pub time_last_used: DateTime, @@ -20,7 +21,13 @@ pub struct ConsoleSession { impl ConsoleSession { pub fn new(token: String, silo_user_id: Uuid) -> Self { let now = Utc::now(); - Self { token, silo_user_id, time_last_used: now, time_created: now } + Self { + id: Uuid::new_v4(), + token, + silo_user_id, + time_last_used: now, + time_created: now, + } } pub fn id(&self) -> String { diff --git a/nexus/db-model/src/device_auth.rs b/nexus/db-model/src/device_auth.rs index 07df902f3dd..e31c75cc6ec 100644 --- a/nexus/db-model/src/device_auth.rs +++ b/nexus/db-model/src/device_auth.rs @@ -117,6 +117,7 @@ impl DeviceAuthRequest { #[derive(Clone, Debug, Insertable, Queryable, Selectable)] #[diesel(table_name = device_access_token)] pub struct DeviceAccessToken { + pub id: Uuid, pub token: String, pub client_id: Uuid, pub device_code: String, @@ -136,6 +137,7 @@ impl DeviceAccessToken { let now = Utc::now(); assert!(time_requested <= now); Self { + id: Uuid::new_v4(), token: generate_token(), client_id, device_code, @@ -146,6 +148,7 @@ impl DeviceAccessToken { } } + // TODO: uhhhh pub fn id(&self) -> String { self.token.clone() } diff --git a/nexus/db-model/src/schema_versions.rs b/nexus/db-model/src/schema_versions.rs index 178d7e4de2b..9c4b6b93f71 100644 --- a/nexus/db-model/src/schema_versions.rs +++ b/nexus/db-model/src/schema_versions.rs @@ -16,7 +16,7 @@ use std::{collections::BTreeMap, sync::LazyLock}; /// /// This must be updated when you change the database schema. Refer to /// schema/crdb/README.adoc in the root of this repository for details. -pub const SCHEMA_VERSION: Version = Version::new(141, 0, 0); +pub const SCHEMA_VERSION: Version = Version::new(142, 0, 0); /// List of all past database schema versions, in *reverse* order /// @@ -28,6 +28,7 @@ static KNOWN_VERSIONS: LazyLock> = LazyLock::new(|| { // | leaving the first copy as an example for the next person. // v // KnownVersion::new(next_int, "unique-dirname-with-the-sql-files"), + KnownVersion::new(142, "token-and-session-ids"), KnownVersion::new(141, "caboose-sign-value"), KnownVersion::new(140, "instance-intended-state"), KnownVersion::new(139, "webhooks"), diff --git a/nexus/db-queries/src/db/datastore/mod.rs b/nexus/db-queries/src/db/datastore/mod.rs index 5a2acb6aff5..fb57a603e7d 100644 --- a/nexus/db-queries/src/db/datastore/mod.rs +++ b/nexus/db-queries/src/db/datastore/mod.rs @@ -575,6 +575,7 @@ mod test { let silo_user_id = Uuid::new_v4(); let session = ConsoleSession { + id: Uuid::new_v4(), token: token.clone(), time_created: Utc::now() - Duration::minutes(5), time_last_used: Utc::now() - Duration::minutes(5), diff --git a/nexus/db-schema/src/schema.rs b/nexus/db-schema/src/schema.rs index e1614e671f0..9eb2e1856d0 100644 --- a/nexus/db-schema/src/schema.rs +++ b/nexus/db-schema/src/schema.rs @@ -929,6 +929,7 @@ table! { table! { console_session (token) { + id -> Uuid, token -> Text, time_created -> Timestamptz, time_last_used -> Timestamptz, @@ -1355,6 +1356,7 @@ table! { table! { device_access_token (token) { + id -> Uuid, token -> Text, client_id -> Uuid, device_code -> Text, diff --git a/nexus/tests/integration_tests/schema.rs b/nexus/tests/integration_tests/schema.rs index 9bdc3125219..bd44db804e0 100644 --- a/nexus/tests/integration_tests/schema.rs +++ b/nexus/tests/integration_tests/schema.rs @@ -2187,6 +2187,16 @@ fn after_140_0_0<'a>(ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> { }) } +// TODO, obviously + +fn before_142_0_0<'a>(_ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> { + Box::pin(async {}) +} + +fn after_142_0_0<'a>(_ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> { + Box::pin(async {}) +} + // Lazily initializes all migration checks. The combination of Rust function // pointers and async makes defining a static table fairly painful, so we're // using lazy initialization instead. @@ -2251,6 +2261,10 @@ fn get_migration_checks() -> BTreeMap { Version::new(140, 0, 0), DataMigrationFns::new().before(before_140_0_0).after(after_140_0_0), ); + map.insert( + Version::new(142, 0, 0), + DataMigrationFns::new().before(before_142_0_0).after(after_142_0_0), + ); map } diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 5cfd68577cb..9652c3167fa 100644 --- a/schema/crdb/dbinit.sql +++ b/schema/crdb/dbinit.sql @@ -2406,7 +2406,8 @@ CREATE TABLE IF NOT EXISTS omicron.public.saga_node_event ( * Sessions for use by web console. */ CREATE TABLE IF NOT EXISTS omicron.public.console_session ( - token STRING(40) PRIMARY KEY, + id UUID PRIMARY KEY, + token STRING(40) NOT NULL, time_created TIMESTAMPTZ NOT NULL, time_last_used TIMESTAMPTZ NOT NULL, silo_user_id UUID NOT NULL @@ -2424,6 +2425,14 @@ CREATE INDEX IF NOT EXISTS lookup_console_by_silo_user ON omicron.public.console silo_user_id ); +-- We added a UUID as the primary key, but we need the token to keep acting like it did before. +-- "When you change a primary key with ALTER PRIMARY KEY, the old primary key index becomes a secondary index." +-- We chose to use DROP CONSTRAINT and ADD CONSTRAINT instead and manually create the index. +-- https://www.cockroachlabs.com/docs/v22.1/primary-key#changing-primary-key-columns +CREATE UNIQUE INDEX IF NOT EXISTS console_session_token_unique ON omicron.public.console_session ( + token +); + /*******************************************************************/ -- Describes a single uploaded TUF repo. @@ -2801,7 +2810,8 @@ CREATE TABLE IF NOT EXISTS omicron.public.device_auth_request ( -- Access tokens granted in response to successful device authorization flows. CREATE TABLE IF NOT EXISTS omicron.public.device_access_token ( - token STRING(40) PRIMARY KEY, + id UUID PRIMARY KEY, + token STRING(40) NOT NULL, client_id UUID NOT NULL, device_code STRING(40) NOT NULL, silo_user_id UUID NOT NULL, @@ -2816,6 +2826,11 @@ CREATE UNIQUE INDEX IF NOT EXISTS lookup_device_access_token_by_client ON omicro client_id, device_code ); +-- We added a UUID as the primary key, but we need the token to keep acting like it did before +CREATE UNIQUE INDEX IF NOT EXISTS device_access_token_unique ON omicron.public.device_access_token ( + token +); + -- This index is used to remove tokens for a user that's being deleted. CREATE INDEX IF NOT EXISTS lookup_device_access_token_by_silo_user ON omicron.public.device_access_token ( silo_user_id @@ -5516,7 +5531,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - (TRUE, NOW(), NOW(), '141.0.0', NULL) + (TRUE, NOW(), NOW(), '142.0.0', NULL) ON CONFLICT DO NOTHING; COMMIT; diff --git a/schema/crdb/token-and-session-ids/up01.sql b/schema/crdb/token-and-session-ids/up01.sql new file mode 100644 index 00000000000..da315fa8e61 --- /dev/null +++ b/schema/crdb/token-and-session-ids/up01.sql @@ -0,0 +1,2 @@ +ALTER TABLE omicron.public.console_session + ADD COLUMN IF NOT EXISTS id UUID; diff --git a/schema/crdb/token-and-session-ids/up02.sql b/schema/crdb/token-and-session-ids/up02.sql new file mode 100644 index 00000000000..47b3453a00a --- /dev/null +++ b/schema/crdb/token-and-session-ids/up02.sql @@ -0,0 +1,5 @@ +set local disallow_full_table_scans = off; + +UPDATE omicron.public.console_session + SET id = gen_random_uuid() + WHERE id IS NULL; diff --git a/schema/crdb/token-and-session-ids/up03.sql b/schema/crdb/token-and-session-ids/up03.sql new file mode 100644 index 00000000000..ef2b58eabe5 --- /dev/null +++ b/schema/crdb/token-and-session-ids/up03.sql @@ -0,0 +1,2 @@ +ALTER TABLE omicron.public.console_session + ALTER COLUMN id SET NOT NULL; diff --git a/schema/crdb/token-and-session-ids/up04.sql b/schema/crdb/token-and-session-ids/up04.sql new file mode 100644 index 00000000000..e10588a194f --- /dev/null +++ b/schema/crdb/token-and-session-ids/up04.sql @@ -0,0 +1,14 @@ +-- the docs say to do both of these in the same transaction +-- https://www.cockroachlabs.com/docs/v22.1/add-constraint#drop-and-add-a-primary-key-constraint + +-- docs use the constraint name "primary" in the example, but that doesn't work +-- because it's actually called console_session_pkey, which I figured out with +-- +-- show create table omicron.public.console_session + +ALTER TABLE omicron.public.console_session + DROP CONSTRAINT IF EXISTS "console_session_pkey"; + +ALTER TABLE omicron.public.console_session + ADD CONSTRAINT "console_session_pkey" PRIMARY KEY (id); + diff --git a/schema/crdb/token-and-session-ids/up05.sql b/schema/crdb/token-and-session-ids/up05.sql new file mode 100644 index 00000000000..93c4d60cebc --- /dev/null +++ b/schema/crdb/token-and-session-ids/up05.sql @@ -0,0 +1,2 @@ +ALTER TABLE omicron.public.console_session + ALTER COLUMN token SET NOT NULL; diff --git a/schema/crdb/token-and-session-ids/up06.sql b/schema/crdb/token-and-session-ids/up06.sql new file mode 100644 index 00000000000..05c89866ce0 --- /dev/null +++ b/schema/crdb/token-and-session-ids/up06.sql @@ -0,0 +1,2 @@ +CREATE UNIQUE INDEX IF NOT EXISTS console_session_token_unique + ON omicron.public.console_session (token); diff --git a/schema/crdb/token-and-session-ids/up07.sql b/schema/crdb/token-and-session-ids/up07.sql new file mode 100644 index 00000000000..519727682f6 --- /dev/null +++ b/schema/crdb/token-and-session-ids/up07.sql @@ -0,0 +1,2 @@ +ALTER TABLE omicron.public.device_access_token + ADD COLUMN IF NOT EXISTS id UUID; diff --git a/schema/crdb/token-and-session-ids/up08.sql b/schema/crdb/token-and-session-ids/up08.sql new file mode 100644 index 00000000000..ec12fa91c33 --- /dev/null +++ b/schema/crdb/token-and-session-ids/up08.sql @@ -0,0 +1,5 @@ +set local disallow_full_table_scans = off; + +UPDATE omicron.public.device_access_token + SET id = gen_random_uuid() + WHERE id IS NULL; diff --git a/schema/crdb/token-and-session-ids/up09.sql b/schema/crdb/token-and-session-ids/up09.sql new file mode 100644 index 00000000000..8e7c543b4cf --- /dev/null +++ b/schema/crdb/token-and-session-ids/up09.sql @@ -0,0 +1,2 @@ +ALTER TABLE omicron.public.device_access_token + ALTER COLUMN id SET NOT NULL; diff --git a/schema/crdb/token-and-session-ids/up10.sql b/schema/crdb/token-and-session-ids/up10.sql new file mode 100644 index 00000000000..7ad697ea356 --- /dev/null +++ b/schema/crdb/token-and-session-ids/up10.sql @@ -0,0 +1,14 @@ +-- the docs say to do both of these in the same transaction +-- https://www.cockroachlabs.com/docs/v22.1/add-constraint#drop-and-add-a-primary-key-constraint + +-- docs use the constraint name "primary" in the example, but that doesn't work +-- because it's actually called console_session_pkey, which I figured out with +-- +-- show create table omicron.public.console_session + +ALTER TABLE omicron.public.device_access_token + DROP CONSTRAINT IF EXISTS "device_access_token_pkey"; + +ALTER TABLE omicron.public.device_access_token + ADD CONSTRAINT "device_access_token_pkey" PRIMARY KEY (id); + diff --git a/schema/crdb/token-and-session-ids/up11.sql b/schema/crdb/token-and-session-ids/up11.sql new file mode 100644 index 00000000000..c5bf7cfca73 --- /dev/null +++ b/schema/crdb/token-and-session-ids/up11.sql @@ -0,0 +1,2 @@ +ALTER TABLE omicron.public.device_access_token + ALTER COLUMN token SET NOT NULL; diff --git a/schema/crdb/token-and-session-ids/up12.sql b/schema/crdb/token-and-session-ids/up12.sql new file mode 100644 index 00000000000..de61688e0a5 --- /dev/null +++ b/schema/crdb/token-and-session-ids/up12.sql @@ -0,0 +1,2 @@ +CREATE UNIQUE INDEX IF NOT EXISTS device_access_token_unique + ON omicron.public.device_access_token (token); From c3d1152856d56abf4b71775c3efc59163fa900a0 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 9 May 2025 15:39:11 -0500 Subject: [PATCH 2/8] use direct datastore method for session lookup by token --- nexus/db-lookup/src/lookup.rs | 18 +++---------- .../src/db/datastore/console_session.rs | 21 +++++++++++++++ nexus/db-queries/src/db/datastore/mod.rs | 27 ++++++++++--------- nexus/src/app/session.rs | 7 ++--- 4 files changed, 41 insertions(+), 32 deletions(-) diff --git a/nexus/db-lookup/src/lookup.rs b/nexus/db-lookup/src/lookup.rs index 9e2172d191d..190281ce8e1 100644 --- a/nexus/db-lookup/src/lookup.rs +++ b/nexus/db-lookup/src/lookup.rs @@ -199,19 +199,9 @@ impl<'a> LookupPath<'a> { // Fleet-level resources - /// Select a resource of type ConsoleSession, identified by its `token` - pub fn console_session_token<'b, 'c>( - self, - token: &'b str, - ) -> ConsoleSession<'c> - where - 'a: 'c, - 'b: 'c, - { - ConsoleSession::PrimaryKey( - Root { lookup_root: self }, - token.to_string(), - ) + /// Select a resource of type ConsoleSession, identified by its `id` + pub fn console_session_id(self, id: Uuid) -> ConsoleSession<'a> { + ConsoleSession::PrimaryKey(Root { lookup_root: self }, id) } /// Select a resource of type DeviceAuthRequest, identified by its `user_code` @@ -762,7 +752,7 @@ lookup_resource! { lookup_by_name = false, soft_deletes = false, primary_key_columns = [ - { column_name = "token", rust_type = String }, + { column_name = "id", rust_type = Uuid }, ] } diff --git a/nexus/db-queries/src/db/datastore/console_session.rs b/nexus/db-queries/src/db/datastore/console_session.rs index c2dd14e5815..f05dfb9237d 100644 --- a/nexus/db-queries/src/db/datastore/console_session.rs +++ b/nexus/db-queries/src/db/datastore/console_session.rs @@ -13,13 +13,34 @@ use async_bb8_diesel::AsyncRunQueryDsl; use chrono::Utc; use diesel::prelude::*; use nexus_db_lookup::LookupPath; +use nexus_db_schema::schema::console_session; use omicron_common::api::external::CreateResult; use omicron_common::api::external::DeleteResult; use omicron_common::api::external::Error; use omicron_common::api::external::InternalContext; +use omicron_common::api::external::LookupResult; +use omicron_common::api::external::LookupType; +use omicron_common::api::external::ResourceType; use omicron_common::api::external::UpdateResult; impl DataStore { + pub async fn session_lookup_by_token( + &self, + opctx: &OpContext, + token: String, + ) -> LookupResult { + // TODO: some special system authz because the presence of the token _is_ the authz + console_session::table + .filter(console_session::token.eq(token)) + .select(ConsoleSession::as_returning()) + .get_result_async(&*self.pool_connection_authorized(opctx).await?) + .await + .map_err(|_e| Error::ObjectNotFound { + type_name: ResourceType::ConsoleSession, + lookup_type: LookupType::ByOther("session token".to_string()), + }) + } + // TODO-correctness: fix session method errors. the map_errs turn all errors // into 500s, most notably (and most frequently) session not found. they // don't end up as 500 in the http response because they get turned into a diff --git a/nexus/db-queries/src/db/datastore/mod.rs b/nexus/db-queries/src/db/datastore/mod.rs index fb57a603e7d..fe9d65dfe68 100644 --- a/nexus/db-queries/src/db/datastore/mod.rs +++ b/nexus/db-queries/src/db/datastore/mod.rs @@ -613,9 +613,8 @@ mod test { assert_eq!(DEFAULT_SILO_ID, db_silo_user.silo_id); // fetch the one we just created - let (.., fetched) = LookupPath::new(&opctx, datastore) - .console_session_token(&token) - .fetch() + let fetched = datastore + .session_lookup_by_token(&authn_opctx, token.clone()) .await .unwrap(); assert_eq!(session.silo_user_id, fetched.silo_user_id); @@ -642,10 +641,12 @@ mod test { renewed.console_session.time_last_used > session.time_last_used ); + // TODO: check the opctx on these changes, make sure we're using the + // right thing between opctx or authn_opctx + // time_last_used change persists in DB - let (.., fetched) = LookupPath::new(&opctx, datastore) - .console_session_token(&token) - .fetch() + let fetched = datastore + .session_lookup_by_token(&opctx, token.clone()) .await .unwrap(); assert!(fetched.time_last_used > session.time_last_used); @@ -656,9 +657,8 @@ mod test { let delete = datastore.session_hard_delete(&opctx, &authz_session).await; assert_eq!(delete, Ok(())); - let fetched = LookupPath::new(&opctx, datastore) - .console_session_token(&token) - .fetch() + let fetched = datastore + .session_lookup_by_token(&authn_opctx, token.clone()) .await; assert!(fetched.is_ok()); @@ -677,10 +677,11 @@ mod test { .session_hard_delete(&silo_user_opctx, &authz_session) .await; assert_eq!(delete, Ok(())); - let fetched = LookupPath::new(&opctx, datastore) - .console_session_token(&token) - .fetch() - .await; + let fetched = dbg!( + datastore + .session_lookup_by_token(&authn_opctx, token.clone()) + .await + ); assert!(matches!( fetched, Err(Error::ObjectNotFound { type_name: _, lookup_type: _ }) diff --git a/nexus/src/app/session.rs b/nexus/src/app/session.rs index 1418576b738..6d5197cd456 100644 --- a/nexus/src/app/session.rs +++ b/nexus/src/app/session.rs @@ -50,11 +50,8 @@ impl super::Nexus { opctx: &OpContext, token: String, ) -> LookupResult { - let (.., db_console_session) = - LookupPath::new(opctx, &self.db_datastore) - .console_session_token(&token) - .fetch() - .await?; + let db_console_session = + self.db_datastore.session_lookup_by_token(&opctx, token).await?; let (.., db_silo_user) = LookupPath::new(opctx, &self.db_datastore) .silo_user_id(db_console_session.silo_user_id) From 4e460322f406b4125f52718fa6f351329bd14426 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 9 May 2025 16:13:43 -0500 Subject: [PATCH 3/8] use TypedUuid for session and token IDs --- nexus/db-model/src/console_session.rs | 8 ++++++-- nexus/db-model/src/device_auth.rs | 7 +++++-- nexus/db-queries/src/db/datastore/mod.rs | 4 ++-- uuid-kinds/src/lib.rs | 2 ++ 4 files changed, 15 insertions(+), 6 deletions(-) diff --git a/nexus/db-model/src/console_session.rs b/nexus/db-model/src/console_session.rs index cec2d6a8ea3..b9ff31e29e8 100644 --- a/nexus/db-model/src/console_session.rs +++ b/nexus/db-model/src/console_session.rs @@ -4,14 +4,18 @@ use chrono::{DateTime, Utc}; use nexus_db_schema::schema::console_session; +use omicron_uuid_kinds::ConsoleSessionKind; +use omicron_uuid_kinds::TypedUuid; use uuid::Uuid; +use crate::typed_uuid::DbTypedUuid; + // TODO: `struct SessionToken(String)` for session token #[derive(Queryable, Insertable, Clone, Debug, Selectable)] #[diesel(table_name = console_session)] pub struct ConsoleSession { - pub id: Uuid, + pub id: DbTypedUuid, pub token: String, pub time_created: DateTime, pub time_last_used: DateTime, @@ -22,7 +26,7 @@ impl ConsoleSession { pub fn new(token: String, silo_user_id: Uuid) -> Self { let now = Utc::now(); Self { - id: Uuid::new_v4(), + id: TypedUuid::new_v4().into(), token, silo_user_id, time_last_used: now, diff --git a/nexus/db-model/src/device_auth.rs b/nexus/db-model/src/device_auth.rs index e31c75cc6ec..0ab8cd43d3e 100644 --- a/nexus/db-model/src/device_auth.rs +++ b/nexus/db-model/src/device_auth.rs @@ -11,9 +11,12 @@ use nexus_db_schema::schema::{device_access_token, device_auth_request}; use chrono::{DateTime, Duration, Utc}; use nexus_types::external_api::views; +use omicron_uuid_kinds::{AccessTokenKind, TypedUuid}; use rand::{Rng, RngCore, SeedableRng, distributions::Slice, rngs::StdRng}; use uuid::Uuid; +use crate::typed_uuid::DbTypedUuid; + /// Default timeout in seconds for client to authenticate for a token request. const CLIENT_AUTHENTICATION_TIMEOUT: i64 = 300; @@ -117,7 +120,7 @@ impl DeviceAuthRequest { #[derive(Clone, Debug, Insertable, Queryable, Selectable)] #[diesel(table_name = device_access_token)] pub struct DeviceAccessToken { - pub id: Uuid, + pub id: DbTypedUuid, pub token: String, pub client_id: Uuid, pub device_code: String, @@ -137,7 +140,7 @@ impl DeviceAccessToken { let now = Utc::now(); assert!(time_requested <= now); Self { - id: Uuid::new_v4(), + id: TypedUuid::new_v4().into(), token: generate_token(), client_id, device_code, diff --git a/nexus/db-queries/src/db/datastore/mod.rs b/nexus/db-queries/src/db/datastore/mod.rs index fe9d65dfe68..1b49ec3d1bd 100644 --- a/nexus/db-queries/src/db/datastore/mod.rs +++ b/nexus/db-queries/src/db/datastore/mod.rs @@ -482,12 +482,12 @@ mod test { ByteCount, Error, IdentityMetadataCreateParams, LookupType, Name, }; use omicron_test_utils::dev; - use omicron_uuid_kinds::CollectionUuid; use omicron_uuid_kinds::DatasetUuid; use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::PhysicalDiskUuid; use omicron_uuid_kinds::SledUuid; use omicron_uuid_kinds::VolumeUuid; + use omicron_uuid_kinds::{CollectionUuid, TypedUuid}; use std::collections::HashMap; use std::collections::HashSet; use std::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddrV6}; @@ -575,7 +575,7 @@ mod test { let silo_user_id = Uuid::new_v4(); let session = ConsoleSession { - id: Uuid::new_v4(), + id: TypedUuid::new_v4().into(), token: token.clone(), time_created: Utc::now() - Duration::minutes(5), time_last_used: Utc::now() - Duration::minutes(5), diff --git a/uuid-kinds/src/lib.rs b/uuid-kinds/src/lib.rs index 4b0faf83eb8..92442e9662c 100644 --- a/uuid-kinds/src/lib.rs +++ b/uuid-kinds/src/lib.rs @@ -51,10 +51,12 @@ macro_rules! impl_typed_uuid_kind { // Please keep this list in alphabetical order. impl_typed_uuid_kind! { + AccessToken => "access_token", AffinityGroup => "affinity_group", AntiAffinityGroup => "anti_affinity_group", Blueprint => "blueprint", Collection => "collection", + ConsoleSession => "console_session", Dataset => "dataset", DemoSaga => "demo_saga", Downstairs => "downstairs", From 7f377d8e1d3ea54172b39254ade5751cbe88afc5 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 9 May 2025 16:13:43 -0500 Subject: [PATCH 4/8] datastore method for access token lookup by token --- nexus/db-lookup/src/lookup.rs | 16 +++------------- .../db-queries/src/db/datastore/device_auth.rs | 18 ++++++++++++++++++ nexus/src/app/device_auth.rs | 6 +++--- 3 files changed, 24 insertions(+), 16 deletions(-) diff --git a/nexus/db-lookup/src/lookup.rs b/nexus/db-lookup/src/lookup.rs index 190281ce8e1..450c7e47dec 100644 --- a/nexus/db-lookup/src/lookup.rs +++ b/nexus/db-lookup/src/lookup.rs @@ -220,18 +220,8 @@ impl<'a> LookupPath<'a> { } /// 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::PrimaryKey( - Root { lookup_root: self }, - token.to_string(), - ) + pub fn device_access_token_id(self, id: Uuid) -> DeviceAccessToken<'a> { + DeviceAccessToken::PrimaryKey(Root { lookup_root: self }, id) } /// Select a resource of type RoleBuiltin, identified by its `name` @@ -772,7 +762,7 @@ lookup_resource! { lookup_by_name = false, soft_deletes = false, primary_key_columns = [ - { column_name = "token", rust_type = String }, + { column_name = "id", rust_type = Uuid }, ] } diff --git a/nexus/db-queries/src/db/datastore/device_auth.rs b/nexus/db-queries/src/db/datastore/device_auth.rs index 25b6e7c73c8..823e323ddef 100644 --- a/nexus/db-queries/src/db/datastore/device_auth.rs +++ b/nexus/db-queries/src/db/datastore/device_auth.rs @@ -13,6 +13,7 @@ use async_bb8_diesel::AsyncRunQueryDsl; use diesel::prelude::*; use nexus_db_errors::ErrorHandler; use nexus_db_errors::public_error_from_diesel; +use nexus_db_schema::schema::device_access_token; use omicron_common::api::external::CreateResult; use omicron_common::api::external::Error; use omicron_common::api::external::LookupResult; @@ -21,6 +22,23 @@ use omicron_common::api::external::ResourceType; use uuid::Uuid; impl DataStore { + pub async fn device_token_lookup_by_token( + &self, + opctx: &OpContext, + token: String, + ) -> LookupResult { + // TODO: some special system authz because the presence of the token _is_ the authz + device_access_token::table + .filter(device_access_token::token.eq(token)) + .select(DeviceAccessToken::as_returning()) + .get_result_async(&*self.pool_connection_authorized(opctx).await?) + .await + .map_err(|_e| Error::ObjectNotFound { + type_name: ResourceType::DeviceAccessToken, + lookup_type: LookupType::ByOther("access token".to_string()), + }) + } + /// Start a device authorization grant flow by recording the request /// and initial response parameters. pub async fn device_auth_request_create( diff --git a/nexus/src/app/device_auth.rs b/nexus/src/app/device_auth.rs index 6a48ce3d672..3e4d815bf59 100644 --- a/nexus/src/app/device_auth.rs +++ b/nexus/src/app/device_auth.rs @@ -166,9 +166,9 @@ impl super::Nexus { opctx: &OpContext, token: String, ) -> Result { - let (.., db_access_token) = LookupPath::new(opctx, &self.db_datastore) - .device_access_token(&token) - .fetch() + let db_access_token = self + .db_datastore + .device_token_lookup_by_token(opctx, token) .await .map_err(|e| match e { Error::ObjectNotFound { .. } => Reason::UnknownActor { From aa2705f8e8a5fafa112bf3104dbf29f6290af8e8 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 13 May 2025 13:52:28 -0500 Subject: [PATCH 5/8] update primary key in schema.rs, fix lookups to use typed uuids --- nexus/auth/src/authz/api_resources.rs | 2 +- nexus/db-lookup/src/lookup.rs | 18 ++++++++++++------ nexus/db-model/src/device_auth.rs | 5 ++--- nexus/db-queries/src/db/datastore/mod.rs | 10 ++++++++++ nexus/db-queries/src/policy_test/resources.rs | 9 ++++++--- nexus/db-queries/tests/output/authz-roles.out | 2 +- nexus/db-schema/src/schema.rs | 4 ++-- 7 files changed, 34 insertions(+), 16 deletions(-) diff --git a/nexus/auth/src/authz/api_resources.rs b/nexus/auth/src/authz/api_resources.rs index 7d9c17bd71c..9e1cd0f297c 100644 --- a/nexus/auth/src/authz/api_resources.rs +++ b/nexus/auth/src/authz/api_resources.rs @@ -957,7 +957,7 @@ authz_resource! { authz_resource! { name = "DeviceAccessToken", parent = "Fleet", - primary_key = String, // token + primary_key = { uuid_kind = AccessTokenKind }, roles_allowed = false, polar_snippet = FleetChild, } diff --git a/nexus/db-lookup/src/lookup.rs b/nexus/db-lookup/src/lookup.rs index 450c7e47dec..c94ed5ff7af 100644 --- a/nexus/db-lookup/src/lookup.rs +++ b/nexus/db-lookup/src/lookup.rs @@ -26,6 +26,8 @@ use nexus_types::identity::Resource; use omicron_common::api::external::Error; use omicron_common::api::external::InternalContext; use omicron_common::api::external::{LookupResult, LookupType, ResourceType}; +use omicron_uuid_kinds::AccessTokenKind; +use omicron_uuid_kinds::ConsoleSessionKind; use omicron_uuid_kinds::PhysicalDiskUuid; use omicron_uuid_kinds::SupportBundleUuid; use omicron_uuid_kinds::TufArtifactKind; @@ -200,7 +202,10 @@ impl<'a> LookupPath<'a> { // Fleet-level resources /// Select a resource of type ConsoleSession, identified by its `id` - pub fn console_session_id(self, id: Uuid) -> ConsoleSession<'a> { + pub fn console_session_id( + self, + id: TypedUuid, + ) -> ConsoleSession<'a> { ConsoleSession::PrimaryKey(Root { lookup_root: self }, id) } @@ -220,7 +225,10 @@ impl<'a> LookupPath<'a> { } /// Select a resource of type DeviceAccessToken, identified by its `token` - pub fn device_access_token_id(self, id: Uuid) -> DeviceAccessToken<'a> { + pub fn device_access_token_id( + self, + id: TypedUuid, + ) -> DeviceAccessToken<'a> { DeviceAccessToken::PrimaryKey(Root { lookup_root: self }, id) } @@ -742,7 +750,7 @@ lookup_resource! { lookup_by_name = false, soft_deletes = false, primary_key_columns = [ - { column_name = "id", rust_type = Uuid }, + { column_name = "id", uuid_kind = ConsoleSessionKind }, ] } @@ -761,9 +769,7 @@ lookup_resource! { ancestors = [], lookup_by_name = false, soft_deletes = false, - primary_key_columns = [ - { column_name = "id", rust_type = Uuid }, - ] + primary_key_columns = [ { column_name = "id", uuid_kind = AccessTokenKind } ] } lookup_resource! { diff --git a/nexus/db-model/src/device_auth.rs b/nexus/db-model/src/device_auth.rs index 0ab8cd43d3e..558d6ae03fa 100644 --- a/nexus/db-model/src/device_auth.rs +++ b/nexus/db-model/src/device_auth.rs @@ -151,9 +151,8 @@ impl DeviceAccessToken { } } - // TODO: uhhhh - pub fn id(&self) -> String { - self.token.clone() + pub fn id(&self) -> TypedUuid { + self.id.0 } pub fn expires(mut self, time: DateTime) -> Self { diff --git a/nexus/db-queries/src/db/datastore/mod.rs b/nexus/db-queries/src/db/datastore/mod.rs index 1b49ec3d1bd..ca408227c00 100644 --- a/nexus/db-queries/src/db/datastore/mod.rs +++ b/nexus/db-queries/src/db/datastore/mod.rs @@ -618,6 +618,16 @@ mod test { .await .unwrap(); assert_eq!(session.silo_user_id, fetched.silo_user_id); + assert_eq!(session.id, fetched.id); + + // also try looking it up by ID + let (.., fetched) = LookupPath::new(&opctx, datastore) + .console_session_id(session.id.into()) + .fetch() + .await + .unwrap(); + assert_eq!(session.silo_user_id, fetched.silo_user_id); + assert_eq!(session.token, fetched.token); // trying to insert the same one again fails let duplicate = diff --git a/nexus/db-queries/src/policy_test/resources.rs b/nexus/db-queries/src/policy_test/resources.rs index 6853288ff09..6fcc103350b 100644 --- a/nexus/db-queries/src/policy_test/resources.rs +++ b/nexus/db-queries/src/policy_test/resources.rs @@ -8,9 +8,11 @@ use super::resource_builder::ResourceBuilder; use super::resource_builder::ResourceSet; use nexus_auth::authz; use omicron_common::api::external::LookupType; +use omicron_uuid_kinds::AccessTokenKind; use omicron_uuid_kinds::GenericUuid; use omicron_uuid_kinds::PhysicalDiskUuid; use omicron_uuid_kinds::SupportBundleUuid; +use omicron_uuid_kinds::TypedUuid; use oso::PolarClass; use std::collections::BTreeSet; use uuid::Uuid; @@ -127,11 +129,12 @@ pub async fn make_resources( LookupType::ByName(device_user_code), )); - let device_access_token = String::from("a-device-access-token"); + let device_access_token_id: TypedUuid = + "3b80c7f9-bee0-4b42-8550-6cdfc74dafdb".parse().unwrap(); builder.new_resource(authz::DeviceAccessToken::new( authz::FLEET, - device_access_token.clone(), - LookupType::ByName(device_access_token), + device_access_token_id, + LookupType::ById(device_access_token_id.into_untyped_uuid()), )); let blueprint_id = "b9e923f6-caf3-4c83-96f9-8ffe8c627dd2".parse().unwrap(); diff --git a/nexus/db-queries/tests/output/authz-roles.out b/nexus/db-queries/tests/output/authz-roles.out index e83cacbe3a9..c70031a1b2b 100644 --- a/nexus/db-queries/tests/output/authz-roles.out +++ b/nexus/db-queries/tests/output/authz-roles.out @@ -1174,7 +1174,7 @@ resource: DeviceAuthRequest "a-device-user-code" silo1-proj1-viewer ✘ ✔ ✘ ✔ ✔ ✔ ✘ ✔ unauthenticated ! ! ! ! ! ! ! ! -resource: DeviceAccessToken "a-device-access-token" +resource: DeviceAccessToken id "3b80c7f9-bee0-4b42-8550-6cdfc74dafdb" USER Q R LC RP M MP CC D fleet-admin ✘ ✔ ✔ ✔ ✔ ✔ ✔ ✔ diff --git a/nexus/db-schema/src/schema.rs b/nexus/db-schema/src/schema.rs index 9eb2e1856d0..8846b863538 100644 --- a/nexus/db-schema/src/schema.rs +++ b/nexus/db-schema/src/schema.rs @@ -928,7 +928,7 @@ table! { } table! { - console_session (token) { + console_session (id) { id -> Uuid, token -> Text, time_created -> Timestamptz, @@ -1355,7 +1355,7 @@ table! { } table! { - device_access_token (token) { + device_access_token (id) { id -> Uuid, token -> Text, client_id -> Uuid, From 2f7d36f1a10641bef3c9e9debb40020dbc6ba4e4 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 13 May 2025 17:00:52 -0500 Subject: [PATCH 6/8] update session methods to operator on ID instead of token --- .../auth/src/authn/external/session_cookie.rs | 125 ++++++++++-------- nexus/auth/src/authz/api_resources.rs | 2 +- nexus/auth/src/context.rs | 6 + nexus/db-lookup/src/lookup.rs | 4 +- nexus/db-model/src/console_session.rs | 4 +- .../src/db/datastore/console_session.rs | 5 +- nexus/db-queries/src/db/datastore/mod.rs | 12 +- nexus/src/app/session.rs | 15 ++- nexus/src/context.rs | 13 +- nexus/src/external_api/http_entrypoints.rs | 15 ++- nexus/tests/integration_tests/authn_http.rs | 59 +++++---- 11 files changed, 159 insertions(+), 101 deletions(-) diff --git a/nexus/auth/src/authn/external/session_cookie.rs b/nexus/auth/src/authn/external/session_cookie.rs index 74ae1e13666..08be5b4d001 100644 --- a/nexus/auth/src/authn/external/session_cookie.rs +++ b/nexus/auth/src/authn/external/session_cookie.rs @@ -13,6 +13,7 @@ use chrono::{DateTime, Duration, Utc}; use dropshot::HttpError; use http::HeaderValue; use nexus_types::authn::cookies::parse_cookies; +use omicron_uuid_kinds::{ConsoleSessionKind, TypedUuid}; use slog::debug; use uuid::Uuid; @@ -20,6 +21,7 @@ use uuid::Uuid; // https://cheatsheetseries.owasp.org/cheatsheets/Session_Management_Cheat_Sheet.html pub trait Session { + fn id(&self) -> TypedUuid; fn silo_user_id(&self) -> Uuid; fn silo_id(&self) -> Uuid; fn time_last_used(&self) -> DateTime; @@ -39,11 +41,14 @@ pub trait SessionStore { /// Extend session by updating time_last_used to now async fn session_update_last_used( &self, - token: String, + id: TypedUuid, ) -> Option; /// Mark session expired - async fn session_expire(&self, token: String) -> Option<()>; + async fn session_expire( + &self, + id: TypedUuid, + ) -> Option<()>; /// Maximum time session can remain idle before expiring fn session_idle_timeout(&self) -> Duration; @@ -131,7 +136,7 @@ where // expired let now = Utc::now(); if session.time_last_used() + ctx.session_idle_timeout() < now { - let expired_session = ctx.session_expire(token.clone()).await; + let expired_session = ctx.session_expire(session.id()).await; if expired_session.is_none() { debug!(log, "failed to expire session") } @@ -151,7 +156,7 @@ where // existed longer than absolute_timeout, it is expired and we can no // longer extend the session if session.time_created() + ctx.session_absolute_timeout() < now { - let expired_session = ctx.session_expire(token.clone()).await; + let expired_session = ctx.session_expire(session.id()).await; if expired_session.is_none() { debug!(log, "failed to expire session") } @@ -172,7 +177,7 @@ where // authenticated for this request at this point. The next request might // be wrongly considered idle, but that's a problem for the next // request. - let updated_session = ctx.session_update_last_used(token).await; + let updated_session = ctx.session_update_last_used(session.id()).await; if updated_session.is_none() { debug!(log, "failed to extend session") } @@ -199,19 +204,22 @@ mod test { use async_trait::async_trait; use chrono::{DateTime, Duration, Utc}; use http; + use omicron_uuid_kinds::ConsoleSessionKind; + use omicron_uuid_kinds::TypedUuid; use slog; - use std::collections::HashMap; use std::sync::Mutex; use uuid::Uuid; // the mutex is annoying, but we need it in order to mutate the hashmap // without passing TestServerContext around as mutable struct TestServerContext { - sessions: Mutex>, + sessions: Mutex>, } - #[derive(Clone, Copy)] + #[derive(Clone)] struct FakeSession { + id: TypedUuid, + token: String, silo_user_id: Uuid, silo_id: Uuid, time_created: DateTime, @@ -219,6 +227,9 @@ mod test { } impl Session for FakeSession { + fn id(&self) -> TypedUuid { + self.id + } fn silo_user_id(&self) -> Uuid { self.silo_user_id } @@ -241,23 +252,37 @@ mod test { &self, token: String, ) -> Option { - self.sessions.lock().unwrap().get(&token).map(|s| *s) + self.sessions + .lock() + .unwrap() + .iter() + .find(|s| s.token == token) + .map(|s| s.clone()) } async fn session_update_last_used( &self, - token: String, + id: TypedUuid, ) -> Option { let mut sessions = self.sessions.lock().unwrap(); - let session = *sessions.get(&token).unwrap(); - let new_session = - FakeSession { time_last_used: Utc::now(), ..session }; - (*sessions).insert(token, new_session) + if let Some(pos) = sessions.iter().position(|s| s.id == id) { + let new_session = FakeSession { + time_last_used: Utc::now(), + ..sessions[pos].clone() + }; + sessions[pos] = new_session.clone(); + Some(new_session) + } else { + None + } } - async fn session_expire(&self, token: String) -> Option<()> { + async fn session_expire( + &self, + id: TypedUuid, + ) -> Option<()> { let mut sessions = self.sessions.lock().unwrap(); - (*sessions).remove(&token); + sessions.retain(|s| s.id != id); Some(()) } @@ -295,16 +320,14 @@ mod test { #[tokio::test] async fn test_missing_cookie() { - let context = - TestServerContext { sessions: Mutex::new(HashMap::new()) }; + let context = TestServerContext { sessions: Mutex::new(Vec::new()) }; let result = authn_with_cookie(&context, None).await; assert!(matches!(result, SchemeResult::NotRequested)); } #[tokio::test] async fn test_other_cookie() { - let context = - TestServerContext { sessions: Mutex::new(HashMap::new()) }; + let context = TestServerContext { sessions: Mutex::new(Vec::new()) }; let result = authn_with_cookie(&context, Some("other=def")).await; assert!(matches!(result, SchemeResult::NotRequested)); } @@ -312,15 +335,14 @@ mod test { #[tokio::test] async fn test_expired_cookie_idle() { let context = TestServerContext { - sessions: Mutex::new(HashMap::from([( - "abc".to_string(), - FakeSession { - silo_user_id: Uuid::new_v4(), - silo_id: Uuid::new_v4(), - time_last_used: Utc::now() - Duration::hours(2), - time_created: Utc::now() - Duration::hours(2), - }, - )])), + sessions: Mutex::new(vec![FakeSession { + id: TypedUuid::new_v4(), + token: "abc".to_string(), + silo_user_id: Uuid::new_v4(), + silo_id: Uuid::new_v4(), + time_last_used: Utc::now() - Duration::hours(2), + time_created: Utc::now() - Duration::hours(2), + }]), }; let result = authn_with_cookie(&context, Some("session=abc")).await; assert!(matches!( @@ -332,21 +354,21 @@ mod test { )); // key should be removed from sessions dict, i.e., session deleted - assert!(!context.sessions.lock().unwrap().contains_key("abc")) + let sessions = context.sessions.lock().unwrap(); + assert!(!sessions.iter().any(|s| s.token == "abc")) } #[tokio::test] async fn test_expired_cookie_absolute() { let context = TestServerContext { - sessions: Mutex::new(HashMap::from([( - "abc".to_string(), - FakeSession { - silo_user_id: Uuid::new_v4(), - silo_id: Uuid::new_v4(), - time_last_used: Utc::now(), - time_created: Utc::now() - Duration::hours(20), - }, - )])), + sessions: Mutex::new(vec![FakeSession { + id: TypedUuid::new_v4(), + token: "abc".to_string(), + silo_user_id: Uuid::new_v4(), + silo_id: Uuid::new_v4(), + time_last_used: Utc::now(), + time_created: Utc::now() - Duration::hours(20), + }]), }; let result = authn_with_cookie(&context, Some("session=abc")).await; assert!(matches!( @@ -359,22 +381,21 @@ mod test { // key should be removed from sessions dict, i.e., session deleted let sessions = context.sessions.lock().unwrap(); - assert!(!sessions.contains_key("abc")) + assert!(!sessions.iter().any(|s| s.token == "abc")) } #[tokio::test] async fn test_valid_cookie() { let time_last_used = Utc::now() - Duration::seconds(5); let context = TestServerContext { - sessions: Mutex::new(HashMap::from([( - "abc".to_string(), - FakeSession { - silo_user_id: Uuid::new_v4(), - silo_id: Uuid::new_v4(), - time_last_used, - time_created: Utc::now(), - }, - )])), + sessions: Mutex::new(vec![FakeSession { + id: TypedUuid::new_v4(), + token: "abc".to_string(), + silo_user_id: Uuid::new_v4(), + silo_id: Uuid::new_v4(), + time_last_used, + time_created: Utc::now(), + }]), }; let result = authn_with_cookie(&context, Some("session=abc")).await; assert!(matches!( @@ -384,13 +405,13 @@ mod test { // valid cookie should have updated time_last_used let sessions = context.sessions.lock().unwrap(); - assert!(sessions.get("abc").unwrap().time_last_used > time_last_used) + let session = sessions.iter().find(|s| s.token == "abc").unwrap(); + assert!(session.time_last_used > time_last_used) } #[tokio::test] async fn test_garbage_cookie() { - let context = - TestServerContext { sessions: Mutex::new(HashMap::new()) }; + let context = TestServerContext { sessions: Mutex::new(Vec::new()) }; let result = authn_with_cookie(&context, Some("unparsable garbage!!!!!1")).await; assert!(matches!(result, SchemeResult::NotRequested)); diff --git a/nexus/auth/src/authz/api_resources.rs b/nexus/auth/src/authz/api_resources.rs index 9e1cd0f297c..646c6ff9b05 100644 --- a/nexus/auth/src/authz/api_resources.rs +++ b/nexus/auth/src/authz/api_resources.rs @@ -941,7 +941,7 @@ authz_resource! { authz_resource! { name = "ConsoleSession", parent = "Fleet", - primary_key = String, + primary_key = { uuid_kind = ConsoleSessionKind }, roles_allowed = false, polar_snippet = FleetChild, } diff --git a/nexus/auth/src/context.rs b/nexus/auth/src/context.rs index 02785f9247c..03ab4455b53 100644 --- a/nexus/auth/src/context.rs +++ b/nexus/auth/src/context.rs @@ -11,6 +11,8 @@ use crate::authz::AuthorizedResource; use crate::storage::Storage; use chrono::{DateTime, Utc}; use omicron_common::api::external::Error; +use omicron_uuid_kinds::ConsoleSessionKind; +use omicron_uuid_kinds::TypedUuid; use slog::debug; use slog::o; use slog::trace; @@ -352,6 +354,10 @@ impl OpContext { } impl Session for ConsoleSessionWithSiloId { + fn id(&self) -> TypedUuid { + self.console_session.id() + } + fn silo_user_id(&self) -> Uuid { self.console_session.silo_user_id } diff --git a/nexus/db-lookup/src/lookup.rs b/nexus/db-lookup/src/lookup.rs index c94ed5ff7af..61a4d3ada06 100644 --- a/nexus/db-lookup/src/lookup.rs +++ b/nexus/db-lookup/src/lookup.rs @@ -749,9 +749,7 @@ lookup_resource! { ancestors = [], lookup_by_name = false, soft_deletes = false, - primary_key_columns = [ - { column_name = "id", uuid_kind = ConsoleSessionKind }, - ] + primary_key_columns = [ { column_name = "id", uuid_kind = ConsoleSessionKind } ] } lookup_resource! { diff --git a/nexus/db-model/src/console_session.rs b/nexus/db-model/src/console_session.rs index b9ff31e29e8..838637ff994 100644 --- a/nexus/db-model/src/console_session.rs +++ b/nexus/db-model/src/console_session.rs @@ -34,7 +34,7 @@ impl ConsoleSession { } } - pub fn id(&self) -> String { - self.token.clone() + pub fn id(&self) -> TypedUuid { + self.id.0 } } diff --git a/nexus/db-queries/src/db/datastore/console_session.rs b/nexus/db-queries/src/db/datastore/console_session.rs index f05dfb9237d..79ee9ab8a3b 100644 --- a/nexus/db-queries/src/db/datastore/console_session.rs +++ b/nexus/db-queries/src/db/datastore/console_session.rs @@ -22,6 +22,7 @@ use omicron_common::api::external::LookupResult; use omicron_common::api::external::LookupType; use omicron_common::api::external::ResourceType; use omicron_common::api::external::UpdateResult; +use omicron_uuid_kinds::GenericUuid; impl DataStore { pub async fn session_lookup_by_token( @@ -85,7 +86,7 @@ impl DataStore { use nexus_db_schema::schema::console_session::dsl; let console_session = diesel::update(dsl::console_session) - .filter(dsl::token.eq(authz_session.id())) + .filter(dsl::id.eq(authz_session.id().into_untyped_uuid())) .set((dsl::time_last_used.eq(Utc::now()),)) .returning(ConsoleSession::as_returning()) .get_result_async(&*self.pool_connection_authorized(opctx).await?) @@ -149,7 +150,7 @@ impl DataStore { use nexus_db_schema::schema::console_session::dsl; diesel::delete(dsl::console_session) .filter(dsl::silo_user_id.eq(silo_user_id)) - .filter(dsl::token.eq(authz_session.id())) + .filter(dsl::id.eq(authz_session.id().into_untyped_uuid())) .execute_async(&*self.pool_connection_authorized(opctx).await?) .await .map(|_rows_deleted| ()) diff --git a/nexus/db-queries/src/db/datastore/mod.rs b/nexus/db-queries/src/db/datastore/mod.rs index ca408227c00..5c097e4adb3 100644 --- a/nexus/db-queries/src/db/datastore/mod.rs +++ b/nexus/db-queries/src/db/datastore/mod.rs @@ -640,8 +640,8 @@ mod test { // update last used (i.e., renew token) let authz_session = authz::ConsoleSession::new( authz::FLEET, - token.clone(), - LookupType::ByCompositeId(token.clone()), + session.id.into(), + LookupType::ById(session.id.into_untyped_uuid()), ); let renewed = datastore .session_update_last_used(&opctx, &authz_session) @@ -687,11 +687,9 @@ mod test { .session_hard_delete(&silo_user_opctx, &authz_session) .await; assert_eq!(delete, Ok(())); - let fetched = dbg!( - datastore - .session_lookup_by_token(&authn_opctx, token.clone()) - .await - ); + let fetched = datastore + .session_lookup_by_token(&authn_opctx, token.clone()) + .await; assert!(matches!( fetched, Err(Error::ObjectNotFound { type_name: _, lookup_type: _ }) diff --git a/nexus/src/app/session.rs b/nexus/src/app/session.rs index 6d5197cd456..5f391fa2a63 100644 --- a/nexus/src/app/session.rs +++ b/nexus/src/app/session.rs @@ -18,6 +18,9 @@ use omicron_common::api::external::Error; use omicron_common::api::external::LookupResult; use omicron_common::api::external::LookupType; use omicron_common::api::external::UpdateResult; +use omicron_uuid_kinds::ConsoleSessionKind; +use omicron_uuid_kinds::GenericUuid; +use omicron_uuid_kinds::TypedUuid; use rand::{RngCore, SeedableRng, rngs::StdRng}; use uuid::Uuid; @@ -68,12 +71,12 @@ impl super::Nexus { pub(crate) async fn session_update_last_used( &self, opctx: &OpContext, - token: &str, + id: TypedUuid, ) -> UpdateResult { let authz_session = authz::ConsoleSession::new( authz::FLEET, - token.to_string(), - LookupType::ByCompositeId(token.to_string()), + id, + LookupType::ById(id.into_untyped_uuid()), ); self.db_datastore.session_update_last_used(opctx, &authz_session).await } @@ -81,12 +84,12 @@ impl super::Nexus { pub(crate) async fn session_hard_delete( &self, opctx: &OpContext, - token: &str, + id: TypedUuid, ) -> DeleteResult { let authz_session = authz::ConsoleSession::new( authz::FLEET, - token.to_string(), - LookupType::ByCompositeId(token.to_string()), + id, + LookupType::ById(id.into_untyped_uuid()), ); self.db_datastore.session_hard_delete(opctx, &authz_session).await } diff --git a/nexus/src/context.rs b/nexus/src/context.rs index a351adb80e6..0717d7c1803 100644 --- a/nexus/src/context.rs +++ b/nexus/src/context.rs @@ -19,7 +19,9 @@ use nexus_db_queries::authn::external::session_cookie::SessionStore; use nexus_db_queries::context::{OpContext, OpKind}; use nexus_db_queries::{authn, authz, db}; use omicron_common::address::{AZ_PREFIX, Ipv6Subnet}; +use omicron_uuid_kinds::ConsoleSessionKind; use omicron_uuid_kinds::GenericUuid; +use omicron_uuid_kinds::TypedUuid; use oximeter::types::ProducerRegistry; use oximeter_instruments::http::{HttpService, LatencyTracker}; use slog::Logger; @@ -470,15 +472,18 @@ impl SessionStore for ServerContext { async fn session_update_last_used( &self, - token: String, + id: TypedUuid, ) -> Option { let opctx = self.nexus.opctx_external_authn(); - self.nexus.session_update_last_used(&opctx, &token).await.ok() + self.nexus.session_update_last_used(&opctx, id).await.ok() } - async fn session_expire(&self, token: String) -> Option<()> { + async fn session_expire( + &self, + id: TypedUuid, + ) -> Option<()> { let opctx = self.nexus.opctx_external_authn(); - self.nexus.session_hard_delete(opctx, &token).await.ok() + self.nexus.session_hard_delete(opctx, id).await.ok() } fn session_idle_timeout(&self) -> Duration { diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index ec62cffcd7b..c3230c5be26 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -7569,7 +7569,20 @@ impl NexusExternalApi for NexusExternalApiImpl { if let Ok(opctx) = opctx { if let Some(token) = token { - nexus.session_hard_delete(&opctx, token.value()).await?; + // TODO: This is the ONE spot where we do the hard delete + // by token and we haven't already looked up the session + // by token. Looking up the token first works but it would + // be nice to avoid it + + // look up session and delete it if present. noop on any errors + let session = nexus + .session_fetch(&opctx, token.value().to_string()) + .await; + if let Ok(session) = session { + let session_id = session.console_session.id(); + let _ = + nexus.session_hard_delete(&opctx, session_id).await; + } } } diff --git a/nexus/tests/integration_tests/authn_http.rs b/nexus/tests/integration_tests/authn_http.rs index 27094f792fc..7f88051be49 100644 --- a/nexus/tests/integration_tests/authn_http.rs +++ b/nexus/tests/integration_tests/authn_http.rs @@ -27,7 +27,8 @@ use nexus_db_queries::authn::external::spoof; use nexus_db_queries::authn::external::spoof::HttpAuthnSpoof; use nexus_db_queries::authn::external::spoof::SPOOF_SCHEME_NAME; use nexus_types::silo::DEFAULT_SILO_ID; -use std::collections::HashMap; +use omicron_uuid_kinds::ConsoleSessionKind; +use omicron_uuid_kinds::TypedUuid; use std::sync::Mutex; use uuid::Uuid; @@ -42,12 +43,9 @@ async fn test_authn_spoof_allowed() { let authn_schemes_configured: Vec< Box + 'static>, > = vec![Box::new(HttpAuthnSpoof)]; - let testctx = start_whoami_server( - test_name, - authn_schemes_configured, - HashMap::new(), - ) - .await; + let testctx = + start_whoami_server(test_name, authn_schemes_configured, Vec::new()) + .await; let tried_spoof = [SPOOF_SCHEME_NAME] .iter() .map(|s| s.to_string()) @@ -108,18 +106,24 @@ async fn test_authn_session_cookie() { Box + 'static>, > = vec![Box::new(session_cookie::HttpAuthnSessionCookie)]; let valid_session = FakeSession { + id: TypedUuid::new_v4(), + token: "valid".to_string(), silo_user_id: Uuid::new_v4(), silo_id: Uuid::new_v4(), time_last_used: Utc::now() - Duration::seconds(5), time_created: Utc::now() - Duration::seconds(5), }; let idle_expired_session = FakeSession { + id: TypedUuid::new_v4(), + token: "idle_expired".to_string(), silo_user_id: Uuid::new_v4(), silo_id: Uuid::new_v4(), time_last_used: Utc::now() - Duration::hours(2), time_created: Utc::now() - Duration::hours(3), }; let abs_expired_session = FakeSession { + id: TypedUuid::new_v4(), + token: "abs_expired".to_string(), silo_user_id: Uuid::new_v4(), silo_id: Uuid::new_v4(), time_last_used: Utc::now(), @@ -128,11 +132,7 @@ async fn test_authn_session_cookie() { let testctx = start_whoami_server( test_name, authn_schemes_configured, - HashMap::from([ - ("valid".to_string(), valid_session), - ("idle_expired".to_string(), idle_expired_session), - ("abs_expired".to_string(), abs_expired_session), - ]), + vec![valid_session.clone(), idle_expired_session, abs_expired_session], ) .await; @@ -192,8 +192,7 @@ async fn test_authn_session_cookie() { #[tokio::test] async fn test_authn_spoof_unconfigured() { let test_name = "test_authn_spoof_disallowed"; - let testctx = - start_whoami_server(test_name, Vec::new(), HashMap::new()).await; + let testctx = start_whoami_server(test_name, Vec::new(), Vec::new()).await; let values = [ None, @@ -283,7 +282,7 @@ fn assert_authn_failed( async fn start_whoami_server( test_name: &str, authn_schemes_configured: Vec>>, - sessions: HashMap, + sessions: Vec, ) -> TestContext { let config = nexus_test_utils::load_test_config(); let logctx = LogContext::new(test_name, &config.pkg.log); @@ -316,7 +315,7 @@ async fn start_whoami_server( struct WhoamiServerState { authn: nexus_db_queries::authn::external::Authenticator, - sessions: Mutex>, + sessions: Mutex>, } #[async_trait] @@ -346,8 +345,10 @@ impl SiloUserSilo for WhoamiServerState { } } -#[derive(Clone, Copy)] +#[derive(Clone)] struct FakeSession { + id: TypedUuid, + token: String, silo_user_id: Uuid, silo_id: Uuid, time_created: DateTime, @@ -355,6 +356,9 @@ struct FakeSession { } impl session_cookie::Session for FakeSession { + fn id(&self) -> TypedUuid { + self.id + } fn silo_user_id(&self) -> Uuid { self.silo_user_id } @@ -374,22 +378,31 @@ impl session_cookie::SessionStore for WhoamiServerState { type SessionModel = FakeSession; async fn session_fetch(&self, token: String) -> Option { - self.sessions.lock().unwrap().get(&token).map(|s| *s) + self.sessions + .lock() + .unwrap() + .iter() + .find(|s| s.token == token) + .map(|s| s.clone()) } async fn session_update_last_used( &self, - token: String, + id: TypedUuid, ) -> Option { let mut sessions = self.sessions.lock().unwrap(); - let session = *sessions.get(&token).unwrap(); + let session = sessions.iter().find(|s| s.id == id).unwrap().clone(); let new_session = FakeSession { time_last_used: Utc::now(), ..session }; - (*sessions).insert(token, new_session) + (*sessions).push(new_session.clone()); + Some(new_session) } - async fn session_expire(&self, token: String) -> Option<()> { + async fn session_expire( + &self, + id: TypedUuid, + ) -> Option<()> { let mut sessions = self.sessions.lock().unwrap(); - (*sessions).remove(&token); + sessions.retain(|s| s.id != id); Some(()) } From 6c476439a5760ac99a2d9a3dd259436d2de5a47f Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 15 May 2025 14:54:13 -0500 Subject: [PATCH 7/8] make both FakeSession things work the same --- nexus/db-lookup/src/lookup.rs | 2 +- nexus/tests/integration_tests/authn_http.rs | 14 ++++++++++---- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/nexus/db-lookup/src/lookup.rs b/nexus/db-lookup/src/lookup.rs index 61a4d3ada06..fa1ca9b5bb9 100644 --- a/nexus/db-lookup/src/lookup.rs +++ b/nexus/db-lookup/src/lookup.rs @@ -224,7 +224,7 @@ impl<'a> LookupPath<'a> { ) } - /// Select a resource of type DeviceAccessToken, identified by its `token` + /// Select a resource of type DeviceAccessToken, identified by its `id` pub fn device_access_token_id( self, id: TypedUuid, diff --git a/nexus/tests/integration_tests/authn_http.rs b/nexus/tests/integration_tests/authn_http.rs index 7f88051be49..bde21eb03ee 100644 --- a/nexus/tests/integration_tests/authn_http.rs +++ b/nexus/tests/integration_tests/authn_http.rs @@ -391,10 +391,16 @@ impl session_cookie::SessionStore for WhoamiServerState { id: TypedUuid, ) -> Option { let mut sessions = self.sessions.lock().unwrap(); - let session = sessions.iter().find(|s| s.id == id).unwrap().clone(); - let new_session = FakeSession { time_last_used: Utc::now(), ..session }; - (*sessions).push(new_session.clone()); - Some(new_session) + if let Some(pos) = sessions.iter().position(|s| s.id == id) { + let new_session = FakeSession { + time_last_used: Utc::now(), + ..sessions[pos].clone() + }; + sessions[pos] = new_session.clone(); + Some(new_session) + } else { + None + } } async fn session_expire( From c6cda5b9d4eff44a16e986e49f35d6425c5e4b2d Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 16 May 2025 15:07:24 -0500 Subject: [PATCH 8/8] migration before/after tests --- nexus/tests/integration_tests/schema.rs | 65 ++++++++++++++++++++++--- 1 file changed, 59 insertions(+), 6 deletions(-) diff --git a/nexus/tests/integration_tests/schema.rs b/nexus/tests/integration_tests/schema.rs index bd44db804e0..9292f6cee43 100644 --- a/nexus/tests/integration_tests/schema.rs +++ b/nexus/tests/integration_tests/schema.rs @@ -2187,14 +2187,67 @@ fn after_140_0_0<'a>(ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> { }) } -// TODO, obviously - -fn before_142_0_0<'a>(_ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> { - Box::pin(async {}) +fn before_142_0_0<'a>(ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> { + Box::pin(async move { + // Create one console_session without id, and one device_access_token without id. + ctx.client + .batch_execute( + " + INSERT INTO omicron.public.console_session + (token, time_created, time_last_used, silo_user_id) + VALUES + ('tok-console-142', now(), now(), gen_random_uuid()); + + INSERT INTO omicron.public.device_access_token + (token, client_id, device_code, silo_user_id, time_created, time_requested) + VALUES + ('tok-device-142', gen_random_uuid(), 'code-142', gen_random_uuid(), now(), now()); + ", + ) + .await + .expect("failed to insert pre-migration rows for 142"); + }) } -fn after_142_0_0<'a>(_ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> { - Box::pin(async {}) +fn after_142_0_0<'a>(ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> { + Box::pin(async move { + // After the migration each row should have a non-null id, + // keep its token, and enforce primary-key/unique index. + + // console_session: check id ≠ NULL and token unchanged + let rows = ctx + .client + .query( + "SELECT id, token FROM omicron.public.console_session WHERE token = 'tok-console-142';", + &[], + ) + .await + .expect("failed to query post-migration console_session"); + assert_eq!(rows.len(), 1); + + let id: Option = (&rows[0]).get("id"); + assert!(id.is_some()); + + let token: &str = (&rows[0]).get("token"); + assert_eq!(token, "tok-console-142"); + + // device_access_token: same checks + let rows = ctx + .client + .query( + "SELECT id, token FROM omicron.public.device_access_token WHERE token = 'tok-device-142';", + &[], + ) + .await + .expect("failed to query post-migration device_access_token"); + assert_eq!(rows.len(), 1); + + let id: Option = (&rows[0]).get("id"); + assert!(id.is_some()); + + let token: &str = (&rows[0]).get("token"); + assert_eq!(token, "tok-device-142",); + }) } // Lazily initializes all migration checks. The combination of Rust function