authz: protect endpoints for firewall rules (#782)

davepacheco · web-flow · commit 5a071cdaafe8 · 2022-03-17T20:40:23.000Z
diff --git 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<VpcFirewallRule> {
+        // 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<VpcFirewallRule>,
+        opctx: &OpContext,
+        authz_vpc: &authz::Vpc,
+        mut rules: Vec<VpcFirewallRule>,
     ) -> UpdateResult<Vec<VpcFirewallRule>> {
+        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
@@ -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
@@ -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<db::model::Vpc> {
-        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<db::model::VpcFirewallRule> {
-        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<Vec<db::model::VpcFirewallRule>> {
-        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
@@ -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
