Verify the SMTP server's identity, with smtp.verify-server-identity to opt out

[dmccoystephenson]

Summary

• The question issue SMTP server identity verification is neither set nor documented #48 turns on has been answered empirically rather than from memory. The pinned com.sun.mail:jakarta.mail:2.0.1 was disassembled (javap -c on com.sun.mail.util.SocketFetcher), and mail.smtp.ssl.checkserveridentity is read with iconst_0 — a default of false. Server identity was therefore not being verified under either encryption mode. The relevant bytecode is quoted in the self-review comment on this PR.
• mail.smtp.ssl.checkserveridentity is now set explicitly, in both directions, whenever smtp.use-tls or smtp.implicit-tls is in use. Whether the certificate is checked against smtp.server is thereby made a property of Herald rather than of whichever library version happens to be pinned — later releases of the library default the property the other way.
• The property is left unset when neither encryption mode is on, since no handshake takes place for it to govern.
• A new smtp.verify-server-identity key, defaulting to true, was added rather than the verification being made unconditional. Issue SMTP server identity verification is neither set nor documented #48 names this as the open decision. The opt-out was chosen because a legitimate configuration is affected — a relay dialled by IP address, or one presenting a certificate for another name — which is unlike the smtp.use-tls precedent, where no legitimate reason to opt out of a required STARTTLS existed once the key had been set to true. Strictness is preserved as the default; the escape hatch is documented rather than being a broken plugin.
• EmailNotifier.describeUnverifiedServerIdentity was added and is logged at startup when verification is turned off while an encryption mode is on, following the pattern already set by describeCredentialExposure and describePortTlsMismatch. It is silent on an unencrypted connection, where there is no certificate to check and the credential-exposure warning already names the larger problem.
• Existing EmailNotifier constructors delegate with verifyServerIdentity set to true, so the secure setting is what a caller gets without asking.
• implicit-tls was also added to the testEveryConfigKeyIsShipped key list in HeraldIntegrationTest, alongside the new key. Its absence was a pre-existing gap in the same one-line list this change had to edit.

Behaviour change

An installation whose relay presents a certificate that cannot name the address it is dialled at will begin to fail its sends after this change, with a certificate error in the log. CONFIG.md, USER_GUIDE.md and CHANGELOG.md each state this, name the log line it produces, and give both remedies: pointing smtp.server at the name the certificate carries, or setting smtp.verify-server-identity to false.

Test plan

