From 34945f3990a2f4dc23cac6305e8fa5f922bd8422 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 3 Jun 2025 13:06:28 -0500 Subject: [PATCH 1/3] test showing expired session delete doesn't work --- nexus/tests/integration_tests/console_api.rs | 60 +++++++++++++++++++- 1 file changed, 59 insertions(+), 1 deletion(-) diff --git a/nexus/tests/integration_tests/console_api.rs b/nexus/tests/integration_tests/console_api.rs index c759618b264..c245696c597 100644 --- a/nexus/tests/integration_tests/console_api.rs +++ b/nexus/tests/integration_tests/console_api.rs @@ -8,6 +8,7 @@ use dropshot::ResultsPage; use dropshot::test_util::ClientTestContext; use http::header::HeaderName; use http::{StatusCode, header, method::Method}; +use nexus_auth::context::OpContext; use std::env::current_dir; use crate::integration_tests::saml::SAML_RESPONSE_IDP_DESCRIPTOR; @@ -30,7 +31,7 @@ use nexus_test_utils_macros::nexus_test; use nexus_types::external_api::params::{self, ProjectCreate}; use nexus_types::external_api::shared::{SiloIdentityMode, SiloRole}; use nexus_types::external_api::{shared, views}; -use omicron_common::api::external::IdentityMetadataCreateParams; +use omicron_common::api::external::{Error, IdentityMetadataCreateParams}; use omicron_sled_agent::sim; use omicron_test_utils::dev::poll::{CondCheckError, wait_for_condition}; @@ -918,3 +919,60 @@ async fn expect_redirect(testctx: &ClientTestContext, from: &str, to: &str) { .await .expect("did not find expected redirect"); } + +/// Make sure an expired session gets deleted when you try to use it +/// +/// This is not the best kind of test because it breaks the API abstraction +/// boundary, but in this case it's necessary because by design we do not +/// expose through the API why authn failed, i.e., whether it's because +/// the session was found but is expired. vs not found at all +#[tokio::test] +async fn test_session_idle_timeout_deletes_session() { + // set idle timeout to 1 second so we can test expiration + let mut config = load_test_config(); + config.pkg.console.session_idle_timeout_minutes = 0; + let cptestctx = test_setup_with_config::( + "test_session_idle_timeout_deletes_session", + &mut config, + sim::SimMode::Explicit, + None, + 0, + ) + .await; + let testctx = &cptestctx.external_client; + + // Start session + let session_cookie = log_in_and_extract_token(&cptestctx).await; + + // sleep here not necessary given TTL of 0 + + // Make a request with the expired session cookie + let me_response = RequestBuilder::new(testctx, Method::GET, "/v1/me") + .header(header::COOKIE, &session_cookie) + .expect_status(Some(StatusCode::UNAUTHORIZED)) + .execute() + .await + .expect("first request with expired cookie did not 401"); + + let error_body: dropshot::HttpErrorResponseBody = + me_response.parsed_body().unwrap(); + assert_eq!(error_body.message, "credentials missing or invalid"); + + // check that the session actually got deleted + + let nexus = &cptestctx.server.server_context().nexus; + let datastore = nexus.datastore(); + let opctx = + OpContext::for_tests(cptestctx.logctx.log.new(o!()), datastore.clone()); + + let token = session_cookie.strip_prefix("session=").unwrap(); + let db_token_error = nexus + .datastore() + .session_lookup_by_token(&opctx, token.to_string()) + .await + .expect_err("session should be deleted"); + assert_matches::assert_matches!( + db_token_error, + Error::ObjectNotFound { .. } + ); +} From fa088246c86f82ed1b3de433f6e3031f4c99fcf9 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 3 Jun 2025 13:06:28 -0500 Subject: [PATCH 2/3] delete session by token instead of by ID --- .../auth/src/authn/external/session_cookie.rs | 10 ++-- .../src/db/datastore/console_session.rs | 48 ------------------- nexus/db-queries/src/db/datastore/mod.rs | 15 +----- nexus/src/app/session.rs | 13 ----- nexus/src/context.rs | 4 +- nexus/tests/integration_tests/authn_http.rs | 4 +- 6 files changed, 11 insertions(+), 83 deletions(-) diff --git a/nexus/auth/src/authn/external/session_cookie.rs b/nexus/auth/src/authn/external/session_cookie.rs index 00d959c56de..94918776927 100644 --- a/nexus/auth/src/authn/external/session_cookie.rs +++ b/nexus/auth/src/authn/external/session_cookie.rs @@ -45,7 +45,7 @@ pub trait SessionStore { ) -> Option; /// Mark session expired - async fn session_expire(&self, id: ConsoleSessionUuid) -> Option<()>; + async fn session_expire(&self, token: String) -> Option<()>; /// Maximum time session can remain idle before expiring fn session_idle_timeout(&self) -> Duration; @@ -133,7 +133,7 @@ where // expired let now = Utc::now(); if session.time_last_used() + ctx.session_idle_timeout() < now { - let expired_session = ctx.session_expire(session.id()).await; + let expired_session = ctx.session_expire(token).await; if expired_session.is_none() { debug!(log, "failed to expire session") } @@ -153,7 +153,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(session.id()).await; + let expired_session = ctx.session_expire(token).await; if expired_session.is_none() { debug!(log, "failed to expire session") } @@ -273,9 +273,9 @@ mod test { } } - async fn session_expire(&self, id: ConsoleSessionUuid) -> Option<()> { + async fn session_expire(&self, token: String) -> Option<()> { let mut sessions = self.sessions.lock().unwrap(); - sessions.retain(|s| s.id != id); + sessions.retain(|s| s.token != token); Some(()) } diff --git a/nexus/db-queries/src/db/datastore/console_session.rs b/nexus/db-queries/src/db/datastore/console_session.rs index 628a9e5bd9e..961566916b7 100644 --- a/nexus/db-queries/src/db/datastore/console_session.rs +++ b/nexus/db-queries/src/db/datastore/console_session.rs @@ -17,7 +17,6 @@ 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; @@ -135,53 +134,6 @@ impl DataStore { }) } - // putting "hard" in the name because we don't do this with any other model - pub async fn session_hard_delete( - &self, - opctx: &OpContext, - authz_session: &authz::ConsoleSession, - ) -> DeleteResult { - // We don't do a typical authz check here. Instead, knowing that every - // user is allowed to delete their own session, the query below filters - // on the session's silo_user_id matching the current actor's id. - // - // We could instead model this more like other authz checks. That would - // involve fetching the session record from the database, storing the - // associated silo_user_id into the `authz::ConsoleSession`, and having - // an Oso rule saying you can delete a session whose associated silo - // user matches the authenticated actor. This would be a fair bit more - // complicated and more work at runtime work than what we're doing here. - // The tradeoff is that we're effectively encoding policy here, but it - // seems worth it in this case. - let actor = opctx - .authn - .actor_required() - .internal_context("deleting current user's session")?; - - // This check shouldn't be required in that there should be no overlap - // between silo user ids and other types of identity ids. But it's easy - // to check, and if we add another type of Actor, we'll be forced here - // to consider if they should be able to have console sessions and log - // out of them. - let silo_user_id = actor - .silo_user_id() - .ok_or_else(|| Error::invalid_request("not a Silo user"))?; - - use nexus_db_schema::schema::console_session::dsl; - diesel::delete(dsl::console_session) - .filter(dsl::silo_user_id.eq(silo_user_id)) - .filter(dsl::id.eq(authz_session.id().into_untyped_uuid())) - .execute_async(&*self.pool_connection_authorized(opctx).await?) - .await - .map(|_rows_deleted| ()) - .map_err(|e| { - Error::internal_error(&format!( - "error deleting session: {:?}", - e - )) - }) - } - pub async fn session_hard_delete_by_token( &self, opctx: &OpContext, diff --git a/nexus/db-queries/src/db/datastore/mod.rs b/nexus/db-queries/src/db/datastore/mod.rs index 87efcc3f6fc..7aa4ae6a00c 100644 --- a/nexus/db-queries/src/db/datastore/mod.rs +++ b/nexus/db-queries/src/db/datastore/mod.rs @@ -659,17 +659,6 @@ mod test { .unwrap(); assert!(fetched.time_last_used > session.time_last_used); - // deleting it using `opctx` (which represents the test-privileged user) - // should succeed but not do anything -- you can't delete someone else's - // session - let delete = - datastore.session_hard_delete(&opctx, &authz_session).await; - assert_eq!(delete, Ok(())); - let fetched = datastore - .session_lookup_by_token(&authn_opctx, token.clone()) - .await; - assert!(fetched.is_ok()); - // delete it and fetch should come back with nothing let silo_user_opctx = OpContext::for_background( logctx.log.new(o!()), @@ -682,7 +671,7 @@ mod test { Arc::clone(&datastore) as Arc, ); let delete = datastore - .session_hard_delete(&silo_user_opctx, &authz_session) + .session_hard_delete_by_token(&silo_user_opctx, token.clone()) .await; assert_eq!(delete, Ok(())); let fetched = datastore @@ -695,7 +684,7 @@ mod test { // deleting an already nonexistent is considered a success let delete_again = - datastore.session_hard_delete(&opctx, &authz_session).await; + datastore.session_hard_delete_by_token(&opctx, token.clone()).await; assert_eq!(delete_again, Ok(())); let delete_again = diff --git a/nexus/src/app/session.rs b/nexus/src/app/session.rs index 38cd695e2a5..36bedf393ac 100644 --- a/nexus/src/app/session.rs +++ b/nexus/src/app/session.rs @@ -80,19 +80,6 @@ impl super::Nexus { self.db_datastore.session_update_last_used(opctx, &authz_session).await } - pub(crate) async fn session_hard_delete( - &self, - opctx: &OpContext, - id: ConsoleSessionUuid, - ) -> DeleteResult { - let authz_session = authz::ConsoleSession::new( - authz::FLEET, - id, - LookupType::ById(id.into_untyped_uuid()), - ); - self.db_datastore.session_hard_delete(opctx, &authz_session).await - } - pub(crate) async fn session_hard_delete_by_token( &self, opctx: &OpContext, diff --git a/nexus/src/context.rs b/nexus/src/context.rs index 60d137101c7..d170e7e79d0 100644 --- a/nexus/src/context.rs +++ b/nexus/src/context.rs @@ -477,9 +477,9 @@ impl SessionStore for ServerContext { self.nexus.session_update_last_used(&opctx, id).await.ok() } - async fn session_expire(&self, id: ConsoleSessionUuid) -> Option<()> { + async fn session_expire(&self, token: String) -> Option<()> { let opctx = self.nexus.opctx_external_authn(); - self.nexus.session_hard_delete(opctx, id).await.ok() + self.nexus.session_hard_delete_by_token(opctx, token).await.ok() } fn session_idle_timeout(&self) -> Duration { diff --git a/nexus/tests/integration_tests/authn_http.rs b/nexus/tests/integration_tests/authn_http.rs index b64097fd7ff..59275e881dd 100644 --- a/nexus/tests/integration_tests/authn_http.rs +++ b/nexus/tests/integration_tests/authn_http.rs @@ -397,9 +397,9 @@ impl session_cookie::SessionStore for WhoamiServerState { } } - async fn session_expire(&self, id: ConsoleSessionUuid) -> Option<()> { + async fn session_expire(&self, token: String) -> Option<()> { let mut sessions = self.sessions.lock().unwrap(); - sessions.retain(|s| s.id != id); + sessions.retain(|s| s.token != token); Some(()) } From cb8ab269d99d8c3db5629c3ab1dd38107c86365c Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 4 Jun 2025 09:38:16 -0500 Subject: [PATCH 3/3] forgot to clean up after test --- nexus/tests/integration_tests/console_api.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/nexus/tests/integration_tests/console_api.rs b/nexus/tests/integration_tests/console_api.rs index c245696c597..80800d95c58 100644 --- a/nexus/tests/integration_tests/console_api.rs +++ b/nexus/tests/integration_tests/console_api.rs @@ -975,4 +975,6 @@ async fn test_session_idle_timeout_deletes_session() { db_token_error, Error::ObjectNotFound { .. } ); + + cptestctx.teardown().await; }