Provide better certificate verification callback (#171)

Motivation:

We've exposed the OpenSSL certificate verify callback for a while now.
Unfortunately, this callback is fairly hostile to callers, as it is
called repeatedly through the certificate verification process, instead
of presenting the unified certificate chain to the user.

BoringSSL has a better verification callback available that we use
internally, but that is not exposed to users. It would be nice to give
users the better API.

Modifications:

- Expose the BoringSSL verification callback to users.
- Re-plumb the Security.framework verification to use our public API.

Result:

Easier certificate chain verification overrides.

Sources/NIOSSL/NIOSSLClientHandler.swift

@@ -31,6 +31,11 @@ private extension String {
 /// This handler can be used in channels that are acting as the client
 /// in the TLS dialog. For server connections, use the `NIOSSLServerHandler`.
 public final class NIOSSLClientHandler: NIOSSLHandler {
+    public convenience init(context: NIOSSLContext, serverHostname: String?) throws {
+        try self.init(context: context, serverHostname: serverHostname, optionalCustomVerificationCallback: nil)
+    }
+
+    @available(*, deprecated, renamed: "init(context:serverHostname:customVerificationCallback:)")
     public init(context: NIOSSLContext, serverHostname: String?, verificationCallback: NIOSSLVerificationCallback? = nil) throws {
         guard let connection = context.createConnection() else {
             throw NIOSSLError.unableToAllocateBoringSSLObject
@@ -39,7 +44,7 @@ public final class NIOSSLClientHandler: NIOSSLHandler {
         connection.setConnectState()
         if let serverHostname = serverHostname {
             if serverHostname.isIPAddress() {
-                throw BoringSSLError.invalidSNIName([])
+                throw NIOSSLExtraError.cannotUseIPAddressInSNI(ipAddress: serverHostname)
             }
 
             // IP addresses must not be provided in the SNI extension, so filter them.
@@ -52,4 +57,32 @@ public final class NIOSSLClientHandler: NIOSSLHandler {
 
         super.init(connection: connection, shutdownTimeout: context.configuration.shutdownTimeout)
     }
+
+
+    public convenience init(context: NIOSSLContext, serverHostname: String?, customVerificationCallback: @escaping NIOSSLCustomVerificationCallback) throws {
+        try self.init(context: context, serverHostname: serverHostname, optionalCustomVerificationCallback: customVerificationCallback)
+    }
+
+    // This exists to handle the explosion of initializers we got when I tried to deprecate the first one. At least they all pass through one path now.
+    private init(context: NIOSSLContext, serverHostname: String?, optionalCustomVerificationCallback: NIOSSLCustomVerificationCallback?) throws {
+        guard let connection = context.createConnection() else {
+            throw NIOSSLError.unableToAllocateBoringSSLObject
+        }
+
+        connection.setConnectState()
+        if let serverHostname = serverHostname {
+            if serverHostname.isIPAddress() {
+                throw NIOSSLExtraError.cannotUseIPAddressInSNI(ipAddress: serverHostname)
+            }
+
+            // IP addresses must not be provided in the SNI extension, so filter them.
+            try connection.setServerName(name: serverHostname)
+        }
+
+        if let verificationCallback = optionalCustomVerificationCallback {
+            connection.setCustomVerificationCallback(CustomVerifyManager(callback: verificationCallback))
+        }
+
+        super.init(connection: connection, shutdownTimeout: context.configuration.shutdownTimeout)
+    }
 }

Sources/NIOSSL/SSLCallbacks.swift

@@ -58,9 +58,34 @@ public enum NIOSSLVerificationResult {
 /// or to store the `NIOSSLCertificate` somewhere for later consumption. The easiest way to be sure that the
 /// `NIOSSLCertificate` is safe to consume is to wait for a user event that shows the handshake as completed,
 /// or for channelInactive.
+///
+/// warning: This callback uses the old-style OpenSSL callback behaviour and is excessively complex to program with.
+///    Instead, prefer using the NIOSSLCustomVerificationCallback style which receives the entire trust chain at once,
+///    and also supports asynchronous certificate verification.
 public typealias NIOSSLVerificationCallback = (NIOSSLVerificationResult, NIOSSLCertificate) -> NIOSSLVerificationResult
 
 
+/// A custom verification callback that allows completely overriding the certificate verification logic of BoringSSL.
+///
+/// This verification callback is called no more than once per connection attempt. It is invoked with two arguments:
+///
+/// 1. The certificate chain presented by the peer, in the order the peer presented them (with the first certificate
+///     being the leaf certificate presented by the peer).
+/// 2. An `EventLoopPromise` that must be completed to signal the result of the verification.
+///
+/// Please be cautious with calling out from this method. This method is always invoked on the event loop,
+/// so you must not block or wait. However, you may perform asynchronous work by leaving the event loop context:
+/// when the verification is complete you must complete the provided `EventLoopPromise`.
+///
+/// This method must take care to ensure that it does not cause any `ChannelHandler` to recursively call back into
+/// the `NIOSSLHandler` that triggered it, as making re-entrant calls into BoringSSL is not supported by SwiftNIO and
+/// leads to undefined behaviour. It is acceptable to leave the event loop context and then call into the `NIOSSLHandler`,
+/// as this will not be re-entrant.
+///
+/// Note that setting this callback will override _all_ verification logic that BoringSSL provides.
+public typealias NIOSSLCustomVerificationCallback = ([NIOSSLCertificate], EventLoopPromise<NIOSSLVerificationResult>) -> Void
+
+
 /// A callback that can be used to implement `SSLKEYLOGFILE` support.
 ///
 /// Wireshark can decrypt packet captures that contain encrypted TLS connections if they have access to the
@@ -108,3 +133,117 @@ extension KeyLogCallbackManager {
         self.callback(self.scratchBuffer)
     }
 }
+
+
+/// A struct that provides helpers for working with a NIOSSLCustomVerificationCallback.
+internal struct CustomVerifyManager {
+    private var callback: CallbackType
+
+    private var result: PendingResult = .notStarted
+
+    init(callback: @escaping NIOSSLCustomVerificationCallback) {
+        self.callback = .public(callback)
+    }
+
+    init(callback: @escaping InternalCallback) {
+        self.callback = .internal(callback)
+    }
+}
+
+
+extension CustomVerifyManager {
+    private enum PendingResult: Hashable {
+        case notStarted
+
+        case pendingResult
+
+        case complete(NIOSSLVerificationResult)
+    }
+}
+
+
+extension CustomVerifyManager {
+    mutating func process(on connection: SSLConnection) -> ssl_verify_result_t {
+        // First, check if we have a result.
+        switch self.result {
+        case .complete(.certificateVerified):
+            return ssl_verify_ok
+        case .complete(.failed):
+            return ssl_verify_invalid
+        case .pendingResult:
+            // Ask me again.
+            return ssl_verify_retry
+        case .notStarted:
+            // The rest of this method handles this case.
+            break
+        }
+
+        self.result = .pendingResult
+
+        // Ok, no result. We need a promise for the user to use to supply a result.
+        guard let eventLoop = connection.eventLoop else {
+            // No event loop. We cannot possibly be negotiating here.
+            preconditionFailure("No event loop present")
+        }
+
+        let promise = eventLoop.makePromise(of: NIOSSLVerificationResult.self)
+
+        // We need to attach our "do the thing" callback. This will always invoke the "ask me again" API, and it will do so in a separate
+        // event loop tick to avoid awkward re-entrancy with this method.
+        promise.futureResult.whenComplete { result in
+            // When we complete here we need to set our result state, and then ask to respin certificate verification.
+            // If we can't respin verification because we've dropped the parent handler, that's fine, no harm no foul.
+            // For that reason, we tolerate both the verify manager and the parent handler being nil.
+            eventLoop.execute {
+                // Note that we don't close over self here: that's to deal with the fact that this is a struct, and we don't want to
+                // escape the mutable ownership of self.
+                precondition(connection.customVerificationManager == nil || connection.customVerificationManager?.result == .some(.pendingResult))
+                connection.customVerificationManager?.result = .complete(NIOSSLVerificationResult(result))
+                connection.parentHandler?.asynchronousCertificateVerificationComplete()
+            }
+        }
+
+        // Ok, let's do it.
+        self.callback.invoke(on: connection, promise: promise)
+        return ssl_verify_retry
+    }
+}
+
+
+extension CustomVerifyManager {
+    private enum CallbackType {
+        case `public`(NIOSSLCustomVerificationCallback)
+        case `internal`(InternalCallback)
+
+        /// For user-supplied callbacks we need to give them public types. For internal ones, we just pass the
+        /// `EventLoopPromise` object through.
+        func invoke(on connection: SSLConnection, promise: EventLoopPromise<NIOSSLVerificationResult>) {
+            switch self {
+            case .public(let publicCallback):
+                do {
+                    let certificates = try connection.peerCertificateChain()
+                    publicCallback(certificates, promise)
+                } catch {
+                    promise.fail(error)
+                }
+
+            case .internal(let internalCallback):
+                internalCallback(promise)
+            }
+        }
+    }
+
+    internal typealias InternalCallback = (EventLoopPromise<NIOSSLVerificationResult>) -> Void
+}
+
+
+extension NIOSSLVerificationResult {
+    init(_ result: Result<NIOSSLVerificationResult, Error>) {
+        switch result {
+        case .success(let s):
+            self = s
+        case .failure:
+            self = .failed
+        }
+    }
+}

Sources/NIOSSL/SSLCertificate.swift

@@ -47,7 +47,7 @@ public class NIOSSLCertificate {
         case ipv6(in6_addr)
     }
 
-    private init(withReference ref: UnsafeMutablePointer<X509>) {
+    private init(withOwnedReference ref: UnsafeMutablePointer<X509>) {
         self._ref = UnsafeMutableRawPointer(ref) // erasing the type for @_implementationOnly import CNIOBoringSSL
     }
 
@@ -73,7 +73,7 @@ public class NIOSSLCertificate {
             throw NIOSSLError.failedToLoadCertificate
         }
 
-        self.init(withReference: x509!)
+        self.init(withOwnedReference: x509!)
     }
 
     /// Create a NIOSSLCertificate from a buffer of bytes in either PEM or
@@ -107,7 +107,36 @@ public class NIOSSLCertificate {
             throw NIOSSLError.failedToLoadCertificate
         }
 
-        self.init(withReference: ref!)
+        self.init(withOwnedReference: ref!)
+    }
+
+    /// Create a NIOSSLCertificate from a buffer of bytes in either PEM or DER format.
+    internal convenience init(bytes ptr: UnsafeRawBufferPointer, format: NIOSSLSerializationFormats) throws {
+        // TODO(cory):
+        // The body of this method is exactly identical to the initializer above, except for the "withUnsafeBytes" call.
+        // ContiguousBytes would have been the lowest effort way to reduce this duplication, but we can't use it without
+        // bringing Foundation in. Probably we should use Sequence where Element == UInt8 and the withUnsafeContiguousBytesIfAvailable
+        // method, but that's a much more substantial refactor. Let's do it later.
+        let bio = CNIOBoringSSL_BIO_new_mem_buf(ptr.baseAddress, CInt(ptr.count))!
+
+        defer {
+            CNIOBoringSSL_BIO_free(bio)
+        }
+
+        let ref: UnsafeMutablePointer<X509>?
+
+        switch format {
+        case .pem:
+            ref = CNIOBoringSSL_PEM_read_bio_X509(bio, nil, nil, nil)
+        case .der:
+            ref = CNIOBoringSSL_d2i_X509_bio(bio, nil)
+        }
+
+        if ref == nil {
+            throw NIOSSLError.failedToLoadCertificate
+        }
+
+        self.init(withOwnedReference: ref!)
     }
 
     /// Create a NIOSSLCertificate wrapping a pointer into BoringSSL.
@@ -121,7 +150,7 @@ public class NIOSSLCertificate {
     /// In general, however, this function should be avoided in favour of one of the convenience
     /// initializers, which ensure that the lifetime of the `X509` object is better-managed.
     static func fromUnsafePointer(takingOwnership pointer: UnsafeMutablePointer<X509>) -> NIOSSLCertificate {
-        return NIOSSLCertificate(withReference: pointer)
+        return NIOSSLCertificate(withOwnedReference: pointer)
     }
 
     /// Get a sequence of the alternative names in the certificate.
@@ -267,10 +296,10 @@ extension NIOSSLCertificate {
             throw NIOSSLError.failedToLoadCertificate
         }
 
-        var certificates = [NIOSSLCertificate(withReference: x509)]
+        var certificates = [NIOSSLCertificate(withOwnedReference: x509)]
 
         while let x = CNIOBoringSSL_PEM_read_bio_X509(bio, nil, nil, nil) {
-            certificates.append(.init(withReference: x))
+            certificates.append(.init(withOwnedReference: x))
         }
 
         let err = CNIOBoringSSL_ERR_peek_error()

Sources/NIOSSL/SSLConnection.swift

@@ -48,13 +48,15 @@ internal final class SSLConnection {
     private let ssl: OpaquePointer
     private let parentContext: NIOSSLContext
     private var bio: ByteBufferBIO?
-    private var verificationCallback: NIOSSLVerificationCallback?
-    internal var platformVerificationState: PlatformVerificationState = PlatformVerificationState()
     internal var expectedHostname: String?
     internal var role: ConnectionRole?
     internal var parentHandler: NIOSSLHandler?
     internal var eventLoop: EventLoop?
 
+    /// Deprecated in favour of customVerificationManager
+    private var verificationCallback: NIOSSLVerificationCallback?
+    internal var customVerificationManager: CustomVerifyManager?
+
     /// Whether certificate hostnames should be validated.
     var validateHostnames: Bool {
         if case .fullVerification = parentContext.configuration.certificateVerification {
@@ -116,7 +118,9 @@ internal final class SSLConnection {
         self.expectedHostname = name
     }
 
-    /// Sets the BoringSSL verification callback.
+    /// Sets the BoringSSL old-style verification callback.
+    ///
+    /// This is deprecated in favour of the new-style verification callback in SSLContext.
     func setVerificationCallback(_ callback: @escaping NIOSSLVerificationCallback) {
         // Store the verification callback. We need to do this to keep it alive throughout the connection.
         // We'll drop this when we're told that it's no longer needed to ensure we break the reference cycles
@@ -156,6 +160,33 @@ internal final class SSLConnection {
         }
     }
 
+    func setCustomVerificationCallback(_ callbackManager: CustomVerifyManager) {
+        // Store the verification callback. We need to do this to keep it alive throughout the connection.
+        // We'll drop this when we're told that it's no longer needed to ensure we break the reference cycles
+        // that this callback inevitably produces.
+        assert(self.customVerificationManager == nil)
+        self.customVerificationManager = callbackManager
+
+        // We need to know what the current mode is.
+        let currentMode = CNIOBoringSSL_SSL_get_verify_mode(self.ssl)
+        CNIOBoringSSL_SSL_set_custom_verify(self.ssl, currentMode) { ssl, outAlert in
+            guard let unwrappedSSL = ssl else {
+                preconditionFailure("Unexpected null pointer in custom verification callback. ssl: \(String(describing: ssl))")
+            }
+
+            // Ok, this call may be a resumption of a previous negotiation. We need to check if our connection object has a pre-existing verifiation state.
+            guard let connectionPointer = CNIOBoringSSL_SSL_get_ex_data(unwrappedSSL, sslConnectionExDataIndex) else {
+                // Uh-ok, our application state is gone. Don't let this error silently pass, go bang.
+                preconditionFailure("Unable to find application data from SSL * \(unwrappedSSL), index \(sslConnectionExDataIndex)")
+            }
+
+            let connection = Unmanaged<SSLConnection>.fromOpaque(connectionPointer).takeUnretainedValue()
+
+            // We force unwrap the custom verification manager because for it to not be set is a programmer error.
+            return connection.customVerificationManager!.process(on: connection)
+        }
+    }
+
     /// Sets whether renegotiation is supported.
     func setRenegotiationSupport(_ state: NIORenegotiationSupport) {
         var baseState: ssl_renegotiate_mode_t
@@ -364,9 +395,10 @@ internal final class SSLConnection {
     /// Must only be called when the connection is no longer needed. The rest of this object
     /// preconditions on that being true, so we'll find out quickly when that's not the case.
     func close() {
-        /// Drop the verification callback. This breaks any reference cycles that are inevitably
-        /// created by this callback.
+        /// Drop the verification callbacks. This breaks any reference cycles that are inevitably
+        /// created by these callbacks.
         self.verificationCallback = nil
+        self.customVerificationManager = nil
 
         // Also drop the reference to the parent channel handler, which is a trivial reference cycle.
         self.parentHandler = nil
@@ -422,6 +454,17 @@ extension SSLConnection {
 
         return try body(PeerCertificateChainBuffers(basePointer: stackPointer))
     }
+
+    /// The certificate chain presented by the peer.
+    func peerCertificateChain() throws -> [NIOSSLCertificate] {
+        return try self.withPeerCertificateChainBuffers { buffers in
+            guard let buffers = buffers else {
+                return []
+            }
+
+            return try buffers.map { try NIOSSLCertificate(bytes: $0, format: .der) }
+        }
+    }
 }
 
 extension SSLConnection.PeerCertificateChainBuffers: RandomAccessCollection {