Perform log.fatal if TLS flags are used when tls.enabled=false #2893 #3030

clock21am · 2021-05-25T19:55:06Z

Signed-off-by: Rajdeep Kaur rajdeep51994@gmail.com

Which problem is this PR solving?

Resolves Error/warn if TLS flags are used when tls.enabled=false #2893 Terminate the program if tls.enabled=false

Short description of the changes

Added log.fatal if tls.enabled=false for client as well as server

pkg/config/tlscfg/flags.go

clock21am · 2021-05-31T14:10:14Z

@yurishkuro let me know what you think now

cmd/all-in-one/main.go

cmd/collector/app/builder_flags.go

cmd/query/app/flags_test.go

yurishkuro · 2021-05-31T17:40:44Z

pkg/config/tlscfg/flags.go

 	var p Options
 	if c.ShowEnabled {
 		p.Enabled = v.GetBool(c.Prefix + tlsEnabled)
+	} else {
+		return p, errors.New("ES_TLS_ENABLED is set to false unable to talk to server(s)")


I think the point of the issue was to raise an error if enabled=false but other flags are present. There's nothing wrong with enabled=false by itself. Also, this is not specific to ES, if you want to refer to the actual property it would be c.Prefix + tlsEnabled

clock21am · 2021-06-05T17:50:04Z

@yurishkuro let me know what you think now

cmd/agent/main.go

yurishkuro · 2021-06-05T21:44:23Z

cmd/agent/app/reporter/grpc/flags.go

 	b.DiscoveryMinPeers = v.GetInt(discoveryMinPeers)
-	return b
+	if TLS, err := tlsFlagsConfig.InitFromViper(v); err != nil {
+		return b, err


not sure if this will pass the linter, if not you can swap the condition and do a return in the else part.

it looks odd when local var is upper case, use tls or cfg

yurishkuro · 2021-06-05T21:45:01Z

cmd/collector/app/builder_flags_test.go

@@ -15,6 +15,7 @@
 package app

 import (
+	"github.com/stretchr/testify/require"


did you run make lint?

yurishkuro · 2021-06-05T21:48:03Z

pkg/config/tlscfg/flags.go

+	if c.ShowEnabled {
+		p.Enabled = v.GetBool(c.Prefix + tlsEnabled)
+	} else {
+		return p, fmt.Errorf("%s has been set to False", c.Prefix+tlsEnabled)


c.ShowEnabled == false doesn't mean that tls was disabled, it means that the CLI option does not need to be present

even if TLS is disabled (p.Enable == false), doesn't mean that this is an error. The error is when it is false but any other flags are non-empty: (!p.Enabled || !c.ShowEnabled) && anyOtherFieldIsNotEmpty

clock21am · 2021-06-20T20:13:13Z

@yurishkuro ran lint and tests let me know what you think now

albertteoh · 2021-06-21T07:09:17Z

plugin/metrics/prometheus/options.go

 	cfg := &opt.Primary
 	cfg.ServerURL = stripWhiteSpace(v.GetString(cfg.namespace + suffixServerURL))
 	cfg.ConnectTimeout = v.GetDuration(cfg.namespace + suffixConnectTimeout)
-	cfg.TLS = cfg.getTLSFlagsConfig().InitFromViper(v)
+	if tls, err := cfg.getTLSFlagsConfig().InitFromViper(v); err == nil {


The failing unit test job is due to linter errors, the majority of which are:

if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)

Whether if the linter is right about this being a good style or not is debatable. :)

One option is:

tls, err := cfg.getTLSFlagsConfig().InitFromViper(v) if err != nil { return err } cfg.TLS = tls return nil

To check for linter errors, run make lint.

@albertteoh don't think see this is the output of make lint and it does point to any of my changes

time staticcheck ./... \ | grep -v \ -e model/model.pb.go \ -e proto-gen \ -e _test.pb.go \ -e thrift-gen/ \ -e swagger-gen/ \ >> .lint.log || true real 0m5.547s user 0m5.165s sys 0m7.180s time gosec -quiet -exclude=G104,G107 ./... Results: [/Users/rkaur/github/jaeger/plugin/sampling/strategystore/adaptive/processor.go:226] - G404 (CWE-338): Use of weak random number generator (math/rand instead of crypto/rand) (Confidence: MEDIUM, Severity: HIGH) 225: func addJitter(jitterAmount time.Duration) { > 226: delay := (jitterAmount / 2) + time.Duration(rand.Int63n(int64(jitterAmount/2))) 227: time.Sleep(delay) [/Users/rkaur/github/jaeger/pkg/discovery/grpcresolver/grpc_resolver.go:68] - G404 (CWE-338): Use of weak random number generator (math/rand instead of crypto/rand) (Confidence: MEDIUM, Severity: HIGH) 67: seed := time.Now().UnixNano() > 68: random := rand.New(rand.NewSource(seed)) 69: r := &Resolver{ [/Users/rkaur/github/jaeger/crossdock/services/tracehandler.go:308] - G404 (CWE-338): Use of weak random number generator (math/rand instead of crypto/rand) (Confidence: MEDIUM, Severity: HIGH) 307: // A random 8-byte hex > 308: return fmt.Sprintf("%x", rand.Int63()) 309: } [/Users/rkaur/github/jaeger/cmd/ingester/app/processor/decorator/retry.go:54] - G404 (CWE-338): Use of weak random number generator (math/rand instead of crypto/rand) (Confidence: MEDIUM, Severity: HIGH) 53: propagateError: false, > 54: rand: rand.New(rand.NewSource(time.Now().UnixNano())), 55: } [/Users/rkaur/github/jaeger/cmd/collector/app/sanitizer/cache/auto_refresh_cache.go:136] - G404 (CWE-338): Use of weak random number generator (math/rand instead of crypto/rand) (Confidence: MEDIUM, Severity: HIGH) 135: func getRandomSleepTime(interval time.Duration) time.Duration { > 136: return (interval / 2) + time.Duration(rand.Int63n(int64(interval/2))) 137: } [/Users/rkaur/github/jaeger/cmd/anonymizer/app/uiconv/reader.go:40] - G304 (CWE-22): Potential file inclusion via variable (Confidence: HIGH, Severity: MEDIUM) 39: func NewReader(capturedFile string, logger *zap.Logger) (*Reader, error) { > 40: cf, err := os.OpenFile(capturedFile, os.O_RDONLY, os.ModePerm) 41: if err != nil { [/Users/rkaur/github/jaeger/cmd/anonymizer/app/uiconv/extractor.go:38] - G304 (CWE-22): Potential file inclusion via variable (Confidence: HIGH, Severity: MEDIUM) 37: func NewExtractor(uiFile string, traceID string, reader *Reader, logger *zap.Logger) (*Extractor, error) { > 38: f, err := os.OpenFile(uiFile, os.O_CREATE|os.O_WRONLY, os.ModePerm) 39: if err != nil { [/Users/rkaur/github/jaeger/model/converter/thrift/jaeger/from_domain.go:106] - G601 (CWE-118): Implicit memory aliasing in for loop. (Confidence: MEDIUM, Severity: MEDIUM) 105: for idx, kv := range kvs { > 106: jaegerTags[idx] = d.keyValueToTag(&kv) 107: } [/Users/rkaur/github/jaeger/cmd/anonymizer/main.go:72] - G601 (CWE-118): Implicit memory aliasing in for loop. (Confidence: MEDIUM, Severity: MEDIUM) 71: for _, span := range spans { > 72: writer.WriteSpan(&span) 73: } Summary: Gosec : dev Files : 441 Lines : 73829 Nosec : 10 Issues : 9 164.77 real 166.34 user 224.53 sys make: *** [lint-gosec] Error 1

@albertteoh ooh it is make go-lint

make lint should run make go-lint, but it looks like it's gosec -quiet -exclude=G104,G107 ./... that's failing for you before it gets to call make go-lint. This is odd, because gosec passes in CI (and for me as well).

Did you run make install-tools to install gosec? which gosec should point to $GOPATH/bin/gosec.

installed explicitly gosec but didn't run make install-tools

it should show a warning now should not show failing

ooh less than 95 got it

codecov · 2021-06-21T11:17:26Z

Codecov Report

Merging #3030 (8ef54be) into main (2dfd77d) will decrease coverage by 0.08%.
The diff coverage is 80.89%.

@@            Coverage Diff             @@
##             main    #3030      +/-   ##
==========================================
- Coverage   96.53%   96.44%   -0.09%     
==========================================
  Files         264      264              
  Lines       15449    15509      +60     
==========================================
+ Hits        14913    14957      +44     
- Misses        451      463      +12     
- Partials       85       89       +4

Impacted Files	Coverage Δ
plugin/storage/grpc/factory.go	`94.64% <0.00%> (-5.36%)`	⬇️
plugin/storage/kafka/options.go	`89.38% <0.00%> (-2.59%)`	⬇️
cmd/es-rollover/app/actions.go	`89.65% <25.00%> (-10.35%)`	⬇️
plugin/storage/cassandra/options.go	`97.91% <33.33%> (-2.09%)`	⬇️
plugin/storage/es/options.go	`98.51% <42.85%> (-1.49%)`	⬇️
cmd/agent/app/reporter/grpc/flags.go	`100.00% <100.00%> (ø)`
cmd/collector/app/builder_flags.go	`100.00% <100.00%> (ø)`
cmd/query/app/flags.go	`100.00% <100.00%> (ø)`
pkg/config/tlscfg/flags.go	`100.00% <100.00%> (ø)`
plugin/metrics/prometheus/factory.go	`100.00% <100.00%> (ø)`
... and 6 more

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 2dfd77d...8ef54be. Read the comment docs.

albertteoh

To help with identifying test coverage gaps locally, I know of a few options (folks reading this, please feel free to add other suggestions):

your IDE should support executing tests with coverage.
run make cover from the project root. This checks coverage for the entire project so it takes some time.
I personally like to use a command that I install in .zshrc that is like make cover but runs from a path relative to the current dir, so it's much quicker. Happy to share that if you prefer this option.

albertteoh · 2021-06-22T06:32:04Z

pkg/config/tlscfg/flags_test.go

+			err := command.ParseFlags(append(cmdLine, test.option))
+			require.NoError(t, err)
+			_, err = flagCfg.InitFromViper(v)
+			require.Error(t, err, "prefix.tls.enabled has been disable")


Error just checks that the error is not nil. I would use EqualError to assert on the actual error message.

albertteoh · 2021-06-22T06:39:01Z

pkg/config/tlscfg/flags.go

-	p.CAPath = v.GetString(c.Prefix + tlsCA)
-	p.CertPath = v.GetString(c.Prefix + tlsCert)
-	p.KeyPath = v.GetString(c.Prefix + tlsKey)
+	if cAPATH := v.GetString(c.Prefix + tlsCA); len(cAPATH) > 0 {


cAPATH seems like an unconventional name, I would use caPath.

albertteoh · 2021-06-22T07:36:48Z

pkg/config/tlscfg/flags.go

 	var p Options
+	var checkAnyFieldIsEmpty bool
 	if c.ShowEnabled {


@yurishkuro what is the purpose of ShowEnabled and ShowServerName?

If I do a search for ClientFlagsConfig{ in the project, both are always set to true, so the zero value of false is not used, or perhaps I've missed something.

Similarly for ShowClientCA.

They weren't always true, some settings did not apply to some use cases. This may have changed since then.

@clock21am The Show* flags have been removed in #3106 which should simplify efforts for you in this PR. Please merge these changes in from master.

and thanks for confirming, Yuri.

Thanks for the review @albertteoh @yurishkuro i will rebase my branch and fix it

albertteoh · 2021-06-22T07:47:18Z

pkg/config/tlscfg/flags.go

-	p.KeyPath = v.GetString(c.Prefix + tlsKey)
+	if cAPATH := v.GetString(c.Prefix + tlsCA); len(cAPATH) > 0 {
+		p.CAPath = cAPATH
+	}


why isn't there an else case here like the others?

albertteoh · 2021-06-22T07:53:50Z

pkg/config/tlscfg/flags.go

 	if c.ShowServerName {
 		p.ServerName = v.GetString(c.Prefix + tlsServerName)
+	} else {
+		checkAnyFieldIsEmpty = true


is this line of code ever called? I can't see where c.ShowServerName == false.

albertteoh · 2021-06-22T10:46:15Z

pkg/config/tlscfg/flags.go

+		p.Enabled = v.GetBool(c.Prefix + tlsEnabled)
+	}
+	if (!p.Enabled || !c.ShowEnabled) && !checkAnyFieldIsEmpty {
+		return p, fmt.Errorf("%s has been disable", c.Prefix+tlsEnabled)


nit: I would extract fmt.Errorf("%s has been disable", c.Prefix+tlsEnabled) into a function so there's only one place where the error message is defined.

Also, consider rewording this to be clearer on what is causing the error, a suggestion:

fmt.Errorf("some tls flags were set while %s=false", c.Prefix+tlsEnabled)

albertteoh · 2021-06-22T10:46:46Z

pkg/config/tlscfg/flags.go

 	if c.ShowClientCA {
 		p.ClientCAPath = v.GetString(c.Prefix + tlsClientCA)
+	} else {
+		checkAnyFieldIsEmpty = true


will this ever be called? I can't see where cShowClientCA == false.

albertteoh · 2021-06-22T10:51:27Z

pkg/config/tlscfg/flags.go

-	p.CAPath = v.GetString(c.Prefix + tlsCA)
-	p.CertPath = v.GetString(c.Prefix + tlsCert)
-	p.KeyPath = v.GetString(c.Prefix + tlsKey)
+	if cAPATH := v.GetString(c.Prefix + tlsCA); len(cAPATH) > 0 {


I think the if/else cases could be simplified back to what it was before but add a bool var. So something like:

p.CAPath = v.GetString(c.Prefix + tlsCA) p.CertPath = v.GetString(c.Prefix + tlsCert) p.KeyPath = v.GetString(c.Prefix + tlsKey) // or alternatively could loop over a list of these options. areTLSFlagsUsed := v.IsSet(c.Prefix+tlsCA) || v.IsSet(c.Prefix+tlsCert) || v.IsSet(c.Prefix+tlsKey) ... if (!p.Enabled || !c.ShowEnabled) && (p.SkipHostVerify && areTLSFlagsUsed) { return p, fmt.Errorf("%s has been disable", c.Prefix+tlsEnabled) }

albertteoh · 2021-06-22T10:59:15Z

pkg/kafka/auth/config.go

@@ -95,7 +95,7 @@ func (config *AuthenticationConfig) InitFromViper(configPrefix string, v *viper.
 		ShowServerName: true,
 	}

-	config.TLS = tlsClientConfig.InitFromViper(v)
+	config.TLS, _ = tlsClientConfig.InitFromViper(v)


why is the error ignored? I thought we'd want to report the TLS flag usage error for Kafka storage too?

because it has been set to true later https://github.com/jaegertracing/jaeger/blob/master/pkg/kafka/auth/config.go#L96

…tracing#2893) Signed-off-by: Rajdeep Kaur <rajdeep51994@gmail.com>

This commit adds tls flag test in collector/app/builder_flag_test.go and query/app/flag_test.go Also fixes InitFromViper check in builder_flag.go (jaegertracing#2893) Signed-off-by: Rajdeep Kaur <rajdeep51994@gmail.com>

yurishkuro · 2021-07-16T21:40:31Z

pkg/config/tlscfg/flags.go

-	return p
+	areTLSFlagsUsed := v.IsSet(c.Prefix+tlsCA) || v.IsSet(c.Prefix+tlsCert) || v.IsSet(c.Prefix+tlsKey) || v.IsSet(c.Prefix+tlsServerName)
+	if (!p.Enabled) && (p.SkipHostVerify && areTLSFlagsUsed) {
+		return p, fmt.Errorf("%s has been disable", c.Prefix+tlsEnabled)


Suggested change

return p, fmt.Errorf("%s has been disable", c.Prefix+tlsEnabled)

return p, fmt.Errorf("TLS parameters do not apply when %s=false", c.Prefix+tlsEnabled)

yurishkuro · 2021-07-16T21:42:09Z

cmd/query/app/flags_test.go

+			v.Set("query.http.tls.ca", "def")
+			v.Set("query.http.tls.key", "xyz")
+			_, err := new(QueryOptions).InitFromViper(v, zap.NewNop())
+			require.Error(t, err, "query.http.tls.enabled has been disable")


This does not test for the type of error, only that it is not nil (the last argument is only used when logging a failure).

If you want to test for the actual error, I would suggest this:

require.Error(t, err) assert.Contains(t, err.Error(), "some relevant substring of the message")

jpkrohling · 2021-08-23T08:24:26Z

What's the status of this PR?

clock21am · 2021-10-10T12:57:00Z

Hey

What's the status of this PR?

hey will start working on it again..somehow lost it will update it today.

stale · 2022-04-16T06:22:11Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. You may re-open it if you need more time.

Signed-off-by: Yuri Shkuro <github@ysh.us>

yurishkuro · 2022-04-18T03:15:34Z

@clock21am thank you!

…tracing#2893 (jaegertracing#3030) Signed-off-by: Albert Teoh <see.kwang.teoh@gmail.com>

clock21am requested a review from a team as a code owner May 25, 2021 19:55

clock21am requested a review from vprithvi May 25, 2021 19:55

yurishkuro reviewed May 25, 2021

View reviewed changes

pkg/config/tlscfg/flags.go Outdated Show resolved Hide resolved

clock21am force-pushed the rajdeep/issue-2893 branch from 39138be to f156584 Compare May 31, 2021 08:42

clock21am requested a review from yurishkuro May 31, 2021 08:49

clock21am force-pushed the rajdeep/issue-2893 branch 2 times, most recently from 3e5607a to b3bf080 Compare May 31, 2021 09:04

yurishkuro reviewed May 31, 2021

View reviewed changes

clock21am force-pushed the rajdeep/issue-2893 branch from b3bf080 to 700af6f Compare June 5, 2021 17:49

clock21am force-pushed the rajdeep/issue-2893 branch from 700af6f to 1f596c0 Compare June 5, 2021 17:51

clock21am requested a review from yurishkuro June 5, 2021 17:53

yurishkuro reviewed Jun 5, 2021

View reviewed changes

clock21am force-pushed the rajdeep/issue-2893 branch 2 times, most recently from 7b116fb to 24b9800 Compare June 20, 2021 20:11

clock21am requested a review from yurishkuro June 20, 2021 20:13

albertteoh reviewed Jun 21, 2021

View reviewed changes

clock21am force-pushed the rajdeep/issue-2893 branch 3 times, most recently from 68279e2 to 8ba6555 Compare June 21, 2021 11:00

clock21am force-pushed the rajdeep/issue-2893 branch from 8ba6555 to 8c74894 Compare June 21, 2021 14:05

clock21am requested a review from albertteoh June 21, 2021 14:25

albertteoh reviewed Jun 22, 2021

View reviewed changes

albertteoh mentioned this pull request Jun 24, 2021

Removing Show* TLS config flags #3104

Closed

Perform log.fatal if TLS flags are used when tls.enabled=false(jaeger…

f92ac6a

…tracing#2893) Signed-off-by: Rajdeep Kaur <rajdeep51994@gmail.com>

clock21am force-pushed the rajdeep/issue-2893 branch from ebbd75c to 9e51d3e Compare July 4, 2021 10:40

Add TLS flags test

f8f1cdf

This commit adds tls flag test in collector/app/builder_flag_test.go and query/app/flag_test.go Also fixes InitFromViper check in builder_flag.go (jaegertracing#2893) Signed-off-by: Rajdeep Kaur <rajdeep51994@gmail.com>

clock21am force-pushed the rajdeep/issue-2893 branch from 9e51d3e to f8f1cdf Compare July 16, 2021 20:08

clock21am requested a review from albertteoh July 16, 2021 20:14

yurishkuro reviewed Jul 16, 2021

View reviewed changes

stale bot added the stale The issue/PR has become stale and may be auto-closed label Apr 16, 2022

yurishkuro added 3 commits April 16, 2022 23:47

Merge branch 'main' into rajdeep/issue-2893

3969321

Signed-off-by: Yuri Shkuro <github@ysh.us>

fix test

d4584f5

Signed-off-by: Yuri Shkuro <github@ysh.us>

Merge branch 'main' into rajdeep/issue-2893

a79f23a

Signed-off-by: Yuri Shkuro <github@ysh.us>

stale bot removed the stale The issue/PR has become stale and may be auto-closed label Apr 17, 2022

yurishkuro and others added 6 commits April 17, 2022 00:42

fix tests

ca1dfac

Signed-off-by: Yuri Shkuro <github@ysh.us>

fix test

5fbd179

Signed-off-by: Yuri Shkuro <github@ysh.us>

Merge branch 'main' into rajdeep/issue-2893

552cf86

fix tests

fad9357

Signed-off-by: Yuri Shkuro <github@ysh.us>

fix tests

2834752

Signed-off-by: Yuri Shkuro <github@ysh.us>

remove comment

8ef54be

Signed-off-by: Yuri Shkuro <github@ysh.us>

yurishkuro approved these changes Apr 17, 2022

View reviewed changes

yurishkuro merged commit 52212b1 into jaegertracing:main Apr 18, 2022

albertteoh mentioned this pull request May 13, 2022

PROMETHEUS_SERVER_URL broken since 1.34 #3677

Closed

This was referenced May 16, 2022

fix: jaeger streaming e2e test for upcoming release 1.34 jaegertracing/jaeger-operator#1890

Closed

adapt tls flag usage for upcoming release jaegertracing/jaeger-operator#1891

Merged

loelu mentioned this pull request May 24, 2022

tls.enabled flag not passed to es-index-cleaner jaegertracing/jaeger-operator#1896

Closed

albertteoh pushed a commit to albertteoh/jaeger that referenced this pull request Jul 13, 2022

Perform log.fatal if TLS flags are used when tls.enabled=false jaeger…

ca049f9

…tracing#2893 (jaegertracing#3030) Signed-off-by: Albert Teoh <see.kwang.teoh@gmail.com>

	return p, fmt.Errorf("%s has been disable", c.Prefix+tlsEnabled)
	return p, fmt.Errorf("TLS parameters do not apply when %s=false", c.Prefix+tlsEnabled)

Perform log.fatal if TLS flags are used when tls.enabled=false #2893 #3030

Perform log.fatal if TLS flags are used when tls.enabled=false #2893 #3030

Conversation

clock21am commented May 25, 2021 • edited by yurishkuro Loading

Which problem is this PR solving?

Short description of the changes

clock21am commented May 31, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clock21am commented Jun 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clock21am commented Jun 20, 2021

Choose a reason for hiding this comment

clock21am Jun 21, 2021 • edited Loading

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

Choose a reason for hiding this comment

codecov bot commented Jun 21, 2021 • edited Loading

Codecov Report

albertteoh 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling commented Aug 23, 2021

clock21am commented Oct 10, 2021

stale bot commented Apr 16, 2022

yurishkuro commented Apr 18, 2022

clock21am commented May 25, 2021 •

edited by yurishkuro

Loading

clock21am Jun 21, 2021 •

edited

Loading

codecov bot commented Jun 21, 2021 •

edited

Loading