ipaddr 1.2.4 breaks URL blocker allowlist
The following discussion from !98777 (merged) should be addressed:
-
@mkaeppler started a discussion: (+18 comments) Tests are failing due to what I believe is the
ipaddr
version bump.This validator is failing:
def validate_local_network(addrs_info) return unless addrs_info.any? { |addr| addr.ipv4_private? || addr.ipv6_sitelocal? || addr.ipv6_unique_local? } raise BlockedUrlError, "Requests to the local network are not allowed" end
For the following test and inputs:
Gitlab::UrlBlocker#blocked_url? when allow_local_network is false when local domain/IP is allowed with IPs in allowlist behaves like allows local requests does not block urls from private networks Failure/Error: expect(described_class).not_to be_blocked_url("http://#{ip}", **url_blocker_attributes) expected `Gitlab::UrlBlocker.blocked_url?("http://[::ffff:ac10:20]", {:allow_local_network=>false, :allow_localhost=>false})` to be falsey, got true Shared Example Group: "allows local requests" called from ./spec/lib/gitlab/url_blocker_spec.rb:548
Input IP:
[#<Addrinfo: 172.16.0.32 TCP (::ffff:ac10:20)>]
This is an IP from this list in the test data: https://gitlab.com/gitlab-org/gitlab/-/blob/7359d23f4e078479969c872924150219c6f1665f/spec/lib/gitlab/url_blocker_spec.rb#L405
But we stub out
Addrinfo
here: https://gitlab.com/gitlab-org/gitlab/-/blob/7359d23f4e078479969c872924150219c6f1665f/spec/lib/gitlab/url_blocker_spec.rb#L760This looks fishy since we stub all inputs to return true for
ipv4_local
, but when I run this locally in a console I see:[9] pry(main)> Addrinfo.ip('::ffff:ac10:20').ipv4_private? => false
So we are stubbing out
Addrinfo
in an unexpected way?
This was discovered while working on Ruby 3 support. Quick summary of what happened:
-
openssl
depends on theipaddr
gem - This gem used to be bundled with the Ruby 2.7 distribution
- Ruby 3 has unbundled this gem i.e. it is now tracked via
Gemfile.lock
. - The latest available version (1.2.4) contains changes compared to what Ruby 2.7 used to ship: https://github.com/ruby/ipaddr/compare/v1.2.2...v1.2.4
- Specifically,
IPAddr.include?
has seen changes upstream that changes the equality relation between IPv4 and IPv6 addresses: !98777 (comment 1117740605) (arguably this is just a bug fix but it leads to behavioral changes). - This in turn means that the semantics of IP allowlists defined via
outbound_local_requests_whitelist
have changed and causes some of our unit tests to fail too.
For example, if previously the allowlist would contain the following entry:
::ffff:169.254.168.100
Then if the input IP was 169.254.168.100
or even ::ffff:169.254.168.100
(note that we convert IPv4-mapped IPv6 addresses to IPv4 before probing into the allowlist), the IP would be allowed because due to the aforementioned bug, the ipaddr
library allowed matching an IPv4 address (source) against an IPv6 address (allowlist).
With the bug fixed in 1.2.4, this check will now fail i.e. return false. Admins would have to change or extend the allowlist to actually include an IPv4 address entry or mask such as:
169.254.0.0/16
We should assess how exactly this affects customers before pulling in the newer version and whether customers might have to review their allowlists prior to us relying on the latest version of ipaddr
.