Skip to content
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

http-kit accepts some invalid SSL certificates #344

Closed
vgeshel opened this issue Jul 24, 2017 · 4 comments
Closed

http-kit accepts some invalid SSL certificates #344

vgeshel opened this issue Jul 24, 2017 · 4 comments

Comments

@vgeshel
Copy link

vgeshel commented Jul 24, 2017

Example URL: https://78recipes.com/. This site has a certificate with a different hostname, but http-kit accepts it:

=> @(org.httpkit.client/request {:url "https://78recipes.com"})
{:status 200 ...}

This certificate is rejected by every other HTTPS client I've tried, including other Java/Clojure clients:

=> (.openStream (java.net.URL. "https://78recipes.com"))
;; exception
=> (clj-http.client/get "https://78recipes.com")
;; exception: SSLException hostname in certificate didn't match: <78recipes.com> != <*.hostgator.com> OR <*.hostgator.com> OR <hostgator.com>  org.apache.http.conn.ssl.AbstractVerifier.verify (AbstractVerifier.java:238)

A similar error was reported in #316. The URLs proposed for testing in that issue still work correctly, e.g., @(org.httpkit.client/get "https://wrong.host.badssl.com") returns an error. But the URL mentioned at the top does not return an error.

@javahippie
Copy link
Contributor

javahippie commented Aug 7, 2017

It seems, that "https://wrong.host.badssl.com" is also not working properly, as the message there is unable to find valid certification path to requested target. The wrong.host certificate also suffers from an incomplete chain, this throws an error. It seems like the http-kit client is not able to handle this hostname mismatch right now, which is a really bad thing.

Edit: I think we need an implementation of java.net.ssl.HostnameVerifier to make this work. A good guideline seems to be the DefaultHostnameVerifier from the apache httpclient, this is not a trivial thing.

@jeffh
Copy link

jeffh commented Dec 29, 2018

I was having this problem with 2.4.0-alpha2 (requesting httpbin) and was looking into a better way to configure stuff for HTTPS. Here’s some prototyping work that seems to work more similarly to URL.openStream:

(defn ssl-configure                                                      
  [^javax.net.ssl.SSLEngine ssl-engine ^URI uri]                          
  (let [^javax.net.ssl.SSLParameters ssl-params (.getSSLParameters ssl-engine)]                                                                    
    (.setUseClientMode ssl-engine true)                                   
    (.setEndpointIdentificationAlgorithm ssl-params "HTTPS")              
    (.setServerNames ssl-params [(javax.net.ssl.SNIHostName. (.getHost uri))])                                                                     
    (.setSSLParameters ssl-engine ssl-params)))                           
                                                                          
(def client (http-kit.client/make-client {:ssl-configurer ssl-configure}))                                                                        
(def ssl-ctx (javax.net.ssl.SSLContext/getDefault))             
                                                                          
(defn engine-for-url [ssl-ctx url]                                       
  (let [addr                                                              
        (.findAddress org.httpkit.client.HttpClient$AddressFinder/DEFAULT 
                      (java.net.URI. url))]                               
    (.createSSLEngine ^javax.net.ssl.SSLContext ssl-ctx                   
                      (.getHostName addr)                                 
                      (.getPort addr))))

With comparison:

(dose [url ["https://wrong.host.badssl.com"                              
             "https://self-signed.badssl.com"                             
             "https://untrusted-root.badssl.com"                          
             "https://revoked.badssl.com"                                 
             "https://pinning-test.badssl.com"                            
             "https://httpbin.org/get"]]                                  
  (print url "-")                                                         
  (try                                                                    
    (.openStream (java.net.URL. url))                                     
    (catch javax.net.ssl.SSLException _                                   
      (print " URL.openStream=FAIL")))                                    
  (when (:error @(http-kit.client/request {:url url}))                             
    (print " http-kit=FAIL"))                                             
  (when (:error @(http-kit.client/request {:client client                          
                                           :url url}))                             
    (print " http-kit-custom-client=FAIL"))                               
  (when (:error @(http-kit.client/request {:client client                          
                                           :sslengine (engine-for-url ssl-ctx url) 
                                           :url url}))                             
    (print "  http-kit-custom-client-and-engine=FAIL"))                    
  (println))  

Output:

https://expired.badssl.com -                                              
 URL.openStream=FAIL                                                      
 http-kit-custom-client=FAIL                                              
 http-kit-custom-client-and-engine=FAIL                                   
https://wrong.host.badssl.com -                                           
 URL.openStream=FAIL                                                      
 http-kit-custom-client=FAIL                                              
 http-kit-custom-client-and-engine=FAIL                                   
https://self-signed.badssl.com -                                          
 URL.openStream=FAIL                                                      
 http-kit-custom-client=FAIL                                              
 http-kit-custom-client-and-engine=FAIL                                   
