diff --git a/CHANGELOG.md b/CHANGELOG.md index 59df1af..cac36f8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **The Keychain backend could see another CLI's credentials** (#55). App scoping was a + dot-prefix match on the service string, and since the service is `{app}.{provider}` and + provider names may contain dots, app `com.acme.cli` could not distinguish its own + `pro.Adobe` item from app `com.acme.cli.pro`'s `Adobe` item. The neighbour could list, + export **and delete** credentials it did not own; reverse-DNS identifiers nest by + convention, so the collision is plausible rather than contrived. Items now record their + owning app in `kSecAttrComment` and every query filters on it exactly. Items written before + this release carry no owner and fall back to the old prefix test, so nothing already stored + becomes invisible — meaning **legacy items stay ambiguous until rewritten**, and only items + written from this release on are scoped exactly. + - **`RestoreCredentialAsync_Preserves…` flaked on the OS-native backends** (#61). The test retried until one credential was visible and then asserted `IsSelected`, which comes from a *separate* store item written moments earlier — so the credential could become visible diff --git a/src/NextIteration.SpectreConsole.Auth/Persistence/Keychain/KeychainCredentialManager.cs b/src/NextIteration.SpectreConsole.Auth/Persistence/Keychain/KeychainCredentialManager.cs index ae82064..20d1981 100644 --- a/src/NextIteration.SpectreConsole.Auth/Persistence/Keychain/KeychainCredentialManager.cs +++ b/src/NextIteration.SpectreConsole.Auth/Persistence/Keychain/KeychainCredentialManager.cs @@ -71,6 +71,7 @@ public Task AddCredentialAsync(string providerName, string accountName, Label = accountName, Description = environment, Data = Encoding.UTF8.GetBytes(credentialData), + OwnerAppIdentifier = _appIdentifier, }; AddItem(attrs); @@ -91,7 +92,7 @@ public Task AddCredentialAsync(string providerName, string accountName, /// Best-effort: returns after a bounded wait even if the item never /// appears, leaving any genuine failure to the caller's own lookup. /// - private static void ConfirmItemVisible(string service, string account) + private void ConfirmItemVisible(string service, string account) { const int maxAttempts = 20; for (var attempt = 1; attempt <= maxAttempts; attempt++) @@ -344,6 +345,7 @@ public Task RestoreCredentialAsync(CredentialExport credential) Label = credential.AccountName, Description = credential.Environment, Data = Encoding.UTF8.GetBytes(credential.CredentialData), + OwnerAppIdentifier = _appIdentifier, }); if (credential.IsSelected) @@ -391,6 +393,33 @@ private string ResolveStoredProviderName(string providerName) return providerName; } + /// + /// Whether belongs to this manager's app. + /// + /// + /// Items written by this version carry the owning app identifier in + /// kSecAttrGeneric and are matched on it exactly. Items written before that + /// attribute existed carry nothing, so they fall back to the old service-prefix + /// check — without which every previously stored credential would vanish. + /// + /// The fallback is not a complete fix and is not presented as one: a legacy item + /// belonging to an app whose identifier is a dot-prefix of another's stays + /// ambiguous until it is rewritten. Everything written from now on is scoped + /// exactly (#55). + /// + /// + private bool IsOwnedByThisApp(KeychainItem item) + { + if (!string.IsNullOrEmpty(item.OwnerAppIdentifier)) + { + return string.Equals(item.OwnerAppIdentifier, _appIdentifier, StringComparison.Ordinal); + } + + // Legacy item: no owner recorded, so fall back to the ambiguous prefix test. + return item.Service.StartsWith(_appIdentifier + ".", StringComparison.Ordinal) + || string.Equals(item.Service, SelectionsService, StringComparison.Ordinal); + } + private string ServiceFor(string providerName) => $"{_appIdentifier}.{providerName}"; private string SelectionsService => $"{_appIdentifier}{SelectionsServiceSuffix}"; @@ -431,6 +460,7 @@ private void WriteSelection(string providerName, string accountId) Label = $"{_appIdentifier} active credential ({providerName})", Description = string.Empty, Data = bytes, + OwnerAppIdentifier = _appIdentifier, }); } else @@ -497,6 +527,12 @@ private sealed class KeychainItem public string? Description { get; init; } public DateTime? CreatedAt { get; init; } public byte[]? Data { get; init; } + + /// + /// Owning app identifier, stored in kSecAttrComment. Null for items + /// written before this attribute existed — see . + /// + public string? OwnerAppIdentifier { get; init; } } private static void AddItem(KeychainItem item) @@ -510,6 +546,13 @@ private static void AddItem(KeychainItem item) var descCf = Track(handles, NewCfString(item.Description ?? string.Empty)); var dataCf = Track(handles, NewCfData(item.Data ?? [])); + // kSecAttrComment records which app owns the item, so scoping does not have + // to be inferred from the service string. That inference was ambiguous: + // the service is "{app}.{provider}" and provider names may contain dots, so + // app "com.acme.cli" could not tell its own "pro.Adobe" item from app + // "com.acme.cli.pro"'s "Adobe" item (#55). + var ownerCf = Track(handles, NewCfString(item.OwnerAppIdentifier ?? string.Empty)); + var pairs = new List<(IntPtr, IntPtr)> { (Constants.KSecClass, Constants.KSecClassGenericPassword), @@ -517,6 +560,7 @@ private static void AddItem(KeychainItem item) (Constants.KSecAttrAccount, accountCf), (Constants.KSecAttrLabel, labelCf), (Constants.KSecAttrDescription, descCf), + (Constants.KSecAttrComment, ownerCf), (Constants.KSecValueData, dataCf), }; @@ -588,7 +632,12 @@ private static void DeleteItem(string service, string account) } } - private static KeychainItem? QuerySingleItem(string service, string account, bool includeData) + /// + /// Fetches one item by service + account, or null if it does not exist or is not + /// ours. See — an exact service query is not on its + /// own proof of ownership, because the service string is ambiguous (#55). + /// + private KeychainItem? QuerySingleItem(string service, string account, bool includeData) { var handles = new List(); try @@ -615,7 +664,11 @@ private static void DeleteItem(string service, string account) try { - return DecodeItem(result); + var decoded = DecodeItem(result); + + // Present but owned by another app: treat as not found rather than + // handing back someone else's credential (#55). + return decoded is not null && IsOwnedByThisApp(decoded) ? decoded : null; } finally { @@ -631,7 +684,7 @@ private static void DeleteItem(string service, string account) } } - private static List QueryItems(string service, bool includeData) + private List QueryItems(string service, bool includeData) { // Bulk query: request attributes only. Combining kSecReturnAttributes // + kSecReturnData + kSecMatchLimitAll in a single SecItemCopyMatching @@ -661,7 +714,9 @@ private static List QueryItems(string service, bool includeData) try { - stubs = DecodeArray(result); + // The service string alone does not establish ownership, so filter even + // though this query pinned an exact service (#55). + stubs = [.. DecodeArray(result).Where(IsOwnedByThisApp)]; } finally { @@ -732,9 +787,7 @@ private List QueryAllItemsForApp(bool includeData) try { var items = DecodeArray(result); - stubs = [.. items - .Where(i => i.Service.StartsWith(_appIdentifier + ".", StringComparison.Ordinal) - || string.Equals(i.Service, SelectionsService, StringComparison.Ordinal))]; + stubs = [.. items.Where(IsOwnedByThisApp)]; } finally { @@ -832,6 +885,7 @@ private static List DecodeArray(IntPtr arrayOrDict) Description = ReadCfStringAt(dict, Constants.KSecAttrDescription), CreatedAt = ReadCfDateAt(dict, Constants.KSecAttrCreationDate), Data = ReadCfDataAt(dict, Constants.KSecValueData), + OwnerAppIdentifier = ReadCfStringAt(dict, Constants.KSecAttrComment), }; } diff --git a/src/NextIteration.SpectreConsole.Auth/Persistence/Keychain/KeychainInterop.cs b/src/NextIteration.SpectreConsole.Auth/Persistence/Keychain/KeychainInterop.cs index 9c2c4e9..d2838ed 100644 --- a/src/NextIteration.SpectreConsole.Auth/Persistence/Keychain/KeychainInterop.cs +++ b/src/NextIteration.SpectreConsole.Auth/Persistence/Keychain/KeychainInterop.cs @@ -153,6 +153,18 @@ internal static class Constants internal static readonly IntPtr KSecAttrLabel = LoadSymbol(Security, "kSecAttrLabel"); internal static readonly IntPtr KSecAttrDescription = LoadSymbol(Security, "kSecAttrDescription"); internal static readonly IntPtr KSecAttrGeneric = LoadSymbol(Security, "kSecAttrGeneric"); + + /// + /// kSecAttrComment. Used to record which app owns an item. + /// + /// + /// Deliberately not kSecAttrGeneric, which participates in the primary + /// key for generic-password items on some macOS versions — adding it to items + /// that previously lacked it would change uniqueness semantics and could let a + /// legacy item and a new one coexist for the same service + account. + /// kSecAttrComment is plain metadata and carries no such risk. + /// + internal static readonly IntPtr KSecAttrComment = LoadSymbol(Security, "kSecAttrComment"); internal static readonly IntPtr KSecAttrCreationDate = LoadSymbol(Security, "kSecAttrCreationDate"); internal static readonly IntPtr KSecValueData = LoadSymbol(Security, "kSecValueData"); diff --git a/tests/NextIteration.SpectreConsole.Auth.Tests/Persistence/KeychainCredentialManagerTests.cs b/tests/NextIteration.SpectreConsole.Auth.Tests/Persistence/KeychainCredentialManagerTests.cs index d3ba450..ed4083a 100644 --- a/tests/NextIteration.SpectreConsole.Auth.Tests/Persistence/KeychainCredentialManagerTests.cs +++ b/tests/NextIteration.SpectreConsole.Auth.Tests/Persistence/KeychainCredentialManagerTests.cs @@ -455,6 +455,62 @@ public async Task SelectCredentialAsync_ReplacingAnExistingSelection_LeavesExact Assert.False(listed.Single(c => c.AccountId == prod).IsSelected); } + [Fact] + [SupportedOSPlatform("macos")] + public async Task Items_AreInvisibleToAnAppWhoseIdentifierIsADotPrefix() + { + Assert.SkipWhen(_skip, SkipReason); + + // "…cli" is a dot-prefix of "…cli.pro", and the service string is + // "{app}.{provider}" — so the neighbour's prefix test matched pro's item and it + // could list, export and delete another CLI's credentials (#55). + var neighbourId = $"test.nextiteration.sca.{Guid.NewGuid():N}"; + var neighbour = new KeychainCredentialManager(neighbourId); + var pro = new KeychainCredentialManager(neighbourId + ".pro"); + + var id = await pro.AddCredentialAsync("Adobe", "prod", "Production", "{\"apiKey\":\"PRO\"}"); + try + { + // pro's service is "{neighbourId}.pro.Adobe": it still starts with + // "{neighbourId}." so the old check matched, but the owner attribute does not. + Assert.Empty(await neighbour.GetProviderNamesAsync()); + Assert.Empty(await neighbour.ExportCredentialsAsync()); + Assert.Empty(await neighbour.ListCredentialsAsync("pro.Adobe")); + + // And the owner still sees its own item. + Assert.Single(await RetryHelper.UntilAsync(() => pro.ExportCredentialsAsync(), r => r.Count == 1)); + } + finally + { + _ = await pro.DeleteCredentialAsync(id); + } + } + + [Fact] + [SupportedOSPlatform("macos")] + public async Task NeighbourCannotDeleteAnotherAppsCredential() + { + Assert.SkipWhen(_skip, SkipReason); + + var neighbourId = $"test.nextiteration.sca.{Guid.NewGuid():N}"; + var neighbour = new KeychainCredentialManager(neighbourId); + var pro = new KeychainCredentialManager(neighbourId + ".pro"); + + var id = await pro.AddCredentialAsync("Adobe", "prod", "Production", "{\"apiKey\":\"PRO\"}"); + try + { + // The destructive half of the same bug: delete resolves the item through the + // same app-scoped lookup, so an unfixed prefix test let a neighbour remove + // credentials it does not own. + Assert.False(await neighbour.DeleteCredentialAsync(id)); + Assert.Single(await RetryHelper.UntilAsync(() => pro.ExportCredentialsAsync(), r => r.Count == 1)); + } + finally + { + _ = await pro.DeleteCredentialAsync(id); + } + } + private sealed class FakeAdobeSummaryProvider : ICredentialSummaryProvider { public string ProviderName => "Adobe";