Skip to content

feat(docker): re-enable tcp+tls:// scheme now that TLS plumbing has landed (#616)#625

Merged
CybotTM merged 2 commits into
mainfrom
fix/issue-616
May 13, 2026
Merged

feat(docker): re-enable tcp+tls:// scheme now that TLS plumbing has landed (#616)#625
CybotTM merged 2 commits into
mainfrom
fix/issue-616

Conversation

@CybotTM
Copy link
Copy Markdown
Member

@CybotTM CybotTM commented May 13, 2026

Summary

Re-enables the tcp+tls:// DOCKER_HOST scheme that was dropped from the allow-list by #612 while waiting for the TLS-plumbing fix. The plumbing landed in #613createHTTPClient already routes "https" and "tcp+tls" through the same TLS-aware arm via applyDockerTLS, and TestCreateHTTPClient_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" 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 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 ### Changed entry.

Pure additive — no other code paths touched, validateAndNormalizeHost signature unchanged.

Fixes #616.

Test plan

  • go test -race ./core/adapters/docker/ — passes locally (5.6s)
  • golangci-lint run --timeout=3m — 0 issues (full project)
  • New positive test TestNewClientWithConfig_TCPPlusTLSScheme exercises the allow-list change
  • Existing TestCreateHTTPClient_TCPPlusTLSEnablesTLS (from fix(docker): honor DOCKER_TLS_VERIFY/DOCKER_CERT_PATH env vars (#607) #613) still asserts the TLS material is wired
  • CI green

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 <info@sebastianmendel.de>
Copilot AI review requested due to automatic review settings May 13, 2026 16:03
@github-actions github-actions Bot added documentation Improvements or additions to documentation tests labels May 13, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 13, 2026

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

github-actions[bot]
github-actions Bot previously approved these changes May 13, 2026
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated approval for maintainer PR

All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.

@github-actions
Copy link
Copy Markdown

⚠️ Mutation Testing Results

Mutation Score: 0.00% (threshold: 60%)

⚠️ Score is below threshold. Consider improving test coverage or test quality.

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.

  • Killed mutants: Tests caught the mutation (good!)
  • Survived mutants: Tests missed the mutation (needs improvement)

@codecov
Copy link
Copy Markdown

codecov Bot commented May 13, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.38%. Comparing base (d014841) to head (718490d).

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           
Flag Coverage Δ
integration 87.34% <ø> (-0.04%) ⬇️
unittests 84.14% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- 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 <info@sebastianmendel.de>
@sonarqubecloud
Copy link
Copy Markdown

Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated approval for maintainer PR

All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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+tls to the DOCKER_HOST scheme 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.

Comment thread core/adapters/docker/client.go
Comment thread docs/TROUBLESHOOTING.md
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread core/adapters/docker/client.go
@CybotTM CybotTM added this pull request to the merge queue May 13, 2026
Merged via the queue into main with commit 98f5f1a May 13, 2026
27 checks passed
@CybotTM CybotTM deleted the fix/issue-616 branch May 13, 2026 18:40
CybotTM added a commit that referenced this pull request May 14, 2026
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 <info@sebastianmendel.de>
@CybotTM CybotTM mentioned this pull request May 14, 2026
5 tasks
techsolo12 pushed a commit to techsolo12/ofelia that referenced this pull request May 14, 2026
…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)
techsolo12 pushed a commit to techsolo12/ofelia that referenced this pull request May 14, 2026
Cut the [Unreleased] block at fad5239 into a versioned [0.25.0] -
2026-05-14 heading, and add two entries for PRs that landed after
the previous Unreleased writeups:

- ### Security: Go toolchain 1.26.2 -> 1.26.3 (netresearch#662) — clears six
  stdlib advisories (net/mail, html/template, net, net/http) reachable
  from this codebase. Direct deps refreshed in lockstep; full
  indirect-graph refresh. Post-bump, only the two unfixable upstream
  moby advisories on docker/docker v28.5.2 remain (GO-2026-4887,
  GO-2026-4883).
- ### Fixed: MaxRuntime cancellation now stops *and removes* the
  container/service (netresearch#659, fixes netresearch#655) — completes netresearch#651's deadline
  wiring with a fresh `context.WithTimeout(context.Background(),
  jobCleanupTimeout)` cleanup context so stop/remove still runs
  after the parent deadline fires. Mirrors the same fix into
  RunServiceJob.

Version bump rationale: 0.x semver — minor bump because v0.24.0..main
includes one `feat:` (tcp+tls:// scheme re-enabled in netresearch#625), several
`fix(security):` PRs that surface previously-silent downgrades, and
the `[global]` label-handling rework. No file other than CHANGELOG.md
is touched; the Release workflow injects the version via ldflags from
the tag.

Test plan:
- [x] go build ./... clean
- [ ] CI green on the PR
- [ ] CHANGELOG renders correctly on GitHub
- [ ] After merge: signed annotated tag v0.25.0 pushed to origin
- [ ] Release workflow run succeeds (binaries, container, cosign
      bundle, SLSA attestations)

Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
techsolo12 pushed a commit to techsolo12/ofelia that referenced this pull request May 14, 2026
## 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Re-enable tcp+tls:// DOCKER_HOST scheme once #613 lands

2 participants