Added flag for user configurable cipher suites #3169

d-luu · 2020-05-26T20:15:22Z

Configuration of list of cipher suites allows a user to disable use
of weak ciphers or continue to support them for legacy usage if they
so choose.

List of available cipher suites at:
https://golang.org/pkg/crypto/tls/#pkg-constants

Default cipher suites have been updated to:

TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384
TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305
TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305
TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
TLS_AES_128_GCM_SHA256
TLS_CHACHA20_POLY1305_SHA256
TLS_AES_256_GCM_SHA384

MinimumTLS has also been updated to include TLS 1.3 as an option
and now defaults to TLS 1.2 since 1.0 and 1.1 have been deprecated.

Also updated install-dev-tools to use recommended golangci-lint
install to fix CI pipeline.

Signed-off-by: David Luu david@davidluu.info

d-luu · 2020-05-26T20:20:39Z

regarding the golangci-lint installation method, they mention that go get isn't guaranteed to work:
https://golangci-lint.run/usage/install/#install-from-source

in regards to cipher suites:

before:

# for v in ssl2 ssl3 tls1 tls1_1 tls1_2; do
>  for c in $(openssl ciphers 'ALL:eNULL' | tr ':' ' '); do
>  openssl s_client -connect ${SERVER} \
>  -cipher $c -$v < /dev/null > /dev/null 2>&1 && echo -e "$v:\t$c"
>  done
> done
tls1:	ECDHE-RSA-AES256-SHA
tls1:	AES256-SHA
tls1:	ECDHE-RSA-AES128-SHA
tls1:	AES128-SHA
tls1_1:	ECDHE-RSA-AES256-SHA
tls1_1:	AES256-SHA
tls1_1:	ECDHE-RSA-AES128-SHA
tls1_1:	AES128-SHA
tls1_2:	ECDHE-RSA-AES256-SHA
tls1_2:	AES256-SHA
tls1_2:	ECDHE-RSA-AES128-GCM-SHA256
tls1_2:	ECDHE-RSA-AES128-SHA
tls1_2:	AES128-SHA

after

# for v in ssl2 ssl3 tls1 tls1_1 tls1_2; do
>  for c in $(openssl ciphers 'ALL:eNULL' | tr ':' ' '); do
>  openssl s_client -connect ${SERVER} \
>  -cipher $c -$v < /dev/null > /dev/null 2>&1 && echo -e "$v:\t$c"
>  done
> done
tls1_2:	ECDHE-RSA-AES256-GCM-SHA384
tls1_2:	ECDHE-RSA-AES128-GCM-SHA256

huornlmj · 2020-09-04T10:42:41Z

When is this going to go through? TLS 1.0 in this day is wrong.

gaby · 2021-02-02T05:24:01Z

Any plans to merge this? This is way overdue.

milosgajdos · 2021-02-17T16:25:21Z

@d-luu can you rebase, please? Thanks.

d-luu · 2021-02-19T00:02:42Z

let me verify something before merging

d-luu · 2021-02-19T01:17:56Z

@milosgajdos
I was checking to see if by using the registry docker image we can set the cipher suite via the indexed environment variable REGISTRY_HTTL_TLS_CIPHERSUITES_0=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 rather than REGISTRY_HTTL_TLS_CIPHERSUITES=' - TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 but it doesn't look to be the case. The former doesn't work, the latter does.

I recall awhile back being able to pass ClientCAs via the list index _0 env var and I don't see anything that stands out at the moment that would cause it to be treated differently: https://github.com/distribution/distribution/blob/main/configuration/parser.go

milosgajdos · 2021-02-19T11:36:37Z

Thanks for the awesome work on this, @d-luu

I'm planning on setting this up locally over this weekend and having a proper look.

milosgajdos

I made a small style suggestion, but this PR looks nice. Thanks for putting it together.

registry/registry.go

registry/registry_test.go

waynr

All I've got are nits, otherwise 🙈

waynr · 2021-02-25T14:25:22Z

registry/registry.go

+	"TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384":         tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,
+	"TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384":       tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,
+	"TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256":   tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,
+	"TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256": tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305,


I was initially a little confused since for these last two 1.0 - 1.2 cipher suites the string name doesn't match the constant name. I looked it up here: https://golang.org/pkg/crypto/tls/#pkg-constants and see that you are using the legacy constant name which ultimately has the same value as the corresponding _SHA256-suffixed constant. I think it'd be less confusing just to use the matching constant name here.

I can remove those

waynr · 2021-02-25T14:26:42Z

registry/registry.go

+	tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,
+	tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,
+	tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305,
+	tls.TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305,


I know it probably doesn't matter much, but I would prefer to be consistent in the usage of cipher suite names by using TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256 and TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256.

waynr · 2021-02-25T14:28:27Z

registry/registry.go

+	if len(names) == 0 {
+		return defaultCipherSuites, nil
+	}
+	cipherSuiteConsts := make([]uint16, 0)


super nit:

cipherSuiteConsts := make([]uint16, len(names))

waynr · 2021-02-25T14:29:34Z

registry/registry.go

+	if len(ids) == 0 {
+		return nil
+	}
+	var names []string


nit: in getCipherSuites you use a different style of slice initialization, it'd be nice to be consistent within the same commit

codecov-io · 2021-02-25T17:01:02Z

Codecov Report

Merging #3169 (7fff010) into main (22c0748) will increase coverage by 0.38%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #3169      +/-   ##
==========================================
+ Coverage   55.84%   56.22%   +0.38%     
==========================================
  Files         102      102              
  Lines        7222     7224       +2     
