fix(swift-sdk): keychain privk storage#3946
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesKeychain Identity Key Lookup Optimization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ZocoLini
force-pushed
the
fix/keychain-privk-storage
branch
from
59a6796 to
7ccee69
Compare
ZocoLini
changed the title
ZocoLini
requested review from
QuantumExplorer,
llbartekll and
shumkov
as code owners
|
✅ Review complete (commit 1804f3f) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift (1)
633-636:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale comment: describes old scan-all behavior that no longer applies.
Lines 633-636 state that
retrieveIdentityPrivateKey(publicKeyHex:)"scans everyidentity_privkey.*item and matches on the metadata'spublicKeyhex", but the method now performs a directkSecAttrLabellookup. This mismatch will confuse future maintainers.📝 Suggested comment update
// Including the wallet id (hex) in the account name keeps // every wallet's identity-key set independent. Reads via - // `retrieveIdentityPrivateKey(publicKeyHex:)` still work - // unmodified — that path scans every `identity_privkey.*` - // item and matches on the metadata's `publicKey` hex, so the - // account-name shape doesn't matter for lookup. The direct + // `retrieveIdentityPrivateKey(publicKeyHex:)` still work + // because that path queries `kSecAttrLabel` (the lowercased + // public-key hex written at store time), so the account-name + // shape doesn't matter for lookup. The direct // `retrieveKeyData(identifier:)` path uses whatever string🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift` around lines 633 - 636, The comment block near lines 633-636 describes outdated behavior for the retrieveIdentityPrivateKey(publicKeyHex:) method, stating it scans every identity_privkey item and matches on metadata's publicKey hex, when it actually now performs a direct kSecAttrLabel lookup. Update this comment block to accurately describe the current implementation behavior of the method, removing references to the scan-all approach and clarifying that it uses a direct label-based lookup instead.
🧹 Nitpick comments (1)
packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift (1)
737-771: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider aligning
hasIdentityPrivateKeywith the new direct-lookup pattern.This method still uses the scan-all-and-decode approach while
retrieveIdentityPrivateKeynow uses directkSecAttrLabellookup. The current implementation is correct (usescaseInsensitiveCompare), but the inconsistency may cause confusion and the scan is less efficient.Since this is a UI diagnostic helper per the doc comment, the performance impact is likely negligible, so this could be deferred.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift` around lines 737 - 771, The hasIdentityPrivateKey method currently uses a scan-all-and-decode pattern with kSecMatchLimitAll to iterate through all items and compare public keys, while retrieveIdentityPrivateKey has been refactored to use direct kSecAttrLabel lookup which is more efficient. Align hasIdentityPrivateKey with the same direct-lookup pattern by using kSecAttrLabel to query for the specific identity private key entry directly, eliminating the need to scan and decode all items. This improves consistency and performance, though the impact is minimal since this is a UI diagnostic helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift`:
- Around line 633-636: The comment block near lines 633-636 describes outdated
behavior for the retrieveIdentityPrivateKey(publicKeyHex:) method, stating it
scans every identity_privkey item and matches on metadata's publicKey hex, when
it actually now performs a direct kSecAttrLabel lookup. Update this comment
block to accurately describe the current implementation behavior of the method,
removing references to the scan-all approach and clarifying that it uses a
direct label-based lookup instead.
---
Nitpick comments:
In `@packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift`:
- Around line 737-771: The hasIdentityPrivateKey method currently uses a
scan-all-and-decode pattern with kSecMatchLimitAll to iterate through all items
and compare public keys, while retrieveIdentityPrivateKey has been refactored to
use direct kSecAttrLabel lookup which is more efficient. Align
hasIdentityPrivateKey with the same direct-lookup pattern by using kSecAttrLabel
to query for the specific identity private key entry directly, eliminating the
need to scan and decode all items. This improves consistency and performance,
though the impact is minimal since this is a UI diagnostic helper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 77148128-55c9-48e7-8d1b-ba0ea616be4e
📒 Files selected for processing (1)
packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Small, focused fix that switches identity-private-key lookup from a scan-and-decode loop to a direct kSecAttrLabel query, addressing macOS file-keyring iteration quirks. One legitimate upgrade-compatibility concern (legacy unlabeled rows become invisible to the fallback path) and two stale docstrings/comments that still describe the removed scan behavior. Speculative defense-in-depth concerns from the security passes do not represent real issues and are dropped.
🟡 1 suggestion(s) | 💬 2 nitpick(s)
2 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/Security/KeychainManager.swift:710-726: Legacy keychain rows without kSecAttrLabel become invisible to this fallback
Before this PR, `storeIdentityPrivateKey` never set `kSecAttrLabel`, so any row already on a device from a prior build has no label attribute. The new query pins `kSecAttrLabel == publicKeyHex.lowercased()`, so `SecItemCopyMatching` returns `errSecItemNotFound` for those legacy rows and the signer trampoline treats the secret as missing until the persister happens to re-`storeIdentityPrivateKey` and overwrite the row with a labeled version. The primary signing path (`retrieveKeyData(identifier:)` via `PersistentPublicKey.privateKeyKeychainIdentifier`) still works, so this only affects the public-key fallback used mid-registration / before the SwiftData row is written — but for users upgrading mid-flow it silently breaks a previously working path. If pre-existing user data is possible, either (a) fall back to the prior metadata scan when the labeled query misses, (b) run a one-shot `SecItemUpdate` migration over `identity_privkey.*` rows to backfill `kSecAttrLabel`, or (c) document that callers must trigger a re-persist after upgrade. If the SDK has not shipped to real users yet, call that out explicitly so this is closed knowingly.
ZocoLini
force-pushed
the
fix/keychain-privk-storage
branch
from
7ccee69 to
1804f3f
Compare
3 participants
How Private keys were been stored didn't work well with the legacy keyring the swift tests where using without signing the app, what I do with this PR is updated the store/load logic to use the public key as the keyring entry label, being able to query for a single priv key instead of requesting a bunch oof items to later iterate over them, what doesn't work well in macOS file based keyring
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit