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

Added support for NIOSSLCustomVerificationCallback for client connection #1107

Merged

Conversation

franck-clement-ug
Copy link
Contributor

@franck-clement-ug franck-clement-ug commented Jan 19, 2021

Hi,

This PR adds the ability for client connections to override the certificate verification logic implemented in nio-ssl.

The main goal is to allow client apps to perform their own certificate verification logic, such as SSL public key pinning.
Nio-ssl provide this feature as described here: nio-ssl-issue-25
But I wasn't able to use it through grpc-swift because the NIOSSLCustomVerificationCallback is not accessible when initializing a secure ClientConnection.

I exposed a NIOSSLCustomVerificationCallback property in ClientConnection.Configuration struct and provided a ClientConnection.Builder func as well.

Usage example for client public key pinning

let myPublicKey = Array(Data(base64Encoded: """
.... (Your base64 encoded public key)
""", options: .ignoreUnknownCharacters)!)

ClientConnection.secure(group: PlatformSupport.makeEventLoopGroup(loopCount: 1))
            .withTLSCustomVerificationCallback({ cert, eventLoopVerification in
                // Extract the leaf certificate's public key,
                // then compare it to the one you have.
                if let leaf = cert.first,
                   let publicKey = try? leaf.extractPublicKey() {

                    if let certSPKI = try? publicKey.toSPKIBytes(),
                       myPublicKey == certSPKI {
                        eventLoopVerification.succeed(.certificateVerified)
                    } else {
                        eventLoopVerification.fail(NIOSSLError.unableToValidateCertificate)
                    }
                } else {
                    eventLoopVerification.fail(NIOSSLError.noCertificateToValidate)
                }
            })
            .connect(host: host, port: port)

This PR + the example above should resolve #723 and resolve #1104

… client connection.

This allows client apps to perform SSL Public Key Pinning, or override the certificate verification logic
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 19, 2021

CLA Signed

The committers are authorized under a signed CLA.

@glbrntt glbrntt added the 🆕 semver/minor Adds new public API. label Jan 20, 2021
Copy link
Collaborator

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, I've left some initial comments inline. You'll need to reformat the code to be consistent with the rest of the project (running ./scripts/format.sh should do it). We'll also need a test for this; you should be able to use the ClientTLSFailureTests as a starting point.

Sources/GRPC/ClientConnection.swift Outdated Show resolved Hide resolved
Sources/GRPC/ClientConnection.swift Show resolved Hide resolved
Sources/GRPC/GRPCChannel/GRPCChannelBuilder.swift Outdated Show resolved Hide resolved
Sources/GRPC/GRPCChannel/GRPCChannelBuilder.swift Outdated Show resolved Hide resolved
* The customVerificationCallback is now part of ClientConnection.Configuration.TLS
* Moved the if-else statement in the do-catch
* The channel builder for customVerificationCallback is now part ClientConnection.Builder.Secure
* renamed withCustomVerification(callback: @escaping NIOSSLCustomVerificationCallback) to withCustomVerificationCallback(
    _ callback: @escaping NIOSSLCustomVerificationCallback
  )
@franck-clement-ug
Copy link
Contributor Author

@glbrntt Thank you for the detailed comments, they all make a lot of sense, and I fixed them.
I also added a test when the validation logic is forced to fail within the customVerificationCallback.

Copy link
Collaborator

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

Thanks @franck-clement-ug this looks good, just one naming nit to fix.

Sources/GRPC/GRPCChannel/GRPCChannelBuilder.swift Outdated Show resolved Hide resolved
Sources/GRPC/GRPCChannel/GRPCChannelBuilder.swift Outdated Show resolved Hide resolved
Copy link
Collaborator

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

This looks good to me -- assuming CI passes -- thanks @franck-clement-ug!

@glbrntt glbrntt merged commit e70c2cf into grpc:main Jan 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Public key pinning support in grpc-swift Certificate pinning with NIO?
2 participants