For https: to work on Android

[johnno1962]

Hi, just wondering whether you would consider merging this change to allow https: fetches using NSURLSession to work on Android. It turns out there is a root certificate authorities file required by libcurl for SSL to work.

This PR gives the user the opportunity to provide one and specify it’s location in the environment variable “URLSessionCAInfo” for it to be configured correctly. As providing an absolute path to anything is spectacularly difficult in Android or if the user’s servers are self-signed it also gives the user the option to specify the value “UNSAFE_SSL_NOVERIFY” and SSL hostname verification will not be performed at all.

The advantage of an environment variable over increasing the API surface is that the value can come all the way from the Java side of the application where it is most likely to know the path.

How does this sound?

[pushkarnk]

The change is acceptable to me. However, though the change is additive in nature, we seem to have introduced a new "non-debug" environment variable here. So, this can be considered to be changing the external behaviour of Foundation on Android.

@parkera @phausler Is there a general guidance around introducing new env variables? Where do we document these?

[pushkarnk]

ProcessInfo.processInfo.environment is another way (Swifty) to access env vars, but I am not sure if it offers benefits in this context.

[phausler]

umm isn't this a security hole? If someone sets URLSessionCAInfo then they can circumvent the root certificate authorities? Thusly making all traffic vulnerable to being spoofed for certificates.

[johnno1962]

Without providing a URLSessionCAInfo file SSL doesn’t work at all using libcurl on Android. It doesn’t seem to have an idea about where the android root CA certs are stored and how to use them.

[phausler]

is there another way we can obtain the path other than env? My worry is that is a global state of the operating system that is publicly writable; so it could easily be hijacked by a malicious actor and circumvent root certificates.

I would presume that most android projects would be an APK that would load the libraries; can we add a JNI call or something like that so that once libFoundation.so is loaded the call is made to assign the appropriate path (as per the trust of the application).

Alternatively if there was some way we could grab that from the OS that would be preferable. Since I would assume that the cacert.pem would be stored as read only unless you are root.

[johnno1962]

Fair enough, It does bear all the hallmarks of an attack vector of some sort. Would a static public setter (Android only) on URLSession be OK? Any preference for the name? Grabbing it from the OS would be a bit of a rewrite somewhere in libcurl which I don’t fancy taking on. Android does not store Root CA’s in .pem format as far as I can see: https://stackoverflow.com/questions/4461360/how-to-install-trusted-ca-certificate-on-android-device

[johnno1962]

New version updated with setter i.e. URLSession.setCAInfoFile( "/mnt/sdcard/cacert.pem" )

[pushkarnk]

@swift-ci please test

[danilaplee]

definitely need this one for my swiftjava android project :)

[phausler]

this seems much less vulnerable to me; thanks

[johnno1962]

I agree

[phausler]

I am going to trust the android side works as expected (since we don't have a CI hook for that)

@swift-ci please test and merge

[xwu]

This comment needs to be updated.

[johnno1962]

Thanks again @xwu, anything else? Naming conventions etc? I’ll raise another PR.

        #if os(Android)
            // See https://curl.haxx.se/docs/sslcerts.html
            // For SSL to work you need a "cacert.pem" to be accessible
            // at the path set by URLSession.setCAInfoFile( <string> ).
            // Downloadable here: https://curl.haxx.se/docs/caextract.html
            if let caInfo = _EasyHandle._CAInfoFile  {
                if String(cString: caInfo) == "UNSAFE_SSL_NOVERIFY" {
                    try! CFURLSession_easy_setopt_int(rawHandle, CFURLSessionOptionSSL_VERIFYPEER, 0).asError()
                }
                else {
                    try! CFURLSession_easy_setopt_ptr(rawHandle, CFURLSessionOptionCAINFO, caInfo).asError()
                }
            }
        #endif

[xwu]

Since you ask... :P

• These should probably be named _CAInfoFilePath and setCAInfoFilePath, as it's not the file but the file path (versus, say, the file URL).
• Style nit: unsure why you're indenting this block between #if and #endif but not any of the others you added.
• Style nit: why have a blank line here and before the closing brace?
• Nit: if you're going to log instead of print from printLibcurlDebug, shouldn't it be logLibcurlDebug?

[johnno1962]

Roger, not much I can do about the last one. There is no stdout on Android. Can you check johnno1962b@d3805cb please and I’ll raise a new PR tomorrow. Thanks again! I updated the comment.

[johnno1962]

Thoughts on URLSession.setCAInfo(filePath: path)?

[phausler]

If you want to follow the naming conventions put forth for Foundation API;

1. if you have a setter you should have a getter (unless it is absolutely justifiable that it can only be set)
2. it should be a URL not a path
3. don't use abbreviations like CA, instead it should be probably named URLSession.defaultCertificateAuthorityFile or something along those lines so that it does what it says on the tin.
4. it should probably validate that the url is a valid file path url, and perhaps it should be nullable?
5. it should probably also be atomic (using some sort of lock to ensure thread safe access)

[xwu]

@johnno1962 👍 , but for the full monty Philippe certainly lists some important considerations.

[johnno1962]

URLSession.sslCertificateAuthorityFile as a URL static var with validation enough? I’m not sure I can stretch to atomicity. I’m thinking I preferred the old API tho. I like URLSession .setCertificateAuthorityInfo(filePath: “UNSAFE_SSL_NOVERIFY”) from a health warning point of view.

#if os(Android)
extension URLSession {
    public static func setCertificateAuthorityInfo(filePath: String) {
        free(_EasyHandle._CAInfoFilePath)
        filePath.withCString {
            _EasyHandle._CAInfoFilePath = strdup($0)
        }
    }
    public static var sslCertificateAuthorityFile: URL? {
        set(value) {
            if value == nil {
                free(_EasyHandle._CAInfoFilePath)
                _EasyHandle._CAInfoFilePath = strdup("UNSAFE_SSL_NOVERIFY")
                return
            }
            guard value?.isFileURL == true else { return }
            value!.path.withCString {
                free(_EasyHandle._CAInfoFilePath)
                _EasyHandle._CAInfoFilePath = strdup($0)
            }
        }
        get {
            return URL(fileURLWithPath: String(cString: _EasyHandle._CAInfoFilePath!))
        }
    }
}
#endif

[xwu]

Validation should check that it's a file URL that actually exists. When that fails, the setter should do something other than return silently. There should be one API, not two, so sslCertificateAuthorityFile should be the only way to set the underlying stored property. And the underlying property should probably be renamed to match.

[johnno1962]

OK, a version with no more dummy values left over from being an env var..

#if os(Android)
extension URLSession {
    /// See https://curl.haxx.se/docs/sslcerts.html
    /// For SSL to work you need a "cacert.pem" to be accessible at the
    /// by setting URLSession.sslCertificateAuthorityFile to a file URL.
    /// Downloadable here: https://curl.haxx.se/docs/caextract.html
    /// Alternatively you can bypass SSL peer verification altogether:
    /// URLSession.unsafeBypassSSLPeerVerify = true (not advised)
    fileprivate static var _sslCertificateAuthorityFile: UnsafeMutablePointer<Int8>?
    public static var unsafeBypassSSLPeerVerify = false
    public static var sslCertificateAuthorityFile: URL {
        set(value) {
            unsafeBypassSSLPeerVerify = false
            free(_sslCertificateAuthorityFile)
            guard value.isFileURL && FileManager.default.fileExists(atPath: value.path) else {
                NSLog("*** sslCertificateAuthorityFile not FileURL or does not exist ***")
                _sslCertificateAuthorityFile = nil
                return
            }
            value.path.withCString {
                _sslCertificateAuthorityFile = strdup($0)
            }
        }
        get {
            if let value = _sslCertificateAuthorityFile {
                return URL(fileURLWithPath: String(cString: value))
            }
            else {
                return URL(fileURLWithPath: "NOVALUE")
            }
        }
    }
}
#endif

[xwu]

Typo: "accessible"

[johnno1962]

If there are no further comments I’m ready to raise a PR based on https://github.com/johnno1962b/swift-corelibs-foundation/commits/master including a few other minor patches for compilation on Android