From 07bf95f3b4eadd6c3617310020a281a677cae943 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 30 Jun 2022 16:52:19 -0700 Subject: [PATCH 1/4] initial work --- nexus/src/db/datastore.rs | 27 +++++++++++- nexus/tests/integration_tests/console_api.rs | 44 +++++++++++++++++++- 2 files changed, 68 insertions(+), 3 deletions(-) diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index c53b84c7a45..778cfa828f0 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -3369,10 +3369,35 @@ impl DataStore { opctx: &OpContext, authz_session: &authz::ConsoleSession, ) -> DeleteResult { - opctx.authorize(authz::Action::Delete, authz_session).await?; + // 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()?; + + // 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 = match actor.actor_type() { + IdentityType::SiloUser => actor.actor_id(), + IdentityType::UserBuiltin => { + return Err(Error::invalid_request("not a Silo user")) + } + }; use db::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())) .execute_async(self.pool_authorized(opctx).await?) .await diff --git a/nexus/tests/integration_tests/console_api.rs b/nexus/tests/integration_tests/console_api.rs index 5c3d8fc5ee7..3cdc51a957d 100644 --- a/nexus/tests/integration_tests/console_api.rs +++ b/nexus/tests/integration_tests/console_api.rs @@ -10,20 +10,31 @@ use std::env::current_dir; use nexus_test_utils::http_testing::{ AuthnMode, NexusRequest, RequestBuilder, TestResponse, }; +use nexus_test_utils::resource_helpers::grant_iam; use nexus_test_utils::{ load_test_config, test_setup_with_config, ControlPlaneTestContext, }; use nexus_test_utils_macros::nexus_test; use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_nexus::authn::{USER_TEST_PRIVILEGED, USER_TEST_UNPRIVILEGED}; +use omicron_nexus::authz::SiloRole; +use omicron_nexus::db::fixed_data::silo::DEFAULT_SILO; use omicron_nexus::db::identity::Asset; use omicron_nexus::external_api::console_api::SpoofLoginBody; use omicron_nexus::external_api::params::OrganizationCreate; -use omicron_nexus::external_api::views; +use omicron_nexus::external_api::{shared, views}; + +// XXX-dap TODO-coverage tests to add: +// - attempt to log out using somebody else's session cookie shouldn't work -- +// is this possible? +// - this might have to be a datastore-level test +// - attempt to create a session for a built-in user and then log out shouldn't +// work #[nexus_test] async fn test_sessions(cptestctx: &ControlPlaneTestContext) { let testctx = &cptestctx.external_client; + let nexus = &cptestctx.server.apictx.nexus; // logout always gives the same response whether you have a session or not RequestBuilder::new(&testctx, Method::POST, "/logout") @@ -61,6 +72,29 @@ async fn test_sessions(cptestctx: &ControlPlaneTestContext) { .await .expect("failed to 302 on unauthed console page request"); + // Our test uses the "unprivileged" user to make sure login/logout works + // without other privileges. However, they _do_ need the privilege to + // create Organizations because we'll be testing that as a smoke test. + // We'll remove that privilege afterwards. + let silo_url = format!("/silos/{}", DEFAULT_SILO.identity.name); + let policy_url = format!("{}/policy", policy_url); + let initial_policy: shared::Policy = + NexusRequest::object_get(testctx, &policy_url) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("failed to fetch Silo policy") + .parsed_body() + .expect("failed to parse Silo policy"); + grant_iam( + testctx, + &policy_url, + SiloRole::Collaborator, + USER_TEST_UNPRIVILEGED.id(), + AuthnMode::PrivilegedUser, + ) + .await; + // now make same requests with cookie RequestBuilder::new(&testctx, Method::POST, "/organizations") .header(header::COOKIE, &session_token) @@ -78,6 +112,12 @@ async fn test_sessions(cptestctx: &ControlPlaneTestContext) { .await .expect("failed to get console page with session cookie"); + NexusRequest::object_put(testctx, &policy_url, Some(&initial_policy)) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .expect("failed to restore Silo policy"); + // logout with an actual session should delete the session in the db RequestBuilder::new(&testctx, Method::POST, "/logout") .header(header::COOKIE, &session_token) @@ -282,7 +322,7 @@ fn get_header_value(resp: TestResponse, header_name: HeaderName) -> String { async fn log_in_and_extract_token(testctx: &ClientTestContext) -> String { let login = RequestBuilder::new(&testctx, Method::POST, "/login") - .body(Some(&SpoofLoginBody { username: "privileged".to_string() })) + .body(Some(&SpoofLoginBody { username: "unprivileged".to_string() })) .expect_status(Some(StatusCode::OK)) .execute() .await From 97d28e31a0bed5ae14391289ef28ad3144df0843 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Thu, 30 Jun 2022 17:19:25 -0700 Subject: [PATCH 2/4] fix test --- nexus/tests/integration_tests/console_api.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/nexus/tests/integration_tests/console_api.rs b/nexus/tests/integration_tests/console_api.rs index 3cdc51a957d..0e50b0b3db8 100644 --- a/nexus/tests/integration_tests/console_api.rs +++ b/nexus/tests/integration_tests/console_api.rs @@ -19,7 +19,7 @@ use omicron_common::api::external::IdentityMetadataCreateParams; use omicron_nexus::authn::{USER_TEST_PRIVILEGED, USER_TEST_UNPRIVILEGED}; use omicron_nexus::authz::SiloRole; use omicron_nexus::db::fixed_data::silo::DEFAULT_SILO; -use omicron_nexus::db::identity::Asset; +use omicron_nexus::db::identity::{Asset, Resource}; use omicron_nexus::external_api::console_api::SpoofLoginBody; use omicron_nexus::external_api::params::OrganizationCreate; use omicron_nexus::external_api::{shared, views}; @@ -34,7 +34,6 @@ use omicron_nexus::external_api::{shared, views}; #[nexus_test] async fn test_sessions(cptestctx: &ControlPlaneTestContext) { let testctx = &cptestctx.external_client; - let nexus = &cptestctx.server.apictx.nexus; // logout always gives the same response whether you have a session or not RequestBuilder::new(&testctx, Method::POST, "/logout") @@ -76,8 +75,8 @@ async fn test_sessions(cptestctx: &ControlPlaneTestContext) { // without other privileges. However, they _do_ need the privilege to // create Organizations because we'll be testing that as a smoke test. // We'll remove that privilege afterwards. - let silo_url = format!("/silos/{}", DEFAULT_SILO.identity.name); - let policy_url = format!("{}/policy", policy_url); + let silo_url = format!("/silos/{}", DEFAULT_SILO.identity().name); + let policy_url = format!("{}/policy", silo_url); let initial_policy: shared::Policy = NexusRequest::object_get(testctx, &policy_url) .authn_as(AuthnMode::PrivilegedUser) @@ -88,7 +87,7 @@ async fn test_sessions(cptestctx: &ControlPlaneTestContext) { .expect("failed to parse Silo policy"); grant_iam( testctx, - &policy_url, + &silo_url, SiloRole::Collaborator, USER_TEST_UNPRIVILEGED.id(), AuthnMode::PrivilegedUser, From d822d71e8d9ed5e109215ebd6ee5fe2d27c1a188 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Fri, 1 Jul 2022 11:04:30 -0700 Subject: [PATCH 3/4] add/fix tests --- nexus/src/authn/mod.rs | 14 +++++++++---- nexus/src/db/datastore.rs | 22 ++++++++++++++++++-- nexus/tests/integration_tests/console_api.rs | 7 ------- 3 files changed, 30 insertions(+), 13 deletions(-) diff --git a/nexus/src/authn/mod.rs b/nexus/src/authn/mod.rs index c790e34236e..8f1cfbc20a6 100644 --- a/nexus/src/authn/mod.rs +++ b/nexus/src/authn/mod.rs @@ -204,12 +204,18 @@ impl Context { /// (for testing only) #[cfg(test)] pub fn unprivileged_test_user() -> Context { + Self::test_silo_user( + USER_TEST_UNPRIVILEGED.silo_id, + USER_TEST_UNPRIVILEGED.id(), + ) + } + + /// Returns an authenticated context for a given silo user + #[cfg(test)] + pub fn test_silo_user(silo_id: Uuid, silo_user_id: Uuid) -> Context { Context { kind: Kind::Authenticated(Details { - actor: Actor::SiloUser { - silo_user_id: USER_TEST_UNPRIVILEGED.id(), - silo_id: USER_TEST_UNPRIVILEGED.silo_id, - }, + actor: Actor::SiloUser { silo_user_id, silo_id }, }), schemes_tried: Vec::new(), } diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 778cfa828f0..75347aa5461 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -4593,6 +4593,7 @@ mod test { duplicate, Err(Error::InternalError { internal_message: _ }) )); + // XXX-dap not an internal error! // update last used (i.e., renew token) let authz_session = authz::ConsoleSession::new( @@ -4616,12 +4617,29 @@ mod test { .unwrap(); assert!(fetched.time_last_used > session.time_last_used); - // delete it and fetch should come back with nothing + // 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 = LookupPath::new(&opctx, &datastore) + .console_session_token(&token) + .fetch() + .await; + assert!(fetched.is_ok()); - // this will be a not found after #347 + // delete it and fetch should come back with nothing + let silo_user_opctx = OpContext::for_background( + logctx.log.new(o!()), + Arc::new(authz::Authz::new(&logctx.log)), + authn::Context::test_silo_user(*SILO_ID, silo_user_id), + Arc::clone(&datastore), + ); + let delete = datastore + .session_hard_delete(&silo_user_opctx, &authz_session) + .await; + assert_eq!(delete, Ok(())); let fetched = LookupPath::new(&opctx, &datastore) .console_session_token(&token) .fetch() diff --git a/nexus/tests/integration_tests/console_api.rs b/nexus/tests/integration_tests/console_api.rs index 0e50b0b3db8..e7eac4440ec 100644 --- a/nexus/tests/integration_tests/console_api.rs +++ b/nexus/tests/integration_tests/console_api.rs @@ -24,13 +24,6 @@ use omicron_nexus::external_api::console_api::SpoofLoginBody; use omicron_nexus::external_api::params::OrganizationCreate; use omicron_nexus::external_api::{shared, views}; -// XXX-dap TODO-coverage tests to add: -// - attempt to log out using somebody else's session cookie shouldn't work -- -// is this possible? -// - this might have to be a datastore-level test -// - attempt to create a session for a built-in user and then log out shouldn't -// work - #[nexus_test] async fn test_sessions(cptestctx: &ControlPlaneTestContext) { let testctx = &cptestctx.external_client; From 678185cf1ea71fd304ddb0aac8838ebc185ea273 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Fri, 1 Jul 2022 11:13:51 -0700 Subject: [PATCH 4/4] remove XXX --- nexus/src/db/datastore.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index 75347aa5461..dc24bf022a4 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -4593,7 +4593,6 @@ mod test { duplicate, Err(Error::InternalError { internal_message: _ }) )); - // XXX-dap not an internal error! // update last used (i.e., renew token) let authz_session = authz::ConsoleSession::new(