-
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
test: Add tests to verify mTLS behaviour #1307
Conversation
This adds some tests to check that configuring for mTLS behaves as expected. Specifically that the server will reject clients.
Signed-off-by: Si Beaumont <beaumont@apple.com>
Signed-off-by: Si Beaumont <beaumont@apple.com>
Signed-off-by: Si Beaumont <beaumont@apple.com>
Tests/GRPCTests/MutualTLSTests.swift
Outdated
let serverErrorDelegate = ServerErrorRecordingDelegate(expectation: serverErrorExpectation) | ||
serverConfiguration.errorDelegate = serverErrorDelegate | ||
|
||
let server = try! Server.start(configuration: serverConfiguration).wait() |
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 still need to shutdown the server
Tests/GRPCTests/MutualTLSTests.swift
Outdated
try self.performTestWith(serverTLSConfiguration, clientTLSConfiguration, expect: .success) | ||
} | ||
|
||
func test_noServerCert_ClientError() 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.
I'm not sure how useful this test is as an mTLS test: it's validates that a client configured with TLS fails to connect to a plaintext server.
Tests/GRPCTests/MutualTLSTests.swift
Outdated
try self.performTestWith(nil, clientTLSConfiguration, expect: .clientError) | ||
} | ||
|
||
func test_noClientCert_ServerError() 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.
Similar to above: this validates that a plaintext client cannot connect to a server configured with TLS. The more interesting test would be to configure the client to not present any certificates and for the server to expect mTLS.
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, my logic here for testing mutual auth was to check that when both sides provide valid certs things work, then when either side doesn't that things don't work.
Specifically this test is I guess testing client-verification by the server or am I missing something.
Is the thing I'm missing that we should be using a TLS client but with no cert (cf. no TLS)?
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.
Specifically this test is I guess testing client-verification by the server or am I missing something.
Yes but more specifically I think we want to test that the server verifies the certificate presented by the client.
At the moment because the client isn't configured to use TLS, the first bytes it sends will be the http/2 preface which the server will attempt to interpret as the SSL client hello, so the server will fail the handshake at the step rather than because the client doesn't present a valid certificate.
Is the thing I'm missing that we should be using a TLS client but with no cert (cf. no TLS)?
Yes
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.
OK, I've updated the PR description, and the makecert
script which had some limitations, and updated the tests to hopefully make more sense.
Signed-off-by: Si Beaumont <beaumont@apple.com>
Signed-off-by: Si Beaumont <beaumont@apple.com>
Signed-off-by: Si Beaumont <beaumont@apple.com>
…or older Swifts Signed-off-by: Si Beaumont <beaumont@apple.com>
privateKey: SamplePrivateKey.server | ||
) | ||
.withTLS(trustRoots: .certificates([SampleCertificate.ca.certificate])) | ||
.withTLS(certificateVerification: .noHostnameVerification) |
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.
Note: This looked to have been latently wrong since AFAIK the default client-verification for server config is .none
.
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 great, thanks Si! Just one tiny nit to avoid CI flakiness.
This adds some tests to check that configuring for mTLS behaves as expected. The tests aim to show the following: * Mutually trusted certs succeed * Untrusted certs from either side results in failure * Plaintext peer on either side results in failure Other changes included in this PR: * Not all certs in the sample data are actually generated in the `makecert` script (e.g. the `localhost` server cert). * The `makecert` script and sample data were extended to include some certs signed by a different CA for mTLS failure mode tests.
This adds some tests to check that configuring for mTLS behaves as expected.
The tests aim to show the following:
Other changes included in this PR:
makecert
script (e.g. thelocalhost
server cert).makecert
script and sample data were extended to include some certs signed by a different CA for mTLS failure mode tests.