Collect: Add support for SNI connections

[yanokwa]

"Hi Group: I've been working for a while now with Collect, using it in conjunction with custom back-end services running on non-Aggregate web hosts, with form downloading and uploading working over secure SSL connections. It all works nicely, but increasingly I'm finding that budget web hosting provides SSL connections using shared IP addresses, which may be accommodating multiple SSL certs. Browser and other app clients don't mind this because they use SNI (https://en.wikipedia.org/wiki/Server_Name_Indication), but Collect doesn't use SNI - it uses an older http connection class which doesn't support SNI. Without SNI, the server doesn't know which cert to present in response to SSL connection requests, and so Collect receives a cert which doesn't match the requested hostname - so the connection fails. Would the maintainers consider a RFC to update the http/s mechanism used in Collect to one which supports SNI? I understand this could be done using the HttpsURLConnection class - I'm not knowledgeable enough to implement it myself, but I think it would be a beneficial update. "

[mitchellsundt]

This should now be supported in ODK Collect 1.4.9 (needs verification)

[batkinson]

I wrote a test that confirms the odk fork of Apache HttpClient _does not_ support SNI as of the latest on master (017c258e6bc). Performing the same test using HttpUrlConnection was successful.

The stack trace shows that the failure manifests itself as a peer verification problem:

javax.net.ssl.SSLPeerUnverifiedException: Host name 'sni.velox.ch' does not match the certificate subject provided by the peer (CN=alice.sni.velox.ch, O=Kaspar Brand, L=Zuerich, ST=Zuerich, C=CH)
at org.opendatakit.httpclientandroidlib.conn.ssl.SSLConnectionSocketFactory.verifyHostname(SSLConnectionSocketFactory.java:465)
at org.opendatakit.httpclientandroidlib.conn.ssl.SSLConnectionSocketFactory.createLayeredSocket(SSLConnectionSocketFactory.java:395)
at org.opendatakit.httpclientandroidlib.conn.ssl.SSLConnectionSocketFactory.connectSocket(SSLConnectionSocketFactory.java:353)
at org.opendatakit.httpclientandroidlib.impl.conn.DefaultHttpClientConnectionOperator.connect(DefaultHttpClientConnectionOperator.java:141)
at org.opendatakit.httpclientandroidlib.impl.conn.PoolingHttpClientConnectionManager.connect(PoolingHttpClientConnectionManager.java:353)
at org.opendatakit.httpclientandroidlib.impl.execchain.MainClientExec.establishRoute(MainClientExec.java:380)
at org.opendatakit.httpclientandroidlib.impl.execchain.MainClientExec.execute(MainClientExec.java:236)
at org.opendatakit.httpclientandroidlib.impl.execchain.ProtocolExec.execute(ProtocolExec.java:184)
at org.opendatakit.httpclientandroidlib.impl.execchain.RetryExec.execute(RetryExec.java:88)
at org.opendatakit.httpclientandroidlib.impl.execchain.RedirectExec.execute(RedirectExec.java:110)
at org.opendatakit.httpclientandroidlib.impl.client.InternalHttpClient.doExecute(InternalHttpClient.java:184)
at org.opendatakit.httpclientandroidlib.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:82)
at org.opendatakit.httpclientandroidlib.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:107)
at org.opendatakit.httpclientandroidlib.impl.client.CloseableHttpClient.execute(CloseableHttpClient.java:1)
at org.odk.collect.android.SNITest.apacheHttpClientSupportsSNI(SNITest.java:41)

[nribeka]

Apparently SNI will only available if the underlying JRE supports it. I found this thread on the HTTPClient JIRA: HTTPCLIENT-1119.

The patch referenced in that issue is in the current output of this script: convert_stock_httpclient.

[nribeka]

I ran the same tests written by @batkinson and I get all green here.

[batkinson]

@nribeka I would not be overly surprised if results of these tests depend on Android version. HttpClient was recommended over UrlConnection for a bit, but now it's not supported. It might be possible to abstract away the differences and use a factory to use the client best for the platform version.

[mitchellsundt]

And, again, please note that ODK COLLECT IS NOT USING the HttpClient that ships with Android.

That is old, ancient, and should rightfully be discarded and disabled. I don't think anyone who has looked into what that version consists of would claim otherwise.

What versions of Android have you been testing on?

[batkinson]

@mitchellsundt thank you for the reminder, but I didn't forget what you said before. Did you look at the test? It is using the same client library that Collect is using on master: org.opendatakit.httpclientandroidlib.client.HttpClient.

No specific version of Android was noted in the issue description. However, I was able to reproduce this on the first phone tried, a Samsung SM-S820L running Android 4.4.4. I just tested it on three additional devices, a Samsung GT-P5210 and SM-T530 running Android 4.4.2 and an ASUS TF300T running Android 4.2.1 with the following results:

| Test | SM-S820L | GT-P5210 | SM-T530 | TF300T |
| --- | --- | --- | --- | --- |
| UrlConnection | ✔️ | ✔️ | ✔️ | ✔️ |
| Collect's HttpClient | ❌ | ❌ | ❌ | ❌ |

Considering I just tried what I had on-hand and was able to reproduce this tells me I am extremely unlucky or there's something to the report. I am interested in what you tested on @nribeka, especially if its the same (or similar) API level.

[batkinson]

I investigated this further and I may have an explanation.

