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

HTTP: Add TLS version configurability for grafana server #67482

Merged
merged 18 commits into from May 8, 2023

Conversation

venkatbvc
Copy link
Contributor

@venkatbvc venkatbvc commented Apr 28, 2023

Backend: Add TLS version configurability for grafana server. Also backward compatibility is supported. A new parameter min_tls_version are added to the [server] section.

What is this feature?

Currently TLS version and ciphers are hardcoded for http_server. This PR adds configurability from the user point of view.

Why do we need this feature?

User doesn't flexibility to add ciphers and fix version of TLS that needs to be used.

Who is this feature for?

For all

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

Add TLS version and cipher configurability for grafana server.
Also backward compatibility is supported. Two new parameters min_tls_version and tls_ciphers
are added to the [server] section.
@venkatbvc venkatbvc requested review from torkelo, a team and chri2547 as code owners April 28, 2023 09:47
@venkatbvc venkatbvc requested review from IevaVasiljeva, danielkenlee, sakjur, zserge and suntala and removed request for a team April 28, 2023 09:47
@grafanabot grafanabot added area/backend type/docs pr/external This PR is from external contributor labels Apr 28, 2023
Add TLS version and cipher configurability for grafana server.
Also backward compatibility is supported. Two new parameters min_tls_version and tls_ciphers
are added to the [server] section.
@CLAassistant
Copy link

CLAassistant commented Apr 28, 2023

CLA assistant check
All committers have signed the CLA.

@venkatbvc venkatbvc changed the title Add TLS version and cipher configurability for grafana server Feature: Add TLS version and cipher configurability for grafana server Apr 28, 2023
@venkatbvc
Copy link
Contributor Author

@torkelo How do i add the following labels:
nobackport
milestone can be next minor version
Changelog - i have updated document. required document, but am not sure whether it needs to be part of changelog.md

@venkatbvc venkatbvc marked this pull request as draft April 28, 2023 16:43
@venkatbvc venkatbvc marked this pull request as ready for review April 28, 2023 16:45
@venkatbvc
Copy link
Contributor Author

Hi All,
Can anyone check and let me know what needs to be done for adding the required label for checks to pass

