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.

Sign up for GitHub

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

Jump to bottom

cmd/skipper: allow exclusion of insecure cipher suites #3123

Merged
merged 3 commits into from
Jun 27, 2024
Merged

cmd/skipper: allow exclusion of insecure cipher suites #3123

merged 3 commits into from
Jun 27, 2024

Conversation

rickhlx
Copy link
Contributor

@rickhlx rickhlx commented Jun 21, 2024 •
edited by AlexanderYastrebov
Loading

Golang maintains a list of cipher suites considered insecure, which are still allowed if requested. This flag will allow those cipher suites to be completely excluded.

Options considered:

Use a list of allowed cipher suites
This may need some maintenance over time as cipher suites are updated, introduced or deprecated.

Exclude used cipher suites based on name
Less maintenance overhead than maintaining desired list of cipher suites, excluding the ones not desired would also require some maintenance overtime as cipher suites are considered insecure.

Exclude known insecure cipher suites
Using golang's list of InsecureCipherSuites reducing maintenance overhead by allowing list to be maintained by golang.

Fixes #3121

@rickhlx rickhlx force-pushed the exclude-cipher-suites branch from 3f15358 to 006701f Compare June 21, 2024 02:45
AlexanderYastrebov
AlexanderYastrebov reviewed Jun 21, 2024
View reviewed changes
config/config.go
@@ -219,6 +219,9 @@ type Config struct {
// TLS version
TLSMinVersion string `yaml:"tls-min-version"`

// Exclude insecure cipher suites
ExcludeInsecureCipherSuites bool `yaml:"exclude-insecure-cipher-suites"`
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to exclude from a default set or rather enumerate allowed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be worthy of a discussion. I've updated the description with the options considered to allow for secure cipher suites.

I don't have a strong preference and went with the one requiring less maintenance.

Copy link
Member

@szuecs szuecs Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

So if Go stdlib adds more "secure" ciphers than nobody needs to do anything and it will deployed automatically as a side effect.
This is good because likely Go stdlib will include only more secure cipher suites and not less secure ones.
The bad thing is for people that have regulations like FIPS, which may or may not be up to date with secure cipher suite definitions.

Right now I have no opinion on which one is better, but maybe rather to say we should have also an allow list or an exclude list for giving the user a choice based on their needs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd favor having an optional allow list for that case, since it's likely one would want to be specific about what's allowed.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should start with a single boolean flag to disable insecure suites and then add a list flag if we even find a need for it.

@rickhlx rickhlx force-pushed the exclude-cipher-suites branch 3 times, most recently from 435cece to 8bc94a6 Compare June 21, 2024 11:51
@rickhlx rickhlx marked this pull request as ready for review June 21, 2024 18:32
@AlexanderYastrebov AlexanderYastrebov added the major moderate risk, for example new API, small filter changes that have no risk like refactoring or logs label Jun 24, 2024
@rickhlx rickhlx force-pushed the exclude-cipher-suites branch from f00d3c1 to a6576ac Compare June 24, 2024 11:04
szuecs
szuecs reviewed Jun 24, 2024
View reviewed changes
skipper.go Outdated Show resolved Hide resolved
@rickhlx rickhlx force-pushed the exclude-cipher-suites branch from a6576ac to 5fae448 Compare June 24, 2024 12:59
AlexanderYastrebov
AlexanderYastrebov reviewed Jun 24, 2024
View reviewed changes
skipper_test.go Outdated Show resolved Hide resolved
@rickhlx rickhlx force-pushed the exclude-cipher-suites branch from 91c6432 to 0364193 Compare June 24, 2024 14:28
@rickhlx rickhlx force-pushed the exclude-cipher-suites branch from 0364193 to 57863a2 Compare June 24, 2024 19:27
@AlexanderYastrebov
Copy link
Member

👍

rickhlx added 3 commits June 25, 2024 05:38
@rickhlx
cmd/skipper: allow exclusion of insecure cipher suites
72a8987 
Golang maintains a list of cipher suites considered
insecure, which are still allowed if requested. This flag
will allow those cipher suites to be completely excluded.

Signed-off-by: Ricardo Herrera <rickhl@outlook.com>
@rickhlx
Add tests for insecure cipher suite config
e0b34d4 
Signed-off-by: Ricardo Herrera <rickhl@outlook.com>
@rickhlx
Test ToOption is a suite list with excluded suites
fcad0f0 
Signed-off-by: Ricardo Herrera <rickhl@outlook.com>
@rickhlx rickhlx force-pushed the exclude-cipher-suites branch from 57863a2 to fcad0f0 Compare June 25, 2024 09:38
@AlexanderYastrebov
Copy link
Member

👍

@rickhlx rickhlx requested a review from szuecs June 26, 2024 13:48
@rickhlx
Copy link
Contributor Author

rickhlx commented Jun 27, 2024

@szuecs @AlexanderYastrebov any chance we can get this merged? We're looking to solve some security issues by deploying this change.

@MustafaSaber
Copy link
Member

👍

@MustafaSaber
Copy link
Member

Thanks for your contribution!

@MustafaSaber MustafaSaber merged commit 428edb8 into zalando:master Jun 27, 2024
10 checks passed
MustafaSaber added a commit to zalando-incubator/kubernetes-on-aws that referenced this pull request Jun 27, 2024
@MustafaSaber
update skipper canary version
a473620 
* build(deps): bump alpine from `77726ef` to `b89d9c9` in /packaging: zalando/skipper#3122
* build(deps): bump docker/build-push-action from 5.4.0 to 6.1.0: zalando/skipper#3124
* build(deps): bump amazonlinux from `0d172f8` to `b0016cb` in /fuzz: zalando/skipper#3125
* Facilitate OPA decision correlation with business flows: zalando/skipper#3041
* config: fix defaultFiltersFlags.String: zalando/skipper#3127
* config: fix defaultFiltersFlags yaml test case: zalando/skipper#3128
* filters/auth: add token validator filter: zalando/skipper#3126
* metrics: register skipper_filter_create_duration_seconds: zalando/skipper#3129
* cmd/skipper: allow exclusion of insecure cipher suites: zalando/skipper#3123

diff zalando/skipper@v0.21.124...v0.21.133
@MustafaSaber MustafaSaber mentioned this pull request Jun 27, 2024
Merged
MustafaSaber added a commit to zalando-incubator/kubernetes-on-aws that referenced this pull request Jun 27, 2024
@MustafaSaber
Merge pull request #7757 from zalando-incubator/skipper/update-canary…
2470e9a 
…-version

skipper: update canary version to v0.21.133

* build(deps): bump alpine from `77726ef` to `b89d9c9` in /packaging: zalando/skipper#3122
* build(deps): bump docker/build-push-action from 5.4.0 to 6.1.0: zalando/skipper#3124
* build(deps): bump amazonlinux from `0d172f8` to `b0016cb` in /fuzz: zalando/skipper#3125
* Facilitate OPA decision correlation with business flows: zalando/skipper#3041
* config: fix defaultFiltersFlags.String: zalando/skipper#3127
* config: fix defaultFiltersFlags yaml test case: zalando/skipper#3128
* filters/auth: add token validator filter: zalando/skipper#3126
* metrics: register skipper_filter_create_duration_seconds: zalando/skipper#3129
* cmd/skipper: allow exclusion of insecure cipher suites: zalando/skipper#3123

diff zalando/skipper@v0.21.124...v0.21.133
MustafaSaber added a commit to zalando-incubator/kubernetes-on-aws that referenced this pull request Jun 27, 2024
@MustafaSaber
skipper: update main fleet version to v0.21.133
cecb07a 
* build(deps): bump alpine from `77726ef` to `b89d9c9` in /packaging: zalando/skipper#3122
* build(deps): bump docker/build-push-action from 5.4.0 to 6.1.0: zalando/skipper#3124
* build(deps): bump amazonlinux from `0d172f8` to `b0016cb` in /fuzz: zalando/skipper#3125
* Facilitate OPA decision correlation with business flows: zalando/skipper#3041
* config: fix defaultFiltersFlags.String: zalando/skipper#3127
* config: fix defaultFiltersFlags yaml test case: zalando/skipper#3128
* filters/auth: add token validator filter: zalando/skipper#3126
* metrics: register skipper_filter_create_duration_seconds: zalando/skipper#3129
* cmd/skipper: allow exclusion of insecure cipher suites: zalando/skipper#3123

diff zalando/skipper@v0.21.124...v0.21.133

depends on #7757
@MustafaSaber MustafaSaber mentioned this pull request Jun 27, 2024
Merged
MustafaSaber added a commit to zalando-incubator/kubernetes-on-aws that referenced this pull request Jun 27, 2024
@MustafaSaber
skipper: update main fleet version to v0.21.133
291e443 
* build(deps): bump alpine from `77726ef` to `b89d9c9` in /packaging: zalando/skipper#3122
* build(deps): bump docker/build-push-action from 5.4.0 to 6.1.0: zalando/skipper#3124
* build(deps): bump amazonlinux from `0d172f8` to `b0016cb` in /fuzz: zalando/skipper#3125
* Facilitate OPA decision correlation with business flows: zalando/skipper#3041
* config: fix defaultFiltersFlags.String: zalando/skipper#3127
* config: fix defaultFiltersFlags yaml test case: zalando/skipper#3128
* filters/auth: add token validator filter: zalando/skipper#3126
* metrics: register skipper_filter_create_duration_seconds: zalando/skipper#3129
* cmd/skipper: allow exclusion of insecure cipher suites: zalando/skipper#3123

diff zalando/skipper@v0.21.124...v0.21.133

depends on #7757
MustafaSaber added a commit to zalando-incubator/kubernetes-on-aws that referenced this pull request Jul 2, 2024
@MustafaSaber
skipper: update main fleet version to v0.21.139
59807a3 
* build(deps): bump alpine from `77726ef` to `b89d9c9` in /packaging: zalando/skipper#3122
* build(deps): bump docker/build-push-action from 5.4.0 to 6.1.0: zalando/skipper#3124
* build(deps): bump amazonlinux from `0d172f8` to `b0016cb` in /fuzz: zalando/skipper#3125
* Facilitate OPA decision correlation with business flows: zalando/skipper#3041
* config: fix defaultFiltersFlags.String: zalando/skipper#3127
* config: fix defaultFiltersFlags yaml test case: zalando/skipper#3128
* filters/auth: add token validator filter: zalando/skipper#3126
* metrics: register skipper_filter_create_duration_seconds: zalando/skipper#3129
* cmd/skipper: allow exclusion of insecure cipher suites: zalando/skipper#3123
* Revert "Facilitate OPA decision correlation with business flows (#3041)": zalando/skipper#3138
* build(deps): bump docker/build-push-action from 6.1.0 to 6.2.0: zalando/skipper#3134
* dependabot: group GoLang dependencies update: zalando/skipper#3136
* build(deps): bump github.com/open-policy-agent/opa from 0.65.0 to 0.66.0: zalando/skipper#3135
* build(deps): bump amazonlinux from `b0016cb` to `5bf7910` in /fuzz: zalando/skipper#3133
* metrics: refactor prometheus metric registration: zalando/skipper#3132

diff zalando/skipper@v0.21.124...v0.21.139

depends on #7786
MustafaSaber added a commit to zalando-incubator/kubernetes-on-aws that referenced this pull request Jul 2, 2024
@MustafaSaber
skipper: update main fleet version to v0.21.139
9847f31 
* build(deps): bump alpine from `77726ef` to `b89d9c9` in /packaging: zalando/skipper#3122
* build(deps): bump docker/build-push-action from 5.4.0 to 6.1.0: zalando/skipper#3124
* build(deps): bump amazonlinux from `0d172f8` to `b0016cb` in /fuzz: zalando/skipper#3125
* Facilitate OPA decision correlation with business flows: zalando/skipper#3041
* config: fix defaultFiltersFlags.String: zalando/skipper#3127
* config: fix defaultFiltersFlags yaml test case: zalando/skipper#3128
* filters/auth: add token validator filter: zalando/skipper#3126
* metrics: register skipper_filter_create_duration_seconds: zalando/skipper#3129
* cmd/skipper: allow exclusion of insecure cipher suites: zalando/skipper#3123
* Revert "Facilitate OPA decision correlation with business flows (#3041)": zalando/skipper#3138
* build(deps): bump docker/build-push-action from 6.1.0 to 6.2.0: zalando/skipper#3134
* dependabot: group GoLang dependencies update: zalando/skipper#3136
* build(deps): bump github.com/open-policy-agent/opa from 0.65.0 to 0.66.0: zalando/skipper#3135
* build(deps): bump amazonlinux from `b0016cb` to `5bf7910` in /fuzz: zalando/skipper#3133
* metrics: refactor prometheus metric registration: zalando/skipper#3132

diff zalando/skipper@v0.21.124...v0.21.139

depends on #7786
MustafaSaber added a commit to zalando-incubator/kubernetes-on-aws that referenced this pull request Jul 2, 2024
@MustafaSaber
skipper: update main fleet version to v0.21.139
eaaf541 
* build(deps): bump alpine from `77726ef` to `b89d9c9` in /packaging: zalando/skipper#3122
* build(deps): bump docker/build-push-action from 5.4.0 to 6.1.0: zalando/skipper#3124
* build(deps): bump amazonlinux from `0d172f8` to `b0016cb` in /fuzz: zalando/skipper#3125
* Facilitate OPA decision correlation with business flows: zalando/skipper#3041
* config: fix defaultFiltersFlags.String: zalando/skipper#3127
* config: fix defaultFiltersFlags yaml test case: zalando/skipper#3128
* filters/auth: add token validator filter: zalando/skipper#3126
* metrics: register skipper_filter_create_duration_seconds: zalando/skipper#3129
* cmd/skipper: allow exclusion of insecure cipher suites: zalando/skipper#3123
* Revert "Facilitate OPA decision correlation with business flows (#3041)": zalando/skipper#3138
* build(deps): bump docker/build-push-action from 6.1.0 to 6.2.0: zalando/skipper#3134
* dependabot: group GoLang dependencies update: zalando/skipper#3136
* build(deps): bump github.com/open-policy-agent/opa from 0.65.0 to 0.66.0: zalando/skipper#3135
* build(deps): bump amazonlinux from `b0016cb` to `5bf7910` in /fuzz: zalando/skipper#3133
* metrics: refactor prometheus metric registration: zalando/skipper#3132

diff zalando/skipper@v0.21.124...v0.21.139

depends on #7786
@zalando-teapot-robot zalando-teapot-robot mentioned this pull request Sep 23, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexanderYastrebov AlexanderYastrebov AlexanderYastrebov left review comments

@szuecs szuecs Awaiting requested review from szuecs

Assignees
No one assigned
Labels
major moderate risk, for example new API, small filter changes that have no risk like refactoring or logs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Insecure cipher suites used for TLS 1.2
4 participants
@rickhlx @AlexanderYastrebov @MustafaSaber @szuecs