-
Notifications
You must be signed in to change notification settings - Fork 0
Add support for custom client certificate verification #34
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
Add support for custom client certificate verification #34
Conversation
…allback Motivation: `NIOSSL` supports specifying a custom verification callback for client certificate verification. When such a callback is provided to `NIOSSLServerHandler`, it will use the callback to _replace_ its default client certificate verification logic, and surface the peer's validated chain of trust (derived and returned from the custom callback). This can be useful for mTLS implementations. `NIOHTTPServer` currently does not support propagating a custom verification callback to `NIOSSLServerHandler`. This change adds support for that and also surfaces the peer's validated certificate chain that then becomes available as a result. Modifications: - Updated `HTTPServerConfiguration` to also include a `customCertificateVerificationCallback` argument for the `mTLS` and `reloadingMTLS` cases of `TransportSecurity`. - Note: This exposes `NIO` types at the `HTTPServerConfiguration` level (in particular, the arguments of the callback are [`NIOSSLCertificate`] and an `EventLoopPromise` that must be fulfilled within the callback). We currently do not have a `NIOHTTPServer`-specific configuration, so let us discuss in the comments what the best way to allow users to specify a custom verification callback would be. - Updated the `serveSecureUpgrade` method in `NIOHTTPServer` to (1) propagate the custom verification callback into the underlying `NIOSSLServerHandler`, (2) extract the peer certificate chain `EventLoopFuture` per connection, and expose that future in a new type in `NIOHTTPServer` named `Context`. - The `Context` type is accessed from a task-local property `context`. Users can await the result of the promise from their route handlers by calling `NIOHTTPServer.context.peerCertificateChain()`. - The name of this type, its properties, the task-local approach, etc. are all very much open for discussion. - Added end-to-end tests for this functionality for both HTTP/1.1 and HTTP2. - Other changes: - Added `NIOSSL+X509` containing some convenience conversions between `NIOSSL` and `X509` types. - Added documentation to the `TransportSecurity` methods as they were missing. Result: Users can now specify a custom verification callback and access the peer's validated certificate chain from the request handler.
| public import NIOCore | ||
| public import NIOSSL |
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 think we need to discuss the larger strategy here. One goal of this package was to not expose any NIO types at all. So that in the future we could transparently change the underlying networking stack if we wanted to. @gjcairo we need to discuss this more.
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 only reason we need to expose NIO types there is because of the verification callback type, whose signature is: ([NIOSSLCertificate], EventLoopPromise<NIOSSLVerificationResultWithMetadata>) -> Void.
We could just expose a callback of type ([X509.Certificate]) async throws -> VerificationResult (where VerificationResult is just a wrapper over NIOSSLVerificationResultWithMetadata) and then bridge to NIOSSL like below:
NIOSSLServerHandler(
...
customVerificationCallbackWithMetadata: { certificates, promise in
promise.completeWithTask {
// Convert input [NIOSSLCertificate] to [X509.Certificate]
let x509Certs = try certificates.map { try Certificate($0) }
let callbackResult = try await customVerificationCallback(x509Certs)
switch callbackResult {
case .certificateVerified(let verificationMetadata):
guard let peerChain = verificationMetadata.validatedCertificateChain else {
return .certificateVerified(.init(nil))
}
// Convert the result back into [NIOSSLCertificate]
let nioSSLCerts = try peerChain.map { try NIOSSLCertificate($0) }
return .certificateVerified(.init(.init(nioSSLCerts)))
case .failed:
return .failed
}
}
}
)
The tradeoff we make here is the conversion cost from [NIOSSLCertificate] to [X509.Certificate] and vice-versa.
But a broader question: should we have an abstract server configuration type? If so, having the custom certificate verification callback as part of the abstract configuration may not be the right choice because that would require different server implementations to support 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.
One goal of this package was to not expose any NIO types at all.
This implementation is a NIO implementation. The abstract APIs don't use any configuration types right now, which means we don't currently need an HTTPServerConfiguration type that's completely independent of the specific implementation details. For this new callback specifically, I am not convinced it would even make sense to expose it in a more generic (i.e. non-NIO-specific) context, since each implementation might handle this differently, not with a closure of this exact shape.
We could have an HTTPServerConfiguration with an implementationSpecific property, similar to what I'd done for the RequestContext originally, and put this callback there if you think that's better. But that brings me to the second point...
So that in the future we could transparently change the underlying networking stack if we wanted to.
I think we need to decide then whether this package should be a NIO implementation of the server transport, or a... default (for lack of a better word) implementation of a server? If we're saying we intend to, in the future, reuse the same package but change the underlying implementation, then we need to find a way to do so in a way that can be evolved without breaking people. I'm not convinced the future NIO replacement will need this callback for example, or maybe something similar but not with this exact shape. Other aspects of the configuration may be available in the new implementation that aren't available in NIO, or viceversa. I think some changes will for sure be necessary at the config level if we change the underlying implementation.
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 implementation is a NIO implementation. The abstract APIs don't use any configuration types right now, which means we don't currently need an HTTPServerConfiguration type that's completely independent of the specific implementation details. For this new callback specifically, I am not convinced it would even make sense to expose it in a more generic (i.e. non-NIO-specific) context, since each implementation might handle this differently, not with a closure of this exact shape.
I think we are mixing two things up here. We always had the goal of the HTTPServer implementation to also be decoupled from the NIO. The fact that we also want to have a high level HTTP server protocol that is decoupled as well is related but orthogonal. The reason why we want the concrete HTTP server implementation to also be decoupled is because we expect more packages to build on top of it. If we require NIO in our API here then these packages also need to use NIO. This was one goal when the SSWG started this repo and I personally don't think we should drop it.
I think we need to decide then whether this package should be a NIO implementation of the server transport, or a... default (for lack of a better word) implementation of a server?
In my opinion and the initial goal of this package was that this is a default implementation of a server and it uses NIO to start with.
Yeah, the only reason we need to expose NIO types there is because of the verification callback type, whose signature is: ([NIOSSLCertificate], EventLoopPromise) -> Void.
We could just expose a callback of type ([X509.Certificate]) async throws -> VerificationResult (where VerificationResult is just a wrapper over NIOSSLVerificationResultWithMetadata) and then bridge to NIOSSL like below:
I would really prefer that approach. The wrapping isn't too bad and it keeps us flexible. We can certainly introduce changes on the NIO side to reduce the perf overhead if we wanted to but going this way keeps us flexible.
But a broader question: should we have an abstract server configuration type? If so, having the custom certificate verification callback as part of the abstract configuration may not be the right choice because that would require different server implementations to support it
I don't think we should have an abstract configuration type. At least yet. Having said that the API you proposed
([X509.Certificate]) async throws -> VerificationResult
Looks like a good API that can stand the battle of time since it uses just ecosystem wide currency types i.e. Certificate. The only question that I have is if VerificationResult should be a type in swift-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.
We always had the goal of the HTTPServer implementation to also be decoupled from the NIO. The fact that we also want to have a high level HTTP server protocol that is decoupled as well is related but orthogonal.
I actually didn't realize this, but if that's the case, then I think this PR should be pre-empted by another PR that splits the targets, and puts the NIO-dependent code into a separate one from the "generic" one. That way, the build system ensures we don't depend on things incorrectly, rather than something that needs to be caught by human review.
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 we go down the wrapping route:
The only question that I have is if VerificationResult should be a type in swift-certificates?
Just remembered that a CertificateValidationResult type was recently introduced in swift-certificates (see PR apple/swift-certificates#272). This type is returned by Verifier's validate method.
If user's don't use swift-certificates' Verifier within the callback, then CertificateValidationResult may not be an ergonomic choice:
- The associated value of the
validCertificatecase isValidatedCertificateChain, which intentionally does not have a nice public initializer (to discourage users from initializing this type with certificates that haven't been validated). - Also, the other
couldNotValidatecase hasPolicyFailureas an associated value, which aside from being tied with theVerifierandVerifierPolicyrelation, can only be initialized with aUnverifiedCertificateChain---UnverifiedCertificateChainis the input toVerifierPolicy, and has no public initializer (again, this makes sense forswift-certificatesbecause users shouldn't be able to useVerifierPolicys in isolation withoutVerifier).
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.
From @gjcairo:
The idea is for this repo to depend on swift-http-api-proposal and remove all the abstract pieces once both repos are public. Leaving the abstract pieces aside, I don't think there's any code that doesn't depend on NIO.
From @FranzBusch:
I think we are mixing two things up here. We always had the goal of the HTTPServer implementation to also be decoupled from the NIO. The fact that we also want to have a high level HTTP server protocol that is decoupled as well is related but orthogonal.
My read of this is that Franz implied that this repo should have two things, which are merged right now: a NIO-less HTTP server "implementation", and a NIO-specific "transport". If that's the case, let's separate these layers early, as it only gets more difficult as more code is added.
This will also simplify discussing where each configuration should go. Otherwise we might spend time creating various wrappers that aren't needed, if actually we meant to configure the transport (which can totally use NIO types in 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.
I don't think we should drop this requirement either, and we're not using NIO in our APIs broadly. It's just the configuration that's required to create an instance of a
NIOHTTPServerexposes a NIO type - which doesn't seem like such a far fetched thing in my eyes.
I think we should avoid it in the configuration as well. This will make the entire package a lot more future proof. Especially since we do have the types already and X509.Certificate is a lot nicer to use.
If we do that though, why would we keep the config as a
NIOHTTPServer-only type instead of in the abstract package?
I think we should bikeshed the type name a bit more. Maybe NIOHTTPServer is not the right thing after all.
Just remembered that a CertificateValidationResult type was recently introduced in swift-certificates (see PR apple/swift-certificates#272). This type is returned by Verifier's validate method.
I think this type is good. Regarding your two comments:
- I think the
uncheckedCertificateChainis great. It is publicly available but nudges the user into the right direction. - I think we should discuss making the
initonUnverifiedCertificateChainpublic.
In the end, we need something to provide the currency types in the ecosystem. You can see that Go does the same. If these types aren't in swift-certificates then we are going to need them to be somewhere different.
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.
My read of this is that Franz implied that this repo should have two things, which are merged right now: a NIO-less HTTP server "implementation", and a NIO-specific "transport".
There are no two layers here. We discussed this before: having a lower level transport protocol is kinda pointless, there's really nothing separating each layer. If we would move the NIO implementation into a transport layer, the "NIO-less server implementation" would literally be just forwarding calls to the NIO layer.
What we might want though is what I suggested in one of my previous comments: a layered configuration, with some implementation-specific stuff.
I think we should bikeshed the type name a bit more. Maybe
NIOHTTPServeris not the right thing after all.
This kinda missed the point of my question, which was: if we're using only swift-certificate types (or more broadly, currency types), then should the config type just live alongside the abstract pieces?
Regarding naming, I'm not sure. It is a NIO implementation. gRPC or OpenAPI transports do disclose the underlying implementation in their names (grpc-swift-nio-transport, swift-openapi-urlsession).
You shouldn't touch this type as a library developer, you'd just use the abstract pieces (e.g. HTTPServer). I understand your point about evolving this into an implementation that doesn't use NIO underneath, but I'm not convinced that requires changing the underlying implementation of this module transparently. Doing so, IMO, would require having a transport protocol layer which we'd discussed seems superfluous.
I don't see what the issue would be with having a new package or module that provides a different implementation not using NIO in the future. Library authors would not require any changes in their packages since they'd be using the abstract pieces anyways. For users using the NIO implementation directly, I don't think it's wrong they'd have to manually switch to the new thing. The only other scenario I can envision is libraries like Vapor or HB bootstrapping a default implementation as a utility, but then they could simply change this bootstrap and everyone using NIO will start using whatever replaces it.
Regarding CertificateValidationResult:
I think this type is good.
I was just chatting with Aryan and Raphael offline, who worked on adding the verifier APIs in gRPC and Vapor, and we're not sure this type is actually good. It is if you're using the Verifier in swift-certificates. But you could be using some custom verifier or Security.framework, and in this case, the verification results may not map easily to CertificateValidationResult cases. For example, PolicyFailures are a swift-certificates thing, and nothing from Security.framework can be mapped easily into that.
To be clear, I'm not saying a validation result type shouldn't exist. I'm just saying that I am not sure reusing this type works outside the context of swift-certificates. If we want to support other use cases (I assume that particularly Security.framework should be supported) then we probably need a new type without the associated values.
Tangentially related question: the client doesn't currently provide functionality like this, does it? Should 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.
This kinda missed the point of my question, which was: if we're using only swift-certificate types (or more broadly, currency types), then should the config type just live alongside the abstract pieces?
Not yet would be my argument. Hopefully in the future yes. I think there is a lot of stuff to figure out.
Regarding naming, I'm not sure. It is a NIO implementation. gRPC or OpenAPI transports do disclose the underlying implementation in their names (
grpc-swift-nio-transport,swift-openapi-urlsession).You shouldn't touch this type as a library developer, you'd just use the abstract pieces (e.g.
HTTPServer). I understand your point about evolving this into an implementation that doesn't use NIO underneath, but I'm not convinced that requires changing the underlying implementation of this module transparently. Doing so, IMO, would require having a transport protocol layer which we'd discussed seems superfluous.I don't see what the issue would be with having a new package or module that provides a different implementation not using NIO in the future. Library authors would not require any changes in their packages since they'd be using the abstract pieces anyways. For users using the NIO implementation directly, I don't think it's wrong they'd have to manually switch to the new thing. The only other scenario I can envision is libraries like Vapor or HB bootstrapping a default implementation as a utility, but then they could simply change this bootstrap and everyone using NIO will start using whatever replaces it.
I think you bring up good points and I am definitely okay with keeping the name NIOHTTPServer for now. I think we still wanna discuss which types we use in the public API. Even if it is NIO based I would still strongly argue that we should use swift-certifactes and async so that the APIs are nice to use.
All kinds of users will hold this type both library and applications authors and I would like them to not touch NIO bits for the basic things which I consider mTLS to be.
To be clear, I'm not saying a validation result type shouldn't exist. I'm just saying that I am not sure reusing this type works outside the context of swift-certificates. If we want to support other use cases (I assume that particularly
Security.frameworkshould be supported) then we probably need a new type without the associated values.
That's fair. I am happy if we introduce a new type as in the scope of this project but I think certificates should be expressed as X509.Certificate.
Tangentially related question: the client doesn't currently provide functionality like this, does it? Should it?
This is one open problem of the client. We probably want to spell tls/mtls settings on a per-request basis which means we gotta solve the TLS currency type problem.
FranzBusch
left a comment
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.
We need to discuss the strategy here. We shouldn't expose NIO types since this is going to couple us to NIO where as right now NIO is just an implementation detail.
| import SwiftASN1 | ||
| import X509 | ||
|
|
||
| /// Some convenience helpers for converting between NIOSSL and X509 certificate and private key types. |
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.
Might be worth filing an issue on swift-nio-ssl to provide this, and link the issue from here.
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've filed the issue here: apple/swift-nio-ssl#568
…tom verification callback
FranzBusch
left a comment
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 is looking great. A few minor comments
| public enum CertificateVerificationResult: Sendable, Hashable { | ||
| /// An error representing certificate verification failure. | ||
| public struct VerificationError: Swift.Error, Hashable { | ||
| let description: String |
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 probably be public
|
|
||
| /// Creates a verification error with a description of the failure. | ||
| /// - Parameter description: A description of why certificate verification failed. | ||
| public init(description: String) { |
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: Maybe reason is better for this?
| /// to provide access to it in the request handler through ``NIOHTTPServer/ConnectionContext/peerCertificateChain``, | ||
| /// accessed via the task-local ``NIOHTTPServer/connectionContext`` property. Otherwise, return | ||
| /// ``CertificateVerificationResult/failed(_:)`` if verification fails. | ||
| public typealias CustomCertificateVerificationCallback = |
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.
Do we really need this type alias? We normally try not to use type aliases because they are imposing some problems for the developer experience.
|
I've added support for also allowing optional client certificate verification, i.e. clients can connect without presenting certificates, but if they do present certificates, they are validated like normal (see the This required adding a new type named |
|
|
||
| let backing: Backing | ||
|
|
||
| public static let plaintext: Self = Self(backing: .plaintext) |
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 we add a doc comment to this?
| // | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| public import NIOCertificateReloading |
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.
Oh I just realized this is another NIO type. Let's open an issue we should really be able to spell this without NIO types if possible.
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've filed the issue here: https://github.com/swift-server/swift-http-server/issues/35.
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 is where things start getting a bit tricky though IMO. Unless certificate reloads also become part of the currency TLS types I think it will be hard to expose this in an abstract way. Anyways, we should have this conversation on the issue.
|
|
||
| /// Task-local storage for connection-specific information accessible from request handlers. | ||
| /// | ||
| /// Use this to access data such as the peer's validated certificate 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.
I would remove this line and just link to the docs for ConnectionContext instead (you can use - SeeAlso:
| /// Connection-specific information available during request handling. | ||
| /// | ||
| /// Provides access to data such as the peer's validated certificate chain. | ||
| public struct ConnectionContext: Sendable { |
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 we move this type to an extension on its own file?
| channel: http2StreamChannel, | ||
| handler: handler | ||
| ) | ||
| Self.$connectionContext.withValue(ConnectionContext(chainFuture)) { |
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.
We should move this outside of the for...await loop - I don't think new streams would trigger the chain to change, right?
| return .certificateVerified(.init(.init(nioSSLCerts))) | ||
|
|
||
| case .failed(let error): | ||
| self.logger.error("Custom certificate verification failed: \(error)") |
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 we add this error as metadata instead of interpolating it?
Sources/HTTPServer/NIOSSL+X509.swift
Outdated
|
|
||
| // MARK: X509 to NIOSSL | ||
|
|
||
| @available(macOS 26.0, iOS 26.0, watchOS 26.0, tvOS 26.0, visionOS 26.0, *) |
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 but I don't think we need these availability guards.
| // | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| public import NIOCertificateReloading |
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 is where things start getting a bit tricky though IMO. Unless certificate reloads also become part of the currency TLS types I think it will be hard to expose this in an abstract way. Anyways, we should have this conversation on the issue.
| /// - customCertificateVerificationCallback: If specified, this callback *overrides* the default NIOSSL client | ||
| /// certificate verification logic. Refer to the documentation for this argument in | ||
| /// ``mTLS(certificateChain:privateKey:trustRoots:certificateVerification:customCertificateVerificationCallback:)`` | ||
| /// for more details. |
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 we copy the docs here instead of referring to a different method's docs? I think if you're using this from an IDE it's a bit annoying to not be able to see the corresponding docs in place.
| try await responseBodySender.produceAndConclude { responseBodyWriter in | ||
| var responseBodyWriter = responseBodyWriter | ||
| try await responseBodyWriter.write([1, 2].span) | ||
| return [.trailer: "test_trailer"] |
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 we use Self.trailer here?
Motivation:
NIOSSLServerHandlerallows a custom verification callback to be used for client certificate verification. This callback returns the peer's validated chain of trust, whichNIOSSLthen surfaces in the channel handler. Having control over certificate verification and accessing the peer's validated certificate chain can be useful for mTLS implementations.NIOHTTPServercurrently does not support propagating a custom verification callback toNIOSSLServerHandler. This change adds support for that and also surfaces the peer's validated certificate chain which becomes available as a result.Modifications:
Updated the server configuration to also include a
customCertificateVerificationCallbackargument for themTLSandreloadingMTLScases ofTransportSecurity.NIOSSLtypes in the server configuration type (the arguments and return type of the callback). I'm also not sure whether custom client certificate verification is a requirement we want to impose for other server implementations, so for now, I have made the configuration typeNIOHTTPServerspecific and renamedHTTPServerConfigurationtoNIOHTTPServerConfiguration.Updated the
serveSecureUpgrademethod inNIOHTTPServerto:NIOSSLServerHandler;EventLoopFutureper connection, and;NIOHTTPServernamedConnectionContext.ConnectionContextis accessible from a task-local property. Users can await the result of the promise from their route handlers by callingNIOHTTPServer. connectionContext.peerCertificateChain().Added end-to-end tests for this functionality for both HTTP/1.1 and HTTP2.
Other changes:
NIOSSL+X509containing some convenience conversions betweenNIOSSLandX509types.TransportSecuritymethods.Result:
Users can now specify a custom verification callback and access the peer's validated certificate chain from the request handler.