Conversation
This commit adds TLS support to influxdb3 and allows users to pass in a path to a key and cert file with the --tls-key and --tls-cert flags in the serve command. It also adds the ability for every command to specify a certificate authority for requests. This is mostly needed when the cert is self signed, but there are other use cases for this. The big thing is that most of our tests now use TLS by default. Included are self signed certs for localhost and the the CA cert included in the commit. Since these are *only* used for testing this should be fine to include as they are not used in nor are they intended to be used in any production system. The expiry has been set for 365 days and the file perms are set to o600 like the original issue mentioned. The tests pass with this restriction. I've verified that the API works via curl with the self signed certs as I did *not* need to pass in the -k option to bypass checking the certs were valid. The same goes for our tests. They use the rootCA.pem file to verify the self signed cert when connecting and reject it otherwise. With this users can be confident that their queries are safely encrypted during transport. Note that TLS works for both FlightSQL and our normal APIs. Closes #25774
jdstrand
approved these changes
Apr 10, 2025
Contributor
There was a problem hiding this comment.
I'll let other review the implementation but I can say I don't see any issues:
- uses
rustls(viahyper-rustlscreate). This is a solid choice: has tls1.2 and higher support (ie, these are safe algorithms), potential for FIPS configuration, etc. We have (mostly) standardized on this in our rust code (a good thing) - supports a custom CA (good for testing, but also good for users that need it (eg, k8s, on-prem CA, etc))
- able to specify a cert and private key via filenames (as opposed to S3 (good, can't be accidentally exposed via API))
- has various tests that verify the functionality
Observations:
- The test pem file has a time bomb:
I suggest a much longer timeout or better yet, creating these dynamically. It would be nice to have expiration tests in the code too.
$ openssl x509 -in /tmp/pem -text -noout Validity Not Before: Apr 9 22:27:29 2025 GMT Not After : Apr 9 22:27:29 2026 GMT - I might have missed them, but it would be good to add negative tests (eg, bad certs) if you haven't already. It would be nice to have a client try to downgrade to tls1.1 and fail, but that is probably overkill since
hyper-rustlslikely won't ever implement 1.1. - I might've missed them, but it would be good to ensure that non-TLS connections still work as expected.
- It would be nice to be able to force only tls1.3 (customers will want it and
hyper-rustlssupports disabling 1.2), but this can be in a followup PR.
None of the observations are blockers from me, but it would be nice to add some of the tests if you haven't already.
Contributor
Author
|
I'm happy to open up some issues for follow up work in the coming week or so if there are no actual blockers, including dynamic cert gen etc. |
Contributor
None from me; all my observations can come as followups. |
Contributor
One other thing for your list of followups which is likely tied to the config file work: having to specify the custom CA in the client commands (when using a custom CA) will be annoying for some. Honoring an env var or a config file option would be nice here. |
Contributor
|
Btw, thanks for getting this in! :) |
This was referenced Apr 11, 2025
mgattozzi
added a commit
that referenced
this pull request
Apr 16, 2025
This commit is a follow up to #26246 and adds env vars to all of the TLS options in Core/Enterprise. This allows users to specify things like a CA file so that they don't need to run every single command with it specified. Same with the cert and key files. This is also nice for letting users configure environments like a Docker container with an env var rather than with command line args. Closes #26253
mgattozzi
added a commit
that referenced
this pull request
Apr 17, 2025
This commit is a follow up to #26246 and generates test certs on the fly for our test suite. In practice this will only need to be done once with a fresh repo check out as the certs will expire long after anyone would reasonably be working on this code in the year 4096! This could be extended in the future to generate negative tls tests where the file should be expired. Closes #26254
mgattozzi
added a commit
that referenced
this pull request
Apr 21, 2025
This commit is a follow up to #26246 and generates test certs on the fly for our test suite. In practice this will only need to be done once with a fresh repo check out as the certs will expire long after anyone would reasonably be working on this code in the year 4096! This could be extended in the future to generate negative tls tests where the file should be expired. Closes #26254
hiltontj
pushed a commit
that referenced
this pull request
May 2, 2025
This commit is a follow up to #26246 and generates test certs on the fly for our test suite. In practice this will only need to be done once with a fresh repo check out as the certs will expire long after anyone would reasonably be working on this code in the year 4096! This could be extended in the future to generate negative tls tests where the file should be expired. Closes #26254
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This commit adds TLS support to influxdb3 and allows users to pass in a path to a key and cert file with the --tls-key and --tls-cert flags in the serve command. It also adds the ability for every command to specify a certificate authority for requests. This is mostly needed when the cert is self signed, but there are other use cases for this.
The big thing is that most of our tests now use TLS by default. Included are self signed certs for localhost and the the CA cert included in the commit. Since these are only used for testing this should be fine to include as they are not used in nor are they intended to be used in any production system. The expiry has been set for 365 days and the file perms are set to o600 like the original issue mentioned. The tests pass with this restriction.
I've verified that the API works via curl with the self signed certs as I did not need to pass in the -k option to bypass checking the certs were valid. The same goes for our tests. They use the rootCA.pem file to verify the self signed cert when connecting and reject it otherwise.
With this users can be confident that their queries are safely encrypted during transport.
Note that TLS works for both FlightSQL and our normal APIs.
Closes #25774