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

feat: Implement minimum TLS version for clients #11493

Merged
merged 13 commits into from
Aug 9, 2022

Conversation

srebhan
Copy link
Member

@srebhan srebhan commented Jul 12, 2022

resolves #8699
resolves #8171
resolves #8124 (at least partially)

replaces #8959

All credits for this PR go to @jdstrand! This PR allows to set a minimum TLS version required by the client for all plugins that use common/tls to configure their TLS setup. This includes, but is probably not limited to, inputs.http, inputs.gnmi and inputs.jti_openconfig_telemetry. This allows to change Golang's default of TLS 1.2 in both directions, i.e. downgrade (which implies some security risk) as well as setting it to TLS 1.3.
Unit-tests are added by the PR to check if lower server versions are rejected successfully.

@telegraf-tiger telegraf-tiger bot added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Jul 12, 2022
@srebhan
Copy link
Member Author

srebhan commented Jul 27, 2022

Made v1.2 the new explicit default for server and client connections. Users that require lower version can revert the used version on a per-plugin basis using tls_min_version. Please check the Breaking changes documentation in the Changelog. This should apply for the release containing this change (i.e. v1.24.0 IIRC)...

Copy link
Contributor

@powersj powersj 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 driving this.

My only concern is around a user knowing what to put in for the tls_min_version value. My naive assumption reading the changelog was they would put "1.0" or "1.2" Not the "TLS12" I later saw in the READMEs and sample configs.

Is this something to address? Either via better comment or using the other mechanism?

@srebhan
Copy link
Member Author

srebhan commented Jul 27, 2022

Good point. I just reused the values available for tlsVersionMap keys that already exist. Should I enumerate all available values or do you have another idea on how to describe this better?

@srebhan
Copy link
Member Author

srebhan commented Jul 27, 2022

Another option would be to add v1.0 etc to the keys linked above or print them on error...

@powersj
Copy link
Contributor

powersj commented Aug 3, 2022

I like the printing of the available options on error. It makes it very clear what versions we recognize.

Thanks!

Copy link
Contributor

@powersj powersj left a comment

Choose a reason for hiding this comment

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

+1 - thank you, before merging we do want a +1 from security

@srebhan
Copy link
Member Author

srebhan commented Aug 3, 2022

Thanks @powersj. I agree, will wait for @jdstrand to do his magic once he has some time... ;-)

Copy link
Contributor

@jdstrand jdstrand left a comment

Choose a reason for hiding this comment

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

This looks great! I particularly like the tests that do a real connection based on telegraf's ClientConfig and ServerConfig structs (it made me wonder if we could start up an http_listener as the server and then use some other input plugin as the client, but that isn't a blocker for this PR).

In addition to the PR review, I manually tested tls_min_version with both inputs.http (client) and inputs.http_listener_v2 (server) and it worked as expected.

plugins/common/tls/config.go Show resolved Hide resolved
plugins/common/tls/config.go Show resolved Hide resolved
plugins/common/tls/config_test.go Show resolved Hide resolved
plugins/common/tls/config_test.go Show resolved Hide resolved
@telegraf-tiger
Copy link
Contributor

telegraf-tiger bot commented Aug 5, 2022

@srebhan
Copy link
Member Author

srebhan commented Aug 5, 2022

@powersj and @jdstrand, may I ask for another approval?

Copy link
Contributor

@jdstrand jdstrand 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 updates! (though the linter still seems unhappy)

@srebhan
Copy link
Member Author

srebhan commented Aug 8, 2022

@jdstrand that's because I manually put the "breaking change" documentation to the Changelog. Not sure how to do it with a happy linter. :-)

@srebhan srebhan requested a review from powersj August 8, 2022 19:27
@srebhan srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Aug 9, 2022
@srebhan srebhan merged commit e96bbe8 into influxdata:master Aug 9, 2022
@mohsin106
Copy link

Hi,
I'm using the Telegraf version 1.23.4-alpine Docker image and still experiencing an issue connecting when setting the tls_max_version parameter.
Error:
2022-08-26T15:15:54Z E! [telegraf] Error running agent: Error loading config file /etc/telegraf/telegraf.conf: plugin inputs.jti_openconfig_telemetry: line 14: configuration specified the fields ["tls_max_version"], but they weren't used

This is my input JTI stanza:

[[inputs.jti_openconfig_telemetry]]
  servers = ["router1.mgt.net:50051"]
  sensors = ["interfaces /interfaces/interface","lldp /lldp","optics /junos/system/linecard/optics/"] 
  sample_frequency = "60000ms"
  username = "$user"
  password = "$password"
  client_id = "$containerName"
  enable_tls = true
  tls_ca = "/etc/telegraf/router_ca.pem"
  insecure_skip_verify = false
  tls_max_version = "TLS12"

Also, tried putting the tls_max_version under the agent settings:

[agent]
  round_interval = true
  metric_batch_size = 1000
  metric_buffer_limit = 15000
  flush_interval = "10s"
  debug = false
  quiet = false
  tls_max_version = "TLS12"
  hostname = "lab-ptx-deployment"
  omit_hostname = false

But get same error:

2022-08-26T15:25:14Z E! [telegraf] Error running agent: Error loading config file /etc/telegraf/telegraf.conf: line 1: configuration specified the fields ["tls_max_version"], but they weren't used

telegraf-alpine:1.13.3 continues to work, anything newer gives the above errors.

@srebhan
Copy link
Member Author

srebhan commented Sep 20, 2022

@mohsin106 this PR adds a TLS minimum version, but you are trying to configure a maximum version. Please open a new issue when you need this additional feature!

@srebhan srebhan deleted the tls_min_version branch November 7, 2022 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
5 participants