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:
getTLSConfigassigns to the outer variable instead of declaring a new one.getCipherSuitesreturnsnilfor an empty list, as its doc comment already says. Today it returns an empty, non-nil slice, andcrypto/tlsfalls back to its defaults only for anilone. 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 onmaster(I ran that case, see below), and, with the third change, every listener whoseciphersuitesis unset.ApplyDefaultsno longer fills in a default list whenhttp.tls.ciphersuitesis 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_SHAandTLS_ECDHE_RSA_WITH_AES_256_CBC_SHA) for everyone usinghttp.tls. The docs say that Go's defaults apply instead of listing a default, andgetTLSConfiglogs the restriction, and the warning that TLS 1.3 ignores it, only when a list is configured.- The debug server gets the setting it never had.
DebugTLShas nociphersuitesfield, sohttp.debug.tls.ciphersuites, which the full example indocs/configuration.mdshows and thedebugsection says is available, is dropped while parsing.DebugTLSgains the field,ApplyDefaultscopieshttp.tls.ciphersuitesinto it when debug TLS is enabled and the field is unset (the rule it already applies toclientcasandminimumtls),configureMonitoringpasses 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
ciphersuitessees no change: it keeps the TLS 1.2 suites Go enables by default, as it has since !1930 (merged). - A deployment that set
ciphersuitesnow gets exactly that list, which can refuse clients that connect today, or admit ones Go leaves out by default (for example theTLS_RSA_*suites). - TLS 1.3 is unaffected, as is
minimumtls: tls1.3. - The debug listener inherits the main list unless
http.debug.tls.ciphersuitessets 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.ciphersuiteswas 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 logfailed to configure TLS for debug serverand serve plain HTTP, which is what an unknownhttp.debug.tls.minimumtlsalready 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 intls.Config.CipherSuites; no configured suites leave itnil;minimumtls: tls1.3leaves itnil; the cipher suite log lines appear only when a list is configured.TestGetCipherSuite:getCipherSuites(nil)andgetCipherSuites([]string{})returnnil.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 fromhttp.tlsand once withhttp.debug.tls.ciphersuitesset to a different list thanhttp.tls(whose suite must then be refused as well).TestParseHTTPDebugTLS_CipherSuites(YAML andREGISTRY_HTTP_DEBUG_TLS_CIPHERSUITES) andTestParseHTTPDebugTLS_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, sinceApplyDefaultsadds none (copyConfignow keeps anillistnil).
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 260On 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=1on 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-raceon Go 1.26.8: pass, withTestGracefulShutdownskipped, which reports a data race onmasterat 15db9e60 as well (itsRegisterOnShutdownhook 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 --diffon the four Go files: no changes.vale,markdownlint-cli2andlychee --offline --include-fragmentsondocs/configuration.mdfrom thelint-markdownimage thedocslintjob uses: no findings.commitlintwith@commitlint/config-conventionalinnode: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.
- If codeowners are absent or the change is urgent, any registry maintainer can temporarily disable CODEOWNERS reviews in the project settings. When doing so:
- 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,FIXMEorOPTIMIZEprefixed comment (there are none). - Changes cannot be rolled back: not applicable, a revert restores the previous behaviour.
Documentation/resources
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).