https://untrusted-root.badssl.com -                                       
 URL.openStream=FAIL                                                      
 http-kit-custom-client=FAIL                                              
 http-kit-custom-client-and-engine=FAIL                                   
https://revoked.badssl.com -                                              
 http-kit-custom-client=FAIL                                              
                                                                          
https://pinning-test.badssl.com -                                         
                                                                          
https://httpbin.org/get -                                                 
 http-kit=FAIL                                                            
 http-kit-custom-client=FAIL

This also seems to solve #187.

I haven’t verify this on other java versions (I’m playing around with this on Ubuntu 18: Java8 and Java11). But this is probably going to require a change to the API to incorporate SSLEngine.createSSLEngine(ipAddr, portAddr) after findAddress is called inside http-kit. Although, I'd like to try to open a pull request in the next week or so.

I didn't look at apache's DefaultHostnameVerifier, so I'm not sure if there's extra things that it's doing beyond java's URL.openStream is doing.

@jimpil
Copy link
Contributor

jimpil commented Jan 5, 2020

Happy new year guys :)

@jeffh I took your code and adapted it to this.

This would basically enable someone to write the following:

(request {:client :default-https  ;; see `default-client-https`                     
          :ssl-context (SSLContext/getDefault)
          :url "https://wrong.host.badssl.com"})

@ptaoussanis I'm not opening a PR because first of all this is a prototype, but also because i haven't got a clue whether something like this would be acceptable. Moreover, I'm quite confused about whether compatibility with Java6 is a goal for a release like 2.4, which aims to address issues with versions 11 (and above). If it is not, then it should be made clear and then we can have the sni-client.clj folded into the client.clj (after all it's just a flag).

Let me know of your thoughts for this general approach. I've tested it on Java11 & 12:

@(request {:client :default-https                  
          :ssl-context (SSLContext/getDefault)
          :url "https://wrong.host.badssl.com"})
=>
{:opts {:client :default-https,
        :ssl-context #object[javax.net.ssl.SSLContext 0x1a69ee10 "javax.net.ssl.SSLContext@1a69ee10"],
        :url "https://wrong.host.badssl.com"},
 :error #error{:cause "No subject alternative DNS name matching wrong.host.badssl.com found.",
               :via [{:type javax.net.ssl.SSLHandshakeException,
                      :message "No subject alternative DNS name matching wrong.host.badssl.com found.",
                      :at [sun.security.ssl.Alert createSSLException "Alert.java" 131]}
                     {:type java.security.cert.CertificateException,
                      :message "No subject alternative DNS name matching wrong.host.badssl.com found.",
                      :at [sun.security.util.HostnameChecker matchDNS "HostnameChecker.java" 211]}],
               :trace [[sun.security.util.HostnameChecker matchDNS "HostnameChecker.java" 211]
                       [sun.security.util.HostnameChecker match "HostnameChecker.java" 102]
                       [sun.security.ssl.X509TrustManagerImpl checkIdentity "X509TrustManagerImpl.java" 455]
                       [sun.security.ssl.X509TrustManagerImpl checkIdentity "X509TrustManagerImpl.java" 415]
                       [sun.security.ssl.X509TrustManagerImpl checkTrusted "X509TrustManagerImpl.java" 283]
                       [sun.security.ssl.X509TrustManagerImpl checkServerTrusted "X509TrustManagerImpl.java" 141]
                       [sun.security.ssl.CertificateMessage$T12CertificateConsumer
                        checkServerCerts
                        "CertificateMessage.java"
                        619]
                       [sun.security.ssl.CertificateMessage$T12CertificateConsumer
                        onCertificate
                        "CertificateMessage.java"
                        460]
                       [sun.security.ssl.CertificateMessage$T12CertificateConsumer
                        consume
                        "CertificateMessage.java"
                        360]
                       [sun.security.ssl.SSLHandshake consume "SSLHandshake.java" 392]
                       [sun.security.ssl.HandshakeContext dispatch "HandshakeContext.java" 443]
                       [sun.security.ssl.SSLEngineImpl$DelegatedTask$DelegatedAction run "SSLEngineImpl.java" 1074]
                       [sun.security.ssl.SSLEngineImpl$DelegatedTask$DelegatedAction run "SSLEngineImpl.java" 1061]
                       [java.security.AccessController doPrivileged "AccessController.java" 689]
                       [sun.security.ssl.SSLEngineImpl$DelegatedTask run "SSLEngineImpl.java" 1008]
                       [org.httpkit.client.HttpsRequest doHandshake "HttpsRequest.java" 89]
                       [org.httpkit.client.HttpClient doRead "HttpClient.java" 188]
                       [org.httpkit.client.HttpClient run "HttpClient.java" 489]
                       [java.lang.Thread run "Thread.java" 835]]}}

Thanks in advance...

[EDIT]: follow up commit fixing a bug and reworking the names.

@jimpil
Copy link
Contributor

jimpil commented Jan 5, 2020

