Skip to content

feat(acl-filter,config): Add restriction for user ACLs "scope: flow" #1655

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
qmonnet merged 1 commit into from
Jul 22, 2026
Merged

feat(acl-filter,config): Add restriction for user ACLs "scope: flow" #1655

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions acl-filter/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,18 @@ fn build_filter_v4(acl: Option<Acl>) -> AclFilter {
build_filter(V1_IPS, V2_IPS, acl)
}

fn build_filter_v4_masquerade(acl: Option<Acl>) -> AclFilter {
acl_filter(&overlay(
&[("vpc1", VNI1), ("vpc2", VNI2)],
vec![peering(
"vpc1-to-vpc2",
("vpc1", vec![expose(V1_IPS)]),
("vpc2", vec![expose_masquerade(V2_IPS, V2_IPS)]),
acl,
)],
))
}

// -------------------------------------------------------------------------------------------------
// ACL rule builders

Expand Down Expand Up @@ -682,7 +694,7 @@ fn flow_scope_allows_reply_for_allowed_request() {
pattern(&[V1_IPS], &[V2_IPS], AclProtoMatch::Tcp),
)],
);
let mut filter = build_filter_v4(Some(acl));
let mut filter = build_filter_v4_masquerade(Some(acl));

// The request is allowed by the rule directly
let request = packet(
Expand Down Expand Up @@ -763,7 +775,7 @@ fn explicit_deny_rule_drops_reply_despite_matching_flow() {
),
],
);
let mut filter = build_filter_v4(Some(acl));
let mut filter = build_filter_v4_masquerade(Some(acl));

// The request is allowed by the request-direction rule
let request = packet(
Expand Down
29 changes: 26 additions & 3 deletions config/src/converters/k8s/config/acl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,14 +333,15 @@ mod test {
to: &str,
action: GatewayAgentPeeringsAclRulesAction,
r#match: Option<GatewayAgentPeeringsAclRulesMatch>,
scope: Option<GatewayAgentPeeringsAclRulesScope>,
) -> GatewayAgentPeeringsAclRules {
GatewayAgentPeeringsAclRules {
action,
from: (!from.is_empty()).then(|| from.to_string()),
log: None,
r#match,
name: (!name.is_empty()).then(|| name.to_string()),
scope: None,
scope,
to: (!to.is_empty()).then(|| to.to_string()),
}
}
Expand Down Expand Up @@ -433,6 +434,7 @@ mod test {
"VPC-2",
GatewayAgentPeeringsAclRulesAction::Allow,
None,
None,
),
)
.unwrap();
Expand All @@ -450,6 +452,7 @@ mod test {
"",
GatewayAgentPeeringsAclRulesAction::Allow,
None,
None,
),
)
.is_ok()
Expand All @@ -461,7 +464,14 @@ mod test {
&vpc_subnets,
"VPC-1",
"VPC-2",
&rule("r", "", "", GatewayAgentPeeringsAclRulesAction::Allow, None),
&rule(
"r",
"",
"",
GatewayAgentPeeringsAclRulesAction::Allow,
None,
None,
),
)
.is_err()
);
Expand All @@ -478,6 +488,7 @@ mod test {
"VPC-2",
GatewayAgentPeeringsAclRulesAction::Allow,
None,
None,
),
)
.is_err()
Expand Down Expand Up @@ -509,6 +520,7 @@ mod test {
"VPC-2",
GatewayAgentPeeringsAclRulesAction::Allow,
Some(m.clone()),
None,
),
)
.unwrap();
Expand All @@ -526,6 +538,7 @@ mod test {
"VPC-1",
GatewayAgentPeeringsAclRulesAction::Allow,
Some(m),
None,
),
)
.is_err()
Expand Down Expand Up @@ -557,6 +570,7 @@ mod test {
"VPC-2",
GatewayAgentPeeringsAclRulesAction::Allow,
Some(m),
None,
),
);
assert!(
Expand Down Expand Up @@ -589,6 +603,7 @@ mod test {
"VPC-2",
GatewayAgentPeeringsAclRulesAction::Allow,
Some(m),
None,
),
)
.unwrap();
Expand Down Expand Up @@ -622,6 +637,7 @@ mod test {
"VPC-2",
GatewayAgentPeeringsAclRulesAction::Allow,
Some(ports_only),
None,
),
)
.unwrap();
Expand All @@ -646,6 +662,7 @@ mod test {
"VPC-2",
GatewayAgentPeeringsAclRulesAction::Allow,
Some(empty_ports),
None,
),
);
assert!(
Expand Down Expand Up @@ -680,6 +697,10 @@ mod test {
"VPC-2",
GatewayAgentPeeringsAclRulesAction::Allow,
Some(m),
// "scope: flow" (default scope) not currently supported with the peering
// configuration, because there's no masquerade/port forwarding.
// See https://github.com/githedgehog/dataplane/issues/1625
Some(GatewayAgentPeeringsAclRulesScope::Packet),
),
)
.unwrap();
Expand Down Expand Up @@ -733,13 +754,15 @@ mod test {
proto: Some("tcp".to_string()),
src: None,
}),
None,
),
rule(
"return",
"VPC-2",
"VPC-1",
GatewayAgentPeeringsAclRulesAction::Allow,
None,
None,
),
]),
};
Expand All @@ -750,8 +773,8 @@ mod test {
"VPC-2",
GatewayAgentPeeringsAclRulesAction::Deny,
None,
Some(GatewayAgentPeeringsAclRulesScope::Packet),
);
rule_with_scope.scope = Some(GatewayAgentPeeringsAclRulesScope::Packet);
rule_with_scope.log = Some(true);
let mut acl_with_scope = acl.clone();
acl_with_scope.rules.as_mut().unwrap().push(rule_with_scope);
Expand Down
117 changes: 112 additions & 5 deletions config/src/external/overlay/acl.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
//! Access Control Lists (ACLs)

