fix(docker): honor DOCKER_TLS_VERIFY/DOCKER_CERT_PATH env vars (#607)#613
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
✅ 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.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #613 +/- ##
==========================================
+ Coverage 87.29% 87.34% +0.04%
==========================================
Files 88 88
Lines 10677 10719 +42
==========================================
+ Hits 9320 9362 +42
- Misses 1115 1116 +1
+ Partials 242 241 -1
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:
|
There was a problem hiding this comment.
Code Review
This pull request updates the Docker SDK adapter to properly honor DOCKER_TLS_VERIFY and DOCKER_CERT_PATH for HTTPS hosts, addressing an issue where custom HTTP clients would silently discard TLS material. It introduces TLSCertPath and TLSVerify to ClientConfig and implements logic to resolve host and TLS settings with precedence for configuration over environment variables. Feedback indicates a potential security risk where errors in TLS configuration resolution are ignored; it is recommended to refactor the client creation to fail-fast upon encountering such errors.
There was a problem hiding this comment.
Pull request overview
This pull request fixes a Docker SDK adapter bug where DOCKER_TLS_VERIFY / DOCKER_CERT_PATH TLS material was being lost when Ofelia installs its custom http.Client via client.WithHTTPClient, ensuring TLS configuration is applied on the custom transport as well.
Changes:
- Add
ClientConfig.TLSCertPathandClientConfig.TLSVerifyto allow explicit TLS configuration with config > env precedence. - Extend
createHTTPClientto resolve the effective host (includingDOCKER_HOST) and apply TLS config for HTTPS transports. - Add regression tests generating temporary TLS fixtures and update
CHANGELOG.mdwith the fix.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| core/adapters/docker/client.go | Adds host resolution for transport selection and TLS config resolution/application for HTTPS hosts. |
| core/adapters/docker/client_mutation_test.go | Adds regression tests ensuring TLS env/config precedence and TLS/no-TLS behavior across host types. |
| CHANGELOG.md | Documents the TLS env var fix and new config overrides under Unreleased. |
Comments suppressed due to low confidence (2)
core/adapters/docker/client.go:199
- TLS is only applied when the resolved host string starts with "https://". However repo docs show the common TLS setup as
DOCKER_HOST=tcp://...withDOCKER_TLS_VERIFY/DOCKER_CERT_PATH(e.g. docs/SECURITY.md). Withtcp://the code falls into the default branch and never calls resolveTLSConfig, so client cert/CA material will still be discarded in that configuration. Consider applying TLS whenever a cert path is configured (and host is not unix://), or explicitly handling thetcp://scheme used by Docker for TLS endpoints.
This issue also appears on line 189 of the same file.
// Configure dialer based on host type
switch {
case strings.HasPrefix(host, "unix://"):
socketPath := strings.TrimPrefix(host, "unix://")
transport.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) {
dialer := &net.Dialer{Timeout: config.DialTimeout}
return dialer.DialContext(ctx, "unix", socketPath)
}
// HTTP/2 not supported on Unix sockets
transport.ForceAttemptHTTP2 = false
case strings.HasPrefix(host, "https://"):
// HTTPS connections can use HTTP/2 via ALPN
transport.ForceAttemptHTTP2 = true
// Apply TLS from explicit config / env. The SDK's client.FromEnv
// also tries to apply this, but client.WithHTTPClient below
// overwrites the SDK's http.Client wholesale and would discard
// that TLS material — see issue #607.
if tlsCfg, err := resolveTLSConfig(config); err == nil && tlsCfg != nil {
transport.TLSClientConfig = tlsCfg
}
default:
// TCP without TLS - HTTP/2 not supported (no h2c in Docker)
transport.ForceAttemptHTTP2 = false
}
core/adapters/docker/client.go:195
- Errors from resolveTLSConfig are silently ignored (
err == nilguard). If TLS files are missing/invalid, Ofelia will proceed with an HTTPS transport but without the intended client cert/CA config, which can lead to confusing failures or an unintended unauthenticated TLS connection. It would be safer to propagate this error (e.g. have createHTTPClient/NewClientWithConfig return an error) so misconfiguration is surfaced early.
// Apply TLS from explicit config / env. The SDK's client.FromEnv
// also tries to apply this, but client.WithHTTPClient below
// overwrites the SDK's http.Client wholesale and would discard
// that TLS material — see issue #607.
if tlsCfg, err := resolveTLSConfig(config); err == nil && tlsCfg != nil {
transport.TLSClientConfig = tlsCfg
}
- Surface resolveTLSConfig errors via slog warning. Previously the call site silently swallowed the error, reproducing the very class of bug this PR fixes (security + test + DX reviewers all flagged this). - Extend TLS branch to also match tcp+tls:// so PR #609's new scheme is not a silent downgrade. Caught by the security review of PR #612. - Use SDK-exported env constants (client.EnvOverrideHost, EnvOverrideCertPath, EnvTLSVerify) instead of string literals; if the SDK ever renames them we get a compile error. - Append "(expected ca.pem, cert.pem, key.pem)" to the cert-load error so operators get the remediation hint inline. - Note in resolveHostForTransport that PR #606 introduces a duplicate helper — whichever PR merges second consolidates. Tests: - TestCreateHTTPClient_TLSVerifyExplicitFalse pins the explicit-opt-out precedence (config > env) for the verify polarity. - TestCreateHTTPClient_InsecureSkipVerifyDefaults pins the SDK-mirroring default: empty DOCKER_TLS_VERIFY -> InsecureSkipVerify=true. - TestCreateHTTPClient_TCPPlusTLSEnablesTLS asserts tcp+tls:// gets the same TLS material as https://. CHANGELOG: moved entry from ### Fixed to ### Security with explicit upgrade-impact callout (operators relying on silent insecure dial will break on upgrade — that is the point). docs/TROUBLESHOOTING.md: new section on TLS handshake / cert path errors covering the post-fix failure modes. 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.
Security review (NO-SHIP) identified that adding tcp+tls:// to the allow-list without wiring TLS material into the custom http.Transport is a silent downgrade to plain TCP - exactly the bug class this PR was meant to prevent. PR #613 (issue #607) adds the TLS plumbing. Until that lands, withhold tcp+tls:// from the allow-list and reject it loudly with the same error as ssh:// / fd://. Once #613 merges, follow-up to re-enable tcp+tls with a transport-level test that asserts TLS is actually wired up. Test review (conditional ship) noted the tcp+tls assertion was at string-transform level, not transport level. With tcp+tls now rejected, the assertion belongs on the rejection path - moved into the existing TestCreateHTTPClient_UnsupportedSchemes table. DX review: changelog reclassified to ### Changed with **BREAKING:** marker per project convention; ssh:// migration advice added inline so operators see the recommendation in the PR notes, not just in TROUBLESHOOTING. docs/TROUBLESHOOTING.md: drop tcp+tls from supported table; add to unsupported list with explanation pointing at #613. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
PR #613's codecov/patch check failed at 78.37% (target 80%) because the new `slog.Default().Warn(...)` branch on resolveTLSConfig errors was unexercised. Add two tests: - TestCreateHTTPClient_TLSLoadErrorDoesNotPanic exercises the surfacing behavior end-to-end: DOCKER_CERT_PATH points at an empty dir, so tlsconfig.Client errors out, the Warn branch fires, and the transport falls back to no TLS without panicking. - TestResolveTLSConfig_MissingCertFiles directly exercises the helper's error wrap, pinning the operator-facing remediation hint ("expected ca.pem, cert.pem, key.pem") into the error message. Coverage of createHTTPClient and resolveTLSConfig now 100%. 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.
…k on PR #613) Copilot review noted that the docker CLI / SDK has long supported DOCKER_HOST=tcp://... + DOCKER_TLS_VERIFY=1 + DOCKER_CERT_PATH=... and silently upgrades the connection to HTTPS. Mirroring that here closes a silent-plaintext-downgrade gap for users following Docker's canonical mTLS docs. Refactor: - Extract applyDockerTLS / hasTLSMaterial helpers so the https://, tcp+tls://, and tcp:// branches share the same TLS-application path. applyDockerTLS surfaces resolveTLSConfig errors via slog (same warn-don't-swallow pattern from the previous review-feedback commit). - tcp:// remains plain TCP / HTTP/1.1 when no TLS material is configured; it upgrades to HTTPS / HTTP/2 only when DOCKER_CERT_PATH (or ClientConfig.TLSCertPath) is present. Tests: - TestCreateHTTPClient_TCPWithTLSEnvUpgrades pins the upgrade. - TestCreateHTTPClient_TCPWithoutTLSEnvStaysPlaintext is the negative case: tcp:// without TLS env stays plaintext (otherwise the upgrade test alone could silently make every tcp:// HTTPS). - TestHasTLSMaterial_ExplicitConfigOnly covers the explicit-config branch of the new helper. 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.
Security review (NO-SHIP) identified that adding tcp+tls:// to the allow-list without wiring TLS material into the custom http.Transport is a silent downgrade to plain TCP - exactly the bug class this PR was meant to prevent. PR #613 (issue #607) adds the TLS plumbing. Until that lands, withhold tcp+tls:// from the allow-list and reject it loudly with the same error as ssh:// / fd://. Once #613 merges, follow-up to re-enable tcp+tls with a transport-level test that asserts TLS is actually wired up. Test review (conditional ship) noted the tcp+tls assertion was at string-transform level, not transport level. With tcp+tls now rejected, the assertion belongs on the rejection path - moved into the existing TestCreateHTTPClient_UnsupportedSchemes table. DX review: changelog reclassified to ### Changed with **BREAKING:** marker per project convention; ssh:// migration advice added inline so operators see the recommendation in the PR notes, not just in TROUBLESHOOTING. docs/TROUBLESHOOTING.md: drop tcp+tls from supported table; add to unsupported list with explanation pointing at #613. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
The Docker SDK adapter built its custom *http.Client via createHTTPClient and attached it with client.WithHTTPClient. That option replaces the SDK's c.client wholesale — and in doing so silently discards the *tls.Config that client.FromEnv had just installed via WithTLSClientConfigFromEnv, which is where DOCKER_CERT_PATH / DOCKER_TLS_VERIFY are consumed. Net effect: with DOCKER_HOST=https://... + DOCKER_TLS_VERIFY=1 + DOCKER_CERT_PATH=..., the resulting transport had a nil TLSClientConfig, so Ofelia either failed the handshake or fell back to system roots without the operator-supplied client cert. Latent since the SDK adapter landed in v0.12.0. Fix: in createHTTPClient, when the resolved host scheme is https://, build a *tls.Config via go-connections/tlsconfig.Client (already a transitive dep) using the same precedence as the SDK and assign it to transport.TLSClientConfig. Also resolve the host via DOCKER_HOST env so the HTTPS branch is selected in the same configurations the SDK targets. Add ClientConfig.TLSCertPath and ClientConfig.TLSVerify so callers can override env-supplied values explicitly. Precedence: config field > env var > unset (no TLS modification) DOCKER_TLS_VERIFY semantics match the SDK exactly: any non-empty value (including "0") means verify; only an unset/empty value disables it. Tests in client_mutation_test.go cover all four combinations (env-only / explicit-override / unix-socket-ignored / no-cert-path) using self-signed throwaway material under t.TempDir() and 127.0.0.1:0. This is the TLS sibling of #606 (DOCKER_HOST dialer divergence). Fixes #607 Signed-off-by: Sebastian Mendel <info@sebastianmendel.de> fix(docker): address PR #613 review feedback - Surface resolveTLSConfig errors via slog warning. Previously the call site silently swallowed the error, reproducing the very class of bug this PR fixes (security + test + DX reviewers all flagged this). - Extend TLS branch to also match tcp+tls:// so PR #609's new scheme is not a silent downgrade. Caught by the security review of PR #612. - Use SDK-exported env constants (client.EnvOverrideHost, EnvOverrideCertPath, EnvTLSVerify) instead of string literals; if the SDK ever renames them we get a compile error. - Append "(expected ca.pem, cert.pem, key.pem)" to the cert-load error so operators get the remediation hint inline. - Note in resolveHostForTransport that PR #606 introduces a duplicate helper — whichever PR merges second consolidates. Tests: - TestCreateHTTPClient_TLSVerifyExplicitFalse pins the explicit-opt-out precedence (config > env) for the verify polarity. - TestCreateHTTPClient_InsecureSkipVerifyDefaults pins the SDK-mirroring default: empty DOCKER_TLS_VERIFY -> InsecureSkipVerify=true. - TestCreateHTTPClient_TCPPlusTLSEnablesTLS asserts tcp+tls:// gets the same TLS material as https://. CHANGELOG: moved entry from ### Fixed to ### Security with explicit upgrade-impact callout (operators relying on silent insecure dial will break on upgrade — that is the point). docs/TROUBLESHOOTING.md: new section on TLS handshake / cert path errors covering the post-fix failure modes. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de> test(docker): cover the resolveTLSConfig error path PR #613's codecov/patch check failed at 78.37% (target 80%) because the new `slog.Default().Warn(...)` branch on resolveTLSConfig errors was unexercised. Add two tests: - TestCreateHTTPClient_TLSLoadErrorDoesNotPanic exercises the surfacing behavior end-to-end: DOCKER_CERT_PATH points at an empty dir, so tlsconfig.Client errors out, the Warn branch fires, and the transport falls back to no TLS without panicking. - TestResolveTLSConfig_MissingCertFiles directly exercises the helper's error wrap, pinning the operator-facing remediation hint ("expected ca.pem, cert.pem, key.pem") into the error message. Coverage of createHTTPClient and resolveTLSConfig now 100%. Signed-off-by: Sebastian Mendel <info@sebastianmendel.de> fix(docker): wire TLS for tcp:// + DOCKER_TLS_VERIFY (Copilot feedback on PR #613) Copilot review noted that the docker CLI / SDK has long supported DOCKER_HOST=tcp://... + DOCKER_TLS_VERIFY=1 + DOCKER_CERT_PATH=... and silently upgrades the connection to HTTPS. Mirroring that here closes a silent-plaintext-downgrade gap for users following Docker's canonical mTLS docs. Refactor: - Extract applyDockerTLS / hasTLSMaterial helpers so the https://, tcp+tls://, and tcp:// branches share the same TLS-application path. applyDockerTLS surfaces resolveTLSConfig errors via slog (same warn-don't-swallow pattern from the previous review-feedback commit). - tcp:// remains plain TCP / HTTP/1.1 when no TLS material is configured; it upgrades to HTTPS / HTTP/2 only when DOCKER_CERT_PATH (or ClientConfig.TLSCertPath) is present. Tests: - TestCreateHTTPClient_TCPWithTLSEnvUpgrades pins the upgrade. - TestCreateHTTPClient_TCPWithoutTLSEnvStaysPlaintext is the negative case: tcp:// without TLS env stays plaintext (otherwise the upgrade test alone could silently make every tcp:// HTTPS). - TestHasTLSMaterial_ExplicitConfigOnly covers the explicit-config branch of the new helper. 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.
The actual code fix for #605 cascaded into main via #613 (which added DOCKER_HOST env-driven host resolution to createHTTPClient as a side-effect of wiring TLS material). This commit adds the parts of PR #606 that did not duplicate code already on main: - TROUBLESHOOTING entry "DOCKER_HOST with TCP Socket Proxy (v0.12.0 - v0.24.0)" with the original symptom, root cause, and a working tecnativa/docker-socket-proxy compose snippet. - CHANGELOG attribution under [Unreleased] -> Fixed for #606/#605 alongside the existing #611/#612/#613/#618 entries, plus a Tests section noting the TestHealthStatus race fix that was also in #606. The TestHealthStatus race fix was cherry-picked separately as the preceding commit (it predates the recent batch and is unrelated to the docker adapter). Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
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>
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>
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>
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>
PR netresearch#612 dropped tcp+tls:// from supportedDockerHostSchemes because the TLS plumbing in PR netresearch#613 had not yet landed; without it, accepting tcp+tls:// would have silently downgraded to plain TCP. PR netresearch#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 netresearch#612 Changed entry. Fixes netresearch#616 Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
…anded (netresearch#616) (netresearch#625) ## Summary Re-enables the `tcp+tls://` `DOCKER_HOST` scheme that was dropped from the allow-list by [netresearch#612](netresearch#612) while waiting for the TLS-plumbing fix. The plumbing landed in [netresearch#613](netresearch#613) — `createHTTPClient` already routes `"https"` and `"tcp+tls"` through the same TLS-aware arm via `applyDockerTLS`, and `TestCreateHTTPClient_TCPPlusTLSEnablesTLS` (netresearch#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"` to `supportedDockerHostSchemes`; refresh the allow-list / switch-arm comments to reflect the new state. - `core/adapters/docker/client_mutation_test.go`: - Drop `tcp_plus_tls_scheme` from `TestNewClientWithConfig_UnsupportedSchemes`. - Add `tcp_plus_tls_lowercase` and `tcp_plus_tls_uppercase` (case-normalization) to the accept table in `TestValidateAndNormalizeHost`. - New `TestNewClientWithConfig_TCPPlusTLSScheme`: asserts that `NewClientWithConfig(Host: "tcp+tls://127.0.0.1:0")` does NOT return `ErrUnsupportedDockerHostScheme` (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 `### Added` entry under `[Unreleased]`; remove the now-stale "tcp+tls:// rejected pending netresearch#613" note from the existing netresearch#612 `### Changed` entry. Pure additive — no other code paths touched, `validateAndNormalizeHost` signature unchanged. Fixes [netresearch#616](netresearch#616). ## Test plan - [x] `go test -race ./core/adapters/docker/` — passes locally (5.6s) - [x] `golangci-lint run --timeout=3m` — 0 issues (full project) - [x] New positive test `TestNewClientWithConfig_TCPPlusTLSScheme` exercises the allow-list change - [x] Existing `TestCreateHTTPClient_TCPPlusTLSEnablesTLS` (from netresearch#613) still asserts the TLS material is wired - [ ] CI green
… seam (netresearch#617) (netresearch#629) ## Summary Collapses the dual `DOCKER_HOST` readers in `core/adapters/docker/client.go` into a single `resolveDockerHost(cfg)` seam — the SDK option chain and the custom HTTP transport now agree on the resolved host without ever re-reading env. This is a pure refactor of the antipattern that produced the bug class fixed by [netresearch#606](netresearch#606), [netresearch#607](netresearch#607), and [netresearch#609](netresearch#609). - One source of truth for host precedence (`config.Host` > `DOCKER_HOST` > `client.DefaultDockerHost`), scheme normalization, and allow-list validation. - The dispatch switch + `supportedDockerHostSchemes` slice collapse into one `schemeHandlers map[string]schemeHandler` — public allow-list and dispatch table derive from the same data. - Scheme spelling lives in named constants (`schemeUnix`, `schemeTCP`, ...) used everywhere. - `formatSupportedSchemes` cached at package init. - Drops `client.FromEnv` (it would re-read `DOCKER_HOST` inside the SDK, recreating the dual-reader bug); preserves `DOCKER_API_VERSION` via `client.WithVersionFromEnv()`. ## Behavior ZERO behavior change. Every existing test from the recent batch ([netresearch#606](netresearch#606) / [netresearch#608](netresearch#608) / [netresearch#609](netresearch#609) / [netresearch#613](netresearch#613) / [netresearch#618](netresearch#618)) passes unmodified, including `TestCreateHTTPClient_TCPPlusTLSEnablesTLS` (kept working by giving `tcp+tls` a dispatch entry with `allowed=false`). ## New tests - `TestNewClientWithConfig_ReadsDOCKERHOSTOnce` — the contract test mandated by the issue: asserts `DOCKER_HOST` is read at most once per `NewClientWithConfig` call across explicit-config / env-only / both-empty paths. Uses a package-level `getenv` seam + atomic counter. - `TestSchemeHandlers_AllowListParity` — asserts `schemeHandlers[allowed=true]` keys exactly equal the historical allow-list `{http, https, npipe, tcp, unix}`. Catches drift in BOTH directions. **The parity test caught no drift between the old list and the new map** — the refactor is clean. - `TestResolveDockerHost_ReadsEnvOnce` — exercises the helper directly. - `TestSupportedSchemesMsg_Cached` — pins the package-var cache idempotency. ## Dependency note This PR will likely conflict with [netresearch#616](netresearch#616) (re-enable `tcp+tls`). Once netresearch#616 lands, the conflict resolution is a one-line flip of `schemeHandlers[schemeTCPTLS].allowed = true` plus updating `historicalAllowedSchemes` in the parity test. Trust the maintainer to sequence the merges. ## Test plan - [x] `go test -race ./core/adapters/docker/` — all green - [x] `go test -race ./...` — all packages green - [x] `golangci-lint run --timeout=3m` — 0 issues - [x] Contract test exercises explicit-config, env-only, and both-empty paths - [x] Parity test asserts handler map is the only source of truth - [x] Signed commit + DCO sign-off Fixes netresearch#617
…etresearch#628) The godoc on `schemeHandlers` and the TROUBLESHOOTING.md scheme table both claimed `tcp://` "auto-upgrades to TLS when DOCKER_TLS_VERIFY / DOCKER_CERT_PATH are set", mirroring the docker CLI. PR netresearch#613 wired `applyDockerTLS` into `applyTCPTransport` to make that promise good at the transport layer — but Go's `http.Transport` only performs TLS on `https://` URLs, so the `TLSClientConfig` was loaded with cert material the SDK never offered on the wire (see netresearch#634 for the SDK URL-rewrite half of this story). Operators following the docker CLI mental model believed they had mTLS while their connections went out as plain TCP. Silent ineffective wiring is worse than failing loud, so this commit: - Strips the `applyDockerTLS` call (and the now-unused `hasTLSMaterial` helper) from `applyTCPTransport`. Plain TCP is now plain HTTP/1.1 with no TLS attempt — matching what the SDK actually does. - Updates the godoc on `schemeHandlers` and the scheme table in `docs/TROUBLESHOOTING.md` to drop the auto-upgrade claim and point operators wanting TLS over TCP at `tcp+tls://` (netresearch#616) or `https://`. - Replaces `TestCreateHTTPClient_TCPWithTLSEnvUpgrades` (which pinned the misleading wiring) with `TestCreateHTTPClient_TCPDoesNotWireTLS` `EvenWithEnv`, pinning the contract going forward. The deeper docker-CLI parity story — automatic `tcp://` to `https://` URL rewriting at the SDK option layer — is tracked in netresearch#634 and is intentionally out of scope here. Refs: netresearch#628, netresearch#634, netresearch#616, netresearch#613 Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
…etresearch#644) ## Summary Reconciles the `tcp://` Docker host scheme docs with reality. Closes [netresearch#628](netresearch#628). The godoc on `schemeHandlers` (in [`core/adapters/docker/client.go`](https://github.com/netresearch/ofelia/blob/main/core/adapters/docker/client.go)) and the scheme table in [`docs/TROUBLESHOOTING.md`](https://github.com/netresearch/ofelia/blob/main/docs/TROUBLESHOOTING.md) both claimed `tcp://` "auto-upgrades to TLS when `DOCKER_TLS_VERIFY` / `DOCKER_CERT_PATH` are set", mirroring the docker CLI. PR [netresearch#613](netresearch#613) made that promise good at the transport layer by wiring `applyDockerTLS` into `applyTCPTransport`. But Go's `http.Transport` only performs TLS on `https://` URLs — so the `TLSClientConfig` was loaded with cert material that the SDK never offered on the wire (the deeper SDK URL-rewrite half is tracked in [netresearch#634](netresearch#634)). Operators following the docker CLI mental model believed they had mTLS while their connections went out as plain TCP. Silent ineffective wiring is worse than failing loud, so this PR strips the dead wiring and points operators wanting TLS over TCP at the explicit `tcp+tls://` ([netresearch#616](netresearch#616)) or `https://` schemes. ## Changes - **Code:** `applyTCPTransport` (and the now-unused `hasTLSMaterial` helper) no longer wires `TLSClientConfig` from `DOCKER_CERT_PATH` / `DOCKER_TLS_VERIFY`. Plain TCP is now plain HTTP/1.1, matching what the SDK actually does on the wire. - **Docs:** godoc on `schemeHandlers` for the `tcp:` row and the `tcp://` row of the scheme table in `docs/TROUBLESHOOTING.md` now state that `tcp://` is plain TCP with no TLS, and explicitly point at `tcp+tls://` / `https://` for TLS over TCP. - **Tests:** replaced `TestCreateHTTPClient_TCPWithTLSEnvUpgrades` (which pinned the misleading wiring) with `TestCreateHTTPClient_TCPDoesNotWireTLSEvenWithEnv`, pinning the new contract. `TestHasTLSMaterial_ExplicitConfigOnly` removed alongside the helper. The negative case `TestCreateHTTPClient_TCPWithoutTLSEnvStaysPlaintext` and the positive `TestCreateHTTPClient_TCPPlusTLSEnablesTLS` continue to hold. - **CHANGELOG:** entry under `### Documentation` for [Unreleased] with the full reconciliation story. ## Scope Intentionally narrow to **netresearch#628**. The deeper docker-CLI parity story — automatic `tcp://` to `https://` URL rewriting at the SDK option layer so the SDK actually performs the TLS handshake — is tracked separately in [netresearch#634](netresearch#634) and is the right place to revisit "operators want docker CLI parity for TLS env vars" if we decide that's worth doing. ## Test plan - [x] `go test ./core/adapters/docker/ -count=1 -short` — clean (4.582s) - [x] `go test ./... -count=1 -short` — all packages pass - [x] `golangci-lint run ./...` — 0 issues - [x] `go build ./...` — clean - [ ] CI green on PR ## Related - Closes [netresearch#628](netresearch#628) - Companion (out of scope here): [netresearch#634](netresearch#634) - Originating PR for the dead wiring: [netresearch#613](netresearch#613) - Explicit TLS-over-TCP scheme: [netresearch#616](netresearch#616)
…arch#646) ## Symptom `tcp+tls://` is an *explicit* TLS opt-in scheme in the Docker SDK adapter. 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), the chain `resolveTLSConfig` -> `(nil, nil)` -> `applyDockerTLS` no-op 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 — the analog, for `tcp+tls://`, of the silent plain-TCP downgrade closed by [netresearch#612](netresearch#612) / [netresearch#625](netresearch#625). Surfaced during the security review of [netresearch#625](netresearch#625); tracked in [netresearch#627](netresearch#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 dedicated section. - `tcp://` remains fail-open (ambiguous scheme; operators may rely on stdlib defaults). - `https://` remains fail-open with a warning, matching the upstream SDK's documented posture. ## New error ``` Error: creating docker client: tcp+tls:// requires TLS material: set DOCKER_CERT_PATH/DOCKER_TLS_VERIFY or ClientConfig.TLSCertPath/TLSVerify; see docs/TROUBLESHOOTING.md ``` Branchable via `errors.Is(err, docker.ErrTCPTLSRequiresCertMaterial)`. ## Tests (TDD) New file `core/adapters/docker/client_tcptls_test.go`: - `TestNewClientWithConfig_TCPPlusTLSRequiresCertMaterial` — drives the gate (RED first, then GREEN). - `TestNewClientWithConfig_TCPPlusTLSAcceptsCertMaterial` — positive: `ClientConfig.TLSCertPath` set -> no error from the gate. - `TestNewClientWithConfig_TCPPlusTLSAcceptsEnvCertPath` — positive: `DOCKER_CERT_PATH` set -> no error from the gate. Existing `TestCreateHTTPClient_TCPPlusTLSEnablesTLS` and `TestNewClientWithConfig_TCPPlusTLSScheme` remain green. Full `go test ./... -count=1 -short` and `golangci-lint run ./...` are clean. ## Refs - Fixes [netresearch#627](netresearch#627) - Triggered by review of [netresearch#625](netresearch#625) (re-enabled `tcp+tls://`) - TLS plumbing context: [netresearch#613](netresearch#613)
…ed (netresearch#647) ## Summary Closes netresearch#634, follow-up to netresearch#613. `DOCKER_HOST=tcp://...` combined with `DOCKER_TLS_VERIFY` / `DOCKER_CERT_PATH` (the canonical docker CLI mTLS setup) was silently going out plaintext. PR netresearch#613 wired the cert material into the custom `http.Transport`, but the SDK kept the `tcp://` URL — and Go's `http.Transport` only triggers TLS for `https://` URLs. So the cert material was loaded into a `TLSClientConfig` the 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://` AND `hasTLSMaterial(cfg)` is true, rewrite to `https://` before passing to `client.WithHost`. The SDK and transport now agree on the scheme; `applyTLSTransport` runs; TLS actually negotiates on the wire. ### Changes - New `upgradeTCPToHTTPSIfTLSMaterial(host, cfg)` helper, called from `NewClientWithConfig` once after `resolveDockerHost`. Pure / deterministic / unit-tested in isolation. - Godoc on the `tcp:` `schemeHandlers` entry refreshed to describe the new auto-upgrade contract. - `docs/TROUBLESHOOTING.md` scheme table updated. - `applyTCPTransport`'s `hasTLSMaterial` branch retained for direct callers of `createHTTPClient` (notably `TestCreateHTTPClient_TCPWithTLSEnvUpgrades`); production now dispatches through `applyTLSTransport`. ### Coordination with sibling PRs - netresearch#627 (fail-closed `tcp+tls://` without cert material) — orthogonal; no overlap. - netresearch#628 (docs claim that `tcp://` stays plain) — at PR-author time netresearch#628 had not merged, so this PR keeps the existing "auto-upgrades" doc language. If netresearch#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 - [x] New `TestUpgradeTCPToHTTPSIfTLSMaterial` (table-driven, 7 cases): `tcp://` upgrades on env or explicit `ClientConfig.TLSCertPath`; stays plain without TLS; `tcp+tls://`, `https://`, `unix://`, `http://` all pass through unchanged. - [x] New `TestNewClientWithConfig_TCPSchemeUpgradesToHTTPSWithTLSEnv` integration test verifies the actual SDK URL via `cli.DaemonHost()` after construction. - [x] New `TestNewClientWithConfig_TCPSchemeStaysPlainWithoutTLSEnv` negative case. - [x] Existing tests unchanged: `TestCreateHTTPClient_TCPWithTLSEnvUpgrades`, `TestCreateHTTPClient_TCPWithoutTLSEnvStaysPlaintext`, `TestNewClientWithConfig_DoesNotMutateConfigHost`, `TestSchemeHandlers_AllowListParity`, `TestHasTLSMaterial_*` all still pass. - [x] `go test ./core/adapters/docker/ -count=1 -short -race` clean. - [x] `golangci-lint run ./...` clean.
## Summary Cut the existing `[Unreleased]` CHANGELOG block at [fad5239](netresearch@fad5239) into a versioned `[0.25.0] - 2026-05-14` heading, and fill in two entries for PRs that landed after the previous `[Unreleased]` writeups: - **### Security** — Go toolchain `1.26.2` → `1.26.3` ([netresearch#662](netresearch#662)). Clears six stdlib advisories reachable from this codebase (`net/mail`, `html/template`, `net`, `net/http`); refreshes direct deps (`docker/cli`, `golang.org/x/{crypto,term,text}`) and the full indirect graph. Post-bump `govulncheck` is down to the two unfixable upstream moby advisories on `docker/docker` v28.5.2. - **### Fixed** — `MaxRuntime` cancellation now stops *and removes* the container/service ([netresearch#659](netresearch#659), fixes [netresearch#655](netresearch#655)). Completes [netresearch#651](netresearch#651 deadline wiring with a fresh `context.WithTimeout(context.Background(), jobCleanupTimeout)` cleanup context so stop/remove still runs after the parent deadline fires. Mirrored into `RunServiceJob`. ## Headline changes since v0.24.0 - **Security**: Go 1.26.3 toolchain, three silent-downgrade vectors closed (`https://` mTLS, SMTP STARTTLS default, webhook allow-list typo), fail-closed on `tcp+tls://` without cert material ([netresearch#660](netresearch#660), [netresearch#646](netresearch#646), [netresearch#662](netresearch#662)) - **New**: `tcp+tls://` `DOCKER_HOST` scheme re-enabled ([netresearch#625](netresearch#625)); `DOCKER_TLS_VERIFY` / `DOCKER_CERT_PATH` honored ([netresearch#613](netresearch#613)) - **Correctness**: bounded contexts in scheduler / health / Docker pings ([netresearch#636](netresearch#636), [netresearch#651](netresearch#651)); orphan-container cleanup on MaxRuntime ([netresearch#659](netresearch#659)); pervasive nil-guard pass across the Docker adapter ([netresearch#626](netresearch#626), [netresearch#639](netresearch#639), [netresearch#648](netresearch#648), [netresearch#658](netresearch#658)) - **Refactor / DX**: unified Docker host / scheme resolution ([netresearch#629](netresearch#629)); webhook global config dual-store collapsed ([netresearch#637](netresearch#637)); `[global]` label handling unified across all subsystems ([netresearch#661](netresearch#661)) ## Version bump rationale Pre-1.0 semver — minor bump because the range includes one `feat:` ([netresearch#625](netresearch#625) — `tcp+tls://` scheme re-enabled), several `fix(security):` PRs that surface previously-silent downgrades, and the `[global]` label-handling rework. The webhook key rename in [netresearch#620](netresearch#620) / [netresearch#637](netresearch#637) is shipped under `### Deprecated` (legacy `ofelia.webhooks` form keeps working with a one-shot warning), not as a breaking change. ## Notes - This PR touches **only `CHANGELOG.md`** — matches the v0.24.0 prep pattern ([netresearch#602](netresearch#602)). The Release workflow injects the version into `cli.Version` via ldflags from the tag, so no `cli/version.go` edit is needed. - After merge: signed annotated tag `v0.25.0` will be pushed to the merge commit, triggering [`release-go-app.yml`](https://github.com/netresearch/.github/blob/main/.github/workflows/release-go-app.yml) for binaries, container image, cosign `--bundle` signatures, and SLSA attestations. - Contributor thanks will be added directly to the GitHub release description (not the CHANGELOG, per project convention from v0.24.0). ## Test plan - [x] `go build ./...` clean - [ ] CI green on this PR - [ ] CHANGELOG renders correctly on GitHub Files Changed tab - [ ] After merge: signed annotated tag `v0.25.0` created on the merge commit (`git tag -s v0.25.0 -m "v0.25.0"`) and pushed - [ ] Release workflow run succeeds end-to-end



Summary
Fixes #607. TLS sibling of #606 (which fixes the dialer-divergence half of the same root cause).
When
DOCKER_HOST=https://...is used together withDOCKER_TLS_VERIFY=1andDOCKER_CERT_PATH=..., Ofelia silently discarded the TLS material — the resulting*http.Transporthad a nilTLSClientConfig, so the daemon either rejected the handshake or got an unauthenticated connection where mTLS was intended. Latent since the SDK adapter landed in v0.12.0.Root cause
NewClientWithConfigconfigures the SDK in two passes that don't compose:client.FromEnvrunsWithTLSClientConfigFromEnv(), which builds a*tls.ConfigfromDOCKER_CERT_PATH/DOCKER_TLS_VERIFYand assigns it onto a freshhttp.Client.Transport— i.e. it replacesc.client.client.WithHTTPClient(httpClient)then overwritesc.clientagain with our custom transport, which never sees the TLS config from step 1.So whichever step runs last wins, and step 2 always wins. The exact same shape as #606's
WithHostFromEnvvs custom dialer divergence.Fix
In
createHTTPClient, when the resolved host scheme ishttps://, build a*tls.Configviago-connections/tlsconfig.Client(already a dependency) and assign it totransport.TLSClientConfig. Two new fields onClientConfigallow explicit override:TLSCertPath stringandTLSVerify *bool. Precedence mirrorsresolveDockerHostfrom #606: config field > env var > unset.DOCKER_TLS_VERIFYsemantics match the SDK exactly: any non-empty value (including"0") means verify; only an unset/empty value disables it. When neither config nor env supply a cert path, no TLS is applied (matching the SDK's "leave TLS untouched" behavior).A small helper,
resolveHostForTransport, also consultsDOCKER_HOSTso the HTTPS branch is selected in the same configurations the SDK targets — avoiding a related divergence where emptyconfig.Hostwould fall back to the unix default and skip the TLS branch entirely.Test plan
core/adapters/docker/client_mutation_test.go:TestCreateHTTPClient_HonorsTLSEnv—DOCKER_HOST=https://127.0.0.1:0+ cert path, assertsTLSClientConfig.{RootCAs, Certificates}populated andInsecureSkipVerify=falseTestCreateHTTPClient_ExplicitTLSOverridesEnv—cfg.TLSCertPath+cfg.TLSVerifywin over env-supplied valuesTestCreateHTTPClient_NoTLSForUnixSocket— unix socket path → no TLS even with env setTestCreateHTTPClient_NoTLSWhenCertPathEmpty— empty cert path → SDK-equivalent "no TLS modification"BUG #607 confirmed: transport.TLSClientConfig is nil; TLS env not honoredbefore the fixgo test -race ./core/adapters/docker/ ./core/passesgo test -race ./...passes (full module)golangci-lint run --timeout=3m ./...cleant.TempDir(); loopback-only127.0.0.1:0(no parallel-test contamination, per fix(docker): honor DOCKER_HOST env var when selecting transport dialer #606 review)Scope
Self-contained — no
go.mod/go.sumchange (go-connections/tlsconfigwas already a transitive dep), no impact on the unix-socket / TCP code paths.