Rewrite persistence to use Windows / macOS / Linux native keyrings#59
Open
matt-edmondson wants to merge 3 commits into
Open
Rewrite persistence to use Windows / macOS / Linux native keyrings#59matt-edmondson wants to merge 3 commits into
matt-edmondson wants to merge 3 commits into
Conversation
The library previously claimed in its README to "encrypt sensitive credentials
using platform-specific security features" but actually serialized credentials
as plaintext JSON to AppData via ktsu.PersistenceProvider. The upstream
ktsu.UniversalSerializer / ktsu.FileSystemProvider / ktsu.PersistenceProvider
namespaces have also shifted, so the project no longer even restored against
the netstandard2.0/2.1 targets the SDK was producing.
Highlights:
- Introduce ICredentialStore with one implementation per supported OS, each
storing one entry per PersonaGUID in the host's native keyring:
Windows -> Credential Manager via advapi32 (CredRead/CredWrite/CredDelete)
macOS -> Keychain Services via Security.framework (SecKeychain*)
Linux -> libsecret (Secret Service) via libsecret-1.so.0
Plus an InMemoryCredentialStore for tests and explicit opt-out.
- Drop the broken AppData / UniversalSerializer / FileSystemProvider deps and
serialize Credential subclasses directly via System.Text.Json, wiring
ktsu.RoundTripStringJsonConverter so SemanticString values round-trip
correctly (previously broke as IEnumerable<char>).
- Make CredentialCache constructible with an explicit ICredentialStore in
addition to the singleton, expose Remove / Dispose semantics, and add
ResetSingletonForTesting so test runs no longer pollute user AppData.
- Constrain TargetFrameworks to net9.0;net10.0 (matches what the remaining
ktsu deps actually publish) and add a cross-platform CI workflow that
builds and tests on ubuntu/macos/windows.
- Rewrite README so it documents the real API (PersonaGUID-keyed
AddOrReplace/TryGet/Remove) and the actual at-rest behaviour per platform.
https://claude.ai/code/session_017B9mN9F7C3pWGZRyoKBd6R
The cross-platform CI run on PR #59 fails on Linux and macOS with IDE0055 (formatting) because .editorconfig declares `end_of_line = crlf` for the whole tree, but actions/checkout@v4 leaves files at their git-normalised LF on non-Windows runners. The original CI was Windows-only, where the hosted runner's git defaults to autocrlf=true, masking the discrepancy. Mark .cs/.csproj/.props/.targets/.editorconfig as `text eol=crlf` so every checkout (including Linux and macOS) renders these files with CRLF to match the editorconfig declaration. The committed blob in git stays normalised; this only affects the working tree. https://claude.ai/code/session_017B9mN9F7C3pWGZRyoKBd6R
`global.json` selects `Microsoft.Testing.Platform` as the test runner, which does not accept the legacy VSTest `--logger "console;verbosity=normal"` form. The argument was being misinterpreted as a project path and the runner reported `Zero tests ran` with exit code 1 on every platform. Removing the unused logger flag is enough -- the platform's default console output is sufficient and all 17 tests pass locally with the same command. https://claude.ai/code/session_017B9mN9F7C3pWGZRyoKBd6R
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Audit findings
This branch is the result of an audit-and-finish pass on the library. The headline issues found on
main:%APPDATA%/ktsu/CredentialCacheviaktsu.PersistenceProvider. There was no encryption anywhere.CredentialCache.csprojinheritedTargetFrameworks=net5..10;netstandard2.0;netstandard2.1fromktsu.Sdkbut every upstreamktsu.*Providerdep only shipsnet9.0/net10.0.dotnet restorefailed on every non-net9/10 TFM.ktsu.UniversalSerializer.Json.JsonSerializerhad moved toktsu.UniversalSerializer.Services.Json.JsonSerializer;FileSystemProvider's ctor now requires options. The library no longer even compiled against the pinned packages.UsernamePasswordCredential,ApiKeyCredential,OAuthCredential,CertificateCredential,CredentialCacheOptions,Store/Get/TryGet/Contains/Remove/Clear, expiration — none of it was real. The real surface is aCredentialCache.Instancesingleton overPersonaGUIDkeys.runs-on: windows-latestwas the only matrix entry.What's in this PR
New persistence layer (
Storage/)WindowsCredentialStoreadvapi32.dll—CredReadW/CredWriteW/CredDeleteW/CredEnumerateWMacOsCredentialStoreSecurity.framework—SecKeychainAddGenericPassword/Find/ModifyAttributesAndData/DeleteLinuxSecretServiceCredentialStorelibsecret-1.so.0—secret_password_store_sync/lookup_sync/clear_syncInMemoryCredentialStoreEach
PersonaGUIDbecomes its own entry in the OS keyring (service-scoped).CredentialStoreFactory.CreateDefault()picks the right one for the currentRuntimeInformation.IsOSPlatform(...)and throwsPlatformNotSupportedExceptionotherwise.CredentialStoreExceptionwraps native failures with the underlyingWin32Exception/status code.CredentialCacherefactorCredentialCache(ICredentialStore)for DI and tests.Instancesingleton (lazy + thread-safe) for back-compat, but it now resolves its store viaCredentialStoreFactory.CreateDefault()instead of hard-wiring the brokenAppDataPersistenceProvider.Remove(PersonaGUID),Dispose()that doesn't double-flush, andResetSingletonForTesting()so tests stop polluting user AppData.ConfigurePersistenceProvider→ConfigureStoreto match the new abstraction.TryGetnow falls through to the backing store on a miss, populating the in-memory cache.Serialization
ktsu.AppDataStorage/ktsu.FileSystemProvider/ktsu.PersistenceProvider/ktsu.UniversalSerializer(broken upstream surface).CredentialSerializationhelper usingSystem.Text.Json+[JsonPolymorphic]directly.ktsu.RoundTripStringJsonConverterFactorysoSemanticString<T>values (CredentialToken,CredentialUsername,CredentialPassword,PersonaGUID) round-trip as JSON strings — without it, STJ treats them asIEnumerable<char>and explodes on deserialize.Project & CI
TargetFrameworkstonet9.0;net10.0(the only TFMs the remaining deps actually publish for)..github/workflows/cross-platform.yml: build + test matrix onubuntu-latest/windows-latest/macos-latestwithlibsecret-1-0installed on Linux. The existing publish-orienteddotnet.ymlis untouched.UseAppHost=falseso distro-specificMicrosoft.NETCore.App.Host.{rid}packages aren't required at restore time.Tests
CredentialCacheover a freshInMemoryCredentialStore— no shared singleton, no disk writes.Assert.ThrowsException<>→Assert.Throws<>for MSTest 4.x.Removeacross both layers,Disposesemantics, null-store guard, singleton store-injection behaviour, and a polymorphic JSON round-trip for everyCredentialsubclass.Platform caveats (also documented in README)
CredentialStoreException.libsecret-1installed and a running Secret Service (gnome-keyring / KWallet bridge / KeePassXC / …). Headless CI agents typically have neither — useInMemoryCredentialStore.EnumerateKeys()is fully implemented on Windows. macOS/Linux currently return an empty sequence (the modernSecItemCopyMatching/secret_search_syncpaths need substantial CoreFoundation / GLib marshalling for a use case most consumers can serve by tracking persona GUIDs themselves).Test plan
dotnet build CredentialCache.sln— clean onnet9.0+net10.0(3 SourceLink warnings only).dotnet teston Linux — 17/17 pass.WindowsCredentialStorewrites into the real Credential Manager (cannot exercise from CI sandbox).MacOsCredentialStoreagainst the login keychain.LinuxSecretServiceCredentialStoreagainstgnome-keyringor equivalent.https://claude.ai/code/session_017B9mN9F7C3pWGZRyoKBd6R
Generated by Claude Code