fix: apply the configured TLS cipher suites

What does this MR do?

The registry has not applied http.tls.ciphersuites since the option was added in !1930 (merged) (38a3e89a, "feat: add option to configure TLS ciphers"). getTLSConfig declares the parsed suites with := inside its else branch, which shadows the tlsCipherSuites variable it later hands to tls.Config, so tls.Config.CipherSuites is always nil. Whatever the configuration says, and including the default list ApplyDefaults fills in, every TLS listener serves the whole set Go enables for TLS 1.2. The startup log still prints restricting TLS cipher suites to: ... with the configured names.

This MR applies a configured list and keeps Go's defaults when none is configured, in four places that have to land together:

  1. getTLSConfig assigns to the outer variable instead of declaring a new one.
  2. getCipherSuites returns nil for an empty list, as its doc comment already says. Today it returns an empty, non-nil slice, and crypto/tls falls back to its defaults only for a nil one. Without this, the first change would make a listener with no list refuse every TLS 1.2 handshake: the debug listener, which passes no suites at all on master (I ran that case, see below), and, with the third change, every listener whose ciphersuites is unset.
  3. ApplyDefaults no longer fills in a default list when http.tls.ciphersuites is not set, so an unset list keeps the TLS 1.2 suites Go enables by default, which is what every registry has served since !1930 (merged). Applying the documented default list now would drop the four ECDHE CBC-SHA suites Go also enables (TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA, TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA and TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA) for everyone using http.tls. The docs say that Go's defaults apply instead of listing a default, and getTLSConfig logs the restriction, and the warning that TLS 1.3 ignores it, only when a list is configured.
  4. The debug server gets the setting it never had. DebugTLS has no ciphersuites field, so http.debug.tls.ciphersuites, which the full example in docs/configuration.md shows and the debug section says is available, is dropped while parsing. DebugTLS gains the field, ApplyDefaults copies http.tls.ciphersuites into it when debug TLS is enabled and the field is unset (the rule it already applies to clientcas and minimumtls), configureMonitoring passes it on, and the docs say so in one sentence.

TestRegistryUnsupportedCipherSuite did not catch this because it offered TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA to a server holding an RSA certificate, which fails whatever the restriction is. It passes on master. It now offers TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, which suits the certificate and which Go enables by default, so only the restriction can refuse it.

What changes for a running registry

This only affects deployments where the registry terminates TLS itself, that is with http.tls (or http.debug.tls) configured. The Omnibus template writes no http.tls for the registry, and the Helm chart writes it only when registry TLS is enabled.

  • A deployment that never set ciphersuites sees no change: it keeps the TLS 1.2 suites Go enables by default, as it has since !1930 (merged).
  • A deployment that set ciphersuites now gets exactly that list, which can refuse clients that connect today, or admit ones Go leaves out by default (for example the TLS_RSA_* suites).
  • TLS 1.3 is unaffected, as is minimumtls: tls1.3.
  • The debug listener inherits the main list unless http.debug.tls.ciphersuites sets its own, and keeps Go's defaults when neither is set. A debug certificate with another key type than the main one therefore needs its own list when the main list only names suites for one key type, or TLS 1.2 handshakes to the debug port fail (TLS 1.3 is unaffected). The Helm chart has no value for the debug list today.
  • http.debug.tls.ciphersuites was ignored until now, so a name the registry does not know there did no harm. It is now parsed, and such a name makes the debug server log failed to configure TLS for debug server and serve plain HTTP, which is what an unknown http.debug.tls.minimumtls already does. Making a TLS setup error fail the debug server at startup, as it fails the main listener, comes in a separate MR.

An unset list behaves as before, so only a deployment that configured a list sees a change, and that change is the fix.

Related to !1930 (merged)

The tests, and how they fail on master

New or changed tests:

  • TestRegistryUnsupportedCipherSuite: offers an RSA CBC suite the server is not configured with and expects a handshake failure.
  • TestGetTLSConfig_CipherSuites: the configured suites end up in tls.Config.CipherSuites; no configured suites leave it nil; minimumtls: tls1.3 leaves it nil; the cipher suite log lines appear only when a list is configured.
  • TestGetCipherSuite: getCipherSuites(nil) and getCipherSuites([]string{}) return nil.
  • TestConfigureMonitoring_DebugTLSCipherSuites: starts the debug server with TLS and checks over a real TLS 1.2 handshake that, with nothing configured, a CBC suite Go enables by default is accepted; and that the configured GCM suite is accepted and a CBC suite refused, once with the list inherited from http.tls and once with http.debug.tls.ciphersuites set to a different list than http.tls (whose suite must then be refused as well).
  • TestParseHTTPDebugTLS_CipherSuites (YAML and REGISTRY_HTTP_DEBUG_TLS_CIPHERSUITES) and TestParseHTTPDebugTLS_CipherSuitesInheritance (inherited when unset, own value kept, Go defaults kept when neither is set, nothing inherited while debug TLS is disabled).
  • TestConfigSuite: the parsed reference configuration carries no cipher suites, since ApplyDefaults adds none (copyConfig now keeps a nil list nil).

I wrote the first version of these tests against master at dcc295c9 and rebased the branch onto 15db9e60 afterwards, which only bumps azidentity in go.mod and go.sum. On dcc295c9, Go 1.26.8, with the set on http.debug.tls case left out because it needs the new field:

--- FAIL: TestConfigureMonitoring_DebugTLSCipherSuites/inherited_from_http.tls
        Error:      An error is expected but got nil.
        Messages:   TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA
--- FAIL: TestGetCipherSuite
        Error:      Expected nil, but got: []uint16{}
--- FAIL: TestGetTLSConfig_CipherSuites/configured_suites_are_applied
        expected: []uint16{0xc02b, 0xc02c}
        actual  : []uint16(nil)
--- FAIL: TestRegistryUnsupportedCipherSuite
        Error:      An error is expected but got nil.

That case and the two new configuration tests do not compile there, because DebugTLS has no CipherSuites field:

configuration/configuration_test.go:1055:54: got.HTTP.Debug.TLS.CipherSuites undefined (type DebugTLS has no field or method CipherSuites)
registry/registry_test.go:456:27: config.HTTP.Debug.TLS.CipherSuites undefined (type configuration.DebugTLS has no field or method CipherSuites)

With only the first change (the shadowing fixed, getCipherSuites unchanged, nothing passed to the debug listener), both debug listener cases fail on the suite that should be accepted:

--- FAIL: TestConfigureMonitoring_DebugTLSCipherSuites/inherited_from_http.tls
        Error:      Received unexpected error:
                    remote error: tls: handshake failure
--- FAIL: TestConfigureMonitoring_DebugTLSCipherSuites/set_on_http.debug.tls
        Error:      Received unexpected error:
                    remote error: tls: handshake failure
--- FAIL: TestGetTLSConfig_CipherSuites/no_configured_suites_keep_the_Go_defaults
        expected: []uint16(nil)
        actual  : []uint16{}

Against the first version of this branch (b1b625cc), which still filled in the default list, the tests for Go's defaults fail on Go 1.26.8:

--- FAIL: TestConfigureMonitoring_DebugTLSCipherSuites/Go_defaults_when_neither_is_set
        Error:      Received unexpected error:
                    remote error: tls: handshake failure
--- FAIL: TestGetTLSConfig_CipherSuites/no_configured_suites_keep_the_Go_defaults
        expected: []string(nil)
        actual  : []string{"restricting TLS cipher suites to: "}
--- FAIL: TestGetTLSConfig_CipherSuites/tls1.3_with_no_configured_suites
        expected: []string(nil)
        actual  : []string{"ignoring configured TLS cipher suites as they are not configurable in tls1.3"}
--- FAIL: TestParseHTTPDebugTLS_CipherSuitesInheritance/Go_defaults_kept_when_neither_is_set
        expected: []string(nil)
        actual  : []string{"TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256", ...six suites}
--- FAIL: TestConfigSuite (13 subtests)
        -   CipherSuites: ([]string) <nil>,
        +   CipherSuites: ([]string) (len=6) {
The shadowed variable, and Go's default suites

The shadow analyzer (golang.org/x/tools v0.51.0) on ./registry/, on master at dcc295c9:

registry/registry.go:265:3: declaration of "tlsCipherSuites" shadows declaration at line 260

On this branch that line is gone; the five err shadows it also reports in registry.go and root.go are unchanged. The project's lint does not run shadow (govet is not in the enable list of .golangci.yml), which is how this one went unnoticed.

In the crypto/tls source of Go 1.25.14, 1.26.8 and 1.27.2, Config.cipherSuites uses the default list only when c.CipherSuites == nil. That default is cipherSuitesPreferenceOrder minus the disabled suites (CBC-SHA256, RC4, and by default the RSA key exchange and 3DES ones), which leaves the six suites the docs listed as the default plus the four ECDHE AES-CBC-SHA suites named above.

Checks I ran locally

All on the current branch:

  • go test -tags continuous_profiler_stackdriver ./registry/ ./configuration/ -count=1 on Go 1.25.14, 1.26.8 and 1.27.2 (the CI matrix): pass.
  • The TLS and configuration tests with -count=10: pass. Both packages under -race on Go 1.26.8: pass, with TestGracefulShutdown skipped, which reports a data race on master at 15db9e60 as well (its RegisterOnShutdown hook writes a variable the test reads without synchronization).
  • go vet ./registry/ ./configuration/: clean.
  • golangci-lint run ./... with golangci-lint v2.13.1, the version CI pins, on Go 1.26.8 and with the project's .golangci.yml: 0 issues; golangci-lint fmt --diff on the four Go files: no changes.
  • vale, markdownlint-cli2 and lychee --offline --include-fragments on docs/configuration.md from the lint-markdown image the docslint job uses: no findings.
  • commitlint with @commitlint/config-conventional in node:lts-alpine: 0 problems.

Author checklist

  • CODEOWNERS Review: This MR requires approval from at least one CODEOWNER per category/file.
    • If codeowners are absent or the change is urgent, any registry maintainer can temporarily disable CODEOWNERS reviews in the project settings. When doing so:
      • Add a comment on the MR justifying the bypass
      • Mention/CC the designated codeowners in the MR for async review
      • Re-enable CODEOWNERS reviews in project settings once the MR is merged
    • If you lack permissions to disable CODEOWNERS reviews, reach out to the Engineering Manager (@jaime) or Senior Engineering Manager (@crystalpoole) for assistance.
  • Assign one of conventional-commit prefixes to the MR.
    • fix: Indicates a bug fix, triggers a patch release.
    • feat: Signals the introduction of a new feature, triggers a minor release.
    • perf: Focuses on performance improvements that don't introduce new features or fix bugs, triggers a patch release.
    • docs: Updates or changes to documentation. Does not trigger a release.
    • style: Changes that do not affect the code's functionality. Does not trigger a release.
    • refactor: Modifications to the code that do not fix bugs or add features but improve code structure or readability. Does not trigger a release.
    • test: Changes related to adding or modifying tests. Does not trigger a release.
    • chore: Routine tasks that don't affect the application, such as updating build processes, package manager configs, etc. Does not trigger a release.
    • build: Changes that affect the build system or external dependencies. May trigger a release.
    • ci: Modifications to continuous integration configuration files and scripts. Does not trigger a release.
    • revert: Reverts a previous commit. It could result in a patch, minor, or major release.
  • MR contains database changes including schema/background migrations: not applicable.
  • Change contains a breaking change - apply the breaking change label: not one, an unset list behaves as before.
  • Change is considered high risk - apply the label high-risk-change
  • I created or linked to an existing issue for every added or updated TODO, BUG, FIXME or OPTIMIZE prefixed comment (there are none).
  • Changes cannot be rolled back: not applicable, a revert restores the previous behaviour.
Documentation/resources

Code review guidelines

Go Style guidelines

Feature flags

When documentation is required

Documentation workflow

Reviewer checklist

  • Ensure the commit and MR tittle are still accurate.
  • If the change contains a breaking change, verify the breaking change label.
  • If the change is considered high risk, verify the label high-risk-change
  • Identify if the change can be rolled back safely. (note: all other reasons for not being able to rollback will be sufficiently captured by major version changes).
Edited by José M. Requena Plens

Merge request reports

Loading
Loading