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

default value for tls cipher-suites #1879

Closed
frzifus opened this issue May 10, 2022 · 13 comments · Fixed by #1884
Closed

default value for tls cipher-suites #1879

frzifus opened this issue May 10, 2022 · 13 comments · Fixed by #1884

Comments

@frzifus
Copy link
Member

frzifus commented May 10, 2022

Requirement - what kind of business use case are you trying to solve?

In jaeger #3564, a new flag was introduced for configuring the cipher used. but this flag is not set by default by the operator.

Problem - what in Jaeger blocks you from solving the requirement?

A decision if a default parameter is basically desired. If yes, we could set them in the Jaeger Collector [pkg/config/tlscfg/flags.go] or on the operator side [pkg/config/tls/tls.go].

Proposal - what do you suggest to solve the problem or improve the existing situation?

From my point of view a default value in the operator would be the way to go.
An extention of tls.go could look like this:

if len(util.FindItem("--collector.grpc.tls.cipher-suites=", *options)) == 0 {
	*options = append(*options,
		"--collector.grpc.tls.cipher-suites=TLS_TODO_1;TLS_TODO_2",
	)
}

Any open questions to address

Which cipher should be enabled by default?


cc @pavolloffay @iblancasa

@frzifus
Copy link
Member Author

frzifus commented May 10, 2022

I was wondering why the problem does not occur on kubernetes. it seems to affect only openshift [tls.go].

@frzifus
Copy link
Member Author

frzifus commented May 10, 2022

A default fallback exists in crypto/tls. I don't quite understand why this is not used? Since the value passed into the tls.Config should be nil. [playground]
Ciphers are configured in the collector in [pkg/config/tlscfg/options.go].
Quote [crypto/tls/common.go]:

If CipherSuites is nil, a safe default list is used. The default cipher
suites might change over time.

@frzifus
Copy link
Member Author

frzifus commented May 10, 2022

Seems this line was causing the issue:
https://github.com/jaegertracing/jaeger/pull/3564/files#diff-24f34a75ab1f6765a63e2361744c174c109053d88e449ce503371a4ae37b7fb1R84

But its already fixed in:
https://github.com/jaegertracing/jaeger/pull/3030/files#diff-24f34a75ab1f6765a63e2361744c174c109053d88e449ce503371a4ae37b7fb1R98


Run

SAMPLING_CONFIG_TYPE=adaptive ./collector --cassandra.keyspace=jaeger_v1_dc1 --cassandra.servers=cassandra --collector.zipkin.host-port=9411 --sampling.initial-sampling-probability=.5 --sampling.target-samples-per-second=.01 --collector.grpc.tls.enabled --collector.grpc.tls.key host.key --collector.grpc.tls.cert host.cert

9b73914b7f6e9b6b1aad08ca41a81b2f1966a127 (v1.33) - failed

{"level":"fatal","ts":1652291276.605182,"caller":"collector/main.go:111","msg":"Failed to start collector","error":"could not start gRPC collector failed to get cipher suite ids from cipher suite names: cipher suite  not supported or doesn't exist","stacktrace":"main.main.func1\n\t/remote-source/jaeger/app/cmd/collector/main.go:111\ngithub.com/spf13/cobra.(*Command).execute\n\t/remote-source/jaeger/deps/gomod/pkg/mod/github.com/spf13/cobra@v1.4.0/command.go:856\ngithub.com/spf13/cobra.(*Command).ExecuteC\n\t/remote-source/jaeger/deps/gomod/pkg/mod/github.com/spf13/cobra@v1.4.0/command.go:974\ngithub.com/spf13/cobra.(*Command).Execute\n\t/remote-source/jaeger/deps/gomod/pkg/mod/github.com/spf13/cobra@v1.4.0/command.go:902\nmain.main\n\t/remote-source/jaeger/app/cmd/collector/main.go:147\nruntime.main\n\t/usr/lib/golang/src/runtime/proc.go:255"}

52212b1a782e0793bce8e9253206e49813098c6a - works as expected

{"level":"info","ts":1652292280.2011824,"caller":"adaptive/strategy_store.go:33","msg":"Using unique participantName in adaptive sampling","participantName":"8a67269fdff1-8b88efa6d0a1d421"}
{"level":"info","ts":1652292280.2012155,"caller":"adaptive/processor.go:166","msg":"starting adaptive sampling processor"}
{"level":"info","ts":1652292280.2096632,"caller":"server/grpc.go:95","msg":"Starting jaeger-collector gRPC server","grpc.host-port":"[::]:14250"}
{"level":"info","ts":1652292280.2096963,"caller":"server/http.go:48","msg":"Starting jaeger-collector HTTP server","http host-port":":14268"}
{"level":"info","ts":1652292280.209837,"caller":"server/zipkin.go:53","msg":"Listening for Zipkin HTTP traffic","zipkin host-port":":9411"}
{"level":"info","ts":1652292280.2237701,"caller":"healthcheck/handler.go:129","msg":"Health Check state change","status":"ready"}

@morningspace
Copy link

I see a similar issue when deploy jaeger on OpenShift. I was not able to start jaeger pod w/ the following error in pod logs:

2022/05/17 08:34:23 could not start gRPC collector failed to get cipher suite ids from cipher suite names: cipher suite  not supported or doesn't exist

Any idea to work around this issue? @frzifus

@frzifus
Copy link
Member Author

frzifus commented May 17, 2022

hi @morningspace, which version are you using?

@morningspace
Copy link

@frzifus I'm using 1.33.0.

@frzifus
Copy link
Member Author

frzifus commented May 17, 2022

mh.. makes sense. Its caused by a line that sets `` as argument when no flag is present. Since its in Jaeger 1.33.0 you could try to specify the flag:

spec.collector.grpc.tls.cipher-suites=TLS_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA"

Alternatives would be to install RHOSDT 2.3. It is based on jaeger 1.30 or wait for the next release of 1.34. (I am working on that right now and i would like to complete it today #1884)

@morningspace
Copy link

Thanks @frzifus for your time and looking forward to the v1.34.0 release. For the workaround, just to confirm, where to find the place to specify the flag, is it the jaegertracing.io/v1/jaeger CR or the deployment w/ the container args?

@frzifus
Copy link
Member Author

frzifus commented May 17, 2022

It needs to be defined in the JaegerCR. The openshift TLS config is defined here.
I assume it should look like this (untested):

apiVersion: jaegertracing.io/v1
kind: Jaeger
metadata:
  name: simple-prod
spec:
  strategy: production
  collector:
    options:
      grpc:
         tls:
           cipher-suites: "TLS_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA"
    maxReplicas: 5
    resources:
      limits:
        cpu: 100m
        memory: 128Mi
  storage:
    type: elasticsearch
    options:
      es:
        server-urls: http://elasticsearch:9200

@morningspace
Copy link

Thanks @frzifus , wonder when 1.34.0 will be released. I see your fix has been merged.

I tried changing the deployment and it will be reverted after a while, so I tried the Jaeger CR as you shared above, but it looks it does not apply the change to the corresponding pod. Probably there is something I missed.

image

image

@frzifus
Copy link
Member Author

frzifus commented May 18, 2022

Do you looks still show the same output that? like ... cipher suite '' not supported ...

Depending on your source when redhat-openshift-ecosystem/community-operators-prod#1217 or k8s-operatorhub/community-operators#1234 are processed :)

@frzifus
Copy link
Member Author

frzifus commented May 18, 2022

@morningspace its out. 👍

@morningspace
Copy link

Bravo! Thanks @frzifus !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants