diff --git a/ZB.MOM.WW.Auth/CLAUDE.md b/ZB.MOM.WW.Auth/CLAUDE.md index 562d6d7..20e2ae3 100644 --- a/ZB.MOM.WW.Auth/CLAUDE.md +++ b/ZB.MOM.WW.Auth/CLAUDE.md @@ -44,6 +44,14 @@ MxAccessGateway & HistorianGateway → the original four; ScadaBridge → those **`0.4.4` (unpublished, scadaproj#40, Auth-007):** `BindMode=User` now proves the entry whose groups it returns is the account that bound — RFC 4532 Who-am-I (`dn:` → DN match; AD `u:DOMAIN\name` → `sAMAccountName` match), falling back to the bind DN only for a DN template (GLAuth has no Who-am-I); otherwise `GroupLookupFailed`. `{0}` is RFC 4514-escaped in DN templates. Before the fix a user could sign in with another account's groups whenever the UPN/DN bind and the `UserNameAttribute` search named different accounts — reproduced live against a throwaway GLAuth (`tests/ZB.MOM.WW.Auth.Ldap.Tests/Integration/glauth-userbind/`, `ZB_LDAP_USERBIND_IT=1`, port 3897): pre-fix `mallory` signed in with `Admins`. **Consumer impact:** AD + down-level/UPN template unchanged for consistent accounts (BTDB Manager is the only user-bind consumer); non-AD user-bind needs a DN template that yields the entry's DN exactly as searched (GLAuth: `cn={0},ou=,ou=users,`). README §0.4.4. +**`0.4.4`, continued (unpublished, scadaproj#46 + #50):** still `0.4.4`. +- **Auth-008 (`ApiKeys.SqlServer`):** over-length key records are refused with `ArgumentException`; an over-64 key-id lookup answers not-found without querying; every `key_id` predicate adds an exact `VARBINARY` match, so lookups are case- and trailing-space-exact like SQLite. No schema change. `ApiKeyImportResult` gains the `init` lists `KeyIdCollisions` and `KeysRejected`, and `KeysSkipped` now counts exact duplicates only. Audit text is still truncated to 64 characters, deliberately (documented). +- **Auth-009:** the canonical username is the `UserNameAttribute` value matching the typed name, ignoring case, not `values[0]`. +- **Auth-010:** the new `ApiKeyFailure.StoreUnavailable` (appended) is returned when `FindByKeyIdAsync` throws; only cancellation still throws. +- **#50 (user bind, AD forests):** for a `u:DOMAIN\name` Who-am-I answer, the entry must be that domain's account. Where AD offers `msDS-PrincipalName` (a base read through the new internal `ILdapConnection.Read`), it must equal the answer. Otherwise the name must have exactly one holder under `SearchBase`. Not live-verified against AD. +- **Consumer-facing:** a new enum value; SQL Server key ids now match exactly; over-length creates throw; the importer reports collisions and rejections. +- Suites: 363 passed / 81 skipped offline; 440 passed / 4 skipped (LDAP live) with `AUTH_SQLSERVER_CONNSTR`, against a throwaway SQL Server 2022. + **`0.4.3` (unpublished, scadaproj#26):** `Microsoft.Data.Sqlite` 10.0.7 -> 10.0.13 (efcore#39009 pool fault); `SQLitePCLRaw.lib.e_sqlite3` transitive pin 2.1.12 -> 2.1.13 (10.0.13 floors 2.1.13). No API change. **`0.4.2` (unpublished, scadaproj#23 + #22):** `ZB.MOM.WW.Auth.ApiKeys.ApiKeyPepperUnavailableException : InvalidOperationException` (message still `"pepper unavailable"`) is now thrown by `ApiKeyAdminCommands` create/rotate when the pepper is missing, so consumers can catch it by type; existing `catch (InvalidOperationException)` blocks keep working (only exact-type assertions like xunit `ThrowsAsync` need updating). **Release note: consumers can drop their own `IApiKeyPepperProvider` pre-checks** — mxaccessgw's `DashboardApiKeyManagementService` and any similar code. Also pins `System.Security.Cryptography.Pkcs 9.0.21` (transitive, via SqlClient 6.0.2) to clear NuGetAudit NU1902/NU1903. Not yet published to the feed. diff --git a/ZB.MOM.WW.Auth/README.md b/ZB.MOM.WW.Auth/README.md index fa430b4..30e6d38 100644 --- a/ZB.MOM.WW.Auth/README.md +++ b/ZB.MOM.WW.Auth/README.md @@ -46,6 +46,11 @@ All five packages are versioned **lockstep** from `Directory.Build.props`. The c - otherwise the name must have exactly one holder under `SearchBase`, and it must be the entry found — no extra search is made when `UserNameAttribute` is `sAMAccountName` and the user typed the bound name, because the user search already proved that. Two holders → `GroupLookupFailed`. - A single-domain AD (BTDB Manager) is unaffected. Not yet live-verified against AD — the fake connection covers the paths; GLAuth has no Who-am-I. - **`ApiKeyFailure.StoreUnavailable` (Auth-010, `Abstractions` addition).** `ApiKeyVerifier.VerifyAsync` no longer lets a key-store fault escape: a `FindByKeyIdAsync` that throws (shared SQL Server unreachable, a SQLite I/O fault) returns `Succeeded = false` with the new `StoreUnavailable` failure, so it takes the host's normal — audited — auth-failure path instead of an unhandled 500 / gRPC `Unknown`. Cancellation still throws. The value is appended, so existing numeric values are unchanged; a host may answer it with 503 rather than 401. A misconfigured `SqlServerApiKeyOptions` still throws when the store is first resolved (outside `VerifyAsync`). +- **SQL Server store: no truncation, exact key ids (Auth-008).** The `ApiKeys.SqlServer` stores now behave like the SQLite store they port: + - `CreateAsync` throws `ArgumentException` for a key id over 64, prefix over 16 or display name over 256 characters (it used to store them silently truncated), and every lookup of an over-64-character key id answers "not found" without querying (a truncated parameter could match a different key sharing its first 64 characters). + - Key-id matching is **exact** — case- and trailing-space-sensitive — via a binary comparison added to every `key_id` predicate; the primary-key seek is unchanged and there is **no schema change**. A token presented with a case-altered key id (`LINE1` for `line1`) now fails `KeyNotFound` instead of resolving. + - `SqliteApiKeyStoreImporter` reports instead of hiding: `ApiKeyImportResult.KeyIdCollisions` lists source keys whose id differs only in case from one already in SQL Server (the primary key follows the database collation and cannot hold both — re-mint those keys), and `KeysRejected` lists over-length keys; neither is counted in `KeysSkipped` any more. Both are new `init` properties, so existing code is unaffected. + - Audit text (`key_id`, `event_type`, `remote_address`) is still truncated to 64 characters — deliberately, and now documented: an audit append runs after the action it records and must not fail it. - **Canonical username from a multi-valued attribute (Auth-009).** `LdapAuthResult.Username` is now the `UserNameAttribute` value that equals the typed name ignoring case — `cn: [John Smith, jsmith]` signs `jsmith` in as `jsmith`, not whichever value the server lists first. The first value is used only when none matches. Both bind modes. ### 0.4.1 — LDAP review fixes diff --git a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys.SqlServer/SqlServerApiKeyAdminStore.cs b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys.SqlServer/SqlServerApiKeyAdminStore.cs index d5310e4..d850f64 100644 --- a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys.SqlServer/SqlServerApiKeyAdminStore.cs +++ b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys.SqlServer/SqlServerApiKeyAdminStore.cs @@ -23,11 +23,17 @@ public sealed class SqlServerApiKeyAdminStore(AuthSqlServerConnectionFactory con /// /// A plain INSERT: a duplicate key_id surfaces as a primary-key /// rather than silently overwriting an existing key (the SQLite store - /// raises its own SqliteException in the same situation). + /// raises its own SqliteException in the same situation). Unlike SQLite, the primary key + /// follows the database collation, so on a case-insensitive database Line1 cannot be + /// created beside line1 either. A key id over 64, prefix over 16 or display name over 256 + /// characters throws before anything is sent (since 0.4.4; it + /// was silently truncated before). /// + /// A field is longer than its column. public async Task CreateAsync(ApiKeyRecord record, CancellationToken ct) { ArgumentNullException.ThrowIfNull(record); + SqlServerColumnLimits.EnsureFits(record); string schema = connectionFactory.Schema; @@ -65,6 +71,8 @@ public sealed class SqlServerApiKeyAdminStore(AuthSqlServerConnectionFactory con public async Task RevokeAsync(string keyId, DateTimeOffset whenUtc, CancellationToken ct) { ArgumentException.ThrowIfNullOrWhiteSpace(keyId); + if (!SqlServerColumnLimits.CanExist(keyId)) + return false; string schema = connectionFactory.Schema; @@ -74,7 +82,7 @@ public sealed class SqlServerApiKeyAdminStore(AuthSqlServerConnectionFactory con await using SqlCommand command = connectionFactory.CreateCommand(connection, $""" UPDATE [{schema}].[{SqlServerAuthSchema.ApiKeysTable}] SET revoked_utc = @revoked_utc - WHERE key_id = @key_id AND revoked_utc IS NULL; + WHERE key_id = @key_id AND {SqlServerColumnLimits.ExactKeyIdMatch} AND revoked_utc IS NULL; """); command.Parameters.Add("@key_id", SqlDbType.NVarChar, 64).Value = keyId; command.Parameters.Add("@revoked_utc", SqlDbType.DateTimeOffset).Value = whenUtc; @@ -93,6 +101,8 @@ public sealed class SqlServerApiKeyAdminStore(AuthSqlServerConnectionFactory con public async Task RotateAsync(string keyId, byte[] newSecretHash, CancellationToken ct) { ArgumentException.ThrowIfNullOrWhiteSpace(keyId); + if (!SqlServerColumnLimits.CanExist(keyId)) + return false; ArgumentNullException.ThrowIfNull(newSecretHash); string schema = connectionFactory.Schema; @@ -105,7 +115,7 @@ public sealed class SqlServerApiKeyAdminStore(AuthSqlServerConnectionFactory con SET secret_hash = @secret_hash, last_used_utc = NULL, revoked_utc = NULL - WHERE key_id = @key_id; + WHERE key_id = @key_id AND {SqlServerColumnLimits.ExactKeyIdMatch}; """); command.Parameters.Add("@key_id", SqlDbType.NVarChar, 64).Value = keyId; command.Parameters.Add("@secret_hash", SqlDbType.VarBinary, 64).Value = newSecretHash; @@ -119,6 +129,8 @@ public sealed class SqlServerApiKeyAdminStore(AuthSqlServerConnectionFactory con public async Task SetScopesAsync(string keyId, IReadOnlySet scopes, CancellationToken ct) { ArgumentException.ThrowIfNullOrWhiteSpace(keyId); + if (!SqlServerColumnLimits.CanExist(keyId)) + return false; ArgumentNullException.ThrowIfNull(scopes); string schema = connectionFactory.Schema; @@ -129,7 +141,7 @@ public sealed class SqlServerApiKeyAdminStore(AuthSqlServerConnectionFactory con await using SqlCommand command = connectionFactory.CreateCommand(connection, $""" UPDATE [{schema}].[{SqlServerAuthSchema.ApiKeysTable}] SET scopes = @scopes - WHERE key_id = @key_id; + WHERE key_id = @key_id AND {SqlServerColumnLimits.ExactKeyIdMatch}; """); command.Parameters.Add("@key_id", SqlDbType.NVarChar, 64).Value = keyId; command.Parameters.Add("@scopes", SqlDbType.NVarChar, -1).Value = ScopeSerializer.Serialize(scopes); @@ -143,6 +155,8 @@ public sealed class SqlServerApiKeyAdminStore(AuthSqlServerConnectionFactory con public async Task SetEnabledAsync(string keyId, bool enabled, DateTimeOffset whenUtc, CancellationToken ct) { ArgumentException.ThrowIfNullOrWhiteSpace(keyId); + if (!SqlServerColumnLimits.CanExist(keyId)) + return false; string schema = connectionFactory.Schema; @@ -159,7 +173,7 @@ public sealed class SqlServerApiKeyAdminStore(AuthSqlServerConnectionFactory con command = connectionFactory.CreateCommand(connection, $""" UPDATE [{schema}].[{SqlServerAuthSchema.ApiKeysTable}] SET revoked_utc = NULL - WHERE key_id = @key_id; + WHERE key_id = @key_id AND {SqlServerColumnLimits.ExactKeyIdMatch}; """); command.Parameters.Add("@key_id", SqlDbType.NVarChar, 64).Value = keyId; } @@ -168,7 +182,7 @@ public sealed class SqlServerApiKeyAdminStore(AuthSqlServerConnectionFactory con command = connectionFactory.CreateCommand(connection, $""" UPDATE [{schema}].[{SqlServerAuthSchema.ApiKeysTable}] SET revoked_utc = @revoked_utc - WHERE key_id = @key_id; + WHERE key_id = @key_id AND {SqlServerColumnLimits.ExactKeyIdMatch}; """); command.Parameters.Add("@key_id", SqlDbType.NVarChar, 64).Value = keyId; command.Parameters.Add("@revoked_utc", SqlDbType.DateTimeOffset).Value = whenUtc; @@ -190,6 +204,8 @@ public sealed class SqlServerApiKeyAdminStore(AuthSqlServerConnectionFactory con public async Task DeleteAsync(string keyId, CancellationToken ct) { ArgumentException.ThrowIfNullOrWhiteSpace(keyId); + if (!SqlServerColumnLimits.CanExist(keyId)) + return false; string schema = connectionFactory.Schema; @@ -198,7 +214,7 @@ public sealed class SqlServerApiKeyAdminStore(AuthSqlServerConnectionFactory con await using SqlCommand command = connectionFactory.CreateCommand(connection, $""" DELETE FROM [{schema}].[{SqlServerAuthSchema.ApiKeysTable}] - WHERE key_id = @key_id AND revoked_utc IS NOT NULL; + WHERE key_id = @key_id AND {SqlServerColumnLimits.ExactKeyIdMatch} AND revoked_utc IS NOT NULL; """); command.Parameters.Add("@key_id", SqlDbType.NVarChar, 64).Value = keyId; diff --git a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys.SqlServer/SqlServerApiKeyAuditStore.cs b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys.SqlServer/SqlServerApiKeyAuditStore.cs index ae82e4f..39e8aa6 100644 --- a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys.SqlServer/SqlServerApiKeyAuditStore.cs +++ b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys.SqlServer/SqlServerApiKeyAuditStore.cs @@ -13,6 +13,14 @@ namespace ZB.MOM.WW.Auth.ApiKeys.SqlServer; public sealed class SqlServerApiKeyAuditStore(AuthSqlServerConnectionFactory connectionFactory) : IApiKeyAuditStore { /// + /// + /// Over-length text is truncated, deliberately. key_id, event_type and + /// remote_address are 64-character columns; a longer value is stored as its first 64 + /// characters (the SQLite store keeps it whole). Unlike a key record (refused when over-length, + /// see ), an audit append is not refused: it + /// runs after the action it records, and failing it would lose the record of a change that has + /// already happened. details is unbounded. + /// public async Task AppendAsync(ApiKeyAuditEntry entry, CancellationToken ct) { ArgumentNullException.ThrowIfNull(entry); diff --git a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys.SqlServer/SqlServerApiKeyStore.cs b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys.SqlServer/SqlServerApiKeyStore.cs index bbdef9d..9a6a5ba 100644 --- a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys.SqlServer/SqlServerApiKeyStore.cs +++ b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys.SqlServer/SqlServerApiKeyStore.cs @@ -42,6 +42,8 @@ public sealed class SqlServerApiKeyStore(AuthSqlServerConnectionFactory connecti public async Task MarkUsedAsync(string keyId, DateTimeOffset whenUtc, CancellationToken ct) { ArgumentException.ThrowIfNullOrWhiteSpace(keyId); + if (!SqlServerColumnLimits.CanExist(keyId)) + return; string schema = connectionFactory.Schema; @@ -51,7 +53,7 @@ public sealed class SqlServerApiKeyStore(AuthSqlServerConnectionFactory connecti await using SqlCommand command = connectionFactory.CreateCommand(connection, $""" UPDATE [{schema}].[{SqlServerAuthSchema.ApiKeysTable}] SET last_used_utc = @last_used_utc - WHERE key_id = @key_id AND revoked_utc IS NULL; + WHERE key_id = @key_id AND {SqlServerColumnLimits.ExactKeyIdMatch} AND revoked_utc IS NULL; """); command.Parameters.Add("@key_id", SqlDbType.NVarChar, 64).Value = keyId; command.Parameters.Add("@last_used_utc", SqlDbType.DateTimeOffset).Value = whenUtc; @@ -61,6 +63,11 @@ public sealed class SqlServerApiKeyStore(AuthSqlServerConnectionFactory connecti private async Task FindAsync(string keyId, bool requireActive, CancellationToken ct) { + // An id longer than the column cannot be stored; querying would truncate it and could match + // another key sharing its first 64 characters (Auth-008). + if (!SqlServerColumnLimits.CanExist(keyId)) + return null; + string schema = connectionFactory.Schema; string table = $"[{schema}].[{SqlServerAuthSchema.ApiKeysTable}]"; @@ -72,8 +79,8 @@ public sealed class SqlServerApiKeyStore(AuthSqlServerConnectionFactory connecti await using SqlCommand command = connectionFactory.CreateCommand( connection, requireActive - ? $"SELECT {SelectColumns} FROM {table} WHERE key_id = @key_id AND revoked_utc IS NULL;" - : $"SELECT {SelectColumns} FROM {table} WHERE key_id = @key_id;"); + ? $"SELECT {SelectColumns} FROM {table} WHERE key_id = @key_id AND {SqlServerColumnLimits.ExactKeyIdMatch} AND revoked_utc IS NULL;" + : $"SELECT {SelectColumns} FROM {table} WHERE key_id = @key_id AND {SqlServerColumnLimits.ExactKeyIdMatch};"); command.Parameters.Add("@key_id", SqlDbType.NVarChar, 64).Value = keyId; await using SqlDataReader reader = await command.ExecuteReaderAsync(ct).ConfigureAwait(false); diff --git a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys.SqlServer/SqlServerColumnLimits.cs b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys.SqlServer/SqlServerColumnLimits.cs new file mode 100644 index 0000000..2f569b6 --- /dev/null +++ b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys.SqlServer/SqlServerColumnLimits.cs @@ -0,0 +1,61 @@ +using ZB.MOM.WW.Auth.Abstractions.ApiKeys; + +namespace ZB.MOM.WW.Auth.ApiKeys.SqlServer; + +/// +/// Keeps the SQL Server stores' key-id handling identical to the SQLite store they port (Auth-008): +/// no silent truncation and exact key-id matching. +/// +/// +/// +/// Lengths. Every parameter carries an explicit size, and SqlClient truncates an over-length +/// value to that size without an error. So an over-length key record is refused up front, and a +/// lookup of an over-length key id answers "not found" without querying — such a key cannot exist, +/// and the truncated parameter would otherwise match a different key that shares its first 64 +/// characters. +/// +/// +/// Case. The key_id column uses the database's collation, case-insensitive on a stock +/// SQL Server, whereas SQLite compares key ids byte-for-byte. Changing the column's collation would +/// need a schema migration of a primary key on a live shared store, so every key_id predicate +/// instead adds : the collation-aware = still drives the primary-key +/// seek, and the binary comparison then rejects a row that differs in case or trailing spaces. +/// +/// +internal static class SqlServerColumnLimits +{ + /// Width of api_keys.key_id (and api_key_audit.key_id). + public const int KeyId = 64; + + /// Width of api_keys.key_prefix. + public const int KeyPrefix = 16; + + /// Width of api_keys.display_name. + public const int DisplayName = 256; + + /// + /// Appended to every key_id = @key_id. NVARCHAR → VARBINARY yields the UTF-16 + /// bytes, so the comparison is exact (case, accents and trailing spaces all significant). + /// + public const string ExactKeyIdMatch = "CAST(key_id AS VARBINARY(128)) = CAST(@key_id AS VARBINARY(128))"; + + /// Whether a key with this id could be stored at all. + public static bool CanExist(string keyId) => keyId.Length <= KeyId; + + /// The first over-length field of , described, or when every field fits. + public static string? OverLengthField(ApiKeyRecord record) => + record.KeyId.Length > KeyId ? Describe(nameof(ApiKeyRecord.KeyId), record.KeyId.Length, KeyId) + : record.KeyPrefix.Length > KeyPrefix ? Describe(nameof(ApiKeyRecord.KeyPrefix), record.KeyPrefix.Length, KeyPrefix) + : record.DisplayName.Length > DisplayName ? Describe(nameof(ApiKeyRecord.DisplayName), record.DisplayName.Length, DisplayName) + : null; + + /// Throws when a field of exceeds its column. + public static void EnsureFits(ApiKeyRecord record) + { + if (OverLengthField(record) is { } problem) + throw new ArgumentException($"API key record cannot be stored in SQL Server: {problem}.", nameof(record)); + } + + private static string Describe(string field, int length, int max) => + $"{field} is {length} characters, more than the column's {max}"; +} diff --git a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys.SqlServer/SqliteApiKeyStoreImporter.cs b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys.SqlServer/SqliteApiKeyStoreImporter.cs index 59c1ee8..b109bac 100644 --- a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys.SqlServer/SqliteApiKeyStoreImporter.cs +++ b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys.SqlServer/SqliteApiKeyStoreImporter.cs @@ -13,7 +13,26 @@ namespace ZB.MOM.WW.Auth.ApiKeys.SqlServer; /// Audit rows that already existed in SQL Server. /// Whether the SQLite file existed at all. public sealed record ApiKeyImportResult( - int KeysInserted, int KeysSkipped, int AuditInserted, int AuditSkipped, bool SourceFound); + int KeysInserted, int KeysSkipped, int AuditInserted, int AuditSkipped, bool SourceFound) +{ + /// + /// Source key ids that were NOT imported because SQL Server already holds a key whose id differs + /// only in case (since 0.4.4, Auth-008). SQLite compares key ids exactly, so Line1 and + /// line1 can both exist there; the SQL Server key_id primary key uses the database's + /// collation (case-insensitive by default) and cannot hold both. Such a key's token will not + /// verify against the shared store — re-mint it under a distinct id. Not counted in + /// , which counts only exact duplicates. + /// + public IReadOnlyList KeyIdCollisions { get; init; } = []; + + /// + /// Source key ids that were NOT imported because a value exceeds its SQL Server column + /// (key_id 64, key_prefix 16, display_name 256 characters) (since 0.4.4, + /// Auth-008). Up to 0.4.3 such a row was imported silently truncated. Not counted in + /// . + /// + public IReadOnlyList KeysRejected { get; init; } = []; +} /// /// Copies every key and audit row from a per-node SQLite store into the shared SQL Server store, @@ -208,13 +227,32 @@ public sealed class SqliteApiKeyStoreImporter(AuthSqlServerConnectionFactory sql (SqlTransaction)await connection.BeginTransactionAsync(ct).ConfigureAwait(false); int keysInserted = 0; + int keysSkipped = 0; int auditInserted = 0; + List collisions = []; + List rejected = []; foreach (ApiKeyRecord key in keys) { - if (await InsertKeyIfAbsentAsync(connection, transaction, schema, key, ct).ConfigureAwait(false)) + // Refused rather than written truncated: a truncated key_id would verify only through an + // equally truncated lookup and authenticate as an id nobody assigned (Auth-008). + if (SqlServerColumnLimits.OverLengthField(key) is not null) { - keysInserted++; + rejected.Add(key.KeyId); + continue; + } + + switch (await InsertKeyIfAbsentAsync(connection, transaction, schema, key, ct).ConfigureAwait(false)) + { + case KeyInsertOutcome.Inserted: + keysInserted++; + break; + case KeyInsertOutcome.AlreadyPresent: + keysSkipped++; + break; + default: + collisions.Add(key.KeyId); + break; } } @@ -230,15 +268,28 @@ public sealed class SqliteApiKeyStoreImporter(AuthSqlServerConnectionFactory sql return new ApiKeyImportResult( KeysInserted: keysInserted, - KeysSkipped: keys.Count - keysInserted, + KeysSkipped: keysSkipped, AuditInserted: auditInserted, AuditSkipped: auditEntries.Count - auditInserted, - SourceFound: true); + SourceFound: true) + { + KeyIdCollisions = collisions, + KeysRejected = rejected, + }; } - // Returns true when this call inserted the row. The guard is inside the statement rather than a - // separate SELECT so the check and the insert cannot straddle a concurrent writer. - private async Task InsertKeyIfAbsentAsync( + private enum KeyInsertOutcome + { + Inserted = 1, + AlreadyPresent = 0, + CaseVariantPresent = 2, + } + + // The guard is inside the statement rather than a separate SELECT so the check and the insert + // cannot straddle a concurrent writer. The collation-aware `key_id = @key_id` finds a row whose + // id differs only in case — the primary key would refuse to hold both — and the exact match tells + // that collision apart from a genuine duplicate. + private async Task InsertKeyIfAbsentAsync( SqlConnection connection, SqlTransaction transaction, string schema, @@ -246,14 +297,23 @@ public sealed class SqliteApiKeyStoreImporter(AuthSqlServerConnectionFactory sql CancellationToken ct) { await using SqlCommand command = sql.CreateCommand(connection, $""" - IF NOT EXISTS ( + IF EXISTS ( + SELECT 1 FROM [{schema}].[{SqlServerAuthSchema.ApiKeysTable}] + WHERE key_id = @key_id AND {SqlServerColumnLimits.ExactKeyIdMatch}) + SELECT 0; + ELSE IF EXISTS ( SELECT 1 FROM [{schema}].[{SqlServerAuthSchema.ApiKeysTable}] WHERE key_id = @key_id) + SELECT 2; + ELSE + BEGIN INSERT INTO [{schema}].[{SqlServerAuthSchema.ApiKeysTable}] ( key_id, key_prefix, secret_hash, display_name, scopes, constraints, created_utc, last_used_utc, revoked_utc, expires_utc) VALUES ( @key_id, @key_prefix, @secret_hash, @display_name, @scopes, @constraints, @created_utc, @last_used_utc, @revoked_utc, @expires_utc); + SELECT 1; + END """, transaction); command.Parameters.Add("@key_id", SqlDbType.NVarChar, 64).Value = key.KeyId; command.Parameters.Add("@key_prefix", SqlDbType.NVarChar, 16).Value = key.KeyPrefix; @@ -266,7 +326,8 @@ public sealed class SqliteApiKeyStoreImporter(AuthSqlServerConnectionFactory sql command.Parameters.Add("@revoked_utc", SqlDbType.DateTimeOffset).Value = (object?)key.RevokedUtc ?? DBNull.Value; command.Parameters.Add("@expires_utc", SqlDbType.DateTimeOffset).Value = (object?)key.ExpiresUtc ?? DBNull.Value; - return await command.ExecuteNonQueryAsync(ct).ConfigureAwait(false) > 0; + object? outcome = await command.ExecuteScalarAsync(ct).ConfigureAwait(false); + return (KeyInsertOutcome)Convert.ToInt32(outcome, System.Globalization.CultureInfo.InvariantCulture); } // Audit rows carry no natural key (audit_id is per-file IDENTITY), so identity is the triple diff --git a/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.ApiKeys.SqlServer.Tests/Live/KeyIdFidelityLiveTests.cs b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.ApiKeys.SqlServer.Tests/Live/KeyIdFidelityLiveTests.cs new file mode 100644 index 0000000..253af8b --- /dev/null +++ b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.ApiKeys.SqlServer.Tests/Live/KeyIdFidelityLiveTests.cs @@ -0,0 +1,114 @@ +using Microsoft.Data.Sqlite; +using ZB.MOM.WW.Auth.Abstractions.ApiKeys; +using ZB.MOM.WW.Auth.ApiKeys.Sqlite; + +namespace ZB.MOM.WW.Auth.ApiKeys.SqlServer.Tests.Live; + +/// +/// Auth-008 (scadaproj#46), the live half: key ids match exactly, as they do in the SQLite store +/// this one ports. The key_id column uses the database's default collation — case-insensitive +/// on a stock SQL Server — so LINE1 found the row stored as line1, and the importer +/// counted a SQLite Line1 beside line1 as an ordinary "skipped" duplicate. +/// +[Collection("live-sqlserver")] +public sealed class KeyIdFidelityLiveTests : IAsyncLifetime +{ + private readonly string _schema = LiveSqlServer.NewSchemaName(); + private readonly string _sqlitePath = + Path.Combine(Path.GetTempPath(), $"zbauth-keyid-{Guid.NewGuid():N}.db"); + + private AuthSqlServerConnectionFactory _factory = null!; + + public async Task InitializeAsync() + { + if (!LiveSqlServer.IsEnabled) + return; + + _factory = new AuthSqlServerConnectionFactory(LiveSqlServer.OptionsFor(_schema)); + await new SqlServerApiKeyStoreMigrator(_factory).MigrateAsync(CancellationToken.None); + } + + public async Task DisposeAsync() + { + SqliteConnection.ClearAllPools(); + foreach (string path in new[] { _sqlitePath, _sqlitePath + "-wal", _sqlitePath + "-shm" }) + { + if (File.Exists(path)) + File.Delete(path); + } + + await LiveSqlServer.DropSchemaAsync(_schema); + } + + private static ApiKeyRecord Record(string keyId) => new( + KeyId: keyId, + KeyPrefix: "sbk", + SecretHash: [1, 2, 3, 4], + DisplayName: "Key " + keyId, + Scopes: new HashSet(["read"], StringComparer.Ordinal), + ConstraintsJson: null, + CreatedUtc: new DateTimeOffset(2026, 5, 1, 8, 30, 0, TimeSpan.Zero), + LastUsedUtc: null, + RevokedUtc: null); + + [SkippableFact] + public async Task Lookups_AreCaseSensitive_LikeTheSqliteStore() + { + Skip.IfNot(LiveSqlServer.IsEnabled, LiveSqlServer.SkipReason); + + var admin = new SqlServerApiKeyAdminStore(_factory); + var read = new SqlServerApiKeyStore(_factory); + await admin.CreateAsync(Record("line1"), CancellationToken.None); + + Assert.NotNull(await read.FindByKeyIdAsync("line1", CancellationToken.None)); + Assert.Null(await read.FindByKeyIdAsync("LINE1", CancellationToken.None)); + Assert.Null(await read.FindActiveByKeyIdAsync("Line1", CancellationToken.None)); + // Trailing spaces are significant too (SQL Server's '=' pads them away). + Assert.Null(await read.FindByKeyIdAsync("line1 ", CancellationToken.None)); + Assert.False(await admin.RevokeAsync("LINE1", DateTimeOffset.UtcNow, CancellationToken.None)); + Assert.Null((await read.FindByKeyIdAsync("line1", CancellationToken.None))!.RevokedUtc); + } + + [SkippableFact] + public async Task Import_CaseVariantKeyIds_AreReportedAsCollisions_NotSkipped() + { + Skip.IfNot(LiveSqlServer.IsEnabled, LiveSqlServer.SkipReason); + + var sqliteFactory = new AuthSqliteConnectionFactory(_sqlitePath); + await new SqliteAuthStoreMigrator(sqliteFactory).MigrateAsync(CancellationToken.None); + var sqliteAdmin = new SqliteApiKeyAdminStore(sqliteFactory); + await sqliteAdmin.CreateAsync(Record("line1"), CancellationToken.None); + await sqliteAdmin.CreateAsync(Record("Line1"), CancellationToken.None); + + ApiKeyImportResult result = await new SqliteApiKeyStoreImporter(_factory).ImportAsync(_sqlitePath, CancellationToken.None); + + Assert.Equal(1, result.KeysInserted); + Assert.Equal(0, result.KeysSkipped); + Assert.Single(result.KeyIdCollisions); + + // Re-running: the inserted one is now an ordinary skip, the other still a collision. + ApiKeyImportResult again = await new SqliteApiKeyStoreImporter(_factory).ImportAsync(_sqlitePath, CancellationToken.None); + Assert.Equal(0, again.KeysInserted); + Assert.Equal(1, again.KeysSkipped); + Assert.Single(again.KeyIdCollisions); + } + + [SkippableFact] + public async Task Import_OverLengthKey_IsRejected_NotTruncated() + { + Skip.IfNot(LiveSqlServer.IsEnabled, LiveSqlServer.SkipReason); + + string longId = new('k', 70); + var sqliteFactory = new AuthSqliteConnectionFactory(_sqlitePath); + await new SqliteAuthStoreMigrator(sqliteFactory).MigrateAsync(CancellationToken.None); + var sqliteAdmin = new SqliteApiKeyAdminStore(sqliteFactory); + await sqliteAdmin.CreateAsync(Record(longId), CancellationToken.None); + await sqliteAdmin.CreateAsync(Record("ok-key"), CancellationToken.None); + + ApiKeyImportResult result = await new SqliteApiKeyStoreImporter(_factory).ImportAsync(_sqlitePath, CancellationToken.None); + + Assert.Equal(1, result.KeysInserted); + Assert.Equal(longId, Assert.Single(result.KeysRejected)); + Assert.Equal(1L, Convert.ToInt64(await LiveSqlServer.ScalarAsync($"SELECT COUNT(*) FROM [{_schema}].[api_keys]"))); + } +} diff --git a/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.ApiKeys.SqlServer.Tests/SqlServerColumnLimitTests.cs b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.ApiKeys.SqlServer.Tests/SqlServerColumnLimitTests.cs new file mode 100644 index 0000000..e9864f4 --- /dev/null +++ b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.ApiKeys.SqlServer.Tests/SqlServerColumnLimitTests.cs @@ -0,0 +1,77 @@ +using ZB.MOM.WW.Auth.Abstractions.ApiKeys; + +namespace ZB.MOM.WW.Auth.ApiKeys.SqlServer.Tests; + +/// +/// Auth-008 (scadaproj#46), the offline half: a value longer than its column is rejected or treated +/// as absent BEFORE any connection is opened. Every SqlParameter carries an explicit size, and +/// SqlClient truncates an over-length value to that size silently — so a 70-character key id was +/// stored as its first 64 characters, and a different 70-character id sharing that prefix then +/// "found" it. These tests point at a refused loopback port: pre-fix they reach the network and throw +/// SqlException; post-fix nothing is sent. +/// +public sealed class SqlServerColumnLimitTests +{ + private static readonly AuthSqlServerConnectionFactory Unreachable = new(new SqlServerApiKeyOptions + { + ConnectionString = "Server=tcp:127.0.0.1,1;Connect Timeout=3;ConnectRetryCount=0;TrustServerCertificate=true", + }); + + private static readonly string LongKeyId = new('k', 65); + + private static ApiKeyRecord Record(string keyId = "key-1", string prefix = "sbk", string displayName = "Key") => new( + KeyId: keyId, + KeyPrefix: prefix, + SecretHash: [1, 2, 3], + DisplayName: displayName, + Scopes: new HashSet(StringComparer.Ordinal), + ConstraintsJson: null, + CreatedUtc: DateTimeOffset.UnixEpoch, + LastUsedUtc: null, + RevokedUtc: null); + + public static TheoryData OverLength() => new() + { + { Record(keyId: LongKeyId), "KeyId" }, + { Record(prefix: new string('p', 17)), "KeyPrefix" }, + { Record(displayName: new string('d', 257)), "DisplayName" }, + }; + + [Theory] + [MemberData(nameof(OverLength))] + public async Task Create_OverLengthValue_IsRejected_NotTruncated(ApiKeyRecord record, string field) + { + var store = new SqlServerApiKeyAdminStore(Unreachable); + + var ex = await Assert.ThrowsAsync(() => store.CreateAsync(record, CancellationToken.None)); + Assert.Contains(field, ex.Message, StringComparison.Ordinal); + } + + [Fact] + public async Task Create_AtTheColumnLimits_ReachesTheDatabase() + { + // Positive control: values exactly at the limits are not rejected — the call gets as far as + // the (unreachable) server. + var store = new SqlServerApiKeyAdminStore(Unreachable); + var record = Record(keyId: new string('k', 64), prefix: new string('p', 16), displayName: new string('d', 256)); + + await Assert.ThrowsAsync(() => store.CreateAsync(record, CancellationToken.None)); + } + + [Fact] + public async Task Lookups_OfAnOverLengthKeyId_FindNothing_WithoutQuerying() + { + var read = new SqlServerApiKeyStore(Unreachable); + var admin = new SqlServerApiKeyAdminStore(Unreachable); + var now = DateTimeOffset.UnixEpoch; + + Assert.Null(await read.FindByKeyIdAsync(LongKeyId, CancellationToken.None)); + Assert.Null(await read.FindActiveByKeyIdAsync(LongKeyId, CancellationToken.None)); + await read.MarkUsedAsync(LongKeyId, now, CancellationToken.None); + Assert.False(await admin.RevokeAsync(LongKeyId, now, CancellationToken.None)); + Assert.False(await admin.RotateAsync(LongKeyId, [1], CancellationToken.None)); + Assert.False(await admin.SetScopesAsync(LongKeyId, new HashSet(), CancellationToken.None)); + Assert.False(await admin.SetEnabledAsync(LongKeyId, true, now, CancellationToken.None)); + Assert.False(await admin.DeleteAsync(LongKeyId, CancellationToken.None)); + } +} diff --git a/code-reviews/Auth/findings.md b/code-reviews/Auth/findings.md index 7392fbf..f45b50e 100644 --- a/code-reviews/Auth/findings.md +++ b/code-reviews/Auth/findings.md @@ -10,7 +10,7 @@ | Last reviewed | 2026-11-09 (re-review; baseline 2026-06-01 at `5f75cd4`) | | Reviewer | Claude (2026-11-09 re-review, scadaproj#15; baseline: Claude automated) | | Commit reviewed | `91fb2b4` | -| Open findings | 1 | +| Open findings | 0 | ## Summary @@ -380,7 +380,7 @@ template that yields the entry's DN exactly as the search reports it. | Severity | Low | | Priority | P3 | | Category | Correctness & logic bugs | -| Status | Open | +| Status | Resolved | | Location | `ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.ApiKeys.SqlServer/SqlServerApiKeyAdminStore.cs:45` (also `:46`, `:48`; `SqlServerApiKeyStore.cs:56,77`; `SqlServerApiKeyAuditStore.cs:30-32`; `SqliteApiKeyStoreImporter.cs:258-261,292-294`; columns in `SqlServerAuthSchema.cs:79-96`) | **Description** @@ -422,6 +422,39 @@ I verified this live against a SQL Server 2022 container: `ApiKeyAdminCommands`. - Make the importer report key-id collisions instead of counting them as plain skips. +**Resolution** + +Resolved 2026-11-09 (scadaproj#46, Auth `0.4.4`, unpublished). Re-verified live against a throwaway SQL Server 2022 +container (`RTM-CU22`): `FindByKeyIdAsync("LINE1")` found the row stored as `line1`, and the importer counted +`Line1` beside `line1` as a plain skip. The offline tests reproduced the truncation: an over-length create was sent +to the server. Fixed without a schema change, so the live shared store needs no migration. A new internal helper, +`SqlServerColumnLimits`, carries the rules: +- **Lengths.** `SqlServerApiKeyAdminStore.CreateAsync` throws `ArgumentException` before connecting when `KeyId` is + over 64, `KeyPrefix` over 16 or `DisplayName` over 256 characters. Every key-id lookup and mutation (`Find*`, + `MarkUsed`, `Revoke`, `Rotate`, `SetScopes`, `SetEnabled`, `Delete`) returns not-found or `false` for an id over + 64 characters without querying. Before, the truncated parameter could match a different key that shares the + first 64 characters. +- **Case.** Every `WHERE key_id = @key_id` also requires `CAST(key_id AS VARBINARY(128)) = CAST(@key_id AS VARBINARY(128))`. + The collation-aware `=` still drives the primary-key seek, and the binary compare makes the match exact, like + SQLite's `BINARY`. The column collation is deliberately not changed: that would be a primary-key migration on a + live shared store. Consequence: on a case-insensitive database, `Line1` and `line1` still cannot both exist. + `CreateAsync` already fails loudly with a PK violation in that case, and the importer now reports it. +- **Importer.** The new `init` properties `ApiKeyImportResult.KeyIdCollisions` (case-only clashes with a key + already in SQL Server) and `KeysRejected` (over-length rows) list what was not imported and why. Before, both kinds + were folded into `KeysSkipped`, or imported truncated. `KeysSkipped` now counts only exact duplicates. The + insert-if-absent statement returns a three-way outcome from inside one statement, so it stays race-safe. +- **Audit text.** `key_id`, `event_type` and `remote_address` are still truncated to 64 characters. This is now + deliberate and documented on `SqlServerApiKeyAuditStore.AppendAsync`: an audit append runs after the action it + records, so refusing it would lose the record of a change that has already happened. + +Tests: +- `SqlServerColumnLimitTests` (offline, refused loopback port). Over-length create is rejected for each of the three + fields, and all 8 key-id operations answer "not found" for an over-length id. These fail pre-fix with + `SqlException`. A positive control at exactly the limits still reaches the server. +- `Live/KeyIdFidelityLiveTests` (`AUTH_SQLSERVER_CONNSTR`). Case- and trailing-space-exact lookups, an importer + case-collision report, and an importer over-length rejection. All 3 fail pre-fix. +- The full SqlServer suite passes live: 101/101. + ### Auth-009 — Canonical username is the first value of a possibly multi-valued attribute, not the value the user matched | | | diff --git a/code-reviews/README.md b/code-reviews/README.md index b6b641f..a90b055 100644 --- a/code-reviews/README.md +++ b/code-reviews/README.md @@ -40,7 +40,7 @@ code-reviews/ ## Summary -10 of 10 libraries reviewed. 16 pending findings across all libraries. +10 of 10 libraries reviewed. 13 pending findings across all libraries. Coverage: Secrets, LocalDb, GalaxyRepository and Overview first reviewed, and Auth, Health, Theme, Audit, Configuration and Telemetry re-reviewed, on 2026-11-09 (#15). Each finding also @@ -51,15 +51,15 @@ carries a P1/P2/P3 priority (P1 = Critical/High, P2 = Medium, P3 = Low). | Critical | 0 | | High | 0 | | Medium | 0 | -| Low | 16 | -| **Total** | **16** | +| Low | 13 | +| **Total** | **13** | ## Library Status | Library | Last reviewed | Commit | Open (C/H/M/L) | Open | Total | |---------|---------------|--------|----------------|------|-------| | [Audit](Audit/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 5 | -| [Auth](Auth/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/3 | 3 | 10 | +| [Auth](Auth/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 10 | | [Configuration](Configuration/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 4 | | [GalaxyRepository](GalaxyRepository/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 8 | | [Health](Health/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 7 | @@ -88,13 +88,10 @@ _None open._ _None open._ -### Low (16) +### Low (13) | ID | Library | Title | |----|---------|-------| -| Auth-008 | [Auth](Auth/findings.md) | SQL Server store silently truncates over-length values and matches key ids case-insensitively, diverging from SQLite | -| Auth-009 | [Auth](Auth/findings.md) | Canonical username is the first value of a possibly multi-valued attribute, not the value the user matched | -| Auth-010 | [Auth](Auth/findings.md) | `ApiKeyVerifier` lets store exceptions escape despite its "only exception path is cancellation" contract | | Secrets-004 | [Secrets](Secrets/findings.md) | Corrupt or wrong-length crypto fields escape `Decrypt` as `ArgumentException`: not audited, and breaks the cipher contract | | Secrets-005 | [Secrets](Secrets/findings.md) | Store-path rules are lexical: drive-relative paths pass "rooted", and `\\?\` / UNC / 8.3 / symlink spellings bypass "under content root" | | Secrets-006 | [Secrets](Secrets/findings.md) | Resolver cache can re-cache a stale value after `Invalidate`, and expired plaintext is never evicted |