@@ -189,6 +189,22 @@ Folder that contains [provisioning]({{< relref "../../administration/provisionin

`http`,`https`,`h2` or `socket`

### min_tls_version

Minimum TLS version that needs to be used in TLS Handshake. Possible Values are TLS1.2 and TLS1.3.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Minimum TLS version that needs to be used in TLS Handshake. Possible Values are TLS1.2 and TLS1.3.
The TLS Handshake requires a minimum TLS version. The available options are TLS1.2 and TLS1.3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have made changes.

@chri2547 chri2547 self-requested a review May 3, 2023 15:14
Copy link
Collaborator

@chri2547 chri2547 left a comment

Choose a reason for hiding this comment

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

Docs approved.

venkatbvc and others added 2 commits May 3, 2023 23:25
Co-authored-by: Christopher Moyer <35463610+chri2547@users.noreply.github.com>
Co-authored-by: Christopher Moyer <35463610+chri2547@users.noreply.github.com>
@venkatbvc
Copy link
Contributor Author

@torkelo @suntala @zserge @danielkenlee @sakjur Can you please review and provide your comments

@sakjur sakjur removed this from the 10.0.x milestone May 4, 2023
@sakjur
Copy link
Contributor

sakjur commented May 4, 2023

This is fairly low prio right now, it missed our Grafana 10.0 window and it'd at best make it into 10.1.

I don't know if I think we should do this at all, I'm all for configuring the minimum TLS version (after all, if all your users are using modern browsers, it makes sense to only permit TLS 1.3), but for exact ciphersuites, I'm skeptical. Partly because we're unable to configure the ciphersuites for TLS 1.3 because that's not supported by Go upstream (golang/go#29349) and partly because detailed configuration of TLS is sort of what reverse proxies such as nginx and Caddy excels at.

Quick review:
If we're to merge this I don't think we should be linking into the Go codebase for documenting the available options (that list is unlikely to change very much, so I think we could copy it into our docs), and you should use github.com/grafana/grafana/pkg/util.SplitString for splitting the list items.

@TomsioncatiGraf
Copy link

TomsioncatiGraf commented May 4, 2023

Configurability of cipher suites
👍 Increased user optionality, normally good
👎 Go is very good at picking cipher suites, so generally bad for security overall

I'd suggest not making cipher suites configurable and trusting Go upstream

@venkatbvc
Copy link
Contributor Author

venkatbvc commented May 4, 2023

@sakjur @TomsioncatiGraf Thanks for your comments. I have started with only make tls version configurable. But when i checked at the ldap.toml these were made configurable. So i thought it may be a good idea to configure.
So if you think tls_ciphers better not to configure and leave what is configured in the code by default, i can change PR only to set min tls version. Let me know your view. so that i can push accordingly

@sakjur
Copy link
Contributor

sakjur commented May 4, 2023

@venkatbvc If we can start with just the TLS version, that’d be something I’d approve 🙂

@venkatbvc
Copy link
Contributor Author

@sakjur Thanks. I will publish changes only for tls version. tls_ciphers i can do later if required

chalapat and others added 2 commits May 4, 2023 21:44
 Changes to be committed:
	modified:   conf/defaults.ini
	modified:   conf/sample.ini
	modified:   docs/sources/setup-grafana/configure-grafana/_index.md
	modified:   pkg/api/http_server.go
	modified:   pkg/setting/setting.go
	modified:   pkg/setting/setting_test.go
	modified:   pkg/util/tls.go
	modified:   pkg/util/tls_test.go
@venkatbvc
Copy link
Contributor Author

@sakjur pushed code only for configuring tls version. Please check

@venkatbvc venkatbvc changed the title Feature: Add TLS version and cipher configurability for grafana server Feature: Add TLS version configurability for grafana server May 4, 2023
@sakjur
Copy link
Contributor

sakjur commented May 4, 2023

@venkatbvc Thank you! I'll put this on my todo to review 🙂

@sakjur sakjur self-assigned this May 4, 2023
@sakjur sakjur added this to the 10.1.x milestone May 4, 2023
@sakjur sakjur added the no-backport Skip backport of PR label May 4, 2023
@rpasche
Copy link

rpasche commented May 8, 2023

2 questions, as I stumbled on this PR while trying to configure ciphers to be used.

  1. I'm questioning, if this isn't an error? What, if I wanted to configure the minTLSVersion == 1.3? (https://github.com/grafana/grafana/pull/67482/files#diff-059f97ccda689ebea0e2f9981cd736d77b1fa01a719d95abae333ad40fb0b9c4R744)
  2. Although we could then set a minTLSVersion to be configurable, the list of ciphers (attached to each version) still looks unchangeable to me. Or am I missing something?

@venkatbvc
Copy link
Contributor Author

venkatbvc commented May 8, 2023

@rpasche Initially, i have made tls version and ciphers configurable. Later, it was found that tlsCiphers setting have issues(Check this comment: #67482 (comment)) . So removed setting the TLS ciphers. only TLS version is configurable now.
If TLS version is set to 1.3, then crpto/tls will choose the default ciphers.
Following is the excerpt from crypto/tls:

// CipherSuites is a list of enabled TLS 1.0–1.2 cipher suites. The order of
// the list is ignored. Note that TLS 1.3 ciphersuites are not configurable.
//
// If CipherSuites is nil, a safe default list is used. The default cipher
// suites might change over time.
CipherSuites [][uint16]

@rpasche
Copy link

rpasche commented May 8, 2023

@rpasche Initially, i have made tls version and ciphers configurable. Later, it was found that tlsCiphers setting have issues(Check this comment: #67482 (comment)) . So removed setting the TLS ciphers. only TLS version is configurable now. If TLS version is set to 1.3, then crpto/tls will choose the default ciphers. Following is the excerpt from crypto/tls:

// CipherSuites is a list of enabled TLS 1.0–1.2 cipher suites. The order of // the list is ignored. Note that TLS 1.3 ciphersuites are not configurable. // // If CipherSuites is nil, a safe default list is used. The default cipher // suites might change over time. CipherSuites [][uint16]

@venkatbvc thank you for this explanation.

Copy link
Contributor

@sakjur sakjur left a comment

Choose a reason for hiding this comment

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

Tested this locally and it works great, thanks for your contribution 🎉

@sakjur sakjur merged commit b9e53f6 into grafana:main May 8, 2023
15 checks passed
@sakjur sakjur added add to changelog and removed no-changelog Skip including change in changelog/release notes labels May 8, 2023
@sakjur sakjur changed the title Feature: Add TLS version configurability for grafana server HTTP: Add TLS version configurability for grafana server May 8, 2023
@ricky-undeadcoders ricky-undeadcoders modified the milestones: 10.1.x, 10.1.0 Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/backend no-backport Skip backport of PR pr/external This PR is from external contributor type/docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants