Add support for additional TLS options in NIOTS transport #18
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
glbrntt
requested changes
Oct 31, 2024
|
|
||
| switch tlsConfig.trustRoots.wrapped { | ||
| case .certificates(let certificates): | ||
| let verifyQueue = DispatchQueue(label: "certificateVerificationQueue") |
There was a problem hiding this comment.
nit: should be prefixed with io.grpc, we can also drop "queue" from the name, it's a bit redundant here.
Suggested change
| let verifyQueue = DispatchQueue(label: "certificateVerificationQueue") | |
| let verifyQueue = DispatchQueue(label: "io.grpc.CertificateVerification") |
| certificateBytes = Data(bytes) | ||
|
|
||
| case .file(_, let format), .bytes(_, let format): | ||
| fatalError("Certificate format must be DER, but was \(format).") |
There was a problem hiding this comment.
Hmm – do we document this requirement somewhere?
Comment on lines
+284
to
+329
| switch tlsConfig.trustRoots.wrapped { | ||
| case .certificates(let certificates): | ||
| let verifyQueue = DispatchQueue(label: "certificateVerificationQueue") | ||
| let verifyBlock: sec_protocol_verify_t = { (metadata, trust, verifyCompleteCallback) in | ||
| let actualTrust = sec_trust_copy_ref(trust).takeRetainedValue() | ||
|
|
||
| let customAnchors: [SecCertificate] | ||
| do { | ||
| customAnchors = try certificates.map { certificateSource in | ||
| let certificateBytes: Data | ||
| switch certificateSource.wrapped { | ||
| case .file(let path, .der): | ||
| certificateBytes = try Data(contentsOf: URL(filePath: path)) | ||
|
|
||
| case .bytes(let bytes, .der): | ||
| certificateBytes = Data(bytes) | ||
|
|
||
| case .file(_, let format), .bytes(_, let format): | ||
| fatalError("Certificate format must be DER, but was \(format).") | ||
| } | ||
|
|
||
| guard let certificate = SecCertificateCreateWithData(nil, certificateBytes as CFData) | ||
| else { | ||
| fatalError("Certificate was not a valid DER-encoded X509 certificate.") | ||
| } | ||
| return certificate | ||
| } | ||
| } catch { | ||
| verifyCompleteCallback(false) | ||
| return | ||
| } | ||
|
|
||
| SecTrustSetAnchorCertificates(actualTrust, customAnchors as CFArray) | ||
| SecTrustEvaluateAsyncWithError(actualTrust, verifyQueue) { _, trusted, _ in | ||
| verifyCompleteCallback(trusted) | ||
| } | ||
| } | ||
|
|
||
| sec_protocol_options_set_verify_block( | ||
| self.securityProtocolOptions, | ||
| verifyBlock, | ||
| verifyQueue | ||
| ) | ||
|
|
||
| case .systemDefault: | ||
| () |
There was a problem hiding this comment.
This is all copy-pasta from the client equivalent, can we avoid the duplication?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds support for a few additional TLS options in the NIOTS transport, to bring it in line with the NIOPosix implementation. In particular, it now supports custom trust roots, which means mTLS can be enabled when using NIOTS, and E2E testing is possible (because we can use self-signed certificates).
Some code has been refactored to be shared across transport implementations, and a small bug with how errors were being surfaced has been fixed.