Skip to content

fix: macOS keychain item metadata defaults#61

Merged
rianjs merged 1 commit into
mainfrom
bug/60-keychain-metadata-trust
Jun 12, 2026
Merged

fix: macOS keychain item metadata defaults#61
rianjs merged 1 commit into
mainfrom
bug/60-keychain-metadata-trust

Conversation

@rianjs

@rianjs rianjs commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Summary

  • generate deterministic keychain labels/descriptions from the credstore service and item key
  • enable trusted-application defaults for macOS keychain writes, document the rewrite-only migration boundary, and preserve metadata through the ByteNess adapter
  • cover the metadata/trust behavior with hermetic unit tests, including the production osKeyring write path

Verification

  • rtk go test ./credstore
  • rtk go test ./...
  • rtk go test -race -tags keyring_no1password,keyring_nopassage ./...
  • rtk make check
  • rtk security find-generic-password -s credstore-manual-check -a default/tok

@rianjs

rianjs commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

Major

  • credstore/osbackend_core.go:130 changes the shared macOS Keychain write contract by trusting the current application by default, and credstore/osbackend_core.go:141 starts generating deterministic item metadata, but the standards docs are unchanged. Since cli-common is the standards home, this needs a small docs/working-with-secrets.md update documenting: new writes/overwrites get label/description, Keychain writes trust the signed caller by default, and existing items are not migrated until rewritten. The PR body is not enough for a shared behavior change.

Minor

  • credstore/osbackend_core.go:233 is the production path that stamps metadata before calling the backend, but the tests only cover the pure helper at credstore/osbackend_test.go:413 and the ByteNess adapter pass-through. Add a fake-backend test for osKeyringBackend.set with service: "codereview" that captures the keyringItem; that pins the actual Store write path.

Verification I ran:

  • go test ./credstore
  • go test -tags keyring_no1password,keyring_nopassage ./...

@rianjs rianjs force-pushed the bug/60-keychain-metadata-trust branch from 9644551 to b7988af Compare June 12, 2026 21:26
@rianjs

rianjs commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

No architectural findings.

The updated PR matches the intended #60 scope: metadata/trust defaults stay inside credstore, the public Store API remains unchanged, file-backend compatibility is preserved, the standards doc now records the new-write/overwrite-only boundary, and tests cover both the ByteNess adapter and the production osKeyringBackend.set stamping path.

Verification I ran:

  • go test ./credstore
  • go test -tags keyring_no1password,keyring_nopassage ./...
  • go test ./...

@rianjs

rianjs commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

Main gap: the suite still proves internal plumbing, not the macOS keychain behavior the ticket is about. The fake adapter test in credstore/osbackend_byteness_test.go and the struct-level assertions in credstore/osbackend_test.go and credstore/osbackend_test.go would all pass if byteness/keyring ignored Label/Description or if KeychainTrustApplication mapped to the wrong ACL semantics. The gated real-backend test in credstore/osbackend_test.go only checks secret round-tripping, not the metadata or access-control state that users actually see. I’d want one macOS-gated test that writes an item and then inspects the stored keychain metadata/ACL back from the real backend.

Minor: there is no overwrite-specific assertion. The ticket explicitly says new or overwritten items should get the metadata, but credstore/osbackend_test.go only covers a first write into an empty fake keyring. A regression that only stamps metadata on insert, or only on overwrite, would still pass. A single overwrite case would pin that edge without expanding the suite much.

@rianjs rianjs merged commit 74c4b8e into main Jun 12, 2026
4 checks passed
@rianjs rianjs deleted the bug/60-keychain-metadata-trust branch June 12, 2026 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant