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

Add configuration to set minimum TLS version accepted by the metrics server port. #666

Merged
merged 2 commits into from
Jun 17, 2022

Conversation

bjosv
Copy link
Contributor

@bjosv bjosv commented Jun 17, 2022

This adds the configuration tls-server-min-version to be able to set the minimum TLS version that will be accepted by the metrics server endpoint.

Since TLS 1.0 and 1.1 have reached end-of-life they are normally not allowed within some organisations, but Go still enables it by default on TLS servers.
Due to the deprecation this PR also changes the default behavior and rejects clients that request to only use the old less secure TLS versions. This can be opened again by the configuration (setting tls-server-min-version to TLS1.0).

This PR also contains a commit to fix the local testruns using the makefile.
Maybe we should add a run of make docker-all in CI?

Seen when running testcase `TestLatencyStats`, which failed
with a connection refused.
This adds the configuration `tls-server-min-version` to be able
to set the minimum TLS version that will be accepted by the metrics
server port.
Since TLS 1.0 and 1.1 have reached end-of-life they are normally not allowed
within some organisations, but Go enables it by default on TLS servers.
With this new configuration only TLS1.2/TLS1.3 clients are accepted by default,
but can be opened up to accept the less secure version via config.
@coveralls
Copy link

Pull Request Test Coverage Report for Build 1594

  • 6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.02%) to 92.111%

Totals Coverage Status
Change from base Build 1592: 0.02%
Covered Lines: 1798
Relevant Lines: 1952

💛 - Coveralls

@codecov
Copy link

codecov bot commented Jun 17, 2022

Codecov Report

Merging #666 (972513e) into master (37d4552) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #666      +/-   ##
==========================================
+ Coverage   88.15%   88.18%   +0.03%     
==========================================
  Files          16       16              
  Lines        1941     1946       +5     
==========================================
+ Hits         1711     1716       +5     
  Misses        154      154              
  Partials       76       76              
Impacted Files Coverage Δ
exporter/tls.go 100.00% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

Copy link
Owner

@oliver006 oliver006 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, change looks good and thx for fixing the docker-compose file.

@@ -15,7 +15,7 @@ services:

redis7:
image: redis:7.0
command: "redis-server --protected-mode no --dbfilename dump7.rdb"
command: "redis-server --port 6384 --protected-mode no --dbfilename dump7.rdb"
Copy link
Owner

Choose a reason for hiding this comment

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

👍

@oliver006 oliver006 merged commit 980a974 into oliver006:master Jun 17, 2022
@bjosv bjosv deleted the tls-v1.2-minimum branch June 17, 2022 15:32
@oliver006
Copy link
Owner

Released as https://github.com/oliver006/redis_exporter/releases/tag/v1.43.0

@bjosv
Copy link
Contributor Author

bjosv commented Jun 18, 2022

Released as https://github.com/oliver006/redis_exporter/releases/tag/v1.43.0

You are the best. Thanks!

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.

3 participants