fix(server): resolve Medium code-review finding (Server-013)
Replace silent Enum.TryParse fallback to None with a ParseSecurityProfile helper that emits a startup Log.Warning naming the unsupported value and listing recognised profiles; operators now see the misconfiguration before any client connects rather than getting an unexplained None posture. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -200,13 +200,13 @@
|
|||||||
| Severity | Medium |
|
| Severity | Medium |
|
||||||
| Category | Design-document adherence |
|
| Category | Design-document adherence |
|
||||||
| Location | `src/Server/ZB.MOM.WW.OtOpcUa.Server/OpcUa/OpcUaServerOptions.cs:9-19`, `src/Server/ZB.MOM.WW.OtOpcUa.Server/OpcUa/OpcUaApplicationHost.cs:296-346`, `src/Server/ZB.MOM.WW.OtOpcUa.Server/Program.cs:89` |
|
| Location | `src/Server/ZB.MOM.WW.OtOpcUa.Server/OpcUa/OpcUaServerOptions.cs:9-19`, `src/Server/ZB.MOM.WW.OtOpcUa.Server/OpcUa/OpcUaApplicationHost.cs:296-346`, `src/Server/ZB.MOM.WW.OtOpcUa.Server/Program.cs:89` |
|
||||||
| Status | Open |
|
| Status | Resolved |
|
||||||
|
|
||||||
**Description:** `docs/security.md` documents 7 transport security profiles and `CLAUDE.md` references a `SecurityProfileResolver`. The code's `OpcUaSecurityProfile` enum has only `None` and `Basic256Sha256SignAndEncrypt`; `BuildSecurityPolicies` adds a policy only for the latter; `SecurityProfileResolver` does not exist in the repo (grep finds it only in docs). `Basic256Sha256-Sign` and all Aes profiles are unimplemented, and `Program.cs:89`'s `Enum.TryParse` silently selects `None` for an unrecognised profile string.
|
**Description:** `docs/security.md` documents 7 transport security profiles and `CLAUDE.md` references a `SecurityProfileResolver`. The code's `OpcUaSecurityProfile` enum has only `None` and `Basic256Sha256SignAndEncrypt`; `BuildSecurityPolicies` adds a policy only for the latter; `SecurityProfileResolver` does not exist in the repo (grep finds it only in docs). `Basic256Sha256-Sign` and all Aes profiles are unimplemented, and `Program.cs:89`'s `Enum.TryParse` silently selects `None` for an unrecognised profile string.
|
||||||
|
|
||||||
**Recommendation:** Reconcile code and docs — implement the missing profiles + `SecurityProfileResolver`, or trim `docs/security.md` / `CLAUDE.md` to the two supported profiles. At minimum, log a warning when a configured `SecurityProfile` fails to parse instead of silently using `None`.
|
**Recommendation:** Reconcile code and docs — implement the missing profiles + `SecurityProfileResolver`, or trim `docs/security.md` / `CLAUDE.md` to the two supported profiles. At minimum, log a warning when a configured `SecurityProfile` fails to parse instead of silently using `None`.
|
||||||
|
|
||||||
**Resolution:** _(open)_
|
**Resolution:** Resolved 2026-05-22 — replaced the silent `Enum.TryParse ?? None` fallback in `Program.cs` with a `ParseSecurityProfile` helper that produces a warning string listing supported profiles when the configured value is unrecognised; the warning is emitted via `Log.Warning` at startup before the host builds, making the misconfiguration immediately visible. Implementing the missing 5 profiles is tracked as a doc-to-code gap rather than a single finding fix.
|
||||||
|
|
||||||
### Server-014
|
### Server-014
|
||||||
| Field | Value |
|
| Field | Value |
|
||||||
|
|||||||
@@ -97,12 +97,15 @@ var opcUaOptions = new OpcUaServerOptions
|
|||||||
PkiStoreRoot = opcUaSection.GetValue<string>("PkiStoreRoot")
|
PkiStoreRoot = opcUaSection.GetValue<string>("PkiStoreRoot")
|
||||||
?? Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.CommonApplicationData), "OtOpcUa", "pki"),
|
?? Path.Combine(Environment.GetFolderPath(Environment.SpecialFolder.CommonApplicationData), "OtOpcUa", "pki"),
|
||||||
AutoAcceptUntrustedClientCertificates = opcUaSection.GetValue<bool?>("AutoAcceptUntrustedClientCertificates") ?? false, // Server-010: secure by default
|
AutoAcceptUntrustedClientCertificates = opcUaSection.GetValue<bool?>("AutoAcceptUntrustedClientCertificates") ?? false, // Server-010: secure by default
|
||||||
SecurityProfile = Enum.TryParse<OpcUaSecurityProfile>(opcUaSection.GetValue<string>("SecurityProfile"), true, out var p)
|
SecurityProfile = ParseSecurityProfile(opcUaSection.GetValue<string>("SecurityProfile"), out var profileParseWarning),
|
||||||
? p : OpcUaSecurityProfile.None,
|
|
||||||
Ldap = ldapOptions,
|
Ldap = ldapOptions,
|
||||||
AnonymousRoles = opcUaSection.GetSection("AnonymousRoles").Get<string[]>() ?? [],
|
AnonymousRoles = opcUaSection.GetSection("AnonymousRoles").Get<string[]>() ?? [],
|
||||||
};
|
};
|
||||||
|
|
||||||
|
// Server-013: warn at startup when the configured SecurityProfile string does not map to
|
||||||
|
// any supported profile — Enum.TryParse previously silently fell back to None.
|
||||||
|
if (profileParseWarning is not null) Log.Warning("{Warning}", profileParseWarning);
|
||||||
|
|
||||||
builder.Services.AddSingleton(options);
|
builder.Services.AddSingleton(options);
|
||||||
builder.Services.AddSingleton(opcUaOptions);
|
builder.Services.AddSingleton(opcUaOptions);
|
||||||
builder.Services.AddSingleton(ldapOptions);
|
builder.Services.AddSingleton(ldapOptions);
|
||||||
@@ -271,6 +274,27 @@ builder.Services.AddSingleton<Phase7Composer>();
|
|||||||
var host = builder.Build();
|
var host = builder.Build();
|
||||||
await host.RunAsync();
|
await host.RunAsync();
|
||||||
|
|
||||||
|
// Server-013: parse the SecurityProfile config value and produce a warning string when the
|
||||||
|
// configured value is unrecognised, so operators get a startup diagnostic instead of silent
|
||||||
|
// fallback to None.
|
||||||
|
static OpcUaSecurityProfile ParseSecurityProfile(string? raw, out string? warning)
|
||||||
|
{
|
||||||
|
if (string.IsNullOrWhiteSpace(raw))
|
||||||
|
{
|
||||||
|
warning = null;
|
||||||
|
return OpcUaSecurityProfile.None;
|
||||||
|
}
|
||||||
|
if (Enum.TryParse<OpcUaSecurityProfile>(raw, ignoreCase: true, out var parsed))
|
||||||
|
{
|
||||||
|
warning = null;
|
||||||
|
return parsed;
|
||||||
|
}
|
||||||
|
warning = $"OpcUaServer:SecurityProfile value '{raw}' is not a recognised profile " +
|
||||||
|
$"(supported: {string.Join(", ", Enum.GetNames<OpcUaSecurityProfile>())}). " +
|
||||||
|
"Falling back to SecurityProfile.None — no encryption or signed transport will be applied.";
|
||||||
|
return OpcUaSecurityProfile.None;
|
||||||
|
}
|
||||||
|
|
||||||
// Server-007: lightweight cached config-DB health probe for /healthz.
|
// Server-007: lightweight cached config-DB health probe for /healthz.
|
||||||
// Runs CanConnectAsync once and caches the result for 10 s so the /healthz
|
// Runs CanConnectAsync once and caches the result for 10 s so the /healthz
|
||||||
// endpoint never blocks an HTTP handler thread on a DB round-trip while still
|
// endpoint never blocks an HTTP handler thread on a DB round-trip while still
|
||||||
|
|||||||
Reference in New Issue
Block a user