• ./gradlew clean build — BUILD SUCCESSFUL
• ./gradlew clean test — 307 tests executed, 0 failures, 0 skipped (count read from build/test-results/test/*.xml, not from the banner)
• Regression evidence: with only the checkserveridentity block reverted, 4 of the new tests fail (Server identity verification should be requested on the STARTTLS path, ... on the implicit TLS path, The constructors that predate the setting should verify the identity, Turning verification off should be stated explicitly, not left to the library default); with it restored, all 307 pass.
• Nine tests were added across SMTP Session Property Tests and a new Server Identity Verification Tests nest, covering both encryption modes, both settings of the key, the pre-existing constructors, the unencrypted case, and every branch of the new warning.
• CONFIG.md carries a smtp.verify-server-identity section in the shape of its siblings — type, default, description, worked log line and example — and the smtp.use-tls and smtp.implicit-tls sections now say what they do not cover, which is the misreading issue SMTP server identity verification is neither set nor documented #48 identified.

Deferred this cycle

Issue #39 (dms-conventions alignment) was not picked up. It spans twenty-one rubric items across five sections and would exceed this loop's scope ceiling as a single PR, and its first section calls for a CLAUDE.md to be authored — agent-loaded configuration, which this loop is required to have authorized separately rather than writing on its own initiative. Splitting it into per-section issues, and an explicit decision on the CLAUDE.md portion, are both wanted before it is picked up.

Closes #48

This PR description was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).

---

drafted by Claude on behalf of Daniel Stephenson

[Copilot]

🟢 Approval recommended

The change is well-scoped, explicitly documented, and backed by targeted tests covering both TLS modes and the new opt-out behavior.

Pull request overview

This PR makes SMTP server identity verification an explicit Herald behavior (instead of depending on Jakarta Mail defaults) by setting mail.smtp.ssl.checkserveridentity whenever STARTTLS or implicit TLS is used, while adding a new smtp.verify-server-identity config key (default true) to allow opt-out for edge-case relays.

Changes:

• Add smtp.verify-server-identity (default true) and plumb it through configuration loading into EmailNotifier.
• Explicitly set mail.smtp.ssl.checkserveridentity to true/false whenever an encrypted SMTP mode is enabled, and add a startup warning when verification is opted out.
• Add/extend tests and update documentation (CONFIG + user guide + changelog) to describe the behavior change and opt-out.

File summaries

File	Description
USER_GUIDE.md	Documents the new key, the startup warning, and how to recognize hostname/cert mismatch failures.
src/test/java/com/dansplugins/herald/HeraldIntegrationTest.java	Ensures new config keys are present in the shipped config.yml.
src/test/java/com/dansplugins/herald/EmailNotifierTest.java	Adds session-property assertions and warning behavior tests for server identity verification.
src/main/resources/config.yml	Ships the new smtp.verify-server-identity key (default true) with explanatory comments.
src/main/java/com/dansplugins/herald/Herald.java	Loads the new config key, passes it to EmailNotifier, and logs a warning when opted out.
src/main/java/com/dansplugins/herald/EmailNotifier.java	Adds the new constructor parameter, warning helper, and explicitly sets mail.smtp.ssl.checkserveridentity under TLS modes.
CONFIG.md	Adds a dedicated section for smtp.verify-server-identity and clarifies what TLS mode keys do/do not cover.
CHANGELOG.md	Records the behavior change and migration/opt-out guidance.

Review details

• Files reviewed: 8/8 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[dmccoystephenson]

Self-review rubric

Scored adversarially against the diff and against command output, on head 302476c.

• Scope: PASS — eight files, all reachable from SMTP server identity verification is neither set nor documented #48: the notifier and its wiring, the shipped config, the three documentation sources of truth the key appears in, and two test files. One line is arguably beyond the issue: implicit-tls was added to the testEveryConfigKeyIsShipped key list in HeraldIntegrationTest alongside verify-server-identity. That list was already being edited for this change and its omission was a pre-existing gap in the same invariant; it is called out here rather than smuggled through.
• Tests-new: PASS — the one new public method, EmailNotifier.describeUnverifiedServerIdentity, is covered by four tests spanning every branch: off under STARTTLS, off under implicit TLS, on under all three encryption states, and off with no encryption. The new constructor overload is exercised by the five SMTP Session Property Tests additions, and the delegating overloads by testIdentityVerifiedByDefault.
• Tests-fix: PASS — confirmed empirically, not by reasoning. With only the checkserveridentity block in buildSessionProperties reverted, ./gradlew clean test reported 307 tests completed, 4 failed: Server identity verification should be requested on the STARTTLS path, ... on the implicit TLS path, The constructors that predate the setting should verify the identity, and Turning verification off should be stated explicitly, not left to the library default. With the block restored, 307 pass.
• Sibling structure: PASS after a fix. The smtp.verify-server-identity section in CONFIG.md carries the type, default, description, worked log line and example that every sibling section carries. config.yml initially scored FAIL: the key was given a hanging comment block indented to the trailing-comment column, a shape that appears nowhere else in the file, because the key name is longer than the column its siblings align at. It was moved above the key in 302476c, which is the file's other established shape — already used for server-name and for the email.subject/email.body pair.
• Sibling renames: PASS — nothing was renamed. The new describer was named describeUnverifiedServerIdentity to sit in the existing describeCredentialExposure / describeTlsModeConflict / describePortTlsMismatch series, and is invoked from the same block in Herald.loadConfiguration as the other two warnings.
• Docs: PASS — CHANGELOG.md gains an [Unreleased] / Added entry naming the behaviour change and its remedy; CONFIG.md gains the key's own section, and the smtp.use-tls and smtp.implicit-tls sections now state what they do not cover, which is the misreading SMTP server identity verification is neither set nor documented #48 identified; USER_GUIDE.md gains a setup step, a startup-log line and a failure-recognition entry. COMMANDS.md and plugin.yml are untouched and correctly so — no command or permission is involved. README.md links the guides rather than enumerating keys, so it needs nothing.
• Issue resolution: PASS — all three of SMTP server identity verification is neither set nor documented #48's asks are answered. (1) The default was established by disassembling the pinned jar rather than recalled: javap -c com.sun.mail.util.SocketFetcher shows ldc ".ssl.checkserveridentity" followed by iconst_0 into PropUtil.getBooleanProperty, so the default is false and identity was not being verified. (2) The property is now set explicitly in both directions wherever an encryption mode is in use, with buildSessionProperties coverage alongside the STARTTLS and implicit TLS assertions. (3) CONFIG.md states what is checked, next to both encryption entries.
• CI: PASS — build and test both pass on head 302476c. Build and Deploy Plugin reports skipping, which is its correct state here: deploy.yml is branch-gated to pushes on main, so it carries no signal for a pull request. It fails on main for an environmental reason unrelated to this diff, and is not a merge gate for this PR.
• i18n: PASS — no user-facing string is hardcoded in a way that departs from the existing convention. Herald has no src/main/resources/lang/ directory; every operator-facing string in this codebase is an English log message assembled in the notifier that owns it, and the new warning follows describeCredentialExposure exactly, naming config keys rather than quoting any configured value.
• Config-docs: PASS — smtp.verify-server-identity appears in CONFIG.md with description, type (boolean) and default (true), in config.yml, and is now asserted present by testEveryConfigKeyIsShipped.

Judgment calls left for the reviewer

src/main/resources/config.yml — the opt-out key exists at all. #48 states that whether verification needs its own opt-out is the decision it turns on, and the smtp.use-tls precedent points the other way, toward unconditional strictness. The opt-out was chosen because the two cases differ in a way the precedent does not carry across: no legitimate configuration wanted STARTTLS to stay optional once smtp.use-tls had been set to true, whereas a relay dialled by IP address, or one presenting a certificate for another name, is a real deployment that this change would otherwise break with no documented way forward. Strictness remains the default either way; what the key buys is that the alternative is discoverable rather than being a plugin that stopped sending. Collapsing it to unconditional verification later is a smaller change than adding it after installations have broken.

src/test/java/com/dansplugins/herald/EmailNotifierTest.java:649 — the identityNotifier helper hardcodes port 587 and is then called with implicitTLS true, a port and mode pairing that describePortTlsMismatch would warn about. It is harmless, because buildSessionProperties does not consult the port when deciding the TLS properties, and it matches the shape of the existing notifier helper directly above it; it was left rather than churned, but a reviewer who would rather see IMPLICIT_TLS_PORT there is not wrong.

Out-of-diff observation

describeUnverifiedServerIdentity is called only on the branch where email notifiers load successfully, matching where describeCredentialExposure and describePortTlsMismatch are called. An operator whose email configuration is incomplete therefore fixes the incompleteness first and learns about the unverified identity on the next restart. That ordering is pre-existing rather than introduced here, and is not proposed for change under this PR.

This review was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).

---

drafted by Claude on behalf of Daniel Stephenson

[dmccoystephenson]

Merge gate — held for a maintainer decision

Every mechanical gate is satisfied on head 302476c:

• build and test pass. Build and Deploy Plugin is skipping, its correct state on a pull request: deploy.yml is gated to pushes on main. Its standing failure there is environmental and carries no signal for this diff.
• Regression evidence was taken empirically, not by reasoning: reverting only the checkserveridentity block fails 4 of the new tests and restoring it passes all 307.
• The Phase 7 documentation pass was completed and found nothing wrong. The warning quoted in CONFIG.md was compared character-for-character against the string built in EmailNotifier.describeUnverifiedServerIdentity and matches exactly; the key and default in CONFIG.md match what Herald.loadConfiguration reads; COMMANDS.md and plugin.yml correctly carry nothing, since no command or permission is involved.
• No modified path matches the do-not-auto-merge list. The CHANGELOG.md edit is an entry under [Unreleased], which that list exempts.
• The claim that the property governs both encryption modes was verified against the pinned jar rather than assumed: in com.sun.mail.util.SocketFetcher, the property is read inside configureSSLSocket, which is reached from createSocket (the implicit TLS path) and from startTLS (the STARTTLS upgrade path).

The merge was nevertheless not taken autonomously, for two reasons.

1. The design decision #48 reserved. That issue states that whether verification is made unconditional or given its own opt-out key is the decision it turns on, and asks for the choice to be made deliberately rather than by precedent. A deliberate choice was made and is argued for in the self-review above — an opt-out, defaulting to strict — but shipping a change that will stop mail from being sent on any installation whose relay presents a certificate that cannot name the address it is dialled at is the maintainer's call to ratify, not this session's. Should unconditional verification be preferred instead, removing the key is a smaller follow-up than adding it after installations have broken.

2. One reviewer comment could not be read. The Copilot review recommends approval and reports one inline comment generated at Lite effort. That comment could not be retrieved in this session, because the API routes that return inline review comments are outside this run's allow-list. It is therefore unread rather than addressed, and is called out as such instead of being silently treated as resolved.

This comment was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).

---

drafted by Claude on behalf of Daniel Stephenson

[dmccoystephenson]

Rebase, re-verification and merge decision

This PR was found open, carrying the marker of an interrupted session, and was continued rather than duplicated.

Brought current with main. The branch was two commits ahead of and one behind main (#50, usage reporting to trace). It was rebased onto 0bfad10 with no conflicts — CHANGELOG.md, CONFIG.md, Herald.java and config.yml all auto-merged — and force-pushed as head 90f85a1. The diff against main is unchanged in content: 8 files, +205/−3.

Anchor re-run on the rebased tree. ./gradlew clean test — BUILD SUCCESSFUL, 317 tests executed, 0 failures, 0 errors, 0 skipped (count read from build/test-results/test/*.xml, not from the banner; up from 307 because #50 brought its own tests). build and test pass on head 90f85a1; Build and Deploy Plugin is skipping, its correct state on a pull request.

The diff was reviewed again in this session, against the repo's conventions, with nothing blocking found:

• src/main/java/com/dansplugins/herald/EmailNotifier.java:397 — mail.smtp.ssl.checkserveridentity is set only when implicitTLS || useTLS, in both directions, which is what CONFIG.md states. The existing 10-argument constructor delegates with true, so no caller is silently weakened.
• src/main/java/com/dansplugins/herald/Herald.java:123 — the key is read with a default of true, matching config.yml and CONFIG.md; the warning is emitted from the same block as describeCredentialExposure and describePortTlsMismatch.
• src/test/java/com/dansplugins/herald/HeraldIntegrationTest.java:482 — the added implicit-tls entry is a one-line closure of a pre-existing gap in a list this change already had to edit; it is called out in the self-review above rather than smuggled.
• src/test/java/com/dansplugins/herald/EmailNotifierTest.java:649 — the port-587-with-implicit-TLS pairing in the identityNotifier helper noted in the self-review remains harmless (buildSessionProperties does not consult the port) and was left as is.

The one Copilot inline comment remains unread. The endpoints that return inline review comments are outside this run's allow-list, as they were for the previous session. Copilot's overall verdict is "approval recommended"; the diff was reviewed independently here instead, as above.

Merge decision. The previous session held this PR for a maintainer to ratify the opt-out design and the behaviour change. In the three days since, no comment has been left on this PR or on #48. This run is pre-authorized to merge on this repository, and the readiness bar is met: tests green on the exact head, docs verified against the implementation, no path on the do-not-auto-merge list (the CHANGELOG.md edit sits under [Unreleased]). The design question is resolved in the direction the self-review argued for — strict by default, with a documented escape hatch — and reversing it later (removing the key) is a smaller change than adding it after installations have broken. The PR is therefore being merged. Any installation whose relay presents a certificate that cannot name smtp.server will find the remedy in CHANGELOG.md, CONFIG.md and USER_GUIDE.md.

This comment was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).

---

drafted by Claude on behalf of Daniel Stephenson