diff --git a/src/changelog/3.5.0/324-config-debug-secret-values.xml b/src/changelog/3.5.0/324-config-debug-secret-values.xml new file mode 100644 index 00000000..6686aadb --- /dev/null +++ b/src/changelog/3.5.0/324-config-debug-secret-values.xml @@ -0,0 +1,14 @@ + + + + + stop the configurator echoing secret configuration values. A parameter whose name reads as a + secret, such as `password` or `connectionString`, is now logged as `*****`, the connection string + keeping only the keywords that name the server. This covers internal debugging, the documented + first troubleshooting step, and the four setter-failure messages, which are written even without + it (audit da18b6fd-f042, fixed by @FreeAndNil) + + diff --git a/src/changelog/3.5.0/324-pickup-dir-dot-terminator.xml b/src/changelog/3.5.0/324-pickup-dir-dot-terminator.xml new file mode 100644 index 00000000..e3826ba0 --- /dev/null +++ b/src/changelog/3.5.0/324-pickup-dir-dot-terminator.xml @@ -0,0 +1,14 @@ + + + + + `SmtpPickupDirAppender` no longer writes a lone dot at the end of the file. The pickup service + reads the file to its end, so the terminator was never needed, and it made a logged line that is + only a dot look like the end of the mail. Body lines are still written verbatim rather than dot + stuffed, so an agent that expects SMTP DATA framing is not a supported consumer (audit + da18b6fd-f037, implemented by @FreeAndNil) + + diff --git a/src/changelog/3.5.0/324-udp-oversize-truncation.xml b/src/changelog/3.5.0/324-udp-oversize-truncation.xml new file mode 100644 index 00000000..948414a8 --- /dev/null +++ b/src/changelog/3.5.0/324-udp-oversize-truncation.xml @@ -0,0 +1,14 @@ + + + + + stop `UdpAppender` and `RemoteSyslogAppender` losing an event the socket refuses to send. An + event over the new `MaxDatagramSize` (default 65507, configurable down to 512) is truncated on a + character boundary and marked `...[truncated]`, instead of being dropped with only the first loss + reported. The limit is the IPv4 maximum and cannot be raised, an IPv6 packet would carry 20 bytes + more (audit da18b6fd-f015, fixed by @FreeAndNil) + + diff --git a/src/log4net.Tests/Appender/RemoteSyslogAppenderTest.cs b/src/log4net.Tests/Appender/RemoteSyslogAppenderTest.cs index 9eb299a2..3a0118f0 100644 --- a/src/log4net.Tests/Appender/RemoteSyslogAppenderTest.cs +++ b/src/log4net.Tests/Appender/RemoteSyslogAppenderTest.cs @@ -67,6 +67,21 @@ public void NonAsciiContentIsEscapedAndNotDeleted() Is.EqualTo(@"<14>TestDomain: INFO - Sch\u00f6nwetter \u4f60\u597d")); } + /// + /// A datagram over the limit is rejected by the socket, which used to cost the whole record. + /// + [Test] + public void AnOversizeRecordIsTruncatedInsteadOfLost() + { + List sentBytes = []; + LogLog.ExecuteWithoutEmittingInternalMessages( + () => sentBytes = ExecuteAppend(new string('a', 1000), maxDatagramSize: 512)); + + Assert.That(sentBytes, Has.Count.EqualTo(1)); + Assert.That(sentBytes[0], Has.Length.EqualTo(512)); + Assert.That(Encoding.ASCII.GetString(sentBytes[0]), Does.EndWith("...[truncated]")); + } + /// A control character other than CR or LF was dropped as well. [Test] public void OtherControlCharactersAreEscaped() @@ -275,7 +290,8 @@ public void IdentityWithoutControlCharactersIsUnchangedAndNotReported() private static List ExecuteAppend(string message, SyslogNewLineHandling newLineHandling = default, - string? identity = null) + string? identity = null, + int? maxDatagramSize = null) { System.Net.IPAddress ipAddress = new([127, 0, 0, 1]); RemoteAppender appender = new() @@ -285,6 +301,10 @@ private static List ExecuteAppend(string message, NewLineHandling = newLineHandling, Identity = identity is null ? null : new PatternLayout(identity) }; + if (maxDatagramSize is int size) + { + appender.MaxDatagramSize = size; + } appender.ActivateOptions(); LoggingEvent loggingEvent = new(new() { diff --git a/src/log4net.Tests/Appender/SmtpPickupDirAppenderTest.cs b/src/log4net.Tests/Appender/SmtpPickupDirAppenderTest.cs index cefcee9f..5e161cc1 100644 --- a/src/log4net.Tests/Appender/SmtpPickupDirAppenderTest.cs +++ b/src/log4net.Tests/Appender/SmtpPickupDirAppenderTest.cs @@ -149,6 +149,29 @@ public void ContentThatCannotBeEncodedDoesNotDestroyTheBatch() Assert.That(sh.Message, Is.EqualTo(string.Empty), "Unexpected error message"); } + /// + /// The appender used to close the file with a lone dot, so a logged line that is only a dot was + /// indistinguishable from it and ended the mail early for an agent honouring the terminator. + /// + [Test] + public void ALoggedDotIsTheOnlyDotLineInTheFile() + { + using AutoTempFolder pickupDir = new(); + SilentErrorHandler sh = new(); + SmtpPickupDirAppender appender = CreateSmtpPickupDirAppender(pickupDir.Path, sh); + ILogger log = CreateLogger(appender); + + log.Log(GetType(), Level.Info, ".", null); + log.Log(GetType(), Level.Info, "the event after it", null); + DestroyLogger(); + + string[] lines = File.ReadAllLines(Directory.GetFiles(pickupDir.Path)[0]); + + Assert.That(Array.FindAll(lines, line => line == "."), Has.Length.EqualTo(1)); + Assert.That(lines, Does.Contain("the event after it")); + Assert.That(sh.Message, Is.EqualTo(string.Empty), "Unexpected error message"); + } + /// /// Tests if the sent message contained the date header. /// diff --git a/src/log4net.Tests/Appender/UdpAppenderTest.cs b/src/log4net.Tests/Appender/UdpAppenderTest.cs new file mode 100644 index 00000000..c7512ad0 --- /dev/null +++ b/src/log4net.Tests/Appender/UdpAppenderTest.cs @@ -0,0 +1,121 @@ +#region Apache License +// +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to you under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +#endregion + +using System; +using System.Linq; +using System.Reflection; +using System.Text; +using log4net.Appender; +using log4net.Tests.Appender.Internal; +using NUnit.Framework; + +namespace log4net.Tests.Appender; + +/// +/// Tests for +/// +[TestFixture] +public sealed class UdpAppenderTest +{ + private const string Marker = "...[truncated]"; + + /// Calls the internal encoder the way Append does. + private static byte[] Encode(UdpAppender appender, string message) + => (byte[])typeof(UdpAppender) + .GetMethod("GetDatagramBytes", BindingFlags.Instance | BindingFlags.NonPublic)! + .Invoke(appender, [message])!; + + /// The maximum UDP payload of an IPv4 packet, measured as the first size the socket rejects minus one. + [Test] + public void MaxDatagramSizeDefaultsTo65507() + => Assert.That(new UdpAppender().MaxDatagramSize, Is.EqualTo(65507)); + + /// Below 512 nothing useful fits, above 65507 an IPv4 socket rejects the datagram. + [TestCase(511)] + [TestCase(65508)] + public void MaxDatagramSizeRejectsValuesOutsideTheRange(int value) + => Assert.That(() => new UdpAppender().MaxDatagramSize = value, + Throws.TypeOf()); + + /// An event that fits is sent exactly as the layout rendered it. + [Test] + public void AMessageThatFitsIsEncodedUnchanged() + { + UdpAppender appender = new() { Encoding = Encoding.UTF8 }; + + Assert.That(Encode(appender, "hello"), Is.EqualTo(Encoding.UTF8.GetBytes("hello"))); + } + + /// An event of exactly the limit still fits, so nothing is cut and nothing reported. + [Test] + public void AMessageOfExactlyTheLimitIsEncodedUnchanged() + { + UdpAppender appender = new() { Encoding = Encoding.ASCII, MaxDatagramSize = 512 }; + RecordingErrorHandler errorHandler = new(); + appender.ErrorHandler = errorHandler; + + byte[] datagram = Encode(appender, new string('a', 512)); + + Assert.That(datagram, Is.EqualTo(Encoding.ASCII.GetBytes(new string('a', 512)))); + Assert.That(errorHandler.Messages, Is.Empty); + } + + /// + /// An oversize datagram used to be rejected by the socket, which cost the whole event. + /// + [Test] + public void AnOversizeMessageIsTruncatedInsteadOfLost() + { + UdpAppender appender = new() { Encoding = Encoding.ASCII, MaxDatagramSize = 512 }; + RecordingErrorHandler errorHandler = new(); + appender.ErrorHandler = errorHandler; + + byte[] datagram = Encode(appender, new string('a', 1000)); + + Assert.That(datagram, Has.Length.EqualTo(512)); + Assert.That(Encoding.ASCII.GetString(datagram), Does.EndWith(Marker)); + Assert.That(errorHandler.Messages, Has.Count.EqualTo(1)); + } + + /// A cut inside a character would encode as the replacement character. + [Test] + public void TruncationDoesNotSplitAMultiByteCharacter() + { + UdpAppender appender = new() { Encoding = Encoding.UTF8, MaxDatagramSize = 513 }; + appender.ErrorHandler = new RecordingErrorHandler(); + + string decoded = Encoding.UTF8.GetString(Encode(appender, new string('\u00e4', 1000))); + + // 513 bytes hold the 14 byte marker and 249 of the two byte characters. + Assert.That(decoded, Is.EqualTo(new string('\u00e4', 249) + Marker)); + } + + /// The same for a cut between the two halves of a surrogate pair. + [Test] + public void TruncationDoesNotSplitASurrogatePair() + { + UdpAppender appender = new() { Encoding = Encoding.UTF8, MaxDatagramSize = 515 }; + appender.ErrorHandler = new RecordingErrorHandler(); + + string decoded = Encoding.UTF8.GetString(Encode(appender, string.Concat(Enumerable.Repeat("\U0001f600", 1000)))); + + Assert.That(decoded, Does.EndWith(Marker)); + Assert.That(decoded, !Contains.Substring("\ufffd").Using(StringComparison.Ordinal)); + } +} diff --git a/src/log4net.Tests/Config/RedactTest.cs b/src/log4net.Tests/Config/RedactTest.cs new file mode 100644 index 00000000..0f9774f7 --- /dev/null +++ b/src/log4net.Tests/Config/RedactTest.cs @@ -0,0 +1,134 @@ +#region Apache License +// +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to you under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +#endregion + +using System; +using System.Collections.Generic; +using System.Xml; +using log4net.Config; +using log4net.Repository; +using log4net.Util; +using NUnit.Framework; + +namespace log4net.Tests.Config; + +/// +/// Tests that the configurator keeps secrets out of its own messages. +/// +[TestFixture] +[NonParallelizable] +public sealed class RedactTest +{ + private const string Password = "Sup3rS3cretPw"; + private const string ConnectionString = "Server=db.example.invalid;User Id=sa;Password=" + Password; + + /// + /// Internal debugging is the documented first troubleshooting step, and it used to echo every + /// configured value, secrets included. + /// + [Test] + public void SecretValuesAreNotEchoedByInternalDebugging() + { + string reported = Configure($""" + + + + + + + + + + + """); + + Assert.That(reported, Does.Contain("Setting Property [Password]"), "internal debugging was off"); + Assert.That(reported, !Contains.Substring(Password).Using(StringComparison.Ordinal)); + // A value that is not a secret is still echoed, so the output stays useful. + Assert.That(reported, Does.Contain("dest@example.invalid")); + } + + /// A connection string keeps the server it names, so it can still be checked. + [Test] + public void AConnectionStringKeepsOnlyTheKeywordsThatNameTheServer() + { + string reported = Configure($""" + + + + + + + + + + """); + + Assert.That(reported, !Contains.Substring(Password).Using(StringComparison.Ordinal)); + Assert.That(reported, Does.Contain("db.example.invalid")); + } + + /// + /// The provider type and the lookup key are the first things read when configuration fails, so + /// neither may be hidden by the connection string rule. + /// + [Test] + public void AParameterOfTheConnectionFamilyThatIsNoSecretSurvives() + { + string reported = Configure(""" + + + + + + + + + + + """); + + Assert.That(reported, Does.Contain("System.Data.SqlClient.SqlConnection, System.Data")); + Assert.That(reported, Does.Contain("myDatabase")); + } + + /// Configures a repository from and returns what log4net reported. + private static string Configure(string xml) + { + List messages = []; + bool internalDebugging = LogLog.InternalDebugging; + try + { + LogLog.InternalDebugging = true; + LogLog.ExecuteWithoutEmittingInternalMessages(() => + { + using LogLog.LogReceivedAdapter _ = new(messages); + XmlDocument document = new(); + document.LoadXml(xml); + ILoggerRepository repository = LogManager.CreateRepository(Guid.NewGuid().ToString()); + XmlConfigurator.Configure(repository, (XmlElement)document.DocumentElement!); + }); + } + finally + { + LogLog.InternalDebugging = internalDebugging; + } + + return string.Join(Environment.NewLine, messages.ConvertAll(message => message.Message)); + } +} diff --git a/src/log4net/Appender/AdoNetAppender.cs b/src/log4net/Appender/AdoNetAppender.cs index 08b69726..ef4a445f 100644 --- a/src/log4net/Appender/AdoNetAppender.cs +++ b/src/log4net/Appender/AdoNetAppender.cs @@ -21,7 +21,6 @@ using System.Collections.Generic; using System.Configuration; using System.Data; -using System.Data.Common; using System.IO; using log4net.Util; @@ -816,75 +815,12 @@ private void InitializeDatabaseConnection() catch (Exception e) when (!e.IsFatal()) { // Sadly, your connection string is bad. - ErrorHandler.Error($"Could not open database connection [{RedactConnectionString(resolvedConnectionString)}]. Connection string context [{connectionStringContext}].", e); + ErrorHandler.Error($"Could not open database connection [{Redact.ConnectionString(resolvedConnectionString)}]. Connection string context [{connectionStringContext}].", e); Connection = null; } } - /// - /// Reduces a connection string to the keywords that identify the server, for a diagnostic message. - /// - /// The connection string to redact. - /// The other values replaced, or if it could not be parsed. - /// - /// - /// An allowlist, because hiding known secret keywords misses `Extended Properties`: it nests a - /// whole connection string that the parser returns as one opaque value. - /// - /// - private static string RedactConnectionString(string connectionString) - { - if (string.IsNullOrEmpty(connectionString)) - { - return connectionString; - } - - try - { - DbConnectionStringBuilder builder = new() { ConnectionString = connectionString }; - - List keys = []; - foreach (string key in builder.Keys) - { - keys.Add(key); - } - - foreach (string key in keys) - { - if (!DiagnosticKeywords.Contains(key)) - { - builder[key] = RedactedValue; - } - } - - return builder.ConnectionString; - } - catch (Exception e) when (!e.IsFatal()) - { - // The connection string could not be parsed - which is likely, given that it just failed - // to connect - so redact all of it rather than risk echoing a password. - LogLog.Debug(_declaringType, "Could not parse the connection string in order to redact it", e); - return RedactedValue; - } - } - - /// - /// Keywords whose values are kept: they name the server and account, not the credentials. - /// - private static readonly HashSet DiagnosticKeywords = new(StringComparer.OrdinalIgnoreCase) - { - "provider", "driver", "data source", "server", "address", "addr", "network address", - "initial catalog", "database", "port", "user id", "uid", "user", "username", - "integrated security", "trusted_connection", "encrypt", "timeout", "connect timeout", - "connection timeout", "application name", "workstation id", "pooling", - }; - - /// - /// Stands in for a value withheld from a diagnostic message. - /// - private const string RedactedValue = "*****"; - /// /// Cleanup the existing connection. /// diff --git a/src/log4net/Appender/RemoteSyslogAppender.cs b/src/log4net/Appender/RemoteSyslogAppender.cs index 00a55dab..128b9127 100644 --- a/src/log4net/Appender/RemoteSyslogAppender.cs +++ b/src/log4net/Appender/RemoteSyslogAppender.cs @@ -367,7 +367,7 @@ protected override void Append(LoggingEvent loggingEvent) AppendMessage(message, ref i, builder); // Grab as a byte array - byte[] buffer = Encoding.GetBytes(builder.ToString()); + byte[] buffer = GetDatagramBytes(builder.ToString()); // A full queue is reported by the sender itself. _sender?.TryEnqueue(buffer, EnqueueTimeoutMillis); diff --git a/src/log4net/Appender/SmtpPickupDirAppender.cs b/src/log4net/Appender/SmtpPickupDirAppender.cs index 86c17c79..24fd82fd 100644 --- a/src/log4net/Appender/SmtpPickupDirAppender.cs +++ b/src/log4net/Appender/SmtpPickupDirAppender.cs @@ -167,8 +167,9 @@ protected override void SendBuffer(LoggingEvent[] events) writer.Write(footer); } + // No SMTP DATA terminator: the pickup service reads the file to its end, and ours only + // made a logged line that is only a dot look like one. writer.WriteLine(); - writer.WriteLine("."); } } catch (Exception e) when (!e.IsFatal()) diff --git a/src/log4net/Appender/UdpAppender.cs b/src/log4net/Appender/UdpAppender.cs index 2e0dc957..d0e75577 100644 --- a/src/log4net/Appender/UdpAppender.cs +++ b/src/log4net/Appender/UdpAppender.cs @@ -257,6 +257,30 @@ public int LocalPort /// public Encoding Encoding { get; set; } = Encoding.Default; + /// + /// Gets or sets the largest datagram this appender sends, in bytes. + /// + /// A value from 512 to 65507, the maximum UDP payload of an IPv4 packet. Default 65507. + /// + /// The socket rejects a larger datagram, so a longer event is truncated to fit instead of lost. + /// Lower the value to match a receiver that accepts less. IPv6 would carry 20 bytes more, which + /// is not worth a second limit. + /// + /// The value specified is outside 512 to 65507. + public int MaxDatagramSize + { + get => _maxDatagramSize; + set + { + if (value is < MinDatagramSize or > MaxIPv4DatagramSize) + { + throw SystemInfo.CreateArgumentOutOfRangeException(nameof(value), value, + $"The value specified is less than {MinDatagramSize} or greater than {MaxIPv4DatagramSize}."); + } + _maxDatagramSize = value; + } + } + /// /// Gets or sets the underlying . /// @@ -348,7 +372,7 @@ protected override void Append(LoggingEvent loggingEvent) try { - byte[] buffer = Encoding.GetBytes(RenderLoggingEvent(loggingEvent).ToCharArray()); + byte[] buffer = GetDatagramBytes(RenderLoggingEvent(loggingEvent)); Client.Send(buffer, buffer.Length, RemoteEndPoint); } catch (Exception e) when (!e.IsFatal()) @@ -359,6 +383,49 @@ protected override void Append(LoggingEvent loggingEvent) } } + /// + /// Encodes a rendered event for one datagram, truncated to . + /// + /// The rendered event. + /// The bytes to send, ending in ...[truncated] when the event did not fit. + /// + /// An oversize datagram is rejected by the socket, which used to cost the whole event. + /// + private protected byte[] GetDatagramBytes(string message) + { + byte[] datagram = Encoding.GetBytes(message.EnsureNotNull()); + if (datagram.Length <= MaxDatagramSize) + { + return datagram; + } + + byte[] result = GetTruncatedBytes(message, datagram); + + ErrorHandler.Error( + $"Truncated a logging event from {datagram.Length} to {result.Length} bytes for {RemoteAddress} on port {RemotePort}."); + return result; + } + + /// Encodes as much of as fits, ending in the marker. + /// The rendered event. + /// The whole encoded event, reused as the encoder's output. + /// The bytes to send, of at most length. + private byte[] GetTruncatedBytes(string message, byte[] datagram) + { + int markerLength = Encoding.GetByteCount(TruncationMarker); + + // Convert fills the budget without splitting a character, and unflushed it holds back a + // trailing high surrogate rather than encoding it as the replacement character. It writes + // into the oversize encoding, which is the one buffer already long enough. + Encoding.GetEncoder().Convert(message.ToCharArray(), 0, message.Length, + datagram, 0, MaxDatagramSize - markerLength, flush: false, out _, out int written, out _); + + byte[] result = new byte[written + markerLength]; + Buffer.BlockCopy(datagram, 0, result, 0, written); + Encoding.GetBytes(TruncationMarker, 0, TruncationMarker.Length, result, written); + return result; + } + /// /// This appender requires a to be set. /// @@ -419,4 +486,24 @@ protected virtual void InitializeClientConnection() /// The TCP port number from which the will communicate. /// private int _localPort; + + /// + /// The largest datagram this appender sends. + /// + private int _maxDatagramSize = MaxIPv4DatagramSize; + + /// + /// The maximum UDP payload of an IPv4 packet. + /// + private const int MaxIPv4DatagramSize = 65507; + + /// + /// The smallest datagram size an operator may configure. + /// + private const int MinDatagramSize = 512; + + /// + /// Marks a datagram that could not carry the whole event. + /// + private const string TruncationMarker = "...[truncated]"; } \ No newline at end of file diff --git a/src/log4net/Repository/Hierarchy/XmlHierarchyConfigurator.cs b/src/log4net/Repository/Hierarchy/XmlHierarchyConfigurator.cs index 8e868b07..37821978 100644 --- a/src/log4net/Repository/Hierarchy/XmlHierarchyConfigurator.cs +++ b/src/log4net/Repository/Hierarchy/XmlHierarchyConfigurator.cs @@ -770,7 +770,7 @@ protected void SetParameter(XmlElement element, object target) if (propInfo is not null) { // Got a converted result - LogLog.Debug(_declaringType, $"Setting Property [{propInfo.Name}] to {convertedValue.GetType().Name} value [{convertedValue}]"); + LogLog.Debug(_declaringType, $"Setting Property [{propInfo.Name}] to {convertedValue.GetType().Name} value [{Redact.Value(name, convertedValue)}]"); try { @@ -779,13 +779,13 @@ protected void SetParameter(XmlElement element, object target) } catch (TargetInvocationException targetInvocationEx) { - LogLog.Error(_declaringType, $"Failed to set parameter [{propInfo.Name}] on object [{target}] using value [{convertedValue}]", targetInvocationEx.InnerException); + LogLog.Error(_declaringType, $"Failed to set parameter [{propInfo.Name}] on object [{target}] using value [{Redact.Value(name, convertedValue)}]", targetInvocationEx.InnerException); } } else { // Got a converted result - LogLog.Debug(_declaringType, $"Setting Collection Property [{methInfo!.Name}] to {convertedValue.GetType().Name} value [{convertedValue}]"); + LogLog.Debug(_declaringType, $"Setting Collection Property [{methInfo!.Name}] to {convertedValue.GetType().Name} value [{Redact.Value(name, convertedValue)}]"); try { @@ -794,13 +794,13 @@ protected void SetParameter(XmlElement element, object target) } catch (TargetInvocationException targetInvocationEx) { - LogLog.Error(_declaringType, $"Failed to set parameter [{name}] on object [{target}] using value [{convertedValue}]", targetInvocationEx.InnerException); + LogLog.Error(_declaringType, $"Failed to set parameter [{name}] on object [{target}] using value [{Redact.Value(name, convertedValue)}]", targetInvocationEx.InnerException); } } } else { - LogLog.Warn(_declaringType, $"Unable to set property [{name}] on object [{target}] using value [{propertyValue}] (with acceptable conversion types)"); + LogLog.Warn(_declaringType, $"Unable to set property [{name}] on object [{target}] using value [{Redact.Value(name, propertyValue)}] (with acceptable conversion types)"); } } else @@ -837,7 +837,7 @@ protected void SetParameter(XmlElement element, object target) if (propInfo is not null) { // Got a converted result - LogLog.Debug(_declaringType, $"Setting Property [{propInfo.Name}] to object [{createdObject}]"); + LogLog.Debug(_declaringType, $"Setting Property [{propInfo.Name}] to object [{Redact.Value(name, createdObject)}]"); try { @@ -846,13 +846,13 @@ protected void SetParameter(XmlElement element, object target) } catch (TargetInvocationException targetInvocationEx) { - LogLog.Error(_declaringType, $"Failed to set parameter [{propInfo.Name}] on object [{target}] using value [{createdObject}]", targetInvocationEx.InnerException); + LogLog.Error(_declaringType, $"Failed to set parameter [{propInfo.Name}] on object [{target}] using value [{Redact.Value(name, createdObject)}]", targetInvocationEx.InnerException); } } else { // Got a converted result - LogLog.Debug(_declaringType, $"Setting Collection Property [{methInfo!.Name}] to object [{createdObject}]"); + LogLog.Debug(_declaringType, $"Setting Collection Property [{methInfo!.Name}] to object [{Redact.Value(name, createdObject)}]"); try { @@ -861,7 +861,7 @@ protected void SetParameter(XmlElement element, object target) } catch (TargetInvocationException targetInvocationEx) { - LogLog.Error(_declaringType, $"Failed to set parameter [{methInfo.Name}] on object [{target}] using value [{createdObject}]", targetInvocationEx.InnerException); + LogLog.Error(_declaringType, $"Failed to set parameter [{methInfo.Name}] on object [{target}] using value [{Redact.Value(name, createdObject)}]", targetInvocationEx.InnerException); } } } diff --git a/src/log4net/Util/Redact.cs b/src/log4net/Util/Redact.cs new file mode 100644 index 00000000..b3e7f8ef --- /dev/null +++ b/src/log4net/Util/Redact.cs @@ -0,0 +1,131 @@ +#region Apache License +// +// Licensed to the Apache Software Foundation (ASF) under one or more +// contributor license agreements. See the NOTICE file distributed with +// this work for additional information regarding copyright ownership. +// The ASF licenses this file to you under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// +#endregion + +using System; +using System.Collections.Generic; +using System.Data.Common; + +namespace log4net.Util; + +/// +/// Keeps secrets out of the messages log4net writes about its own configuration. +/// +internal static class Redact +{ + /// + /// Reduces a connection string to the keywords that identify the server. + /// + /// The connection string to redact. + /// The other values replaced, or if it could not be parsed. + /// + /// + /// An allowlist, because hiding known secret keywords misses `Extended Properties`: it nests a + /// whole connection string that the parser returns as one opaque value. + /// + /// + internal static string ConnectionString(string connectionString) + { + if (string.IsNullOrEmpty(connectionString)) + { + return connectionString; + } + + try + { + DbConnectionStringBuilder builder = new() { ConnectionString = connectionString }; + + List keys = []; + foreach (string key in builder.Keys) + { + keys.Add(key); + } + + foreach (string key in keys) + { + if (!DiagnosticKeywords.Contains(key)) + { + builder[key] = RedactedValue; + } + } + + return builder.ConnectionString; + } + catch (Exception e) when (!e.IsFatal()) + { + // The connection string could not be parsed - which is likely, given that it just failed + // to connect - so redact all of it rather than risk echoing a password. + LogLog.Debug(_declaringType, "Could not parse the connection string in order to redact it", e); + return RedactedValue; + } + } + + /// + /// Hides a configured value whose parameter name says it is a secret. + /// + /// The configured parameter name. + /// The value about to be logged. + /// The value, , or a redacted connection string. + internal static object? Value(string name, object? value) + { + if (value is null) + { + return null; + } + + // EndsWith, not Contains: ConnectionType and ConnectionStringName are diagnostics, not secrets. + if (name.EndsWith("connectionstring", StringComparison.OrdinalIgnoreCase)) + { + return ConnectionString(value.ToString() ?? string.Empty); + } + + foreach (string secret in SecretNames) + { + if (name.IndexOf(secret, StringComparison.OrdinalIgnoreCase) >= 0) + { + return RedactedValue; + } + } + + return value; + } + + /// + /// Keywords whose values are kept: they name the server and account, not the credentials. + /// + private static readonly HashSet DiagnosticKeywords = new(StringComparer.OrdinalIgnoreCase) + { + "provider", "driver", "data source", "server", "address", "addr", "network address", + "initial catalog", "database", "port", "user id", "uid", "user", "username", + "integrated security", "trusted_connection", "encrypt", "timeout", "connect timeout", + "connection timeout", "application name", "workstation id", "pooling", + }; + + /// + /// Parameter name fragments that mark a value as a secret. + /// + private static readonly string[] SecretNames = + ["password", "pwd", "passphrase", "secret", "token", "credential", "apikey"]; + + /// + /// Stands in for a value withheld from a diagnostic message. + /// + internal const string RedactedValue = "*****"; + + private static readonly Type _declaringType = typeof(Redact); +} diff --git a/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/remotesyslogappender.adoc b/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/remotesyslogappender.adoc index 9fef8942..b35d314b 100644 --- a/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/remotesyslogappender.adoc +++ b/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/remotesyslogappender.adoc @@ -44,6 +44,7 @@ You can also specify: * Identity (default: application name) * SendQueueSize (default: 500), how many datagrams may wait to be sent * EnqueueTimeoutMillis (default: 5000), how long a logging call waits for room in a full queue +* MaxDatagramSize (default: 65507), the largest datagram sent, inherited from the xref:./udpappender.adoc[] RFC 3164 allows only the visible ASCII characters and space in the message, so anything else is written as a `\uXXXX` escape rather than dropped: a message in a non-Latin script reaches the diff --git a/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/smtppickupdirappender.adoc b/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/smtppickupdirappender.adoc index 742e22d9..a3a57794 100644 --- a/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/smtppickupdirappender.adoc +++ b/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/smtppickupdirappender.adoc @@ -27,6 +27,10 @@ It is therefore unaffected by the deprecation of `System.Net.Mail.SmtpClient`, a The `PickupDir` parameter is a path that must exist and the code executing the appender must have permission to create new files and write to them in this directory. The path is relative to the application's base directory (AppDomain.BaseDirectory). +Each file is a plain RFC 822 message, read to its end by the pickup service, and carries no SMTP +DATA terminator. Body lines are written verbatim and are not dot stuffed, so an agent that expects +SMTP DATA framing is not a supported consumer. + The following example shows how to configure the SmtpPickupDirAppender to deliver log events via SMTP email. The `To`, `From`, `Subject` and `PickupDir` are required parameters. This example shows how to deliver only significant events. diff --git a/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/udpappender.adoc b/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/udpappender.adoc index 0cac823d..d19a0dca 100644 --- a/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/udpappender.adoc +++ b/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/udpappender.adoc @@ -33,4 +33,10 @@ The following example shows how to configure the `UdpAppender` to send events to ----- \ No newline at end of file +---- + +An event that does not fit one datagram is truncated and marked with `...[truncated]` rather than +dropped: the socket rejects a payload above `MaxDatagramSize` (default 65507, the maximum UDP +payload of an IPv4 packet), and that used to cost the whole event. Lower `MaxDatagramSize`, down to +512, to match a receiver that accepts less. It cannot be raised: an IPv6 packet would carry 20 bytes +more, which is not worth a second limit. diff --git a/src/site/antora/modules/ROOT/pages/manual/faq.adoc b/src/site/antora/modules/ROOT/pages/manual/faq.adoc index fc318b57..98c11d0f 100644 --- a/src/site/antora/modules/ROOT/pages/manual/faq.adoc +++ b/src/site/antora/modules/ROOT/pages/manual/faq.adoc @@ -298,6 +298,8 @@ Set `log4net.Util.LogLog.InternalDebugging` to `true` in code as early as possib Internal debug messages are written to the console and `System.Diagnostics.Trace`. If there's no console, messages are lost unless redirected. +A configured value whose name reads as a secret, such as `password` or `connectionString`, is +replaced with `*****` in these messages, so the property is still named but the value is not shown. You can capture these messages using a utility like DebugView from http://www.sysinternals.com[Sysinternals]. To redirect debug messages to a file, add a trace listener in the config file: