feat(docker): re-enable tcp+tls:// scheme now that TLS plumbing has landed (#616)#625
Conversation
PR #612 dropped tcp+tls:// from supportedDockerHostSchemes because the TLS plumbing in PR #613 had not yet landed; without it, accepting tcp+tls:// would have silently downgraded to plain TCP. PR #613 has since merged. The createHTTPClient switch already routes "https" and "tcp+tls" through the same TLS-aware arm via applyDockerTLS, and the regression test TestCreateHTTPClient_TCPPlusTLSEnablesTLS pins that contract. The allow-list can therefore safely accept tcp+tls:// again. - Re-add "tcp+tls" to supportedDockerHostSchemes (incl. case-insensitive normalization handled by validateAndNormalizeHost). - Move tcp_plus_tls_scheme out of TestNewClientWithConfig_UnsupportedSchemes and add a positive entry (lowercase + uppercase) in TestValidateAndNormalizeHost. - Add TestNewClientWithConfig_TCPPlusTLSScheme asserting that NewClientWithConfig(Host: "tcp+tls://127.0.0.1:0") does not return ErrUnsupportedDockerHostScheme. - Update docs/TROUBLESHOOTING.md: tcp+tls:// row added to the supported table; pending-PR note removed from the unsupported list. - CHANGELOG: new Added entry; remove stale "tcp+tls:// rejected" note from the existing #612 Changed entry. Fixes #616 Signed-off-by: Sebastian Mendel <[email protected]>
github-actions
Bot
added
documentation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #625 +/- ##
=======================================
Coverage 87.38% 87.38%
=======================================
Files 88 88
Lines 10722 10722
=======================================
Hits 9369 9369
Misses 1112 1112
Partials 241 241
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- CHANGELOG: correct the PR ref (was a copy-paste of #620, should be #625) — flagged by code review. - TROUBLESHOOTING: refresh the example error-message scheme list to include tcp+tls://, and update the "fell through" sentence so it no longer reads as if tcp+tls is still unsupported (DX review). Two follow-up issues filed separately for the deeper findings (security "tcp+tls without certs falls back to system-CA TLS — should fail-closed" and code review "tcp:// docs claim auto-upgrade but the switch arm doesn't actually call applyDockerTLS"). Out of scope for this PR's allow-list re-enable. Signed-off-by: Sebastian Mendel <[email protected]>
|
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
There was a problem hiding this comment.
Pull request overview
Re-enables the tcp+tls:// DOCKER_HOST scheme in Ofelia’s Docker adapter now that TLS material is correctly applied to the custom HTTP transport (per the prior TLS plumbing work).
Changes:
- Re-add
tcp+tlsto theDOCKER_HOSTscheme allow-list and update related inline comments. - Update unit tests to treat
tcp+tls://as supported (including scheme case-normalization coverage) and add a targeted allow-list acceptance test. - Refresh operator-facing documentation and changelog entries to reflect
tcp+tls://support.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
core/adapters/docker/client.go |
Re-adds tcp+tls to supported schemes and updates scheme/transport commentary. |
core/adapters/docker/client_mutation_test.go |
Adjusts scheme validation tests and adds a new tcp+tls:// acceptance test. |
docs/TROUBLESHOOTING.md |
Reintroduces tcp+tls:// in the supported schemes table and removes the “pending” note. |
CHANGELOG.md |
Documents the re-enabled scheme under Unreleased and removes the stale “pending TLS plumbing” note. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request re-enables the tcp+tls:// scheme for DOCKER_HOST validation, following the implementation of necessary TLS plumbing. It updates the allow-list, documentation, and test suites to reflect this change. One piece of feedback was provided regarding the documentation for the tcp scheme: while the code enables HTTP/2 during auto-upgrades to TLS, the comments still stated that HTTP/2 was not supported for that scheme.
tcp+tls:// is an *explicit* TLS opt-in scheme. Until now, if an operator set DOCKER_HOST=tcp+tls://daemon:2376 but forgot DOCKER_CERT_PATH / DOCKER_TLS_VERIFY (or the equivalent ClientConfig.TLSCertPath / TLSVerify overrides), resolveTLSConfig returned (nil, nil), applyDockerTLS left transport.TLSClientConfig nil, and the SDK silently dialed TLS using Go's stdlib defaults -- system CA bundle, NO client certificate. Operators who declared mTLS were getting unauthenticated TLS handshakes against any daemon that did not strictly require client auth. This is the analog, for tcp+tls://, of the silent plain-TCP downgrade closed by #612 / #625. Surfaced during the security review of #625; tracked in #627. Fix: in NewClientWithConfig, after resolveDockerHost succeeds, if scheme == tcp+tls and hasTLSMaterial(config) == false, wrap and return the new typed sentinel ErrTCPTLSRequiresCertMaterial. The error message points operators at docs/TROUBLESHOOTING.md, which gains a new section explaining the fix. tcp:// remains fail-open (it is ambiguous -- operators may rely on stdlib defaults). https:// remains fail-open with a warning, matching the upstream SDK's documented posture. TDD: TestNewClientWithConfig_TCPPlusTLSRequiresCertMaterial drives the gate; two positive counterparts cover both ClientConfig.TLSCertPath and DOCKER_CERT_PATH branches of hasTLSMaterial. Existing TestCreateHTTPClient_TCPPlusTLSEnablesTLS and TestNewClientWithConfig_TCPPlusTLSScheme remain green. Refs: #627, #625 Signed-off-by: Sebastian Mendel <[email protected]>
Summary
Re-enables the
tcp+tls://DOCKER_HOSTscheme that was dropped from the allow-list by #612 while waiting for the TLS-plumbing fix. The plumbing landed in #613 —createHTTPClientalready routes"https"and"tcp+tls"through the same TLS-aware arm viaapplyDockerTLS, andTestCreateHTTPClient_TCPPlusTLSEnablesTLS(#613) pins that contract. The silent-downgrade risk that justified the temporary rejection is therefore closed.Changes
core/adapters/docker/client.go: re-add"tcp+tls"tosupportedDockerHostSchemes; refresh the allow-list / switch-arm comments to reflect the new state.core/adapters/docker/client_mutation_test.go:tcp_plus_tls_schemefromTestNewClientWithConfig_UnsupportedSchemes.tcp_plus_tls_lowercaseandtcp_plus_tls_uppercase(case-normalization) to the accept table inTestValidateAndNormalizeHost.TestNewClientWithConfig_TCPPlusTLSScheme: asserts thatNewClientWithConfig(Host: "tcp+tls://127.0.0.1:0")does NOT returnErrUnsupportedDockerHostScheme(any unrelated dial / negotiate error is acceptable — the test only pins scheme acceptance).docs/TROUBLESHOOTING.md:tcp+tls://row added back to the supported schemes table; pending-PR note removed from the unsupported list.CHANGELOG.md: new### Addedentry under[Unreleased]; remove the now-stale "tcp+tls:// rejected pending fix(docker): honor DOCKER_TLS_VERIFY/DOCKER_CERT_PATH env vars (#607) #613" note from the existing fix(docker): validate DOCKER_HOST scheme and reject unsupported transports (#609) #612### Changedentry.Pure additive — no other code paths touched,
validateAndNormalizeHostsignature unchanged.Fixes #616.
Test plan
go test -race ./core/adapters/docker/— passes locally (5.6s)golangci-lint run --timeout=3m— 0 issues (full project)TestNewClientWithConfig_TCPPlusTLSSchemeexercises the allow-list changeTestCreateHTTPClient_TCPPlusTLSEnablesTLS(from fix(docker): honor DOCKER_TLS_VERIFY/DOCKER_CERT_PATH env vars (#607) #613) still asserts the TLS material is wired