From c2bc1ff928fab57f75c170c7e182f5a166255f9e Mon Sep 17 00:00:00 2001 From: Dotan Nahum Date: Sun, 20 Nov 2022 09:59:45 +0200 Subject: [PATCH 1/9] net: finalize implementation --- Cargo.toml | 4 + src/builtins/impls/net.rs | 210 ++++++++++++++++++++++- src/builtins/mod.rs | 5 + tests/infra-fixtures/test-net.rego | 53 ++++++ tests/smoke_test.rs | 1 + tests/snapshots/smoke_test__net.snap | 60 +++++++ tests/snapshots/smoke_test__net.snap.new | 67 ++++++++ 7 files changed, 394 insertions(+), 6 deletions(-) create mode 100644 tests/infra-fixtures/test-net.rego create mode 100644 tests/snapshots/smoke_test__net.snap create mode 100644 tests/snapshots/smoke_test__net.snap.new diff --git a/Cargo.toml b/Cargo.toml index 02801f1..d1e602f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -54,6 +54,8 @@ route-pattern = { version = "0.1.0", optional = true } regex-intersect = { version = "1.2.0", optional = true } form_urlencoded = { version = "1.1.0", optional = true } urlencoding = { version = "2.1.2", optional = true } +trust-dns-resolver = { version = "0.22.0", optional = true } +ipnet = { version = "2.5.1", optional = true } [dev-dependencies.tokio] version = "1.5" @@ -111,6 +113,7 @@ yaml-builtins = ["dep:serde_yaml"] glob-builtins = ["dep:globset"] regex-builtins = ["dep:regex", "dep:route-pattern", "dep:regex-intersect"] urlquery-builtins = ["dep:form_urlencoded", "dep:urlencoding"] +net-builtins = ["dep:ipnet", "dep:trust-dns-resolver"] all-crypto-builtins = [ "crypto-digest-builtins", @@ -133,6 +136,7 @@ all-builtins = [ "glob-builtins", "regex-builtins", "urlquery-builtins", + "net-builtins", ] [[test]] diff --git a/src/builtins/impls/net.rs b/src/builtins/impls/net.rs index f467bdf..57ce590 100644 --- a/src/builtins/impls/net.rs +++ b/src/builtins/impls/net.rs @@ -14,9 +14,129 @@ //! Builtins related to network operations and IP handling -use std::collections::HashSet; +use std::{collections::HashSet, net::IpAddr, str::FromStr}; -use anyhow::{bail, Result}; +use anyhow::{bail, Context, Result}; +use ipnet::IpNet; +use serde_json::Number; +use trust_dns_resolver::{AsyncResolver, Resolver, TokioAsyncResolver}; + +#[allow(clippy::upper_case_acronyms)] +#[derive(Debug, Clone, Copy)] +enum Addr { + CIDR(IpNet), + IP(IpAddr), +} + +/// builds a generic IP or CIDR container +fn addr_from_cidr_or_ip(s: &str) -> Result { + IpNet::from_str(s).map(Addr::CIDR).or_else(|_| { + IpAddr::from_str(s) + .map(Addr::IP) + .context(format!("cannot parse {} into cidr or ip", s)) + }) +} + +/// this expand upon the go side handling additional cases where: +/// ip can contain ip (by way of strict equality) +/// cidr contains ip, but also ip is contained within cidr (flip the arguments) +/// * in the Go case, it's assumed that root call of `cidr_contains_matches` LHS +/// is CIDR and RHS is "CIDR or IP". here by being direction agnosic we make +/// no such assumption and handle a larger set of cases +#[allow(clippy::similar_names)] +fn addr_contains(left: &Addr, right: &Addr) -> bool { + match (left, right) { + (Addr::CIDR(cidr), Addr::IP(ip)) | (Addr::IP(ip), Addr::CIDR(cidr)) => cidr.contains(ip), + (Addr::CIDR(lcidr), Addr::CIDR(rcidr)) => lcidr.contains(rcidr), + (Addr::IP(lip), Addr::IP(rip)) => lip == rip, + } +} + +/// this is modeled after the Go side, where we want to get a term generically +/// from various IP/CIDR nestings: +/// "x":"y" -> "y" +/// "x":["y", "rest"] -> "y" +/// etc. +/// in this case, we work on the lowest level term: atom or array, and in the +/// case of array we always take the first atom. +fn get_addr_term(t: &serde_json::Value) -> Result { + if let Some(s) = t.as_str() { + addr_from_cidr_or_ip(s) + } else if let Some(v) = t.as_array() { + let s = v + .first() + .context("value '' does not contain an address or cidr in first position")? + .as_str() + .context("value is not an address")?; + addr_from_cidr_or_ip(s) + } else { + bail!("value '{:?}' does not contain an address or cidr", t) + } +} + +/// match one of the LHS items against a collection from the RHS +fn match_collection( + item: &Addr, + item_key: &serde_json::Value, + iter: I, +) -> Result> +where + I: Iterator>, +{ + Ok(iter + .collect::>>()? + .into_iter() + .filter(|(_, candidate)| addr_contains(item, candidate)) + .map(|(candidate_key, _)| (item_key.clone(), candidate_key)) + .collect::>()) +} + +/// match one of the LHS items against an atom, collection, or hash +fn match_any( + item: &Addr, + item_key: &serde_json::Value, + cidrs_or_ips: &serde_json::Value, +) -> Result> { + if let Some(rs) = cidrs_or_ips.as_str() { + let ip_or_cidr = addr_from_cidr_or_ip(rs)?; + if addr_contains(item, &ip_or_cidr) { + Ok(vec![(item_key.clone(), cidrs_or_ips.clone())]) + } else { + Ok(vec![]) + } + } else if let Some(rv) = cidrs_or_ips.as_array() { + match_collection( + item, + item_key, + rv.iter().enumerate().map(|(idx, el)| { + get_addr_term(el).map(|t| (serde_json::Value::Number(Number::from(idx)), t)) + }), + ) + } else if let Some(robj) = cidrs_or_ips.as_object() { + match_collection( + item, + item_key, + robj.iter().map(|(k, el)| { + get_addr_term(el).map(|t| (serde_json::Value::String(k.to_string()), t)) + }), + ) + } else { + bail!("cannot match against this data type") + } +} + +/// flatten `cidr_contains_matches` top level matching results +fn flatten_matches(iter: I) -> Result> +where + I: Iterator>>, +{ + Ok(iter + .collect::>>()? + .into_iter() + .flatten() + .map(|(v1, v2)| serde_json::Value::Array(vec![v1, v2])) + .collect::>()) +} /// Checks if collections of cidrs or ips are contained within another /// collection of cidrs and returns matches. This function is similar to @@ -28,7 +148,51 @@ pub fn cidr_contains_matches( cidrs: serde_json::Value, cidrs_or_ips: serde_json::Value, ) -> Result { - bail!("not implemented"); + println!("lhs: {:?}, rhs: {:?}", cidrs, cidrs_or_ips); + + let res = if let Some(s) = cidrs.as_str() { + match_any( + &addr_from_cidr_or_ip(s)?, + &serde_json::Value::String(s.to_string()), + &cidrs_or_ips, + )? + .into_iter() + .map(|(v1, v2)| serde_json::Value::Array(vec![v1, v2])) + .collect::>() + } else if let Some(v) = cidrs.as_array() { + flatten_matches( + v.iter() + .map(get_addr_term) + .collect::>>()? + .into_iter() + .enumerate() + .map(|(idx, item)| { + match_any( + &item, + &serde_json::Value::Number(Number::from(idx)), + &cidrs_or_ips, + ) + }), + )? + } else if let Some(obj) = cidrs.as_object() { + flatten_matches( + obj.iter() + .map(|(k, el)| get_addr_term(el).map(|item| (k, item))) + .collect::>>()? + .into_iter() + .map(|(k, item)| { + match_any( + &item, + &serde_json::Value::String(k.to_string()), + &cidrs_or_ips, + ) + }), + )? + } else { + bail!("cannot match against these arguments") + }; + + Ok(serde_json::Value::Array(res)) } /// Expands CIDR to set of hosts (e.g., `net.cidr_expand("192.168.0.0/30")` @@ -36,7 +200,17 @@ pub fn cidr_contains_matches( /// "192.168.0.3"}`). #[tracing::instrument(name = "net.cidr_expand", err)] pub fn cidr_expand(cidr: String) -> Result> { - bail!("not implemented"); + // IpNet.hosts() is too smart because it excludes the + // broadcast and network IP. which is why we're doing it manually here to + // include all addresses in range including broadcast and network: + Ok(match IpNet::from_str(&cidr)? { + IpNet::V4(net) => ipnet::Ipv4AddrRange::new(net.network(), net.broadcast()) + .map(|addr| addr.to_string()) + .collect::>(), + IpNet::V6(net) => ipnet::Ipv6AddrRange::new(net.network(), net.broadcast()) + .map(|addr| addr.to_string()) + .collect::>(), + }) } /// Merges IP addresses and subnets into the smallest possible list of CIDRs @@ -48,12 +222,36 @@ pub fn cidr_expand(cidr: String) -> Result> { /// (e.g. "/128"). #[tracing::instrument(name = "net.cidr_merge", err)] pub fn cidr_merge(addrs: serde_json::Value) -> Result> { - bail!("not implemented"); + if let Some(addrs) = addrs.as_array() { + let ipnets = addrs + .iter() + .map(|addr| { + addr.as_str() + .and_then(|s| IpNet::from_str(s).ok()) + .context("is not an address string") + }) + .collect::>>()?; + Ok(IpNet::aggregate(&ipnets) + .iter() + .map(ToString::to_string) + .collect::>()) + } else { + bail!("data type not supported: {}", addrs); + } } /// Returns the set of IP addresses (both v4 and v6) that the passed-in `name` /// resolves to using the standard name resolution mechanisms available. #[tracing::instrument(name = "net.lookup_ip_addr", err)] pub async fn lookup_ip_addr(name: String) -> Result> { - bail!("not implemented"); + let resolver = TokioAsyncResolver::tokio( + trust_dns_resolver::config::ResolverConfig::default(), + trust_dns_resolver::config::ResolverOpts::default(), + )?; + + let response = resolver.lookup_ip(&name).await?; + Ok(response + .iter() + .map(|addr| addr.to_string()) + .collect::>()) } diff --git a/src/builtins/mod.rs b/src/builtins/mod.rs index 79348c8..27c989d 100644 --- a/src/builtins/mod.rs +++ b/src/builtins/mod.rs @@ -105,10 +105,15 @@ pub fn resolve(name: &str) -> Result>> #[cfg(feature = "json-builtins")] "json.patch" => Ok(self::impls::json::patch.wrap()), + #[cfg(feature = "net-builtins")] "net.cidr_contains_matches" => Ok(self::impls::net::cidr_contains_matches.wrap()), + #[cfg(feature = "net-builtins")] "net.cidr_expand" => Ok(self::impls::net::cidr_expand.wrap()), + #[cfg(feature = "net-builtins")] "net.cidr_merge" => Ok(self::impls::net::cidr_merge.wrap()), + #[cfg(feature = "net-builtins")] "net.lookup_ip_addr" => Ok(self::impls::net::lookup_ip_addr.wrap()), + "object.union_n" => Ok(self::impls::object::union_n.wrap()), "opa.runtime" => Ok(self::impls::opa::runtime.wrap()), diff --git a/tests/infra-fixtures/test-net.rego b/tests/infra-fixtures/test-net.rego new file mode 100644 index 0000000..959d93c --- /dev/null +++ b/tests/infra-fixtures/test-net.rego @@ -0,0 +1,53 @@ +package test + +output_1 := net.cidr_contains_matches({"x": "1.1.0.0/16"}, [["1.1.1.128", "foo"]]) + +output_2 := net.cidr_contains_matches("1.1.0.0/16", "1.1.1.128") + +output_3 := net.cidr_contains_matches(["1.1.0.0/16"], ["1.1.1.128"]) + +output_4 := net.cidr_contains_matches([["1.1.0.0/16"]], [["1.1.1.128"]]) + +output_5 := net.cidr_contains_matches([["1.1.0.0/16", "x"]], [["1.1.1.128", "y"], ["2.2.2.2", "z"]]) + +output_6 := net.cidr_contains_matches([["1.1.0.0/16", "x"]], {"y": ["1.1.1.128", "foo"], "z": ["2.2.2.2", "bar"]}) + +output_7 := net.cidr_contains_matches([["1.1.0.0/16", "x"]], {"y": "1.1.1.128", "z": "2.2.2.2"}) + +# from docs + +docs_test_1 := net.cidr_contains_matches("1.1.1.0/24", "1.1.1.128") + +docs_test_2 := net.cidr_contains_matches(["1.1.1.0/24", "1.1.2.0/24"], "1.1.1.128") + +docs_test_3 := net.cidr_contains_matches([["1.1.0.0/16", "foo"], "1.1.2.0/24"], ["1.1.1.128", ["1.1.254.254", "bar"]]) + +# https://github.com/open-policy-agent/opa/issues/3252 +# notice sets are reversing the order on WASM, stated by design +# and a WASM vs Rego thing we'll "have to live with" +# the engine will flip +# ["1.1.0.0/16", "foo"], "1.1.2.0/24" +# to be: +# "1.1.2.0/24", ["1.1.0.0/16", "foo"], +# and so, index in the result is '1' and not '0' like in the docs +docs_test_4 := net.cidr_contains_matches({"1.1.2.0/24", ["1.1.0.0/16", "foo"]}, {"x": "1.1.1.128", "y": ["1.1.254.254", "bar"]}) + +# switching to arrays works as intended, which is the recommended workaround +docs_test_4_1 := net.cidr_contains_matches([["1.1.0.0/16", "foo"], "1.1.2.0/24"], {"x": "1.1.1.128", "y": ["1.1.254.254", "bar"]}) + +cidr_expand_1 := net.cidr_expand("1.1.0.0/30") + +cidr_expand_2 := net.cidr_expand("1.1.0.0/32") + +cidr_expand_3 := net.cidr_expand("2002::1234:abcd:ffff:c0a8:101/128") + +cidr_expand_4 := net.cidr_expand("2002::1234:abcd:ffff:c0a8:101/127") + +cidr_merge_1 := net.cidr_merge(["192.0.128.0/24", "192.0.129.0/24"]) + +cidr_merge_2 := net.cidr_merge(["1900::1/96", "1900::20/64"]) + +# will example.com forever be: 93.184.216.34 ? +# taking the risk of years in the future this test fails because they +# changed IPs. So this will go out to network to do the lookup: +net_lookup := net.lookup_ip_addr("www.example.com") diff --git a/tests/smoke_test.rs b/tests/smoke_test.rs index ce9f9d0..8addb2d 100644 --- a/tests/smoke_test.rs +++ b/tests/smoke_test.rs @@ -122,6 +122,7 @@ integration_test!(test_yaml, "test-yaml"); integration_test!(test_glob, "test-glob"); integration_test!(test_regex, "test-regex"); integration_test!(test_urlquery, "test-urlquery"); +integration_test!(test_net, "test-net"); /* #[tokio::test] diff --git a/tests/snapshots/smoke_test__net.snap b/tests/snapshots/smoke_test__net.snap new file mode 100644 index 0000000..3651887 --- /dev/null +++ b/tests/snapshots/smoke_test__net.snap @@ -0,0 +1,60 @@ +--- +source: tests/smoke_test.rs +expression: "test_policy(\"test-net\", None).await.expect(\"error in test suite\")" +--- +- result: + cidr_expand_1: + - 1.1.0.1 + - 1.1.0.0 + - 1.1.0.2 + - 1.1.0.3 + cidr_expand_2: + - 1.1.0.0 + cidr_expand_3: + - "2002::1234:abcd:ffff:c0a8:101" + cidr_expand_4: + - "2002::1234:abcd:ffff:c0a8:100" + - "2002::1234:abcd:ffff:c0a8:101" + docs_test_1: + - - 1.1.1.0/24 + - 1.1.1.128 + docs_test_2: + - - 0 + - 1.1.1.128 + docs_test_3: + - - 0 + - 0 + - - 0 + - 1 + docs_test_4: + - - 1 + - x + - - 1 + - y + docs_test_4_1: + - - 0 + - x + - - 0 + - y + output_1: + - - x + - 0 + output_2: + - - 1.1.0.0/16 + - 1.1.1.128 + output_3: + - - 0 + - 0 + output_4: + - - 0 + - 0 + output_5: + - - 0 + - 0 + output_6: + - - 0 + - y + output_7: + - - 0 + - y + diff --git a/tests/snapshots/smoke_test__net.snap.new b/tests/snapshots/smoke_test__net.snap.new new file mode 100644 index 0000000..1d0a5da --- /dev/null +++ b/tests/snapshots/smoke_test__net.snap.new @@ -0,0 +1,67 @@ +--- +source: tests/smoke_test.rs +assertion_line: 125 +expression: "test_policy(\"test-net\", None).await.expect(\"error in test suite\")" +--- +- result: + cidr_expand_1: + - 1.1.0.1 + - 1.1.0.2 + - 1.1.0.3 + - 1.1.0.0 + cidr_expand_2: + - 1.1.0.0 + cidr_expand_3: + - "2002::1234:abcd:ffff:c0a8:101" + cidr_expand_4: + - "2002::1234:abcd:ffff:c0a8:101" + - "2002::1234:abcd:ffff:c0a8:100" + cidr_merge_1: + - 192.0.128.0/23 + cidr_merge_2: + - "1900::/64" + docs_test_1: + - - 1.1.1.0/24 + - 1.1.1.128 + docs_test_2: + - - 0 + - 1.1.1.128 + docs_test_3: + - - 0 + - 0 + - - 0 + - 1 + docs_test_4: + - - 1 + - x + - - 1 + - y + docs_test_4_1: + - - 0 + - x + - - 0 + - y + net_lookup: + - 93.184.216.34 + output_1: + - - x + - 0 + output_2: + - - 1.1.0.0/16 + - 1.1.1.128 + output_3: + - - 0 + - 0 + output_4: + - - 0 + - 0 + output_5: + - - 0 + - 0 + output_6: + - - 0 + - y + output_7: + - - 0 + - y + From 6875d639a39bf0321b3254e503332f1bea697369 Mon Sep 17 00:00:00 2001 From: Dotan Nahum Date: Sun, 20 Nov 2022 10:04:08 +0200 Subject: [PATCH 2/9] force ci --- src/builtins/impls/net.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/builtins/impls/net.rs b/src/builtins/impls/net.rs index 57ce590..f92cb01 100644 --- a/src/builtins/impls/net.rs +++ b/src/builtins/impls/net.rs @@ -250,6 +250,7 @@ pub async fn lookup_ip_addr(name: String) -> Result> { )?; let response = resolver.lookup_ip(&name).await?; + Ok(response .iter() .map(|addr| addr.to_string()) From a025b60c6c14885c645643b9f1384d8a61a8e6a7 Mon Sep 17 00:00:00 2001 From: Dotan Nahum Date: Sun, 20 Nov 2022 10:12:42 +0200 Subject: [PATCH 3/9] clippy --- src/builtins/impls/net.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/builtins/impls/net.rs b/src/builtins/impls/net.rs index f92cb01..ecb14f7 100644 --- a/src/builtins/impls/net.rs +++ b/src/builtins/impls/net.rs @@ -19,7 +19,7 @@ use std::{collections::HashSet, net::IpAddr, str::FromStr}; use anyhow::{bail, Context, Result}; use ipnet::IpNet; use serde_json::Number; -use trust_dns_resolver::{AsyncResolver, Resolver, TokioAsyncResolver}; +use trust_dns_resolver::TokioAsyncResolver; #[allow(clippy::upper_case_acronyms)] #[derive(Debug, Clone, Copy)] From 760d56e9aca6582546d58b3083af2e5a1ecdc6cb Mon Sep 17 00:00:00 2001 From: Dotan Nahum Date: Sun, 20 Nov 2022 10:17:49 +0200 Subject: [PATCH 4/9] clippy --- src/builtins/impls/mod.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/builtins/impls/mod.rs b/src/builtins/impls/mod.rs index 0ee97b8..8c9e95a 100644 --- a/src/builtins/impls/mod.rs +++ b/src/builtins/impls/mod.rs @@ -33,7 +33,9 @@ pub mod http; pub mod io; #[cfg(feature = "json-builtins")] pub mod json; +#[cfg(feature = "net-builtins")] pub mod net; + pub mod object; pub mod opa; #[cfg(feature = "rng")] From e8ab004c24134d17c8049fc72dfab9d1af5ec792 Mon Sep 17 00:00:00 2001 From: Dotan Nahum Date: Sun, 20 Nov 2022 10:32:55 +0200 Subject: [PATCH 5/9] fix snaps --- tests/snapshots/smoke_test__net.snap | 10 +++- tests/snapshots/smoke_test__net.snap.new | 67 ------------------------ 2 files changed, 8 insertions(+), 69 deletions(-) delete mode 100644 tests/snapshots/smoke_test__net.snap.new diff --git a/tests/snapshots/smoke_test__net.snap b/tests/snapshots/smoke_test__net.snap index 3651887..ceed996 100644 --- a/tests/snapshots/smoke_test__net.snap +++ b/tests/snapshots/smoke_test__net.snap @@ -5,16 +5,20 @@ expression: "test_policy(\"test-net\", None).await.expect(\"error in test suite\ - result: cidr_expand_1: - 1.1.0.1 - - 1.1.0.0 - 1.1.0.2 - 1.1.0.3 + - 1.1.0.0 cidr_expand_2: - 1.1.0.0 cidr_expand_3: - "2002::1234:abcd:ffff:c0a8:101" cidr_expand_4: - - "2002::1234:abcd:ffff:c0a8:100" - "2002::1234:abcd:ffff:c0a8:101" + - "2002::1234:abcd:ffff:c0a8:100" + cidr_merge_1: + - 192.0.128.0/23 + cidr_merge_2: + - "1900::/64" docs_test_1: - - 1.1.1.0/24 - 1.1.1.128 @@ -36,6 +40,8 @@ expression: "test_policy(\"test-net\", None).await.expect(\"error in test suite\ - x - - 0 - y + net_lookup: + - 93.184.216.34 output_1: - - x - 0 diff --git a/tests/snapshots/smoke_test__net.snap.new b/tests/snapshots/smoke_test__net.snap.new deleted file mode 100644 index 1d0a5da..0000000 --- a/tests/snapshots/smoke_test__net.snap.new +++ /dev/null @@ -1,67 +0,0 @@ ---- -source: tests/smoke_test.rs -assertion_line: 125 -expression: "test_policy(\"test-net\", None).await.expect(\"error in test suite\")" ---- -- result: - cidr_expand_1: - - 1.1.0.1 - - 1.1.0.2 - - 1.1.0.3 - - 1.1.0.0 - cidr_expand_2: - - 1.1.0.0 - cidr_expand_3: - - "2002::1234:abcd:ffff:c0a8:101" - cidr_expand_4: - - "2002::1234:abcd:ffff:c0a8:101" - - "2002::1234:abcd:ffff:c0a8:100" - cidr_merge_1: - - 192.0.128.0/23 - cidr_merge_2: - - "1900::/64" - docs_test_1: - - - 1.1.1.0/24 - - 1.1.1.128 - docs_test_2: - - - 0 - - 1.1.1.128 - docs_test_3: - - - 0 - - 0 - - - 0 - - 1 - docs_test_4: - - - 1 - - x - - - 1 - - y - docs_test_4_1: - - - 0 - - x - - - 0 - - y - net_lookup: - - 93.184.216.34 - output_1: - - - x - - 0 - output_2: - - - 1.1.0.0/16 - - 1.1.1.128 - output_3: - - - 0 - - 0 - output_4: - - - 0 - - 0 - output_5: - - - 0 - - 0 - output_6: - - - 0 - - y - output_7: - - - 0 - - y - From c7ed8fa12911f7fc9818a57b0da84ad9a88797eb Mon Sep 17 00:00:00 2001 From: Dotan Nahum Date: Mon, 21 Nov 2022 10:04:24 +0200 Subject: [PATCH 6/9] fix tests --- src/builtins/impls/net.rs | 12 ++++++------ tests/snapshots/smoke_test__net.snap | 6 +++--- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/builtins/impls/net.rs b/src/builtins/impls/net.rs index ecb14f7..6e73bed 100644 --- a/src/builtins/impls/net.rs +++ b/src/builtins/impls/net.rs @@ -148,8 +148,6 @@ pub fn cidr_contains_matches( cidrs: serde_json::Value, cidrs_or_ips: serde_json::Value, ) -> Result { - println!("lhs: {:?}, rhs: {:?}", cidrs, cidrs_or_ips); - let res = if let Some(s) = cidrs.as_str() { match_any( &addr_from_cidr_or_ip(s)?, @@ -203,14 +201,16 @@ pub fn cidr_expand(cidr: String) -> Result> { // IpNet.hosts() is too smart because it excludes the // broadcast and network IP. which is why we're doing it manually here to // include all addresses in range including broadcast and network: - Ok(match IpNet::from_str(&cidr)? { + let mut addrs = match IpNet::from_str(&cidr)? { IpNet::V4(net) => ipnet::Ipv4AddrRange::new(net.network(), net.broadcast()) .map(|addr| addr.to_string()) - .collect::>(), + .collect::>(), IpNet::V6(net) => ipnet::Ipv6AddrRange::new(net.network(), net.broadcast()) .map(|addr| addr.to_string()) - .collect::>(), - }) + .collect::>(), + }; + addrs.sort(); + Ok(addrs.into_iter().collect::>()) } /// Merges IP addresses and subnets into the smallest possible list of CIDRs diff --git a/tests/snapshots/smoke_test__net.snap b/tests/snapshots/smoke_test__net.snap index ceed996..6627ee2 100644 --- a/tests/snapshots/smoke_test__net.snap +++ b/tests/snapshots/smoke_test__net.snap @@ -4,17 +4,17 @@ expression: "test_policy(\"test-net\", None).await.expect(\"error in test suite\ --- - result: cidr_expand_1: - - 1.1.0.1 + - 1.1.0.0 - 1.1.0.2 + - 1.1.0.1 - 1.1.0.3 - - 1.1.0.0 cidr_expand_2: - 1.1.0.0 cidr_expand_3: - "2002::1234:abcd:ffff:c0a8:101" cidr_expand_4: - - "2002::1234:abcd:ffff:c0a8:101" - "2002::1234:abcd:ffff:c0a8:100" + - "2002::1234:abcd:ffff:c0a8:101" cidr_merge_1: - 192.0.128.0/23 cidr_merge_2: From 3dcf59d975bc48c88168cc8a75247edf2b16832b Mon Sep 17 00:00:00 2001 From: Dotan Nahum Date: Mon, 21 Nov 2022 18:08:12 +0200 Subject: [PATCH 7/9] fix tests --- src/builtins/impls/net.rs | 31 +++++++++++++++++----------- tests/snapshots/smoke_test__net.snap | 2 +- 2 files changed, 20 insertions(+), 13 deletions(-) diff --git a/src/builtins/impls/net.rs b/src/builtins/impls/net.rs index 6e73bed..c47eb5c 100644 --- a/src/builtins/impls/net.rs +++ b/src/builtins/impls/net.rs @@ -13,6 +13,11 @@ // limitations under the License. //! Builtins related to network operations and IP handling +//! +//! Note: spec says to return a hashset, but we're returning Vec for order +//! stability. where it is not inherently unique, we make guarantee that items +//! are unique as in a hash before converting to a vec. representation on the +//! result side in OPA is always an array (JSON) use std::{collections::HashSet, net::IpAddr, str::FromStr}; @@ -40,9 +45,9 @@ fn addr_from_cidr_or_ip(s: &str) -> Result { /// this expand upon the go side handling additional cases where: /// ip can contain ip (by way of strict equality) /// cidr contains ip, but also ip is contained within cidr (flip the arguments) -/// * in the Go case, it's assumed that root call of `cidr_contains_matches` LHS -/// is CIDR and RHS is "CIDR or IP". here by being direction agnosic we make -/// no such assumption and handle a larger set of cases +/// * in the Go case, it's assumed thatmut root call of `cidr_contains_matches` +/// LHS is CIDR and RHS is "CIDR or IP". here by being direction agnosic we +/// make no such assumption and handle a larger set of cases #[allow(clippy::similar_names)] fn addr_contains(left: &Addr, right: &Addr) -> bool { match (left, right) { @@ -197,20 +202,22 @@ pub fn cidr_contains_matches( /// generates 4 hosts: `{"192.168.0.0", "192.168.0.1", "192.168.0.2", /// "192.168.0.3"}`). #[tracing::instrument(name = "net.cidr_expand", err)] -pub fn cidr_expand(cidr: String) -> Result> { +pub fn cidr_expand(cidr: String) -> Result> { // IpNet.hosts() is too smart because it excludes the // broadcast and network IP. which is why we're doing it manually here to // include all addresses in range including broadcast and network: let mut addrs = match IpNet::from_str(&cidr)? { IpNet::V4(net) => ipnet::Ipv4AddrRange::new(net.network(), net.broadcast()) .map(|addr| addr.to_string()) - .collect::>(), + .collect::>(), IpNet::V6(net) => ipnet::Ipv6AddrRange::new(net.network(), net.broadcast()) .map(|addr| addr.to_string()) - .collect::>(), + .collect::>(), }; - addrs.sort(); - Ok(addrs.into_iter().collect::>()) + let mut res = addrs.into_iter().collect::>(); + res.sort(); + + Ok(res) } /// Merges IP addresses and subnets into the smallest possible list of CIDRs @@ -221,7 +228,7 @@ pub fn cidr_expand(cidr: String) -> Result> { /// Supports both IPv4 and IPv6 notations. IPv6 inputs need a prefix length /// (e.g. "/128"). #[tracing::instrument(name = "net.cidr_merge", err)] -pub fn cidr_merge(addrs: serde_json::Value) -> Result> { +pub fn cidr_merge(addrs: serde_json::Value) -> Result> { if let Some(addrs) = addrs.as_array() { let ipnets = addrs .iter() @@ -234,7 +241,7 @@ pub fn cidr_merge(addrs: serde_json::Value) -> Result> { Ok(IpNet::aggregate(&ipnets) .iter() .map(ToString::to_string) - .collect::>()) + .collect::>()) } else { bail!("data type not supported: {}", addrs); } @@ -243,7 +250,7 @@ pub fn cidr_merge(addrs: serde_json::Value) -> Result> { /// Returns the set of IP addresses (both v4 and v6) that the passed-in `name` /// resolves to using the standard name resolution mechanisms available. #[tracing::instrument(name = "net.lookup_ip_addr", err)] -pub async fn lookup_ip_addr(name: String) -> Result> { +pub async fn lookup_ip_addr(name: String) -> Result> { let resolver = TokioAsyncResolver::tokio( trust_dns_resolver::config::ResolverConfig::default(), trust_dns_resolver::config::ResolverOpts::default(), @@ -254,5 +261,5 @@ pub async fn lookup_ip_addr(name: String) -> Result> { Ok(response .iter() .map(|addr| addr.to_string()) - .collect::>()) + .collect::>()) } diff --git a/tests/snapshots/smoke_test__net.snap b/tests/snapshots/smoke_test__net.snap index 6627ee2..416a1fa 100644 --- a/tests/snapshots/smoke_test__net.snap +++ b/tests/snapshots/smoke_test__net.snap @@ -5,8 +5,8 @@ expression: "test_policy(\"test-net\", None).await.expect(\"error in test suite\ - result: cidr_expand_1: - 1.1.0.0 - - 1.1.0.2 - 1.1.0.1 + - 1.1.0.2 - 1.1.0.3 cidr_expand_2: - 1.1.0.0 From c0422728f903cdf738cbae833951ec4a97275a55 Mon Sep 17 00:00:00 2001 From: Dotan Nahum Date: Wed, 23 Nov 2022 18:06:14 +0200 Subject: [PATCH 8/9] fix tests --- src/builtins/impls/net.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/builtins/impls/net.rs b/src/builtins/impls/net.rs index c47eb5c..b6f3878 100644 --- a/src/builtins/impls/net.rs +++ b/src/builtins/impls/net.rs @@ -206,7 +206,7 @@ pub fn cidr_expand(cidr: String) -> Result> { // IpNet.hosts() is too smart because it excludes the // broadcast and network IP. which is why we're doing it manually here to // include all addresses in range including broadcast and network: - let mut addrs = match IpNet::from_str(&cidr)? { + let addrs = match IpNet::from_str(&cidr)? { IpNet::V4(net) => ipnet::Ipv4AddrRange::new(net.network(), net.broadcast()) .map(|addr| addr.to_string()) .collect::>(), From e9dda178536bd9338fb6c61ca9b4e626950b8451 Mon Sep 17 00:00:00 2001 From: Dotan Nahum Date: Wed, 23 Nov 2022 18:39:46 +0200 Subject: [PATCH 9/9] fix tests --- src/builtins/impls/net.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/builtins/impls/net.rs b/src/builtins/impls/net.rs index b6f3878..7fd4c00 100644 --- a/src/builtins/impls/net.rs +++ b/src/builtins/impls/net.rs @@ -26,6 +26,7 @@ use ipnet::IpNet; use serde_json::Number; use trust_dns_resolver::TokioAsyncResolver; +/// A unified address or CIDR block type #[allow(clippy::upper_case_acronyms)] #[derive(Debug, Clone, Copy)] enum Addr {