Add common TLS configuration by backjo · Pull Request #1838 · jaegertracing/jaeger

backjo · 2019-10-07T05:04:54Z

Signed-off-by: Jonah Back jonah@jonahback.com

Closes #1837

Which problem is this PR solving?

Makes TLS configuration standard across various storage systems

Short description of the changes

Moves TLS configurations into the tls package
Refactors cassandra/kafka/elasticsearch backends to use the common configuration

backjo · 2019-10-07T05:06:01Z

This change is

yurishkuro · 2019-10-07T06:04:57Z

@backjo Thanks!

I was also working on this on the plane, see #1840. It is slightly more configurable than your version, but so far I only switched gRPC client/server configs to it. Let me get it merged, and then you should be able to use that for storage configs.

NB We will need to add SkipHostVerify as another configurable option.

backjo · 2019-10-07T17:36:58Z

Sounds good 👍

yurishkuro · 2019-10-08T21:17:53Z

@backjo #1840 was merged, do you want to try to integrate it into this PR?

backjo · 2019-10-08T21:19:59Z

@yurishkuro yeah, I'll try to integrate it in tonight

pavolloffay

the current PR introduces breaking change from --storage.tls=true to --storage.tls.enabled=true
~~grpc reporter in agent also exposes TLS flags - could we use this new package there too?~~ - done in #1840

plugin/storage/es/options_test.go

pkg/config/tls/tls.go

pavolloffay · 2019-10-09T13:28:55Z

The problem with current TLS options is that due to --es.tls: true and other embedded options, the config cannot be defined in JSON. In yaml there is a workaround:

es:
    tls: true
es.tls.cert: "/foo/bar"

So let's deprecate the --es.tls and if it is used use it but create a warning message. The tls package should not add deprecated enabled flag for new components (e.g tls for kafka)

pkg/config/tls/tls.go

backjo · 2019-10-09T19:51:50Z

Yeah, this isn't ready for review yet with your changes - let me update when it is

yurishkuro · 2019-10-15T17:35:42Z

@backjo do you plan to continue this PR?

backjo · 2019-10-16T16:43:29Z

yep - will try to push what I have locally tonight - sorry for the delay!