use super::vpcpeering::ValidatedManifest;
use crate::ConfigError;
use crate::utils::normalize;
use crate::{ConfigError, ConfigResult};
use lpm::prefix::{PortRange, Prefix, PrefixPortsSet, PrefixWithOptionalPorts};
use match_action::MaskSpec;
use net::ip::NextHeader;
Expand Down Expand Up @@ -182,7 +182,7 @@ impl AclRule {
&self,
manifest_left: &ValidatedManifest,
manifest_right: &ValidatedManifest,
) -> Result<(), ConfigError> {
) -> ConfigResult {
match (self.from.as_str(), self.to.as_str()) {
(from, to) if from == manifest_left.name() && to == manifest_right.name() => Ok(()),
(from, to) if from == manifest_right.name() && to == manifest_left.name() => Ok(()),
Expand Down Expand Up @@ -221,6 +221,7 @@ impl AclRule {
&src_any_ports,
&dst_any_ports,
)?;
validated_rule.validate_scope(manifest_left, manifest_right)?;
Ok(validated_rule)
}
}
Expand Down Expand Up @@ -333,7 +334,7 @@ impl ValidatedAclRule {
name: &str,
prefixes_acl: &mut PrefixPortsSet,
prefixes_manifest: &PrefixPortsSet,
) -> Result<(), ConfigError> {
) -> ConfigResult {
// If the list of prefixes is empty, it means we match all prefixes from the manifest.
if prefixes_acl.is_empty() {
*prefixes_acl = prefixes_manifest.clone();
Expand Down Expand Up @@ -365,7 +366,7 @@ impl ValidatedAclRule {
manifest_right: &ValidatedManifest,
src_any_ports: &[PortRange],
dst_any_ports: &[PortRange],
) -> Result<(), ConfigError> {
) -> ConfigResult {
// A default-only manifest's is_v4()/is_v6() are always false, so fall back to the other side
let is_v4 = if manifest_left.is_default_only() {
manifest_right.is_v4()
Expand All @@ -387,6 +388,41 @@ impl ValidatedAclRule {

Ok(())
}

// TODO: Remove once we support flow tracking for non-NAT and static NAT flows
// See https://github.com/githedgehog/dataplane/issues/1625
fn validate_scope(
&self,
manifest_left: &ValidatedManifest,
manifest_right: &ValidatedManifest,
) -> ConfigResult {
if self.scope() != AclScope::Flow {
return Ok(());
}
// If one side uses masquerade or port forwarding for all exposes, then we're good
if manifest_left.valexp().iter().all(|expose| {
expose
.nat()
.is_some_and(|nat| nat.is_masquerade() || nat.is_port_forwarding())
}) {
return Ok(());
}
// If the other side uses masquerade or port forwarding for all exposes, then we're good
if manifest_right.valexp().iter().all(|expose| {
expose
.nat()
.is_some_and(|nat| nat.is_masquerade() || nat.is_port_forwarding())
}) {
return Ok(());
}
// If both sides have at least one prefix without masquerade or port forwarding, the peering
// will expose flows that won't get entries in the flow table, and ACLs with "scope: flow"
// are not supported
Err(ConfigError::InvalidAcl(format!(
"ACL rule '{}': At the moment, 'scope: flow' is only supported when all connections in the peering use masquerade or port forwarding",
self.name,
)))
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
}

#[derive(Debug, Clone, PartialEq, Eq, Default)]
Expand All @@ -411,7 +447,7 @@ impl Acl {
&self.rules
}

fn validate_rules_names(&self) -> Result<(), ConfigError> {
fn validate_rules_names(&self) -> ConfigResult {
let mut seen_names = std::collections::HashSet::new();
for rule in &self.rules {
if rule.name.is_empty() {
Expand Down Expand Up @@ -500,6 +536,20 @@ mod validation_tests {
.unwrap()
}

// Helper: build a validated manifest, exposing the given prefixes with masquerade
fn manifest_masquerade(name: &str, ips: &[&str], as_ranges: &[&str]) -> ValidatedManifest {
let mut expose = VpcExpose::empty().make_masquerade(None).unwrap();
for ip in ips {
expose = expose.ip((*ip).into());
}
for as_range in as_ranges {
expose = expose.as_range((*as_range).into()).unwrap();
}
VpcManifest::with_exposes(name, vec![expose])
.validate()
.unwrap()
}

// Helper: build a validated manifest with only a default expose (no concrete prefixes)
fn default_manifest(name: &str) -> ValidatedManifest {
VpcManifest::with_exposes(name, vec![VpcExpose::empty().set_default()])
Expand Down Expand Up @@ -545,6 +595,25 @@ mod validation_tests {

// Helper: assemble an AclRule
fn rule(name: &str, from: &str, to: &str, action: AclAction, pattern: AclPattern) -> AclRule {
AclRule {
name: name.to_owned(),
from: from.to_owned(),
to: to.to_owned(),
action,
pattern,
scope: AclScope::Packet,
log: false,
}
}

// Helper: assemble an AclRule
fn rule_scope_flow(
name: &str,
from: &str,
to: &str,
action: AclAction,
pattern: AclPattern,
) -> AclRule {
AclRule {
name: name.to_owned(),
from: from.to_owned(),
Expand Down Expand Up @@ -999,6 +1068,44 @@ mod validation_tests {
);
}

// =============================================================================================
// User ACLs limitation: restriction on "scope: flow"
// =============================================================================================

#[test]
fn test_flow_scope_rejected_when_no_flow_tracking() {
let left = manifest("VPC-1", &["10.0.0.0/16"]);
let right = manifest("VPC-2", &["10.1.0.0/16"]);

let p = pattern(
PrefixPortsSet::new(),
PrefixPortsSet::new(),
AclProtoMatch::Any,
);
let rule = rule_scope_flow("r", "VPC-1", "VPC-2", AclAction::Allow, p);

let result = rule.validate(&left, &right);
assert!(
matches!(result, Err(ConfigError::InvalidAcl(_))),
"{result:?}"
);
}

#[test]
fn test_flow_scope_valid_with_flow_tracking() {
let left = manifest("VPC-1", &["10.0.0.0/16"]);
let right = manifest_masquerade("VPC-2", &["1.0.0.0/16"], &["10.1.0.0/16"]);

let p = pattern(
PrefixPortsSet::new(),
PrefixPortsSet::new(),
AclProtoMatch::Any,
);
let rule = rule_scope_flow("r", "VPC-1", "VPC-2", AclAction::Allow, p);

rule.validate(&left, &right).expect("should validate");
}

// =============================================================================================
// Default ACL values
// =============================================================================================
Expand Down
Loading