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:
@@ -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` (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.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.
|
**`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.
|
||||||
|
|||||||
@@ -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`.
|
- 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.
|
- 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`).
|
- **`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.
|
- **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
|
### 0.4.1 — LDAP review fixes
|
||||||
|
|||||||
@@ -23,11 +23,17 @@ public sealed class SqlServerApiKeyAdminStore(AuthSqlServerConnectionFactory con
|
|||||||
/// <remarks>
|
/// <remarks>
|
||||||
/// A plain INSERT: a duplicate <c>key_id</c> surfaces as a primary-key
|
/// 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
|
/// <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>
|
/// </remarks>
|
||||||
|
/// <exception cref="ArgumentException">A field is longer than its column.</exception>
|
||||||
public async Task CreateAsync(ApiKeyRecord record, CancellationToken ct)
|
public async Task CreateAsync(ApiKeyRecord record, CancellationToken ct)
|
||||||
{
|
{
|
||||||
ArgumentNullException.ThrowIfNull(record);
|
ArgumentNullException.ThrowIfNull(record);
|
||||||
|
SqlServerColumnLimits.EnsureFits(record);
|
||||||
|
|
||||||
string schema = connectionFactory.Schema;
|
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)
|
public async Task<bool> RevokeAsync(string keyId, DateTimeOffset whenUtc, CancellationToken ct)
|
||||||
{
|
{
|
||||||
ArgumentException.ThrowIfNullOrWhiteSpace(keyId);
|
ArgumentException.ThrowIfNullOrWhiteSpace(keyId);
|
||||||
|
if (!SqlServerColumnLimits.CanExist(keyId))
|
||||||
|
return false;
|
||||||
|
|
||||||
string schema = connectionFactory.Schema;
|
string schema = connectionFactory.Schema;
|
||||||
|
|
||||||
@@ -74,7 +82,7 @@ public sealed class SqlServerApiKeyAdminStore(AuthSqlServerConnectionFactory con
|
|||||||
await using SqlCommand command = connectionFactory.CreateCommand(connection, $"""
|
await using SqlCommand command = connectionFactory.CreateCommand(connection, $"""
|
||||||
UPDATE [{schema}].[{SqlServerAuthSchema.ApiKeysTable}]
|
UPDATE [{schema}].[{SqlServerAuthSchema.ApiKeysTable}]
|
||||||
SET revoked_utc = @revoked_utc
|
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("@key_id", SqlDbType.NVarChar, 64).Value = keyId;
|
||||||
command.Parameters.Add("@revoked_utc", SqlDbType.DateTimeOffset).Value = whenUtc;
|
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)
|
public async Task<bool> RotateAsync(string keyId, byte[] newSecretHash, CancellationToken ct)
|
||||||
{
|
{
|
||||||
ArgumentException.ThrowIfNullOrWhiteSpace(keyId);
|
ArgumentException.ThrowIfNullOrWhiteSpace(keyId);
|
||||||
|
if (!SqlServerColumnLimits.CanExist(keyId))
|
||||||
|
return false;
|
||||||
ArgumentNullException.ThrowIfNull(newSecretHash);
|
ArgumentNullException.ThrowIfNull(newSecretHash);
|
||||||
|
|
||||||
string schema = connectionFactory.Schema;
|
string schema = connectionFactory.Schema;
|
||||||
@@ -105,7 +115,7 @@ public sealed class SqlServerApiKeyAdminStore(AuthSqlServerConnectionFactory con
|
|||||||
SET secret_hash = @secret_hash,
|
SET secret_hash = @secret_hash,
|
||||||
last_used_utc = NULL,
|
last_used_utc = NULL,
|
||||||
revoked_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("@key_id", SqlDbType.NVarChar, 64).Value = keyId;
|
||||||
command.Parameters.Add("@secret_hash", SqlDbType.VarBinary, 64).Value = newSecretHash;
|
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)
|
public async Task<bool> SetScopesAsync(string keyId, IReadOnlySet<string> scopes, CancellationToken ct)
|
||||||
{
|
{
|
||||||
ArgumentException.ThrowIfNullOrWhiteSpace(keyId);
|
ArgumentException.ThrowIfNullOrWhiteSpace(keyId);
|
||||||
|
if (!SqlServerColumnLimits.CanExist(keyId))
|
||||||
|
return false;
|
||||||
ArgumentNullException.ThrowIfNull(scopes);
|
ArgumentNullException.ThrowIfNull(scopes);
|
||||||
|
|
||||||
string schema = connectionFactory.Schema;
|
string schema = connectionFactory.Schema;
|
||||||
@@ -129,7 +141,7 @@ public sealed class SqlServerApiKeyAdminStore(AuthSqlServerConnectionFactory con
|
|||||||
await using SqlCommand command = connectionFactory.CreateCommand(connection, $"""
|
await using SqlCommand command = connectionFactory.CreateCommand(connection, $"""
|
||||||
UPDATE [{schema}].[{SqlServerAuthSchema.ApiKeysTable}]
|
UPDATE [{schema}].[{SqlServerAuthSchema.ApiKeysTable}]
|
||||||
SET scopes = @scopes
|
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("@key_id", SqlDbType.NVarChar, 64).Value = keyId;
|
||||||
command.Parameters.Add("@scopes", SqlDbType.NVarChar, -1).Value = ScopeSerializer.Serialize(scopes);
|
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)
|
public async Task<bool> SetEnabledAsync(string keyId, bool enabled, DateTimeOffset whenUtc, CancellationToken ct)
|
||||||
{
|
{
|
||||||
ArgumentException.ThrowIfNullOrWhiteSpace(keyId);
|
ArgumentException.ThrowIfNullOrWhiteSpace(keyId);
|
||||||
|
if (!SqlServerColumnLimits.CanExist(keyId))
|
||||||
|
return false;
|
||||||
|
|
||||||
string schema = connectionFactory.Schema;
|
string schema = connectionFactory.Schema;
|
||||||
|
|
||||||
@@ -159,7 +173,7 @@ public sealed class SqlServerApiKeyAdminStore(AuthSqlServerConnectionFactory con
|
|||||||
command = connectionFactory.CreateCommand(connection, $"""
|
command = connectionFactory.CreateCommand(connection, $"""
|
||||||
UPDATE [{schema}].[{SqlServerAuthSchema.ApiKeysTable}]
|
UPDATE [{schema}].[{SqlServerAuthSchema.ApiKeysTable}]
|
||||||
SET revoked_utc = NULL
|
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;
|
command.Parameters.Add("@key_id", SqlDbType.NVarChar, 64).Value = keyId;
|
||||||
}
|
}
|
||||||
@@ -168,7 +182,7 @@ public sealed class SqlServerApiKeyAdminStore(AuthSqlServerConnectionFactory con
|
|||||||
command = connectionFactory.CreateCommand(connection, $"""
|
command = connectionFactory.CreateCommand(connection, $"""
|
||||||
UPDATE [{schema}].[{SqlServerAuthSchema.ApiKeysTable}]
|
UPDATE [{schema}].[{SqlServerAuthSchema.ApiKeysTable}]
|
||||||
SET revoked_utc = @revoked_utc
|
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("@key_id", SqlDbType.NVarChar, 64).Value = keyId;
|
||||||
command.Parameters.Add("@revoked_utc", SqlDbType.DateTimeOffset).Value = whenUtc;
|
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)
|
public async Task<bool> DeleteAsync(string keyId, CancellationToken ct)
|
||||||
{
|
{
|
||||||
ArgumentException.ThrowIfNullOrWhiteSpace(keyId);
|
ArgumentException.ThrowIfNullOrWhiteSpace(keyId);
|
||||||
|
if (!SqlServerColumnLimits.CanExist(keyId))
|
||||||
|
return false;
|
||||||
|
|
||||||
string schema = connectionFactory.Schema;
|
string schema = connectionFactory.Schema;
|
||||||
|
|
||||||
@@ -198,7 +214,7 @@ public sealed class SqlServerApiKeyAdminStore(AuthSqlServerConnectionFactory con
|
|||||||
|
|
||||||
await using SqlCommand command = connectionFactory.CreateCommand(connection, $"""
|
await using SqlCommand command = connectionFactory.CreateCommand(connection, $"""
|
||||||
DELETE FROM [{schema}].[{SqlServerAuthSchema.ApiKeysTable}]
|
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;
|
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
|
public sealed class SqlServerApiKeyAuditStore(AuthSqlServerConnectionFactory connectionFactory) : IApiKeyAuditStore
|
||||||
{
|
{
|
||||||
/// <inheritdoc />
|
/// <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)
|
public async Task AppendAsync(ApiKeyAuditEntry entry, CancellationToken ct)
|
||||||
{
|
{
|
||||||
ArgumentNullException.ThrowIfNull(entry);
|
ArgumentNullException.ThrowIfNull(entry);
|
||||||
|
|||||||
@@ -42,6 +42,8 @@ public sealed class SqlServerApiKeyStore(AuthSqlServerConnectionFactory connecti
|
|||||||
public async Task MarkUsedAsync(string keyId, DateTimeOffset whenUtc, CancellationToken ct)
|
public async Task MarkUsedAsync(string keyId, DateTimeOffset whenUtc, CancellationToken ct)
|
||||||
{
|
{
|
||||||
ArgumentException.ThrowIfNullOrWhiteSpace(keyId);
|
ArgumentException.ThrowIfNullOrWhiteSpace(keyId);
|
||||||
|
if (!SqlServerColumnLimits.CanExist(keyId))
|
||||||
|
return;
|
||||||
|
|
||||||
string schema = connectionFactory.Schema;
|
string schema = connectionFactory.Schema;
|
||||||
|
|
||||||
@@ -51,7 +53,7 @@ public sealed class SqlServerApiKeyStore(AuthSqlServerConnectionFactory connecti
|
|||||||
await using SqlCommand command = connectionFactory.CreateCommand(connection, $"""
|
await using SqlCommand command = connectionFactory.CreateCommand(connection, $"""
|
||||||
UPDATE [{schema}].[{SqlServerAuthSchema.ApiKeysTable}]
|
UPDATE [{schema}].[{SqlServerAuthSchema.ApiKeysTable}]
|
||||||
SET last_used_utc = @last_used_utc
|
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("@key_id", SqlDbType.NVarChar, 64).Value = keyId;
|
||||||
command.Parameters.Add("@last_used_utc", SqlDbType.DateTimeOffset).Value = whenUtc;
|
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)
|
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 schema = connectionFactory.Schema;
|
||||||
string table = $"[{schema}].[{SqlServerAuthSchema.ApiKeysTable}]";
|
string table = $"[{schema}].[{SqlServerAuthSchema.ApiKeysTable}]";
|
||||||
|
|
||||||
@@ -72,8 +79,8 @@ public sealed class SqlServerApiKeyStore(AuthSqlServerConnectionFactory connecti
|
|||||||
await using SqlCommand command = connectionFactory.CreateCommand(
|
await using SqlCommand command = connectionFactory.CreateCommand(
|
||||||
connection,
|
connection,
|
||||||
requireActive
|
requireActive
|
||||||
? $"SELECT {SelectColumns} FROM {table} WHERE key_id = @key_id AND revoked_utc IS NULL;"
|
? $"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;");
|
: $"SELECT {SelectColumns} FROM {table} WHERE key_id = @key_id AND {SqlServerColumnLimits.ExactKeyIdMatch};");
|
||||||
command.Parameters.Add("@key_id", SqlDbType.NVarChar, 64).Value = keyId;
|
command.Parameters.Add("@key_id", SqlDbType.NVarChar, 64).Value = keyId;
|
||||||
|
|
||||||
await using SqlDataReader reader = await command.ExecuteReaderAsync(ct).ConfigureAwait(false);
|
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="AuditSkipped">Audit rows that already existed in SQL Server.</param>
|
||||||
/// <param name="SourceFound">Whether the SQLite file existed at all.</param>
|
/// <param name="SourceFound">Whether the SQLite file existed at all.</param>
|
||||||
public sealed record ApiKeyImportResult(
|
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>
|
/// <summary>
|
||||||
/// Copies every key and audit row from a per-node SQLite store into the shared SQL Server store,
|
/// 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);
|
(SqlTransaction)await connection.BeginTransactionAsync(ct).ConfigureAwait(false);
|
||||||
|
|
||||||
int keysInserted = 0;
|
int keysInserted = 0;
|
||||||
|
int keysSkipped = 0;
|
||||||
int auditInserted = 0;
|
int auditInserted = 0;
|
||||||
|
List<string> collisions = [];
|
||||||
|
List<string> rejected = [];
|
||||||
|
|
||||||
foreach (ApiKeyRecord key in keys)
|
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(
|
return new ApiKeyImportResult(
|
||||||
KeysInserted: keysInserted,
|
KeysInserted: keysInserted,
|
||||||
KeysSkipped: keys.Count - keysInserted,
|
KeysSkipped: keysSkipped,
|
||||||
AuditInserted: auditInserted,
|
AuditInserted: auditInserted,
|
||||||
AuditSkipped: auditEntries.Count - 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
|
private enum KeyInsertOutcome
|
||||||
// separate SELECT so the check and the insert cannot straddle a concurrent writer.
|
{
|
||||||
private async Task<bool> InsertKeyIfAbsentAsync(
|
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,
|
SqlConnection connection,
|
||||||
SqlTransaction transaction,
|
SqlTransaction transaction,
|
||||||
string schema,
|
string schema,
|
||||||
@@ -246,14 +297,23 @@ public sealed class SqliteApiKeyStoreImporter(AuthSqlServerConnectionFactory sql
|
|||||||
CancellationToken ct)
|
CancellationToken ct)
|
||||||
{
|
{
|
||||||
await using SqlCommand command = sql.CreateCommand(connection, $"""
|
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 1 FROM [{schema}].[{SqlServerAuthSchema.ApiKeysTable}] WHERE key_id = @key_id)
|
||||||
|
SELECT 2;
|
||||||
|
ELSE
|
||||||
|
BEGIN
|
||||||
INSERT INTO [{schema}].[{SqlServerAuthSchema.ApiKeysTable}] (
|
INSERT INTO [{schema}].[{SqlServerAuthSchema.ApiKeysTable}] (
|
||||||
key_id, key_prefix, secret_hash, display_name, scopes,
|
key_id, key_prefix, secret_hash, display_name, scopes,
|
||||||
constraints, created_utc, last_used_utc, revoked_utc, expires_utc)
|
constraints, created_utc, last_used_utc, revoked_utc, expires_utc)
|
||||||
VALUES (
|
VALUES (
|
||||||
@key_id, @key_prefix, @secret_hash, @display_name, @scopes,
|
@key_id, @key_prefix, @secret_hash, @display_name, @scopes,
|
||||||
@constraints, @created_utc, @last_used_utc, @revoked_utc, @expires_utc);
|
@constraints, @created_utc, @last_used_utc, @revoked_utc, @expires_utc);
|
||||||
|
SELECT 1;
|
||||||
|
END
|
||||||
""", transaction);
|
""", transaction);
|
||||||
command.Parameters.Add("@key_id", SqlDbType.NVarChar, 64).Value = key.KeyId;
|
command.Parameters.Add("@key_id", SqlDbType.NVarChar, 64).Value = key.KeyId;
|
||||||
command.Parameters.Add("@key_prefix", SqlDbType.NVarChar, 16).Value = key.KeyPrefix;
|
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("@revoked_utc", SqlDbType.DateTimeOffset).Value = (object?)key.RevokedUtc ?? DBNull.Value;
|
||||||
command.Parameters.Add("@expires_utc", SqlDbType.DateTimeOffset).Value = (object?)key.ExpiresUtc ?? 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
|
// Audit rows carry no natural key (audit_id is per-file IDENTITY), so identity is the triple
|
||||||
|
|||||||
+114
@@ -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]")));
|
||||||
|
}
|
||||||
|
}
|
||||||
+77
@@ -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));
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -10,7 +10,7 @@
|
|||||||
| Last reviewed | 2026-11-09 (re-review; baseline 2026-06-01 at `5f75cd4`) |
|
| 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) |
|
| Reviewer | Claude (2026-11-09 re-review, scadaproj#15; baseline: Claude automated) |
|
||||||
| Commit reviewed | `91fb2b4` |
|
| Commit reviewed | `91fb2b4` |
|
||||||
| Open findings | 1 |
|
| Open findings | 0 |
|
||||||
|
|
||||||
## Summary
|
## Summary
|
||||||
|
|
||||||
@@ -380,7 +380,7 @@ template that yields the entry's DN exactly as the search reports it.
|
|||||||
| Severity | Low |
|
| Severity | Low |
|
||||||
| Priority | P3 |
|
| Priority | P3 |
|
||||||
| Category | Correctness & logic bugs |
|
| 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`) |
|
| 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**
|
**Description**
|
||||||
@@ -422,6 +422,39 @@ I verified this live against a SQL Server 2022 container:
|
|||||||
`ApiKeyAdminCommands`.
|
`ApiKeyAdminCommands`.
|
||||||
- Make the importer report key-id collisions instead of counting them as plain skips.
|
- 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
|
### Auth-009 — Canonical username is the first value of a possibly multi-valued attribute, not the value the user matched
|
||||||
|
|
||||||
| | |
|
| | |
|
||||||
|
|||||||
@@ -40,7 +40,7 @@ code-reviews/
|
|||||||
|
|
||||||
## Summary
|
## 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,
|
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
|
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 |
|
| Critical | 0 |
|
||||||
| High | 0 |
|
| High | 0 |
|
||||||
| Medium | 0 |
|
| Medium | 0 |
|
||||||
| Low | 16 |
|
| Low | 13 |
|
||||||
| **Total** | **16** |
|
| **Total** | **13** |
|
||||||
|
|
||||||
## Library Status
|
## Library Status
|
||||||
|
|
||||||
| Library | Last reviewed | Commit | Open (C/H/M/L) | Open | Total |
|
| 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 |
|
| [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 |
|
| [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 |
|
| [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 |
|
| [Health](Health/findings.md) | 2026-11-09 | `91fb2b4` | 0/0/0/0 | 0 | 7 |
|
||||||
@@ -88,13 +88,10 @@ _None open._
|
|||||||
|
|
||||||
_None open._
|
_None open._
|
||||||
|
|
||||||
### Low (16)
|
### Low (13)
|
||||||
|
|
||||||
| ID | Library | Title |
|
| 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-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-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 |
|
| Secrets-006 | [Secrets](Secrets/findings.md) | Resolver cache can re-cache a stale value after `Invalidate`, and expired plaintext is never evicted |
|
||||||
|
|||||||
Reference in New Issue
Block a user