…kend (jaegertracing#1845) * Revert Support for TLS as it will handled with issue jaegertracing#1838 Signed-off-by: somnath <somnath.ghosh@honeywell.com>

codecov · 2019-10-18T17:38:25Z

Codecov Report

Merging #1838 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1838      +/-   ##
==========================================
- Coverage   97.45%   97.42%   -0.03%     
==========================================
  Files         207      207              
  Lines       10314    10286      -28     
==========================================
- Hits        10051    10021      -30     
  Misses        221      221              
- Partials       42       44       +2

Impacted Files	Coverage Δ
cmd/agent/app/reporter/grpc/flags.go	`100% <ø> (ø)`	⬆️
cmd/collector/app/builder/builder_flags.go	`100% <ø> (ø)`	⬆️
plugin/storage/es/options.go	`100% <100%> (ø)`	⬆️
pkg/config/tlscfg/options.go	`100% <100%> (ø)`	⬆️
plugin/storage/cassandra/options.go	`100% <100%> (ø)`	⬆️
pkg/config/tlscfg/flags.go	`100% <100%> (ø)`	⬆️
cmd/query/app/static_handler.go	`86.84% <0%> (-1.76%)`	⬇️
plugin/storage/badger/spanstore/reader.go	`96.79% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b58b94...4863a22. Read the comment docs.

backjo · 2019-10-18T22:17:27Z

@yurishkuro @pavolloffay I've updated this and I believe it's ready for review.

Summary of changes:

Move Kafka/ES/C* TLS setup and configuration to use the shared TLS config.
Added SkipHostVerify as a TLS option
Added a tls.enabled: true flag that is the recommended flag for enabling tls. tls: true is now marked as deprecated.

backjo · 2019-10-23T16:21:15Z

@yurishkuro mind taking a look?

yurishkuro · 2019-10-24T04:57:15Z

I'll take a look, most likely over the weekend, a bit swamped this week

yurishkuro

looks great overall, +132 −256 is nice to see.

Could you please attach the diffs between before and after output of the help commands with different storage types for agent/collector/query/ingester (we may actually want to script it for future use)?

e.g. SPAN_STORAGE_TYPE=kafka go run ./cmd/collector help > collector-kafka.txt

This way it makes it much easier to look at the changes being introduced to the flags across the board.

yurishkuro · 2019-10-25T02:51:34Z

pkg/config/tlscfg/flags.go

 	var p Options
 	if c.ShowEnabled {
 		p.Enabled = v.GetBool(c.Prefix + tlsEnabled)
+


blank line not necessary

yurishkuro · 2019-10-25T02:51:45Z

pkg/config/tlscfg/flags.go

 	var p Options
 	if c.ShowEnabled {
 		p.Enabled = v.GetBool(c.Prefix + tlsEnabled)
+


no blank line

yurishkuro · 2019-10-25T02:55:21Z

pkg/es/config/config.go

-				return nil, err
-			}
-			httpTransport.TLSClientConfig = &tls.Config{RootCAs: ca}
-		}


what was this code doing? It seems deleting it will not preserve some functionality

Yeah, you're correct. adding it back.

yurishkuro · 2019-10-25T02:58:13Z

pkg/kafka/auth/config.go

-	config.TLS.KeyPath = v.GetString(configPrefix + tlsPrefix + suffixTLSKey)
+	var tlsClientConfig = tlscfg.ClientFlagsConfig{
+		Prefix:         configPrefix,
+		ShowEnabled:    true,


do kafka options actually need to show the enabled flag? There is a dedicated switch flag .authentication that indicates TLS or other auth types

yurishkuro · 2019-10-25T03:00:18Z

plugin/storage/cassandra/options.go

-					EnableHostVerification: true,
+				TLS: tlscfg.Options{
+					Enabled:        false,
+					SkipHostVerify: false,


this is just setting zero values, can omit the TLS key completely

yurishkuro · 2019-10-25T03:02:21Z

plugin/storage/cassandra/options.go

 }

 func (cfg *namespaceConfig) initFromViper(v *viper.Viper) {
+	var tlsFlagsConfig = tlscfg.ClientFlagsConfig{


please DRY it w/ L117, e.g. a helper function

func tlsFlagsConfig(namespace string) tlscfg.ClientFlagsConfig

yurishkuro · 2019-10-25T03:05:28Z

plugin/storage/es/options.go

 	return options
 }

+func (config *namespaceConfig) GetTLSFlagsConfig() *tlscfg.ClientFlagsConfig {


does it need to be public?

it can return config by value, we're not using pointers anywhere

pkg/config/tlscfg/flags.go

pavolloffay · 2019-12-19T11:35:15Z

Some of the flags look weird - two dots:

      --reporter.grpc..tls                                        (deprecated) see --reporter.grpc..tls.enabled
      --reporter.grpc..tls.ca string                              Path to a TLS CA (Certification Authority) file used to verify the remote server(s) (by default will use the system truststore)
      --reporter.grpc..tls.cert string                            Path to a TLS Certificate file, used to identify this process to the remote server(s)
      --reporter.grpc..tls.enabled                                Enable TLS when talking to the remote server(s)
      --reporter.grpc..tls.key string                             Path to a TLS Private Key file, used to identify this process to the remote server(s)
      --reporter.grpc..tls.server-name string                     Override the TLS server name we expect in the certificate of the remove server(s)
      --reporter.grpc..tls.skip-host-verify                       Skip TLS hostname verification

pavolloffay

The reporter grpc tls flags have two dots. At the moment it also breaks TLS handling in ES

pavolloffay · 2019-12-19T11:50:19Z

pkg/es/config/config.go

 		}
 	} else {
 		httpTransport := &http.Transport{
-			Proxy: http.ProxyFromEnvironment,


revert all the changes in this function except line 285. It breaks a couple of things e.g. 47d2029#diff-06fc7142fac7ac3ed9db42948f2558c8R306

yep, i've got a TODO still to restore this

pavolloffay · 2019-12-19T11:50:57Z

pkg/config/tlscfg/options.go

+	KeyPath        string
+	ServerName     string // only for client-side TLS config
+	ClientCAPath   string // only for server-side TLS config for client auth
+	SkipHostVerify bool


On line 50 add InsecureSkipVerify: p.SkipHostVerify,

pavolloffay · 2019-12-20T09:37:44Z

pkg/es/config/config.go

-			ctls := &TLSConfig{CaPath: c.TLS.CaPath}
-			ca, err := ctls.loadCertificate()
+		if c.TLS.CAPath != "" {
+			config, err := c.TLS.Config()


Here we want to load only CA cert. Does it load only CA. What happens is client cert or key are not specified?

I think this does the right thing, although it will try to load SystemCertPool if CAPath==""

yurishkuro

Please use ./scripts/generate-help-output.sh to compare flags between master and your branch, e.g. in master run

mkdir -p .tmp/v1
./scripts/generate-help-output.sh .tmp/v1

then in your branch

mkdir -p .tmp/v2
./scripts/generate-help-output.sh .tmp/v2

and then compare

diff -brw .tmp/v1 .tmp/v2

In particular, the following differences don't look correct:

>       --collector.grpc..tls.enabled                     Enable TLS on the server

yurishkuro · 2019-12-21T21:15:36Z

pkg/es/config/config.go

-			ctls := &TLSConfig{CaPath: c.TLS.CaPath}
-			ca, err := ctls.loadCertificate()
+		if c.TLS.CAPath != "" {
+			config, err := c.TLS.Config()


I think this does the right thing, although it will try to load SystemCertPool if CAPath==""

plugin/storage/cassandra/options.go

pkg/config/tlscfg/flags.go

yurishkuro · 2020-01-12T02:30:16Z

@backjo thanks for your patience, and sorry for the delay in reviewing. There are just a few small issues remaining:

(a) can you amend the commit b7d5be8 to include the sign-off?

(b) These flags are missing, need to restore and deprecate them:

       --cassandra-archive.tls.verify-host
       --cassandra.tls.verify-host

(c) sync with master

backjo · 2020-01-16T03:22:10Z

@yurishkuro will do - I think I'm going to rebase the changes into one commit and bring it current with master.

Re: verify-host - I'm a bit uncertain how we're going to handle conflicting behavior between the two, since the flags are inverted. Specifically, thinking about these cases:

User does not set --cassandra.tls.verify-host, which implicitly sets it to false. This default behavior conflicts with the default behavior of skip-host-verification, which would be to implicitly enable host verification.
User explicitly sets --cassandra.tls.verify-host to false. This could be backward-compatible / respected by using the IsSet call in Viper
User explicitly sets --cassandra.tls.verify-host to true. This aligns with the default value of skip-host-verification so the two would be in alignment. If both were explicitly set for some reason, we would respect skip-host-verification.

The first case is a bit tricky - the most backward-compatible way would be to keep the host verification off by default - which would mean that users would need to explicitly enable hostname verification with the new property, until some time later on when we decide that we should enable the more secure option by default.

yurishkuro · 2020-01-16T03:48:42Z

I think it is ok to change the behavior to verify bu default. We will document it in the release notes as a breaking change. The legacy option is needed to that people's start scripts don't break (they won't be affected by the default behavior change since they're already specifying the legacy flag explicitly).

yurishkuro · 2020-01-26T04:09:25Z

@backjo one last push to get this over the line?

Signed-off-by: Jonah Back <jonah_back@intuit.com>

Signed-off-by: Jonah Back <jonah@jonahback.com>

yurishkuro

Thank you! 🎉 🎉 🎉

yurishkuro · 2020-01-27T02:51:15Z

Gopkg.toml

 [[constraint]]
  name = "github.com/spf13/viper"
-  version = "^1.0.0"
+  version = "^1.5.0"


Is this change necessary? If we're changing this file, we need to run dep ensure --update and also check-in the lock file.

It is - there was a bug in Viper where isSet was returning true even in cases where it was not set, but there was a default value. This was fixed in 1.5

yurishkuro · 2020-01-27T02:52:04Z

@pavolloffay are you ok to approve/merge?

Signed-off-by: Jonah Back <jonah@jonahback.com>

pavolloffay · 2020-01-27T12:59:13Z

I have just tested the following ES configurations against elastic.co operator. So far all works as before.

to test skip-host-verify

cat <<EOF | kubectl apply -f -
apiVersion: jaegertracing.io/v1
kind: Jaeger
metadata:
  name: simple-prod
spec:
  strategy: production
  collector:
    image: pavolloffay/jaeger-collector:es-tls2
  query:
    image: pavolloffay/jaeger-query:es-tls2
  storage:
    type: elasticsearch
    options:
      es:
        server-urls: https://quickstart-es-http:9200
        tls:
          skip-host-verify: true
        num-shards: 1
        num-replicas: 0
    secretName: jaeger-secret
  volumeMounts:
    - name: certificates
      mountPath: /es/certificates/
      readOnly: true
  volumes:
    - name: certificates
      secret:
        secretName: quickstart-es-http-certs-public
EOF

To test CA cert without providing all certs.

cat <<EOF | kubectl apply -f -
apiVersion: jaegertracing.io/v1
kind: Jaeger
metadata:
  name: simple-prod
spec:
  strategy: production
  collector:
    image: pavolloffay/jaeger-collector:es-tls2
  query:
    image: pavolloffay/jaeger-query:es-tls2
  storage:
    type: elasticsearch
    options:
      es:
        server-urls: https://quickstart-es-http:9200
        tls:
          skip-host-verify: true
        num-shards: 1
        num-replicas: 0
    secretName: jaeger-secret
  volumeMounts:
    - name: certificates
      mountPath: /es/certificates/
      readOnly: true
  volumes:
    - name: certificates
      secret:
        secretName: quickstart-es-http-certs-public
EOF

pavolloffay

LGTM,

Could you please list it with an explanation regarding skip tls and Cassandra in our changelog?

pavolloffay · 2020-01-27T13:02:57Z

plugin/storage/es/options.go

+	return tlscfg.ClientFlagsConfig{
+		Prefix:         config.namespace,
+		ShowEnabled:    true,
+		ShowServerName: true,


This flag wasn't supported before.

yurishkuro · 2020-01-28T01:06:36Z

Thank you, @backjo!

jpkrohling · 2020-02-27T09:55:18Z

pkg/kafka/auth/tls.go

-	if err != nil {
-		return errors.Wrap(err, "error loading tls config")
+func setTLSConfiguration(config *tlscfg.Options, saramaConfig *sarama.Config) error {
+	if config.Enabled {


This is a breaking change: previously, there was no --kafka.consumer.tls option (or producer), and TLS config would be used on whether the getTLS() function returned an error or not. Now, even if all the parameters are set, the collector/ingester won't be using TLS unless a new option is added to the command line, --kafka.consumer.tls.enabled (or the one marked as deprecated but that never existed: --kafka.consumer.tls).

For example, we had this in the jaeger-operator before, when auto-provisioning a Kafka cluster:

--kafka.producer.authentication=tls --kafka.producer.brokers=auto-provision-kafka-kafka-bootstrap.default.svc.cluster.local:9093 --kafka.producer.tls.ca=/var/run/secrets/auto-provision-kafka-cluster-ca/ca.crt --kafka.producer.tls.cert=/var/run/secrets/auto-provision-kafka/user.crt --kafka.producer.tls.key=/var/run/secrets/auto-provision-kafka/user.key

It now requires the new property to be added, otherwise, the collector/ingester will crash with the following error:

{"level":"fatal","ts":1582796332.9417691,"caller":"collector/main.go:91","msg":"Failed to init storage factory","error":"kafka: client has run out of available brokers to talk to (Is your cluster reachable?)","stacktrace":"main.main.func1\n\t/home/travis/gopath/src/github.com/jaegertracing/jaeger/cmd/collector/main.go:91\ngithub.com/jaegertracing/jaeger/vendor/github.com/spf13/cobra.(*Command).execute\n\t/home/travis/gopath/src/github.com/jaegertracing/jaeger/vendor/github.com/spf13/cobra/command.go:698\ngithub.com/jaegertracing/jaeger/vendor/github.com/spf13/cobra.(*Command).ExecuteC\n\t/home/travis/gopath/src/github.com/jaegertracing/jaeger/vendor/github.com/spf13/cobra/command.go:783\ngithub.com/jaegertracing/jaeger/vendor/github.com/spf13/cobra.(*Command).Execute\n\t/home/travis/gopath/src/github.com/jaegertracing/jaeger/vendor/github.com/spf13/cobra/command.go:736\nmain.main\n\t/home/travis/gopath/src/github.com/jaegertracing/jaeger/cmd/collector/main.go:183\nruntime.main\n\t/home/travis/.gimme/versions/go1.13.5.linux.amd64/src/runtime/proc.go:203"}

What is the "new" set of parameters to make it work?

The tls.enabled ones: kafka.consumer.tls.enabled and kafka.producer.tls.enabled.

So to be backward compatible, it would have to use the getTLS() approach still - possibly with the getTLS() method checking the enabled flag (i.e. if enabled = false then don't use)?

That would work, unless "not specified" (nil) means false when checking the tls.enabled flag.

Yep - unfortunately tls.enabled default is false.

Created #2092 to track this.

backjo requested review from black-adder, jpkrohling, objectiser, pavolloffay, tiffon, vprithvi and yurishkuro as code owners October 7, 2019 05:04

pavolloffay requested changes Oct 9, 2019

View reviewed changes

plugin/storage/es/options_test.go Show resolved Hide resolved

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

yurishkuro reviewed Oct 9, 2019

View reviewed changes

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

yurishkuro mentioned this pull request Oct 15, 2019

Add sasl plain auth kafka plugin #1855

Closed

backjo closed this Oct 18, 2019

backjo reopened this Oct 18, 2019

backjo requested a review from pavolloffay October 18, 2019 23:13

pavolloffay previously approved these changes Oct 21, 2019

View reviewed changes

yurishkuro reviewed Oct 25, 2019

View reviewed changes

pavolloffay mentioned this pull request Oct 25, 2019

Release 1.15.0 #1877

Closed

4 tasks

backjo force-pushed the commonTlsConfig branch 3 times, most recently from e2dc8ec to 15650eb Compare December 19, 2019 03:03

pavolloffay requested changes Dec 19, 2019

View reviewed changes

pavolloffay reviewed Dec 20, 2019

View reviewed changes

yurishkuro reviewed Dec 21, 2019

View reviewed changes

yurishkuro mentioned this pull request Jan 26, 2020

Clarification in docs #2038

Closed

backjo requested a review from a team as a code owner January 26, 2020 22:34

Standarize TLS configuration

bf32eb2

Signed-off-by: Jonah Back <jonah_back@intuit.com>

backjo force-pushed the commonTlsConfig branch from a89193f to bf32eb2 Compare January 26, 2020 23:03

backjo added 2 commits January 26, 2020 15:31

Add check for deprecated tls verify-host flag

9330efd

Signed-off-by: Jonah Back <jonah@jonahback.com>

fix #nosec line placement

0fdf538

Signed-off-by: Jonah Back <jonah@jonahback.com>

backjo force-pushed the commonTlsConfig branch from a4cd78a to 0fdf538 Compare January 26, 2020 23:59

yurishkuro approved these changes Jan 27, 2020

View reviewed changes

yurishkuro reviewed Jan 27, 2020

View reviewed changes

Add updated lock file

4863a22

Signed-off-by: Jonah Back <jonah@jonahback.com>

pavolloffay approved these changes Jan 27, 2020

View reviewed changes

yurishkuro merged commit c33f5ea into jaegertracing:master Jan 28, 2020

jpkrohling reviewed Feb 27, 2020

View reviewed changes

This was referenced Feb 27, 2020

Breaking change in v1.17.0 when using to Kafka + TLS #2092

Closed

Bump jaeger to 1.17.0 jaegertracing/helm-charts#55

Merged

pavolloffay mentioned this pull request Mar 3, 2020

Remove deprecated kafka tls.enabled flags #2111

Closed

Conversation

backjo commented Oct 7, 2019

Which problem is this PR solving?

Short description of the changes

Uh oh!

backjo commented Oct 7, 2019

Uh oh!

yurishkuro commented Oct 7, 2019

Uh oh!

backjo commented Oct 7, 2019

Uh oh!

yurishkuro commented Oct 8, 2019

Uh oh!

backjo commented Oct 8, 2019

Uh oh!

pavolloffay left a comment • 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!

Uh oh!

Uh oh!

pavolloffay commented Oct 9, 2019

Uh oh!

Uh oh!

backjo commented Oct 9, 2019

Uh oh!

yurishkuro commented Oct 15, 2019

Uh oh!

backjo commented Oct 16, 2019

Uh oh!

codecov bot commented Oct 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

backjo commented Oct 18, 2019

Uh oh!

backjo commented Oct 23, 2019

Uh oh!

yurishkuro commented Oct 24, 2019

Uh oh!

yurishkuro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yurishkuro Oct 25, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pavolloffay commented Dec 19, 2019

Uh oh!

pavolloffay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yurishkuro left a comment

Choose a reason for hiding this comment

Uh oh!

pavolloffay left a comment •

edited

Loading

codecov bot commented Oct 18, 2019 •

edited

Loading

yurishkuro Oct 25, 2019 •

edited

Loading

jpkrohling Feb 27, 2020 •

edited

Loading

jpkrohling Feb 27, 2020 •

edited

Loading