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

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

Merged
merged 6 commits into from
Sep 28, 2022

Conversation

pracucci
Copy link
Collaborator

What this PR does

In this PR I'm updating dskit to get grafana/dskit#217, which adds support for overriding the default list of accepted cipher suites and minimum TLS version for clients.

Which issue(s) this PR fixes or relates to

N/A

Checklist

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

…r clients

Signed-off-by: Marco Pracucci <marco@pracucci.com>
@pracucci pracucci marked this pull request as ready for review September 28, 2022 08:54
@pracucci pracucci requested review from osg-grafana and a team as code owners September 28, 2022 08:54
@pracucci pracucci added the type/docs Improvements or additions to documentation label Sep 28, 2022
Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Could we put grpc-client config into a separate documentation block to avoid repeating all the fields multiple times in rendered documentation?

@pracucci
Copy link
Collaborator Author

Could we put grpc-client config into a separate documentation block to avoid repeating all the fields multiple times in rendered documentation?

The gRPC client config is already in a separate doc block. I wanted to put TLS in a separate doc, but we include it "inline" so can't be put in a separate block.

Am I missing anything?

Copy link
Member

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we put grpc-client config into a separate documentation block to avoid repeating all the fields multiple times in rendered documentation?

The gRPC client config is already in a separate doc block. I wanted to put TLS in a separate doc, but we include it "inline" so can't be put in a separate block.

Am I missing anything?

You are right. I got confused by seeing that other (non-grpc) client configs (-alertmanager.alertmanager-client or -ruler.alertmanager-client)

Comment on lines 137 to 163
The TLS cipher suites supported by Mimir are:

- `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`
- `TLS_RSA_WITH_RC4_128_SHA` (insecure)
- `TLS_RSA_WITH_3DES_EDE_CBC_SHA` (insecure)
- `TLS_RSA_WITH_AES_128_CBC_SHA256` (insecure)
- `TLS_ECDHE_ECDSA_WITH_RC4_128_SHA` (insecure)
- `TLS_ECDHE_RSA_WITH_RC4_128_SHA` (insecure)
- `TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA` (insecure)
- `TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256` (insecure)
- `TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256` (insecure)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should include this list in the generic documentation. It can change between Go versions, and we may not update it properly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a valid concern. What's about 2a51144 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

… because it could go out of sync soon

Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
# (advanced) Override the default cipher suite list (separated by commas).
# Allowed values:
#
# Secure Ciphers:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Secure Ciphers:
# Secure ciphers:

# - TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
# - TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256
#
# Insecure Ciphers:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Insecure Ciphers:
# Insecure ciphers:

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are auto-generated from code in another library. I will change it there.

# (advanced) Override the default cipher suite list (separated by commas).
# Allowed values:
#
# Secure Ciphers:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Secure Ciphers:
# Secure ciphers:

# - TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
# - TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256
#
# Insecure Ciphers:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Insecure Ciphers:
# Insecure ciphers:

# (advanced) Override the default cipher suite list (separated by commas).
# Allowed values:
#
# Secure Ciphers:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Secure Ciphers:
# Secure ciphers:

# - TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
# - TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256
#
# Insecure Ciphers:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Insecure Ciphers:
# Insecure ciphers:

# (advanced) Override the default cipher suite list (separated by commas).
# Allowed values:
#
# Secure Ciphers:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Secure Ciphers:
# Secure ciphers:

# - TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
# - TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256
#
# Insecure Ciphers:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Insecure Ciphers:
# Insecure ciphers:

# (advanced) Override the default cipher suite list (separated by commas).
# Allowed values:
#
# Secure Ciphers:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Secure Ciphers:
# Secure ciphers:

# - TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
# - TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256
#
# Insecure Ciphers:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Insecure Ciphers:
# Insecure ciphers:

# (advanced) Override the default cipher suite list (separated by commas).
# Allowed values:
#
# Secure Ciphers:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Secure Ciphers:
# Secure ciphers:

# - TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
# - TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256
#
# Insecure Ciphers:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Insecure Ciphers:
# Insecure ciphers:

Copy link
Contributor

@osg-grafana osg-grafana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving with minor nits that I found along the way while looking at every line.

pracucci and others added 2 commits September 28, 2022 13:17
Co-authored-by: Ursula Kallio <ursula.kallio@grafana.com>
@pracucci pracucci enabled auto-merge (squash) September 28, 2022 11:24
@pracucci pracucci merged commit 00a5740 into main Sep 28, 2022
@pracucci pracucci deleted the update-dskit branch September 28, 2022 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/docs Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants