From debd3090f7f08d6059c1db5d9f5c1222d012cb8d Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 19 Apr 2022 10:24:08 -0700 Subject: [PATCH 1/3] Revert "authz: silo endpoints (#936)" This reverts commit 10196430670caa2bf3e60ba1eb63b267a42ae9b7. --- nexus/src/authn/external/mod.rs | 4 +- nexus/src/authn/external/spoof.rs | 8 +- nexus/src/authn/mod.rs | 2 +- nexus/src/db/datastore.rs | 132 ++++++++++-------- nexus/src/db/fixed_data/mod.rs | 2 +- nexus/src/db/fixed_data/silo.rs | 24 ---- nexus/src/db/fixed_data/silo_builtin.rs | 11 ++ nexus/src/db/fixed_data/user_builtin.rs | 2 +- nexus/src/external_api/http_entrypoints.rs | 2 + nexus/src/nexus.rs | 7 +- nexus/tests/integration_tests/endpoints.rs | 36 ----- nexus/tests/integration_tests/silos.rs | 2 +- .../output/uncovered-authz-endpoints.txt | 4 + 13 files changed, 103 insertions(+), 133 deletions(-) delete mode 100644 nexus/src/db/fixed_data/silo.rs create mode 100644 nexus/src/db/fixed_data/silo_builtin.rs diff --git a/nexus/src/authn/external/mod.rs b/nexus/src/authn/external/mod.rs index dcda0364a2..e50c15c0c5 100644 --- a/nexus/src/authn/external/mod.rs +++ b/nexus/src/authn/external/mod.rs @@ -189,7 +189,7 @@ mod test { let name1 = authn::SchemeName("grunt1"); let actor1 = authn::Actor { id: "1c91bab2-4841-669f-cc32-de80da5bbf39".parse().unwrap(), - silo_id: *crate::db::fixed_data::silo::SILO_ID, + silo_id: *crate::db::fixed_data::silo_builtin::SILO_ID, }; let grunt1 = Box::new(GruntScheme { name: name1, @@ -204,7 +204,7 @@ mod test { let name2 = authn::SchemeName("grunt2"); let actor2 = authn::Actor { id: "799684af-533a-cb66-b5ac-ab55a791d5ef".parse().unwrap(), - silo_id: *crate::db::fixed_data::silo::SILO_ID, + silo_id: *crate::db::fixed_data::silo_builtin::SILO_ID, }; let grunt2 = Box::new(GruntScheme { name: name2, diff --git a/nexus/src/authn/external/spoof.rs b/nexus/src/authn/external/spoof.rs index d8d85993b5..da1500ba16 100644 --- a/nexus/src/authn/external/spoof.rs +++ b/nexus/src/authn/external/spoof.rs @@ -57,7 +57,7 @@ lazy_static! { /// Actor (id) used for the special "bad credentials" error static ref SPOOF_RESERVED_BAD_CREDS_ACTOR: Actor = Actor { id: "22222222-2222-2222-2222-222222222222".parse().unwrap(), - silo_id: *crate::db::fixed_data::silo::SILO_ID, + silo_id: *crate::db::fixed_data::silo_builtin::SILO_ID, }; /// Complete HTTP header value to trigger the "bad actor" error pub static ref SPOOF_HEADER_BAD_ACTOR: Authorization = @@ -122,8 +122,10 @@ fn authn_spoof(raw_value: Option<&Authorization>) -> SchemeResult { match Uuid::parse_str(str_value).context("parsing header value as UUID") { Ok(id) => { - let actor = - Actor { id, silo_id: *crate::db::fixed_data::silo::SILO_ID }; + let actor = Actor { + id, + silo_id: *crate::db::fixed_data::silo_builtin::SILO_ID, + }; SchemeResult::Authenticated(Details { actor }) } Err(source) => SchemeResult::Failed(Reason::BadFormat { source }), diff --git a/nexus/src/authn/mod.rs b/nexus/src/authn/mod.rs index af083fe7b3..3520a0b00c 100644 --- a/nexus/src/authn/mod.rs +++ b/nexus/src/authn/mod.rs @@ -164,7 +164,7 @@ impl Context { pub fn test_context_for_actor(actor_id: Uuid) -> Context { Context::context_for_actor( actor_id, - *crate::db::fixed_data::silo::SILO_ID, + *crate::db::fixed_data::silo_builtin::SILO_ID, ) } } diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index eaae9562df..c3b32c125f 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -31,29 +31,7 @@ use crate::authz::ApiResourceError; use crate::context::OpContext; use crate::db::fixed_data::role_assignment_builtin::BUILTIN_ROLE_ASSIGNMENTS; use crate::db::fixed_data::role_builtin::BUILTIN_ROLES; -use crate::db::fixed_data::silo::{DEFAULT_SILO, SILO_ID}; -use crate::db::lookup::LookupPath; -use crate::db::{ - self, - error::{public_error_from_diesel_pool, ErrorHandler, TransactionError}, - model::{ - ConsoleSession, Dataset, DatasetKind, Disk, DiskRuntimeState, - Generation, IncompleteNetworkInterface, Instance, InstanceRuntimeState, - Name, NetworkInterface, Organization, OrganizationUpdate, OximeterInfo, - ProducerEndpoint, Project, ProjectUpdate, Region, - RoleAssignmentBuiltin, RoleBuiltin, RouterRoute, RouterRouteUpdate, - Silo, SiloUser, Sled, UpdateAvailableArtifact, UserBuiltin, Volume, - Vpc, VpcFirewallRule, VpcRouter, VpcRouterUpdate, VpcSubnet, - VpcSubnetUpdate, VpcUpdate, Zpool, - }, - pagination::paginated, - pagination::paginated_multicolumn, - subnet_allocation::FilterConflictingVpcSubnetRangesQuery, - subnet_allocation::InsertNetworkInterfaceQuery, - subnet_allocation::NetworkInterfaceError, - subnet_allocation::SubnetError, - update_and_check::{UpdateAndCheck, UpdateStatus}, -}; +use crate::db::fixed_data::silo_builtin::SILO_ID; use crate::external_api::params; use async_bb8_diesel::{AsyncConnection, AsyncRunQueryDsl, ConnectionManager}; use chrono::Utc; @@ -81,6 +59,29 @@ use std::net::Ipv6Addr; use std::sync::Arc; use uuid::Uuid; +use crate::db::lookup::LookupPath; +use crate::db::{ + self, + error::{public_error_from_diesel_pool, ErrorHandler, TransactionError}, + model::{ + ConsoleSession, Dataset, DatasetKind, Disk, DiskRuntimeState, + Generation, IncompleteNetworkInterface, Instance, InstanceRuntimeState, + Name, NetworkInterface, Organization, OrganizationUpdate, OximeterInfo, + ProducerEndpoint, Project, ProjectUpdate, Region, + RoleAssignmentBuiltin, RoleBuiltin, RouterRoute, RouterRouteUpdate, + Silo, SiloUser, Sled, UpdateAvailableArtifact, UserBuiltin, Volume, + Vpc, VpcFirewallRule, VpcRouter, VpcRouterUpdate, VpcSubnet, + VpcSubnetUpdate, VpcUpdate, Zpool, + }, + pagination::paginated, + pagination::paginated_multicolumn, + subnet_allocation::FilterConflictingVpcSubnetRangesQuery, + subnet_allocation::InsertNetworkInterfaceQuery, + subnet_allocation::NetworkInterfaceError, + subnet_allocation::SubnetError, + update_and_check::{UpdateAndCheck, UpdateStatus}, +}; + // Number of unique datasets required to back a region. // TODO: This should likely turn into a configuration option. const REGION_REDUNDANCY_THRESHOLD: usize = 3; @@ -2487,21 +2488,22 @@ impl DataStore { &self, opctx: &OpContext, ) -> Result<(), Error> { - opctx.authorize(authz::Action::Modify, &authz::DATABASE).await?; - debug!(opctx.log, "attempting to create built-in silo"); - use db::schema::silo::dsl; - let count = diesel::insert_into(dsl::silo) - .values(&*DEFAULT_SILO) - .on_conflict(dsl::id) - .do_nothing() - .execute_async(self.pool_authorized(opctx).await?) - .await - .map_err(|e| { - public_error_from_diesel_pool(e, ErrorHandler::Server) - })?; - info!(opctx.log, "created {} built-in silos", count); + let builtin_silo = Silo::new_with_id( + *SILO_ID, + params::SiloCreate { + identity: IdentityMetadataCreateParams { + name: "fakesilo".parse().unwrap(), + description: "fake silo".to_string(), + }, + discoverable: false, + }, + ); + + let _create_result = self.silo_create(opctx, builtin_silo).await?; + info!(opctx.log, "created built-in silo"); + Ok(()) } @@ -2510,11 +2512,12 @@ impl DataStore { opctx: &OpContext, silo: Silo, ) -> CreateResult { - opctx.authorize(authz::Action::CreateChild, &authz::FLEET).await?; + use db::schema::silo::dsl; + + // TODO opctx.authorize let silo_id = silo.id(); - use db::schema::silo::dsl; diesel::insert_into(dsl::silo) .values(silo) .returning(Silo::as_returning()) @@ -2536,9 +2539,8 @@ impl DataStore { opctx: &OpContext, pagparams: &DataPageParams<'_, Uuid>, ) -> ListResultVec { - opctx.authorize(authz::Action::ListChildren, &authz::FLEET).await?; - use db::schema::silo::dsl; + // TODO opctx.authorize paginated(dsl::silo, dsl::id, pagparams) .filter(dsl::time_deleted.is_null()) .filter(dsl::discoverable.eq(true)) @@ -2553,9 +2555,8 @@ impl DataStore { opctx: &OpContext, pagparams: &DataPageParams<'_, Name>, ) -> ListResultVec { - opctx.authorize(authz::Action::ListChildren, &authz::FLEET).await?; - use db::schema::silo::dsl; + // TODO opctx.authorize paginated(dsl::silo, dsl::name, pagparams) .filter(dsl::time_deleted.is_null()) .filter(dsl::discoverable.eq(true)) @@ -2568,26 +2569,38 @@ impl DataStore { pub async fn silo_delete( &self, opctx: &OpContext, - authz_silo: &authz::Silo, - db_silo: &db::model::Silo, + name: &Name, ) -> DeleteResult { - assert_eq!(authz_silo.id(), db_silo.id()); - opctx.authorize(authz::Action::Delete, authz_silo).await?; - use db::schema::organization; use db::schema::silo; use db::schema::silo_user; + // TODO opctx.authorize + + let (id, rcgen) = silo::dsl::silo + .filter(silo::dsl::time_deleted.is_null()) + .filter(silo::dsl::name.eq(name.clone())) + .select((silo::dsl::id, silo::dsl::rcgen)) + .get_result_async::<(Uuid, Generation)>(self.pool()) + .await + .map_err(|e| { + public_error_from_diesel_pool( + e, + ErrorHandler::NotFoundByLookup( + ResourceType::Silo, + LookupType::ByName(name.to_string()), + ), + ) + })?; + // Make sure there are no organizations present within this silo. - let id = authz_silo.id(); - let rcgen = db_silo.rcgen; let org_found = diesel_pool_result_optional( organization::dsl::organization .filter(organization::dsl::silo_id.eq(id)) .filter(organization::dsl::time_deleted.is_null()) .select(organization::dsl::id) .limit(1) - .first_async::(self.pool_authorized(opctx).await?) + .first_async::(self.pool()) .await, ) .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server))?; @@ -2605,12 +2618,15 @@ impl DataStore { .filter(silo::dsl::id.eq(id)) .filter(silo::dsl::rcgen.eq(rcgen)) .set(silo::dsl::time_deleted.eq(now)) - .execute_async(self.pool_authorized(opctx).await?) + .execute_async(self.pool()) .await .map_err(|e| { public_error_from_diesel_pool( e, - ErrorHandler::NotFoundByResource(authz_silo), + ErrorHandler::NotFoundByLookup( + ResourceType::Silo, + LookupType::ById(id), + ), ) })?; @@ -2624,22 +2640,22 @@ impl DataStore { info!(opctx.log, "deleted silo {}", id); // If silo deletion succeeded, delete all silo users - // TODO-correctness This needs to happen in a saga or some other - // mechanism that ensures it happens even if we crash at this point. - // TODO-scalability This needs to happen in batches let updated_rows = diesel::update(silo_user::dsl::silo_user) .filter(silo_user::dsl::silo_id.eq(id)) .set(silo_user::dsl::time_deleted.eq(now)) - .execute_async(self.pool_authorized(opctx).await?) + .execute_async(self.pool()) .await .map_err(|e| { public_error_from_diesel_pool( e, - ErrorHandler::NotFoundByResource(authz_silo), + ErrorHandler::NotFoundByLookup( + ResourceType::Silo, + LookupType::ById(id), + ), ) })?; - info!(opctx.log, "deleted {} silo users for silo {}", updated_rows, id); + info!(opctx.log, "deleted {} silo users for silo {}", updated_rows, id,); Ok(()) } diff --git a/nexus/src/db/fixed_data/mod.rs b/nexus/src/db/fixed_data/mod.rs index 57ddf92987..0137eec2ae 100644 --- a/nexus/src/db/fixed_data/mod.rs +++ b/nexus/src/db/fixed_data/mod.rs @@ -32,7 +32,7 @@ use lazy_static::lazy_static; pub mod role_assignment_builtin; pub mod role_builtin; -pub mod silo; +pub mod silo_builtin; pub mod user_builtin; lazy_static! { diff --git a/nexus/src/db/fixed_data/silo.rs b/nexus/src/db/fixed_data/silo.rs deleted file mode 100644 index e328da8cf7..0000000000 --- a/nexus/src/db/fixed_data/silo.rs +++ /dev/null @@ -1,24 +0,0 @@ -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this -// file, You can obtain one at https://mozilla.org/MPL/2.0/. - -use crate::db; -use crate::external_api::params; -use lazy_static::lazy_static; -use omicron_common::api::external::IdentityMetadataCreateParams; - -lazy_static! { - pub static ref SILO_ID: uuid::Uuid = "001de000-5110-4000-8000-000000000000" - .parse() - .expect("invalid uuid for builtin silo id"); - pub static ref DEFAULT_SILO: db::model::Silo = db::model::Silo::new_with_id( - *SILO_ID, - params::SiloCreate { - identity: IdentityMetadataCreateParams { - name: "default-silo".parse().unwrap(), - description: "default silo".to_string(), - }, - discoverable: false, - } - ); -} diff --git a/nexus/src/db/fixed_data/silo_builtin.rs b/nexus/src/db/fixed_data/silo_builtin.rs new file mode 100644 index 0000000000..6610f673c4 --- /dev/null +++ b/nexus/src/db/fixed_data/silo_builtin.rs @@ -0,0 +1,11 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +use lazy_static::lazy_static; + +lazy_static! { + pub static ref SILO_ID: uuid::Uuid = "001de000-5110-4000-8000-000000000000" + .parse() + .expect("invalid uuid for builtin silo id"); +} diff --git a/nexus/src/db/fixed_data/user_builtin.rs b/nexus/src/db/fixed_data/user_builtin.rs index f878feb187..22e83a60a3 100644 --- a/nexus/src/db/fixed_data/user_builtin.rs +++ b/nexus/src/db/fixed_data/user_builtin.rs @@ -3,7 +3,7 @@ // file, You can obtain one at https://mozilla.org/MPL/2.0/. //! Built-in users -use crate::db::fixed_data::silo::SILO_ID; +use crate::db::fixed_data::silo_builtin::SILO_ID; use lazy_static::lazy_static; use omicron_common::api; use uuid::Uuid; diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 90046ff4c8..ad24c4b748 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -230,6 +230,8 @@ pub fn external_api() -> NexusApiDescription { // clients. Client generators use operationId to name API methods, so changing // a function name is a breaking change from a client perspective. +// TODO authz for silo endpoints + // List all silos (that are discoverable). #[endpoint { method = GET, diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index c8c6bf1488..8c0ed86cc8 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -738,12 +738,7 @@ impl Nexus { opctx: &OpContext, name: &Name, ) -> DeleteResult { - let (.., authz_silo, db_silo) = - LookupPath::new(opctx, &self.db_datastore) - .silo_name(name) - .fetch_for(authz::Action::Delete) - .await?; - self.db_datastore.silo_delete(opctx, &authz_silo, &db_silo).await + self.db_datastore.silo_delete(opctx, name).await } // Organizations diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index d428c64113..cc31c02c04 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -23,8 +23,6 @@ use omicron_common::api::external::RouterRouteCreateParams; use omicron_common::api::external::RouterRouteUpdateParams; use omicron_common::api::external::VpcFirewallRuleUpdateParams; use omicron_nexus::authn; -use omicron_nexus::db::fixed_data::silo::DEFAULT_SILO; -use omicron_nexus::db::identity::Resource; use omicron_nexus::external_api::params; use std::net::IpAddr; use std::net::Ipv4Addr; @@ -35,19 +33,6 @@ lazy_static! { pub static ref HARDWARE_SLED_URL: String = format!("/hardware/sleds/{}", SLED_AGENT_UUID); - // Silo used for testing - pub static ref DEFAULT_SILO_NAME: &'static Name = &DEFAULT_SILO.name().0; - pub static ref DEFAULT_SILO_URL: String = - format!("/silos/{}", *DEFAULT_SILO_NAME); - pub static ref DEMO_SILO_CREATE: params::SiloCreate = - params::SiloCreate { - identity: IdentityMetadataCreateParams { - name: DEFAULT_SILO_NAME.clone(), - description: String::from(""), - }, - discoverable: true, - }; - // Organization used for testing pub static ref DEMO_ORG_NAME: Name = "demo-org".parse().unwrap(); pub static ref DEMO_ORG_URL: String = @@ -364,27 +349,6 @@ lazy_static! { /// List of endpoints to be verified pub static ref VERIFY_ENDPOINTS: Vec = vec![ - /* Silos */ - VerifyEndpoint { - url: "/silos", - visibility: Visibility::Public, - allowed_methods: vec![ - AllowedMethod::Get, - AllowedMethod::Post( - serde_json::to_value(&*DEMO_SILO_CREATE).unwrap() - ) - ], - }, - VerifyEndpoint { - url: &*DEFAULT_SILO_URL, - visibility: Visibility::Protected, - allowed_methods: vec![ - AllowedMethod::Get, - AllowedMethod::Delete, - ], - }, - - /* Organizations */ VerifyEndpoint { diff --git a/nexus/tests/integration_tests/silos.rs b/nexus/tests/integration_tests/silos.rs index ab74ffc923..79a7c8991f 100644 --- a/nexus/tests/integration_tests/silos.rs +++ b/nexus/tests/integration_tests/silos.rs @@ -110,7 +110,7 @@ async fn test_silos(cptestctx: &ControlPlaneTestContext) { &client, StatusCode::BAD_REQUEST, Method::DELETE, - &"/silos/default-silo", + &"/silos/fakesilo", ) .authn_as(AuthnMode::PrivilegedUser) .execute() diff --git a/nexus/tests/output/uncovered-authz-endpoints.txt b/nexus/tests/output/uncovered-authz-endpoints.txt index 48197f499b..37134686de 100644 --- a/nexus/tests/output/uncovered-authz-endpoints.txt +++ b/nexus/tests/output/uncovered-authz-endpoints.txt @@ -1,4 +1,8 @@ API endpoints with no coverage in authz tests: +silos_delete_silo (delete "/silos/{silo_name}") session_me (get "/session/me") +silos_get (get "/silos") +silos_get_silo (get "/silos/{silo_name}") spoof_login (post "/login") logout (post "/logout") +silos_post (post "/silos") From 4d1cf3726f92ba0205c5fdff77707ed7f63389c2 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 19 Apr 2022 11:06:08 -0700 Subject: [PATCH 2/3] authz: silo endpoints (take two) --- nexus/src/authn/external/mod.rs | 4 +- nexus/src/authn/external/spoof.rs | 8 +- nexus/src/authn/mod.rs | 2 +- nexus/src/db/datastore.rs | 132 ++++++++---------- nexus/src/db/fixed_data/mod.rs | 2 +- nexus/src/db/fixed_data/silo.rs | 24 ++++ nexus/src/db/fixed_data/silo_builtin.rs | 11 -- nexus/src/db/fixed_data/user_builtin.rs | 2 +- nexus/src/external_api/http_entrypoints.rs | 2 - nexus/src/nexus.rs | 7 +- nexus/tests/integration_tests/endpoints.rs | 36 +++++ nexus/tests/integration_tests/silos.rs | 2 +- .../output/uncovered-authz-endpoints.txt | 4 - 13 files changed, 133 insertions(+), 103 deletions(-) create mode 100644 nexus/src/db/fixed_data/silo.rs delete mode 100644 nexus/src/db/fixed_data/silo_builtin.rs diff --git a/nexus/src/authn/external/mod.rs b/nexus/src/authn/external/mod.rs index e50c15c0c5..dcda0364a2 100644 --- a/nexus/src/authn/external/mod.rs +++ b/nexus/src/authn/external/mod.rs @@ -189,7 +189,7 @@ mod test { let name1 = authn::SchemeName("grunt1"); let actor1 = authn::Actor { id: "1c91bab2-4841-669f-cc32-de80da5bbf39".parse().unwrap(), - silo_id: *crate::db::fixed_data::silo_builtin::SILO_ID, + silo_id: *crate::db::fixed_data::silo::SILO_ID, }; let grunt1 = Box::new(GruntScheme { name: name1, @@ -204,7 +204,7 @@ mod test { let name2 = authn::SchemeName("grunt2"); let actor2 = authn::Actor { id: "799684af-533a-cb66-b5ac-ab55a791d5ef".parse().unwrap(), - silo_id: *crate::db::fixed_data::silo_builtin::SILO_ID, + silo_id: *crate::db::fixed_data::silo::SILO_ID, }; let grunt2 = Box::new(GruntScheme { name: name2, diff --git a/nexus/src/authn/external/spoof.rs b/nexus/src/authn/external/spoof.rs index da1500ba16..d8d85993b5 100644 --- a/nexus/src/authn/external/spoof.rs +++ b/nexus/src/authn/external/spoof.rs @@ -57,7 +57,7 @@ lazy_static! { /// Actor (id) used for the special "bad credentials" error static ref SPOOF_RESERVED_BAD_CREDS_ACTOR: Actor = Actor { id: "22222222-2222-2222-2222-222222222222".parse().unwrap(), - silo_id: *crate::db::fixed_data::silo_builtin::SILO_ID, + silo_id: *crate::db::fixed_data::silo::SILO_ID, }; /// Complete HTTP header value to trigger the "bad actor" error pub static ref SPOOF_HEADER_BAD_ACTOR: Authorization = @@ -122,10 +122,8 @@ fn authn_spoof(raw_value: Option<&Authorization>) -> SchemeResult { match Uuid::parse_str(str_value).context("parsing header value as UUID") { Ok(id) => { - let actor = Actor { - id, - silo_id: *crate::db::fixed_data::silo_builtin::SILO_ID, - }; + let actor = + Actor { id, silo_id: *crate::db::fixed_data::silo::SILO_ID }; SchemeResult::Authenticated(Details { actor }) } Err(source) => SchemeResult::Failed(Reason::BadFormat { source }), diff --git a/nexus/src/authn/mod.rs b/nexus/src/authn/mod.rs index 3520a0b00c..af083fe7b3 100644 --- a/nexus/src/authn/mod.rs +++ b/nexus/src/authn/mod.rs @@ -164,7 +164,7 @@ impl Context { pub fn test_context_for_actor(actor_id: Uuid) -> Context { Context::context_for_actor( actor_id, - *crate::db::fixed_data::silo_builtin::SILO_ID, + *crate::db::fixed_data::silo::SILO_ID, ) } } diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index c3b32c125f..eaae9562df 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -31,7 +31,29 @@ use crate::authz::ApiResourceError; use crate::context::OpContext; use crate::db::fixed_data::role_assignment_builtin::BUILTIN_ROLE_ASSIGNMENTS; use crate::db::fixed_data::role_builtin::BUILTIN_ROLES; -use crate::db::fixed_data::silo_builtin::SILO_ID; +use crate::db::fixed_data::silo::{DEFAULT_SILO, SILO_ID}; +use crate::db::lookup::LookupPath; +use crate::db::{ + self, + error::{public_error_from_diesel_pool, ErrorHandler, TransactionError}, + model::{ + ConsoleSession, Dataset, DatasetKind, Disk, DiskRuntimeState, + Generation, IncompleteNetworkInterface, Instance, InstanceRuntimeState, + Name, NetworkInterface, Organization, OrganizationUpdate, OximeterInfo, + ProducerEndpoint, Project, ProjectUpdate, Region, + RoleAssignmentBuiltin, RoleBuiltin, RouterRoute, RouterRouteUpdate, + Silo, SiloUser, Sled, UpdateAvailableArtifact, UserBuiltin, Volume, + Vpc, VpcFirewallRule, VpcRouter, VpcRouterUpdate, VpcSubnet, + VpcSubnetUpdate, VpcUpdate, Zpool, + }, + pagination::paginated, + pagination::paginated_multicolumn, + subnet_allocation::FilterConflictingVpcSubnetRangesQuery, + subnet_allocation::InsertNetworkInterfaceQuery, + subnet_allocation::NetworkInterfaceError, + subnet_allocation::SubnetError, + update_and_check::{UpdateAndCheck, UpdateStatus}, +}; use crate::external_api::params; use async_bb8_diesel::{AsyncConnection, AsyncRunQueryDsl, ConnectionManager}; use chrono::Utc; @@ -59,29 +81,6 @@ use std::net::Ipv6Addr; use std::sync::Arc; use uuid::Uuid; -use crate::db::lookup::LookupPath; -use crate::db::{ - self, - error::{public_error_from_diesel_pool, ErrorHandler, TransactionError}, - model::{ - ConsoleSession, Dataset, DatasetKind, Disk, DiskRuntimeState, - Generation, IncompleteNetworkInterface, Instance, InstanceRuntimeState, - Name, NetworkInterface, Organization, OrganizationUpdate, OximeterInfo, - ProducerEndpoint, Project, ProjectUpdate, Region, - RoleAssignmentBuiltin, RoleBuiltin, RouterRoute, RouterRouteUpdate, - Silo, SiloUser, Sled, UpdateAvailableArtifact, UserBuiltin, Volume, - Vpc, VpcFirewallRule, VpcRouter, VpcRouterUpdate, VpcSubnet, - VpcSubnetUpdate, VpcUpdate, Zpool, - }, - pagination::paginated, - pagination::paginated_multicolumn, - subnet_allocation::FilterConflictingVpcSubnetRangesQuery, - subnet_allocation::InsertNetworkInterfaceQuery, - subnet_allocation::NetworkInterfaceError, - subnet_allocation::SubnetError, - update_and_check::{UpdateAndCheck, UpdateStatus}, -}; - // Number of unique datasets required to back a region. // TODO: This should likely turn into a configuration option. const REGION_REDUNDANCY_THRESHOLD: usize = 3; @@ -2488,22 +2487,21 @@ impl DataStore { &self, opctx: &OpContext, ) -> Result<(), Error> { - debug!(opctx.log, "attempting to create built-in silo"); - - let builtin_silo = Silo::new_with_id( - *SILO_ID, - params::SiloCreate { - identity: IdentityMetadataCreateParams { - name: "fakesilo".parse().unwrap(), - description: "fake silo".to_string(), - }, - discoverable: false, - }, - ); + opctx.authorize(authz::Action::Modify, &authz::DATABASE).await?; - let _create_result = self.silo_create(opctx, builtin_silo).await?; - info!(opctx.log, "created built-in silo"); + debug!(opctx.log, "attempting to create built-in silo"); + use db::schema::silo::dsl; + let count = diesel::insert_into(dsl::silo) + .values(&*DEFAULT_SILO) + .on_conflict(dsl::id) + .do_nothing() + .execute_async(self.pool_authorized(opctx).await?) + .await + .map_err(|e| { + public_error_from_diesel_pool(e, ErrorHandler::Server) + })?; + info!(opctx.log, "created {} built-in silos", count); Ok(()) } @@ -2512,12 +2510,11 @@ impl DataStore { opctx: &OpContext, silo: Silo, ) -> CreateResult { - use db::schema::silo::dsl; - - // TODO opctx.authorize + opctx.authorize(authz::Action::CreateChild, &authz::FLEET).await?; let silo_id = silo.id(); + use db::schema::silo::dsl; diesel::insert_into(dsl::silo) .values(silo) .returning(Silo::as_returning()) @@ -2539,8 +2536,9 @@ impl DataStore { opctx: &OpContext, pagparams: &DataPageParams<'_, Uuid>, ) -> ListResultVec { + opctx.authorize(authz::Action::ListChildren, &authz::FLEET).await?; + use db::schema::silo::dsl; - // TODO opctx.authorize paginated(dsl::silo, dsl::id, pagparams) .filter(dsl::time_deleted.is_null()) .filter(dsl::discoverable.eq(true)) @@ -2555,8 +2553,9 @@ impl DataStore { opctx: &OpContext, pagparams: &DataPageParams<'_, Name>, ) -> ListResultVec { + opctx.authorize(authz::Action::ListChildren, &authz::FLEET).await?; + use db::schema::silo::dsl; - // TODO opctx.authorize paginated(dsl::silo, dsl::name, pagparams) .filter(dsl::time_deleted.is_null()) .filter(dsl::discoverable.eq(true)) @@ -2569,38 +2568,26 @@ impl DataStore { pub async fn silo_delete( &self, opctx: &OpContext, - name: &Name, + authz_silo: &authz::Silo, + db_silo: &db::model::Silo, ) -> DeleteResult { + assert_eq!(authz_silo.id(), db_silo.id()); + opctx.authorize(authz::Action::Delete, authz_silo).await?; + use db::schema::organization; use db::schema::silo; use db::schema::silo_user; - // TODO opctx.authorize - - let (id, rcgen) = silo::dsl::silo - .filter(silo::dsl::time_deleted.is_null()) - .filter(silo::dsl::name.eq(name.clone())) - .select((silo::dsl::id, silo::dsl::rcgen)) - .get_result_async::<(Uuid, Generation)>(self.pool()) - .await - .map_err(|e| { - public_error_from_diesel_pool( - e, - ErrorHandler::NotFoundByLookup( - ResourceType::Silo, - LookupType::ByName(name.to_string()), - ), - ) - })?; - // Make sure there are no organizations present within this silo. + let id = authz_silo.id(); + let rcgen = db_silo.rcgen; let org_found = diesel_pool_result_optional( organization::dsl::organization .filter(organization::dsl::silo_id.eq(id)) .filter(organization::dsl::time_deleted.is_null()) .select(organization::dsl::id) .limit(1) - .first_async::(self.pool()) + .first_async::(self.pool_authorized(opctx).await?) .await, ) .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server))?; @@ -2618,15 +2605,12 @@ impl DataStore { .filter(silo::dsl::id.eq(id)) .filter(silo::dsl::rcgen.eq(rcgen)) .set(silo::dsl::time_deleted.eq(now)) - .execute_async(self.pool()) + .execute_async(self.pool_authorized(opctx).await?) .await .map_err(|e| { public_error_from_diesel_pool( e, - ErrorHandler::NotFoundByLookup( - ResourceType::Silo, - LookupType::ById(id), - ), + ErrorHandler::NotFoundByResource(authz_silo), ) })?; @@ -2640,22 +2624,22 @@ impl DataStore { info!(opctx.log, "deleted silo {}", id); // If silo deletion succeeded, delete all silo users + // TODO-correctness This needs to happen in a saga or some other + // mechanism that ensures it happens even if we crash at this point. + // TODO-scalability This needs to happen in batches let updated_rows = diesel::update(silo_user::dsl::silo_user) .filter(silo_user::dsl::silo_id.eq(id)) .set(silo_user::dsl::time_deleted.eq(now)) - .execute_async(self.pool()) + .execute_async(self.pool_authorized(opctx).await?) .await .map_err(|e| { public_error_from_diesel_pool( e, - ErrorHandler::NotFoundByLookup( - ResourceType::Silo, - LookupType::ById(id), - ), + ErrorHandler::NotFoundByResource(authz_silo), ) })?; - info!(opctx.log, "deleted {} silo users for silo {}", updated_rows, id,); + info!(opctx.log, "deleted {} silo users for silo {}", updated_rows, id); Ok(()) } diff --git a/nexus/src/db/fixed_data/mod.rs b/nexus/src/db/fixed_data/mod.rs index 0137eec2ae..57ddf92987 100644 --- a/nexus/src/db/fixed_data/mod.rs +++ b/nexus/src/db/fixed_data/mod.rs @@ -32,7 +32,7 @@ use lazy_static::lazy_static; pub mod role_assignment_builtin; pub mod role_builtin; -pub mod silo_builtin; +pub mod silo; pub mod user_builtin; lazy_static! { diff --git a/nexus/src/db/fixed_data/silo.rs b/nexus/src/db/fixed_data/silo.rs new file mode 100644 index 0000000000..e328da8cf7 --- /dev/null +++ b/nexus/src/db/fixed_data/silo.rs @@ -0,0 +1,24 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/. + +use crate::db; +use crate::external_api::params; +use lazy_static::lazy_static; +use omicron_common::api::external::IdentityMetadataCreateParams; + +lazy_static! { + pub static ref SILO_ID: uuid::Uuid = "001de000-5110-4000-8000-000000000000" + .parse() + .expect("invalid uuid for builtin silo id"); + pub static ref DEFAULT_SILO: db::model::Silo = db::model::Silo::new_with_id( + *SILO_ID, + params::SiloCreate { + identity: IdentityMetadataCreateParams { + name: "default-silo".parse().unwrap(), + description: "default silo".to_string(), + }, + discoverable: false, + } + ); +} diff --git a/nexus/src/db/fixed_data/silo_builtin.rs b/nexus/src/db/fixed_data/silo_builtin.rs deleted file mode 100644 index 6610f673c4..0000000000 --- a/nexus/src/db/fixed_data/silo_builtin.rs +++ /dev/null @@ -1,11 +0,0 @@ -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this -// file, You can obtain one at https://mozilla.org/MPL/2.0/. - -use lazy_static::lazy_static; - -lazy_static! { - pub static ref SILO_ID: uuid::Uuid = "001de000-5110-4000-8000-000000000000" - .parse() - .expect("invalid uuid for builtin silo id"); -} diff --git a/nexus/src/db/fixed_data/user_builtin.rs b/nexus/src/db/fixed_data/user_builtin.rs index 22e83a60a3..f878feb187 100644 --- a/nexus/src/db/fixed_data/user_builtin.rs +++ b/nexus/src/db/fixed_data/user_builtin.rs @@ -3,7 +3,7 @@ // file, You can obtain one at https://mozilla.org/MPL/2.0/. //! Built-in users -use crate::db::fixed_data::silo_builtin::SILO_ID; +use crate::db::fixed_data::silo::SILO_ID; use lazy_static::lazy_static; use omicron_common::api; use uuid::Uuid; diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index ad24c4b748..90046ff4c8 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -230,8 +230,6 @@ pub fn external_api() -> NexusApiDescription { // clients. Client generators use operationId to name API methods, so changing // a function name is a breaking change from a client perspective. -// TODO authz for silo endpoints - // List all silos (that are discoverable). #[endpoint { method = GET, diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index 8c0ed86cc8..c8c6bf1488 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -738,7 +738,12 @@ impl Nexus { opctx: &OpContext, name: &Name, ) -> DeleteResult { - self.db_datastore.silo_delete(opctx, name).await + let (.., authz_silo, db_silo) = + LookupPath::new(opctx, &self.db_datastore) + .silo_name(name) + .fetch_for(authz::Action::Delete) + .await?; + self.db_datastore.silo_delete(opctx, &authz_silo, &db_silo).await } // Organizations diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index cc31c02c04..d428c64113 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -23,6 +23,8 @@ use omicron_common::api::external::RouterRouteCreateParams; use omicron_common::api::external::RouterRouteUpdateParams; use omicron_common::api::external::VpcFirewallRuleUpdateParams; use omicron_nexus::authn; +use omicron_nexus::db::fixed_data::silo::DEFAULT_SILO; +use omicron_nexus::db::identity::Resource; use omicron_nexus::external_api::params; use std::net::IpAddr; use std::net::Ipv4Addr; @@ -33,6 +35,19 @@ lazy_static! { pub static ref HARDWARE_SLED_URL: String = format!("/hardware/sleds/{}", SLED_AGENT_UUID); + // Silo used for testing + pub static ref DEFAULT_SILO_NAME: &'static Name = &DEFAULT_SILO.name().0; + pub static ref DEFAULT_SILO_URL: String = + format!("/silos/{}", *DEFAULT_SILO_NAME); + pub static ref DEMO_SILO_CREATE: params::SiloCreate = + params::SiloCreate { + identity: IdentityMetadataCreateParams { + name: DEFAULT_SILO_NAME.clone(), + description: String::from(""), + }, + discoverable: true, + }; + // Organization used for testing pub static ref DEMO_ORG_NAME: Name = "demo-org".parse().unwrap(); pub static ref DEMO_ORG_URL: String = @@ -349,6 +364,27 @@ lazy_static! { /// List of endpoints to be verified pub static ref VERIFY_ENDPOINTS: Vec = vec![ + /* Silos */ + VerifyEndpoint { + url: "/silos", + visibility: Visibility::Public, + allowed_methods: vec![ + AllowedMethod::Get, + AllowedMethod::Post( + serde_json::to_value(&*DEMO_SILO_CREATE).unwrap() + ) + ], + }, + VerifyEndpoint { + url: &*DEFAULT_SILO_URL, + visibility: Visibility::Protected, + allowed_methods: vec![ + AllowedMethod::Get, + AllowedMethod::Delete, + ], + }, + + /* Organizations */ VerifyEndpoint { diff --git a/nexus/tests/integration_tests/silos.rs b/nexus/tests/integration_tests/silos.rs index 79a7c8991f..ab74ffc923 100644 --- a/nexus/tests/integration_tests/silos.rs +++ b/nexus/tests/integration_tests/silos.rs @@ -110,7 +110,7 @@ async fn test_silos(cptestctx: &ControlPlaneTestContext) { &client, StatusCode::BAD_REQUEST, Method::DELETE, - &"/silos/fakesilo", + &"/silos/default-silo", ) .authn_as(AuthnMode::PrivilegedUser) .execute() diff --git a/nexus/tests/output/uncovered-authz-endpoints.txt b/nexus/tests/output/uncovered-authz-endpoints.txt index 37134686de..48197f499b 100644 --- a/nexus/tests/output/uncovered-authz-endpoints.txt +++ b/nexus/tests/output/uncovered-authz-endpoints.txt @@ -1,8 +1,4 @@ API endpoints with no coverage in authz tests: -silos_delete_silo (delete "/silos/{silo_name}") session_me (get "/session/me") -silos_get (get "/silos") -silos_get_silo (get "/silos/{silo_name}") spoof_login (post "/login") logout (post "/logout") -silos_post (post "/silos") From 8eca0cebbfbeae14ae78fecc06f3efd6c6df7303 Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 19 Apr 2022 11:15:07 -0700 Subject: [PATCH 3/3] fix tests --- nexus/tests/integration_tests/endpoints.rs | 12 +++++------- nexus/tests/integration_tests/unauthorized.rs | 5 +++++ 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/nexus/tests/integration_tests/endpoints.rs b/nexus/tests/integration_tests/endpoints.rs index d428c64113..0cac4b1ca9 100644 --- a/nexus/tests/integration_tests/endpoints.rs +++ b/nexus/tests/integration_tests/endpoints.rs @@ -23,8 +23,6 @@ use omicron_common::api::external::RouterRouteCreateParams; use omicron_common::api::external::RouterRouteUpdateParams; use omicron_common::api::external::VpcFirewallRuleUpdateParams; use omicron_nexus::authn; -use omicron_nexus::db::fixed_data::silo::DEFAULT_SILO; -use omicron_nexus::db::identity::Resource; use omicron_nexus::external_api::params; use std::net::IpAddr; use std::net::Ipv4Addr; @@ -36,13 +34,13 @@ lazy_static! { format!("/hardware/sleds/{}", SLED_AGENT_UUID); // Silo used for testing - pub static ref DEFAULT_SILO_NAME: &'static Name = &DEFAULT_SILO.name().0; - pub static ref DEFAULT_SILO_URL: String = - format!("/silos/{}", *DEFAULT_SILO_NAME); + pub static ref DEMO_SILO_NAME: Name = "demo-silo".parse().unwrap(); + pub static ref DEMO_SILO_URL: String = + format!("/silos/{}", *DEMO_SILO_NAME); pub static ref DEMO_SILO_CREATE: params::SiloCreate = params::SiloCreate { identity: IdentityMetadataCreateParams { - name: DEFAULT_SILO_NAME.clone(), + name: DEMO_SILO_NAME.clone(), description: String::from(""), }, discoverable: true, @@ -376,7 +374,7 @@ lazy_static! { ], }, VerifyEndpoint { - url: &*DEFAULT_SILO_URL, + url: &*DEMO_SILO_URL, visibility: Visibility::Protected, allowed_methods: vec![ AllowedMethod::Get, diff --git a/nexus/tests/integration_tests/unauthorized.rs b/nexus/tests/integration_tests/unauthorized.rs index b3c7e4cb09..ff5fc64787 100644 --- a/nexus/tests/integration_tests/unauthorized.rs +++ b/nexus/tests/integration_tests/unauthorized.rs @@ -124,6 +124,11 @@ struct SetupReq { lazy_static! { /// List of requests to execute at setup time static ref SETUP_REQUESTS: Vec = vec![ + // Create a separate Silo (not used for anything else) + SetupReq { + url: "/silos", + body: serde_json::to_value(&*DEMO_SILO_CREATE).unwrap(), + }, // Create an Organization SetupReq { url: "/organizations",