common/tls: Allow specifying SNI hostnames by antifuchs · Pull Request #7897 · influxdata/telegraf

antifuchs · 2020-07-25T18:40:36Z

This PR adds a new configration field sni_server_name that allows specifying the server name that'll be sent in the ClientHello when telegraf makes an HTTP request to TLS servers. This allows checking against load balancers responding to specific hostnames that otherwise wouldn't resolve to their addresses, or other more complex TLS configurations.

Fixes #7598.

Required for all PRs:

Signed CLA.
Associated README.md updated.
Has appropriate unit tests.

antifuchs · 2020-07-25T18:44:35Z

The idea behind this PR is to test with something like the following setting:

[[inputs.http_response]]
insecure_skip_verify = false
response_string_match = "<title>Login</title>"
response_timeout = "10s"
urls = [
    "https://load-balancer-a.internal.example.com/login/",
    "https://load-balancer-b.internal.example.com/login/",
]
tls_ca = "/run/secrets/tls-cacert"
sni_server_name = "www.extremely-important-apex.com"
[inputs.http_response.headers]
Host = "www.extremely-important-apex.com"

In this example, load-balancer-a and -b are internal addresses and don't get resolved by www.extremely-important-apex.com; but they require the correct SNI server name to serve the content that they should serve (as well as the right Host header).

Hope that makes sense!

srebhan · 2020-12-11T15:50:13Z

I think this is not the correct approach. We rather should add this option to the tls.ClientConfig so it is available for all plugins using TLS.
@antifuchs are you willing to look into this!?

antifuchs · 2020-12-12T04:28:24Z

Huh, I didn't realize there was a common way to configure plugins' TLS client config. I'll look into it!

docs/TLS.md

Add a new configration field `tls_server_name` that allows specifying the server name that'll be sent in the ClientHello when telegraf makes a request to TLS servers. This allows checking against load balancers responding to specific hostnames that otherwise wouldn't resolve to their addresses. Add the setting to the documentation of common TLS options, as well as to the http_response plugin. Fixes influxdata#7598.

antifuchs · 2020-12-12T17:31:15Z

Updated! What do you think, @srebhan? (I personally think it is much nicer, thanks for the suggestion!)

srebhan · 2020-12-13T20:13:50Z

plugins/common/tls/config.go

 	// case of an HTTP plugin, you could use `https`.  Other plugins may need
 	// the dedicated option `TLSEnable`.
-	if c.TLSCA == "" && c.TLSKey == "" && c.TLSCert == "" && !c.InsecureSkipVerify {
+	if c.TLSCA == "" && c.TLSKey == "" && c.TLSCert == "" && !c.InsecureSkipVerify && c.ServerName == "" {


I don't think this is correct. If you neither specify a CA nor a KEY or CERT but a ServerName then this check will not trigger but I doubt it makes sense to check the servername without validating the certificate!?

The ServerName directive does not affect only the host name in certificate checking: It's also the hostname requested via SNI in the TLS ClientHello message - that's the thing that I need to customize that prompted me to submit the PR in the first place (:

Even if that weren't the case, an empty TLS Config is documented to use the host's root CA set if no root CAs are provided, so setting the ServerName value on an otherwise-empty TLS config would be meaningful.

Could you please put a comment there linking to the docu you mentioned. Just for the next one asking himself why this should make sense!? Thanks for the explanation!

Hm, so I re-read the TODO comment above & started auditing our code's usage of TLSConfig(), where I found out that the only case where a nil tls.Config matters (because the stdlib function tls.Client requires a non-nil value) is the x509_cert.go code... where we override the ServerName based on a configuration setting (incompatibly with the "common" ServerName setting now)!

So I'll have to fix the ServerName usage in x509_cert.go, but more importantly: I think we should remove that TODO, as it could be a pretty meaningful distinction in terms of "were there any settings that the operator customized" (but ultimately, it looks like there's not much of a difference). What do you think?

srebhan

One question in the code and a request to add a test-case for only adding a server-name but no other TLS configuration... :-)

This plugin has been using ServerName previously, and will have to deal with the new setting, too: Extract the server-name choosing into a method & add a test to ensure we choose the right value (and error under the right circumstances). Also document that the two settings are mutually exclusive.

Also get rid of the TODO, as I am fairly certain this behavior is the correct one.

antifuchs · 2020-12-16T03:25:24Z

@srebhan I think this should cover what we discussed above - lmk if you see any other things that need improving!

srebhan

Nice catch with the x509 plugin! Looks good to me.

sspaink

Great work @antifuchs ! I just added a minor comment.

sspaink · 2020-12-23T16:07:14Z

plugins/common/tls/config_test.go

+		client     tls.ClientConfig
+		expNil     bool
+		expErr     bool
+		serverName string


serverName doesn't seem to be used in this test. Can you remove it please?

Yup, vestige of a previous idea for a test - Removing it now.

* tls_config: Allow specifying SNI hostnames Add a new configration field `tls_server_name` that allows specifying the server name that'll be sent in the ClientHello when telegraf makes a request to TLS servers. This allows checking against load balancers responding to specific hostnames that otherwise wouldn't resolve to their addresses. Add the setting to the documentation of common TLS options, as well as to the http_response plugin. Fixes #7598. * Adjust the x509_cert to allow usage of tls_server_name This plugin has been using ServerName previously, and will have to deal with the new setting, too: Extract the server-name choosing into a method & add a test to ensure we choose the right value (and error under the right circumstances). Also document that the two settings are mutually exclusive. * Improve documentation on what we try to accomplish in the nil return Also get rid of the TODO, as I am fairly certain this behavior is the correct one. * Remove unused struct field in tests (cherry picked from commit 3c9c013)

* tls_config: Allow specifying SNI hostnames Add a new configration field `tls_server_name` that allows specifying the server name that'll be sent in the ClientHello when telegraf makes a request to TLS servers. This allows checking against load balancers responding to specific hostnames that otherwise wouldn't resolve to their addresses. Add the setting to the documentation of common TLS options, as well as to the http_response plugin. Fixes influxdata#7598. * Adjust the x509_cert to allow usage of tls_server_name This plugin has been using ServerName previously, and will have to deal with the new setting, too: Extract the server-name choosing into a method & add a test to ensure we choose the right value (and error under the right circumstances). Also document that the two settings are mutually exclusive. * Improve documentation on what we try to accomplish in the nil return Also get rid of the TODO, as I am fairly certain this behavior is the correct one. * Remove unused struct field in tests

antifuchs mentioned this pull request Jul 25, 2020

Override IP address in http_response #7598

Closed

sjwang90 added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Aug 18, 2020

sjwang90 added this to the Planned milestone Dec 9, 2020

srebhan self-assigned this Dec 11, 2020

antifuchs commented Dec 12, 2020

View reviewed changes

docs/TLS.md Show resolved Hide resolved

antifuchs force-pushed the use-sni-in-http-response branch from b50911f to 0e4e7bc Compare December 12, 2020 17:19

antifuchs force-pushed the use-sni-in-http-response branch from 0e4e7bc to 43f6526 Compare December 12, 2020 17:30

srebhan reviewed Dec 13, 2020

View reviewed changes

antifuchs changed the title ~~http_response: Allow specifying SNI hostnames~~ common/tls: Allow specifying SNI hostnames Dec 15, 2020

Improve documentation on what we try to accomplish in the nil return

ceb9c49

Also get rid of the TODO, as I am fairly certain this behavior is the correct one.

srebhan approved these changes Dec 16, 2020

View reviewed changes

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 Dec 16, 2020

sspaink reviewed Dec 23, 2020

View reviewed changes

Remove unused struct field in tests

f96b6f5

sspaink merged commit 3c9c013 into influxdata:master Dec 23, 2020

antifuchs deleted the use-sni-in-http-response branch December 28, 2020 00:08

MorphBonehunter mentioned this pull request Feb 26, 2021

inputs.x509_cert unexpected behavior with SNI #8914

Closed

Conversation

antifuchs commented Jul 25, 2020

Required for all PRs:

Uh oh!

antifuchs commented Jul 25, 2020

Uh oh!

srebhan commented Dec 11, 2020

Uh oh!

antifuchs commented Dec 12, 2020

Uh oh!

Uh oh!

antifuchs commented Dec 12, 2020

Uh oh!

srebhan Dec 13, 2020

Choose a reason for hiding this comment

Uh oh!

antifuchs Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srebhan Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antifuchs Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

srebhan left a comment

Choose a reason for hiding this comment

Uh oh!

antifuchs commented Dec 16, 2020

Uh oh!

srebhan left a comment

Choose a reason for hiding this comment

Uh oh!

sspaink left a comment

Choose a reason for hiding this comment

Uh oh!

sspaink Dec 23, 2020

Choose a reason for hiding this comment

Uh oh!

antifuchs Dec 23, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

antifuchs Dec 14, 2020 •

edited

Loading

srebhan Dec 14, 2020 •

edited

Loading