FreeAndNil commented on PR #324:
URL: https://github.com/apache/logging-log4net/pull/324#issuecomment-6121984337
Implementation plan for #324, kept here for the record. Revised after the
merge so that it describes the change as it landed.
<details>
<summary>Plan</summary>
# PR #324: the sinks cluster (f015, f042, f037)
## Context
Continuing the remediation of the 2026-08-29 scan after #323, with three
findings from scan
`da18b6fd`, all about what leaves the process:
- **f015**, MEDIUM, `UdpAppender`: a rendered event over the datagram limit
makes `Send` throw, the
event is lost, and `OnlyOnceErrorHandler` reports only the first loss.
Event loss is this
project's main risk class, so it goes first.
- **f042**, LOW, `XmlHierarchyConfigurator`: with `debug="true"` the
configurator echoes every
configured value, passwords and connection strings included.
- **f037**, LOW, `SmtpPickupDirAppender`: the file ends with a lone dot but
body lines are not dot
stuffed, so a logged line that is only a dot can end the mail early for a
dot-honouring agent.
None of the three is a vulnerability. Outcome: three commits on a new
branch, each with a
regression test, a changelog entry and, where a property or a file format
changes, the Antora
page.
## Branch
Changelog entries go in `src/changelog/3.5.0/324-*.xml` with
`<issue id="324"
link="https://github.com/apache/logging-log4net/pull/324"/>`.
## Commit 1: f015, oversize datagram truncated instead of dropped
**Measure first.** Send a datagram over the limit through a `UdpClient` on
loopback and record the
threshold: 65507 bytes are sent, 65508 fail. That is the IPv4 UDP payload
maximum, and an IPv6
packet would carry 20 bytes more, which is not worth a second limit. The
numbers go into the PR
body.
`src/log4net/Appender/UdpAppender.cs`:
- New `public int MaxDatagramSize` property, default 65507, validated in its
setter rather than in
`ActivateOptions`, range 512 to 65507 as two private constants, via
`SystemInfo.CreateArgumentOutOfRangeException`. The remarks say why it
cannot be raised.
- New `private protected byte[] GetDatagramBytes(string message)`: encode
with `Encoding`; return
as is when it fits. Otherwise a private `GetTruncatedBytes` runs the
message through
`Encoding.GetEncoder().Convert` with `flush: false` into a budget of
`MaxDatagramSize` minus the
marker length, writing into the oversize encoding itself, which is the one
buffer already long
enough. `Convert` never splits a character and, unflushed, holds back a
trailing high surrogate
rather than emitting the replacement character. Append the marker
`...[truncated]` and report
the truncation with the byte counts through `ErrorHandler`.
- `Append` calls it instead of `Encoding.GetBytes(...)`.
`src/log4net/Appender/RemoteSyslogAppender.cs`: `Append` runs the finished
`builder.ToString()`
through the same helper before enqueueing. Its `Encoding` is ASCII and
`AppendMessage` has already
escaped everything outside 0x20 to 0x7E, so the marker stays inside the RFC
3164 range.
Tests, `src/log4net.Tests/Appender/UdpAppenderTest.cs` (new) and an addition
to
`RemoteSyslogAppenderTest.cs`:
- `GetDatagramBytes` is `private protected`, so drive it by reflection from
a small `Encode`
helper in the fixture; no socket needed for the size cases. Cover: the
default is 65507; 511
and 65508 throw `ArgumentOutOfRangeException`; a message that fits is
byte-for-byte unchanged; a
message of exactly the limit is unchanged and nothing is reported; an
oversize one comes back at
`MaxDatagramSize` bytes, ends in the marker and is reported once through a
`RecordingErrorHandler`; a two-byte encoding is cut on a character
boundary (513 bytes hold the
14 byte marker and 249 characters); a surrogate pair straddling the cut is
not split, asserted
as no replacement character, ordinal.
- The syslog case goes through the existing `ExecuteAppend` helper in
`RemoteSyslogAppenderTest`,
which gains an optional `maxDatagramSize`, asserting one datagram of
exactly the limit ending in
the marker.
- Wrap the expected `ErrorHandler`/`LogLog` output in
`LogLog.ExecuteWithoutEmittingInternalMessages` so the suite stays quiet.
- Revert the production change and watch each new test fail.
Docs: a paragraph on
`src/site/antora/modules/ROOT/pages/manual/configuration/appenders/udpappender.adoc`
(the page has
no property table) with the default, the lower bound and why it cannot be
raised, and a bullet on
`remotesyslogappender.adoc` naming the inherited property.
Changelog `324-udp-oversize-truncation.xml`, `type="fixed"`, credit
`(audit da18b6fd-f015, fixed by @FreeAndNil)`.
## Commit 2: f042, no secret values in the configuration debug echo
Share one redaction rule rather than growing a second one:
- Move `RedactConnectionString`, `DiagnosticKeywords` and `RedactedValue`
out of
`src/log4net/Appender/AdoNetAppender.cs:829-886` into a new internal
static helper,
`src/log4net/Util/Redact.cs`, as `Redact.ConnectionString`, the keyword
allowlist and an
internal `RedactedValue` constant. `AdoNetAppender` keeps calling it, so
its behaviour is
unchanged, and it loses its `System.Data.Common` using.
- Add to that helper a `Value(string name, object? value)` that returns the
masked form when the
configured parameter name contains a secret-bearing fragment
(case-insensitive: `password`,
`pwd`, `passphrase`, `secret`, `token`, `credential`, `apikey`), and
routes a name that **ends
with** `connectionstring` through the allowlist redaction so the server
part stays visible for
troubleshooting. Ends with, not contains: `ConnectionType` and
`ConnectionStringName` are
diagnostics, not secrets, and are the first things read when configuration
fails. A null value
passes through as null, everything else unchanged.
- Apply it at the nine `SetParameter` message sites in
`src/log4net/Repository/Hierarchy/XmlHierarchyConfigurator.cs`: the two
`Debug` value paths,
the failed-conversion `Warn`, the four setter-exception `Error` paths and
the two `Debug` object
paths, always keyed by the configured `name`. The `Error` paths matter
most: they are emitted
even without `debug="true"`. Property names and types keep being logged.
No other site echoes a
raw configuration value or the config XML.
Test, `src/log4net.Tests/Config/RedactTest.cs`, `[NonParallelizable]` since
it mutates
`LogLog.InternalDebugging`, with a `Configure` helper that turns internal
debugging on inside
`LogLog.ExecuteWithoutEmittingInternalMessages`, captures with
`LogLog.LogReceivedAdapter`,
configures a repository from an XML string and restores the flag in
`finally`. Three tests: a
password-named parameter on an SMTP appender is absent while the property
name and a non-secret
value are still present; a connection string keeps the server name and loses
the password; the
connection type and the connection string name survive. `Does.Not.Contain`
is culture sensitive,
so use `!Contains.Substring(secret).Using(StringComparison.Ordinal)`. Revert
and watch it fail.
Docs: one sentence in the internal debugging entry of `manual/faq.adoc`,
since that is the
documented first troubleshooting step: a value whose name reads as a secret
is replaced with
`*****`, the property is still named.
Changelog `324-config-debug-secret-values.xml`, `type="fixed"`, credit
`(audit da18b6fd-f042, fixed by @FreeAndNil)`.
## Commit 3: f037, drop the lone-dot terminator from the pickup file
Decided: remove the hazard rather than encode around it. A pickup file is an
RFC 822 message, not a
DATA stream, so the terminator buys nothing and no dot stuffing is needed
once it is gone. An agent
that expects SMTP DATA framing is not a supported consumer, and the page
says so.
`src/log4net/Appender/SmtpPickupDirAppender.cs:171`: delete
`writer.WriteLine(".");` and keep the
preceding `writer.WriteLine();` so the file still ends with a newline. Two
lines of comment saying
why there is no terminator.
Test in `src/log4net.Tests/Appender/SmtpPickupDirAppenderTest.cs`,
`ALoggedDotIsTheOnlyDotLineInTheFile`: log an event that is only a dot and
one after it, close the
logger, then assert the written file holds exactly one bare-dot line and the
second event, with
the silent error handler still empty. Use `using AutoTempFolder folder =
new();` as a local and
close the appender before the scope ends. Revert and watch it fail.
Docs: a paragraph in `smtppickupdirappender.adoc` stating the file is a
plain RFC 822 message read
to its end, with no SMTP DATA terminator and no dot stuffing.
Changelog `324-pickup-dir-dot-terminator.xml`, `type="changed"` (the written
file format changes),
credit `(audit da18b6fd-f037, implemented by @FreeAndNil)`.
## Verification, after every commit
```
dotnet build src/log4net.sln #
0 warnings, both TFMs
dotnet build src/log4net.Tests/log4net.Tests.csproj -f net462
-p:TargetFrameworks=net462
dotnet test src/log4net.Tests/log4net.Tests.csproj -f net10.0 #
413 at the end
dotnet test src/log4net.Ext.Mail.Tests/log4net.Ext.Mail.Tests.csproj #
50
```
Redirect build output to a file and read the whole file.
## Around the work
- Stage and propose a commit message, one file per commit in
`.git/plans/324/`, then wait. No
commits are made here.
- Cite findings as `audit da18b6fd-fNNN`.
- Read the scan's own patches for the three findings before writing each
test; its tests have
more than once been better than ours.
</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]
