fix: allow calling internal IP range 100.64.0.0/10 with relevant ResilientClient options

[David-Wobrock]

The ResilientClient options ResilientClientDisallowInternalIPs and ResilientClientAllowInternalIPRequestsTo were not allowing to call certain IP ranges, like 100.64.0.0/10 properly.

Related Issue or Design Document

Fixes: #805

And relates to Kratos issue: ory/kratos#4049

Checklist

• I have read the contributing guidelines and signed the CLA.
• I have referenced an issue containing the design document if my change introduces a new feature.
• I have read the security policy.
• I confirm that this pull request does not address a security vulnerability.
  If this pull request addresses a security vulnerability,
  I confirm that I got approval (please contact [email protected]) from the maintainers to push the changes.
• I have added tests that prove my fix is effective or that my feature works.
• I have added the necessary documentation within the code base (if appropriate).

Further comments

[David-Wobrock]

Thanks for the review @alnr 🙏

I pushed some edits around the suggestion about the tests.

Let me know what you think :)

[aeneasr]

Thank you for the PR - I just want to note that we have pretty strict security requirements in our internal systems, and generally do not allow merging a weakening of those guarantees. Whatever ends up in the final code must deny/allow the same IP ranges as before.

[David-Wobrock]

Thank you for the PR - I just want to note that we have pretty strict security requirements in our internal systems, and generally do not allow merging a weakening of those guarantees. Whatever ends up in the final code must deny/allow the same IP ranges as before.

Thanks for the details, however I am unsure how to interpret them 🤔

Would you consider adding 100.64.0.0/10 as "deny/allow the same IP ranges as before"? :)

diff --git a/httpx/ssrf.go b/httpx/ssrf.go
index 99b16e9..ae3b817 100644
--- a/httpx/ssrf.go
+++ b/httpx/ssrf.go
@@ -89,6 +89,7 @@ func init() {
                ssrf.WithNetworks("tcp4", "tcp6"),
                ssrf.WithAllowedV4Prefixes(
                        netip.MustParsePrefix("10.0.0.0/8"),     // Private-Use (RFC 1918)
+                       netip.MustParsePrefix("100.64.0.0/10"),  // Shared Address Space (RFC 6598)
                        netip.MustParsePrefix("127.0.0.0/8"),    // Loopback (RFC 1122, Section 3.2.1.3))
                        netip.MustParsePrefix("169.254.0.0/16"), // Link Local (RFC 3927)
                        netip.MustParsePrefix("172.16.0.0/12"),  // Private-Use (RFC 1918)

By default, we would deny the same IP ranges (the ones in ssrf), however we would allow adding an exception for 100.64.0.0/10 IPs (which can never be called today).

[David-Wobrock]

Hello @alnr @aeneasr

Thanks for your input.

Since we don't seem to be on a path to make the option to call internal IPs work properly for all IP ranges, I've opted for the minimal approach now to make only 100.64.0.0/10 work for now (which is an IP range used in our Kube cluster).

[David-Wobrock]

Hello @alnr @aeneasr 👋

Let me know if you think we should adapt the patch 😇

[Atar-rr]

@David-Wobrock
Hi
We’ve also encountered this issue. Could you apply this patch?

[David-Wobrock]

@David-Wobrock Hi We’ve also encountered this issue. Could you apply this patch?

I rebased the PR, even though I'm unsure why 035d1e2 removed all test files 🤔

[alnr]

I rebased the PR, even though I'm unsure why 035d1e2 removed all test files 🤔

We've recently moved to an internal monorepo using https://github.com/google/copybara to synchronize PRs with the open-source repose, and there are still a couple of rough edges to iron out. I think this is one of them.

As to the general fate of this PR: I want to believe it is correct and would like to merge this change. However, #806 (comment) is still relevant. Maybe you can produce some documentation links on how adding this CIDR range is guaranteed to be safe?

We value community contributions immensely. For a while now we have not had enough resources to review and merge enough of them. This will improve with time as Ory grows (looking good). ❤️

[David-Wobrock]

Hey @alnr
Thanks for the answer!

As to the general fate of this PR: I want to believe it is correct and would like to merge this change. However, #806 (comment) is still relevant. Maybe you can produce some documentation links on how adding this CIDR range is guaranteed to be safe?

In terms of SSRF prevention, I think this PR should neither be weakening nor increasing security.
Default behaviour should remain unchanged: we deny by default the internal/private/local IP ranges.

The goal is to add the ability to allow this specific range, like it's done for others. It's not a special case compared to others.
The RFC6598 range should be pretty similar to how RFC1918 spaces behave.

And I want to allow users to create an exception for this range, so that some specific IPs can be defined as valid internal callbacks. I found a related discussion on kOps: kubernetes/kops#7325.

Does that make sense? :)

[waldner]

Just for my understanding here...while the default config is ok, why is there no option to override or integrate it with user-supplied config or env vars? That way, each environment could manage its own specificities. Would a PR introducing this possibility even be considered?