toCanonicalHost() about okhttp

[yschimke]

Also if I read correctly, ::192.168.0.1 is now a deprecated form "IPv4-Compatible IPv6 Address". https://datatracker.ietf.org/doc/html/rfc4291#section-2.5.5.1

While "IPv4-Mapped IPv6 Address" would be ::FFFF:192.168.0.1

[yschimke]

OK, so we are doing this for "IPv4-Mapped IPv6 Address", I want to double check that.

canonicalizeInetAddress and isMappedIpv4Address

But we aren't for the deprecated "IPv4-Compatible IPv6 Address".

[yschimke]

I can't find examples of servers with IP Addresses that are exposed on the IPv4 mapped IPv6 address also.

PS C:\Users\yuri> curl https://[::1.1.1.1]/
curl: (7) Failed to connect to ::1.1.1.1 port 443 after 0 ms: Couldn't connect to server
PS C:\Users\yuri> curl https://[::ffff:1.1.1.1]/
curl: (60) schannel: SNI or certificate check failed: SEC_E_WRONG_PRINCIPAL (0x80090322) - The target principal name is incorrect.
More details here: https://curl.se/docs/sslcerts.html

curl failed to verify the legitimacy of the server and therefore could not
establish a secure connection to it. To learn more about this situation and
how to fix it, please visit the web page mentioned above.

If we were meant to convert to the IPv4 address I think curl above would succeed.

Looking at https://crt.sh/?q=63e7d22b2c577656fda31462799d86cb725da7112c7f59b42615b0f96ac3c348

            X509v3 Subject Alternative Name: 
                DNS:cloudflare-dns.com
                DNS:*.cloudflare-dns.com
                DNS:one.one.one.one
                IP Address:1.0.0.1
                IP Address:1.1.1.1
                IP Address:162.159.36.1
                IP Address:162.159.46.1
                IP Address:2606:4700:4700:0:0:0:0:1001
                IP Address:2606:4700:4700:0:0:0:0:1111
                IP Address:2606:4700:4700:0:0:0:0:64
                IP Address:2606:4700:4700:0:0:0:0:6400

[hyunjic]

Thanks for your detailed comments! If I understand correctly, "IPv4 compatible IPv6 address" is no longer used in DNS resolution or certificates, so it would be "safe" to not correctly normalise this? If so, I can just remove these from the unit tests.
We are using this function for client side hostname verifier, so we wanted to make sure this does not affect any users.

[swankjesse]

Yeah good call. This looks like a bug!

[yschimke]

[edit]

I think spec might be

https://datatracker.ietf.org/doc/html/rfc6125

3.1.3.2. Comparison of IP Addresses
When the reference identity is an IP address, the identity MUST be
converted to the "network byte order" octet string representation
[IP] [IPv6]. For IP Version 4, as specified in RFC 791, the octet
string will contain exactly four octets. For IP Version 6, as
specified in RFC 2460, the octet string will contain exactly sixteen
octets. This octet string is then compared against subjectAltName
values of type iPAddress. A match occurs if the reference identity
octet string and value octet strings are identical.

[yschimke]

I think curl agrees. https://github.com/curl/curl/blob/0f4c19b66ad5c646ebc3c4268a0f3ad9c15bf57c/lib/vtls/openssl.c#L2222

[yschimke]

Listing where an why we use this

OkHostnameVerifier - to verify IP address matches according to RFC-6125
CertificatePinner - to check against IP
Cookie - domain
Route.toString
HttpUrl - decoding and setting host

If we change for HttpUrl and route, we may need to leave hostname verification and certificate pinning as is.

[yschimke]

I think JDK also doesn't normalise these for certificates. So I'm tempted to we should do less canonicalisation/normalisation here.

https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/sun/security/util/HostnameChecker.java#L134

[yschimke]

Chrome/Brave also fails

[yschimke]

Yep - but I think you've pointed out that we do too much, rather than not enough.