Use generated certificates in unit tests #6346
Conversation
| @@ -61,7 +60,7 @@ object OkHostnameVerifier : HostnameVerifier { | |||
|
|
|||
| /** Returns true if [certificate] matches [hostname]. */ | |||
| private fun verifyHostname(hostname: String, certificate: X509Certificate): Boolean { | |||
| val hostname = hostname.toLowerCase(Locale.US) | |||
| val hostname = DnsUtils.normalizeIA5String(hostname) | |||
There was a problem hiding this comment.
should we use hostname.toCanonicalHost() instead?
There was a problem hiding this comment.
It's worse actually, causes us to fail on the tel example also.
okhttp3.internal.tls.HostnameVerifierTest > specialKInHostname FAILED
org.junit.ComparisonFailure: expected:<[fals]e> but was:<[tru]e>
at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
at okhttp3.internal.tls.HostnameVerifierTest.specialKInHostname(HostnameVerifierTest.java:586)
There was a problem hiding this comment.
Maybe there is a fix where fix toCanonicalHost logic and then we use it, but maybe that's a good second pass PR once the test is in.
There was a problem hiding this comment.
What about just removing the toLowerCase(Locale.US) call completely? I’m trying to avoid having a bunch of code in here that doesn’t get exercised in practice.
There was a problem hiding this comment.
It's still public API
@Test
public void thatCatchesErrorsWithBadSession() {
HostnameVerifier localVerifier = new OkHttpClient().hostnameVerifier();
// Since this is public API, okhttp3.internal.tls.OkHostnameVerifier.verify is also
assertThat(verifier).isInstanceOf(OkHostnameVerifier.class);
SSLSession session = TlsUtil.localhost().sslContext().createSSLEngine().getSession();
assertThat(localVerifier.verify("\uD83D\uDCA9.com", session)).isFalse();
}
| @@ -61,7 +60,7 @@ object OkHostnameVerifier : HostnameVerifier { | |||
|
|
|||
| /** Returns true if [certificate] matches [hostname]. */ | |||
| private fun verifyHostname(hostname: String, certificate: X509Certificate): Boolean { | |||
| val hostname = hostname.toLowerCase(Locale.US) | |||
| val hostname = DnsUtils.normalizeIA5String(hostname) | |||
There was a problem hiding this comment.
What about just removing the toLowerCase(Locale.US) call completely? I’m trying to avoid having a bunch of code in here that doesn’t get exercised in practice.
| } | ||
|
|
||
| // https://tools.ietf.org/html/rfc2459#section-4.2.1.11 | ||
| fun normalizeIA5String(s: String): String { |
There was a problem hiding this comment.
In Okio this function is called ByteString.toAsciiLowercase() (though to use that one we’d need to roundtrip through a ByteString)
|
I pushed a commit here: |
Nice simplification Picasso :) I'll land based on your commit. |
* Use generated certificates in unit tests * Strict to ascii lowercase implementation Co-authored-by: Jesse Wilson <jwilson@squareup.com>
* Use generated certificates in unit tests (#6346) * Use generated certificates in unit tests * Strict to ascii lowercase implementation Co-authored-by: Jesse Wilson <jwilson@squareup.com> * More restrictive behaviour of OkHostnameVerifier (#6353) * Test quirks of HostnameVerifier. * Restrict successful results to ascii input. Co-authored-by: Jesse Wilson <jwilson@squareup.com>
We currently embed externally generated certificates in our unit tests, but we can now generate our own certificates so we should.