From debd3090f7f08d6059c1db5d9f5c1222d012cb8d Mon Sep 17 00:00:00 2001 From: David Pacheco Date: Tue, 19 Apr 2022 10:24:08 -0700 Subject: [PATCH] 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 dcda0364a27..e50c15c0c59 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 d8d85993b57..da1500ba16a 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 af083fe7b31..3520a0b00cc 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 eaae9562dfb..c3b32c125f8 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 57ddf929872..0137eec2ae6 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 e328da8cf7a..00000000000 --- 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 00000000000..6610f673c4e --- /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 f878feb1874..22e83a60a34 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 90046ff4c89..ad24c4b7484 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 c8c6bf14888..8c0ed86cc88 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 d428c641135..cc31c02c042 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 ab74ffc9233..79a7c8991fa 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 48197f499b2..37134686de2 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")