diff --git a/docs/security.md b/docs/security.md index 37e27b4a0..422bb06f5 100644 --- a/docs/security.md +++ b/docs/security.md @@ -70,7 +70,7 @@ case-insensitively: | SubmodelElement `idShort` | Type | Result | |---------------------------|-------------------------------|-------------------------------------------------------------------------------------------------------------| -| `authenticationServer` | `SubmodelElementCollection` | `endpoint` / `type` / `publicCertificate` → `Program.redirectServer`, `Program.authType`, cert in `GlobalSecurityVariables.ServerCertificates`. Also reads side-car files `trustlist.txt` / `trustlist.xml` for additional issuer certs, JWKS URLs and kids. | +| `authenticationServer` | `SubmodelElementCollection` | `endpoint` / `type` / `publicCertificate` → `Program.redirectServer`, `Program.authType`, cert in `GlobalSecurityVariables.TrustedServers`. Also reads side-car files `trustlist.txt` / `trustlist.xml` for trusted token issuers (see §3.3). | | `roleMapping` | `SubmodelElementCollection` | Per entry: `subjects` (`email`, `emailDomain`, or any property `idShort`) × `roles` → `GlobalSecurityVariables.SecurityRights` entries (`SecurityRight { Name=subject, Role=role }`). | | `basicAuth` | `SubmodelElementCollection` | Each property is added to `GlobalSecurityVariables.SecurityUsernamePassword` (`IdShort` → `Value`). | @@ -112,6 +112,45 @@ properties with their password as `Value`: > collection (or disable it) and provide an `authenticationServer` entry that > points at a real OIDC / JWKS provider. +### 3.3 Trusted token issuers (`trustlist.txt` / `trustlist.xml`) + +Bearer tokens (other than Entra ID tokens) are only accepted from issuers listed +in a trust list. A token with a `kid` header is matched against the trust list +first by its `iss` claim, then by its `kid`; tokens from unlisted issuers are +rejected. The JWKS is always taken from the trust list entry, never from the +token. Tokens with a `serverName` claim are validated against the certificate +entries. + +`trustlist.txt` is a list of `key: value` lines, lines starting with `#` are +comments. `serverName`, `domain`, `jwks` and `audience` are collected for the +next entry; an entry is completed by: + +| Line | Entry | +|-----------------------------|---------------------------------------------------------------------------------------------------------| +| `issuer: ` | OIDC issuer. `iss` must equal the url (trailing `/` ignored). Keys from `jwks`, else from `/.well-known/openid-configuration`. | +| `kid: ` | Single key id, requires `jwks`. The issuer is not validated. | +| `-----END CERTIFICATE-----` | PEM certificate for tokens with `serverName: `. | + +`audience: a, b` lists accepted audiences (any of them must be in `aud`). Without +an `audience` line the comma-separated environment variable `TOKEN_AUDIENCE` is +used; if neither is set, the audience is not validated (a warning is logged). +`domain` is attached as `domain` claim to tokens that carry a user name. +All values are reset after each completed entry. + +``` +# Keycloak realm, service client with Audience mapper "aasx-server" +audience: aasx-server +issuer: https://identity.example.com/realms/fx +``` + +`trustlist.xml` (ETSI trust service list, optionally from the url in `TRUST_LIST` +and signature-checked with `publickey.pem`) adds one issuer entry per +`ServiceSupplyPoint`. + +Tokens without a user name (e.g. client-credentials service accounts) get no +role from `roleMapping`; grant them access with rules on their token claims, +e.g. `$contains` on `token:realm_access` for a Keycloak realm role. + --- ## 4. `AllAccessPermissionRules` — rule language at `/Access` diff --git a/src/AasSecurity/AasSecurityAuthenticationHandler.cs b/src/AasSecurity/AasSecurityAuthenticationHandler.cs index 29f1dd586..2c3540820 100644 --- a/src/AasSecurity/AasSecurityAuthenticationHandler.cs +++ b/src/AasSecurity/AasSecurityAuthenticationHandler.cs @@ -54,14 +54,6 @@ protected override Task HandleAuthenticateAsync() _logger.LogInformation($"Request is successfully authenticated."); - _logger.LogInformation("***************** LIST TICKET CLAIMS ********************"); - - foreach (var tickeClaim in ticket.Principal.Claims) - { - _logger.LogInformation($"CLAIM {tickeClaim?.ToString()}"); - _logger.LogInformation($"next claim"); - } - return Task.FromResult(AuthenticateResult.Success(ticket)); } diff --git a/src/AasSecurity/GlobalSecurityVariables.cs b/src/AasSecurity/GlobalSecurityVariables.cs index ac45694e9..fcbe4cc2f 100644 --- a/src/AasSecurity/GlobalSecurityVariables.cs +++ b/src/AasSecurity/GlobalSecurityVariables.cs @@ -12,7 +12,6 @@ ********************************************************************************/ using AasSecurity.Models; -using System.Security.Cryptography.X509Certificates; namespace AasSecurity { @@ -20,14 +19,9 @@ public static class GlobalSecurityVariables { public static bool WithAuthentication { get; set; } public static List SecurityRoles = new(); - internal static List ServerCertificates = new(); - internal static List ServerCertFileNames = new(); - internal static List ServerDomain = new(); - internal static List ServerJwksUrl = new(); - internal static List ServerIssuerUrl = new(); //Used when parsing from trustlist.xml - internal static List ServerKid = new(); + internal static readonly List TrustedServers = new(); internal static List SecurityRights = new(); - internal static Dictionary SecurityUsernamePassword = new(); + internal static readonly Dictionary SecurityUsernamePassword = new(); internal static Property ConditionSM = null; internal static Property ConditionSME = null; } diff --git a/src/AasSecurity/JwksTokenValidator.cs b/src/AasSecurity/JwksTokenValidator.cs new file mode 100644 index 000000000..b178554b3 --- /dev/null +++ b/src/AasSecurity/JwksTokenValidator.cs @@ -0,0 +1,143 @@ +/******************************************************************************** +* Copyright (c) {2019 - 2024} Contributors to the Eclipse Foundation +* +* See the NOTICE file(s) distributed with this work for additional +* information regarding copyright ownership. +* +* This program and the accompanying materials are made available under the +* terms of the Apache License Version 2.0 which is available at +* https://www.apache.org/licenses/LICENSE-2.0 +* +* SPDX-License-Identifier: Apache-2.0 +********************************************************************************/ + +using System.Collections.Concurrent; +using System.IdentityModel.Tokens.Jwt; +using System.Text.Json; +using AasSecurity.Models; +using AasxServerStandardBib.Services; +using Microsoft.Extensions.Logging; +using Microsoft.IdentityModel.Tokens; + +namespace AasSecurity +{ + /// + /// Validates bearer tokens with a "kid" header against the JWKS of a trusted issuer. + /// Only issuers/kids from the trust list are accepted; JWKS urls are derived from the trust list + /// entry only, never from the token itself. + /// + internal static class JwksTokenValidator + { + private static readonly ConcurrentDictionary _warnedNoAudience = new(); + + /// Fetches the content of a url (injected for testability). + /// Fallback audiences (TOKEN_AUDIENCE) for entries without own audience. + internal static bool TryValidate(string bearerToken, string? kid, string? iss, IReadOnlyList entries, + Func httpGet, string? globalAudienceCsv, ILogger logger, out string domain) + { + domain = ""; + + var entry = SecurityHelper.FindTrustedJwksEntry(entries, kid, iss); + if (entry == null) + { + logger.LogWarning("Token rejected: issuer {Issuer} / kid {Kid} is not in the trust list.", + LogSanitizer.Sanitize(iss), LogSanitizer.Sanitize(kid)); + return false; + } + + var jwksUrl = ResolveJwksUrl(entry, httpGet, logger); + if (string.IsNullOrEmpty(jwksUrl)) + { + logger.LogWarning("Token rejected: no jwks url for issuer {Issuer} / kid {Kid}.", + LogSanitizer.Sanitize(iss), LogSanitizer.Sanitize(kid)); + return false; + } + + try + { + var signingKeys = new JsonWebKeySet(httpGet(jwksUrl)).GetSigningKeys(); + var audiences = ResolveAudiences(entry, globalAudienceCsv); + if (audiences.Count == 0 && _warnedNoAudience.TryAdd(entry, true)) + { + logger.LogWarning("No audience configured for issuer {Issuer} / kid {Kid} and TOKEN_AUDIENCE is not set: audience is not validated.", + LogSanitizer.Sanitize(iss), LogSanitizer.Sanitize(kid)); + } + + var validationParameters = BuildParameters(entry, signingKeys, audiences); + new JwtSecurityTokenHandler().ValidateToken(bearerToken, validationParameters, out _); + } + catch (Exception ex) + { + logger.LogWarning("Token rejected for issuer {Issuer} / kid {Kid}: {Reason}", + LogSanitizer.Sanitize(iss), LogSanitizer.Sanitize(kid), LogSanitizer.Sanitize(ex.Message)); + return false; + } + + domain = entry.Domain; + return true; + } + + internal static string? ResolveJwksUrl(TrustedServer entry, Func httpGet, ILogger logger) + { + if (!string.IsNullOrEmpty(entry.JwksUrl)) + { + return entry.JwksUrl; + } + + var issuer = SecurityHelper.NormalizeIssuer(entry.Issuer); + if (issuer == "") + { + return null; + } + + try + { + using var openIdConfig = JsonDocument.Parse(httpGet($"{issuer}/.well-known/openid-configuration")); + if (openIdConfig.RootElement.TryGetProperty("jwks_uri", out var jwksUri)) + { + return jwksUri.GetString(); + } + } + catch (Exception ex) + { + logger.LogWarning("Could not access well-known from {Issuer}: {Reason}", LogSanitizer.Sanitize(issuer), LogSanitizer.Sanitize(ex.Message)); + } + + return $"{issuer}/jwks"; + } + + internal static IReadOnlyList ResolveAudiences(TrustedServer entry, string? globalAudienceCsv) + { + if (entry.Audiences.Count > 0) + { + return entry.Audiences; + } + + return (globalAudienceCsv ?? "").Split(',').Select(a => a.Trim()).Where(a => a != "").ToList(); + } + + internal static TokenValidationParameters BuildParameters(TrustedServer entry, IEnumerable signingKeys, + IReadOnlyList audiences) + { + var parameters = new TokenValidationParameters + { + ValidateLifetime = true, + ValidateIssuerSigningKey = true, + RequireSignedTokens = true, + IssuerSigningKeys = signingKeys, + // Entries matched by kid only have no configured issuer; their keys are pinned by the configured jwks url. + ValidateIssuer = entry.Issuer != "", + ValidateAudience = audiences.Count > 0, + ValidAudiences = audiences + }; + + if (entry.Issuer != "") + { + var normalized = SecurityHelper.NormalizeIssuer(entry.Issuer); + parameters.ValidIssuers = new[] { entry.Issuer, normalized, normalized + "/" }.Distinct().ToList(); + } + + return parameters; + } + } +} diff --git a/src/AasSecurity/Models/TrustedServer.cs b/src/AasSecurity/Models/TrustedServer.cs new file mode 100644 index 000000000..93ad54c38 --- /dev/null +++ b/src/AasSecurity/Models/TrustedServer.cs @@ -0,0 +1,42 @@ +/******************************************************************************** +* Copyright (c) {2019 - 2024} Contributors to the Eclipse Foundation +* +* See the NOTICE file(s) distributed with this work for additional +* information regarding copyright ownership. +* +* This program and the accompanying materials are made available under the +* terms of the Apache License Version 2.0 which is available at +* https://www.apache.org/licenses/LICENSE-2.0 +* +* SPDX-License-Identifier: Apache-2.0 +********************************************************************************/ + +using System.Security.Cryptography.X509Certificates; + +namespace AasSecurity.Models +{ + /// + /// One trusted token issuer, read from trustlist.txt, trustlist.xml or the publicCertificate + /// of the SecuritySettingsForServer submodel. + /// A certificate entry (Certificate + CertFileName) validates tokens carrying a "serverName" claim. + /// A JWKS entry (Issuer and/or Kid, optional JwksUrl) validates tokens carrying a "kid" header. + /// + internal sealed class TrustedServer + { + public X509Certificate2? Certificate { get; init; } + public string CertFileName { get; init; } = ""; + public string Domain { get; init; } = ""; + public string JwksUrl { get; init; } = ""; + public string Issuer { get; init; } = ""; + public string Kid { get; init; } = ""; + public IReadOnlyList Audiences { get; init; } = []; + public string Source { get; init; } = ""; + + public bool IsJwksEntry => !string.IsNullOrEmpty(Issuer) || !string.IsNullOrEmpty(Kid); + + public override string ToString() => + !string.IsNullOrEmpty(Issuer) ? $"issuer {Issuer} ({Source})" : + !string.IsNullOrEmpty(Kid) ? $"kid {Kid} ({Source})" : + $"certificate {CertFileName} ({Source})"; + } +} diff --git a/src/AasSecurity/SecurityHelper.cs b/src/AasSecurity/SecurityHelper.cs index bcfc29536..92291249a 100644 --- a/src/AasSecurity/SecurityHelper.cs +++ b/src/AasSecurity/SecurityHelper.cs @@ -11,6 +11,7 @@ * SPDX-License-Identifier: Apache-2.0 ********************************************************************************/ +using AasSecurity.Models; using AasxServer; using Extensions; using Microsoft.IdentityModel.Tokens; @@ -33,11 +34,11 @@ public static void SecurityInit() } private static void ParseSecurityMetamodel() - { - if (Program.env == null) - { - return; - } + { + if (Program.env == null) + { + return; + } foreach (var env in Program.env) { @@ -82,41 +83,47 @@ private static void ParseSecurityMetamodel() } internal static X509Certificate2? FindServerCertificate(string serverName, out string domain) - { + { domain = ""; - if (GlobalSecurityVariables.ServerCertFileNames != null) + foreach (var entry in GlobalSecurityVariables.TrustedServers) { - for (var i = 0; i < GlobalSecurityVariables.ServerCertFileNames.Count; i++) + if (entry.Certificate != null && Path.GetFileName(entry.CertFileName) == serverName + ".cer") { - if (Path.GetFileName(GlobalSecurityVariables.ServerCertFileNames[i]) == serverName + ".cer") - { - domain = GlobalSecurityVariables.ServerDomain[i]; - return GlobalSecurityVariables.ServerCertificates[i]; - } + domain = entry.Domain; + return entry.Certificate; } } return null; } - - internal static string? FindServerJwksUrl(string kid, string iss, out string domain) - { - domain = ""; - if (GlobalSecurityVariables.ServerKid != null) + + internal static TrustedServer? FindTrustedJwksEntry(string? kid, string? iss) + => FindTrustedJwksEntry(GlobalSecurityVariables.TrustedServers, kid, iss); + + /// + /// Finds the trust list entry for a token: first by issuer (iss claim), then by kid header. + /// Empty values never match. + /// + internal static TrustedServer? FindTrustedJwksEntry(IReadOnlyList entries, string? kid, string? iss) + { + var normalizedIss = NormalizeIssuer(iss); + if (normalizedIss != "") { - for (var i = 0; i < GlobalSecurityVariables.ServerKid.Count; i++) - { - if ((GlobalSecurityVariables.ServerKid[i] != "" && - GlobalSecurityVariables.ServerKid[i] == kid) - || GlobalSecurityVariables.ServerIssuerUrl[i] == iss) - { - domain = GlobalSecurityVariables.ServerDomain[i]; - return GlobalSecurityVariables.ServerJwksUrl[i]; - } + var byIssuer = entries.FirstOrDefault(e => e.Issuer != "" && NormalizeIssuer(e.Issuer) == normalizedIss); + if (byIssuer != null) + { + return byIssuer; } } + if (!string.IsNullOrEmpty(kid)) + { + return entries.FirstOrDefault(e => e.Kid != "" && e.Kid == kid); + } + return null; } + + internal static string NormalizeIssuer(string? issuer) => (issuer ?? "").Trim().TrimEnd('/'); } } diff --git a/src/AasSecurity/SecurityService.cs b/src/AasSecurity/SecurityService.cs index 9aedcdb5f..9fcf14e13 100644 --- a/src/AasSecurity/SecurityService.cs +++ b/src/AasSecurity/SecurityService.cs @@ -110,6 +110,12 @@ public void parseAccessRuleFile() r.Acl.Rights != null && r.Acl.Rights.Contains(neededRightsClaim) && r.Acl.Attributes != null && + // An ATTRIBUTE is a single CLAIM key/value pair (Contracts.JsonData.AttributeItem), + // so it has no slot for an expected value: a non-token attribute is compared to + // the access role, a "token:*" attribute only tests that the token CARRIES such a + // claim. Value tests belong in FORMULA via $attribute(CLAIM(...)). A rule like + // {"CLAIM": "token:sub"} therefore matches every token-authenticated user by + // design and is expected to evaluate false for most of them. r.Acl.Attributes.All(a => a.ItemType == "CLAIM" && ( @@ -163,7 +169,12 @@ public void parseAccessRuleFile() { var ruleConditions = CombineAccessRuleFormulaAndFilter(rule); if (ruleConditions == null) - continue; + { + // Neither FORMULA nor FILTER: the rule grants unconditionally, so no row-level + // restriction can apply. Skipping it would leave the other rules' predicates as + // the effective filter — the same mistake OrMerge used to make for $boolean: true. + return null; + } var perRequestRuleConditions = ruleConditions.Clone(); merged = merged == null @@ -171,14 +182,20 @@ public void parseAccessRuleFile() : SqlConditionsMerger.OrMerge(merged, perRequestRuleConditions); } - if (merged != null) + if (SqlConditionsMerger.IsUnrestricted(merged)) { - // Resolve $attribute(CLAIM(...)) sentinels with the request's token claims before - // deriving the C# mirror — see SqlConditions.SubstituteTokenClaims for the contract. - merged.SubstituteTokenClaims(tokenClaims); - SqlConditions.RefreshFormulaConditionsCSharpFromFormulaSql(merged); + // Same "no restriction" meaning as the no-matching-rules return above. Allow/deny is + // decided by AuthorizeRequest, not here, and null spares consumers such as + // CrudOperator.IsSubmodelAllowedBySqlCondition a per-submodel query that can only + // ever answer "allowed". + return null; } + // Resolve $attribute(CLAIM(...)) sentinels with the request's token claims before + // deriving the C# mirror — see SqlConditions.SubstituteTokenClaims for the contract. + merged!.SubstituteTokenClaims(tokenClaims); + SqlConditions.RefreshFormulaConditionsCSharpFromFormulaSql(merged); + return merged; } @@ -210,12 +227,15 @@ private bool AnyTreeReadRulePasses(string? accessRole, string httpRoute, List @@ -463,8 +485,8 @@ private string GetAccessRole(NameValueCollection queries, NameValueCollection he ParseBearerToken(queries, headers, ref bearerToken, ref error, ref user, ref accessRole); if (accessRole != null) - { - _logger.LogDebug($"Access role found {accessRole}"); + { + _logger.LogDebug("Access role found"); return accessRole; } @@ -517,15 +539,15 @@ private string GetAccessRole(NameValueCollection queries, NameValueCollection he private string HandleBearerToken(string? bearerToken, ref string user, ref bool error, out string policy, out string policyRequestedResource, out List tokenClaims, out string domain) - { + { domain = ""; policy = ""; policyRequestedResource = ""; tokenClaims = []; - if (bearerToken == null) - { - _logger.LogDebug("NO BEARER TOKEN"); + if (bearerToken == null) + { + _logger.LogDebug("NO BEARER TOKEN"); return null; } @@ -577,7 +599,7 @@ private string HandleBearerToken(string? bearerToken, ref string user, ref bool } catch (Exception ex) { - _logger.LogError($"Token validation failed: {ex.Message}"); + _logger.LogError("Token validation failed: {Reason}", LogSanitizer.Sanitize(ex.Message)); user = ""; return ""; } @@ -598,8 +620,7 @@ private string HandleBearerToken(string? bearerToken, ref string user, ref bool audience = audienceEnvVar; } - _logger.LogDebug($"TOKENEXCHANGE1 = {tokenExchange1}"); - _logger.LogDebug($"TOKENEXCHANGE2 = {tokenExchange2}"); + _logger.LogDebug("Token exchange is configured for issuer {Issuer}", LogSanitizer.Sanitize(tokenExchange1)); var handlerExchange = new HttpClientHandler { DefaultProxyCredentials = CredentialCache.DefaultCredentials }; var client = new HttpClient(handlerExchange); @@ -627,7 +648,7 @@ private string HandleBearerToken(string? bearerToken, ref string user, ref bool if (doc.RootElement.TryGetProperty("access_token", out var tokenElement)) { bearerToken = tokenElement.GetString(); - _logger.LogDebug("token exchange 2 " + bearerToken); + _logger.LogDebug("Token exchange 2: access token received"); jwtSecurityToken = handler.ReadJwtToken(bearerToken); iss = ""; @@ -657,7 +678,7 @@ private string HandleBearerToken(string? bearerToken, ref string user, ref bool } catch (Exception ex) { - _logger.LogDebug($"Could not access well-known from {iss}"); + _logger.LogDebug("Could not access well-known from {Issuer}", LogSanitizer.Sanitize(iss)); } parameters = new Dictionary @@ -684,7 +705,7 @@ private string HandleBearerToken(string? bearerToken, ref string user, ref bool jwtSecurityToken = null; bearerToken = tokenElementFromLocalIdp.GetString(); - _logger.LogDebug("token exchange 3 (with local idP) " + bearerToken); + _logger.LogDebug("Token exchange 3 (with local idP): token received"); jwtSecurityToken = handler.ReadJwtToken(bearerToken); issClaim = jwtSecurityToken.Claims.Where(c => c.Type == "iss"); @@ -711,67 +732,20 @@ private string HandleBearerToken(string? bearerToken, ref string user, ref bool } if (!valid) { - if (jwtSecurityToken.Header.TryGetValue("kid", out _)) + if (jwtSecurityToken.Header.TryGetValue("kid", out var kidHeader)) { user = ""; - var jwksUrl = ""; - var kid = jwtSecurityToken.Header["kid"].ToString(); var clientHandler = new HttpClientHandler { DefaultProxyCredentials = CredentialCache.DefaultCredentials }; using var httpClient = new HttpClient(clientHandler); - if (kid != null) + // Only issuers/kids from the trust list are accepted, with issuer and audience validated. + // If not valid, tokens with a "serverName" claim are still checked against the server certificates below. + valid = JwksTokenValidator.TryValidate(bearerToken, kidHeader?.ToString(), iss, GlobalSecurityVariables.TrustedServers, + url => httpClient.GetStringAsync(url).Result, System.Environment.GetEnvironmentVariable("TOKEN_AUDIENCE"), + _logger, out var jwksDomain); + if (valid) { - jwksUrl = SecurityHelper.FindServerJwksUrl(kid, iss, out domain); - } - if (jwksUrl.IsNullOrEmpty()) - { - jwksUrl = $"{iss}/jwks"; - - try - { - var openIdConfig = httpClient.GetStringAsync($"{iss}/.well-known/openid-configuration").Result; - var openIdConfigJson = JsonDocument.Parse(openIdConfig); - - if (openIdConfigJson.RootElement.TryGetProperty("jwks_uri", out var propJwksUri)) - { - jwksUrl = propJwksUri.GetString(); - } - } - catch (Exception ex) - { - _logger.LogWarning($"Could not access well-known from {iss}"); - } - } - try - { - var jwksJson = httpClient.GetStringAsync(jwksUrl).Result; - var jwks = new JsonWebKeySet(jwksJson); - var signingKeys = jwks.GetSigningKeys(); - - var tokenHandler = new JwtSecurityTokenHandler(); - var validationParameters = new TokenValidationParameters - { - ValidateIssuer = false, - ValidateAudience = false, - ValidateLifetime = true, - ValidateIssuerSigningKey = true, - IssuerSigningKeys = signingKeys - }; - - try - { - var principal = tokenHandler.ValidateToken(bearerToken, validationParameters, out var validatedToken); - - valid = true; - } - catch (Exception ex) - { - _logger.LogWarning($"Error in validation of token {bearerToken}: {ex.Message}."); - } - } - catch (Exception ex) - { - _logger.LogWarning($"Error in loading jwks from {jwksUrl}: {ex.Message}."); + domain = jwksDomain; } } } @@ -784,7 +758,7 @@ private string HandleBearerToken(string? bearerToken, ref string user, ref bool X509Certificate2? cert = SecurityHelper.FindServerCertificate(serverName, out domain); if (cert == null) { - user = ""; + user = ""; _logger.LogDebug("NO SERVER CERTIFICATE FOUND"); return ""; } @@ -794,7 +768,7 @@ private string HandleBearerToken(string? bearerToken, ref string user, ref bool builder.AppendLine( Convert.ToBase64String(cert.RawData, Base64FormattingOptions.InsertLineBreaks)); builder.AppendLine("-----END CERTIFICATE-----"); - _logger.LogDebug("Token Server Certificate: " + serverName); + _logger.LogDebug("Token Server Certificate: {ServerName}", LogSanitizer.Sanitize(serverName)); _logger.LogDebug(builder.ToString()); //check if server cert is correctly signed try @@ -805,7 +779,7 @@ private string HandleBearerToken(string? bearerToken, ref string user, ref bool } catch { - user = ""; + user = ""; _logger.LogWarning("DECODE SERVER CERTIFICATE FAILED"); return ""; } @@ -884,7 +858,7 @@ private string HandleBearerToken(string? bearerToken, ref string user, ref bool if (domain != "") { tokenClaims.Add(new Claim("domain", domain)); - } + } return ""; } @@ -894,11 +868,11 @@ private string HandleBearerToken(string? bearerToken, ref string user, ref bool catch (Exception ex) { error = true; - _logger.LogWarning($"Exception occurred while parsing the bearer token.{ex.Message}"); + _logger.LogWarning("Exception occurred while parsing the bearer token: {Reason}", LogSanitizer.Sanitize(ex.Message)); _logger.LogDebug(ex.StackTrace); } - user = ""; + user = ""; return ""; } @@ -919,8 +893,7 @@ private string HandleBearerToken(string? bearerToken, ref string user, ref bool switch (split[0].ToLower()) { case "bearer": - _logger.LogDebug($"Received bearer token {split[1]}"); - //_logger.LogInformation("Received bearer token {Sanitize}", LogSanitizer.Sanitize(split[1])); + _logger.LogDebug("Received bearer token from authorization header"); bearerToken = split[1]; break; case "basic" when bearerToken == null: @@ -932,7 +905,7 @@ private string HandleBearerToken(string? bearerToken, ref string user, ref bool var credentials = Encoding.UTF8.GetString(credentialBytes).Split(new[] { ':' }, 2); string u = credentials[0]; string p = credentials[1]; - _logger.LogInformation("Received username+password http header = " + u + " : " + p); + _logger.LogInformation("Received username+password http header for user {User}", LogSanitizer.Sanitize(u)); if (u == "secret") { @@ -951,7 +924,7 @@ private string HandleBearerToken(string? bearerToken, ref string user, ref bool if (username != null) { user = username; - _logger.LogInformation("Received username+password http header = " + user); + _logger.LogInformation("Received username+password http header for user {User}", LogSanitizer.Sanitize(user)); } else { @@ -963,7 +936,7 @@ private string HandleBearerToken(string? bearerToken, ref string user, ref bool } catch (ArgumentException argumentException) { - _logger.LogInformation($"Exception while extracting {nameof(Program.secretStringAPI)}: {argumentException.Message}"); + _logger.LogInformation("Exception while extracting {Name}: {Reason}", nameof(Program.secretStringAPI), LogSanitizer.Sanitize(argumentException.Message)); } break; @@ -997,7 +970,7 @@ private string HandleBearerToken(string? bearerToken, ref string user, ref bool var secretQuery = queries["s"]!; if (!string.IsNullOrEmpty(secretQuery)) { - _logger.LogInformation("Received token of type s: {Sanitize}", LogSanitizer.Sanitize(secretQuery)); + _logger.LogInformation("Received token of type s"); if (Program.secretStringAPI != null) { if (secretQuery.Equals(Program.secretStringAPI)) @@ -1014,7 +987,7 @@ private string HandleBearerToken(string? bearerToken, ref string user, ref bool var token = queries[key]; if (token != null) { - _logger.LogInformation("Received token of type bear {Sanitize}", LogSanitizer.Sanitize(token)); + _logger.LogInformation("Received token of type bear"); bearerToken = token; } @@ -1038,7 +1011,7 @@ private string HandleBearerToken(string? bearerToken, ref string user, ref bool var token = queries[key]; if (token != null) { - _logger.LogInformation("Received token of type username-password {Sanitize}", LogSanitizer.Sanitize(token)); + _logger.LogInformation("Received token of type username-password"); var username = CheckUserPW(token, out var userName, out var passWord); if (username != null) { @@ -1114,12 +1087,8 @@ private static bool CheckAccessLevelWithError(out string error, string currentRo currentRole = "isNotAuthenticated"; } - _logger.LogDebug("checkAccessLevel: " + - " currentRole = " + currentRole + - " operation = " + operation + - " neededRights = " + neededRights + - " objPath = " + objPath - ); + _logger.LogDebug("checkAccessLevel: operation = {Operation} neededRights = {NeededRights} objPath = {ObjPath}", + LogSanitizer.Sanitize(operation), neededRights, LogSanitizer.Sanitize(objPath)); if (aasResource == null) { diff --git a/src/AasSecurity/SecuritySettingsForServerParser.cs b/src/AasSecurity/SecuritySettingsForServerParser.cs index 7db144d40..b6b3bc945 100644 --- a/src/AasSecurity/SecuritySettingsForServerParser.cs +++ b/src/AasSecurity/SecuritySettingsForServerParser.cs @@ -16,6 +16,7 @@ using System.Net; using System.Security.Cryptography; using System.Security.Cryptography.X509Certificates; +using System.Text; using System.Text.Json; using System.Xml; using System.Xml.Linq; @@ -100,85 +101,146 @@ private static void ParseBasicAuth(SubmodelElementCollection basicAuth) } } - private async static void ParseAuthenticationServer(AdminShellPackageEnv env, SubmodelElementCollection? authServer) + /// + /// Parses trustlist.txt. Lines are "key: value"; lines starting with '#' are comments. + /// Keys serverName, domain, jwks and audience (comma-separated, repeatable) are collected for the + /// next entry. An entry is committed by + /// "END CERTIFICATE" (after a PEM block) - certificate entry for tokens with a "serverName" claim, + /// "kid: " - JWKS entry matched by the token's kid header (requires jwks), + /// "issuer: " - JWKS entry matched by the token's iss claim; issuer is validated, keys are + /// taken from jwks or from the issuer's .well-known/openid-configuration. + /// All collected values are reset after each committed entry. + /// + internal static List ParseTrustListText(IEnumerable lines) { - var trustListOnServer = System.Environment.GetEnvironmentVariable("TRUST_LIST"); - var trustListPubKeyOnServer = System.Environment.GetEnvironmentVariable("TRUST_LIIST_PUBLIC_KEY"); + var entries = new List(); - if (System.IO.File.Exists("trustlist.txt")) + var serverName = ""; + var domain = ""; + var jwks = ""; + var audiences = new List(); + var base64 = new StringBuilder(); + var insideBase64 = false; + + void Reset() { - Console.WriteLine("Read trustlist.txt"); - var lines = System.IO.File.ReadAllLines("trustlist.txt"); + serverName = ""; + domain = ""; + jwks = ""; + audiences = new List(); + } + + foreach (var rawLine in lines) + { + var line = rawLine.Trim(); + + if (insideBase64) { - var serverName = ""; - var domain = ""; - var base64 = ""; - var insideBas64 = false; - var jwks = ""; - var kid = ""; - foreach (var line in lines) + if (line.Contains("END CERTIFICATE")) { - if (line == "" || line.StartsWith("# ")) + insideBase64 = false; + var x509 = new X509Certificate2(Convert.FromBase64String(base64.ToString())); + entries.Add(new TrustedServer { - continue; - } + Certificate = x509, + CertFileName = serverName + ".cer", + Domain = domain, + Source = "trustlist.txt" + }); + Reset(); + } + else + { + base64.Append(line); + } - if (line.Contains("serverName: ")) - { - var split = line.Split(": "); - serverName = split[1]; - Console.WriteLine(" serverName: " + serverName); - } - else if (line.Contains("domain: ")) - { - var split = line.Split(": "); - domain = split[1]; - Console.WriteLine(" domain: " + domain); - } - else if (line.Contains("jwks: ")) - { - var split = line.Split(": "); - jwks = split[1]; - Console.WriteLine(" jwks: " + jwks); - } - else if (line.Contains("kid: ")) - { - var split = line.Split(": "); - kid = split[1]; - Console.WriteLine(" kid: " + kid); - GlobalSecurityVariables.ServerCertificates.Add(null); - GlobalSecurityVariables.ServerCertFileNames.Add(""); - GlobalSecurityVariables.ServerDomain.Add(domain); - GlobalSecurityVariables.ServerJwksUrl.Add(jwks); - GlobalSecurityVariables.ServerIssuerUrl.Add(""); - GlobalSecurityVariables.ServerKid.Add(kid); - } - else if (line.Contains("BEGIN CERTIFICATE")) + continue; + } + + if (line == "" || line.StartsWith('#')) + { + continue; + } + + if (line.Contains("BEGIN CERTIFICATE")) + { + insideBase64 = true; + base64.Clear(); + continue; + } + + var separator = line.IndexOf(": ", StringComparison.Ordinal); + if (separator <= 0) + { + continue; + } + + var key = line[..separator].Trim(); + var value = line[(separator + 2)..].Trim(); + Console.WriteLine($" {key}: {value}"); + + switch (key) + { + case "serverName": + serverName = value; + break; + case "domain": + domain = value; + break; + case "jwks": + jwks = value; + break; + case "audience": + audiences.AddRange(value.Split(',').Select(a => a.Trim()).Where(a => a != "")); + break; + case "kid": + if (string.IsNullOrEmpty(jwks)) { - insideBas64 = true; - base64 = ""; + Console.WriteLine($" WARNING: kid {value} has no jwks url, entry ignored"); } - else if (line.Contains("END CERTIFICATE")) + else { - insideBas64 = false; - base64 = base64.Replace("\r", "").Replace("\n", "").Trim(); - var certBytes = Convert.FromBase64String(base64); - var x509 = new X509Certificate2(certBytes); - GlobalSecurityVariables.ServerCertificates.Add(x509); - GlobalSecurityVariables.ServerCertFileNames.Add(serverName + ".cer"); - GlobalSecurityVariables.ServerDomain.Add(domain); - GlobalSecurityVariables.ServerJwksUrl.Add(""); - GlobalSecurityVariables.ServerIssuerUrl.Add(""); - GlobalSecurityVariables.ServerKid.Add(""); + Console.WriteLine($" WARNING: kid {value} has no issuer, the issuer of its tokens is not validated"); + entries.Add(new TrustedServer + { + Kid = value, + JwksUrl = jwks, + Domain = domain, + Audiences = audiences, + Source = "trustlist.txt" + }); } - else if (insideBas64) + Reset(); + break; + case "issuer": + entries.Add(new TrustedServer { - base64 += line; - } - } + Issuer = value, + JwksUrl = jwks, + Domain = domain, + Audiences = audiences, + Source = "trustlist.txt" + }); + Reset(); + break; } } + return entries; + } + + private async static void ParseAuthenticationServer(AdminShellPackageEnv env, SubmodelElementCollection? authServer) + { + var trustListOnServer = System.Environment.GetEnvironmentVariable("TRUST_LIST"); + var trustListPubKeyOnServer = System.Environment.GetEnvironmentVariable("TRUST_LIIST_PUBLIC_KEY"); + + if (System.IO.File.Exists("trustlist.txt")) + { + Console.WriteLine("Read trustlist.txt"); + var lines = System.IO.File.ReadAllLines("trustlist.txt"); + GlobalSecurityVariables.TrustedServers.AddRange(ParseTrustListText(lines)); + } + XDocument doc = null; // Create a new XML document. @@ -331,15 +393,13 @@ private async static void ParseAuthenticationServer(AdminShellPackageEnv env, Su var issuer = service?.SupplyPoint; Console.WriteLine(" issuer url: " + issuer); - var kid = ""; - - GlobalSecurityVariables.ServerCertificates.Add(null); - GlobalSecurityVariables.ServerCertFileNames.Add(""); - GlobalSecurityVariables.ServerCertFileNames.Add(serverName + ".cer"); - GlobalSecurityVariables.ServerDomain.Add(domain); - GlobalSecurityVariables.ServerIssuerUrl.Add(issuer); - GlobalSecurityVariables.ServerJwksUrl.Add(""); - GlobalSecurityVariables.ServerKid.Add(kid); + GlobalSecurityVariables.TrustedServers.Add(new TrustedServer + { + CertFileName = serverName + ".cer", + Domain = domain ?? "", + Issuer = issuer ?? "", + Source = "trustlist.xml" + }); } } } @@ -380,9 +440,13 @@ private async static void ParseAuthenticationServer(AdminShellPackageEnv env, Su { certStream.CopyTo(memoryStream); var buffer = memoryStream.GetBuffer(); - GlobalSecurityVariables.ServerCertificates.Add(new X509Certificate2(buffer)); string[] split = publicCert.Value.Split('/'); - GlobalSecurityVariables.ServerCertFileNames.Add(split[3]); + GlobalSecurityVariables.TrustedServers.Add(new TrustedServer + { + Certificate = new X509Certificate2(buffer), + CertFileName = split[3], + Source = "aasx" + }); //_logger.LogDebug($"Loaded auth server certificate: {split[3]}"); } } diff --git a/src/AasSecurityTests/TrustListTests.cs b/src/AasSecurityTests/TrustListTests.cs new file mode 100644 index 000000000..de3e34bfe --- /dev/null +++ b/src/AasSecurityTests/TrustListTests.cs @@ -0,0 +1,286 @@ +/******************************************************************************** +* Copyright (c) {2019 - 2024} Contributors to the Eclipse Foundation +* +* See the NOTICE file(s) distributed with this work for additional +* information regarding copyright ownership. +* +* This program and the accompanying materials are made available under the +* terms of the Apache License Version 2.0 which is available at +* https://www.apache.org/licenses/LICENSE-2.0 +* +* SPDX-License-Identifier: Apache-2.0 +********************************************************************************/ + +using System.IdentityModel.Tokens.Jwt; +using System.Security.Cryptography; +using AasSecurity.Models; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.IdentityModel.Tokens; + +namespace AasSecurity; + +public class TrustListTextParserTests +{ + [Fact] + public void IssuerEntry_WithAudiences_IsParsed() + { + var entries = SecuritySettingsForServerParser.ParseTrustListText(new[] + { + "# comment", + "", + "audience: aasx-server, other", + "audience: third", + "issuer: https://idp.example.com/realms/fx" + }); + + entries.Should().ContainSingle(); + entries[0].Issuer.Should().Be("https://idp.example.com/realms/fx"); + entries[0].JwksUrl.Should().BeEmpty(); + entries[0].Audiences.Should().Equal("aasx-server", "other", "third"); + } + + [Fact] + public void KidEntry_WithJwks_IsParsed() + { + var entries = SecuritySettingsForServerParser.ParseTrustListText(new[] + { + "domain: example.com", + "jwks: https://idp.example.com/jwks", + "kid: k1" + }); + + entries.Should().ContainSingle(); + entries[0].Kid.Should().Be("k1"); + entries[0].JwksUrl.Should().Be("https://idp.example.com/jwks"); + entries[0].Domain.Should().Be("example.com"); + entries[0].Issuer.Should().BeEmpty(); + } + + [Fact] + public void KidEntry_WithoutJwks_IsIgnored() + { + SecuritySettingsForServerParser.ParseTrustListText(new[] { "kid: k1" }).Should().BeEmpty(); + } + + [Fact] + public void Values_DoNotLeakIntoNextEntry() + { + var entries = SecuritySettingsForServerParser.ParseTrustListText(new[] + { + "domain: example.com", + "jwks: https://idp.example.com/jwks", + "audience: a", + "kid: k1", + "issuer: https://other.example.com" + }); + + entries.Should().HaveCount(2); + entries[1].Domain.Should().BeEmpty(); + entries[1].JwksUrl.Should().BeEmpty(); + entries[1].Audiences.Should().BeEmpty(); + } + + [Fact] + public void RepositoryTrustList_ContainsCertificates() + { + var path = FindRepoFile(Path.Combine("src", "AasxServerBlazor", "trustlist.txt")); + + var entries = SecuritySettingsForServerParser.ParseTrustListText(System.IO.File.ReadAllLines(path)); + + entries.Where(e => e.Certificate != null).Should().HaveCount(4); + } + + private static string FindRepoFile(string relativePath) + { + var dir = new DirectoryInfo(AppContext.BaseDirectory); + while (dir != null && !System.IO.File.Exists(Path.Combine(dir.FullName, relativePath))) + { + dir = dir.Parent; + } + + dir.Should().NotBeNull($"{relativePath} must exist in a parent directory of the test output"); + return Path.Combine(dir!.FullName, relativePath); + } +} + +public class TrustedJwksEntryMatcherTests +{ + private static readonly TrustedServer IssuerEntry = new() { Issuer = "https://idp.example.com/realms/fx/" }; + private static readonly TrustedServer KidEntry = new() { Kid = "k1", JwksUrl = "https://keys.example.com" }; + private static readonly TrustedServer CertEntry = new() { CertFileName = "server.cer" }; + private static readonly TrustedServer[] Entries = { CertEntry, KidEntry, IssuerEntry }; + + [Theory] + [InlineData("https://idp.example.com/realms/fx")] + [InlineData("https://idp.example.com/realms/fx/")] + public void MatchesIssuer_IgnoringTrailingSlash(string iss) + { + SecurityHelper.FindTrustedJwksEntry(Entries, null, iss).Should().BeSameAs(IssuerEntry); + } + + [Fact] + public void IssuerWinsOverKid() + { + SecurityHelper.FindTrustedJwksEntry(Entries, "k1", "https://idp.example.com/realms/fx").Should().BeSameAs(IssuerEntry); + } + + [Fact] + public void MatchesKid() + { + SecurityHelper.FindTrustedJwksEntry(Entries, "k1", "https://unknown.example.com").Should().BeSameAs(KidEntry); + } + + [Theory] + [InlineData(null, null)] + [InlineData("", "")] + [InlineData("k2", "https://unknown.example.com")] + public void NoMatch_ReturnsNull(string? kid, string? iss) + { + SecurityHelper.FindTrustedJwksEntry(Entries, kid, iss).Should().BeNull(); + } +} + +public class JwksTokenValidatorTests +{ + private const string Issuer = "https://idp.example.com/realms/fx"; + private const string JwksUri = Issuer + "/protocol/openid-connect/certs"; + + private readonly RsaSecurityKey _key = new(RSA.Create(2048)) { KeyId = "k1" }; + private readonly RsaSecurityKey _otherKey = new(RSA.Create(2048)) { KeyId = "k1" }; + private readonly Dictionary _web = new(); + private readonly List _requested = new(); + + public JwksTokenValidatorTests() + { + _web[Issuer + "/.well-known/openid-configuration"] = $"{{\"issuer\":\"{Issuer}\",\"jwks_uri\":\"{JwksUri}\"}}"; + _web[JwksUri] = Jwks(_key); + } + + private string HttpGet(string url) + { + _requested.Add(url); + return _web.TryGetValue(url, out var content) ? content : throw new HttpRequestException($"404 {url}"); + } + + private static string Jwks(RsaSecurityKey key) + { + var p = key.Rsa.ExportParameters(false); + return $"{{\"keys\":[{{\"kty\":\"RSA\",\"use\":\"sig\",\"alg\":\"RS256\",\"kid\":\"{key.KeyId}\"," + + $"\"n\":\"{Base64UrlEncoder.Encode(p.Modulus)}\",\"e\":\"{Base64UrlEncoder.Encode(p.Exponent)}\"}}]}}"; + } + + private string Token(string iss = Issuer, object? aud = null, RsaSecurityKey? key = null, int expiresInMinutes = 5) + { + var now = DateTimeOffset.UtcNow; + var payload = new JwtPayload + { + { "iss", iss }, + { "sub", "d3eceb07-ba47-4f8a-8aa1-337bf6eb17b6" }, + { "iat", now.AddMinutes(-10).ToUnixTimeSeconds() }, + { "nbf", now.AddMinutes(-10).ToUnixTimeSeconds() }, + { "exp", now.AddMinutes(expiresInMinutes).ToUnixTimeSeconds() } + }; + if (aud != null) + { + payload.Add("aud", aud); + } + + var header = new JwtHeader(new SigningCredentials(key ?? _key, SecurityAlgorithms.RsaSha256)); + return new JwtSecurityTokenHandler().WriteToken(new JwtSecurityToken(header, payload)); + } + + private bool Validate(string token, TrustedServer[] entries, string? globalAudience = null, string? iss = Issuer) + => JwksTokenValidator.TryValidate(token, "k1", iss, entries, HttpGet, globalAudience, NullLogger.Instance, out _); + + private static TrustedServer IssuerEntry(params string[] audiences) => new() { Issuer = Issuer, Audiences = audiences, Domain = "example.com" }; + + [Fact] + public void TrustedIssuer_IsAccepted_UsingOnlyConfiguredUrls() + { + var valid = JwksTokenValidator.TryValidate(Token(aud: "aasx-server"), "k1", Issuer, new[] { IssuerEntry("aasx-server") }, + HttpGet, null, NullLogger.Instance, out var domain); + + valid.Should().BeTrue(); + domain.Should().Be("example.com"); + _requested.Should().Equal(Issuer + "/.well-known/openid-configuration", JwksUri); + } + + [Fact] + public void UntrustedIssuer_IsRejected_WithoutHttpCall() + { + const string attacker = "https://attacker.example.com"; + _web[attacker + "/.well-known/openid-configuration"] = $"{{\"jwks_uri\":\"{JwksUri}\"}}"; + + Validate(Token(iss: attacker, aud: "aasx-server"), new[] { IssuerEntry("aasx-server") }, iss: attacker).Should().BeFalse(); + _requested.Should().BeEmpty(); + } + + [Fact] + public void WrongSigningKey_IsRejected() + { + Validate(Token(aud: "aasx-server", key: _otherKey), new[] { IssuerEntry("aasx-server") }).Should().BeFalse(); + } + + [Fact] + public void KidEntryWithIssuer_TokenWithOtherIssuer_IsRejected() + { + var entry = new TrustedServer { Kid = "k1", JwksUrl = JwksUri, Issuer = "https://configured.example.com" }; + + Validate(Token(iss: "https://other.example.com"), new[] { entry }, iss: "https://other.example.com").Should().BeFalse(); + } + + [Fact] + public void KidOnlyEntry_DoesNotValidateIssuer() + { + var entry = new TrustedServer { Kid = "k1", JwksUrl = JwksUri }; + + Validate(Token(iss: "https://any.example.com"), new[] { entry }, iss: "https://any.example.com").Should().BeTrue(); + } + + [Fact] + public void WrongAudience_IsRejected() + { + Validate(Token(aud: "account"), new[] { IssuerEntry("aasx-server") }).Should().BeFalse(); + } + + [Fact] + public void AudienceArray_ContainingExpectedAudience_IsAccepted() + { + Validate(Token(aud: new[] { "aasx-server", "account" }), new[] { IssuerEntry("aasx-server") }).Should().BeTrue(); + } + + [Fact] + public void NoAudienceConfigured_AudienceIsNotValidated() + { + Validate(Token(aud: "account"), new[] { IssuerEntry() }).Should().BeTrue(); + } + + [Fact] + public void GlobalAudience_IsUsedWhenEntryHasNone() + { + Validate(Token(aud: "account"), new[] { IssuerEntry() }, globalAudience: "aasx-server").Should().BeFalse(); + Validate(Token(aud: "aasx-server"), new[] { IssuerEntry() }, globalAudience: "x, aasx-server").Should().BeTrue(); + } + + [Fact] + public void EntryAudience_TakesPrecedenceOverGlobalAudience() + { + Validate(Token(aud: "aasx-server"), new[] { IssuerEntry("aasx-server") }, globalAudience: "other").Should().BeTrue(); + } + + [Fact] + public void ExpiredToken_IsRejected() + { + Validate(Token(aud: "aasx-server", expiresInMinutes: -10), new[] { IssuerEntry("aasx-server") }).Should().BeFalse(); + } + + [Fact] + public void MissingDiscovery_FallsBackToConfiguredIssuerJwks() + { + _web.Remove(Issuer + "/.well-known/openid-configuration"); + _web[Issuer + "/jwks"] = Jwks(_key); + + Validate(Token(aud: "aasx-server"), new[] { IssuerEntry("aasx-server") }).Should().BeTrue(); + _requested.Should().EndWith(Issuer + "/jwks"); + } +} diff --git a/src/AasxServerBlazor/accessrules.txt b/src/AasxServerBlazor/accessrules.txt index 5cd551f67..518d4e968 100644 --- a/src/AasxServerBlazor/accessrules.txt +++ b/src/AasxServerBlazor/accessrules.txt @@ -2,7 +2,40 @@ "AllAccessPermissionRules": { - "rules": [ + "rules": [ + { + "ACL": { + "ATTRIBUTES": [ + { + "CLAIM": "token:realm_access" + } + ], + "RIGHTS": [ + "READ", + "CREATE", + "UPDATE", + "DELETE" + ], + "ACCESS": "ALLOW" + }, + "OBJECTS": [ + { + "ROUTE": "*" + } + ], + "FORMULA": { + "$contains": [ + { + "$attribute": { + "CLAIM": "token:realm_access" + } + }, + { + "$strVal": "aas-writer" + } + ] + } + }, { diff --git a/src/AasxServerDB.Tests/AccessRulesCollection.cs b/src/AasxServerDB.Tests/AccessRulesCollection.cs new file mode 100644 index 000000000..1e89509e4 --- /dev/null +++ b/src/AasxServerDB.Tests/AccessRulesCollection.cs @@ -0,0 +1,15 @@ +namespace AasxServerDB.Tests; + +/// +/// Groups every test that parses access rules, so they cannot interleave over the shared statics +/// (QueryGrammarJSON._accessRules, QueryGrammarJSON.allAccessRuleSqlConditions, +/// SecurityService._accessRules) or over the current working directory that +/// ParseAccessRules has to switch. Assembly-wide parallelization is already off +/// (see AssemblyInfo.cs); this collection documents the intent and keeps it enforced if that +/// assembly-level switch is ever relaxed. +/// +[CollectionDefinition(AccessRulesCollection.Name, DisableParallelization = true)] +public sealed class AccessRulesCollection +{ + public const string Name = "AccessRules"; +} diff --git a/src/AasxServerDB.Tests/AccessRulesTestHost.cs b/src/AasxServerDB.Tests/AccessRulesTestHost.cs new file mode 100644 index 000000000..a2345ac5a --- /dev/null +++ b/src/AasxServerDB.Tests/AccessRulesTestHost.cs @@ -0,0 +1,63 @@ +namespace AasxServerDB.Tests; + +using AasSecurity; +using Contracts; + +/// +/// Parses an inline AllAccessPermissionRules JSON document into the process-wide access-rule +/// caches and hands back a handle that restores the previous values. +/// +/// Two pieces of global state have to be managed: QueryGrammarJSON.ParseAccessRules reads +/// jsonschema-access.txt from the current working directory (so the cwd has to point at +/// src/AasxServerBlazor for the duration of the parse), and the parsed result lands in public static +/// fields that outlive the test. Tests must dispose the handle so they cannot leak rules into each +/// other — see . +/// +/// +internal sealed class AccessRulesTestHost : IDisposable +{ + private readonly AllAccessPermissionRules? _previousGrammarRules; + private readonly SqlConditions? _previousAccumulator; + private readonly AllAccessPermissionRules? _previousServiceRules; + + private AccessRulesTestHost() + { + _previousGrammarRules = QueryGrammarJSON._accessRules; + _previousAccumulator = QueryGrammarJSON.allAccessRuleSqlConditions; + _previousServiceRules = SecurityService._accessRules; + } + + /// + /// Parses and publishes it to both + /// QueryGrammarJSON._accessRules and SecurityService._accessRules, the way + /// SecurityService.parseAccessRuleFile does for the real accessrules.txt. + /// + internal static AccessRulesTestHost Parse(string expression) + { + var host = new AccessRulesTestHost(); + + var blazorDir = Path.GetFullPath(Path.Combine( + AppContext.BaseDirectory, "..", "..", "..", "..", "AasxServerBlazor")); + var grammar = new QueryGrammarJSON(new NoSecurityRules()); + var originalCwd = Directory.GetCurrentDirectory(); + try + { + Directory.SetCurrentDirectory(blazorDir); + grammar.ParseAccessRules(expression); + } + finally + { + Directory.SetCurrentDirectory(originalCwd); + } + + SecurityService._accessRules = QueryGrammarJSON._accessRules; + return host; + } + + public void Dispose() + { + QueryGrammarJSON._accessRules = _previousGrammarRules!; + QueryGrammarJSON.allAccessRuleSqlConditions = _previousAccumulator; + SecurityService._accessRules = _previousServiceRules; + } +} diff --git a/src/AasxServerDB.Tests/AssemblyInfo.cs b/src/AasxServerDB.Tests/AssemblyInfo.cs new file mode 100644 index 000000000..0adfbaa94 --- /dev/null +++ b/src/AasxServerDB.Tests/AssemblyInfo.cs @@ -0,0 +1,11 @@ +using Xunit; + +// Several tests in this assembly mutate process-global state that cannot be scoped to a collection: +// * Directory.SetCurrentDirectory(...) — needed so QueryGrammarJSON.ParseAccessRules finds +// jsonschema-access.txt next to accessrules.txt in src/AasxServerBlazor. +// * the public static caches QueryGrammarJSON._accessRules, +// QueryGrammarJSON.allAccessRuleSqlConditions and SecurityService._accessRules. +// Collections only serialize tests *within* a collection, so parallel collections would still race +// on those. Disabling parallelization assembly-wide is the only airtight fix, and it costs close to +// nothing here because the suite is dominated by the serialized DatabaseFixture anyway. +[assembly: CollectionBehavior(DisableTestParallelization = true)] diff --git a/src/AasxServerDB.Tests/SecurityAccessRuleMergeTests.cs b/src/AasxServerDB.Tests/SecurityAccessRuleMergeTests.cs new file mode 100644 index 000000000..f51ed9b34 --- /dev/null +++ b/src/AasxServerDB.Tests/SecurityAccessRuleMergeTests.cs @@ -0,0 +1,303 @@ +namespace AasxServerDB.Tests; + +using System.Security.Claims; +using AasSecurity; +using Contracts; +using FluentAssertions; + +/// +/// End-to-end behaviour of / +/// GetSqlConditions when several rules match one request. +/// +/// Reproduces the reported "isSuperDuperUser sees no data" case: an unconditional grant +/// (FORMULA: {"$boolean": true}) matches alongside rules written for other subjects, and the +/// OR-combination has to let the unconditional grant win instead of dropping it and leaving the +/// other rules' predicates as the effective filter. +/// +/// The rule set is inline on purpose — src/AasxServerBlazor/accessrules.txt is edited by developers +/// and must not be a test fixture, even though these rules mirror its shapes. +/// +[Collection(AccessRulesCollection.Name)] +public sealed class SecurityAccessRuleMergeTests : IDisposable +{ + private const string SuperDuperUser = "isSuperDuperUser"; + private const string Route = "/submodels"; + private const string Read = "READ"; + + private readonly AccessRulesTestHost _accessRules; + private readonly SecurityService _service; + + public SecurityAccessRuleMergeTests() + { + // Construct the service first: its ctor's parseAccessRuleFile early-returns because the test + // bin directory has no accessrules.txt, so it cannot clobber what we publish afterwards. + _service = new SecurityService(); + _accessRules = AccessRulesTestHost.Parse(RuleSetJson); + } + + public void Dispose() => _accessRules.Dispose(); + + // --------------------------------------------------------------------------------------------- + // Premise: rule selection itself is correct — several rules legitimately match. + // --------------------------------------------------------------------------------------------- + + [Fact] + public void GetAccessRules_SuperUserWithTokenClaims_MatchesRealmSuperAndSubRules() + { + // A token: ATTRIBUTE matches on claim *presence* (the attribute's single slot already holds + // the claim type, so it cannot carry an expected value — that test lives in FORMULA). Hence + // the realm_access and sub rules apply to every token-authenticated user by design. + var rules = _service.GetAccessRules(SuperDuperUser, Read, Route, Claims("someOtherRole", "someone@other.com")); + + rules.Should().NotBeNull(); + rules!.Should().HaveCount(3, because: "the realm_access, isSuperDuperUser and token:sub rules all match"); + } + + [Fact] + public void ParsedUnconditionalRule_YieldsUnrestrictedConditions() + { + var rules = _service.GetAccessRules(SuperDuperUser, Read, Route, Claims("someOtherRole", "someone@other.com")); + + var unconditional = rules!.Where(r => SqlConditionsMerger.IsUnrestricted(r._formula_sqlConditions)).ToList(); + unconditional.Should().HaveCount(1, because: "FORMULA {\"$boolean\": true} places no restriction"); + unconditional[0]._filter_sqlConditions.Should().BeNull(); + } + + // --------------------------------------------------------------------------------------------- + // The reported bug. + // --------------------------------------------------------------------------------------------- + + [Fact] + public void GetSqlConditions_UnrestrictedRuleAmongMatches_ReturnsNull() + { + // Claim values are deliberately chosen so BOTH other rules evaluate to false: + // realm_access does not contain "isSuperDuperUser" and sub does not end with "xx.com". + // Only the unconditional isSuperDuperUser rule can grant here — so there must be no + // row-level restriction at all. + var conditions = _service.GetSqlConditions(SuperDuperUser, Read, Route, Claims("someOtherRole", "someone@other.com")); + + conditions.Should().BeNull( + because: "an unconditional grant among the matching rules absorbs the other rules' predicates"); + } + + [Fact] + public void GetSqlConditions_UnrestrictedRuleMatchesAlone_ReturnsNull() + { + // Same rule, no token claims at all — the single-rule path, where OrMerge never runs. + var conditions = _service.GetSqlConditions(SuperDuperUser, Read, Route, tokenClaims: null); + + conditions.Should().BeNull(); + } + + [Fact] + public void GetSqlConditions_RuleWithoutFormulaOrFilter_ReturnsNull() + { + var conditions = _service.GetSqlConditions("isAclOnlyUser", Read, Route, tokenClaims: null); + + _service.GetAccessRules("isAclOnlyUser", Read, Route, tokenClaims: null).Should().NotBeNull( + because: "the ACL-only rule must match, otherwise this test proves nothing"); + conditions.Should().BeNull(because: "a rule with neither FORMULA nor FILTER is an unconditional grant"); + } + + // --------------------------------------------------------------------------------------------- + // Guards against over-widening: without an unconditional grant nothing may change. + // --------------------------------------------------------------------------------------------- + + [Fact] + public void GetSqlConditions_OnlyRestrictiveRuleMatches_StillRestricts() + { + var conditions = _service.GetSqlConditions("", Read, Route, Claims(realmAccess: null, sub: "someone@other.com")); + + conditions.Should().NotBeNull(); + conditions!.FormulaConditions["sm"].Should().Contain("'*xx.com'") + .And.Contain("'Nameplate'") + .And.Contain("'TechnicalData'"); + } + + [Fact] + public void GetSqlConditions_TwoRestrictiveRules_OrCombinesOveralls() + { + var conditions = _service.GetSqlConditions("", Read, Route, Claims("someOtherRole", "someone@other.com")); + + conditions.Should().NotBeNull(); + conditions!.FormulaConditions["all"].Should().Contain(" OR "); + // Claim sentinels are substituted after merging, so both operands are non-empty at merge + // time and can never be mistaken for the neutral element. + conditions.FormulaConditions["sm"].Should().Contain("someOtherRole") + .And.Contain("'*xx.com'"); + } + + [Fact] + public void GetSqlConditions_NotAuthenticated_SingleRestrictiveRule_Unchanged() + { + var conditions = _service.GetSqlConditions("isNotAuthenticated", Read, Route, tokenClaims: null); + + conditions.Should().NotBeNull(); + conditions!.FormulaConditions["sm"].Should().Contain("'Nameplate'").And.Contain("'TechnicalData'"); + conditions.FormulaConditions["sme"].Should().Contain("'General*'").And.Contain("'Manufacturer*'"); + } + + // --------------------------------------------------------------------------------------------- + // Tree preview (TreePage.razor) — same defect class, separate code path. + // --------------------------------------------------------------------------------------------- + + [Fact] + public void EvaluateTreeSubmodelRead_UnrestrictedRuleMatches_ReturnsTrue() + { + // TreePage.razor calls the tree evaluators without tokenClaims, so only the + // isSuperDuperUser rule can match here. + _service.EvaluateTreeSubmodelRead(SuperDuperUser, Submodel("SomeOtherSubmodel"), Route).Should().BeTrue(); + } + + [Fact] + public void EvaluateTreeSubmodelRead_RuleWithoutFormula_ReturnsTrue() + { + _service.EvaluateTreeSubmodelRead("isAclOnlyUser", Submodel("SomeOtherSubmodel"), Route).Should().BeTrue( + because: "a rule with no FORMULA grants READ unconditionally, like FORMULA $boolean: true does"); + } + + [Fact] + public void EvaluateTreeSubmodelElementRead_RuleWithoutFormula_ReturnsTrue() + { + var sm = Submodel("SomeOtherSubmodel"); + + _service.EvaluateTreeSubmodelElementRead("isAclOnlyUser", sm, "SomeOtherSubmodel.SomeElement", Route) + .Should().BeTrue(); + } + + [Fact] + public void EvaluateTreeSubmodelRead_NotAuthenticated_StillFiltersBySubmodelIdShort() + { + _service.EvaluateTreeSubmodelRead("isNotAuthenticated", Submodel("Nameplate"), Route).Should().BeTrue(); + _service.EvaluateTreeSubmodelRead("isNotAuthenticated", Submodel("SomeOtherSubmodel"), Route).Should().BeFalse(); + } + + // --------------------------------------------------------------------------------------------- + // The global accumulator has the same merge bug. It has no production consumer today; this test + // pins the corrected semantics so nobody "fixes" it back. + // --------------------------------------------------------------------------------------------- + + [Fact] + public void ParseAccessRules_UnconditionalRuleAmongRules_MakesGlobalAccumulatorUnrestricted() + { + SqlConditionsMerger.IsUnrestricted(QueryGrammarJSON.allAccessRuleSqlConditions).Should().BeTrue(); + } + + // --------------------------------------------------------------------------------------------- + + private static List Claims(string? realmAccess, string? sub) + { + var claims = new List(); + if (realmAccess != null) + { + claims.Add(new Claim("token:realm_access", realmAccess)); + } + if (sub != null) + { + claims.Add(new Claim("token:sub", sub)); + } + return claims; + } + + private static AasCore.Aas3_1.Submodel Submodel(string idShort) + => new($"https://example.com/sm/{idShort}") + { + IdShort = idShort, + SubmodelElements = new List + { + new AasCore.Aas3_1.Property(AasCore.Aas3_1.DataTypeDefXsd.String) { IdShort = "SomeElement" } + } + }; + + /// + /// Mirrors the shapes in accessrules.txt: a claim-value rule, an unconditional role rule, a + /// per-subject rule gated only in FORMULA, the anonymous FORMULA+FILTER rule, and an ACL-only + /// rule with no FORMULA at all. + /// + private const string RuleSetJson = """ + { + "AllAccessPermissionRules": { + "rules": [ + { + "ACL": { + "ATTRIBUTES": [ { "CLAIM": "token:realm_access" } ], + "RIGHTS": [ "READ" ], + "ACCESS": "ALLOW" + }, + "OBJECTS": [ { "ROUTE": "*" } ], + "FORMULA": { + "$contains": [ + { "$attribute": { "CLAIM": "token:realm_access" } }, + { "$strVal": "isSuperDuperUser" } + ] + } + }, + { + "ACL": { + "ATTRIBUTES": [ { "CLAIM": "isSuperDuperUser" } ], + "RIGHTS": [ "READ" ], + "ACCESS": "ALLOW" + }, + "OBJECTS": [ { "ROUTE": "*" } ], + "FORMULA": { "$boolean": true } + }, + { + "ACL": { + "ATTRIBUTES": [ { "CLAIM": "token:sub" } ], + "RIGHTS": [ "READ" ], + "ACCESS": "ALLOW" + }, + "OBJECTS": [ { "ROUTE": "/submodels" } ], + "FORMULA": { + "$or": [ + { + "$and": [ + { "$ends-with": [ { "$attribute": { "CLAIM": "token:sub" } }, { "$strVal": "xx.com" } ] }, + { "$eq": [ { "$field": "$sm#idShort" }, { "$strVal": "Nameplate" } ] } + ] + }, + { + "$and": [ + { "$ends-with": [ { "$attribute": { "CLAIM": "token:sub" } }, { "$strVal": "xx.com" } ] }, + { "$eq": [ { "$field": "$sm#idShort" }, { "$strVal": "TechnicalData" } ] } + ] + } + ] + } + }, + { + "ACL": { + "ATTRIBUTES": [ { "CLAIM": "isNotAuthenticated" } ], + "RIGHTS": [ "READ" ], + "ACCESS": "ALLOW" + }, + "OBJECTS": [ { "ROUTE": "/submodels" } ], + "FORMULA": { + "$or": [ + { "$eq": [ { "$field": "$sm#idShort" }, { "$strVal": "Nameplate" } ] }, + { "$eq": [ { "$field": "$sm#idShort" }, { "$strVal": "TechnicalData" } ] } + ] + }, + "FILTER": { + "FRAGMENT": "xxx", + "CONDITION": { + "$or": [ + { "$starts-with": [ { "$field": "$sme#idShort" }, { "$strVal": "General" } ] }, + { "$starts-with": [ { "$field": "$sme#idShort" }, { "$strVal": "Manufacturer" } ] } + ] + } + } + }, + { + "ACL": { + "ATTRIBUTES": [ { "CLAIM": "isAclOnlyUser" } ], + "RIGHTS": [ "READ" ], + "ACCESS": "ALLOW" + }, + "OBJECTS": [ { "ROUTE": "/submodels" } ] + } + ] + } + } + """; +} diff --git a/src/AasxServerDB.Tests/SqlConditionsClaimScopeTests.cs b/src/AasxServerDB.Tests/SqlConditionsClaimScopeTests.cs index 2edfe3b38..43277bdb0 100644 --- a/src/AasxServerDB.Tests/SqlConditionsClaimScopeTests.cs +++ b/src/AasxServerDB.Tests/SqlConditionsClaimScopeTests.cs @@ -4,8 +4,13 @@ namespace AasxServerDB.Tests; using Contracts; using FluentAssertions; -public sealed class SqlConditionsClaimScopeTests +[Collection(AccessRulesCollection.Name)] +public sealed class SqlConditionsClaimScopeTests : IDisposable { + private AccessRulesTestHost? _accessRules; + + public void Dispose() => _accessRules?.Dispose(); + [Fact] public void CreateSqlConditions_ClaimOrSmCondition_KeepsClaimInSmScope() { @@ -215,20 +220,5 @@ private static LogicalExpression Eq(LogicalExpression left, LogicalExpression ri private static LogicalExpression StartsWith(LogicalExpression left, LogicalExpression right) => new() { ExpressionType = "$starts-with", ExpressionValue = new List { left, right } }; - private static void ParseAccessRules(string expression) - { - var blazorDir = Path.GetFullPath(Path.Combine( - AppContext.BaseDirectory, "..", "..", "..", "..", "AasxServerBlazor")); - var grammar = new QueryGrammarJSON(new NoSecurityRules()); - var originalCwd = Directory.GetCurrentDirectory(); - try - { - Directory.SetCurrentDirectory(blazorDir); - grammar.ParseAccessRules(expression); - } - finally - { - Directory.SetCurrentDirectory(originalCwd); - } - } + private void ParseAccessRules(string expression) => _accessRules = AccessRulesTestHost.Parse(expression); } diff --git a/src/AasxServerStandardBib/Services/LogSanitizer.cs b/src/AasxServerStandardBib/Services/LogSanitizer.cs index d58c3a874..18c184af7 100644 --- a/src/AasxServerStandardBib/Services/LogSanitizer.cs +++ b/src/AasxServerStandardBib/Services/LogSanitizer.cs @@ -11,7 +11,7 @@ * SPDX-License-Identifier: Apache-2.0 ********************************************************************************/ -using Environment = System.Environment; +using System.Linq; namespace AasxServerStandardBib.Services; @@ -21,10 +21,11 @@ namespace AasxServerStandardBib.Services; public static class LogSanitizer { /// - /// Sanitizes the input string for plain text logs by removing new line characters. - /// This helps prevent log forging by ensuring user input cannot inject new log entries. + /// Sanitizes the input string for plain text logs by removing all control characters + /// (new lines, tabs, escape sequences, ...). This helps prevent log forging by ensuring + /// user input cannot inject new log entries. It does not hide secrets: never log those. /// - /// The user input string to sanitize. - /// The sanitized string with new line characters removed. - public static string Sanitize(string input) => input.Replace(Environment.NewLine, "").Replace("\n", "").Replace("\r", ""); + /// The user input string to sanitize, may be null. + /// The sanitized string without control characters, or an empty string for null. + public static string Sanitize(string? input) => input == null ? "" : string.Concat(input.Where(c => !char.IsControl(c))); } \ No newline at end of file diff --git a/src/Contracts/SqlConditions.cs b/src/Contracts/SqlConditions.cs index 11eae82a6..7f9dba80f 100644 --- a/src/Contracts/SqlConditions.cs +++ b/src/Contracts/SqlConditions.cs @@ -287,9 +287,28 @@ public class PathJoin /// before being appended — this keeps every $$path{n}$$/$$match{n}$$ reference /// in the merged FormulaConditions["all"] unambiguous. /// +/// +/// An empty condition means "no restriction" = TRUE, so it is the neutral element under AND +/// () and the absorbing element under OR (). +/// See for why conflating the two silently drops an unconditional grant. +/// /// public static class SqlConditionsMerger { + /// + /// How two condition sets combine — decides what an empty operand means, which is the whole + /// difference between and . + /// + /// : empty is the NEUTRAL element, so the other side survives. + /// : empty is TRUE and therefore ABSORBS — the result is unrestricted. + /// + /// + private enum MergeOp + { + And, + Or + } + public static SqlConditions? Merge(SqlConditions? query, SqlConditions? security) { if (query == null && security == null) return null; @@ -299,7 +318,7 @@ public static class SqlConditionsMerger var merged = new SqlConditions(); // --- FormulaConditions: AND per key --- - MergeConditions(query.FormulaConditions, security.FormulaConditions, merged.FormulaConditions, "AND", skipAll: true); + MergeConditions(query.FormulaConditions, security.FormulaConditions, merged.FormulaConditions, MergeOp.And, skipAll: true); // --- Renumber security placeholders --- int pathOffset = query.Paths.Count; @@ -384,6 +403,13 @@ public static class SqlConditionsMerger /// OR-combines two — used when accumulating rules where any rule may match. /// FormulaConditions are OR-combined per key. /// Paths/Matches are renumbered and appended (same logic as ). + /// + /// An empty (or purely tautological) operand means that rule grants without restriction, so it + /// ABSORBS: the result is unrestricted, not the other operand. Falling back to the other operand + /// would silently drop the unconditional grant and leave an unrelated rule's predicate as the + /// effective filter. Same invariant as QueryGrammarJSON.BuildScopeSqlNode case + /// "$or" enforces for the branches of a single formula. + /// /// public static SqlConditions? OrMerge(SqlConditions? left, SqlConditions? right) { @@ -391,10 +417,27 @@ public static class SqlConditionsMerger if (left == null) return right; if (right == null) return left; + // Emptiness is unaffected by the placeholder renumbering below, so this can be decided up front. + if (string.IsNullOrWhiteSpace(NormalizeNeutralCondition(left.FormulaConditions.GetValueOrDefault("all", ""))) || + string.IsNullOrWhiteSpace(NormalizeNeutralCondition(right.FormulaConditions.GetValueOrDefault("all", "")))) + { + // One side grants unconditionally, so the disjunction is unrestricted. Every scope + // prefilter has to go too: a prefilter not implied by the overall condition would drop + // rows the unconditional side allows. And with the overall condition gone, every + // $$path/$$match/$$exists placeholder is unreferenced — leaving those joins attached + // would disable the direct fast paths and let a stale EXISTS pattern suppress the + // trigram index of an unrelated user query (see Query.ClassifyCommonValueMatches). + // A fresh bag is exactly "no restriction": all scope keys "" and no joins. + return new SqlConditions + { + Select = !string.IsNullOrWhiteSpace(left.Select) ? left.Select : right.Select ?? "" + }; + } + var merged = new SqlConditions(); // --- FormulaConditions: OR per key --- - MergeConditions(left.FormulaConditions, right.FormulaConditions, merged.FormulaConditions, "OR", skipAll: true); + MergeConditions(left.FormulaConditions, right.FormulaConditions, merged.FormulaConditions, MergeOp.Or, skipAll: true); // --- Renumber right placeholders --- int pathOffset = left.Paths.Count; @@ -410,12 +453,16 @@ public static class SqlConditionsMerger rightOverall = rightOverall.Replace($"$$exists{i}$$", $"$$exists{i + existsOffset}$$"); // --- FormulaConditions["all"]: OR-combine --- + // Both sides are known non-empty here (the absorb case returned early), but keep the + // absorbing rule local so the invariant survives future refactors. var lOver = NormalizeNeutralCondition(left.FormulaConditions.GetValueOrDefault("all", "")); rightOverall = NormalizeNeutralCondition(rightOverall); merged.FormulaConditions["all"] = - !string.IsNullOrWhiteSpace(lOver) && !string.IsNullOrWhiteSpace(rightOverall) - ? $"({lOver}) OR ({rightOverall})" - : string.IsNullOrWhiteSpace(lOver) ? rightOverall : lOver; + string.IsNullOrWhiteSpace(lOver) || string.IsNullOrWhiteSpace(rightOverall) + ? "" + : string.Equals(lOver, rightOverall, StringComparison.Ordinal) + ? lOver + : $"({lOver}) OR ({rightOverall})"; // --- Paths/Matches: left first, then renumbered right --- foreach (var p in left.Paths) merged.Paths.Add(p); @@ -457,7 +504,7 @@ private static void MergeConditions( Dictionary left, Dictionary right, Dictionary target, - string op, + MergeOp op, bool skipAll = false) { var allKeys = left.Keys.Union(right.Keys); @@ -468,20 +515,45 @@ private static void MergeConditions( var lVal = NormalizeNeutralCondition(left.GetValueOrDefault(key, "")); var rVal = NormalizeNeutralCondition(right.GetValueOrDefault(key, "")); + + if (op == MergeOp.Or && (string.IsNullOrWhiteSpace(lVal) || string.IsNullOrWhiteSpace(rVal))) + { + // "" means "no restriction for this scope" = TRUE, and TRUE absorbs under OR. A scope + // prefilter is only sound if it is a NECESSARY condition of the disjunction, so if one + // side constrains this scope and the other does not, the scope cannot be prefiltered + // at all — only the "all" condition may decide. + target[key] = ""; + continue; + } + if (!string.IsNullOrWhiteSpace(lVal) && !string.IsNullOrWhiteSpace(rVal)) { // Avoid (X) OR (X) / (X) AND (X) when merging duplicate rule fragments (same string). target[key] = string.Equals(lVal, rVal, StringComparison.Ordinal) ? lVal - : $"({lVal}) {op} ({rVal})"; + : $"({lVal}) {(op == MergeOp.And ? "AND" : "OR")} ({rVal})"; } else { + // AND: empty is the neutral element, so the non-empty side survives unchanged. target[key] = string.IsNullOrWhiteSpace(lVal) ? rVal : lVal; } } } + /// + /// True if this bag places no restriction at all: every scope condition is empty or a pure + /// tautology and no path/match/exists join is attached. That is what an access rule with + /// FORMULA: {"$boolean": true} (or with no FORMULA/FILTER at all) produces, i.e. an + /// unconditional grant — see for why such an operand absorbs. + /// null counts as unrestricted, matching the "no restriction" meaning of a null bag + /// throughout the query and security paths. + /// + public static bool IsUnrestricted(SqlConditions? sc) + => sc == null + || (sc.Paths.Count == 0 && sc.Matches.Count == 0 && sc.ExistsConditions.Count == 0 + && sc.FormulaConditions.Values.All(v => string.IsNullOrWhiteSpace(NormalizeNeutralCondition(v)))); + /// /// Empty, 1=1, or parenthesized tautologies only (e.g. (1=1), ((1=1))) — for AND/OR merges so query $boolean: true does not add noise. ///