Code Monkey home page Code Monkey logo

Comments (16)

jaypatrickhoward avatar jaypatrickhoward commented on July 20, 2024

I tried setting the SSLSocketFactory manually on each HttpsURLConnection, as opposed to just setting the system default, and it still didn't work. I'm pretty sure (but not certain) that this problem occurs even when loading locally cached content. Since I can't figure out a workaround I'm going to have to disable HttpResponseCache installation on Android versions prior to Gingerbread (2.3).

To test:

  1. Create the class LoggingSSLSocketFactory that extends SSLSocketFactory and delegates all calls to super, but logs each method call.
  2. Create an instance of LoggingSSLSocketFactory and set it as the system default by passing it to HttpsURLConnection.setDefaultSSLSocketFactory().
  3. Install HttpResponseCache.
  4. Open a connection to a "https" URL and try to read from it. You should not see any log statements from the LoggingSSLSocketFactory you set as the system default.

from httpresponsecache.

jaypatrickhoward avatar jaypatrickhoward commented on July 20, 2024

I spent some time debugging this today and my original description of the issue is not accurate.

The HttpResponseCache code does in fact honor the default SSLSocketFactory set on HttpsURLConnection.

What's happening is that HttpConnection.setupSecureSocket() (from HttpResponseCache) is failing to set advanced TLS options on the SSLSocket. This causes it to fall back to SSLv3, which I verified is supported by the underlying socket implementation. However, when startHandshake() is called, I see this in the log:

08-30 16:12:12.774: E/NativeCrypto(2918): Unknown error 1 during connect

So setupSecureSocket() must be configuring the SSLSocket incorrectly somehow, since this "works" when HttpResponseCache isn't installed.

A search for "Unknown error 1 during connect" turned up several Stack Overflow posts with a very similar error in relation to SSL calls, but no solutions or insight into what's causing it.

Somehow, however, this "works" when HttpResponseCache isn't in the picture. I will try to figure out how the Android 2.2 version of HttpConnection sets up its SSLSocket.

from httpresponsecache.

jaypatrickhoward avatar jaypatrickhoward commented on July 20, 2024

All the Android HttpConnection source code I can find online looks identical to the version in your library.

from httpresponsecache.

jaypatrickhoward avatar jaypatrickhoward commented on July 20, 2024

More info:

The SSLSocketFactory I set as the default is created from a SSLContext that was instantiated with "TLS" as the protocol, despite TLS apparently not being supported by the underlying SocketImpl class. I'm guessing this is the root of the problem. Still, no idea how or why it works when no HttpResponseCache is installed.

I'm going to try to modify my code to use the protocol of the default SSLContext (SSLContext.getDefault().getProtocol()) instead of using a hard-coded value of "TLS" and see if that fixes the issue.

from httpresponsecache.

jaypatrickhoward avatar jaypatrickhoward commented on July 20, 2024

Red herring. Getting a SSLContext instance via getInstance("TLS") is the correct way. The bug may simply be that setupSecureSocket() falls back to SSLv3 unnecessarily when the methods it wants to see on unverifiedSocket aren't there.

from httpresponsecache.

jaypatrickhoward avatar jaypatrickhoward commented on July 20, 2024

This is in fact the case. When I modify HttpConnection.setupSecureSocket() to not fall back to SSLv3 when the methods it wants to call (setEnabledCompressionMethods, setUseSessionTickets, setHostname) on the SSLSocket don't exist then everything seems to work fine.

from httpresponsecache.

candrews avatar candrews commented on July 20, 2024

@jaypatrickhoward I'm not sure I'm following your last comment - are you saying that you found a workaround? If so, could you submit a merge request? If not... could you explain? My apologies for my density!

from httpresponsecache.

jaypatrickhoward avatar jaypatrickhoward commented on July 20, 2024

