fix(auth): SQL Server key store refuses truncation and matches key ids exactly (Auth-008)

- Over-length key records (key_id > 64, prefix > 16, display name > 256) are
  refused with ArgumentException before connecting; an over-64 key-id lookup
  or mutation answers not-found without querying (SqlClient silently
  truncated the sized parameter, so a different id sharing the first 64
  characters matched).
- Every key_id predicate adds an exact VARBINARY comparison, so lookups are
  case- and trailing-space-exact like SQLite; the PK seek is unchanged and
  there is no schema change.
- SqliteApiKeyStoreImporter reports case-only key-id collisions and
  over-length rows in new init lists (KeyIdCollisions, KeysRejected) instead
  of counting them as skips or importing them truncated.
- Audit text truncation kept deliberately and documented.

Offline refused-port tests and live SQL Server 2022 tests fail pre-fix;
SqlServer suite 101/101 live. Auth findings now 0 open; CLAUDE.md records
the #46/#50 work; code-reviews/README.md regenerated.

(scadaproj#46)

Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV
This commit is contained in:
Joseph Doherty
2026-11-09 15:26:03 -05:00
parent c4366539ca
commit b33f0f93c9
11 changed files with 417 additions and 30 deletions
+8
View File
@@ -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=<group>,ou=users,<base>`). 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<InvalidOperationException>` 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.
+5
View File
@@ -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
@@ -23,11 +23,17 @@ public sealed class SqlServerApiKeyAdminStore(AuthSqlServerConnectionFactory con
/// <remarks>
/// A plain INSERT: a duplicate <c>key_id</c> surfaces as a primary-key
/// <see cref="SqlException"/> rather than silently overwriting an existing key (the SQLite store
/// raises its own <c>SqliteException</c> in the same situation).
/// raises its own <c>SqliteException</c> in the same situation). Unlike SQLite, the primary key
/// follows the database collation, so on a case-insensitive database <c>Line1</c> cannot be
/// created beside <c>line1</c> either. A key id over 64, prefix over 16 or display name over 256
/// characters throws <see cref="ArgumentException"/> before anything is sent (since 0.4.4; it
/// was silently truncated before).
/// </remarks>
/// <exception cref="ArgumentException">A field is longer than its column.</exception>
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<bool> 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<bool> 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<bool> SetScopesAsync(string keyId, IReadOnlySet<string> 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<bool> 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<bool> 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;
@@ -13,6 +13,14 @@ namespace ZB.MOM.WW.Auth.ApiKeys.SqlServer;
public sealed class SqlServerApiKeyAuditStore(AuthSqlServerConnectionFactory connectionFactory) : IApiKeyAuditStore
{
/// <inheritdoc />
/// <remarks>
/// <b>Over-length text is truncated, deliberately.</b> <c>key_id</c>, <c>event_type</c> and
/// <c>remote_address</c> 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 <see cref="SqlServerApiKeyAdminStore.CreateAsync"/>), 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. <c>details</c> is unbounded.
/// </remarks>
public async Task AppendAsync(ApiKeyAuditEntry entry, CancellationToken ct)
{
ArgumentNullException.ThrowIfNull(entry);
@@ -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<ApiKeyRecord?> 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);
@@ -0,0 +1,61 @@
using ZB.MOM.WW.Auth.Abstractions.ApiKeys;
namespace ZB.MOM.WW.Auth.ApiKeys.SqlServer;
/// <summary>
/// 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.
/// </summary>
/// <remarks>
/// <para>
/// <b>Lengths.</b> 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.
/// </para>
/// <para>
/// <b>Case.</b> The <c>key_id</c> 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 <c>key_id</c> predicate
/// instead adds <see cref="ExactKeyIdMatch"/>: the collation-aware <c>=</c> still drives the primary-key
/// seek, and the binary comparison then rejects a row that differs in case or trailing spaces.
/// </para>
/// </remarks>
internal static class SqlServerColumnLimits
{
/// <summary>Width of <c>api_keys.key_id</c> (and <c>api_key_audit.key_id</c>).</summary>
public const int KeyId = 64;
/// <summary>Width of <c>api_keys.key_prefix</c>.</summary>
public const int KeyPrefix = 16;
/// <summary>Width of <c>api_keys.display_name</c>.</summary>
public const int DisplayName = 256;
/// <summary>
/// Appended to every <c>key_id = @key_id</c>. <c>NVARCHAR</c> → <c>VARBINARY</c> yields the UTF-16
/// bytes, so the comparison is exact (case, accents and trailing spaces all significant).
/// </summary>
public const string ExactKeyIdMatch = "CAST(key_id AS VARBINARY(128)) = CAST(@key_id AS VARBINARY(128))";
/// <summary>Whether a key with this id could be stored at all.</summary>
public static bool CanExist(string keyId) => keyId.Length <= KeyId;
/// <summary>The first over-length field of <paramref name="record"/>, described, or <see langword="null"/> when every field fits.</summary>
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;
/// <summary>Throws <see cref="ArgumentException"/> when a field of <paramref name="record"/> exceeds its column.</summary>
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}";
}
@@ -13,7 +13,26 @@ namespace ZB.MOM.WW.Auth.ApiKeys.SqlServer;
/// <param name="AuditSkipped">Audit rows that already existed in SQL Server.</param>
/// <param name="SourceFound">Whether the SQLite file existed at all.</param>
public sealed record ApiKeyImportResult(
int KeysInserted, int KeysSkipped, int AuditInserted, int AuditSkipped, bool SourceFound);
int KeysInserted, int KeysSkipped, int AuditInserted, int AuditSkipped, bool SourceFound)
{
/// <summary>
/// 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 <c>Line1</c> and
/// <c>line1</c> can both exist there; the SQL Server <c>key_id</c> 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
/// <see cref="KeysSkipped"/>, which counts only exact duplicates.
/// </summary>
public IReadOnlyList<string> KeyIdCollisions { get; init; } = [];
/// <summary>
/// Source key ids that were NOT imported because a value exceeds its SQL Server column
/// (<c>key_id</c> 64, <c>key_prefix</c> 16, <c>display_name</c> 256 characters) (since 0.4.4,
/// Auth-008). Up to 0.4.3 such a row was imported silently truncated. Not counted in
/// <see cref="KeysSkipped"/>.
/// </summary>
public IReadOnlyList<string> KeysRejected { get; init; } = [];
}
/// <summary>
/// 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<string> collisions = [];
List<string> 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<bool> 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<KeyInsertOutcome> 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
@@ -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;
/// <summary>
/// Auth-008 (scadaproj#46), the live half: key ids match exactly, as they do in the SQLite store
/// this one ports. The <c>key_id</c> column uses the database's default collation — case-insensitive
/// on a stock SQL Server — so <c>LINE1</c> found the row stored as <c>line1</c>, and the importer
/// counted a SQLite <c>Line1</c> beside <c>line1</c> as an ordinary "skipped" duplicate.
/// </summary>
[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<string>(["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]")));
}
}
@@ -0,0 +1,77 @@
using ZB.MOM.WW.Auth.Abstractions.ApiKeys;
namespace ZB.MOM.WW.Auth.ApiKeys.SqlServer.Tests;
/// <summary>
/// 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 <c>SqlParameter</c> 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
/// <c>SqlException</c>; post-fix nothing is sent.
/// </summary>
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<string>(StringComparer.Ordinal),
ConstraintsJson: null,
CreatedUtc: DateTimeOffset.UnixEpoch,
LastUsedUtc: null,
RevokedUtc: null);
public static TheoryData<ApiKeyRecord, string> 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<ArgumentException>(() => 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<Microsoft.Data.SqlClient.SqlException>(() => 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<string>(), CancellationToken.None));
Assert.False(await admin.SetEnabledAsync(LongKeyId, true, now, CancellationToken.None));
Assert.False(await admin.DeleteAsync(LongKeyId, CancellationToken.None));
}
}
+35 -2
View File
@@ -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
| | |
+5 -8
View File
@@ -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 |