Skip to content
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

Support configuring a CA certificates bundle #290

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

main--
Copy link

@main-- main-- commented Apr 27, 2023

The trust_cert_ca() config option configures one specific trusted CA certificate. However, there are two downsides:

  • it requires a file path, so an in-memory certificate would have to be written to a temporary file
  • it supports loading exactly one certificate, so if you need to load an entire bundle (e.g. the AWS RDS bundle) you're out of luck

The trust_cert_ca_bundle() method implemented here solves both of these issues by taking a bundle of PEM-encoded CA certificates in a Vec and adding all of them to the TLS context. For cases where a CA bundle needs to be loaded from disk, users can of course simply read the file on their end and pass the contents to trust_cert_ca_bundle.

The trust_cert_ca() config option configures one specific trusted CA certificate. However, there are two downsides:
- it requires a file path, so an in-memory certificate would have to be written to a temporary file
- it supports loading exactly one certificate, so if you need to load an entire bundle (e.g. the AWS RDS bundle) you're out of luck

The trust_cert_ca_bundle() method implemented here solves both of these issues by taking a bundle of PEM-encoded CA certificates in a Vec<u8> and adding all of them to the TLS context.
For cases where a CA bundle needs to be loaded from disk, users can of course simply read the file on their end and pass the contents to trust_cert_ca_bundle.
&self.bundle[begin..next_begin.unwrap_or(self.bundle.len() - 1)]
})
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks good, but I think it ignores the markers for the end of the certificate. Have you tested this outside of this repo? Do you think a test case in this repo would be doable, to confirm that this doesn't break?

Copy link
Author

Choose a reason for hiding this comment

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

I think it ignores the markers for the end of the certificate

I'm not sure I understand. In any valid PEM bundle, each certificate is terminated before the next one begins. The only thing this code does is splitting a bundle of concatenated PEM structures into individual PEM structures. The PEM format explicitly allows arbitrary data (which parsers must skip over) before the BEGIN and after the END marker, so cleanly terminating after the END marker is not a requirement.

Have you tested this outside of this repo

We're using this code in production.

Do you think a test case in this repo would be doable

Sure. Do you think it would be sufficient to just have a small unit test that loads an arbitrary bundle using include_str!() and verifies that the expected number of certificates is produced? Or did you have something else in mind?

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.

2 participants