Skip to content

CDRIVER-4206 KMIP support #881

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

Merged
merged 93 commits into from
Nov 13, 2021
Merged

Conversation

kevinAlbs
Copy link
Collaborator

@kevinAlbs kevinAlbs commented Oct 16, 2021

Summary

Implements the specification changes of DRIVERS-1353. Please see the comments of DRIVERS-1353 for the linked specification change commits.

  • Add mongoc_auto_encryption_opts_set_tls_opts and mongoc_client_encryption_opts_set_tls_opts to set custom TLS options on KMS connections.
  • Update specification tests and prose tests.
  • Update CSFLE documentation:
    • Add KMIP KMS provider.
    • Add missing documentation for Azure and GCP KMS providers (CDRIVER-4087).
    • Fix documentation for local KMS provider (CDRIVER-4088).

Rejected alternatives

Passing TLS options with mongoc_ssl_opt_t

The specification is not prescriptive in the API for passing TLS options:

Drivers SHOULD provide API that is consistent with configuring TLS options for MongoDB server TLS connections. New API to support the options MUST be independent of the KMS provider to permit future extension. The following is an example:

I considered an alternative API for passing TLS options:

void
mongoc_auto_encryption_opts_set_tls_opts (
   mongoc_auto_encryption_opts_t *opts,
   const char* provider,
   const mongoc_ssl_opt_t *ssl_opt
);

It is more consistent with mongoc_client_set_ssl_opts. But I chose to reject it because:

  • The mongoc_ssl_opt_t does not permit configuring all TLS options supported in the URI specification (tlsDisableOCSPEndpointCheck, tlsDisableCertificateRevocationCheck).
  • mongoc_ssl_opt_t includes insecure options (allow_invalid_hostname and weak_cert_validation) which are prohibited in the specification, as well as C specific options ca_dir and crl_file, which are not required and are not supported on all TLS implementations.
  • SSL is a deprecated term in favor of TLS. E.g. mongoc_uri_get_ssl is deprecated for mongoc_uri_get_tls.

Caveats

  • Error messages in the "no client certificate" cases are empty. Commented inline.

@kevinAlbs kevinAlbs changed the title DRIVERS-1535 KMIP support POC CDRIVER-4100 KMIP support POC Nov 4, 2021
@kevinAlbs kevinAlbs changed the title CDRIVER-4100 KMIP support POC CDRIVER-4100 KMIP support Nov 7, 2021
@kevinAlbs kevinAlbs changed the title CDRIVER-4100 KMIP support CDRIVER-4206 KMIP support Nov 8, 2021
@kevinAlbs kevinAlbs requested a review from chardan November 9, 2021 01:19
Copy link
Contributor

@chardan chardan left a comment

Choose a reason for hiding this comment

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

Lots of work in here! I especially appreciate the careful documentation. Please take a look at just a few things in here.

Copy link
Contributor

@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.

Other than some minor formatting, LGTM. 👍

Copy link
Contributor

@chardan chardan left a comment

Choose a reason for hiding this comment

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

Strong improvements! Just a couple of things you may want to consider, but +1 from me.

@kevinAlbs kevinAlbs merged commit 2078ab2 into mongodb:master Nov 13, 2021
@kevinAlbs kevinAlbs deleted the kmip.DRIVERS-1353 branch November 13, 2021 22:20
kevinAlbs added a commit to kevinAlbs/mongo-c-driver that referenced this pull request Dec 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants