Skip to content

tlsconfig: align client and server defaults, remove weak CBC ciphers#128

Merged
thaJeztah merged 2 commits into
docker:masterfrom
thaJeztah:remove_old_cyphers
May 30, 2025
Merged

tlsconfig: align client and server defaults, remove weak CBC ciphers#128
thaJeztah merged 2 commits into
docker:masterfrom
thaJeztah:remove_old_cyphers

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah commented May 13, 2025


tlsconfig: align client and server, remove weak CBC ciphers

These ciphers were split between server and client in docker@9b43f5a
(Docker v1.8.0, Jun 11, 2015);

removing the CBC ciphers from the client preferred TLS cipher suites.
This will allow a future version of the server to also remove the CBC
ciphers from the accepted list.

This changes the server default to client + additional CBC cipher list,
and client default to the non-CBC ciphers.

That change allowed phasing out the use of these ciphers in the client,
but (for backward-compatibility with older clients) the daemon to still
accept old ones.

Given that no current client versions should still be using these, we
should be able to remove them from the list of ciphers that are supported
by the daemon.

Now that client and server are the same, we can also use a single implementation
for both.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah thaJeztah force-pushed the remove_old_cyphers branch from 4223a0c to 9022a54 Compare May 13, 2025 15:00
@thaJeztah thaJeztah changed the title tlsconfig: align client and server, remove weak CBC ciphers tlsconfig: align client and server defaults, remove weak CBC ciphers May 13, 2025
@thaJeztah thaJeztah force-pushed the remove_old_cyphers branch from 9022a54 to 376f97b Compare May 14, 2025 11:00
@thaJeztah thaJeztah marked this pull request as ready for review May 14, 2025 11:01
@thaJeztah
Copy link
Copy Markdown
Member Author

cc @vvoland (just an extra pair of eyes)

@thaJeztah thaJeztah force-pushed the remove_old_cyphers branch from 376f97b to 9848a3a Compare May 29, 2025 10:28
@thaJeztah thaJeztah requested review from Copilot and vvoland May 29, 2025 13:22
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR unifies client and server TLS cipher defaults, removes outdated CBC ciphers, and consolidates configuration logic into a single helper.

  • Removed the separate client-only cipher list and replaced it with defaultCipherSuites
  • Introduced defaultConfig for both ClientDefault and ServerDefault, simplifying duplication
  • Updated tests to assert against the new defaultCipherSuites

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
tlsconfig/config.go Consolidated cipher lists, added defaultConfig, refactored ClientDefault/ServerDefault
tlsconfig/config_client_ciphers.go Deleted obsolete file defining client-only ciphers
tlsconfig/config_test.go Updated tests to expect defaultCipherSuites instead of clientCipherSuites
Comments suppressed due to low confidence (2)

tlsconfig/config.go:39

  • Fix the typo in the comment: change 'should be uses' to 'should be used'.
// DefaultServerAcceptedCiphers should be uses by code which already has a crypto/tls

tlsconfig/config_test.go:398

  • Enhance the failure message to include both expected and actual slices, e.g., t.Fatalf("Unexpected client cipher suites: got %v, want %v", tlsConfig.CipherSuites, defaultCipherSuites) for better diagnostics.
if !reflect.DeepEqual(tlsConfig.CipherSuites, defaultCipherSuites) {
Comment thread tlsconfig/config.go
Comment thread tlsconfig/config.go
// Client returns a TLS configuration meant to be used by a client.
func Client(options Options) (*tls.Config, error) {
tlsConfig := ClientDefault()
tlsConfig := defaultConfig()
Copy link

Copilot AI May 29, 2025

Choose a reason for hiding this comment

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

[nitpick] Use ClientDefault() instead of calling defaultConfig() directly to leverage the named wrapper and ensure consistency if the wrapper logic evolves.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not doing this currently; considering to deprecate ClientDefault and ServerDefault, in favour of a single default (or potentially "none"); see moby/moby#49611

Comment thread tlsconfig/config.go
// Server returns a TLS configuration meant to be used by a server.
func Server(options Options) (*tls.Config, error) {
tlsConfig := ServerDefault()
tlsConfig := defaultConfig()
Copy link

Copilot AI May 29, 2025

Choose a reason for hiding this comment

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

[nitpick] Call ServerDefault() here rather than invoking defaultConfig() directly, preserving the intended abstraction and future-proofing any server-specific options.

Suggested change
tlsConfig := defaultConfig()
tlsConfig := ServerDefault()
Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not doing this currently; considering to deprecate ClientDefault and ServerDefault, in favour of a single default (or potentially "none"); see moby/moby#49611

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the remove_old_cyphers branch from 9848a3a to 2282968 Compare May 30, 2025 07:19
@thaJeztah thaJeztah requested a review from Copilot May 30, 2025 08:23
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR unifies the TLS client and server default configurations by consolidating cipher suites into a single set and removing weak CBC cipher suites.

  • Updated tests to check for the unified default cipher suites.
  • Removed the dedicated client cipher suites file and replaced it with a common default.
  • Refactored configuration functions to use a shared defaultConfig function.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
tlsconfig/config_test.go Updated tests to expect the unified default cipher suites.
tlsconfig/config_client_ciphers.go Removed dedicated client cipher suite variable.
tlsconfig/config.go Consolidated cipher suite defaults and refactored configuration functions.
Comment thread tlsconfig/config.go Outdated
These ciphers were split between server and client in [docker/go-connections@9b43f5a]
(Docker v1.8.0, Jun 11, 2015);

> removing the CBC ciphers from the client preferred TLS cipher suites.
> This will allow a future version of the server to also remove the CBC
> ciphers from the accepted list.
>
> This changes the server default to client + additional CBC cipher list,
> and client default to the non-CBC ciphers.

That change allowed phasing out the use of these ciphers in the client,
but (for backward-compatibility with older clients) the daemon to still
accept old ones.

Given that no current client versions should still be using these, we
should be able to remove them from the list of ciphers that are supported
by the daemon.

Now that client and server are the same, we can also use a single implementation
for both.

[docker/go-connections@9b43f5a]: moby/moby@9b43f5a

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the remove_old_cyphers branch from 2282968 to deccd71 Compare May 30, 2025 08:27
@thaJeztah
Copy link
Copy Markdown
Member Author

Let me bring this one in, thanks!

@thaJeztah thaJeztah merged commit b071e04 into docker:master May 30, 2025
13 checks passed
@thaJeztah thaJeztah deleted the remove_old_cyphers branch May 30, 2025 09:13
@slonopotamus
Copy link
Copy Markdown
Contributor

slonopotamus commented May 30, 2025

Is it intentional that this PR closed moby/moby#36442? Changes seem to be unrelated at first glance...

@thaJeztah
Copy link
Copy Markdown
Member Author

OH! Definitely not intentional; looks like I may have copy/pasta'd a wrong link.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

5 participants