Limit number of firewall rules per VPC (#5914)

Closes #5662 
Closes #6032

- [x] Max firewall rules
- [x] Enforce max length on filters (hosts, protocols, and ports) and
targets too
- [x] Improve doc comments on firewall rules types and API endpoint

Author: david-crespo

common/src/api/external/mod.rs

@@ -1467,7 +1467,7 @@ pub enum RouterRouteKind {
 /// its destination.
 #[derive(ObjectIdentity, Clone, Debug, Deserialize, Serialize, JsonSchema)]
 pub struct RouterRoute {
-    /// common identifying metadata
+    /// Common identifying metadata
     #[serde(flatten)]
     pub identity: IdentityMetadata,
     /// The ID of the VPC Router to which the route belongs
@@ -1483,22 +1483,22 @@ pub struct RouterRoute {
 /// A single rule in a VPC firewall
 #[derive(ObjectIdentity, Clone, Debug, Deserialize, Serialize, JsonSchema)]
 pub struct VpcFirewallRule {
-    /// common identifying metadata
+    /// Common identifying metadata
     #[serde(flatten)]
     pub identity: IdentityMetadata,
-    /// whether this rule is in effect
+    /// Whether this rule is in effect
     pub status: VpcFirewallRuleStatus,
-    /// whether this rule is for incoming or outgoing traffic
+    /// Whether this rule is for incoming or outgoing traffic
     pub direction: VpcFirewallRuleDirection,
-    /// list of sets of instances that the rule applies to
+    /// Determine the set of instances that the rule applies to
     pub targets: Vec<VpcFirewallRuleTarget>,
-    /// reductions on the scope of the rule
+    /// Reductions on the scope of the rule
     pub filters: VpcFirewallRuleFilter,
-    /// whether traffic matching the rule should be allowed or dropped
+    /// Whether traffic matching the rule should be allowed or dropped
     pub action: VpcFirewallRuleAction,
-    /// the relative priority of this rule
+    /// The relative priority of this rule
     pub priority: VpcFirewallRulePriority,
-    /// the VPC to which this rule belongs
+    /// The VPC to which this rule belongs
     pub vpc_id: Uuid,
 }
 
@@ -1511,29 +1511,29 @@ pub struct VpcFirewallRules {
 /// A single rule in a VPC firewall
 #[derive(Clone, Debug, Deserialize, PartialEq, Serialize, JsonSchema)]
 pub struct VpcFirewallRuleUpdate {
-    /// name of the rule, unique to this VPC
+    /// Name of the rule, unique to this VPC
     pub name: Name,
-    /// human-readable free-form text about a resource
+    /// Human-readable free-form text about a resource
     pub description: String,
-    /// whether this rule is in effect
+    /// Whether this rule is in effect
     pub status: VpcFirewallRuleStatus,
-    /// whether this rule is for incoming or outgoing traffic
+    /// Whether this rule is for incoming or outgoing traffic
     pub direction: VpcFirewallRuleDirection,
-    /// list of sets of instances that the rule applies to
+    /// Determine the set of instances that the rule applies to
+    #[schemars(length(max = 256))]
     pub targets: Vec<VpcFirewallRuleTarget>,
-    /// reductions on the scope of the rule
+    /// Reductions on the scope of the rule
     pub filters: VpcFirewallRuleFilter,
-    /// whether traffic matching the rule should be allowed or dropped
+    /// Whether traffic matching the rule should be allowed or dropped
     pub action: VpcFirewallRuleAction,
-    /// the relative priority of this rule
+    /// The relative priority of this rule
     pub priority: VpcFirewallRulePriority,
 }
 
-/// Updateable properties of a `Vpc`'s firewall
-/// Note that VpcFirewallRules are implicitly created along with a Vpc,
-/// so there is no explicit creation.
+/// Updated list of firewall rules. Will replace all existing rules.
 #[derive(Clone, Debug, Deserialize, Serialize, JsonSchema)]
 pub struct VpcFirewallRuleUpdateParams {
+    #[schemars(length(max = 1024))]
     pub rules: Vec<VpcFirewallRuleUpdate>,
 }
 
@@ -1553,19 +1553,24 @@ pub struct VpcFirewallRuleUpdateParams {
 #[repr(transparent)]
 pub struct VpcFirewallRulePriority(pub u16);
 
-/// Filter for a firewall rule. A given packet must match every field that is
-/// present for the rule to apply to it. A packet matches a field if any entry
-/// in that field matches the packet.
+/// Filters reduce the scope of a firewall rule. Without filters, the rule
+/// applies to all packets to the targets (or from the targets, if it's an
+/// outbound rule). With multiple filters, the rule applies only to packets
+/// matching ALL filters. The maximum number of each type of filter is 256.
 #[derive(Clone, Debug, PartialEq, Deserialize, Serialize, JsonSchema)]
 pub struct VpcFirewallRuleFilter {
-    /// If present, the sources (if incoming) or destinations (if outgoing)
-    /// this rule applies to.
+    /// If present, host filters match the "other end" of traffic from the
+    /// target’s perspective: for an inbound rule, they match the source of
+    /// traffic. For an outbound rule, they match the destination.
+    #[schemars(length(max = 256))]
     pub hosts: Option<Vec<VpcFirewallRuleHostFilter>>,
 
     /// If present, the networking protocols this rule applies to.
+    #[schemars(length(max = 256))]
     pub protocols: Option<Vec<VpcFirewallRuleProtocol>>,
 
-    /// If present, the destination ports this rule applies to.
+    /// If present, the destination ports or port ranges this rule applies to.
+    #[schemars(length(max = 256))]
     pub ports: Option<Vec<L4PortRange>>,
 }
 
@@ -1599,8 +1604,11 @@ pub enum VpcFirewallRuleAction {
     Deny,
 }
 
-/// A `VpcFirewallRuleTarget` is used to specify the set of `Instance`s to
-/// which a firewall rule applies.
+/// A `VpcFirewallRuleTarget` is used to specify the set of instances to which
+/// a firewall rule applies. You can target instances directly by name, or
+/// specify a VPC, VPC subnet, IP, or IP subnet, which will apply the rule to
+/// traffic going to all matching instances. Targets are additive: the rule
+/// applies to instances matching ANY target.
 #[derive(
     Clone,
     Debug,
@@ -1760,7 +1768,7 @@ impl JsonSchema for L4PortRange {
                 title: Some("A range of IP ports".to_string()),
                 description: Some(
                     "An inclusive-inclusive range of IP ports. The second port \
-                    may be omitted to represent a single port"
+                    may be omitted to represent a single port."
                         .to_string(),
                 ),
                 examples: vec!["22".into(), "6667-7000".into()],

nexus/db-model/src/vpc_firewall_rule.rs

@@ -211,20 +211,55 @@ pub struct VpcFirewallRule {
     pub priority: VpcFirewallRulePriority,
 }
 
+/// Cap on the number of rules in a VPC
+///
+/// The choice of value is somewhat arbitrary, but the goal is to have a
+/// large number that customers are unlikely to actually hit, but which still
+/// meaningfully limits the ability to overload the DB with a single request.
+const MAX_FW_RULES_PER_VPC: usize = 1024;
+
+/// Cap on targets and on each type of filter
+const MAX_FW_RULE_PARTS: usize = 256;
+
+fn ensure_max_len<T>(
+    items: &Vec<T>,
+    label: &str,
+    max: usize,
+) -> Result<(), external::Error> {
+    if items.len() > max {
+        let msg = format!("max length {}", max);
+        return Err(external::Error::invalid_value(label, msg));
+    }
+    Ok(())
+}
+
 impl VpcFirewallRule {
     pub fn new(
         rule_id: Uuid,
         vpc_id: Uuid,
         rule: &external::VpcFirewallRuleUpdate,
-    ) -> Self {
+    ) -> Result<Self, external::Error> {
         let identity = VpcFirewallRuleIdentity::new(
             rule_id,
             external::IdentityMetadataCreateParams {
                 name: rule.name.clone(),
                 description: rule.description.clone(),
             },
         );
-        Self {
+
+        ensure_max_len(&rule.targets, "targets", MAX_FW_RULE_PARTS)?;
+
+        if let Some(hosts) = rule.filters.hosts.as_ref() {
+            ensure_max_len(&hosts, "filters.hosts", MAX_FW_RULE_PARTS)?;
+        }
+        if let Some(ports) = rule.filters.ports.as_ref() {
+            ensure_max_len(&ports, "filters.ports", MAX_FW_RULE_PARTS)?;
+        }
+        if let Some(protocols) = rule.filters.protocols.as_ref() {
+            ensure_max_len(&protocols, "filters.protocols", MAX_FW_RULE_PARTS)?;
+        }
+
+        Ok(Self {
             identity,
             vpc_id,
             status: rule.status.into(),
@@ -248,19 +283,20 @@ impl VpcFirewallRule {
             }),
             action: rule.action.into(),
             priority: rule.priority.into(),
-        }
+        })
     }
 
     pub fn vec_from_params(
         vpc_id: Uuid,
         params: external::VpcFirewallRuleUpdateParams,
     ) -> Result<Vec<VpcFirewallRule>, external::Error> {
         ensure_no_duplicates(&params)?;
-        Ok(params
+        ensure_max_len(&params.rules, "rules", MAX_FW_RULES_PER_VPC)?;
+        params
             .rules
-            .iter()
-            .map(|rule| VpcFirewallRule::new(Uuid::new_v4(), vpc_id, rule))
-            .collect())
+            .into_iter()
+            .map(|rule| VpcFirewallRule::new(Uuid::new_v4(), vpc_id, &rule))
+            .collect()
     }
 }
 

nexus/db-queries/src/db/datastore/vpc.rs

@@ -221,20 +221,24 @@ impl DataStore {
             .map(|rule| (rule.name().clone(), rule))
             .collect::<BTreeMap<_, _>>();
 
-        fw_rules.entry(DNS_VPC_FW_RULE.name.clone()).or_insert_with(|| {
-            VpcFirewallRule::new(
+        // these have to be done this way because the contructor returns a result
+        if !fw_rules.contains_key(&DNS_VPC_FW_RULE.name) {
+            let rule = VpcFirewallRule::new(
                 Uuid::new_v4(),
                 *SERVICES_VPC_ID,
                 &DNS_VPC_FW_RULE,
-            )
-        });
-        fw_rules.entry(NEXUS_VPC_FW_RULE.name.clone()).or_insert_with(|| {
-            VpcFirewallRule::new(
+            )?;
+            fw_rules.insert(DNS_VPC_FW_RULE.name.clone(), rule);
+        }
+
+        if !fw_rules.contains_key(&NEXUS_VPC_FW_RULE.name) {
+            let rule = VpcFirewallRule::new(
                 Uuid::new_v4(),
                 *SERVICES_VPC_ID,
                 &NEXUS_VPC_FW_RULE,
-            )
-        });
+            )?;
+            fw_rules.insert(NEXUS_VPC_FW_RULE.name.clone(), rule);
+        }
 
         let rules = fw_rules
             .into_values()

nexus/src/external_api/http_entrypoints.rs

@@ -5378,7 +5378,6 @@ async fn vpc_subnet_list_network_interfaces(
 
 // VPC Firewalls
 
-// TODO Is the number of firewall rules bounded?
 /// List firewall rules
 #[endpoint {
     method = GET,
@@ -5410,7 +5409,23 @@ async fn vpc_firewall_rules_view(
         .await
 }
 
+// Note: the limits in the below comment come from the firewall rules model
+// file, nexus/db-model/src/vpc_firewall_rule.rs.
+
 /// Replace firewall rules
+///
+/// The maximum number of rules per VPC is 1024.
+///
+/// Targets are used to specify the set of instances to which a firewall rule
+/// applies. You can target instances directly by name, or specify a VPC, VPC
+/// subnet, IP, or IP subnet, which will apply the rule to traffic going to
+/// all matching instances. Targets are additive: the rule applies to instances
+/// matching ANY target. The maximum number of targets is 256.
+///
+/// Filters reduce the scope of a firewall rule. Without filters, the rule
+/// applies to all packets to the targets (or from the targets, if it's an
+/// outbound rule). With multiple filters, the rule applies only to packets
+/// matching ALL filters. The maximum number of each type of filter is 256.
 #[endpoint {
     method = PUT,
     path = "/v1/vpc-firewall-rules",