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

TLS config: Enable selection of min TLS version #375

Merged
merged 2 commits into from Apr 19, 2022

Conversation

roidelapluie
Copy link
Member

go1.18 changes the default minimum TLS version to 1.2.

Let's make the default minimum version configurable, while following go
default.

The allowed values (TLS10, ..) come from the exporter-toolkit:
https://github.com/prometheus/exporter-toolkit/blob/master/docs/web-configuration.md

TLSVersion is exported so the exporter toolkit can reuse them later.

Signed-off-by: Julien Pivotto roidelapluie@o11y.eu

go1.18 changes the default minimum TLS version to 1.2.

Let's make the default minimum version configurable, while following go
default.

The allowed values (TLS10, ..) come from the exporter-toolkit:
https://github.com/prometheus/exporter-toolkit/blob/master/docs/web-configuration.md

TLSVersion is exported so the exporter toolkit can reuse them later.

Signed-off-by: Julien Pivotto <roidelapluie@o11y.eu>
Copy link
Member

@Nexucis Nexucis left a comment

Choose a reason for hiding this comment

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

Overall it looks ok. I put a couple of comments / notes.

config/http_config.go Show resolved Hide resolved
@@ -714,6 +750,8 @@ type TLSConfig struct {
ServerName string `yaml:"server_name,omitempty" json:"server_name,omitempty"`
// Disable target certificate validation.
InsecureSkipVerify bool `yaml:"insecure_skip_verify" json:"insecure_skip_verify"`
// Minimum TLS version.
MinVersion TLSVersion `yaml:"min_version,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

it is missing here the json tag

Suggested change
MinVersion TLSVersion `yaml:"min_version,omitempty"`
MinVersion TLSVersion `yaml:"min_version,omitempty" json:"min_version,omitempty"`

Copy link
Member

Choose a reason for hiding this comment

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

if MinVersion is not mandatory, don't you want to provide a default value ?

You could do it by overriding the Unmarshal's method.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I want to use go default. I think we should let them define the default while providing our users a way to divert if really needed.

Copy link
Member

Choose a reason for hiding this comment

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

mmm ok. And I guess in Prometheus you will define that the default / Minimum version is the previous one used, right ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, the plan is to mark it as a CHANGE in the changelog.

I think out users will benefit from defaults that go runtime provides. We should probably not make the call.

Possibly it can break some users, but they have the option to opt-out this change by explicitly setting tls1.0 as supported version.

Copy link
Member

Choose a reason for hiding this comment

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

I can agree on this assumption and I agree it's better to use the default tls version supported by Go.
I'm just wondering how much it will impact the users.
Let's see if someone answers to your email.

Signed-off-by: Julien Pivotto <roidelapluie@o11y.eu>
Copy link
Member

@Nexucis Nexucis left a comment

Choose a reason for hiding this comment

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

thanks !

@roidelapluie roidelapluie merged commit 3763a1d into prometheus:main Apr 19, 2022
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.

None yet

2 participants