diff --git a/nexus/src/db/datastore.rs b/nexus/src/db/datastore.rs index b2419ea9f8e..08050cab5ca 100644 --- a/nexus/src/db/datastore.rs +++ b/nexus/src/db/datastore.rs @@ -2336,41 +2336,47 @@ impl DataStore { pub async fn vpc_list_firewall_rules( &self, - vpc_id: &Uuid, + opctx: &OpContext, + authz_vpc: &authz::Vpc, ) -> ListResultVec { + // Firewall rules are modeled in the API as a single resource under the + // Vpc (rather than individual child resources with their own CRUD + // endpoints). You cannot look them up individually, create them, + // remove them, or update them. You can only modify the whole set. So + // for authz, we treat them as part of the Vpc itself. + opctx.authorize(authz::Action::Read, authz_vpc).await?; use db::schema::vpc_firewall_rule::dsl; dsl::vpc_firewall_rule .filter(dsl::time_deleted.is_null()) - .filter(dsl::vpc_id.eq(*vpc_id)) + .filter(dsl::vpc_id.eq(authz_vpc.id())) .order(dsl::name.asc()) .select(VpcFirewallRule::as_select()) - .load_async(self.pool()) + .load_async(self.pool_authorized(opctx).await?) .await .map_err(|e| public_error_from_diesel_pool(e, ErrorHandler::Server)) } pub async fn vpc_delete_all_firewall_rules( &self, - vpc_id: &Uuid, + opctx: &OpContext, + authz_vpc: &authz::Vpc, ) -> DeleteResult { + opctx.authorize(authz::Action::Modify, authz_vpc).await?; use db::schema::vpc_firewall_rule::dsl; let now = Utc::now(); // TODO-performance: Paginate this update to avoid long queries diesel::update(dsl::vpc_firewall_rule) .filter(dsl::time_deleted.is_null()) - .filter(dsl::vpc_id.eq(*vpc_id)) + .filter(dsl::vpc_id.eq(authz_vpc.id())) .set(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::Vpc, - LookupType::ById(*vpc_id), - ), + ErrorHandler::NotFoundByResource(authz_vpc), ) })?; Ok(()) @@ -2379,19 +2385,31 @@ impl DataStore { /// Replace all firewall rules with the given rules pub async fn vpc_update_firewall_rules( &self, - vpc_id: &Uuid, - rules: Vec, + opctx: &OpContext, + authz_vpc: &authz::Vpc, + mut rules: Vec, ) -> UpdateResult> { + opctx.authorize(authz::Action::Modify, authz_vpc).await?; + for r in &rules { + assert_eq!(r.vpc_id, authz_vpc.id()); + } + + // Sort the rules in the same order that we would return them when + // listing them. This is because we're going to use RETURNING to return + // the inserted rows from the database and we want them to come back in + // the same order that we would normally list them. + rules.sort_by_key(|r| r.name().to_string()); + use db::schema::vpc_firewall_rule::dsl; let now = Utc::now(); let delete_old_query = diesel::update(dsl::vpc_firewall_rule) .filter(dsl::time_deleted.is_null()) - .filter(dsl::vpc_id.eq(*vpc_id)) + .filter(dsl::vpc_id.eq(authz_vpc.id())) .set(dsl::time_deleted.eq(now)); let insert_new_query = Vpc::insert_resource( - *vpc_id, + authz_vpc.id(), diesel::insert_into(dsl::vpc_firewall_rule).values(rules), ); @@ -2405,7 +2423,8 @@ impl DataStore { // hold a transaction open across multiple roundtrips from the database, // but for now we're using a transaction due to the severely decreased // legibility of CTEs via diesel right now. - self.pool() + self.pool_authorized(opctx) + .await? .transaction(move |conn| { delete_old_query.execute(conn)?; @@ -2427,13 +2446,10 @@ impl DataStore { .map_err(|e| match e { TxnError::CustomError( FirewallUpdateError::CollectionNotFound, - ) => Error::not_found_by_id(ResourceType::Vpc, vpc_id), + ) => Error::not_found_by_id(ResourceType::Vpc, &authz_vpc.id()), TxnError::Pool(e) => public_error_from_diesel_pool( e, - ErrorHandler::NotFoundByLookup( - ResourceType::VpcFirewallRule, - LookupType::ById(*vpc_id), - ), + ErrorHandler::NotFoundByResource(authz_vpc), ), }) } diff --git a/nexus/src/external_api/http_entrypoints.rs b/nexus/src/external_api/http_entrypoints.rs index 7aecf8a114d..75858456220 100644 --- a/nexus/src/external_api/http_entrypoints.rs +++ b/nexus/src/external_api/http_entrypoints.rs @@ -1719,8 +1719,10 @@ async fn vpc_firewall_rules_get( let nexus = &apictx.nexus; let path = path_params.into_inner(); let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; let rules = nexus .vpc_list_firewall_rules( + &opctx, &path.organization_name, &path.project_name, &path.vpc_name, @@ -1750,8 +1752,10 @@ async fn vpc_firewall_rules_put( let nexus = &apictx.nexus; let path = path_params.into_inner(); let handler = async { + let opctx = OpContext::for_external_api(&rqctx).await?; let rules = nexus .vpc_update_firewall_rules( + &opctx, &path.organization_name, &path.project_name, &path.vpc_name, diff --git a/nexus/src/nexus.rs b/nexus/src/nexus.rs index 302e98936bf..96131ff6e9e 100644 --- a/nexus/src/nexus.rs +++ b/nexus/src/nexus.rs @@ -1808,20 +1808,14 @@ impl Nexus { } SubnetError::External(e) => e, })?; - self.create_default_vpc_firewall(&vpc_id).await?; - Ok(db_vpc) - } - - async fn create_default_vpc_firewall( - &self, - vpc_id: &Uuid, - ) -> CreateResult<()> { let rules = db::model::VpcFirewallRule::vec_from_params( - *vpc_id, + authz_vpc.id(), defaults::DEFAULT_FIREWALL_RULES.clone(), ); - self.db_datastore.vpc_update_firewall_rules(&vpc_id, rules).await?; - Ok(()) + self.db_datastore + .vpc_update_firewall_rules(opctx, &authz_vpc, rules) + .await?; + Ok(db_vpc) } pub async fn vpc_fetch( @@ -1842,23 +1836,6 @@ impl Nexus { .1) } - // TODO-security TODO-cleanup Remove this function. Callers should use - // vpc_lookup_by_path() / vpc_fetch() instead, or we should create a more - // useful pattern for looking up records by path (e.g., *_fetch_by_path()). - pub async fn project_lookup_vpc( - &self, - organization_name: &Name, - project_name: &Name, - vpc_name: &Name, - ) -> LookupResult { - let project_id = self - .db_datastore - .project_lookup_by_path(organization_name, project_name) - .await? - .id(); - Ok(self.db_datastore.vpc_fetch_by_name(&project_id, vpc_name).await?) - } - pub async fn project_update_vpc( &self, opctx: &OpContext, @@ -1904,38 +1881,48 @@ impl Nexus { // Delete all firewall rules after deleting the VPC, to ensure no // firewall rules get added between rules deletion and VPC deletion. - self.db_datastore.vpc_delete_all_firewall_rules(&authz_vpc.id()).await + self.db_datastore + .vpc_delete_all_firewall_rules(&opctx, &authz_vpc) + .await } pub async fn vpc_list_firewall_rules( &self, + opctx: &OpContext, organization_name: &Name, project_name: &Name, vpc_name: &Name, ) -> ListResultVec { - let vpc = self - .project_lookup_vpc(organization_name, project_name, vpc_name) + let authz_vpc = self + .db_datastore + .vpc_lookup_by_path(organization_name, project_name, vpc_name) + .await?; + let rules = self + .db_datastore + .vpc_list_firewall_rules(&opctx, &authz_vpc) .await?; - let rules = - self.db_datastore.vpc_list_firewall_rules(&vpc.id()).await?; Ok(rules) } pub async fn vpc_update_firewall_rules( &self, + opctx: &OpContext, organization_name: &Name, project_name: &Name, vpc_name: &Name, params: &VpcFirewallRuleUpdateParams, ) -> UpdateResult> { - let vpc = self - .project_lookup_vpc(organization_name, project_name, vpc_name) + let authz_vpc = self + .db_datastore + .vpc_lookup_by_path(organization_name, project_name, vpc_name) .await?; let rules = db::model::VpcFirewallRule::vec_from_params( - vpc.id(), + authz_vpc.id(), params.clone(), ); - self.db_datastore.vpc_update_firewall_rules(&vpc.id(), rules).await + self.db_datastore + .vpc_update_firewall_rules(opctx, &authz_vpc, rules) + .await } pub async fn vpc_list_subnets( diff --git a/nexus/tests/integration_tests/unauthorized.rs b/nexus/tests/integration_tests/unauthorized.rs index d3234a45827..0b2c7bd27c4 100644 --- a/nexus/tests/integration_tests/unauthorized.rs +++ b/nexus/tests/integration_tests/unauthorized.rs @@ -31,6 +31,7 @@ use omicron_common::api::external::RouteDestination; use omicron_common::api::external::RouteTarget; 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::authn::external::spoof; use omicron_nexus::external_api::params; @@ -182,10 +183,12 @@ lazy_static! { static ref DEMO_VPC_NAME: Name = "demo-vpc".parse().unwrap(); static ref DEMO_VPC_URL: String = format!("{}/{}", *DEMO_PROJECT_URL_VPCS, *DEMO_VPC_NAME); - static ref DEMO_VPC_URL_SUBNETS: String = - format!("{}/subnets", *DEMO_VPC_URL); + static ref DEMO_VPC_URL_FIREWALL_RULES: String = + format!("{}/firewall/rules", *DEMO_VPC_URL); static ref DEMO_VPC_URL_ROUTERS: String = format!("{}/routers", *DEMO_VPC_URL); + static ref DEMO_VPC_URL_SUBNETS: String = + format!("{}/subnets", *DEMO_VPC_URL); static ref DEMO_VPC_CREATE: params::VpcCreate = params::VpcCreate { identity: IdentityMetadataCreateParams { @@ -473,6 +476,20 @@ lazy_static! { ], }, + /* Firewall rules */ + VerifyEndpoint { + url: &*DEMO_VPC_URL_FIREWALL_RULES, + visibility: Visibility::Protected, + allowed_methods: vec![ + AllowedMethod::Get, + AllowedMethod::Put( + serde_json::to_value(VpcFirewallRuleUpdateParams { + rules: vec![], + }).unwrap() + ), + ], + }, + /* VPC Subnets */ VerifyEndpoint { url: &*DEMO_VPC_URL_SUBNETS, diff --git a/nexus/tests/integration_tests/vpc_firewall.rs b/nexus/tests/integration_tests/vpc_firewall.rs index f0fcb92de35..d60c0d35bdf 100644 --- a/nexus/tests/integration_tests/vpc_firewall.rs +++ b/nexus/tests/integration_tests/vpc_firewall.rs @@ -2,7 +2,6 @@ // 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 dropshot::test_util::object_get; use http::method::Method; use http::StatusCode; use nexus_test_utils::http_testing::{AuthnMode, NexusRequest}; @@ -45,9 +44,7 @@ async fn test_vpc_firewall(cptestctx: &ControlPlaneTestContext) { .unwrap(); let default_vpc_firewall = format!("{}/firewall/rules", default_vpc_url); - let rules = object_get::(client, &default_vpc_firewall) - .await - .rules; + let rules = get_rules(client, &default_vpc_firewall).await; assert!(rules.iter().all(|r| r.vpc_id == default_vpc.identity.id)); assert!(is_default_firewall_rules(&rules)); @@ -56,8 +53,7 @@ async fn test_vpc_firewall(cptestctx: &ControlPlaneTestContext) { let other_vpc_firewall = format!("{}/{}/firewall/rules", vpcs_url, other_vpc); let vpc2 = create_vpc(&client, &org_name, &project_name, &other_vpc).await; - let rules = - object_get::(client, &other_vpc_firewall).await.rules; + let rules = get_rules(client, &other_vpc_firewall).await; assert!(rules.iter().all(|r| r.vpc_id == vpc2.identity.id)); assert!(is_default_firewall_rules(&rules)); @@ -98,38 +94,45 @@ async fn test_vpc_firewall(cptestctx: &ControlPlaneTestContext) { ]; let update_params = VpcFirewallRuleUpdateParams { rules: new_rules.clone() }; - client - .make_request( - Method::PUT, - &default_vpc_firewall, - Some(update_params), - StatusCode::OK, - ) - .await - .unwrap(); + let updated_rules = NexusRequest::object_put( + client, + &default_vpc_firewall, + Some(&update_params), + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body::() + .unwrap() + .rules; + assert!(!is_default_firewall_rules(&updated_rules)); + assert_eq!(updated_rules.len(), new_rules.len()); + assert_eq!(updated_rules[0].identity.name, "allow-icmp"); + assert_eq!(updated_rules[1].identity.name, "deny-all-incoming"); // Make sure the firewall is changed - let rules = object_get::(client, &default_vpc_firewall) - .await - .rules; + let rules = get_rules(client, &default_vpc_firewall).await; assert!(!is_default_firewall_rules(&rules)); assert_eq!(rules.len(), new_rules.len()); assert_eq!(rules[0].identity.name, "allow-icmp"); assert_eq!(rules[1].identity.name, "deny-all-incoming"); // Make sure the other firewall is unchanged - let rules = - object_get::(client, &other_vpc_firewall).await.rules; + let rules = get_rules(client, &other_vpc_firewall).await; assert!(is_default_firewall_rules(&rules)); - // DELETE is unspported - client - .make_request_error( - Method::DELETE, - &default_vpc_firewall, - StatusCode::METHOD_NOT_ALLOWED, - ) - .await; + // DELETE is unsupported + NexusRequest::expect_failure( + client, + StatusCode::METHOD_NOT_ALLOWED, + Method::DELETE, + &default_vpc_firewall, + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap(); // Delete a VPC and ensure we can't read its firewall anymore NexusRequest::object_delete( @@ -140,13 +143,30 @@ async fn test_vpc_firewall(cptestctx: &ControlPlaneTestContext) { .execute() .await .unwrap(); - client - .make_request_error( - Method::GET, - &other_vpc_firewall, - StatusCode::NOT_FOUND, - ) - .await; + NexusRequest::expect_failure( + client, + StatusCode::NOT_FOUND, + Method::GET, + &other_vpc_firewall, + ) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap(); +} + +async fn get_rules( + client: &dropshot::test_util::ClientTestContext, + url: &str, +) -> Vec { + NexusRequest::object_get(client, url) + .authn_as(AuthnMode::PrivilegedUser) + .execute() + .await + .unwrap() + .parsed_body::() + .unwrap() + .rules } fn is_default_firewall_rules(rules: &Vec) -> bool {