fix(diagnostics): redact WriteSecuredBulk/WriteSecured2Bulk payloads (#193)

Adds both secured bulk kinds to GatewayLogRedactor's credential-bearing set,
plus a contract cross-check test that walks every MxCommand payload descriptor
and fails if a secured or password-bearing command kind is not redacted.
This commit is contained in:
Joseph Doherty
2026-11-06 15:45:50 -05:00
parent 5a235776b6
commit 9354d69bf9
3 changed files with 91 additions and 1 deletions
+3 -1
View File
@@ -71,6 +71,8 @@ private static readonly HashSet<string> SensitiveCommandMethods = new(StringComp
"AuthenticateUser", "AuthenticateUser",
"WriteSecured", "WriteSecured",
"WriteSecured2", "WriteSecured2",
"WriteSecuredBulk",
"WriteSecured2Bulk",
"AcknowledgeAlarm", "AcknowledgeAlarm",
"AcknowledgeAlarmByName" "AcknowledgeAlarmByName"
}; };
@@ -82,7 +84,7 @@ public static bool IsCredentialBearingCommand(string? commandMethod)
} }
``` ```
The names match the MXAccess command list in `gateway.md` exactly. `Write` and `Write2` are not in the set because their payloads are tag values, not credentials, and are governed by the `valueLoggingEnabled` flag described below. `AcknowledgeAlarm` and `AcknowledgeAlarmByName` are in the set because their commands carry the operator's `operator_password`. Independently of the method set, the name-based seam redacts any property whose name contains `password`, so a credential field added to a payload later is redacted without a code change. The names match the MXAccess command list in `gateway.md` exactly. `Write` and `Write2` are not in the set because their payloads are tag values, not credentials, and are governed by the `valueLoggingEnabled` flag described below. `WriteSecured`, `WriteSecured2`, `WriteSecuredBulk` and `WriteSecured2Bulk` are in the set because every secured write — single-item or bulk — carries a credential-sensitive value. `AcknowledgeAlarm` and `AcknowledgeAlarmByName` are in the set because their commands carry the operator's `operator_password`. `GatewayLogRedactorTests` walks the `MxCommand` payload descriptors and fails if a secured or password-bearing command kind is missing from the set. Independently of the method set, the name-based seam redacts any property whose name contains `password`, so a credential field added to a payload later is redacted without a code change.
### API key redaction ### API key redaction
@@ -11,8 +11,11 @@ public static class GatewayLogRedactor
private static readonly HashSet<string> SensitiveCommandMethods = new(StringComparer.OrdinalIgnoreCase) private static readonly HashSet<string> SensitiveCommandMethods = new(StringComparer.OrdinalIgnoreCase)
{ {
"AuthenticateUser", "AuthenticateUser",
// Carry a credential-sensitive write value (single item and bulk).
"WriteSecured", "WriteSecured",
"WriteSecured2", "WriteSecured2",
"WriteSecuredBulk",
"WriteSecured2Bulk",
// Carry operator_password. // Carry operator_password.
"AcknowledgeAlarm", "AcknowledgeAlarm",
"AcknowledgeAlarmByName" "AcknowledgeAlarmByName"
@@ -1,3 +1,5 @@
using Google.Protobuf.Reflection;
using ZB.MOM.WW.MxGateway.Contracts.Proto;
using ZB.MOM.WW.MxGateway.Server.Diagnostics; using ZB.MOM.WW.MxGateway.Server.Diagnostics;
namespace ZB.MOM.WW.MxGateway.Tests.Diagnostics; namespace ZB.MOM.WW.MxGateway.Tests.Diagnostics;
@@ -118,6 +120,8 @@ public sealed class GatewayLogRedactorTests
[InlineData("AuthenticateUser")] [InlineData("AuthenticateUser")]
[InlineData("WriteSecured")] [InlineData("WriteSecured")]
[InlineData("WriteSecured2")] [InlineData("WriteSecured2")]
[InlineData("WriteSecuredBulk")]
[InlineData("WriteSecured2Bulk")]
[InlineData("AcknowledgeAlarm")] [InlineData("AcknowledgeAlarm")]
[InlineData("AcknowledgeAlarmByName")] [InlineData("AcknowledgeAlarmByName")]
public void IsCredentialBearingCommand_IdentifiesSensitiveMxAccessCommands(string commandMethod) public void IsCredentialBearingCommand_IdentifiesSensitiveMxAccessCommands(string commandMethod)
@@ -146,6 +150,87 @@ public sealed class GatewayLogRedactorTests
Assert.Equal("[redacted]", redacted); Assert.Equal("[redacted]", redacted);
} }
/// <summary>
/// Verifies that the secured bulk writes are redacted even with value logging enabled. These
/// carry the same credential-sensitive value as the single-item secured writes (#193).
/// </summary>
/// <param name="kind">The secured bulk command kind.</param>
[Theory]
[InlineData(MxCommandKind.WriteSecuredBulk)]
[InlineData(MxCommandKind.WriteSecured2Bulk)]
public void RedactCommandValue_RedactsSecuredBulkWriteEvenWhenValueLoggingIsEnabled(MxCommandKind kind)
{
// WorkerClient logs the method as MxCommandKind.ToString(); exercise that exact spelling.
object? redacted = GatewayLogRedactor.RedactCommandValue(
kind.ToString(),
"credential-bearing-bulk-value",
valueLoggingEnabled: true);
Assert.Equal("[redacted]", redacted);
}
/// <summary>
/// Cross-checks the redactor against the contract: every <see cref="MxCommand"/> payload that
/// carries a password field, or is a secured write, must map to a credential-bearing command
/// kind. A new credential-bearing command added to the proto without a redactor entry fails here.
/// </summary>
[Fact]
public void IsCredentialBearingCommand_CoversEveryCredentialBearingPayloadInTheContract()
{
List<string> sensitivePayloads = [];
foreach (FieldDescriptor field in MxCommand.Descriptor.Oneofs.Single().Fields)
{
if (field.FieldType != FieldType.Message)
{
continue;
}
MessageDescriptor payload = field.MessageType;
if (payload.Name.Contains("Secured", StringComparison.Ordinal)
|| CarriesPassword(payload, []))
{
sensitivePayloads.Add(payload.Name);
}
}
// Positive control: the walk must find the known credential carriers, or it proves nothing.
Assert.Contains("AuthenticateUserCommand", sensitivePayloads);
Assert.Contains("WriteSecuredBulkCommand", sensitivePayloads);
Assert.Contains("AcknowledgeAlarmByNameCommand", sensitivePayloads);
foreach (string payloadName in sensitivePayloads)
{
string kindName = payloadName[..^"Command".Length];
Assert.True(Enum.TryParse(kindName, out MxCommandKind _), $"No MxCommandKind named {kindName}.");
Assert.True(
GatewayLogRedactor.IsCredentialBearingCommand(kindName),
$"{kindName} carries credential material but is not redacted.");
}
}
private static bool CarriesPassword(MessageDescriptor message, HashSet<string> visited)
{
if (!visited.Add(message.FullName))
{
return false;
}
foreach (FieldDescriptor field in message.Fields.InDeclarationOrder())
{
if (field.Name.Contains("password", StringComparison.OrdinalIgnoreCase))
{
return true;
}
if (field.FieldType == FieldType.Message && CarriesPassword(field.MessageType, visited))
{
return true;
}
}
return false;
}
/// <summary>Verifies that RedactCommandValue allows non-sensitive values only when value logging is enabled.</summary> /// <summary>Verifies that RedactCommandValue allows non-sensitive values only when value logging is enabled.</summary>
[Fact] [Fact]
public void RedactCommandValue_AllowsNonSensitiveValueOnlyWhenValueLoggingIsEnabled() public void RedactCommandValue_AllowsNonSensitiveValueOnlyWhenValueLoggingIsEnabled()