Every device I ran the tests on are API 17 and greater. Looking at the description of the SNI fix from HTTPCLIENT-1591, it is version dependent:

// Android specific code to enable SNI
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN_MR1) {
    if (Log.isLoggable(TAG, Log.DEBUG)) {
        Log.d(TAG, "Enabling SNI for " + target);
    }
    try {
        // !!! for successful hostname verification, call setHostname() on socket !!!
        Method method = sslsock.getClass().getMethod("setHostname", String.class);
        method.invoke(sslsock, target);
    } catch (Exception ex) {
        if (Log.isLoggable(TAG, Log.DEBUG)) {
            Log.d(TAG, "SNI configuration failed", ex);
        }
    }
}

The fix relies on using reflection to call the method setHostname(...) on the socket, which is only available in Android version Build.VERSION_CODES.JELLY_BEAN_MR1 or later (API >= 17).

However, this does not explain why Collect's httpclient doesn't work. The issue: we're not using the right version of httpclient.

The project is using 4.5.2 so it should include the fix, but as the httpclient docs state, Android clients should either be using the android port for API<23 or Marek's Sebera's fork for API>=23.

I decompiled the SSLConnectionSocketFactory class used by Collect right now and there is no reference to the Android version codes or the reflection call to setHostname. That lead me to how we generated the httpclient. I looked at the source code of SSLConnectionSocketFactory used by the scripts and it appears it is the plain-old-java version. I compared that with the patch included in Marek's version and you can immediately see the difference: there is the fix as above.

I believe we just need to update to the proper version of the library.

[batkinson]

Because of the way the project currently manages this library, it isn't as simple as updating to one of the libraries the Apache HttpComponents doc recommended. I attempted to adapt the existing script to use one of these to build our local lib, but I quickly ran into problems due to available published versions and aborted that approach.

Instead, to confirm everything above I adapted the library generation script to apply a simple patch with an appropriate fix. After regenerating the library, updating Collect to use the generated output and re-running the tests, all tests are passing on device.

Any suggestion on how to best remedy this @mitchellsundt? I don't have enough context around the existing setup.

[mitchellsundt]

Where is your test?

[batkinson]

Sorry @mitchellsundt, I thought since @nribeka was able to run them it was obvious from the thread. The test for this issue is on my fork, but it's listed above because I referenced this issue. If there is a better way to do this as an outside contributor, just let me know.

[batkinson]

@mitchellsundt did you end up taking a peek at this? This definitely appears to be an issue, but given the way the project is maintaining things, I'm just not sure how we would prefer to handle the fix.

[yanokwa]

@lognaturel Can you take a quick look at this and offer your feedback. It's tied to #166.

[lognaturel]

@batkinson Ran your test on a Nexus 5 running Android 6.0.1 and get all green. I followed your reasoning and agree with your conclusions (especially given that you can get passing tests by updating the library generation script). You say you ran into issues with how the library is managed, do you know a better way? What would that change entail? Or alternately, what are the issues you see with patching the library generation script like you did locally to get your passing tests?

[batkinson]

Thank you @lognaturel for taking a look at this.

I am curious, did you confirm the test fails without the fix as well?

We probably should test the patched lib on pre-API 17 as well, since it always attempts to call setHostname(). I diverged from the fix bundled in the official client based on this suggestion to remove the version check. I am traveling, but if I can get enough network bandwidth to download a pre-17 image I will give it a try and update you.

As for how the library is managed, I guess my question was more about trying to understand the current setup enough to be surgical if it's essential we self-manage or work to move us to the official libs if not. If we want to get this issue fixed immediately, the easiest is to just do what I did. Do not try to solve the bigger issue and re-generate a library within the current system. Does that make sense? Let me know if I need to articulate this better.

[lognaturel]

I didn't apply a fix and was for some reason thinking all green was expected given the Android version. I read over what you wrote again and it looks like I got that wrong.

To recap - you got consistent failures on API-19 across several devices. I got success on API-23 but now I see that given your hypotheses, the expectation would have been for it to fail since the Marek patch is not applied. Am I now getting that right? @nribeka, it would be helpful to know more about your device and API level.

[batkinson]

@lognaturel so, my guess is that the difference in behavior may be related to the fact that Google re-implemented the TLS layer by replacing OpenSSL with BoringSSL.

[lognaturel]

Ahh, ok, so then failure is expected on API<23 and it's making sense again.

I don't think I saw a branch with your lib patch, could you put that somewhere? I should be able to do a trial with API<17 tomorrow so you don't need to. Doing the patch now and taking time to look at the broader lib question later sounds good to me. Could you please file an issue with some of the things you've learned about the lib setup and your ideas on what might need to be explored before transitioning to official libs?

[lognaturel]

@batkinson I can confirm that the test fails with the same error you reported here on the Nexus 5 running KitKat (API-19). Unfortunately it's the only device I have access to and I can't downgrade Android any further. I think you should issue a pull request for your lib patch whenever you have a chance. I can then verify that it fixes it on my device + API-19 and we can wait for someone/you when you have time to do the API-17 check before merging.

[batkinson]

Hi @lognaturel, thank you for confirming. I confirmed what you and @nribeka observed; the issue is not present on more recent android versions (I used Android 7). I suspect this is related to the SSL stack changes in Android 6.

I also was able to confirm the issue by running the test on a pre API-17 version of Android as well (using an API 16 Android version). After updating the library with my patch, the tests pass.

I will wrap it up and submit PRs.