Skip to content

Conversation

@iamdanfox
Copy link
Contributor

@iamdanfox iamdanfox commented Oct 15, 2018

Before this PR

An earlier PR (#794) upgraded okhttp from 3.9.0 -> 3.11.0. This change from okhttp 3.10.0 meant conjure-java-runtime clients were unable to talk to a particular type of internal server (as it is currently only given certs with a CN and not a proper SAN):

New: Don't fall back to common name (CN) verification for hostnames. This behavior was deprecated with RFC 2818 in May 2000 and was recently dropped from major web browsers.

After this PR

Jersey and Retrofit clients can be configured to use the old deprecated CN verification fallback behaviour.

TODO:

  • is "fallbackToCommonNameVerification" the right boolean option name to add to conjure-java-runtime-api?
  • agree on a timeframe when this can be removed

@iamdanfox iamdanfox requested a review from a team as a code owner October 15, 2018 19:55
@iamdanfox iamdanfox changed the title [fix] Backport OkHostnameVerifier from okhttp 3.9 [fix] Preserve OkHostnameVerifier from okhttp 3.9 Oct 15, 2018
@robert3005
Copy link
Contributor

@iamdanfox what's latest here? Do we still want to merge this?

@iamdanfox
Copy link
Contributor Author

Yep, if nobody has any concerns about the name then I just need to make a PR to add this extra field to the conjure-java-runtime-api!

@darora
Copy link
Contributor

darora commented Oct 24, 2018

lgtm

@carterkozak
Copy link
Contributor

Is there more context somewhere? This seems dangerous and counterproductive. Can we not fix the service with bad certs?

@palantir palantir deleted a comment from iamdanfox Oct 25, 2018
@j-baker
Copy link
Contributor

j-baker commented Oct 25, 2018

We are not intending to merge this as is - we're going to put it behind a feature flag, which is only expected to be enabled on the (3?) internal services which talk to the relevant internal server.

@robert3005
Copy link
Contributor

@iamdanfox do you have timeline for adding the feature flag or should I submit the pr for that?

@tpetracca
Copy link
Contributor

@iamdanfox @robert3005 is this waiting on something?

@kmblake
Copy link

kmblake commented Nov 26, 2018

@j-baker any updates on the feature flag? This issue became a problem for gemini after we upgraded conjure-java-runtime because certain axiom hosts only include hostname in the CN, so gemini was running into a bunch of javax.net.ssl.SSLPeerUnverifiedException errors (see internal issue 2694). We're working on making sure that all axiom instances include hostname in the SAN, but we would definitely like to use this legacy CN verification option in the meantime.

@robert3005
Copy link
Contributor

This is superseded by #907

@robert3005 robert3005 closed this Dec 21, 2018
@robert3005 robert3005 deleted the dfox/backport-okhttp-3-9-hostname-verifier branch December 21, 2018 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants