Required For HTTPS URLSession requests to work on Android

[johnno1962]

Separating out the nub of PR #1113 into a separate PR as required for HTTPS to work on Android. We can look at the precise code which is best for the Android port separately

[johnno1962]

@alblue, I added the removal of the scripts in the “android” directory which I originally wrote to this PR as they are now hopelessly out of date compared to the far better swifty-robot scripts

[alblue]

@johnno1962 the problem with having unrelated changes in the same pull request is that they end up unnecessarily causing (potential) conflicts with, for example, #1249.

It's also not great having numerical titles for patches; can we be a bit more descriptive as to what the change is actually for? In this case, it might be something like "Change the option for the URLSessionCertificateAuthorityInfoFile to use UNSAFE_SSL_NOVERIFY to ignore peer validation" or some such. That's going to be a lot more useful in the log than Change III.

[johnno1962]

There is no clash with #1249, that’s why I’m breaking this out into a separate PR. The deleting files part of this PR I’d like to remain in thanks. What is left of #1113 will clash however and I’m likely to walk away from that until I next port to Android

[ianpartridge]

I agree with @alblue that mixing unrelated changes into one PR makes it difficult for us non-Android people to quickly merge your PRs. Having said that, this PR deletes a whole bunch of stuff and only affects Android code, so 👍

[ianpartridge]

@swift-ci please test and merge

[alblue]

@johnno1962 @ianpartridge I would have preferred this not to be merged as is. Changing the commit message is not the same as changing the title of the pull request on GitHub.

Commit messages should be descriptive about why the change was introduced; now we will have a commit in the swift-corelibs-foundation history that says 'For HTTPS on Android III' and 'Merge branch 'master' into HTTPS4Android', neither of which are descriptive or relevant to the log.

I am especially concerned that the 'Merge branch master into ...' type commits - please ensure that the commit is rebased rather than merged. This just adds noise to the log and adds no relevant information. Please ensure you configure your swift-corelibs-foundation repository with 'git config pull.rebase true' to avoid this in future.

[ianpartridge]

Ah, fair points. Thanks @alblue - I'll bear this in mind in future.

[johnno1962]

Sorry, I hadn’t realised it was the commit message you were objecting to and there was a merge commit too - I should have rebased. Anyway, just happy to see this merged at last. Thanks!

[ephemer]

Thanks for making this change, I was able to get SSL working on Android today using this env variable. Is this really the nicest way about this though? The fact that it's alterable like this is kind of confusing as an API consumer, plus it looks a lot like a security risk to me.

https://curl.haxx.se/docs/sslcerts.html details the option of building libcurl with the flag --with-ca-bundle=FILE. So from what I understand, we could remove this code complexity entirely by just building libcurl with the certificate mentioned in your comment here. That would also save the need to bundle this certificate file in the APK (another potential security risk).

What do you think @johnno1962 ?

[johnno1962]

Hi @ephemer, there was a lot of to-ing and fro-ing on the PR. The original PR #1103 which was merged was an API which was developed in #1113 until it was decided we couldn't add api to foundation and we switched back to the original proposal. As far as one can know I don’t think it’s a security issue as if you use it it will be set explicitly in the code and there will be no opportunity to root the device and override it by setting the var maliciously for example. I wasn’t aware you could hard code it into libcurl but that doesn’t help as it wouldn't have a fixed path on the menagerie that is android.

If I were to do it all again and known #1113 wouldn’t be merged for three months. I would have left it as it was after #1103 but by then the toolchain was committed to an env var.

[ephemer]

Hi @johnno1962, if I understand correctly, this is essentially a process-wide public variable that can be changed at any time before any URLSession is created. That certainly seems like a security risk to me. I don't know enough about SSL (or the finer details of Android for that matter) to know to what extent this may be risky, but it'd be hard to argue that it's secure. That is, unless you're calling setenv religiously before every initialization of URLSession, but that seems like an unreasonable expectation to have of Foundation's API consumers.

From what I understand, implementing --with-ca-bundle wouldn't mean hard-coding a file path into libcurl, but rather incorporating the certificate bundle into libcurl.so statically. If that's not the case I agree it'd be pointless though. I'll do some more testing on this tomorrow. If a static solution does turn out to be possible, would you be ok with this code being removed?

EDIT: If statically including the file doesn't work (and it does search a device-specific path after all), what about telling it to search in /system/etc/security/cacerts/ via --with-ca-path=?

[johnno1962]

Having fought hard to get this small change in I’d rather it wasn’t rolled back! Besides, I don’t see the alternative though I’m open to practical alternatives. You can hard code the path to the certificates in libcurl if there was a standard one but the problem is they are in the wrong format on most android devices ass libssl1.0+ moved from a MD5 hash as the filename to to a SHA1 http://www.javacms.tech/questions/1728116/how-to-make-ssl-peer-verify-work-on-android. Including the certificate bundle in libcurl i.e. the toolchain seems like a very bad idea to me.

Whether this presents a security risk, not being an expert this is something I’d rather not express an opinion on but I doubt it. If you can’t rely on an apps code bundle all bets are probably off anyway. In this case, using an environment variable vs. an api doesn’t make much difference. If you find a better way though then all power to your elbow.

[ephemer]

Hi @johnno1962, thanks for your patience. I've just spent a couple of hours reading into this and have come to the conclusion that you may well have the only reasonable solution that is usable for this. I am still sceptical that it's entirely secure but now equally as sceptical that there's any other way on Android.

Do you have an idea of how best to use this in an APK? libcurl expects an absolute path.

[johnno1962]

Hi @ephemer, checkout https://github.com/SwiftJava/swift-android-kotlin/blob/master/app/src/main/java/com/example/user/myapplication/MainActivity.kt#L48 & https://github.com/SwiftJava/swift-android-kotlin/blob/master/app/src/main/swift/Sources/main.swift#L60 for examples on how it all comes together. You need to have the cacert file as a resource, copy it to the app’s cacheDir and set the environment variable to that path in Swift. There just didn’t seem to be a better way to do it.

[ephemer]

Thanks for the info. Ok, so you're basically copying the .pem from the APK's resources into the app cache (which has an absolute path), accessed via the applicationContext. It's a pain in the butt but I guess there's not much else we could do other than loading the .pem ourselves and adding the certs manually.

Maybe it's worth us putting in an issue to the NDK asking them to provide a libcurl (with SSL, using the system certificates)? It might take a few years, but hey..