A bit more testing on all those badssl URLs shows that my new default-client-https behaves exactly like (.openStream (URL. "https://...")) on Java12. Observe this:

(slurp (.openStream (java.net.URL. "https://wrong.host.badssl.com"))) 
=> CertificateException No subject alternative DNS name matching wrong.host.badssl.com found.  sun.security.util.HostnameChecker.matchDNS (HostnameChecker.java:211)
(slurp (.openStream (java.net.URL. "https://self-signed.badssl.com" ))) 
=> SunCertPathBuilderException unable to find valid certification path to requested target  sun.security.provider.certpath.SunCertPathBuilder.build (SunCertPathBuilder.java:141)
(slurp (.openStream (java.net.URL. "https://untrusted-root.badssl.com"))) 
=> SunCertPathBuilderException unable to find valid certification path to requested target  sun.security.provider.certpath.SunCertPathBuilder.build (SunCertPathBuilder.java:141)

;; the rest 3 return successfully return 

Now compare using the default-client-https:

(def SSL_CONTEXT (SSLContext/getDefault)))

@(request {:client :default-https                    
           :ssl-context SSL_CONTEXT
           :keepalive -1
           :url "https://wrong.host.badssl.com" })
=>
{:opts {:client :default-https,
        :ssl-context #object[javax.net.ssl.SSLContext 0x72c88a66 "javax.net.ssl.SSLContext@72c88a66"],
        :keepalive -1,
        :url "https://wrong.host.badssl.com"},
 :error #error{:cause "No subject alternative DNS name matching wrong.host.badssl.com found.",
               :via [{:type javax.net.ssl.SSLHandshakeException,
                      :message "No subject alternative DNS name matching wrong.host.badssl.com found.",
                      :at [sun.security.ssl.Alert createSSLException "Alert.java" 131]}
                     {:type java.security.cert.CertificateException,
                      :message "No subject alternative DNS name matching wrong.host.badssl.com found.",
                      :at [sun.security.util.HostnameChecker matchDNS "HostnameChecker.java" 211]}]}

@(request {:client :default-https              
           :ssl-context SSL_CONTEXT
           :keepalive -1
           :url "https://self-signed.badssl.com"  })
=>
{:opts {:client :default-https,
        :ssl-context #object[javax.net.ssl.SSLContext 0x72c88a66 "javax.net.ssl.SSLContext@72c88a66"],
        :keepalive -1,
        :url "https://self-signed.badssl.com"},
 :error #error{:cause "unable to find valid certification path to requested target",
               :via [{:type javax.net.ssl.SSLHandshakeException,
                      :message "PKIX path building failed: sun.security.provider.certpath.SunCertPathBuilderException: unable to find valid certification path to requested target",
                      :at [sun.security.ssl.Alert createSSLException "Alert.java" 131]}
                     {:type sun.security.validator.ValidatorException,
                      :message "PKIX path building failed: sun.security.provider.certpath.SunCertPathBuilderException: unable to find valid certification path to requested target",
                      :at [sun.security.validator.PKIXValidator doBuild "PKIXValidator.java" 384]}
                     {:type sun.security.provider.certpath.SunCertPathBuilderException,
                      :message "unable to find valid certification path to requested target",
                      :at [sun.security.provider.certpath.SunCertPathBuilder build "SunCertPathBuilder.java" 141]}]}

@(request {:client :default-https                  
           :ssl-context SSL_CONTEXT
           :keepalive -1
           :url "https://untrusted-root.badssl.com" })
=>
{:opts {:client :default-https,
        :ssl-context #object[javax.net.ssl.SSLContext 0x72c88a66 "javax.net.ssl.SSLContext@72c88a66"],
        :keepalive -1,
        :url "https://untrusted-root.badssl.com"},
 :error #error{:cause "unable to find valid certification path to requested target",
               :via [{:type javax.net.ssl.SSLHandshakeException,
                      :message "PKIX path building failed: sun.security.provider.certpath.SunCertPathBuilderException: unable to find valid certification path to requested target",
                      :at [sun.security.ssl.Alert createSSLException "Alert.java" 131]}
                     {:type sun.security.validator.ValidatorException,
                      :message "PKIX path building failed: sun.security.provider.certpath.SunCertPathBuilderException: unable to find valid certification path to requested target",
                      :at [sun.security.validator.PKIXValidator doBuild "PKIXValidator.java" 384]}
                     {:type sun.security.provider.certpath.SunCertPathBuilderException,
                      :message "unable to find valid certification path to requested target",
                      :at [sun.security.provider.certpath.SunCertPathBuilder build "SunCertPathBuilder.java" 141]}]}

;; the rest 3 successfully return 

I'd say that's a great start from a consistency perspective. The remaining three don't work (in ether case) because we're using the default SSLContext (which is more or less equivalent to an SSLContext obtained with protocol TLS and initialised with the default truststore parameters).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants