Support other TLS modes than mutual auth in Client #3156

simonswine · 2020-09-10T13:56:55Z

What this PR does:

Increase TLS test coverage and support other TLS modes than mutual auth in Client by implementing explicit TLS enable flag.

This also adds a tls-server-name flag to require a specific server name instead of the hostname on the certificate.

Which issue(s) this PR fixes:

Fixes #3062 #3063

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

pkg/util/tls/tls_integration_test.go

simonswine · 2020-09-15T10:04:45Z

pkg/util/tls/tls_integration_test.go

+
+			// TODO: Investigate why the client doesn't really receive the
+			// error about the bad certificate from the server side and just
+			// see connection closed instead


Anyone having an idea how we can propagate the server error of "bad certificate" in the GRPC case?

grpc/grpc-go#622 (comment) suggests that you need to use a WithBlock() call option to get the certificate error. This might not be possible with the existing clients.

I will have to investigate a bit more but I have found that issue and tried WithBlock() and had the request being blocked for a while rather than having any change of error behaviour. I will take another look tomorrow

pracucci

Amazing, thanks! Changes LGTM. I just left a couple of nits and we should be good to go 🚀

pkg/util/tls/tls.go

pkg/util/tls/tls_integration_test.go

CHANGELOG.md

pkg/util/tls/tls.go

pkg/chunk/gcp/bigtable_index_client.go

pstibrany · 2020-09-25T13:48:58Z

pkg/util/tls/tls.go

@@ -74,12 +72,27 @@ func (cfg *ClientConfig) GetTLSConfig() (*tls.Config, error) {
 	return config, nil
 }

