fix(docker): upgrade tcp:// to https:// when TLS material is configured#647
Conversation
github-actions
Bot
added
documentation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
✅ Mutation Testing ResultsMutation Score: 100.00% (threshold: 60%)
What is mutation testing?Mutation testing measures test quality by introducing small changes (mutations) to the code and checking if tests detect them. A higher score means better test effectiveness.
|
![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.
There was a problem hiding this comment.
Pull request overview
This PR fixes a security-impacting Docker SDK integration gap where DOCKER_HOST=tcp://... combined with Docker’s mTLS environment variables could silently result in plaintext connections. It aligns Ofelia’s resolved host scheme with the configured TLS transport by rewriting tcp:// to https:// when TLS cert material is present, mirroring long-standing docker CLI behavior.
Changes:
- Add a
tcp://→https://normalization step inNewClientWithConfigwhen TLS cert material is configured. - Introduce unit + integration tests covering the upgrade behavior and non-upgrade negative cases.
- Update troubleshooting docs and changelog to describe the new behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
core/adapters/docker/client.go |
Adds host-scheme rewrite helper and wires it into client construction; updates scheme documentation. |
core/adapters/docker/client_tcp_upgrade_test.go |
Adds table-driven unit tests and constructor-level integration tests for tcp→https upgrade behavior. |
docs/TROUBLESHOOTING.md |
Updates the supported-schemes table to reflect tcp→https upgrade behavior. |
CHANGELOG.md |
Documents the fix and its operator-visible impact. |
Comments suppressed due to low confidence (1)
core/adapters/docker/client.go:536
- The function doc says the tcp:// → https:// rewrite triggers when
DOCKER_TLS_VERIFY / DOCKER_CERT_PATH(orClientConfig.TLSCertPath / TLSVerify) are configured. The implementation only checks cert-path presence viahasTLSMaterial(configTLSCertPathorDOCKER_CERT_PATH), soDOCKER_TLS_VERIFY/TLSVerifyalone won’t cause an upgrade. Consider tightening the doc to avoid implying thatTLSVerifyparticipates in the decision, and clarify thatDOCKER_TLS_VERIFYonly affects server verification once TLS is enabled.
// upgradeTCPToHTTPSIfTLSMaterial mirrors the docker CLI's silent scheme
// upgrade: when the resolved host uses the plain tcp:// scheme AND TLS
// material is configured (DOCKER_TLS_VERIFY / DOCKER_CERT_PATH env or the
// equivalent ClientConfig.TLSCertPath / TLSVerify overrides), the scheme is
// rewritten to https:// before being handed to client.WithHost. Without this
![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 fixes an issue where using DOCKER_HOST=tcp://... alongside TLS environment variables failed to negotiate TLS end-to-end. The implementation now mirrors the Docker CLI by automatically upgrading the tcp:// scheme to https:// when TLS material is present, ensuring the Go HTTP transport correctly activates TLS. The changes include the new upgrade logic in NewClientWithConfig, updated documentation in the CHANGELOG and troubleshooting guide, and comprehensive tests to verify the scheme rewriting. I have no feedback to provide as there were no review comments to evaluate.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #647 +/- ##
==========================================
- Coverage 87.87% 87.86% -0.01%
==========================================
Files 88 88
Lines 10954 10962 +8
==========================================
+ Hits 9626 9632 +6
- Misses 1084 1086 +2
Partials 244 244
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:
|
PR review (`Code review`, `Security review`) flagged that deleting `hasTLSMaterial` creates a build break for two in-flight sibling PRs: - #646 (fix/issue-627): fail-closed gate on `tcp+tls://` without cert material — calls `hasTLSMaterial(config)` from `NewClientWithConfig`. - #647 (fix/issue-634): rewrite `tcp://` → `https://` when TLS material is set — calls `hasTLSMaterial(cfg)` from `upgradeTCPToHTTPSIfTLSMaterial`. Restore the helper with a `//nolint:unused` annotation explaining why it is HEAD-unused. The dead `applyDockerTLS` call is still removed from `applyTCPTransport` (the actual reconciliation #628 was filed for); the helper itself is retained so the sibling PRs rebase cleanly. Refs #628. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
github-merge-queue
Bot
removed this pull request from the merge queue due to a conflict with the base branch
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
CybotTM
force-pushed
the
fix/issue-634
branch
2 times, most recently
from
629efd4 to
44ff5d6
Compare
Mirror the docker CLI's silent scheme upgrade: when DOCKER_HOST=tcp://... is combined with DOCKER_TLS_VERIFY / DOCKER_CERT_PATH (or the equivalent ClientConfig.TLSCertPath / TLSVerify overrides), rewrite the host scheme to https:// before passing it to client.WithHost. Without the rewrite, the SDK was given a tcp:// URL while the custom HTTP transport was wired with TLS material via #613. Go's http.Transport only triggers TLS for https:// URLs, so the cert material was silently unused and connections went out plaintext, leaving operators believing they had mTLS when they did not. The rewrite is implemented as a tested helper (upgradeTCPToHTTPSIfTLSMaterial) called once from NewClientWithConfig right after host resolution. Other schemes (tcp+tls://, https://, unix://, http://, npipe://) pass through unchanged. The applyTCPTransport hasTLSMaterial branch stays in place to support direct callers of createHTTPClient (notably TestCreateHTTPClient_TCPWithTLSEnvUpgrades); in production this rewrite means dispatch goes through applyTLSTransport. Updates the godoc on the tcp: schemeHandlers entry and the TROUBLESHOOTING.md scheme table to make the new contract explicit. Closes #634, follow-up to #613. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
|
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
Summary
Closes #634, follow-up to #613.
DOCKER_HOST=tcp://...combined withDOCKER_TLS_VERIFY/DOCKER_CERT_PATH(the canonical docker CLI mTLS setup) was silently going out plaintext. PR #613 wired the cert material into the customhttp.Transport, but the SDK kept thetcp://URL — and Go'shttp.Transportonly triggers TLS forhttps://URLs. So the cert material was loaded into aTLSClientConfigthe SDK never used. Operators believing they had mTLS were getting unauthenticated connections.This PR mirrors the docker CLI's long-standing silent upgrade: when the resolved host is
tcp://ANDhasTLSMaterial(cfg)is true, rewrite tohttps://before passing toclient.WithHost. The SDK and transport now agree on the scheme;applyTLSTransportruns; TLS actually negotiates on the wire.Changes
upgradeTCPToHTTPSIfTLSMaterial(host, cfg)helper, called fromNewClientWithConfigonce afterresolveDockerHost. Pure / deterministic / unit-tested in isolation.tcp:schemeHandlersentry refreshed to describe the new auto-upgrade contract.docs/TROUBLESHOOTING.mdscheme table updated.applyTCPTransport'shasTLSMaterialbranch retained for direct callers ofcreateHTTPClient(notablyTestCreateHTTPClient_TCPWithTLSEnvUpgrades); production now dispatches throughapplyTLSTransport.Coordination with sibling PRs
tcp+tls://without cert material silently uses system CA / no client cert (should fail-closed) #627 (fail-closedtcp+tls://without cert material) — orthogonal; no overlap.tcp://doc claims TLS auto-upgrade but switch arm doesn't callapplyDockerTLS#628 (docs claim thattcp://stays plain) — at PR-author time Docker SDK:tcp://doc claims TLS auto-upgrade but switch arm doesn't callapplyDockerTLS#628 had not merged, so this PR keeps the existing "auto-upgrades" doc language. If Docker SDK:tcp://doc claims TLS auto-upgrade but switch arm doesn't callapplyDockerTLS#628 lands first, the troubleshooting line in this PR will need to be reverted into the auto-upgrade form again — easy 1-line conflict.Test plan
TestUpgradeTCPToHTTPSIfTLSMaterial(table-driven, 7 cases):tcp://upgrades on env or explicitClientConfig.TLSCertPath; stays plain without TLS;tcp+tls://,https://,unix://,http://all pass through unchanged.TestNewClientWithConfig_TCPSchemeUpgradesToHTTPSWithTLSEnvintegration test verifies the actual SDK URL viacli.DaemonHost()after construction.TestNewClientWithConfig_TCPSchemeStaysPlainWithoutTLSEnvnegative case.TestCreateHTTPClient_TCPWithTLSEnvUpgrades,TestCreateHTTPClient_TCPWithoutTLSEnvStaysPlaintext,TestNewClientWithConfig_DoesNotMutateConfigHost,TestSchemeHandlers_AllowListParity,TestHasTLSMaterial_*all still pass.go test ./core/adapters/docker/ -count=1 -short -raceclean.golangci-lint run ./...clean.