diff --git a/code-reviews/Server/findings.md b/code-reviews/Server/findings.md index daf5860..1d12bbf 100644 --- a/code-reviews/Server/findings.md +++ b/code-reviews/Server/findings.md @@ -7,7 +7,7 @@ | Review date | 2026-05-22 | | Commit reviewed | `76d35d1` | | Status | Reviewed | -| Open findings | 14 | +| Open findings | 12 | ## Checklist coverage @@ -46,13 +46,13 @@ | Severity | High | | Category | Correctness & logic bugs | | Location | `src/Server/ZB.MOM.WW.OtOpcUa.Server/Security/AuthorizationGate.cs:60-63` | -| Status | Open | +| Status | Resolved | **Description:** `IsAllowed` does `if (decision.IsAllowed) return true; return !_strictMode;`. When a session carries resolved LDAP groups and the evaluator returns an explicit deny, lax mode (default) overrides it to `true`. The lax fallback is intended only for sessions lacking LDAP groups / missing tries, but here it also nullifies authored `NodeAcl` deny rules for fully-resolved sessions. Per-tag deny ACLs do nothing until `StrictMode` is on. **Recommendation:** Distinguish "indeterminate / no grant" from "explicit deny." Fall through to `!_strictMode` only when indeterminate; an explicit deny returns `false` regardless of mode. Extend `AuthorizeDecision` with an `IsExplicitDeny` flag if needed. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — `AuthorizationGate.IsAllowed` now switches on the evaluator's `AuthorizationVerdict`: `Allow` returns true, `Denied` (explicit deny rule matched) returns false in both strict and lax mode, and only the indeterminate `NotGranted` case falls through to `!_strictMode`. The existing `AuthorizationVerdict.Denied` tri-state member is now honoured rather than collapsed into the lax fallback. Regression tests `ExplicitDeny_LaxMode_Denies` / `ExplicitDeny_StrictMode_Denies` / `NotGranted_LaxMode_Allows` / `NotGranted_StrictMode_Denies` in `AuthorizationGateTests` cover all four verdict×mode combinations via a fixed-verdict evaluator stub. ### Server-003 | Field | Value | @@ -144,13 +144,13 @@ | Severity | High | | Category | Security | | Location | `src/Server/ZB.MOM.WW.OtOpcUa.Server/Security/LdapOptions.cs:44`, `src/Server/ZB.MOM.WW.OtOpcUa.Server/Program.cs:74` | -| Status | Open | +| Status | Resolved | **Description:** `AllowInsecureLdap` defaults to `true` (and `Program.cs` reads `?? true`); `UseTls` defaults to `false`. Out of the box, usernames and plaintext passwords are bound to LDAP over an unencrypted socket. A production deployment enabling LDAP without explicitly setting `AllowInsecureLdap=false` ships credentials in clear text on the server→LDAP hop. **Recommendation:** Default `AllowInsecureLdap` to `false` in both the property initializer and the `Program.cs` fallback. Log a startup warning when LDAP is enabled with `UseTls=false && AllowInsecureLdap=true`. -**Resolution:** _(open)_ +**Resolution:** Resolved 2026-05-22 — `LdapOptions.AllowInsecureLdap` now defaults to `false` (secure-by-default) and `Program.cs`'s config fallback reads `?? false`. `Program.cs` logs a startup `Log.Warning` when LDAP is enabled with `UseTls=false && AllowInsecureLdap=true`, flagging the clear-text credential hop. Regression tests in `LdapOptionsTests` assert the new secure defaults. ### Server-010 | Field | Value | diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Server/Program.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Server/Program.cs index 2f87dcf..f9bcb56 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Server/Program.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Server/Program.cs @@ -71,13 +71,24 @@ var ldapOptions = new LdapOptions Server = ldapSection.GetValue("Server") ?? "localhost", Port = ldapSection.GetValue("Port") ?? 3893, UseTls = ldapSection.GetValue("UseTls") ?? false, - AllowInsecureLdap = ldapSection.GetValue("AllowInsecureLdap") ?? true, + AllowInsecureLdap = ldapSection.GetValue("AllowInsecureLdap") ?? false, SearchBase = ldapSection.GetValue("SearchBase") ?? "dc=lmxopcua,dc=local", ServiceAccountDn = ldapSection.GetValue("ServiceAccountDn") ?? string.Empty, ServiceAccountPassword = ldapSection.GetValue("ServiceAccountPassword") ?? string.Empty, GroupToRole = ldapSection.GetSection("GroupToRole").Get>() ?? new(StringComparer.OrdinalIgnoreCase), }; +// Security: an LDAP bind without TLS sends usernames + plaintext passwords in clear +// text on the server→LDAP hop. AllowInsecureLdap is a dev-only escape hatch; warn +// loudly when a deployment has enabled LDAP and opted into the insecure path. +if (ldapOptions.Enabled && !ldapOptions.UseTls && ldapOptions.AllowInsecureLdap) +{ + Log.Warning( + "LDAP authentication is enabled with UseTls=false and AllowInsecureLdap=true — " + + "credentials are sent in clear text on the server→LDAP hop. Set Ldap:UseTls=true " + + "(LDAPS) for production deployments."); +} + var opcUaOptions = new OpcUaServerOptions { EndpointUrl = opcUaSection.GetValue("EndpointUrl") ?? "opc.tcp://0.0.0.0:4840/OtOpcUa", diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Server/Security/AuthorizationGate.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Server/Security/AuthorizationGate.cs index adaed4a..aebeba0 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Server/Security/AuthorizationGate.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Server/Security/AuthorizationGate.cs @@ -22,6 +22,12 @@ namespace ZB.MOM.WW.OtOpcUa.Server.Security; /// StrictMode = false, missing cluster tries OR sessions without resolved /// LDAP groups get true so existing deployments keep working while ACLs are /// populated. Flip to strict via Authorization:StrictMode = true in production. +/// +/// The lax-mode fallback only covers the indeterminate case +/// ( — no grant matched). An explicit +/// verdict from an authored deny rule is a +/// definite decision and is honoured in both strict and lax mode — lax mode never +/// overrides a deny. /// public sealed class AuthorizationGate { @@ -58,9 +64,20 @@ public sealed class AuthorizationGate } var decision = _evaluator.Authorize(session, operation, scope); - if (decision.IsAllowed) return true; + return decision.Verdict switch + { + // At least one grant matched — allow regardless of mode. + AuthorizationVerdict.Allow => true, - return !_strictMode; + // An authored deny rule matched. This is a definite verdict, not a gap in + // the ACLs, so the lax-mode fallback must NOT override it — an explicit + // deny denies in both strict and lax mode. + AuthorizationVerdict.Denied => false, + + // No grant matched (indeterminate). Strict mode denies; lax mode lets the + // dispatch proceed so deployments keep working while ACLs are populated. + _ => !_strictMode, + }; } /// diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Server/Security/LdapOptions.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Server/Security/LdapOptions.cs index 6dbd411..4b48f42 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Server/Security/LdapOptions.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Server/Security/LdapOptions.cs @@ -40,8 +40,12 @@ public sealed class LdapOptions public int Port { get; init; } = 3893; public bool UseTls { get; init; } = false; - /// Dev-only escape hatch — must be false in production. - public bool AllowInsecureLdap { get; init; } = true; + /// + /// Dev-only escape hatch that permits an unencrypted LDAP bind (plaintext credentials on + /// the server→LDAP hop) when is false. Defaults to false — + /// production deployments are secure-by-default and must opt in explicitly to run insecure. + /// + public bool AllowInsecureLdap { get; init; } = false; public string SearchBase { get; init; } = "dc=lmxopcua,dc=local"; public string ServiceAccountDn { get; init; } = string.Empty; diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Server.Tests/AuthorizationGateTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Server.Tests/AuthorizationGateTests.cs index c0605d2..4060232 100644 --- a/tests/Server/ZB.MOM.WW.OtOpcUa.Server.Tests/AuthorizationGateTests.cs +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Server.Tests/AuthorizationGateTests.cs @@ -133,4 +133,52 @@ public sealed class AuthorizationGateTests gate.BuildSessionState(new UserIdentity(), "c1").ShouldBeNull(); } + + /// Evaluator stub that always returns a fixed verdict — lets the gate's + /// verdict handling be exercised independent of the trie evaluator (which only ever + /// produces Allow / NotGranted). + private sealed class FixedVerdictEvaluator(AuthorizationVerdict verdict) : IPermissionEvaluator + { + public AuthorizationDecision Authorize(UserAuthorizationState session, OpcUaOperation operation, NodeScope scope) + => new(verdict, []); + } + + // Server-002 regression: an explicit Denied verdict must be honoured in BOTH modes — + // the lax-mode fallback covers only the indeterminate (NotGranted) case. + + [Fact] + public void ExplicitDeny_LaxMode_Denies() + { + var gate = new AuthorizationGate(new FixedVerdictEvaluator(AuthorizationVerdict.Denied), strictMode: false); + var identity = new FakeIdentity("ops-user", ["cn=ops"]); + + gate.IsAllowed(identity, OpcUaOperation.Read, Scope()).ShouldBeFalse(); + } + + [Fact] + public void ExplicitDeny_StrictMode_Denies() + { + var gate = new AuthorizationGate(new FixedVerdictEvaluator(AuthorizationVerdict.Denied), strictMode: true); + var identity = new FakeIdentity("ops-user", ["cn=ops"]); + + gate.IsAllowed(identity, OpcUaOperation.Read, Scope()).ShouldBeFalse(); + } + + [Fact] + public void NotGranted_LaxMode_Allows() + { + var gate = new AuthorizationGate(new FixedVerdictEvaluator(AuthorizationVerdict.NotGranted), strictMode: false); + var identity = new FakeIdentity("ops-user", ["cn=ops"]); + + gate.IsAllowed(identity, OpcUaOperation.Read, Scope()).ShouldBeTrue(); + } + + [Fact] + public void NotGranted_StrictMode_Denies() + { + var gate = new AuthorizationGate(new FixedVerdictEvaluator(AuthorizationVerdict.NotGranted), strictMode: true); + var identity = new FakeIdentity("ops-user", ["cn=ops"]); + + gate.IsAllowed(identity, OpcUaOperation.Read, Scope()).ShouldBeFalse(); + } } diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Server.Tests/LdapOptionsTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Server.Tests/LdapOptionsTests.cs new file mode 100644 index 0000000..7741b3c --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Server.Tests/LdapOptionsTests.cs @@ -0,0 +1,31 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Server.Security; + +namespace ZB.MOM.WW.OtOpcUa.Server.Tests; + +[Trait("Category", "Unit")] +public sealed class LdapOptionsTests +{ + // Server-009 regression: the out-of-the-box posture must be secure. AllowInsecureLdap + // is a dev-only escape hatch — a deployment that enables LDAP without explicitly + // opting in must not bind credentials over an unencrypted socket. + + [Fact] + public void AllowInsecureLdap_DefaultsToFalse() + { + new LdapOptions().AllowInsecureLdap.ShouldBeFalse(); + } + + [Fact] + public void UseTls_DefaultsToFalse_SoInsecureBindRequiresExplicitOptIn() + { + // UseTls=false on its own is fine — without AllowInsecureLdap the bind path + // refuses to send plaintext credentials. The two flags together are the only + // way to reach the insecure path, and both must be set deliberately. + var options = new LdapOptions(); + + options.UseTls.ShouldBeFalse(); + options.AllowInsecureLdap.ShouldBeFalse(); + } +}