feat: Allow setting a minimum TLS version#26307
Merged
Conversation
This commit allows users to set a minimum TLS version. The default is 1.2. The choices are TLS 1.2 or TLS 1.3 which can be set via env var: INFLUXDB3_TLS_MINIMUM_VERSION="tls-1.2" or INFLUXDB3_TLS_MINIMUM_VERSION="tls-1.3" and for the command line flag for the serve command: --tls-minimum-version tls-1.2 or --tls-minimum-version tls-1.3 With this users have more fine grained control over what tls version they require. Closes #26255
hiltontj
approved these changes
Apr 22, 2025
Contributor
hiltontj
left a comment
There was a problem hiding this comment.
The changes look good. I'll approve but some automated tests would be nice (see below)
It looks like you can specify a min/max TLS version for a reqwest::Client. So, you might be able to spin up the server in a CLI test and check using a hand-rolled reqwest client, similar to what you did using curl in the PR description.
Contributor
Author
|
Opened up an issue to keep track of it here |
mgattozzi
added a commit
that referenced
this pull request
Apr 24, 2025
This is a follow on to #26307. In this commit we add a test where we check that connections only pass if TLS is set to v1.3. The default is 1.2 and other tests connect with that just fine. In this test we spin up a server using only v1.3 as the minimum and try to connect with v1.2 which we expect to fail and then v1.3 which should pass. Closes #26308
mgattozzi
added a commit
that referenced
this pull request
Apr 24, 2025
* feat: Add a negative cert test This adds a test that will panic on server startup because connections to said server are invalid. We add a bad expired cert to our cert generation for usage in our tests. Note that this test is only really valid if other tests pass as it depends on waiting for the server start checks to fail. If other tests run then their server started fine and so did this one, the only difference being that connections will error due to a bad tls cert. Closes #26256 * feat: Add minimum TLS version test This is a follow on to #26307. In this commit we add a test where we check that connections only pass if TLS is set to v1.3. The default is 1.2 and other tests connect with that just fine. In this test we spin up a server using only v1.3 as the minimum and try to connect with v1.2 which we expect to fail and then v1.3 which should pass. Closes #26308
hiltontj
pushed a commit
that referenced
this pull request
May 2, 2025
This commit allows users to set a minimum TLS version. The default is 1.2. The choices are TLS 1.2 or TLS 1.3 which can be set via env var: INFLUXDB3_TLS_MINIMUM_VERSION="tls-1.2" or INFLUXDB3_TLS_MINIMUM_VERSION="tls-1.3" and for the command line flag for the serve command: --tls-minimum-version tls-1.2 or --tls-minimum-version tls-1.3 With this users have more fine grained control over what tls version they require. Closes #26255
hiltontj
pushed a commit
that referenced
this pull request
May 2, 2025
* feat: Add a negative cert test This adds a test that will panic on server startup because connections to said server are invalid. We add a bad expired cert to our cert generation for usage in our tests. Note that this test is only really valid if other tests pass as it depends on waiting for the server start checks to fail. If other tests run then their server started fine and so did this one, the only difference being that connections will error due to a bad tls cert. Closes #26256 * feat: Add minimum TLS version test This is a follow on to #26307. In this commit we add a test where we check that connections only pass if TLS is set to v1.3. The default is 1.2 and other tests connect with that just fine. In this test we spin up a server using only v1.3 as the minimum and try to connect with v1.2 which we expect to fail and then v1.3 which should pass. Closes #26308
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
2 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 allows users to set a minimum TLS version. The default is 1.2. The choices are TLS 1.2 or TLS 1.3 which can be set via env var:
INFLUXDB3_TLS_MINIMUM_VERSION="tls-1.2"
or
INFLUXDB3_TLS_MINIMUM_VERSION="tls-1.3"
and for the command line flag for the serve command:
--tls-minimum-version tls-1.2
or
--tls-minimum-version tls-1.3
With this users have more fine grained control over what tls version they require.
Closes #26255
Below are some curl commands showing that this works. This one shows that 1.3 is the minimum
And this one is 1.2
