-
Notifications
You must be signed in to change notification settings - Fork 337
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SNI support to be enabled by default? #393
Comments
Yup. It could have a check via java interop to check that the java.vm.version system property is 1.8 -> |
I've just spent the better part of a day debugging this problem and ended up here. I think the current situation is untenable: sites proxied via CloudFlare require SNI, as do many other places on the Internet. I think support for older JVM versions is secondary. These days code running on Java 6 and 7 will not be able to connect to many modern APIs or sites anyway, because of 1) SNI, and 2) limited crypto policy that is enforced by default, so without manual installation of JCE unlimited policy files many sites will fail because of cipher negotiation. I implemented the workaround suggested by @kumarshantanu (thanks!), but I think http-kit client is deficient right now and the lack of SNI is a major pitfall which deserves at least a mention in the README. |
Hey everyone, sorry for the inactivity on this- haven't had an opportunity to spend any time on this. Could someone kindly summarise the current situation, options, and trade-offs? |
Well, I've outlined my rationale above — not sure what I could add to that. To rephrase and summarize:
My conclusion is that while I love backwards compatibility, in this case I think it makes little sense, at least for the http-client part. Perhaps a last release that supports Java 6/7 could be made, and then we should start requiring Java 8 and support SNI. |
Totally agree with @jwr Java world has changed a lot along the years and as licensing models have also changed I kinda assume that there are less and less users who stay in Oracle's supported old JDKs / JREs - except for some enterprise applications, which then work inside a corporate network. For them these kind of troubles are not necessarily at all an issue, and neither would using a legacy version of http-kit... If they would be using clojure at all. Rest of us who are building or interoperating with public facing internet / cloud services need a more modern and maintained JDK versions in any case. |
Thanks Jan. Like I mentioned, what would be helpful to have now is:
So this a non trivial topic, with several related threads. I haven't had (and won't have for some weeks) an opportunity to properly catch up on this and related threads, so if someone wants to help move this along- the above info would be a great way to assist! Appreciate that there's folks strongly motivated to get something changed ASAP- but please keep in mind that a lot of folks depend on http-kit, and not everyone's needs (or environment) is the same. The above info represents a minimal set of clarity I think we should aim for before taking an action on this. Thanks everyone! |
For those wondering exactly how to implement the workaround: (ns foo.core
(:require [org.httpkit.client :as http-client])
(:import java.net.URI
[javax.net.ssl SNIHostName SSLEngine]))
(defn ssl-configurer [^SSLEngine eng, ^URI uri]
(let [host-name (SNIHostName. (.getHost uri))
params (doto (.getSSLParameters eng)
(.setServerNames [host-name]))]
(doto eng
(.setUseClientMode true) ;; required for JDK12/13 but not for JDK1.8
(.setSSLParameters params))))
(comment
(def client (http-client/make-client {:ssl-configurer ssl-configurer}))
@(http-client/request {:method :get
:url "https://www.google.com/"
:client client})
) |
@ptaoussanis is absolutely right. This is a non-trivial breaking change and it does need careful consideration. Unfortunately, I am unable to dedicate time to this in the near future, so unless someone else takes the lead on this issue, we will have to make do with workarounds. |
Maybe in the short term, we can document the workaround and make it easy for people to find it. Currently, if you just use http-kit with a cloudflare server, you get an exception:
And it's not clear what to do. I'm not sure people will find this issue, since the exception doesn't say anything about SNI. I volunteer to write up a page, "Debugging SSLHandshakeException" with a bunch of things I've learned while debugging this issue myself, including the workaround for CloudFlare/SNI. But where should this page go, and how do we help people find it? The first thought that comes to mind is to create an open github issue with "handshake_failure" in the title. When I hit this exception, one of the first things I did was look at the open issues, but I totally missed this one because it took me a whole day to figure out it was related to SNI. |
@aiba I think this is a great idea. I had also spent about a day debugging and searching until I figured out the handshake failure is SNI-related. |
…configurer enabled
…configurer enabled
…configurer enabled
…configurer enabled
…configurer enabled
…schedda) Adds a new namespace which provides an SNI-capable client without breaking backwards compatibility with older JVM versions (< Java 8). The SNI-capable client can be provided as an argument to `org.httpkit.client/make-client`, or swapped in to `org.httpkit.client/*default-client*`.
I would like to revive this thread by asking if SNI can be enabled by default on runtimes that support it, as an internal library detail. |
FWIW, babashka already ships http-kit with SNI enabled by default (since its runtime is Java 11 this is not a problem). I would prefer a solution that is compatible with GraalVM. E.g.: (defmacro with-java-8 [& body]
(when (resolve 'javax.net.ssl.SNIHostName)
`(do ~@body))) and then (with-java-8
(def *default-client* ...)) Something like that would work. |
@ptaoussanis Just letting you know that I've seen the same question over and over in the beginners channel in Clojurians slack: Why doesn't httpkit client work with this request? "Can you test with the native babashka httpkit client (which has SNI enabled)?" Yes, that works. "Have you enabled SNI in httpkit?" Oh, no, I haven't. Happened multiple times this week. |
I've also had people in my company ask me why http-kit doesn't just work by default. I am strongly in favor of enabling this by default. I think even Java's built-in |
Hi everyone, sorry for the delayed response - has been a bit of a crazy period for me at (and outside) work. Just to clarify what we're talking about here- there's 3x cases to consider:
I've only had an opportunity to skim this thread, but if I understand correctly from Michiel's comment that we believe we can successfully achieve case 1, then that'd be perfect and someone just needs to please submit a PR. Am I understanding correctly? |
I think that's correct, although I would be surprised if people were still depending on Java 7 at this point. It's not officially support by Clojure itself anymore. |
Echoing @borkdude, I would imagine anyone still using clojure with Java 7 is not in the habit of bumping any of the version numbers of their dependencies. And if http-kit must always continue to support Java 7, that comes at a cost of additional complexity (such as cases 1 and 2 above). And even more complexity for each new feature that requires different code paths to support Java 7. So I would vote for case 3, http-kit versions above some # require Java >=8. Java 7 users (if there are any) would just stick with a version of http-kit that supports Java 7. |
Hi Aaron!
There's several statements here that I think are possibly unsupported:
So all I'm advocating for at this point is:
With the above info, we can jointly make an informed decision about how we want to proceed. This would include weighing the value of continued Java 7 support. Just saying "enable SNI support by default!" is not specific enough, since it doesn't distinguish between cases 1, 2, 3 - which have different implementations and trade-offs. I hope this position seems reasonable? |
Yes, I actually agree with all of that. And I think your comment correctly understands and identifies the three cases. And I agree it's a tradeoff that is hard to quantify. I didn't mean to accuse cases 1 and 2 of being obviously bad (sorry if it came off that way). I just wanted to weigh in with my 2¢ in favor of case 3, for what that's worth. |
@aiba Thanks for clarifying, makes sense- think we're on the same page 👍
That's definitely useful input- as was your important observation about potential complexity costs for cases 1 and 2. Next step: input/ideas/PRs from anyone welcome on this. Otherwise I can try find an opportunity myself, but can't guarantee how soon that'd be since I'm going to continue to be pretty heavily loaded till end June. |
I vote for this approach, Clojure no longer supports Java < 8, it doesn't make sense for http-kit to continue such support. If you want to use Java < 8, you are stuck on an older version of Clojure, and likely an older version of http-kit.
|
Totally agree. I think it's about time. I think there are some other issues/PR's that are closely related too. |
We're on the same page, SNI support will be enabled by default in v2.7 👍 |
PR 513 is up to update default client, merge is blocked on updating broken client tests. Assistance welcome for that, otherwise I'll try take a look myself in a few weeks. |
Issue appears to be that the (ClientSslEngineFactory/trustAnybody) SSL engine created when :insecure? is true is incompatible with the new default SSL client. Still needs to be properly investigated, but think it's reasonable to just disable these particular tests for the upcoming alpha to prevent further holding up #393.
Closing, will be addressed in upcoming v2.7 beta 👍 |
If I would make a pull request which would enable the SNI support in http-kit client by default, is there any reasons why it would not be accepted? Are there any security or other reasons to keep the current config where SNI needs to be enabled by own ssl configurator.
We are using http-kit heavily and now need to configure SNI support to be enabled in every project.
The text was updated successfully, but these errors were encountered: