Skip to content

For https: to work on Android #1103

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jul 10, 2017
Merged

For https: to work on Android #1103

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
674fc16
For https: to work on Android
johnno1962 Jul 9, 2017
0c3a45f
For debug logging on Android
johnno1962 Jul 9, 2017
712f270
UNSAFE_SSL_NOVERIFY
johnno1962 Jul 9, 2017
9655700
Use setter rather than environment
johnno1962 Jul 10, 2017
5890353
Use setter rather than environment - lint
johnno1962 Jul 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 31 additions & 1 deletion Foundation/NSURLSession/http/EasyHandle.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,9 @@ internal final class _EasyHandle {
fileprivate var pauseState: _PauseState = []
internal var fileLength: Int64 = 0
internal var timeoutTimer: _TimeoutSource!
#if os(Android)
static fileprivate var _CAInfoFile: UnsafeMutablePointer<Int8>?
#endif

init(delegate: _EasyHandleDelegate) {
self.delegate = delegate
Expand Down Expand Up @@ -168,6 +171,20 @@ extension _EasyHandle {
let protocols = (CFURLSessionProtocolHTTP | CFURLSessionProtocolHTTPS)
try! CFURLSession_easy_setopt_long(rawHandle, CFURLSessionOptionPROTOCOLS, protocols).asError()
try! CFURLSession_easy_setopt_long(rawHandle, CFURLSessionOptionREDIR_PROTOCOLS, protocols).asError()
#if os(Android)
// See https://curl.haxx.se/docs/sslcerts.html
// For SSL to work you need "cacert.pem" to be accessable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: "accessible"

// at the path pointed to by the URLSessionCAInfo env var.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment needs to be updated.

Copy link
Contributor Author

@johnno1962 johnno1962 Jul 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@johnno1962 johnno1962 Jul 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on URLSession.setCAInfo(filePath: path)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@johnno1962 johnno1962 Jul 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@johnno1962 johnno1962 Jul 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// Downloadable here: https://curl.haxx.se/ca/cacert.pem
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
//TODO: Added in libcurl 7.45.0
//TODO: Set default protocol for schemeless URLs
//CURLOPT_DEFAULT_PROTOCOL available only in libcurl 7.45.0
Expand Down Expand Up @@ -268,7 +285,7 @@ fileprivate func printLibcurlDebug(handle: CFURLSessionEasyHandle, type: CInt, d

fileprivate func printLibcurlDebug(type: CFURLSessionInfo, data: String, task: URLSessionTask) {
// libcurl sends is data with trailing CRLF which inserts lots of newlines into our output.
print("[\(task.taskIdentifier)] \(type.debugHeader) \(data.mapControlToPictures)")
NSLog("[\(task.taskIdentifier)] \(type.debugHeader) \(data.mapControlToPictures)")
}

fileprivate extension String {
Expand Down Expand Up @@ -615,6 +632,19 @@ extension _EasyHandle._CurlStringList {
}
}

#if os(Android)
extension URLSession {

public static func setCAInfoFile(_ _CAInfoFile: String) {
free(_EasyHandle._CAInfoFile)
_CAInfoFile.withCString {
_EasyHandle._CAInfoFile = strdup($0)
}
}

}
#endif

extension CFURLSessionEasyCode : Equatable {
public static func ==(lhs: CFURLSessionEasyCode, rhs: CFURLSessionEasyCode) -> Bool {
return lhs.value == rhs.value
Expand Down