fix(security): close 3 silent-downgrade vectors (#653)#660
Merged
Conversation
…RTTLS, webhook egress) Sibling-hunt findings from PR #646 review. Three additional defense-in-depth gaps where operator intent (TLS, encryption, egress restriction) was silently not honored. 1. Docker https:// with broken cert material (#653 vector 1): applyDockerTLS warned-and-continued when resolveTLSConfig failed; the SDK then dialed with default TLS — system CA, no client cert. NewClientWithConfig now gates https:// when material IS configured but unloadable, returning ErrHTTPSRequiresUsableCertMaterial. Asymmetry vs tcp+tls://: https:// without any material remains fail-open (system CA bundle is a legitimate posture). 2. SMTP OpportunisticStartTLS default (#653 vector 2): middlewares.NewMail inherited go-mail's OpportunisticStartTLS, sending credentials and message body in cleartext when STARTTLS was not advertised. New smtp-tls-policy INI key (mandatory|opportunistic|none); default mandatory. Unknown values normalize to mandatory with a WARN log line so a typo cannot weaken transport security. BREAKING for plaintext-only SMTP — see docs/TROUBLESHOOTING.md migration recipes. 3. Webhook empty-allowlist silent allow-all (#653 vector 3): SetGlobalSecurityConfig now emits a single startup-time slog.Warn when the resolved AllowedHosts admits all hosts. A typo in webhook-allowed-hosts previously yielded wide-open egress with no operator-visible signal. Includes: - Defensive-enum SMTPTLSPolicy.Valid()/Validate() with ErrInvalidSMTPTLSPolicy - Per-job inheritance for SMTPTLSPolicy in cli/config.go - smtp-tls-policy added to docker-labels allow-list - Documentation updates in CONFIGURATION.md and TROUBLESHOOTING.md - TDD tests for all three fixes (red→green verified) Fixes #653 Signed-off-by: Sebastian Mendel <info@sebastianmendel.de>
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.
✅ 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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #660 +/- ##
==========================================
+ Coverage 87.87% 87.89% +0.01%
==========================================
Files 88 88
Lines 10972 11036 +64
==========================================
+ Hits 9642 9700 +58
- Misses 1086 1091 +5
- Partials 244 245 +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:
|
![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 implements several security enhancements to prevent silent downgrades and improve configuration visibility. Key changes include a fail-closed mechanism for Docker HTTPS connections when configured TLS material is invalid, a new smtp-tls-policy setting that defaults SMTP STARTTLS to mandatory, and a startup warning for wide-open webhook egress. Review feedback suggests moving the SMTP policy resolution warning from the per-job execution path to the middleware initialization to avoid log spam and surface configuration issues earlier.
There was a problem hiding this comment.
Pull request overview
Closes three silent-downgrade vectors (sibling-hunt findings from PR #646) where operator security intent was not honored: HTTPS Docker daemon with broken TLS material, SMTP defaulting to opportunistic STARTTLS, and webhook empty-allowlist collapsing into allow-all egress.
Changes:
- Add
ErrHTTPSRequiresUsableCertMaterialgate inNewClientWithConfigsohttps://with configured-but-unloadable TLS material fails closed instead of dialing with default TLS. - Introduce
SMTPTLSPolicy(mandatory/opportunistic/none) withmandatorydefault, hydrated from a newsmtp-tls-policyINI key (also added to Docker label allow-list and inherited per-job). - Emit a one-shot
slog.WarnfromSetGlobalSecurityConfigwhen the resolved webhookAllowedHostsadmits all hosts, plus extensive TDD test coverage and docs inCONFIGURATION.md/TROUBLESHOOTING.md/CHANGELOG.md.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| core/adapters/docker/client.go | New https:// fail-closed gate when TLS material is configured but unloadable. |
| core/adapters/docker/errors.go | New typed sentinel ErrHTTPSRequiresUsableCertMaterial with rationale. |
| core/adapters/docker/client_https_tls_test.go | TDD coverage for the new HTTPS gate (reject/allow/positive cases). |
| middlewares/mail.go | Adds SMTPTLSPolicy enum, validation, resolver, and wires MandatoryStartTLS as the new default. |
| middlewares/mail_tls_test.go | Pins Valid/Validate enum surface and default-mandatory / policy-none behavior. |
| middlewares/mail_test.go | Updates existing plaintext fixture tests to opt into SMTPTLSPolicyNone. |
| middlewares/uncovered_ext_test.go | Same plaintext fixture adjustment for the dedup test. |
| middlewares/webhook_security.go | Adds warnIfWideOpenEgress startup-time WARN for ["*"] allow-list. |
| middlewares/webhook_security_warn_test.go | New test file pinning warn/no-warn matrix for the egress warning. |
| cli/config.go | Per-job inheritance of SMTPTLSPolicy from global. |
| cli/config_smtp_tls_policy_test.go | INI hydration + job inheritance/override coverage. |
| cli/docker-labels.go | Adds smtp-tls-policy to the global label allow-list. |
| docs/CONFIGURATION.md | Documents the new smtp-tls-policy key. |
| docs/TROUBLESHOOTING.md | Migration recipes for the breaking SMTP default. |
| CHANGELOG.md | Security-section entries for all three fixes. |
5 inline fixes per gemini + Copilot review: - resolveSMTPTLSPolicy now logs unknown values once per (typo) value via sync.Map gate instead of per-sendMail (gemini) - TestNewMail_RejectsInvalidTLSPolicy renamed to TestResolveSMTPTLSPolicy_UnknownFallsBackToMandatory (it never called NewMail; resolver is the actual choke point) (Copilot) - TestMail_DefaultPolicyIsMandatory no longer gates on a 500ms timer: waits for Run to complete first (deterministic), then asserts fromCh non-blockingly. Removes CI-runner flake (Copilot) - SetGlobalSecurityConfig godoc now correctly states it is invoked from BOTH NewWebhookManager (boot) and the live-reload path; the previous 'called once' wording was wrong (Copilot) - Validate method retained — tests pin it as a public surface that callers wanting hard-fail at config-load can branch on 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.
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Sibling-hunt findings from PR #646 review. Three additional silent-downgrade vectors where operator intent (TLS, encryption, egress restriction) was silently not honored. All share the same operator-trust failure mode: operator believes a security feature is on; it silently is not.
https://with broken cert material:applyDockerTLSwarned-and-continued whenresolveTLSConfigfailed; the SDK then dialed with default TLS (system CA, no client cert).NewClientWithConfignow gateshttps://when material IS configured but unloadable, returningErrHTTPSRequiresUsableCertMaterial. Asymmetry vstcp+tls://:https://without any material remains fail-open (system CA bundle is a legitimate posture).OpportunisticStartTLSdefault:middlewares.NewMailinherited go-mail'sOpportunisticStartTLS, sending credentials and message body in cleartext when STARTTLS was not advertised. Newsmtp-tls-policyINI key (mandatory|opportunistic|none); defaultmandatory. Unknown values normalize tomandatorywith aWARNlog line so a typo cannot weaken transport security. BREAKING for plaintext-only SMTP — seedocs/TROUBLESHOOTING.mdmigration recipes.SetGlobalSecurityConfignow emits a single startup-timeslog.Warnwhen the resolvedAllowedHostsadmits all hosts. A typo inwebhook-allowed-hostspreviously yielded wide-open egress with no operator-visible signal.Also includes:
SMTPTLSPolicy.Valid()/Validate()withErrInvalidSMTPTLSPolicySMTPTLSPolicyincli/config.gosmtp-tls-policyadded to docker-labels allow-listCONFIGURATION.mdandTROUBLESHOOTING.mdFixes #653
Test plan
go test ./... -count=1 -short -race— all packages greengolangci-lint run ./...— 0 issuesTestNewClientWithConfig_HTTPSRejectsUnreadableCertPathTestNewClientWithConfig_HTTPSAllowsNoCertMaterialTestNewClientWithConfig_HTTPSAcceptsValidCertMaterialTestSMTPTLSPolicy_Valid(table-driven)TestSMTPTLSPolicy_ValidateTestNewMail_RejectsInvalidTLSPolicyTestMail_DefaultPolicyIsMandatoryTestMail_PolicyNoneAllowsPlaintextTestSMTPTLSPolicy_ParsedFromINI(4 sub-tests for INI hydration)TestSMTPTLSPolicy_JobInheritsFromGlobalTestSMTPTLSPolicy_JobOverridesGlobalTestSetGlobalSecurityConfig_WarnsOnAllowAllTestSetGlobalSecurityConfig_WarnsOnEmptyTestSetGlobalSecurityConfig_NoWarnOnExplicitAllowListTestSetGlobalSecurityConfig_NoWarnOnNilSMTPTLSPolicy: SMTPTLSPolicyNoneagainst the plaintext test SMTP fixture (5 tests)TestConfigGlobalKeysAreDocumented) green for newsmtp-tls-policykey