I did find a work around, but I don't know enough about the SSL/TLS logic to know whether it's advisable. The change is to com.integralblue.httpresponsecache.compat.libcore.net.http.HttpConnection.setupSecureSocket(). On this particular device, the SSLSocket class returned by sslSocketFactory.createSocket() lacks the methods the TLS branch attempts to invoke via reflection (e.g. setEnabledCompressionMethods). This causes it to fall back to SSLv3, which then (for some reason) doesn't work on this device.

When I let those method invocations fail silently (without falling back to SSLv3) it seems to work.

This is probably a really dumb question, but are all those compat classes really necessary? I get that HttpResponseCache has to provide custom implementations of HttpURLConnectionImpl and HttpsURLConnectionImpl in order to perform the caching, but is it really necessary to provide custom versions of all the rest? For instance, if we were able to use the "stock" HttpConnection class then this problem might just disappear.

from httpresponsecache.

candrews avatar candrews commented on July 20, 2024

Unfortunately, they are necessary. There are a few reasons:
The stock classes aren't visible (application code cannot see/access them)
They don't exist, or have different features/bugs/methods for different versions of Android, so even if I could access them and use them, then applications would get different behaviors and features on different versions of Android, which is one of the problems I'd like to avoid by using this library
I think a neat feature of this library is that it doesn't have to be used on Android - it'll work on any JVM >= 5

from httpresponsecache.

jaypatrickhoward avatar jaypatrickhoward commented on July 20, 2024

Hmm. Sounds reasonable. I'm going to try to create a wrapper class that extends the underlying SSLSocket implementation. (Though if this class isn't visible then that's not going to work). My class will delegate each method call to its wrapped instance but will log each one. That way I'll be able to tell exactly how your compat code differs from the stock code with respect to how it configures SSLSockets before calling their startHandshake() method.

Another thought: I'm actually not certain this is a 2.2 issue per se. It may just be an issue with this specific device. Unfortunately I don't have any other 2.2 devices to test with. I do have a DeviceAnywhere account, though; I may get on there and see if I can find one.

from httpresponsecache.

jaypatrickhoward avatar jaypatrickhoward commented on July 20, 2024

The more I think about it, the more I think HttpConnection shouldn't automatically fall back to SSLv3 when those "advanced" TLS methods aren't present. Here's why. It has no way to know what the implementation of SSLSocket is that it gets back from the SSLSocketFactory. For instance, maybe the user set his own custom SSLSocketFactory that constructs custom SSLSocket implementations that lack those methods, but still support that functionality (e.g. compression). Maybe my custom SSLSocketImpl supports TLS but has has ZLIB compression "hard-coded" and so lacks that setter.

For that matter, I question whether HttpConnection should even do this kind of configuration even when it IS possible, since the caller may not want those values changed. For instance, maybe I set a custom SSLSocketFactory that configures the sockets it produces to use something other than ZLIB compression. The code in your HttpConnection class is going to overwrite my custom value with "ZLIB".

By the way, here's what I got from my logging wrapper classes (I could only overwrite the methods from SSLSocketFactory and SSLSocket, not those from the underlying implementations):

2.2 with cache installed:

08-31 08:39:07.122: E/LoggingSSLSocketFactoryImpl(5660): LoggingSSLSocketFactoryImpl(SSLSocketFactory delegate)
08-31 08:39:07.754: E/LoggingSSLSocketFactoryImpl(5660): createSocket(Socket arg0, String arg1, int arg2, boolean arg3)
08-31 08:39:07.862: E/LoggingSSLSocketImpl(5660): LoggingSSLSocketImpl(SSLSocket delegate)
08-31 08:39:07.872: E/LoggingSSLSocketImpl(5660): setEnabledProtocols(String[] arg0)
08-31 08:39:07.872: E/LoggingSSLSocketImpl(5660): startHandshake()

2.2 w/o cache installed:

