Skip to content

Conversation

@eramongodb
Copy link
Contributor

This PR comes in three parts:

  1. Ensure that all four SSL libraries (OpenSSL, Secure Channel, Secure Transport, LibreSSL) report descriptive errors for each certificate verification failure.

The TLS handshake implementation for OpenSSL and Secure Channel require some modification to report descriptive certificate verification errors. The TLS handshake implementation for Secure Transport and LibreSSL do not require any modification to obtain descriptive error messages.

  1. Add tests that verify correct error (or lack thereof) against mock KMS servers.

Client Side Encryption tests were unconditionally being included by the test suite even when SSL is not enabled, despite CSE requiring SSL to be enabled. Therefore, the relevant source file and test functions are now moved into appropriate conditionals so they are only built and run when MONGOC_ENABLE_SSL is defined by the build system.

Existing unit test helper functions are modified to enable reuse of AWS KMS provider setup helpers for KMS TLS tests separately from other KMS providers. The test function test_kms_tls_cert_valid(), which confirms a valid TLS handshake on valid server certificate, is not required by CDRIVER-3927, but nevertheless useful to confirm proper CA certificate registration in the environment. It explicitly calls mongoc_stream_tls_handshake_block() and relevant setup functions rather mongoc_client_encryption_create_datakey() to avoid unnecessary errors on datakey creation due to running against the mock KMS server.

  1. Enable KMS TLS tests on Evergreen.

Evergreen config scripts are updated to start mock KMS servers in the background only for tasks with Client Side Encryption enabled. The run_tests.sh script also requires an update to ensure proper CA certificate registration on Windows and Red Hat Linux, which use different commands and directories.

@eramongodb eramongodb self-assigned this Oct 4, 2021
Copy link
Collaborator

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! I appreciate the detailed description. Splitting up the work into meaningful commits made reviewing much easier.

@eramongodb eramongodb requested a review from kevinAlbs October 6, 2021 21:45
Copy link
Collaborator

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Nice detective work in finding CDRIVER-4181. I think the additional error details are likely to help users diagnose TLS configuration problems.

Copy link
Contributor

@rcsanchez97 rcsanchez97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, modulo the one minor nit.

Copy link
Contributor Author

@eramongodb eramongodb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Investigating certificate registration failure on RHEL 8.1. Will merge once addressed.

@eramongodb
Copy link
Contributor Author

@kevinAlbs Per external discussion, this PR will now disable CSE tests on RHEL upon merge. This change will be reverted on resolution of BUILD-14068, which will obviate the need to manually register the CA certificate via Evergreen build scripts. This followup task is recorded as CDRIVER-4183.

@eramongodb eramongodb requested a review from kevinAlbs October 11, 2021 21:28
@eramongodb eramongodb merged commit 33c8d31 into mongodb:master Oct 12, 2021
@eramongodb eramongodb deleted the cdriver-3927 branch October 12, 2021 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants