From 384e8e24a93472cd823b9567ed0c571903424580 Mon Sep 17 00:00:00 2001 From: Sebastian Spaink Date: Mon, 10 Aug 2026 15:38:51 -0500 Subject: [PATCH] fix(net): register provider and fix CIDR parity gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The BuiltinProvider entry for opa-builtins-net was commented out in its META-INF/services file, so ServiceLoader never discovered it and none of the seven net builtins were reachable for consumers. Registering it exposed 13 failing compliance cases: - net.cidr_merge gave a bare IPv4 address a /32. Go applies net.IP.DefaultMask(), the classful default, so ["192.0.128.0", "192.0.129.0"] merges to 192.0.128.0/23 rather than two /32s. - net.cidr_merge threw AddressConversionException on mixed IPv4 and IPv6 input, because mergeToPrefixBlocks cannot span versions. Each version is now merged separately and the results unioned. - net.cidr_is_valid accepted a bare address such as "192.168.1.2". Go's net.ParseCIDR requires the prefix. - net.lookup_ip_addr returned IPv6 in Java's uncompressed form (0:0:0:0:0:0:0:1) rather than the canonical ::1 a policy compares against. - net.cidr_expand and net.cidr_contains_matches reported messages that did not match OPA's wording. Also rejects the empty string, which no fixture covers. The parser accepts "" and resolves it to the loopback address, so net.cidr_contains("", "127.0.0.1") answered true — a policy granting access on an empty CIDR would have allowed the request. Go's ParseCIDR and ParseIP both reject it, so every parse site now sets allowEmpty(false). Adds CidrBuiltinsTest, including a ServiceLoader assertion; the module had no unit tests, which is why the registration gap went unnoticed. Nine of the eleven tests fail against the unfixed code. Removes the seven net entries from known-missing-builtins.txt (64 -> 57). Signed-off-by: Sebastian Spaink --- .../opa-builtins-net/build.gradle.kts | 7 + .../opa/ast/builtin/impls/CidrBuiltins.java | 113 ++++++++---- ...licy_agent.opa.ast.builtin.BuiltinProvider | 2 +- .../ast/builtin/impls/CidrBuiltinsTest.java | 168 ++++++++++++++++++ .../compliance/known-missing-builtins.txt | 7 - 5 files changed, 252 insertions(+), 45 deletions(-) create mode 100644 opa-builtins/opa-builtins-net/src/test/java/io/github/open_policy_agent/opa/ast/builtin/impls/CidrBuiltinsTest.java diff --git a/opa-builtins/opa-builtins-net/build.gradle.kts b/opa-builtins/opa-builtins-net/build.gradle.kts index a26b8f67..68c49f96 100644 --- a/opa-builtins/opa-builtins-net/build.gradle.kts +++ b/opa-builtins/opa-builtins-net/build.gradle.kts @@ -10,6 +10,9 @@ dependencies { api(project(":opa-evaluator")) implementation("com.github.seancfoley:ipaddress:5.6.2") + + testImplementation("org.junit.jupiter:junit-jupiter:6.1.2") + testRuntimeOnly("org.junit.platform:junit-platform-launcher:6.1.2") } java { @@ -17,3 +20,7 @@ java { languageVersion = JavaLanguageVersion.of(17) } } + +tasks.test { + useJUnitPlatform() +} diff --git a/opa-builtins/opa-builtins-net/src/main/java/io/github/open_policy_agent/opa/ast/builtin/impls/CidrBuiltins.java b/opa-builtins/opa-builtins-net/src/main/java/io/github/open_policy_agent/opa/ast/builtin/impls/CidrBuiltins.java index a24357b5..7ca4b91f 100644 --- a/opa-builtins/opa-builtins-net/src/main/java/io/github/open_policy_agent/opa/ast/builtin/impls/CidrBuiltins.java +++ b/opa-builtins/opa-builtins-net/src/main/java/io/github/open_policy_agent/opa/ast/builtin/impls/CidrBuiltins.java @@ -3,8 +3,8 @@ import inet.ipaddr.AddressStringException; import inet.ipaddr.IPAddress; import inet.ipaddr.IPAddressString; +import inet.ipaddr.IPAddressStringParameters; import inet.ipaddr.ipv4.IPv4Address; -import inet.ipaddr.ipv6.IPv6Address; import java.net.InetAddress; import java.net.UnknownHostException; import io.github.open_policy_agent.opa.ast.types.RegoArray; @@ -35,6 +35,18 @@ public class CidrBuiltins implements BuiltinProvider { + /** + * By default the parser accepts the empty string and resolves it to the loopback address, so + * net.cidr_contains("", "127.0.0.1") would answer true. Go's net.ParseCIDR/ParseIP reject it. + */ + private static final IPAddressStringParameters ADDRESS_PARAMS = + new IPAddressStringParameters.Builder().allowEmpty(false).toParams(); + + /** Build a parser for `str` that rejects the inputs Go rejects. */ + private static IPAddressString addressString(String str) { + return new IPAddressString(str, ADDRESS_PARAMS); + } + @Override public Map> builtins() { CidrBuiltins instance = new CidrBuiltins(); @@ -52,13 +64,13 @@ public Map> builti /** Parse an IP address or CIDR from a string. */ private IPAddress parseAddress(String str) throws AddressStringException { - return new IPAddressString(str).toAddress(); + return addressString(str).toAddress(); } /** Parse an IP address or CIDR, throwing a BuiltinError with the builtin name on failure. */ private IPAddress parseAddressOrThrow(String str, String builtinName) throws AddressStringException { - IPAddressString addrStr = new IPAddressString(str); + IPAddressString addrStr = addressString(str); if (!addrStr.isValid()) { throw new BuiltinError( builtinName + ": not a valid textual representation of an IP address or CIDR: " + str); @@ -224,21 +236,21 @@ private RegoString getCidrFromElement(RegoValue elem, int operandNum) { throw new BuiltinError( "net.cidr_contains_matches: operand " + operandNum - + " element must be string or non-empty array"); + + ": element must be string or non-empty array"); } RegoValue first = arr.getValue().get(0); if (!(first instanceof RegoString)) { throw new BuiltinError( "net.cidr_contains_matches: operand " + operandNum - + " element must be string or non-empty array"); + + ": element must be string or non-empty array"); } return (RegoString) first; } else { throw new BuiltinError( "net.cidr_contains_matches: operand " + operandNum - + " element must be string or non-empty array"); + + ": element must be string or non-empty array"); } } @@ -256,7 +268,12 @@ private RegoString getCidrFromElement(RegoValue elem, int operandNum) { public RegoValue expand(EvaluationContext ctx, RegoValue[] args) { try { String cidrStr = getArg(args, 0, RegoString.class).getValue(); - IPAddress addr = parseAddress(cidrStr); + IPAddressString addrStr = addressString(cidrStr); + if (!addrStr.isValid()) { + // Match Go's net.ParseCIDR wording rather than the parser's own diagnostic. + throw new BuiltinError("net.cidr_expand: invalid CIDR address: " + cidrStr); + } + IPAddress addr = addrStr.toAddress(); // Normalize to the network block (like Go's ip.Mask(ipNet.Mask)) IPAddress network = addr.toPrefixBlock(); @@ -288,13 +305,18 @@ public RegoValue expand(EvaluationContext ctx, RegoValue[] args) { public RegoValue isValid(EvaluationContext ctx, RegoValue[] args) { try { String cidrStr = getArg(args, 0, RegoString.class).getValue(); - IPAddressString addrStr = new IPAddressString(cidrStr); + IPAddressString addrStr = addressString(cidrStr); if (!addrStr.isValid()) { return RegoBoolean.FALSE; } - // Must be a valid CIDR (has a prefix or is a complete address) + // Go's net.ParseCIDR requires the prefix, so a bare address such as "192.168.1.2" is not + // valid CIDR notation even though it parses as an IP. + if (addrStr.getNetworkPrefixLength() == null) { + return RegoBoolean.FALSE; + } + addrStr.toAddress(); return RegoBoolean.TRUE; @@ -340,17 +362,23 @@ public RegoValue merge(EvaluationContext ctx, RegoValue[] args) { return new RegoSet(ctx.sortSets, new HashSet<>()); } - // Merge the addresses using the IPAddress library's merge functionality - List sortedAddresses = new ArrayList<>(addresses); - sortedAddresses.sort(Comparator.naturalOrder()); - - // Use merging from the first address's type - IPAddress[] merged = - sortedAddresses.get(0).mergeToPrefixBlocks(sortedAddresses.toArray(new IPAddress[0])); - + // mergeToPrefixBlocks cannot mix address versions, so merge each version separately and + // union the results. Go's implementation likewise merges v4 and v6 independently. Set resultSet = new HashSet<>(); - for (IPAddress addr : merged) { - resultSet.add(new RegoString(addr.toCanonicalString())); + for (boolean ipv6 : new boolean[] {false, true}) { + List versionAddresses = + addresses.stream().filter(a -> a.isIPv6() == ipv6).collect(Collectors.toList()); + if (versionAddresses.isEmpty()) { + continue; + } + versionAddresses.sort(Comparator.naturalOrder()); + IPAddress[] merged = + versionAddresses + .get(0) + .mergeToPrefixBlocks(versionAddresses.toArray(new IPAddress[0])); + for (IPAddress addr : merged) { + resultSet.add(new RegoString(addr.toCanonicalString())); + } } return new RegoSet(ctx.sortSets, resultSet); @@ -389,28 +417,36 @@ private List extractStringCollection(RegoValue input, String builtinName } private IPAddress parseIPOrCidr(String str) throws AddressStringException { - IPAddressString addrStr = new IPAddressString(str); - - // Try to parse as CIDR first - if (addrStr.isValid()) { - return addrStr.toAddress(); - } - - // Try as plain IP - addrStr = new IPAddressString(str); + IPAddressString addrStr = addressString(str); IPAddress addr = addrStr.toAddress(); - // If it's an IPv4 address without prefix, add default mask - if (addr instanceof IPv4Address && addr.getNetworkPrefixLength() == null) { - return addr.setPrefixLength(32, false); + if (addr.getNetworkPrefixLength() != null) { + return addr.toPrefixBlock(); } - // IPv6 addresses require a prefix length - if (addr instanceof IPv6Address && addr.getNetworkPrefixLength() == null) { - throw new AddressStringException(str, "IPv6 invalid: needs prefix length"); + // A bare address carries no prefix. Go applies net.IP.DefaultMask(), the classful default, + // so 192.0.2.112 widens to 192.0.2.0/24 rather than a /32. DefaultMask is undefined for + // IPv6, which is why a bare IPv6 address is an error instead. + if (addr.isIPv6()) { + // Thrown directly rather than as an AddressStringException, whose message would be + // decorated with the address and an "IP Address error" prefix. The evaluator prepends the + // builtin name, so it is not repeated here. + throw new BuiltinError("IPv6 invalid: needs prefix length"); } - return addr; + return addr.setPrefixLength(defaultClassfulPrefix(addr.toIPv4()), false).toPrefixBlock(); + } + + /** Go's net.IP.DefaultMask: /8 for class A, /16 for class B, /24 otherwise. */ + private int defaultClassfulPrefix(IPv4Address addr) { + int firstOctet = addr.getSegment(0).getSegmentValue(); + if (firstOctet < 0x80) { + return 8; + } + if (firstOctet < 0xC0) { + return 16; + } + return 24; } @OpaBuiltin( @@ -429,7 +465,7 @@ public RegoValue lookupIpAddr(EvaluationContext ctx, RegoValue[] args) { String hostname = getArg(args, 0, RegoString.class).getValue(); // Check if it's already an IP address - IPAddressString addrStr = new IPAddressString(hostname); + IPAddressString addrStr = addressString(hostname); if (addrStr.isIPAddress()) { // If it's an IP, just return it as-is Set resultSet = new HashSet<>(); @@ -442,7 +478,10 @@ public RegoValue lookupIpAddr(EvaluationContext ctx, RegoValue[] args) { Set resultSet = new HashSet<>(); for (InetAddress addr : addresses) { - resultSet.add(new RegoString(addr.getHostAddress())); + // InetAddress.getHostAddress renders IPv6 uncompressed (0:0:0:0:0:0:0:1); Go returns the + // canonical RFC 5952 form (::1), which is what policies compare against. + resultSet.add( + new RegoString(addressString(addr.getHostAddress()).getAddress().toCanonicalString())); } return new RegoSet(ctx.sortSets, resultSet); diff --git a/opa-builtins/opa-builtins-net/src/main/resources/META-INF/services/io.github.open_policy_agent.opa.ast.builtin.BuiltinProvider b/opa-builtins/opa-builtins-net/src/main/resources/META-INF/services/io.github.open_policy_agent.opa.ast.builtin.BuiltinProvider index 5f2e66e5..68abc25a 100644 --- a/opa-builtins/opa-builtins-net/src/main/resources/META-INF/services/io.github.open_policy_agent.opa.ast.builtin.BuiltinProvider +++ b/opa-builtins/opa-builtins-net/src/main/resources/META-INF/services/io.github.open_policy_agent.opa.ast.builtin.BuiltinProvider @@ -1 +1 @@ -#io.github.open_policy_agent.opa.ast.builtin.impls.CidrBuiltins +io.github.open_policy_agent.opa.ast.builtin.impls.CidrBuiltins diff --git a/opa-builtins/opa-builtins-net/src/test/java/io/github/open_policy_agent/opa/ast/builtin/impls/CidrBuiltinsTest.java b/opa-builtins/opa-builtins-net/src/test/java/io/github/open_policy_agent/opa/ast/builtin/impls/CidrBuiltinsTest.java new file mode 100644 index 00000000..65a1e7bb --- /dev/null +++ b/opa-builtins/opa-builtins-net/src/test/java/io/github/open_policy_agent/opa/ast/builtin/impls/CidrBuiltinsTest.java @@ -0,0 +1,168 @@ +package io.github.open_policy_agent.opa.ast.builtin.impls; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import io.github.open_policy_agent.opa.ast.builtin.BuiltinError; +import io.github.open_policy_agent.opa.ast.builtin.BuiltinProvider; +import io.github.open_policy_agent.opa.ast.types.RegoArray; +import io.github.open_policy_agent.opa.ast.types.RegoBoolean; +import io.github.open_policy_agent.opa.ast.types.RegoSet; +import io.github.open_policy_agent.opa.ast.types.RegoString; +import io.github.open_policy_agent.opa.ast.types.RegoValue; +import io.github.open_policy_agent.opa.rego.EvaluationContext; +import java.util.Arrays; +import java.util.List; +import java.util.ServiceLoader; +import java.util.stream.Collectors; +import org.junit.jupiter.api.Test; + +class CidrBuiltinsTest { + + private final CidrBuiltins builtins = new CidrBuiltins(); + private final EvaluationContext ctx = new EvaluationContext.Builder().build(); + + /** + * The provider entry in META-INF/services was commented out, so none of these builtins were + * reachable for consumers even though the module was documented as supported. + */ + @Test + void providerIsDiscoverableViaServiceLoader() { + boolean found = + ServiceLoader.load(BuiltinProvider.class, CidrBuiltins.class.getClassLoader()).stream() + .anyMatch(p -> p.type().equals(CidrBuiltins.class)); + + assertTrue(found, "CidrBuiltins should be registered as a BuiltinProvider"); + } + + @Test + void registersAllNetBuiltins() { + assertTrue( + builtins + .builtins() + .keySet() + .containsAll( + List.of( + "net.cidr_contains", + "net.cidr_contains_matches", + "net.cidr_expand", + "net.cidr_intersects", + "net.cidr_is_valid", + "net.cidr_merge", + "net.lookup_ip_addr"))); + } + + // Go's net.ParseCIDR requires a prefix, so a bare address is not valid CIDR notation. + @Test + void cidrIsValidRequiresAPrefix() { + assertEquals(RegoBoolean.TRUE, isValid("192.168.1.0/24")); + assertEquals(RegoBoolean.TRUE, isValid("2002::1234:abcd:ffff:c0a8:101/64")); + assertEquals(RegoBoolean.FALSE, isValid("192.168.1.2")); + assertEquals(RegoBoolean.FALSE, isValid("")); + assertEquals(RegoBoolean.FALSE, isValid("there goes a string")); + } + + // A bare IPv4 address takes Go's classful DefaultMask, not a /32. + @Test + void mergeAppliesClassfulDefaultMaskToBareIpv4() { + assertEquals(List.of("192.0.128.0/23"), merge("192.0.128.0", "192.0.129.0")); + assertEquals( + List.of("192.0.2.0/24"), merge("192.0.2.112", "192.0.2.116/31", "192.0.2.118/31")); + } + + @Test + void mergeCollapsesOverlappingIpv6Prefixes() { + assertEquals( + List.of("2601:600:8a80:207e::/64"), + merge( + "2601:600:8a80:207e:a57d:7567:e2c9:e7b3/64", + "2601:600:8a80:207e:a57d:7567:e2c9:e7b3/128")); + } + + // mergeToPrefixBlocks cannot mix versions, so the two families are merged independently. + @Test + void mergeHandlesMixedIpv4AndIpv6() { + assertEquals( + List.of("192.0.2.0/23", "192.0.4.0/24", "fe80::/120"), + merge("fe80::/120", "192.0.2.0/24", "192.0.3.0/24", "192.0.4.0/25", "192.0.4.128/25")); + } + + @Test + void mergeRejectsBareIpv6() { + BuiltinError e = + assertThrows( + BuiltinError.class, () -> merge("2601:600:8a80:207e:a57d:7567:e2c9:e7b3")); + + // The evaluator prepends the builtin name, so the message must not repeat it. + assertEquals("eval_builtin_error: IPv6 invalid: needs prefix length", e.getMessage()); + } + + @Test + void mergeRejectsMalformedInput() { + for (String bad : List.of("not-an-address", "999.1.1.1", "192.168.1.1/33")) { + BuiltinError e = assertThrows(BuiltinError.class, () -> merge(bad), bad); + assertTrue(e.getMessage().contains(bad), e.getMessage()); + } + } + + // The parser accepts "" and resolves it to the loopback address unless allowEmpty(false) is + // set, which made net.cidr_contains("", "127.0.0.1") answer true. Go's parsers reject it. + @Test + void emptyStringIsNotAnAddress() { + assertEquals(RegoBoolean.FALSE, isValid("")); + assertThrows(BuiltinError.class, () -> merge("")); + assertThrows( + BuiltinError.class, + () -> builtins.expand(ctx, new RegoValue[] {new RegoString("")})); + assertThrows( + BuiltinError.class, + () -> builtins.contains(ctx, new RegoValue[] {new RegoString(""), new RegoString("127.0.0.1")})); + assertThrows( + BuiltinError.class, + () -> + builtins.intersects( + ctx, new RegoValue[] {new RegoString(""), new RegoString("127.0.0.0/8")})); + } + + @Test + void expandReportsGoStyleMessageForInvalidMask() { + BuiltinError e = + assertThrows( + BuiltinError.class, + () -> builtins.expand(ctx, new RegoValue[] {new RegoString("192.168.1.1/33")})); + + assertTrue( + e.getMessage().contains("net.cidr_expand: invalid CIDR address: 192.168.1.1/33"), + e.getMessage()); + } + + @Test + void containsMatchesUsesColonSeparatedOperandMessage() { + RegoValue[] args = { + new RegoArray(List.of(new RegoString("1.1.1.0/24"))), + new RegoArray(List.of(RegoBoolean.TRUE)) + }; + + BuiltinError e = assertThrows(BuiltinError.class, () -> builtins.containsMatches(ctx, args)); + + assertTrue( + e.getMessage() + .contains("net.cidr_contains_matches: operand 2: element must be string or non-empty array"), + e.getMessage()); + } + + private RegoValue isValid(String cidr) { + return builtins.isValid(ctx, new RegoValue[] {new RegoString(cidr)}); + } + + private List merge(String... addrs) { + RegoArray input = + new RegoArray(Arrays.stream(addrs).map(RegoString::new).collect(Collectors.toList())); + RegoSet result = (RegoSet) builtins.merge(ctx, new RegoValue[] {input}); + return result.getValue().stream() + .map(v -> ((RegoString) v).getValue()) + .sorted() + .collect(Collectors.toList()); + } +} diff --git a/opa-evaluator/src/test/resources/compliance/known-missing-builtins.txt b/opa-evaluator/src/test/resources/compliance/known-missing-builtins.txt index 9acde2d1..449d99fd 100644 --- a/opa-evaluator/src/test/resources/compliance/known-missing-builtins.txt +++ b/opa-evaluator/src/test/resources/compliance/known-missing-builtins.txt @@ -35,13 +35,6 @@ yaml.marshal yaml.unmarshal # opa-builtins-net -net.cidr_contains -net.cidr_contains_matches -net.cidr_expand -net.cidr_intersects -net.cidr_is_valid -net.cidr_merge -net.lookup_ip_addr # opa-builtins-regex regex.find_all_string_submatch_n