okhttp: properly verify IPv6 address hosts

[OnlyInAmerica]

Address mismatch between IPv6 address hosts derived from URIs and X509 subjectAltName extensions as discussed in #4278

[ericgribkoff]

Thanks for the fix! A few small comments to address, then we can get this in

[ericgribkoff]

This can be private

[OnlyInAmerica]

Is package-private with @VisibleForTesting (ala TLS_PROTOCOLS) acceptable?

[ericgribkoff]

Oops, my mistake - yes, the visibility is fine, but the @VisibleForTesting annotation would be helpful.

[ericgribkoff]

This may be safe as-is, but we should be more conservative here: if the first and last characters are [ and ], strip only those two characters out. Otherwise, just return the string.

This would mean that we can add a test like assertEquals("[someIp]", canonicalizeHost("[[someIp]]"));. This address would, as in #4278, result in OkHostnameVerifier failing to match the host - but that's good, because having two brackets is malformed anyways, and probably indicates a bug somewhere else that we don't want to mask here.

[ericgribkoff]

I think this fails checkstyle (100 char limit per line)

[ericgribkoff]

The argument order for assertEquals is reversed. These should be in the form: assertEquals("ip", canonicalizeHost("ip")). (this results in better output when a test fails, as error message will label the first argument as the expected value and the second argument as the actual value)

[ericgribkoff]

Tests coverage look good to me (with the addition of a case to ensure we don't strip out additional brackets for malformed URIs). Out of curiosity, what methodology went into choosing the test hosts?

[OnlyInAmerica]

Will update to ensure we don't strip out additional brackets.

I lifted these from okhttp's HostnameVerifierTest, so I'll add some attribution.

[ericgribkoff]

Let's simplify the tests somewhat here, which will also remove the need for attribution - we actually don't care about all of the edge cases that the linked test case covers, because we don't need to distinguish between IP addresses and hostnames. Probably just the following are enough:

assertEquals("::1", canonicalizeHost("::1"));
assertEquals("::1", canonicalizeHost("[::1]"));
assertEquals("127.0.0.1", canonicalizeHost("127.0.0.1"));
assertEquals("some.long.url.com", canonicalizeHost("some.long.url.com"));

// Extra square brackets in a malformed URI are retained
assertEquals("[::1]", canonicalizeHost("[[::1]]"));

Also, does it make sense to rename the test method to just canonicalizeHost, as this isn't testing any other part of the upgrade logic?

[OnlyInAmerica]

👍 Is canonicalizeHosts ok to avoid clash with the static import of canonicalizeHost?

[ericgribkoff]

sounds good to me

[OnlyInAmerica]

fixed whitespace issue to appease travis. One more time from the top 🥁

[ericgribkoff]

Thanks!