feat(runtime/permissions): support IP CIDR ranges in net allowlist

[dsseng]

Fixes #9816

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[bartlomieju]

@lucacasonato requested not to land this one until he can review it. Removing milestone for now

[bnoordhuis]

Can I suggest switching to Ipv4Network and Ipv6Network from the ipnetwork crate? It'll be easier to read and maintain than the regex approach.

It also supports IPv6 CIDRs like fc00::/7, something your PR doesn't currently handle.

[dsseng]

Yes, currently I only did that for IPv4. ipnetwork looks really good and should make code clearer. If Deno team members are not against including this crate as a dependency (seems not installed), I will integrate it in place of regex.

[ry]

@sh7dm the added dependency seems worth it

[dsseng]

ipnet crate which is already being built with some Deno's dependencies should do the job as well: https://docs.rs/ipnet/2.3.1/ipnet/struct.Ipv4Net.html#examples-11.

[lucacasonato]

I haven't reviewed implementation, but this would likely block full URL network allow list due to possible parsing ambiguity between CIDR ranges and URLs (is this concern warranted?).

With full URL network allow list I mean the following: --allow-net=https://api.github.com/meta would allow fetching https://api.github.com/meta, but not https://api.github.com/users/lucacasonato. Currently filtering is just per domain.

[dsseng]

I haven't reviewed implementation, but this would likely block full URL network allow list due to possible parsing ambiguity between CIDR ranges and URLs (is this concern warranted?).

Can also be a trouble. Maybe we should either add some prefix or another CLI flag for CIDR?

[lucacasonato]

Actually maybe it is not even ambiguous without square brackets:

The port should come after the CIDR range, so after the /. If the string does not contain /, then it is not a cidr range.

So should look like this:

ffff:ffff:ffff::/24:443
|-------1------||2||3-|

1: address
2: subnet size
3: port

If the subnet size is missing address must be enclosed in square brackets if it is IPv6. We already enforce this.

[dsseng]

Yes, but what about URLs? How do we find out that ffff:ffff:ffff::/24 is CIDR allowing any connection to any IP starting with ffff:ffff:ffff... by mask of 24 bits or is allowing requesting HTTP path /24 from ffff:ffff:ffff:0:0:0:0:0? If we force users to use ffff:ffff:ffff::/24 for the first case and [ffff:ffff:ffff::]/24 this will be solved.

[lucacasonato]

@sh7dm IPv6 in URLs already requires that you wrap the address in square brackets: https://url.spec.whatwg.org/#valid-host-string

[lucacasonato]

Overall, happy to get this feature. I have a few more asks though:

• CIDR ranges are re-parsed on each permission check. CIDR ranges should be parsed once, and then stored in the NetDescriptor as a ipnet::IpNet (maybe turn NetDescriptor into an enum?).

• Please also add a test that IPv6 CIDR where the IP address is surrounded by [ ] does not work.

[dsseng]

Well, enum should be a great idea

[bnoordhuis]

@sh7dm Do you still want to move forward with this? Seems like a great feature to have.

[dsseng]

Quite busy currently, sorry. No time for builds and proper testing. I do hope maintainers can help delivering this feature.

[bnoordhuis]

That's too bad but thanks for your reply. Since this PR has merge conflicts, I'll go ahead and take the liberty of closing it.