From fe237ec287d89bf7fa21cc2b76033a3226eb3feb Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 9 May 2025 10:34:08 -0500 Subject: [PATCH 01/11] 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 31957d82b3e..d5d3a8af2ba 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 3a787f4e141..6f85bc0b6d7 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(144, 0, 0); +pub const SCHEMA_VERSION: Version = Version::new(145, 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(145, "token-and-session-ids"), KnownVersion::new(144, "inventory-omicron-sled-config"), KnownVersion::new(143, "alerts-renamening"), KnownVersion::new(142, "bp-add-remove-mupdate-override"), diff --git a/nexus/db-queries/src/db/datastore/mod.rs b/nexus/db-queries/src/db/datastore/mod.rs index 123ee6c97e9..29921acaef6 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 9875d3bc395..877fad40ed7 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 1fa3651dd74..1ba48a46cf7 100644 --- a/nexus/tests/integration_tests/schema.rs +++ b/nexus/tests/integration_tests/schema.rs @@ -2198,6 +2198,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. @@ -2262,6 +2272,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(145, 0, 0), + DataMigrationFns::new().before(before_145_0_0).after(after_145_0_0), + ); map } diff --git a/schema/crdb/dbinit.sql b/schema/crdb/dbinit.sql index 257db58c75f..82e32338157 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 @@ -5671,7 +5686,7 @@ INSERT INTO omicron.public.db_metadata ( version, target_version ) VALUES - (TRUE, NOW(), NOW(), '144.0.0', NULL) + (TRUE, NOW(), NOW(), '145.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 5f6c956f5a2f93fd06bc7ee2d88d6618f268c643 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 9 May 2025 15:39:11 -0500 Subject: [PATCH 02/11] 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 a73f09f232e..4fb5d58cf9a 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 29921acaef6..4518fd65e23 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 abbe6117308b4cf0c470be83835337481ce79fbd Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 9 May 2025 16:13:43 -0500 Subject: [PATCH 03/11] 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 d5d3a8af2ba..7e2bf3e6f64 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 4518fd65e23..873caa477de 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 1d10e9642ec..55cfc603f13 100644 --- a/uuid-kinds/src/lib.rs +++ b/uuid-kinds/src/lib.rs @@ -51,12 +51,14 @@ macro_rules! impl_typed_uuid_kind { // Please keep this list in alphabetical order. impl_typed_uuid_kind! { + AccessToken => "access_token", AffinityGroup => "affinity_group", Alert => "alert", AlertReceiver => "alert_receiver", AntiAffinityGroup => "anti_affinity_group", Blueprint => "blueprint", Collection => "collection", + ConsoleSession => "console_session", Dataset => "dataset", DemoSaga => "demo_saga", Downstairs => "downstairs", From d15117c2aa0c7422b8c0d5a24e07a4a4bf2a2375 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 9 May 2025 16:13:43 -0500 Subject: [PATCH 04/11] 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 4fb5d58cf9a..d042ea18c01 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 532ad4721243bcb9937a9ef6dc4c1d30dd15a435 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 13 May 2025 13:52:28 -0500 Subject: [PATCH 05/11] 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 28b61f770d1..c29fdbceaed 100644 --- a/nexus/auth/src/authz/api_resources.rs +++ b/nexus/auth/src/authz/api_resources.rs @@ -955,7 +955,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 d042ea18c01..763ec6746e4 100644 --- a/nexus/db-lookup/src/lookup.rs +++ b/nexus/db-lookup/src/lookup.rs @@ -26,8 +26,10 @@ 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::AlertReceiverUuid; use omicron_uuid_kinds::AlertUuid; +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 7e2bf3e6f64..2cca57cda49 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 873caa477de..e4346f32418 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 14da469ccd1..182d1a8802b 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 6d95baf280a..3964a7ef75a 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 877fad40ed7..8eae637040b 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 db83d55771a68d971cfb6533ac6f18811a4e822d Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 13 May 2025 17:00:52 -0500 Subject: [PATCH 06/11] 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 c29fdbceaed..4883b6a99e1 100644 --- a/nexus/auth/src/authz/api_resources.rs +++ b/nexus/auth/src/authz/api_resources.rs @@ -939,7 +939,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 763ec6746e4..108656d87f9 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 e4346f32418..8555a7a3490 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 bf1b7996f5c..6be749adffa 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -7587,7 +7587,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 bf5804ddfa3abb530cb6001ff519357d7c396dd1 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 15 May 2025 14:54:13 -0500 Subject: [PATCH 07/11] 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 108656d87f9..a9f12ea53c9 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 f359c8b170032cd70cc2ba58f557de6a5e5bb319 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 16 May 2025 15:07:24 -0500 Subject: [PATCH 08/11] 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 1ba48a46cf7..d33b2de6444 100644 --- a/nexus/tests/integration_tests/schema.rs +++ b/nexus/tests/integration_tests/schema.rs @@ -2198,14 +2198,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_145_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-145', 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-145', gen_random_uuid(), 'code-145', gen_random_uuid(), now(), now()); + ", + ) + .await + .expect("failed to insert pre-migration rows for 145"); + }) } -fn after_142_0_0<'a>(_ctx: &'a MigrationContext<'a>) -> BoxFuture<'a, ()> { - Box::pin(async {}) +fn after_145_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-145';", + &[], + ) + .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-145"); + + // device_access_token: same checks + let rows = ctx + .client + .query( + "SELECT id, token FROM omicron.public.device_access_token WHERE token = 'tok-device-145';", + &[], + ) + .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-145",); + }) } // Lazily initializes all migration checks. The combination of Rust function From f9f9d8fd952f0df621b8a82fa50f8d308007e876 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 21 May 2025 16:38:54 -0500 Subject: [PATCH 09/11] do the right authz thing for lookups by token --- .../src/db/datastore/console_session.rs | 22 ++++++++++++++---- .../src/db/datastore/device_auth.rs | 23 +++++++++++++++---- nexus/db-queries/src/db/datastore/mod.rs | 4 ++-- nexus/src/app/device_auth.rs | 2 +- nexus/src/app/session.rs | 6 ++--- 5 files changed, 43 insertions(+), 14 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/console_session.rs b/nexus/db-queries/src/db/datastore/console_session.rs index 79ee9ab8a3b..caba38418a2 100644 --- a/nexus/db-queries/src/db/datastore/console_session.rs +++ b/nexus/db-queries/src/db/datastore/console_session.rs @@ -29,9 +29,8 @@ impl DataStore { &self, opctx: &OpContext, token: String, - ) -> LookupResult { - // TODO: some special system authz because the presence of the token _is_ the authz - console_session::table + ) -> LookupResult<(authz::ConsoleSession, ConsoleSession)> { + let db_session = console_session::table .filter(console_session::token.eq(token)) .select(ConsoleSession::as_returning()) .get_result_async(&*self.pool_connection_authorized(opctx).await?) @@ -39,7 +38,22 @@ impl DataStore { .map_err(|_e| Error::ObjectNotFound { type_name: ResourceType::ConsoleSession, lookup_type: LookupType::ByOther("session token".to_string()), - }) + })?; + + // we have to construct the authz resource after the lookup because we don't + // have its ID on hand until then + let authz_session = authz::ConsoleSession::new( + authz::FLEET, + db_session.id(), + LookupType::ById(db_session.id().into_untyped_uuid()), + ); + + // This check might seem superfluous, but (for now at least) only the + // fleet external authenticator user can read a session, so this is + // essentially checking that the opctx comes from that user. + opctx.authorize(authz::Action::Read, &authz_session).await?; + + Ok((authz_session, db_session)) } // TODO-correctness: fix session method errors. the map_errs turn all errors diff --git a/nexus/db-queries/src/db/datastore/device_auth.rs b/nexus/db-queries/src/db/datastore/device_auth.rs index 823e323ddef..77ede5dc8d4 100644 --- a/nexus/db-queries/src/db/datastore/device_auth.rs +++ b/nexus/db-queries/src/db/datastore/device_auth.rs @@ -19,6 +19,7 @@ use omicron_common::api::external::Error; use omicron_common::api::external::LookupResult; use omicron_common::api::external::LookupType; use omicron_common::api::external::ResourceType; +use omicron_uuid_kinds::GenericUuid; use uuid::Uuid; impl DataStore { @@ -26,9 +27,8 @@ impl DataStore { &self, opctx: &OpContext, token: String, - ) -> LookupResult { - // TODO: some special system authz because the presence of the token _is_ the authz - device_access_token::table + ) -> LookupResult<(authz::DeviceAccessToken, DeviceAccessToken)> { + let db_token = device_access_token::table .filter(device_access_token::token.eq(token)) .select(DeviceAccessToken::as_returning()) .get_result_async(&*self.pool_connection_authorized(opctx).await?) @@ -36,7 +36,22 @@ impl DataStore { .map_err(|_e| Error::ObjectNotFound { type_name: ResourceType::DeviceAccessToken, lookup_type: LookupType::ByOther("access token".to_string()), - }) + })?; + + // we have to construct the authz resource after the lookup because we don't + // have its ID on hand until then + let authz_token = authz::DeviceAccessToken::new( + authz::FLEET, + db_token.id(), + LookupType::ById(db_token.id().into_untyped_uuid()), + ); + + // This check might seem superfluous, but (for now at least) only the + // fleet external authenticator user can read a token, so this is + // essentially checking that the opctx comes from that user. + opctx.authorize(authz::Action::Read, &authz_token).await?; + + Ok((authz_token, db_token)) } /// Start a device authorization grant flow by recording the request diff --git a/nexus/db-queries/src/db/datastore/mod.rs b/nexus/db-queries/src/db/datastore/mod.rs index 8555a7a3490..7753ea0ff05 100644 --- a/nexus/db-queries/src/db/datastore/mod.rs +++ b/nexus/db-queries/src/db/datastore/mod.rs @@ -613,7 +613,7 @@ mod test { assert_eq!(DEFAULT_SILO_ID, db_silo_user.silo_id); // fetch the one we just created - let fetched = datastore + let (.., fetched) = datastore .session_lookup_by_token(&authn_opctx, token.clone()) .await .unwrap(); @@ -655,7 +655,7 @@ mod test { // right thing between opctx or authn_opctx // time_last_used change persists in DB - let fetched = datastore + let (.., fetched) = datastore .session_lookup_by_token(&opctx, token.clone()) .await .unwrap(); diff --git a/nexus/src/app/device_auth.rs b/nexus/src/app/device_auth.rs index 3e4d815bf59..e4a964de491 100644 --- a/nexus/src/app/device_auth.rs +++ b/nexus/src/app/device_auth.rs @@ -166,7 +166,7 @@ impl super::Nexus { opctx: &OpContext, token: String, ) -> Result { - let db_access_token = self + let (.., db_access_token) = self .db_datastore .device_token_lookup_by_token(opctx, token) .await diff --git a/nexus/src/app/session.rs b/nexus/src/app/session.rs index 5f391fa2a63..f90317afae3 100644 --- a/nexus/src/app/session.rs +++ b/nexus/src/app/session.rs @@ -53,16 +53,16 @@ impl super::Nexus { opctx: &OpContext, token: String, ) -> LookupResult { - let db_console_session = + let (.., db_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) + .silo_user_id(db_session.silo_user_id) .fetch() .await?; Ok(authn::ConsoleSessionWithSiloId { - console_session: db_console_session, + console_session: db_session, silo_id: db_silo_user.silo_id, }) } From 424598bba8145a7989c472b82b8c5c9e7c4949f5 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 21 May 2025 17:41:54 -0500 Subject: [PATCH 10/11] tweak logout logic and comment better --- nexus/src/external_api/http_entrypoints.rs | 48 +++++++++++++--------- 1 file changed, 28 insertions(+), 20 deletions(-) diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 6be749adffa..10a0e5c562e 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -7581,33 +7581,41 @@ impl NexusExternalApi for NexusExternalApiImpl { let apictx = rqctx.context(); let handler = async { let nexus = &apictx.context.nexus; + // this is unique among the hundreds of calls to this function in + // that we are not using ? to return early on error let opctx = crate::context::op_context_for_external_api(&rqctx).await; - let token = cookies.get(session_cookie::SESSION_COOKIE_COOKIE_NAME); + let session_cookie = + cookies.get(session_cookie::SESSION_COOKIE_COOKIE_NAME); + // Look up session and delete it if present. Noop on any errors. + // 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. if let Ok(opctx) = opctx { - if let Some(token) = token { - // 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; - } + if let Some(cookie) = session_cookie { + let token = cookie.value().to_string(); + match nexus.session_fetch(&opctx, token).await { + Ok(session) => { + let id = session.console_session.id(); + // ? here because if this fails, we did not delete the + // session when we meant to + nexus.session_hard_delete(&opctx, id).await?; + } + // blow up only on errors other than not found, because not + // found is fine: nothing to delete + Err(Error::ObjectNotFound { .. }) => {} // noop + Err(e) => return Err(e.into()), + }; } } - // If user's session was already expired, they failed auth and their - // session was automatically deleted by the auth scheme. If they have no - // session (e.g., they cleared their cookies while sitting on the page) - // they will also fail auth. + // If user's session was already expired, they fail auth and their + // session is automatically deleted by the auth scheme. If they + // have no session at all (because, e.g., they cleared their cookies + // while sitting on the page) they will also fail auth, but nothing + // is deleted and the above lookup by token fails (but doesn't early + // return). // Even if the user failed auth, we don't want to send them back a 401 // like we would for a normal request. They are in fact logged out like From 6f16cfeec14186f5d6e5915a6ef30d5487121eac Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 22 May 2025 17:54:36 -0500 Subject: [PATCH 11/11] on second thought, add hard delete by token --- .../src/db/datastore/console_session.rs | 21 ++++++++++++ nexus/db-queries/src/db/datastore/mod.rs | 4 +++ nexus/src/app/session.rs | 8 +++++ nexus/src/external_api/http_entrypoints.rs | 32 +++++-------------- nexus/tests/integration_tests/console_api.rs | 13 ++++++++ 5 files changed, 54 insertions(+), 24 deletions(-) diff --git a/nexus/db-queries/src/db/datastore/console_session.rs b/nexus/db-queries/src/db/datastore/console_session.rs index caba38418a2..6f01fd3486b 100644 --- a/nexus/db-queries/src/db/datastore/console_session.rs +++ b/nexus/db-queries/src/db/datastore/console_session.rs @@ -175,4 +175,25 @@ impl DataStore { )) }) } + + pub async fn session_hard_delete_by_token( + &self, + opctx: &OpContext, + token: String, + ) -> DeleteResult { + // we don't do an authz check here because the possession of + // the token is the check + use nexus_db_schema::schema::console_session; + diesel::delete(console_session::table) + .filter(console_session::token.eq(token)) + .execute_async(&*self.pool_connection_authorized(opctx).await?) + .await + .map(|_rows_deleted| ()) + .map_err(|e| { + Error::internal_error(&format!( + "error deleting session by token: {:?}", + e + )) + }) + } } diff --git a/nexus/db-queries/src/db/datastore/mod.rs b/nexus/db-queries/src/db/datastore/mod.rs index 7753ea0ff05..723461e4338 100644 --- a/nexus/db-queries/src/db/datastore/mod.rs +++ b/nexus/db-queries/src/db/datastore/mod.rs @@ -700,6 +700,10 @@ mod test { datastore.session_hard_delete(&opctx, &authz_session).await; assert_eq!(delete_again, Ok(())); + let delete_again = + datastore.session_hard_delete_by_token(&opctx, token.clone()).await; + assert_eq!(delete_again, Ok(())); + db.terminate().await; logctx.cleanup_successful(); } diff --git a/nexus/src/app/session.rs b/nexus/src/app/session.rs index f90317afae3..1f73cdbcb37 100644 --- a/nexus/src/app/session.rs +++ b/nexus/src/app/session.rs @@ -94,6 +94,14 @@ impl super::Nexus { self.db_datastore.session_hard_delete(opctx, &authz_session).await } + pub(crate) async fn session_hard_delete_by_token( + &self, + opctx: &OpContext, + token: String, + ) -> DeleteResult { + self.db_datastore.session_hard_delete_by_token(opctx, token).await + } + pub(crate) async fn lookup_silo_for_authn( &self, opctx: &OpContext, diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 10a0e5c562e..8eb4dadd82c 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -7581,33 +7581,17 @@ impl NexusExternalApi for NexusExternalApiImpl { let apictx = rqctx.context(); let handler = async { let nexus = &apictx.context.nexus; - // this is unique among the hundreds of calls to this function in - // that we are not using ? to return early on error - let opctx = - crate::context::op_context_for_external_api(&rqctx).await; + // this is kind of a weird one, but we're only doing things here + // that are authorized directly by the possession of the token, + // which makes it somewhat like a login + let opctx = nexus.opctx_external_authn(); let session_cookie = cookies.get(session_cookie::SESSION_COOKIE_COOKIE_NAME); - // Look up session and delete it if present. Noop on any errors. - // 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. - if let Ok(opctx) = opctx { - if let Some(cookie) = session_cookie { - let token = cookie.value().to_string(); - match nexus.session_fetch(&opctx, token).await { - Ok(session) => { - let id = session.console_session.id(); - // ? here because if this fails, we did not delete the - // session when we meant to - nexus.session_hard_delete(&opctx, id).await?; - } - // blow up only on errors other than not found, because not - // found is fine: nothing to delete - Err(Error::ObjectNotFound { .. }) => {} // noop - Err(e) => return Err(e.into()), - }; - } + // Look up session and delete it if present + if let Some(cookie) = session_cookie { + let token = cookie.value().to_string(); + nexus.session_hard_delete_by_token(&opctx, token).await?; } // If user's session was already expired, they fail auth and their diff --git a/nexus/tests/integration_tests/console_api.rs b/nexus/tests/integration_tests/console_api.rs index e1c0d5d23e4..c759618b264 100644 --- a/nexus/tests/integration_tests/console_api.rs +++ b/nexus/tests/integration_tests/console_api.rs @@ -136,6 +136,19 @@ async fn test_sessions(cptestctx: &ControlPlaneTestContext) { .await .expect("failed to log out"); + // logout with a nonexistent session token does the same thing: clears it out + RequestBuilder::new(&testctx, Method::POST, "/v1/logout") + .header(header::COOKIE, "session=abc") + .expect_status(Some(StatusCode::NO_CONTENT)) + // logout also clears the cookie client-side + .expect_response_header( + header::SET_COOKIE, + "session=; Path=/; HttpOnly; SameSite=Lax; Max-Age=0", + ) + .execute() + .await + .expect("failed to log out"); + // now the same requests with the same session cookie should 401/302 because // logout also deletes the session server-side RequestBuilder::new(&testctx, Method::POST, "/v1/projects")