Skip to content
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.

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

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

Open
pacu opened this issue Jul 14, 2022 · 0 comments

Comments

@pacu
Copy link

pacu commented Jul 14, 2022

Context: I'm using swift-nio-ssl through Swift GRPC. And found some improvement points along the way of their API.
see: grpc/grpc-swift#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!

@pacu pacu changed the title Enhancement - NIOSSLContext throwing initializer error API is difficult to follow Enhancement - NIOSSLContext throwing initializer error API may be difficult to follow Jul 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant