apple / swift-nio-ssl

TLS Support for SwiftNIO, based on BoringSSL.
https://swiftpackageindex.com/apple/swift-nio-ssl/main/documentation/niossl
Apache License 2.0
388 stars 139 forks source link

Enhancement - NIOSSLContext throwing initializer error API may be difficult to follow #381

Open pacu opened 2 years ago

pacu commented 2 years ago

Context: I'm using swift-nio-ssl through Swift GRPC. And found some improvement points along the way of their API. see: https://github.com/grpc/grpc-swift/issues/1459

API Calls that make use of this initializer

    /// Initialize a context that will create multiple connections, all with the same
    /// configuration.
    internal init(
        configuration: TLSConfiguration,
        callbackManager: CallbackManagerProtocol?,
        additionalCertificateChainVerification: AdditionalCertificateChainVerificationCallback?
    ) throws {

end up being hard to follow due to the complex throwing interface. Source Code

Clients of this API could receive a lot of errors for many reasons and they are not simple to streamline to clients. I don't know much about this at all, so I read through the code and made some notes on what the failure points are and the errors they return.

Throwing reason 1: PEM Files could fail to read throws self-explanatory error 🥰 ✅

// On non-Linux platforms, when using the platform default trust roots, we make use of a
        // custom verify callback. If we have also been presented with additional trust roots of
        // type `.file`, we take the opportunity now to load them in memory to avoid doing so
        // repeatedly on the request path.
        //
        // However, to avoid closely coupling this code with other parts (e.g. the platform-specific
        // concerns, and the defaulting of `trustRoots` to `.default` when `nil`), we unilaterally
        // convert any `additionalTrustRoots` of type `.file` to `.certificates`.
        var configuration = configuration
        configuration.additionalTrustRoots = try configuration.additionalTrustRoots.map { trustRoots in
            switch trustRoots {
            case .file(let path):
                return .certificates(try NIOSSLCertificate.fromPEMFile(path))
            default:
                return trustRoots
            }
        }

Throwing reason 2: Verification Algorithms cause some error code that's not detailed but it could be better typed ⚠️

  // Configure verification algorithms
        if let verifySignatureAlgorithms = configuration.verifySignatureAlgorithms {
            returnCode = verifySignatureAlgorithms
                .map { $0.rawValue }
                .withUnsafeBufferPointer { algo in
                    CNIOBoringSSL_SSL_CTX_set_verify_algorithm_prefs(context, algo.baseAddress, algo.count)
            }
            if returnCode != 1 {
                let errorStack = BoringSSLError.buildErrorStack()
                throw BoringSSLError.unknownError(errorStack)
            }
        }

Throwing reason 3: Configuring signing algorithms fail with some error that's not detailed but it could be better typed ⚠️

// Configure signing algorithms
        if let signingSignatureAlgorithms = configuration.resolvedSigningSignatureAlgorithms {
            returnCode = signingSignatureAlgorithms
                .map { $0.rawValue }
                .withUnsafeBufferPointer { algo in
                    CNIOBoringSSL_SSL_CTX_set_signing_algorithm_prefs(context, algo.baseAddress, algo.count)
            }
            if returnCode != 1 {
                let errorStack = BoringSSLError.buildErrorStack()
                throw BoringSSLError.unknownError(errorStack)
            }
        }

Throwing Reason 4: Certificate chain setup failures that DO have typed self-explanatory errors 🥰✅

var leaf = true
        try configuration.certificateChain.forEach {
            switch $0 {
            case .file(let p):
                NIOSSLContext.useCertificateChainFile(p, context: context)
                leaf = false
            case .certificate(let cert):
                if leaf {
                    try NIOSSLContext.setLeafCertificate(cert, context: context)
                    leaf = false
                } else {
                    try NIOSSLContext.addAdditionalChainCertificate(cert, context: context)
                }
            }
        }

Throwing reason 5: private key configuration with self-explanatory error 🥰 ✅


        if let pkey = configuration.privateKey {
            switch pkey {
            case .file(let p):
                try NIOSSLContext.usePrivateKeyFile(p, context: context)
            case .privateKey(let key):
                try NIOSSLContext.setPrivateKey(key, context: context)
            }
        }

Throwing Reason 6: AlpnProtocol setup Error throws some typed error 🥰 ✅


        if configuration.encodedApplicationProtocols.count > 0 {
            try NIOSSLContext.setAlpnProtocols(configuration.encodedApplicationProtocols, context: context)
            NIOSSLContext.setAlpnCallback(context: context)
        }

throws a typed error BoringSSLError.failedToSetALPN(errorStack)

Throwing Reason 7: adding key log callback calls throws but it does not call any throwing functions. ⚠️

 // Add a key log callback.
        if let keyLogCallback = configuration.keyLogCallback {
            self.keyLogManager = KeyLogCallbackManager(callback: keyLogCallback)
            try NIOSSLContext.setKeylogCallback(context: context)
        } else {
            self.keyLogManager = nil
        }

This function doesn't appear to be throwing anything

private static func setKeylogCallback(context: OpaquePointer) throws {
        CNIOBoringSSL_SSL_CTX_set_keylog_callback(context) { (ssl, linePointer) in
            guard let ssl = ssl, let linePointer = linePointer else {
                return
            }

            // We want to take the SSL pointer and extract the parent Swift object. These force-unwraps are for
            // safety: a correct NIO program can never fail to set these pointers, and if it does failing loudly is
            // more useful than failing quietly.
            let parentCtx = CNIOBoringSSL_SSL_get_SSL_CTX(ssl)!
            let parentPtr = CNIOBoringSSLShims_SSL_CTX_get_app_data(parentCtx)!
            let parentSwiftContext: NIOSSLContext = Unmanaged.fromOpaque(parentPtr).takeUnretainedValue()

            // Similarly, this force-unwrap is safe because a correct NIO program can never fail to unwrap this entry
            // either.
            parentSwiftContext.keyLogManager!.log(linePointer)
        }
    }

It would be great if some error interface would be documented for developers to expect, wrap and upstream with the adecquate to callers.

Great work and thanks for your time!