FreeAndNil commented on PR #329:
URL: https://github.com/apache/logging-log4net/pull/329#issuecomment-6122125149
Implementation plan for #329, kept here for the record. Revised after the
merge so that it describes the change as it landed.
<details>
<summary>Plan</summary>
# PR #329: require transport security in the Ext.Mail SmtpAppender
## Context
f026 of scan `da18b6fd`, the last undecided finding: `log4net.Ext.Mail`'s
`SmtpAppender` defaults to
`TransportSecurity = SmtpTransportSecurity.None`, so the mail body, and with
`Basic` or `Ntlm`
configured the SMTP credentials, cross the network in cleartext unless the
operator finds the knob.
A network-adjacent attacker on the path to the relay captures both
passively. The panel split 3/2,
the majority holding that an insecure security-relevant default in new code
is not operator
configuration, because the operator did nothing.
**Decision: flip the default to `Required`.** Implicit TLS on port 465,
mandatory `STARTTLS` on
every other port, connection fails rather than continuing in cleartext. The
opt-out is the existing
setting, `TransportSecurity=None` or `EnableSsl=false`, so no new API.
The package shipped in **3.4.0**, so this lands on existing users: a relay
on port 25 that offers no
`STARTTLS` stops working on upgrade. The blast radius is one to five users
in total, so the
compatibility argument that decided f005 does not reach here, and two things
separate the cases
anyway. Say both in the commit message and keep the changelog to the opt-out:
- f005's raw-SQL fallback is a configuration the operator **chose**, is
documented, and warns at
startup. Removing it takes away a working deployment with no replacement.
- f026 is a default **nobody chose**. The fix is one line of configuration
away for anyone who
really wants cleartext, and the failure is loud at connect time rather
than silent on the wire.
`#310` already built the machinery in 3.4.0: `TransportSecurity`,
`EnableSsl` as its shorthand, and
`ResolveSecureSocketOptions` deliberately avoiding MailKit's downgrade-prone
`Auto`. This changes
one initialiser and the documentation around it, in one commit.
## Branch
`Feature/329-ext-mail-tls-default`, branched from the **#328 tip**
(`81efd407`) while #328 is open,
the way #324 branched off #323. `git switch -c` from the local branch, never
off a remote ref.
## The change
`src/log4net.Ext.Mail/Appender/SmtpAppender.cs`
```csharp
public SmtpTransportSecurity TransportSecurity { get; set; } =
SmtpTransportSecurity.Required;
```
Everything else follows from it and must be updated in the same commit:
- The `<value>` doc on `TransportSecurity` says "The default is `None`". It
becomes `Required`, with
one sentence naming `None` as the opt-out for a cleartext connection.
- `EnableSsl` is `TransportSecurity != None`, so **its default flips from
`false` to `true`**. Its
`<remarks>` must say so: this is the property most configurations touch,
and its old default
matched `System.Net.Mail.SmtpClient.EnableSsl`, which it no longer does.
The sentence reads
"which matches the behaviour, though not the default, of" that property.
- `Port` stays 25, so the default resolves to
`SecureSocketOptions.StartTls`, not `SslOnConnect`.
That is the behaviour users will actually meet.
`src/log4net.Ext.Mail/README.md`
- Line 86, "`enableSsl` - negotiate TLS/STARTTLS (true/false, default:
false)" becomes "require
TLS/STARTTLS (true/false, default: true)", and a `transportSecurity`
bullet follows it listing
the five values with `Required` marked as the default.
- Lines 67 and 70 still describe the transport as "Full (Auto, Explicit,
Implicit)" and `enableSsl`
as "negotiates Auto/None", which #310 made wrong: `Required` never uses
`Auto`. That is the text
behind f027, which the scan filed as a false positive and which is
nonetheless stale. The TLS row
lists the four secured values, the `enableSsl` row shows both defaults,
`false` built in and
`true` here. Fix it here rather than leaving a contradiction in the file
we are editing, and say
so in the commit message.
`src/site/antora/modules/ROOT/pages/manual/configuration/appenders/smtpappender.adoc`
documents both
properties, so it changes in four places: the `enableSsl` row defaults to
`true`, the
`transportSecurity` row lists `Required` first as the default, the `None`
and `Required` rows of the
value table swap which one says "the default" (`None` becomes "the opt-out
from the secure
default"), and the migration paragraph says `enableSsl` keeps its meaning
but not its default, so a
configuration that set neither option now requires transport security.
## Tests
`src/log4net.Ext.Mail.Tests/Appender/SmtpAppenderTest.cs`. The fixture
drives a hand-written
`FakeSmtpTransport` that records `SecureSocketOptions` from `Connect`, so
every assertion here is
exact and offline. Three existing tests encode the old default and must
change:
- `EnableSslOffConnectsWithoutTransportSecurity` calls `CreateAppender()`
and asserts
`SecureSocketOptions.None` **without ever setting `EnableSsl`**. It has to
set
`appender.EnableSsl = false` explicitly, which is what its name always
claimed it did.
- `EnableSslIsAShorthandForTransportSecurity` asserts a fresh appender is
`None` and `EnableSsl`
false. It starts by setting `EnableSsl = false` and asserting `None`,
keeps the round trip
through `Required` and `StartTlsWhenAvailable`, and closes by setting
`TransportSecurity` to
`None` and asserting `EnableSsl` false, so both directions of the
shorthand are still covered.
- `DefaultConstructorUsesTheMailKitTransport` asserts the defaults of a
fresh appender, among them
`EnableSsl` false, which becomes true.
New, and red against the old code, each with a doc comment:
- `TransportSecurityDefaultsToRequired`, a one-line property assertion.
- `EnableSslDefaultsToTrue`, because that public property's default is part
of the break.
- `TheDefaultConnectionRequiresStartTls`: `CreateAppender()`, `Append`,
assert
`SecureSocketOptions.StartTls` on the default port 25. This is the
regression test for the
finding, and it takes the place of `EnableSslOnRequiresStartTls`, whose
explicit `EnableSsl =
true` now says nothing the default does not.
## Changelog
`src/changelog/3.5.0/329-ext-mail-tls-by-default.xml`, `type="changed"`,
issue 329 with both `id`
and `link`. The description names the weakness class (CWE-319) and the
opt-out, so an upgrade
surprise is searchable, the way the `SendTimeoutMillis`,
`MatchTimeoutMillis` and
`LockTimeoutMillis` entries do, and closes with `(audit da18b6fd-f026,
implemented by @FreeAndNil)`.
Prose only, no bullets.
Answer the scan on the finding as a decision: we took the majority position
and accept the break
it costs, and f005 went the other way for a reason that does not apply here.
## Verification
```
dotnet build src/log4net.sln #
0 warnings, both TFMs
dotnet test src/log4net.Ext.Mail.Tests/log4net.Ext.Mail.Tests.csproj #
50 before, 52 after
dotnet test src/log4net.Tests/log4net.Tests.csproj -f net10.0 #
424, unchanged
```
- Revert the one-line default in place, no stash, and confirm the three new
tests fail.
- The whole change is offline: `FakeSmtpTransport` means no relay and no
network in the suite.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