==========================================
+ Hits         4033     4062      +29     
+ Misses       2552     2520      -32     
- Partials      637      642       +5

Impacted Files	Coverage Δ
...bution/distribution/configuration/configuration.go	`62.03% <0.00%> (ø)`
...com/distribution/distribution/registry/registry.go	`43.39% <0.00%> (+13.39%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22c0748...7fff010. Read the comment docs.

Configuration of list of cipher suites allows a user to disable use of weak ciphers or continue to support them for legacy usage if they so choose. List of available cipher suites at: https://golang.org/pkg/crypto/tls/#pkg-constants Default cipher suites have been updated to: - TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384 - TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 - TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305 - TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305 - TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 - TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 - TLS_AES_128_GCM_SHA256 - TLS_CHACHA20_POLY1305_SHA256 - TLS_AES_256_GCM_SHA384 MinimumTLS has also been updated to include TLS 1.3 as an option and now defaults to TLS 1.2 since 1.0 and 1.1 have been deprecated. Signed-off-by: David Luu <david@davidluu.info>

d-luu · 2021-02-25T23:23:02Z

squashed/rebased only

gaby · 2021-03-04T03:09:09Z

Any plans to make a new released? Docker Registry is still using TLS 1.0 see #3333

lefranco6910 · 2021-05-31T19:20:19Z

I ran the registry image from the development branch (2.7.0-272-gc63b5805) and TLS 1.0 and 1.1 are deprecated as indicated by d-luu. I think it's just a matter to get the modifications in the next release. You can always do like I did and get the development registry image in the mean time.

d-luu force-pushed the configurable_ciphersuites branch 4 times, most recently from f04d537 to aa22c55 Compare May 27, 2020 15:55

Base automatically changed from master to main January 27, 2021 15:51

This was referenced Feb 2, 2021

Docker Registry is vulnerable to CVE-2013-0169 (LUCKY13) and CVE-2011-3389 (BEAST) #3333

Closed

Registry - Allow minimum TLS version to be configurable #2807

Closed

milosgajdos added feature priority/P1 Major item. Should definitely have it. security status/2-code-review status/needs-attention labels Feb 2, 2021

d-luu force-pushed the configurable_ciphersuites branch from aa22c55 to 44553eb Compare February 18, 2021 21:32

milosgajdos approved these changes Feb 24, 2021

View reviewed changes

registry/registry.go Outdated Show resolved Hide resolved

registry/registry_test.go Outdated Show resolved Hide resolved

registry/registry_test.go Show resolved Hide resolved

d-luu force-pushed the configurable_ciphersuites branch from 54cd241 to 990b318 Compare February 25, 2021 06:47

waynr approved these changes Feb 25, 2021

View reviewed changes

d-luu force-pushed the configurable_ciphersuites branch from 7fff010 to 1e625d0 Compare February 25, 2021 23:22

milosgajdos merged commit 5a76dc8 into distribution:main Feb 26, 2021

This was referenced Feb 26, 2021

[backport release/2.7] Registry - make minimum TLS version user configurable #3359

Closed

Support for Handling Ciphers via Configuration #3096

Closed

rwwiv mentioned this pull request Jul 12, 2021

FEATURE: Set minimum TLS version for registry to 1.2 to match nginx config Security-Onion-Solutions/securityonion#4403

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added flag for user configurable cipher suites #3169

Added flag for user configurable cipher suites #3169

d-luu commented May 26, 2020 •

edited

d-luu commented May 26, 2020 •

edited

huornlmj commented Sep 4, 2020

gaby commented Feb 2, 2021

milosgajdos commented Feb 17, 2021

d-luu commented Feb 19, 2021

d-luu commented Feb 19, 2021

milosgajdos commented Feb 19, 2021

milosgajdos left a comment

waynr left a comment

waynr Feb 25, 2021

d-luu Feb 25, 2021

waynr Feb 25, 2021

waynr Feb 25, 2021

waynr Feb 25, 2021

codecov-io commented Feb 25, 2021

d-luu commented Feb 25, 2021 •

edited

gaby commented Mar 4, 2021

lefranco6910 commented May 31, 2021

Added flag for user configurable cipher suites #3169

Added flag for user configurable cipher suites #3169

Conversation

d-luu commented May 26, 2020 • edited

d-luu commented May 26, 2020 • edited

in regards to cipher suites:

huornlmj commented Sep 4, 2020

gaby commented Feb 2, 2021

milosgajdos commented Feb 17, 2021

d-luu commented Feb 19, 2021

d-luu commented Feb 19, 2021

milosgajdos commented Feb 19, 2021

milosgajdos left a comment

Choose a reason for hiding this comment

waynr left a comment

Choose a reason for hiding this comment

waynr Feb 25, 2021

Choose a reason for hiding this comment

d-luu Feb 25, 2021

Choose a reason for hiding this comment

waynr Feb 25, 2021

Choose a reason for hiding this comment

waynr Feb 25, 2021

Choose a reason for hiding this comment

waynr Feb 25, 2021

Choose a reason for hiding this comment

codecov-io commented Feb 25, 2021

Codecov Report

d-luu commented Feb 25, 2021 • edited

gaby commented Mar 4, 2021

lefranco6910 commented May 31, 2021

d-luu commented May 26, 2020 •

edited

d-luu commented May 26, 2020 •

edited

d-luu commented Feb 25, 2021 •

edited