Use a more conservative set of CipherSuites

[stevendanna]

The default cipher suites used by Go include a number of ciphers that
have known weaknesses. In addition to leaving users open to these
weaknesses, the inclusion of these weaker ciphers causes problems with
various automated scanning tools.

This PR disables the CBC-mode, RC4, and 3DES ciphers included in the
Go standard library by passing an explicit cipher suite list.

The ciphers included here are more line with those recommended by
Mozilla for "Intermediate" compatibility. [0]

Performance Implications

The Go standard library does capability-based cipher ordering,
preferring AES ciphers if the underlying hardware has AES specific
instructions. [1] Since all of the relevant code is internal modules,
to do the same thing ourselves would require duplicating that
code. Here, I've placed AES based ciphers first.

Compatibility Implications

This does reduce the number of clients who will be able to communicate
with dex.

[0] https://ssl-config.mozilla.org/#server=nginx&server-version=1.17.0&config=intermediate&hsts=false&ocsp=false
[1] https://github.com/golang/go/blob/a8c2e5c6adc0d8f9b976a55bf4e22fcf5770ea55/src/crypto/tls/common.go#L1091

Signed-off-by: Steven Danna [email protected]

[stevendanna]

I can make the CipherSuites configurable if that is preferred. I held off since y'all preferred avoiding configuration for the TLS protocol version.

[srenatus]

Thank you 🎉 👏

I think not duplicating the internal code is the right call for now.

I wonder if there's anything re-usable, like, the internal code turned into a package, out there. But that shouldn't block this. 😃

[srenatus]

@bonifaido @JoelSpeed: Since @stevendanna and I are coworkers, could one of you please review this, too? 😉

[JoelSpeed]

Happy with the suggested improvements, only thing I'd potentially do is add this as a configuration option.

Making this stricter will likely block some people from upgrading their Dex in the future, if it is possible to allow them to go back to the old ciphers then we wouldn't block them. Though the current suggestion should be the default if we go for that riute

What do others think?

[srenatus]

Making this stricter will likely block some people from upgrading their Dex in the future

In theory, you have a point -- I don't know about the concrete fallout there, though; are there really such clients? In deployment schemes that would be affected? (Since this only applies to direct connections to dex, and not those reverse proxied by nginx, haproxy etc.) I'm having a difficult time reasoning about this, and I'm also lacking evidence for my sceptical stance 😉

[bonifaido]

I also think that - since the GRPC interface is updated only - that making this configurable is not mandatory.

[JoelSpeed]

I'm having a difficult time reasoning about this, and I'm also lacking evidence for my sceptical stance 😉

I also have no evidence, just suspicion. Let's merge this as is but, I withhold my "I told you so" rights for a future issue that references this change 😉

[stevendanna]

I also think that - since the GRPC interface is updated only - that making this configurable is not mandatory.

I believe this modifies the HTTPS listener as well, no? Happy to put together a PR to make this configurable if that changes y'alls mind.

[JoelSpeed]

I believe this modifies the HTTPS listener as well, no?

I think you're right

Happy to put together a PR to make this configurable if that changes y'alls mind.

Let's wait until someone complains