+type ClientConfigWithEnabled struct {
+	ClientConfig `yaml:",inline"`
+	Enabled      bool `yaml:"tls_enabled"`


I expected to see Enabled flag in grpcclient.Config, and then using that instead of current ClientConfigWithEnabled. WDYT? (It may require passing tlsEnabled flag to GetGRPCDialOptions)

I actually have implemented it that way the first time around.

It ended up being quite tedious in terms of duplicated work and passing the boolean around.

Esp. in places where there is not a full grpcclient.Config used, like here:

https://github.com/cortexproject/cortex/blob/342e17c592e6c63c3d01227732a7ea0ebdb27111/pkg/querier/querier.go#L63
or there:
https://github.com/cortexproject/cortex/blob/342e17c592e6c63c3d01227732a7ea0ebdb27111/pkg/querier/blocks_store_replicated_set.go#L40

Wdyt if we maybe call it tls.GRPCClientConfig instead of tls.ClientConfigWithEnabled as this is more accurate in the way we use it

pkg/util/tls/tls.go

CHANGELOG.md

pracucci · 2020-10-26T18:51:51Z

pkg/util/tls/tls.go

 	f.BoolVar(&cfg.InsecureSkipVerify, prefix+".tls-insecure-skip-verify", false, "Skip validating server certificate.")
 }

 // GetTLSConfig initialises tls.Config from config options
 func (cfg *ClientConfig) GetTLSConfig() (*tls.Config, error) {
-	// no tls config given at all
-	if cfg.CertPath == "" && cfg.KeyPath == "" && cfg.CAPath == "" {


Isn't this a breaking change? What's the impact of this change for components that are directly using ClientConfig? Before we were retuning nil (TLS disabled) while now we do return tls.Config{} (TLS enabled).

My understanding is that for clients using tls.Dial which includes HTTP clients, the behaviour is the exact same if it is set to nil or &tls.Config{}:

https://github.com/golang/go/blob/1095dd6339dbaf8d7c92214396c0a4dbcfa38521/src/crypto/tls/tls.go#L201

pkg/util/tls/tls.go

pracucci · 2020-10-26T18:57:47Z

pkg/util/grpcclient/grpcclient.go

@@ -27,6 +27,8 @@ type Config struct {

 	BackoffOnRatelimits bool               `yaml:"backoff_on_ratelimits"`
 	BackoffConfig       util.BackoffConfig `yaml:"backoff_config"`
+
+	TLS tls.ClientConfigWithEnabled `yaml:",inline"`


This change is non an innocuous refactoring. The problem is that doing this change we add TLS to any gRPC client, even the ones for which we don't support it. For example BigTable client (see generated doc).

This was done on on purpose as an outcome of this thread (#3156 (comment)), so there's an unified grpcclient.Config. We ensure that we enable TLS on the bigtable client by default, as TLS is on for bigtable by default in Cortex today.

I am not too sure if I caught every usage, but we should respect the TLS arguments (but I am not too sure if we want to support them according to your comment).

CHANGELOG.md

This allows to override the expected server name during TLS server validation. This simplifies the TLS setup as a ServerName can be more predictable than for example IP addresses. Fixes cortexproject#3063 Improve TLS client test coverage Add integration tests that spin up a HTTP/GRPC server and verify that the client options behave in the expected way. Allow configuration of non-mutual TLS Explicitly enable TLS in the client with the flag `-<prefix>.tls-enabled`. This flag is implicitly enabled when any other TLS flag is set. This flag will only be respected by the GRPC client, as for the HTTP client the scheme used in the URL will take precedence. Signed-off-by: Christian Simon <simon@swine.de> Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

pstibrany · 2021-01-28T14:28:52Z

I've rebased this patch on top of current master, and did the changes suggested in review comments:

we should remove "tls_enabled" flag from tls.ClientConfig struct.

for grpc clients, we should put "tls_enabled" bool flag into grpcclient.Config, and merge grpcclient.ConfigWithTLS with grpcclient.Config

etcd client which has its own config should embed tls.ClientConfig instead, but keep tls_enabled flag.

I haven't updated ruler to use tls when communicating with AM, since #3752 is already doing that.

pracucci

Thanks a lot @pstibrany for taking over this work 👏 A part form a couple of non blocking nits, I've the feeling we're introducing a regression in the configs client. Could you check it, please?

pkg/querier/blocks_store_replicated_set.go

pkg/querier/store_gateway_client.go

pkg/ring/kv/etcd/etcd.go

pracucci · 2021-01-29T14:02:51Z

pkg/util/tls/tls.go

@@ -29,18 +30,15 @@ func (cfg *ClientConfig) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet)
 	f.StringVar(&cfg.CertPath, prefix+".tls-cert-path", "", "Path to the client certificate file, which will be used for authenticating with the server. Also requires the key path to be configured.")
 	f.StringVar(&cfg.KeyPath, prefix+".tls-key-path", "", "Path to the key file for the client certificate. Also requires the client certificate to be configured.")
 	f.StringVar(&cfg.CAPath, prefix+".tls-ca-path", "", "Path to the CA certificates file to validate server certificate against. If not set, the host's root CA certificates are used.")
+	f.StringVar(&cfg.ServerName, prefix+".tls-server-name", "", "Override the expected name on the server certificate.")
 	f.BoolVar(&cfg.InsecureSkipVerify, prefix+".tls-insecure-skip-verify", false, "Skip validating server certificate.")
 }

 // GetTLSConfig initialises tls.Config from config options
 func (cfg *ClientConfig) GetTLSConfig() (*tls.Config, error) {


This function is also used by pkg/configs/client/client.go. Before it was returning nil if TLS was disabled, while now it always returns a config. Could you check the impact on the configs client? I've the feeling we're introducing a regression here.

If operator doesn't set any TLS options, then I think that returned structure will work fine even when using TLS in configs.
Defaults we are setting are good for client-side use. (For server-side, Certificates shouldn't be nil).

I just tested http client with no Transport and Transport: &http.Transport{TLSClientConfig: tlsCfg} (where tlsCfg is result of calling GetTLSConfig on empty cortex_tls.ClientConfig) against public site using https, and I'm getting the same 200 response in both cases.

I think integration test in tls_integration_test.go covers this too.

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

pracucci

Thanks Peter for checking it. LGTM.

pull-request-size bot added the size/L label Sep 10, 2020

simonswine force-pushed the tls-server-name branch 2 times, most recently from e5b0a04 to c85522e Compare September 15, 2020 08:42

pull-request-size bot added size/XL and removed size/L labels Sep 15, 2020

simonswine force-pushed the tls-server-name branch 2 times, most recently from 6c4719b to 8a4e74c Compare September 15, 2020 10:02

simonswine changed the title ~~WIP: Support other TLS modes than mutual auth in Client~~ Support other TLS modes than mutual auth in Client Sep 15, 2020

simonswine marked this pull request as ready for review September 15, 2020 10:03

simonswine commented Sep 15, 2020

View reviewed changes

pkg/util/tls/tls_integration_test.go Outdated Show resolved Hide resolved

simonswine commented Sep 15, 2020

View reviewed changes

pracucci approved these changes Sep 15, 2020

View reviewed changes

pkg/util/tls/tls.go Outdated Show resolved Hide resolved

pkg/util/tls/tls.go Outdated Show resolved Hide resolved

pkg/util/tls/tls_integration_test.go Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

simonswine force-pushed the tls-server-name branch 2 times, most recently from 90ab965 to 098c8ce Compare September 15, 2020 16:40

pracucci reviewed Sep 15, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

simonswine force-pushed the tls-server-name branch from 098c8ce to e63fe3a Compare September 16, 2020 08:39

pstibrany reviewed Sep 17, 2020

View reviewed changes

pkg/util/tls/tls.go Outdated Show resolved Hide resolved

pstibrany mentioned this pull request Sep 18, 2020

Mark TLS Configuration in gRPC, HTTP and etcd clients as experimental. #3198

Merged

simonswine force-pushed the tls-server-name branch 2 times, most recently from 0d968c5 to 1edefd1 Compare September 24, 2020 11:59

pstibrany reviewed Sep 25, 2020

View reviewed changes

This was referenced Oct 19, 2020

Expose TLS server name parameter in TLS client options #3063

Closed

Support other TLS modes than mutual auth in Client #3062

Closed

simonswine force-pushed the tls-server-name branch from 1edefd1 to 342e17c Compare October 26, 2020 18:13

pracucci reviewed Oct 26, 2020

View reviewed changes

simonswine force-pushed the tls-server-name branch 3 times, most recently from c00866b to 05ce847 Compare October 28, 2020 08:47

pracucci reviewed Dec 10, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

pstibrany force-pushed the tls-server-name branch from 05ce847 to 1243221 Compare January 28, 2021 14:15

pstibrany added 2 commits January 28, 2021 15:17

Mention that insecure connection is used when tls is disabled for gRPC.

9a00ef9

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

Use tls.ClientConfig for etcd client.

10e288e

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

pstibrany requested a review from pracucci January 28, 2021 14:29

pracucci reviewed Jan 29, 2021

View reviewed changes

pracucci mentioned this pull request Jan 29, 2021

feat: add TLS support for communication from the Ruler-->AM #3752

Merged

3 tasks

Review feedback.

4f3890b

Signed-off-by: Peter Štibraný <peter.stibrany@grafana.com>

pracucci approved these changes Jan 29, 2021

View reviewed changes

pstibrany approved these changes Jan 29, 2021

View reviewed changes

pstibrany merged commit eb6a6f5 into cortexproject:master Jan 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support other TLS modes than mutual auth in Client #3156

Support other TLS modes than mutual auth in Client #3156

simonswine commented Sep 10, 2020 •

edited

simonswine Sep 15, 2020

MichelHollands Sep 15, 2020

simonswine Sep 15, 2020

pracucci left a comment

pstibrany Sep 25, 2020

simonswine Oct 26, 2020

pracucci Oct 26, 2020

simonswine Oct 27, 2020 •

edited

pracucci Oct 26, 2020

simonswine Oct 27, 2020

pstibrany commented Jan 28, 2021 •

edited

pracucci left a comment

pracucci Jan 29, 2021

pstibrany Jan 29, 2021

pstibrany Jan 29, 2021

pstibrany Jan 29, 2021

pracucci left a comment

Support other TLS modes than mutual auth in Client #3156

Support other TLS modes than mutual auth in Client #3156

Conversation

simonswine commented Sep 10, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonswine Oct 27, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pstibrany commented Jan 28, 2021 • edited

pracucci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

simonswine commented Sep 10, 2020 •

edited

simonswine Oct 27, 2020 •

edited

pstibrany commented Jan 28, 2021 •

edited