Adding min version and cipher suite variables to tls client configuration

[Red-GV]

What this PR does:
This PR adds MinVersion and CipherSuites to the TLS configuration. This allows these TLS configuration paramters to be configurable outside of the defaults set by golang.

Which issue(s) this PR fixes:

Checklist

• Tests updated
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

[CLAassistant]

All committers have signed the CLA.

[aknuds1]

Just commenting on some nits, but you also need to fix the build (linting fails) and to sign the CLA.

[Red-GV]

Thanks @aknuds1 !
I have addressed the issues you highlighted.

[pstibrany]

Thank you for your PR.

[bboreham]

Is this the same change (roughly) as weaveworks/common#256 ?

Oh I see, this is client, while the common one is server-side.

Still, we should make the naming consistent, and I coded the other PR to avoid bringing in Kubernetes as a dependency.

[Red-GV]

Hello @bboreham @pstibrany,

Yes, this is more or less the same (goal-wise of allowing a configurable TLS config) as the weaveworks PR. I had just pulled in the K8s dependency for an easier look up, but I can make a map for all the values for a lookup table instead.

[pstibrany]

This is different in that it configures client side rather than server.

[aknuds1]

I think it looks pretty good now, thanks for dropping the new dependency!

Leaving some suggestions that come to mind.

[pstibrany]

Thank you for addressing my feedback.

[Red-GV]

Hello @pstibrany , your comments have been addressed.

[pstibrany]

Hello @pstibrany , your comments have been addressed.

Thank you for your work on this. I would like to make sure that this PR and grafana/mimir#2898 end up using same constants and help strings. /cc @bboreham

I have tried to integrate this PR into Mimir to see what the documentation and help string will look like.

Here is an example for CLI flags:

 -compactor.ring.etcd.tls-cipher-suites string
      Override the default cipher suite list (separated by commas). Allowed values:

      Secure Ciphers:
      - TLS_RSA_WITH_AES_128_CBC_SHA
      - TLS_RSA_WITH_AES_256_CBC_SHA
      - TLS_RSA_WITH_AES_128_GCM_SHA256
      - TLS_RSA_WITH_AES_256_GCM_SHA384
      - TLS_AES_128_GCM_SHA256
      - TLS_AES_256_GCM_SHA384
      - TLS_CHACHA20_POLY1305_SHA256
      - TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA
      - TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA
      - TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA
      - TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA
      - TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
      - TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384
      - TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
      - TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
      - TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
      - TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256

      Insecure Ciphers:
      - TLS_RSA_WITH_RC4_128_SHA
      - TLS_RSA_WITH_3DES_EDE_CBC_SHA
      - TLS_RSA_WITH_AES_128_CBC_SHA256
      - TLS_ECDHE_ECDSA_WITH_RC4_128_SHA
      - TLS_ECDHE_RSA_WITH_RC4_128_SHA
      - TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA
      - TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256
      - TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256

  -compactor.ring.etcd.tls-min-version string
       Override the default minimum TLS version. Allowed values: VersionTLS10, VersionTLS11, VersionTLS12, VersionTLS13

and documentation:

# (advanced) Override the default cipher suite list (separated by commas).
# Allowed values:
#
# Secure Ciphers:
# - TLS_RSA_WITH_AES_128_CBC_SHA
# - TLS_RSA_WITH_AES_256_CBC_SHA
# - TLS_RSA_WITH_AES_128_GCM_SHA256
# - TLS_RSA_WITH_AES_256_GCM_SHA384
# - TLS_AES_128_GCM_SHA256
# - TLS_AES_256_GCM_SHA384
# - TLS_CHACHA20_POLY1305_SHA256
# - TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA
# - TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA
# - TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA
# - TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA
# - TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
# - TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384
# - TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
# - TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
# - TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
# - TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256
#
# Insecure Ciphers:
# - TLS_RSA_WITH_RC4_128_SHA
# - TLS_RSA_WITH_3DES_EDE_CBC_SHA
# - TLS_RSA_WITH_AES_128_CBC_SHA256
# - TLS_ECDHE_ECDSA_WITH_RC4_128_SHA
# - TLS_ECDHE_RSA_WITH_RC4_128_SHA
# - TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA
# - TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256
# - TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256
# CLI flag: -<prefix>.etcd.tls-cipher-suites
[tls_cipher_suites: <string> | default = ""]

# (advanced) Override the default minimum TLS version. Allowed values:
# VersionTLS10, VersionTLS11, VersionTLS12, VersionTLS13
# CLI flag: -<prefix>.etcd.tls-min-version
[tls_min_version: <string> | default = ""]

[bboreham]

That list of strings seems way too long for help text, to me.

[pstibrany]

That list of strings seems way too long for help text, to me.

In Mimir we can extract it to a single doc block so that it doesn't repeat. Would that help? (I agree it's a bit long, and this is quite low-level option, so not many people will need it.)

[Red-GV]

So, should I revert the help text change on the cipher_suites?

[pstibrany]

So, should I revert the help text change on the cipher_suites?

I still think it's valuable to have it in the help without the need for additional lookup in another documentation. Please keep it. In Mimir we will try to minimise the repetition by introducing new doc block.

[Red-GV]

Okay, so then I just need to fix the conflict in the Changelog then. I will do that today.

[Red-GV]

Hello @pstibrany,

I was just what is the status of this PR? Is there anything more I need to do in order to get this PR through?

[pracucci]

Good job! CLI flags naming and supported values match the server side ones (weaveworks/common#256) so I'm happy with it 👍