Adding min version and cipher suite variables to tls client configuration #217

Red-GV · 2022-09-06T22:30:26Z

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 · 2022-09-06T22:30:39Z

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.

crypto/tls/tls.go

Red-GV · 2022-09-07T12:01:09Z

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

pstibrany

Thank you for your PR.

crypto/tls/tls.go

CHANGELOG.md

go.mod

bboreham · 2022-09-07T12:09:25Z

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 · 2022-09-07T12:28:05Z

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 · 2022-09-07T12:29:32Z

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.

CHANGELOG.md

crypto/tls/tls.go

crypto/tls/tls_test.go

pstibrany

Thank you for addressing my feedback.

crypto/tls/tls.go

CHANGELOG.md

Red-GV · 2022-09-13T12:26:57Z

Hello @pstibrany , your comments have been addressed.

pstibrany · 2022-09-14T13:14:05Z

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 · 2022-09-16T16:44:35Z

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

pstibrany · 2022-09-19T07:26:37Z

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 · 2022-09-20T17:59:09Z

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

pstibrany · 2022-09-20T18:14:50Z

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 · 2022-09-20T18:34:03Z

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

Red-GV · 2022-09-26T13:09:00Z

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 👍

Signed-off-by: Marco Pracucci <marco@pracucci.com>

Red-GV force-pushed the cli-tls-variables branch from 9160365 to a5d88fe Compare September 6, 2022 22:40

aknuds1 reviewed Sep 7, 2022

View reviewed changes

crypto/tls/tls.go Outdated Show resolved Hide resolved

crypto/tls/tls.go Outdated Show resolved Hide resolved

aknuds1 requested a review from a team September 7, 2022 08:34

aknuds1 added the enhancement New feature or request label Sep 7, 2022

Red-GV force-pushed the cli-tls-variables branch from a5d88fe to ee3c31e Compare September 7, 2022 11:58

pstibrany reviewed Sep 7, 2022

View reviewed changes

crypto/tls/tls.go Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

go.mod Outdated Show resolved Hide resolved

aknuds1 reviewed Sep 8, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

crypto/tls/tls.go Outdated Show resolved Hide resolved

crypto/tls/tls.go Outdated Show resolved Hide resolved

crypto/tls/tls.go Outdated Show resolved Hide resolved

crypto/tls/tls_test.go Outdated Show resolved Hide resolved

Red-GV force-pushed the cli-tls-variables branch 3 times, most recently from ed372fc to 6bf3fe3 Compare September 8, 2022 17:03

pstibrany reviewed Sep 9, 2022

View reviewed changes

aknuds1 self-requested a review September 9, 2022 07:11

Red-GV force-pushed the cli-tls-variables branch from 68d2c08 to 30dd973 Compare September 9, 2022 18:20

pstibrany mentioned this pull request Sep 14, 2022

TLS: make cipher suite list and min version configurable grafana/mimir#2898

Merged

3 tasks

Red-GV added 4 commits September 20, 2022 19:50

Adding min version and cipher suite variables to tls configuration

b056b97

Addressing review comments

21f0031

Addressing review comments

2c58a77

Generating cli flags help text

01a19ad

Red-GV force-pushed the cli-tls-variables branch from 30dd973 to 01a19ad Compare September 20, 2022 23:53

Red-GV mentioned this pull request Sep 22, 2022

Configurable TLS MinVersion and CipherSuites grafana/loki#7227

Merged

5 tasks

bboreham changed the title ~~Adding min version and cipher suite variables to tls configuration~~ Adding min version and cipher suite variables to tls client configuration Sep 27, 2022

pracucci approved these changes Sep 27, 2022

View reviewed changes

pracucci mentioned this pull request Sep 28, 2022

Docs: add description_method struct tag support grafana/mimir#3064

Merged

3 tasks

pracucci added 2 commits September 28, 2022 10:23

Move long cipher suites description to auto-generated doc only

dab26fb

Signed-off-by: Marco Pracucci <marco@pracucci.com>

Merge branch 'main' into cli-tls-variables

8604158

pracucci merged commit b1b307d into grafana:main Sep 28, 2022

pracucci mentioned this pull request Sep 28, 2022

Update dskit and add TLS cipher suites and minimum version support for clients grafana/mimir#3070

Merged

3 tasks

Red-GV deleted the cli-tls-variables branch September 28, 2022 12:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding min version and cipher suite variables to tls client configuration #217

Adding min version and cipher suite variables to tls client configuration #217

Red-GV commented Sep 6, 2022 •

edited

CLAassistant commented Sep 6, 2022 •

edited

aknuds1 left a comment •

edited

Red-GV commented Sep 7, 2022

pstibrany left a comment

bboreham commented Sep 7, 2022 •

edited

Red-GV commented Sep 7, 2022 •

edited

pstibrany commented Sep 7, 2022

aknuds1 left a comment •

edited

pstibrany left a comment

Red-GV commented Sep 13, 2022

pstibrany commented Sep 14, 2022 •

edited

bboreham commented Sep 16, 2022

pstibrany commented Sep 19, 2022

Red-GV commented Sep 20, 2022

pstibrany commented Sep 20, 2022 •

edited

Red-GV commented Sep 20, 2022

Red-GV commented Sep 26, 2022

pracucci left a comment

Adding min version and cipher suite variables to tls client configuration #217

Adding min version and cipher suite variables to tls client configuration #217

Conversation

Red-GV commented Sep 6, 2022 • edited

CLAassistant commented Sep 6, 2022 • edited

aknuds1 left a comment • edited

Choose a reason for hiding this comment

Red-GV commented Sep 7, 2022

pstibrany left a comment

Choose a reason for hiding this comment

bboreham commented Sep 7, 2022 • edited

Red-GV commented Sep 7, 2022 • edited

pstibrany commented Sep 7, 2022

aknuds1 left a comment • edited

Choose a reason for hiding this comment

pstibrany left a comment

Choose a reason for hiding this comment

Red-GV commented Sep 13, 2022

pstibrany commented Sep 14, 2022 • edited

bboreham commented Sep 16, 2022

pstibrany commented Sep 19, 2022

Red-GV commented Sep 20, 2022

pstibrany commented Sep 20, 2022 • edited

Red-GV commented Sep 20, 2022

Red-GV commented Sep 26, 2022

pracucci left a comment

Choose a reason for hiding this comment

Red-GV commented Sep 6, 2022 •

edited

CLAassistant commented Sep 6, 2022 •

edited

aknuds1 left a comment •

edited

bboreham commented Sep 7, 2022 •

edited

Red-GV commented Sep 7, 2022 •

edited

aknuds1 left a comment •

edited

pstibrany commented Sep 14, 2022 •

edited

pstibrany commented Sep 20, 2022 •

edited