Add support for specifying a custom hostname verifier when using on OkHttpChannelBuilder#3205
Conversation
|
Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok for us to run on our infrastructure. |
|
okay to test |
ejona86
added
the
kokoro:run
|
This is fair. What we don't want is for people to disable hostname verification, so during review I may ask for some docs strongly discouraging doing such things. I assume you could do things like certificate pinning to avoid user-aided MITM attacks and the like. I'm leaving for the day, so not doing the actual review at the moment (although I see it is pretty trivial). |
kokoro-team
removed
the
kokoro:run
|
Yep, would definitely support some extra javadoc suggesting you avoid using this to shoot yourself in the foot... This let's you add a lot of extra security measures (certificate pinning being the most common one I've heard of), but unfortunately that means you can do silly things as well. |
| * if an sslSocketFactory is set. If the hostname verifier is not set, a default hostname | ||
| * verifier is used. | ||
| * @see io.grpc.okhttp.internal.OkHostnameVerifier | ||
| */ |
There was a problem hiding this comment.
Please consider adding @Nullable
| /** | ||
| * Set the hostname verifier to use when using TLS negotiation. The hostnameVerifier is only used | ||
| * if an sslSocketFactory is set. If the hostname verifier is not set, a default hostname | ||
| * verifier is used. |
There was a problem hiding this comment.
Would you please add a @since 1.6.0 and a @returns this ?
| InetSocketAddress address = InetSocketAddress.createUnresolved("hostname", 31415); | ||
| clientTransport = new OkHttpClientTransport( | ||
| address, "hostname", null /* agent */, executor, null, | ||
| address, "hostname", null /* agent */, executor, null, null, |
There was a problem hiding this comment.
please include an inline comment for what this null is.
| * Set the hostname verifier to use when using TLS negotiation. The hostnameVerifier is only used | ||
| * if an sslSocketFactory is set. If the hostname verifier is not set, a default hostname | ||
| * verifier is used. | ||
| * |
There was a problem hiding this comment.
Also add something like:
This method should not be used to avoid hostname verification, even during testing, since {@link #overrideAuthority} is a safer alternative as it does not disable any security checks.
There was a problem hiding this comment.
I liked your wording on that; went ahead and added that to the method doc verbatim.
ejona86
left a comment
ejona86
left a comment
There was a problem hiding this comment.
Actually, I like your feature, so there should be a test to make sure it stays a feature :).
I think Http2OkHttpTest is probably the easiest place for it. I think two simple tests should work: hard-code a HostnameVerifier to return false in one and true in the other.
| } | ||
|
|
||
| /** | ||
| * Set the hostname verifier to use when using TLS negotiation. The hostnameVerifier is only used |
There was a problem hiding this comment.
The hostnameVerifier is only used if an sslSocketFactory is set.
Actually, that's a bit misleading. You can use TLS without manually setting the sslSocketFactory(); grpc will just create one for you (in createSocketFactory()).
There was a problem hiding this comment.
Oops, right you are; I missed that createSocketFactory makes one for you. I changed it to say that it's only used when using TLS negotiation, which I think is accurate.
|
Thanks for the pointer to Http2OkHttpTest. Knowing where to utilize the existing testing framework is half the battle sometimes. ;) I got a couple of tests added in there. |
|
@JackOfMostTrades, the CI tests are now failing. Sorry, no lambdas allowed :-). Yeah, in this case I'd say finding testing framework is 90%. Http2OkHttpTest would have been hard for you to find, and the unit tests in okhttp/src/test would have been painful to adapt. I think it's likely an outlier though. Thank you! |
|
@JackOfMostTrades could you update the commit message to use the "package: what improved" format? Something like: oktthp: add support for specifying a custom hostname verifier when using on OkHttpChannelBuilder |
Oops. I have too much faith in my IDE to set the right language level on projects. ;) Tests updated.
Should I collapse all the commits, or do you want me to just update the message first one? |
|
@JackOfMostTrades Squash, if you would. |
…ing on OkHttpChannelBuilder.
|
Jenkins, retest this please |
6 participants
We use custom x509 extensions to encode multiple dimensions of server identity, and clients verify different dimensions based on individual client requirements. This is all coded up into HostnameVerifier implementations, but we need to ability to pass this to the the OkHttpChannelBuilder. I've implemented the ability to pass this through, defaulting to the existing OkHostnameVerifier implementation when unspecified.