08-31 08:42:50.402: E/LoggingSSLSocketFactoryImpl(5729): LoggingSSLSocketFactoryImpl(SSLSocketFactory delegate)
08-31 08:42:50.642: E/LoggingSSLSocketFactoryImpl(5729): createSocket(Socket arg0, String arg1, int arg2, boolean arg3)
08-31 08:42:50.712: E/LoggingSSLSocketImpl(5729): LoggingSSLSocketImpl(SSLSocket delegate)
08-31 08:42:50.712: E/LoggingSSLSocketImpl(5729): setUseClientMode(boolean arg0)
08-31 08:42:50.722: E/LoggingSSLSocketImpl(5729): startHandshake()

Since I'm using a client certificate, it may be the lack of setUseClientMode() that's causing the problem.

from httpresponsecache.

jaypatrickhoward avatar jaypatrickhoward commented on July 20, 2024

Manually setting UseClientMode = true in my custom SSLSocketFactory had no effect when the cache is installed. I still get the failure. So that's not the issue.

If you'll notice, the stock code never changes the enabled protocols. It asks the SSLSocketFactory for a SSLSocket and assumes that the factory has correctly set the enabled protocols. The only way it modifies the SSLSocket's configuration prior to calling startHandshake() is to call setUseClientMode().

I wonder whether the HttpConnection.setupSecureSocket() should just be:

public void setupSecureSocket(SSLSocketFactory sslSocketFactory, boolean tlsTolerant) throws IOException {
    unverifiedSocket = (SSLSocket) sslSocketFactory.createSocket(socket, address.uriHost, address.uriPort, true);
    unverifiedSocket.startHandshake();
}

Just assume the SSLSocketFactory performs all the configuration it wants to do in its createSocket() method.

from httpresponsecache.

jaypatrickhoward avatar jaypatrickhoward commented on July 20, 2024

Actually that causes problems. This doesn't:

public void setupSecureSocket(SSLSocketFactory sslSocketFactory, boolean tlsTolerant) throws IOException {
    unverifiedSocket = (SSLSocket) sslSocketFactory.createSocket(socket, address.uriHost, address.uriPort, true);
    if (tlsTolerant) {
        // do nothing
    } else {
        unverifiedSocket.setEnabledProtocols(new String [] { "SSLv3" });
    }
    unverifiedSocket.startHandshake();
}

from httpresponsecache.

jaypatrickhoward avatar jaypatrickhoward commented on July 20, 2024

Alternately, if you want to keep the previous functionality in place when those methods exist, just surround each method invocation in its own try/catch block and swallow the exception if the method doesn't exist.

from httpresponsecache.

candrews avatar candrews commented on July 20, 2024

I think I follow what you're saying - can you submit a merge request against head?

3 methods are called on the SSLSocket: setEnabledCompressionMethods, setUseSessionTickets, setHostname. So at least one of those is missing on <= 2.2... do you know which ones?

from httpresponsecache.

jaypatrickhoward avatar jaypatrickhoward commented on July 20, 2024

All the methods are missing on this device. The phone runs Android 2.2, but I'm not sure whether it's an OS issue or a device issue. There may be other devices running 2.2 that have support for advanced TLS configuration. Either way it probably deserves a fix. I see two options:

  1. Remove those method calls altogether (i.e. the "do nothing" version I posted above), or
  2. Call the methods only if they're available, otherwise fail silently instead of falling back to SSLv3.

Option #2 has the benefit of retaining "current behavior" on devices where these methods are present. What I wonder about is whether "current behavior" is actually "good behavior". I say that because it's somewhat unintuitive that the act of installing HttpResponseCache should override my SSL settings. Consider the situation where I set a default SSLSocketFactory that produces SSLSockets with ZLIB compression disabled. The HttpResponseCache code as it currently stands would re-enable ZLIB compression for every SSL connection it makes, despite my attempts to turn it off.

Let me know which option you prefer and I'll try to merge in a change. (I'm not familiar with Git and/or Github, so I'll have to figure out how to do that.)

from httpresponsecache.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.