-
Notifications
You must be signed in to change notification settings - Fork 420
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
Added support for NIOSSLCustomVerificationCallback for client connection #1107
Conversation
… client connection. This allows client apps to perform SSL Public Key Pinning, or override the certificate verification logic
|
There was a problem hiding this 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.
* 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 )
…l within the customVerificationCallback
@glbrntt Thank you for the detailed comments, they all make a lot of sense, and I fixed them. |
There was a problem hiding this 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.
Co-authored-by: George Barnett <[email protected]>
…thTLSCustomVerificationCallback
There was a problem hiding this 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!
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 secureClientConnection
.I exposed a
NIOSSLCustomVerificationCallback
property inClientConnection.Configuration
struct and provided aClientConnection.Builder
func as well.Usage example for client public key pinning
This PR + the example above should resolve #723 and resolve #1104