[5/n] Expire sessions by token again (#8260)

Built on top of #8231. Fix for an issue with the internals of session
expiration discussed in
#8137 (comment).

When I added ID primary keys to sessions and tokens, I changed the
session delete method on the Session trait to operate by ID as well, and
that meant a more complex authz situation compared to deleting by token.
A bearer token is its own authz, at least according to the implicit
policy we decided on. When I changed it to delete by ID, I made the
delete function (now removed in this PR) use the user ID from the
`opctx` to filter tokens to ensure that users could only delete their
own sessions.

The problem is that the `opctx` used in that case is the external
authenticator op context, which has its own special actor that is
distinct from the user in question. This means we would never actually
delete a token this way because the query would always come up empty.
This is not actually a problem from a correctness point of view because
on subsequent requests we always check the expiration time on the
session we pull out of the DB anyway. But it does mean that we probably
end up with a lot more sessions piling up in the DB than we otherwise
would. Not a big deal either way, but I feel more comfortable having it
keep working the way it has worked for several years.

It's also nice that we are deleting some app code here and replacing it
with a test for a behavior that was previously untested. (I added the
test in the first commit and confirmed that it fails without the changes
from the second commit.)

Author: david-crespo

nexus/auth/src/authn/external/session_cookie.rs

@@ -45,7 +45,7 @@ pub trait SessionStore {
     ) -> Option<Self::SessionModel>;
 
     /// 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(())
         }
 

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,

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<dyn nexus_auth::storage::Storage>,
         );
         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 =

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,

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 {

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(())
     }
 

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,62 @@ 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::<omicron_nexus::Server>(
+        "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 { .. }
+    );
+
+    cptestctx.teardown().await;
+}