Support specifying cipher suites in tls connection

[jannyHou]

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

Hello team, in PR #2798 the tls connection is enabled in collector, it would be good to support configuring cipher for it.

It would be similar to the fluentd tls settings in https://docs.fluentd.org/configuration/transport-section#tls-setting, you can config the ciphers field.

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

Currently people could not configure the ciphers field.

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

Same as the requirement

Any open questions to address

n/a

[clock21am]

@yurishkuro can I take this?

[yurishkuro]

It's yours

[yurishkuro]

I wasn't too familiar with cipher suites, but now that I read about them, I am questioning why we need this.

1. The documentation says that they are only supported up to TLS 1.2, phased out in TLS 1.3. The default behavior is to use whichever ciphers the server supports.

2. I don't see an established format of describing the cipher family. The Go struct uses an array of uint16's, Fluentd doesn't describe the format but gives a weird example "ALL:!aNULL:!eNULL:!SSLv2", other sources show strings like "ECDHE-ECDSA-AES256-GCM-SHA384"

[Ashmita152]

why we need this

In my understanding, some organisations fulfil certain compliances (especially fintech regulated by operating country's law) that have strict requirements that all SSL communication should happen only within certain ciphers. In those cases, this option will be quite useful to specify as a config param.

[clock21am]

make sense @Ashmita152

[jpkrohling]

@yurishkuro, the server provides a list of ciphers to clients, and cipher negotiation happens. The key is to strike a balance between modern features and old clients. For instance, there might be older Java clients supporting TLS 1.1 only. Your example of Fluentd is basically filtering out classes of ciphers, while the last example is an example of a concrete cipher that would be part of a class. They should both be compatible.

Allowing the configuration of ciphers would allow admins to adjust the balance I mentioned before: if they are sure all their applications are capable of using modern ciphers, they can remove older, less safe ones. This way, a man-in-the-middle would have a harder time performing a downgrade attack.

[jpkrohling]

They should both be compatible.

I'll take this back. Looking at how Go implemented the Cipher suites, not sure this notation actually works and would be interested in knowing how fluentd did it.

Edit: not my best day :-) Fluentd isn't in Go...

[clock21am]

@jpkrohling @yurishkuro what do you think we should do then?

[jpkrohling]

I would prefer to have the OpenSSL format, which is what fluentd seems to be using. But if there aren't good libraries that parse OpenSSL format into a set of cipher suites understood by Go, I would prefer to see what other projects are doing in this matter.

[clock21am]

let me check and get back to you then

[sirzooro]

+1 for this. Also allow configuration of minimum TLS version - standards which require use of selected ciphers considered as safe also require use of some TLS version, e.g. 1.2+.

[yurishkuro]

After a bit more digging:

• crypto/tls supports a bunch of ciphers https://pkg.go.dev/crypto/tls#pkg-constants
• the names are defined in https://www.iana.org/assignments/tls-parameters/tls-parameters.xml#tls-parameters-4
• if we were to take those names as arguments, we could populate tls.Config like this:

func CipherSuitesFromNames(names []string) ([]uint16, error) {
	suites := tls.CipherSuites()
	var out []uint16
	for _, name := range names {
		found := false
		for _, suite := range suites {
			if suite.Name == name {
				out = append(out, suite.ID)
				found = true
				break
			}
		}
		if !found {
			return nil, fmt.Errorf("cipher '%v' is not suppored by crypto/tls, see https://pkg.go.dev/crypto/tls#pkg-constants", name)
		}
	}
	return out, nil
}

[tejaswiniVadlamudi]

+1 for this. Also allow configuration of minimum TLS version - standards which require use of selected ciphers considered as safe also require use of some TLS version, e.g. 1.2+.

+1 to @sirzooro, good to have config options for TLS versions together with cipher selection

[yurishkuro]

Seems this is now resolved by #3564