-
Notifications
You must be signed in to change notification settings - Fork 155
Allow the peer's validated certificate chain to be returned in the custom verification handler #553
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
Conversation
…stom verification handler Motivation: Users are currently able to register a custom callback in `NIOSSLClientHandler` and `NIOSSLServerHandler` for verifiying the certificates presented by the peer. However, there is no mechanism for storing additional metadata for future use. This change adds support for the peer's validated certificate chain to be returned from the custom verification callback and later be accessed from the handler. Modifications: - Added a new custom verification callback type `NIOSSLCustomVerificationCallbackWithMetadata`. - This type is identical to the existing `NIOSSLCustomVerificationCallback`, with the exception that callers must complete the callback with a `NIOSSLVerificationResultWithMetadata` (introduced in this change). This result type can either be initialized with no fields, or with a validated certificate chain. - Added properties/methods to `NIOSSLHandler`, `Channel`, and `ChannelPipeline` for accessing the validated certificate chain. Result: Users are now able to store the peer's validated certificate chain from the custom verification callback and use the result downstream.
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.
I like it. I left two questions that should be easily resolvable. Just want to make sure we are using the right words.
/// - Note: Use ``init(context:serverHostname:customVerificationCallbackWithMetadata:)`` to provide a custom | ||
/// verification callback where metadata such as the peer's *validated* certificate chain can be returned. | ||
/// This data can then be accessed from the handler. |
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.
At the moment the only thing the metadata includes is the validated certificate chain. "Such as" implies that there could be more. Is this a subtle hint for a future extension or should we be direct here and state that this only includes the chain?
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.
Fair point. I feel it is best to be direct here. I will remove "metadata such as"
.
/// - This callback is provided the certificates presented by the peer. NIOSSL will not have pre-processed | ||
/// them. Therefore, a validated chain must be derived *within* this callback (potentially involving fetching | ||
/// additional intermediate certificates). The *validated* certificate chain returned in the promise result | ||
/// **must** be a verified path to a trusted root. Importantly, the certificate chain presented by the peer | ||
/// should not be assumed to be valid. |
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.
"Certificate chain" itself might not be a very well-defined term.
Here, it refers to both, the certificate chain presented by the client and the validated certificate chain that is the result of verification. However, they differ in so far, that a client can present more certificates than necessary for the verification and the client's certificates do not include the root.
In contrast, what we call validated certificate chain should include a leaf certificate in the front, a root certificate at the end, and the intermediate certificates that trace trust form the leaf to the root in between them.
It might very well be the best word we can use, I am easily convinced here. I just want to make sure we have given it enough thought. Just to offer a few alternatives.
- chain of trust (these are the certificates that establish trust from leaf to root)
- full validated chain (full includes the certificate chain of the client and the root)
- certificate path (IIRC the verification process is usually called path verification in the RFCs?)
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.
Yeah, the peer hasn't provided a chain, it's provided a blob.
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.
You are saying "verified certificate chain" is a good term to refer to the set of certificates that establishes trust for a client (leaf, intermediates, root), but we could update the comment and remove the last sentence or potentially replace "certificate chain" there with "sets of certificates"?
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.
I agree with your points.
I have removed documentation stating that the peer provides a certificate chain. Open for suggestions about the validatedCertificateChain
naming
Sources/NIOSSL/SSLCallbacks.swift
Outdated
public struct VerificationMetadata: Sendable, Hashable { | ||
/// A container for the validated certificate chain: an array of certificates forming a verified chain of trust | ||
/// from the peer's leaf certificate to a trusted root certificate. | ||
var validatedCertificateChain: ValidatedCertificateChain? |
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 should be public
.
Sources/NIOSSL/SSLCallbacks.swift
Outdated
/// The certificate was not verified. | ||
case failed | ||
|
||
static let certificateVerified: Self = .certificateVerified(.init()) |
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 may want to be public
Sources/NIOSSL/SSLCallbacks.swift
Outdated
// Prepares the promise that will be provided as an argument to the callback. Since different callbacks have | ||
// different result types, the generic `transformResult` closure is required for converting the specific result | ||
// type into ``CustomVerifyManager/PendingResult``. | ||
func preparePromise<T>( |
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.
Should be private
.
Also this looks like it's static
.
Sources/NIOSSL/SSLCallbacks.swift
Outdated
connection.customVerificationManager == nil | ||
|| connection.customVerificationManager?.result == .some(.pendingResult) | ||
) | ||
connection.customVerificationManager?.result = transformResult(result) |
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.
So this will have the awkward effect of triggering a heap allocation of this closure. Is there any way we can recover the transformResult
function without needing to close over it?
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.
I have addressed this by:
- Introducing a new fileprivate protocol
PendingResultConvertible
that requires a static function to convertResult<Self, any Error>
toCustomVerifyManager.PendingResult
- Adding conformance to both result types (
NIOSSLVerificationResult
andNIOSSLVerificationResultWithMetadata
) - Constraining the generic parameter in
preparePromise
to types conforming to the new protocol, and then simply calling thependingResult
function on the result.
However, this has introduced some complexity and redirection. We could also simply overload the preparePromise
function for both result types---the function itself isn't too long.
Sources/NIOSSL/SSLCallbacks.swift
Outdated
|
||
/// Represents a *validated* certificate chain, an array of certificates forming a verified path from the peer's | ||
/// certificate to a trusted root certificate. | ||
public struct ValidatedCertificateChain: Sendable, Collection, Hashable { |
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.
If you implement Collection
, it's almost always a good idea to implement RandomAccessCollection
if you can. Here, we can.
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.
Good to know this. I have added the RandomAccessCollection
conformance.
Sources/NIOSSL/SSLCallbacks.swift
Outdated
|
||
/// Represents a *validated* certificate chain, an array of certificates forming a verified path from the peer's | ||
/// certificate to a trusted root certificate. | ||
public struct ValidatedCertificateChain: Sendable, Collection, Hashable { |
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.
Should we offer a computed property for the leaf
?
Sources/NIOSSL/SSLCallbacks.swift
Outdated
} | ||
|
||
/// Returns the first element of the chain: the leaf certificate. | ||
public var leaf: NIOSSLCertificate? { |
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.
Have we considered requiring >0 certs in the validated chain, and thus making this non-optional?
If I get a validated chain with a nil leaf... what does it mean? What does that tell me? What can I do with it?
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.
Agreed. I have added a precondition to the initializer requiring >= 1 certificate and made the leaf
property non-optional.
Sources/NIOSSL/SSLCallbacks.swift
Outdated
case failed | ||
|
||
/// The certificate was successfully verified with no metadata returned. | ||
public static let certificateVerified: Self = .certificateVerified(.init()) |
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.
I'm not entirely convinced this is a helper we want, actually. Users seem unlikely to want to use the new API and then return nothing.
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.
Agreed, I have removed this now.
@@ -2365,6 +2364,57 @@ class NIOSSLIntegrationTest: XCTestCase { | |||
XCTAssertEqual(certificates.withLockedValue { $0 }, [NIOSSLIntegrationTest.cert]) | |||
} | |||
|
|||
func testExtractingCertificateChainFromCustomVerificationCallback() throws { |
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.
Can I suggest that we also add tests to confirm:
- The failure path
- Delayed success (i.e. return success later on, from the main thread).
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.
Agreed, I have added tests for these two cases.
customVerificationCallbackWithMetadata: @escaping NIOSSLCustomVerificationCallbackWithMetadata, | ||
configuration: Configuration |
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.
Closures should generally go last in parameter lists
public convenience init( | ||
context: NIOSSLContext, | ||
customVerificationCallbackWithMetadata: @escaping NIOSSLCustomVerificationCallbackWithMetadata, | ||
configuration: Configuration |
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.
nit: closures should generally go last in parameter lists
Sources/NIOSSL/SSLCallbacks.swift
Outdated
public var validatedCertificateChain: ValidatedCertificateChain? | ||
|
||
/// Creates an instance with empty metadata. | ||
public init() { |
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.
Why two init
s vs. one which accepts an optional ValidatedCertificateChain
?
/// - Important: Do not blindly pass in the array of certificates presented by the peer; the array *must* represent | ||
/// a fully validated and trusted chain. |
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.
FWIW: you can also document preconditions: - Precondition: ...
. It'd be worth adding that the chain must have at least one certificate in.
Motivation:
Users are currently able to register a custom callback in
NIOSSLClientHandler
andNIOSSLServerHandler
for verifiying the certificates presented by the peer. However, there is no mechanism for storing additional metadata for future use. This change adds support for the peer's validated certificate chain to be returned from the custom verification callback and later be accessed from the handler.Modifications:
NIOSSLCustomVerificationCallbackWithMetadata
.NIOSSLCustomVerificationCallback
, with the exception that callers must complete the callback with aNIOSSLVerificationResultWithMetadata
(also introduced in this change). This result type can either be initialized with no fields, or with a validated certificate chain.NIOSSLHandler
,Channel
, andChannelPipeline
for accessing the validated certificate chain.Result:
Users are now able to store the peer's validated certificate chain from the custom verification callback and use the result downstream.