feat: Swift and Kotlin credential storage tests#271
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Made-with: Cursor
…ign-tests-v2 Made-with: Cursor # Conflicts: # swift/tests/WalletKitTests/LoggingTests.swift
|
@codex review |
| } | ||
| } | ||
|
|
||
| public func openSealed(associatedData: Data, ciphertext: Data) throws -> Data { |
There was a problem hiding this comment.
@forceunwrap @NnnOooPppEee could you help double check this is how we should do things?
| override fun seal(associatedData: ByteArray, plaintext: ByteArray): ByteArray { | ||
| try { | ||
| val key = getOrCreateKey() | ||
| val cipher = Cipher.getInstance("AES/GCM/NoPadding") |
There was a problem hiding this comment.
@Guardiola31337 @NnnOooPppEee this warrants particular attention
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1bc22854b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!temp.renameTo(file)) { | ||
| throw StorageException.BlobStore("failed to atomically replace existing file") |
There was a problem hiding this comment.
writeAtomic writes to a temp file and then throws if temp.renameTo(file) returns false, but File.renameTo is allowed to fail when the destination already exists. In environments/filesystems where that happens, any rewrite of an existing blob path will fail, so callers cannot safely update previously written data even though writeAtomic is expected to replace atomically. Use a replace-capable move (REPLACE_EXISTING/ATOMIC_MOVE where available) or an explicit overwrite fallback.
Useful? React with 👍 / 👎.
forceunwrap
left a comment
forceunwrap
left a comment
There was a problem hiding this comment.
Reviewed from iOS/swift perspective.
|
|
||
| private func keychainBaseQuery() -> [String: Any] { | ||
| [ | ||
| kSecClass as String: kSecClassGenericPassword, |
There was a problem hiding this comment.
This is different from iOS's current implementation (SecKeyCreateEncryptedData with .eciesEncryptionCofactorVariableIVX963SHA256AESGCM). Are they meant to be compatible? This doesn't use the secure enclave either. It's missing kSecAttrTokenIDSecureEnclave for real devices.
There was a problem hiding this comment.
/otto please update this test to match the iOS implementation that @forceunwrap pointed out
There was a problem hiding this comment.
I've read the current IOSDeviceKeystore implementation and the thread. Before making the change I want to confirm scope, since this touches data compatibility:
What @forceunwrap pointed out (from the thread):
- Current impl:
AES.GCM+ symmetric key stored inkSecClassGenericPasswordkeychain,kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly - world-app-ios impl:
SecKeyCreateEncryptedDatawith.eciesEncryptionCofactorVariableIVX963SHA256AESGCM, EC key pair backed bykSecAttrTokenIDSecureEnclave,kSecAttrAccessibleWhenUnlockedThisDeviceOnly
Switching to the SE-based ECIES approach would mean:
- Data format changes — blobs sealed with the current AES-GCM approach can't be opened with ECIES, so this is a breaking storage migration if any existing data uses this keystore.
- Associated data handling —
SecKeyCreateEncryptedDatahas no AAD parameter; theseal(associatedData:plaintext:)contract would need a custom AAD scheme (e.g. prepend AD to plaintext or HMAC over ciphertext). - Simulator/test environment — SE isn't available in simulators, so the current
errSecMissingEntitlementfallback path would need to change.
Questions before I proceed:
- Is
IOSDeviceKeystorein this PR meant to replaceWalletKitSecureEnclave.swiftin world-app-ios, or is it independent new storage that doesn't need to be compatible? - If switching to ECIES/SE: how should
associatedDatabe handled? Should I mirror exactly whatWalletKitSecureEnclave.swiftdoes, or use a new scheme? - Or is the ask narrower — e.g. just update the keychain accessibility attribute to
kSecAttrAccessibleWhenUnlockedThisDeviceOnlyto match the existing app?
Happy to make the full change once the intent is clear. cc @forceunwrap
| public static func makeDefaultProvider( | ||
| bundleIdentifier: String? = Bundle.main.bundleIdentifier | ||
| ) throws -> IOSStorageProvider { | ||
| let fileManager = FileManager.default |
There was a problem hiding this comment.
Currently, we store in Documents/worldid/ (via StoragePaths.fromRoot) this would move storage to Library/Application Support/
Is that intended?
There was a problem hiding this comment.
/otto also update to match
There was a problem hiding this comment.
Done — pushed in 54a1188.
Changes to makeDefaultProvider:
- Switched from
.applicationSupportDirectoryto.documentDirectory, so the resolved blob-store base becomesDocuments/worldid/matching the existing app layout. - Removed the
bundleIdentifierparameter (not needed — iOS sandboxing already gives each app its ownDocuments/folder, so there's no collision to avoid).
Note: on the broader IOSDeviceKeystore question I've asked for clarification in that thread before making the encryption-scheme change — that one has data-compatibility implications worth confirming first.
| kSecClass as String: kSecClassGenericPassword, | ||
| kSecAttrService as String: service, | ||
| kSecAttrAccount as String: account, | ||
| kSecAttrAccessible as String: kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly |
There was a problem hiding this comment.
This is different from the existing DeviceKeystoreImp use of
kSecAttrAccessibleWhenUnlockedThisDeviceOnly. Do we want to change this to make the keychain accessible when the device is locked?
There was a problem hiding this comment.
@forceunwrap is the point that this is different in the iOS implementation? the data should definitely not be accessible if the device is locked
| import Foundation | ||
| import Security | ||
|
|
||
| public final class IOSDeviceKeystore: DeviceKeystore { |
There was a problem hiding this comment.
nit: I think this and WalletKitFileSystem will will bring in swift 6 concurrency errors. Maybe unchecked Sendable or an actor?
There was a problem hiding this comment.
Fixed in 577d51a — added , @unchecked Sendable to all three iOS support classes:
IOSDeviceKeystore— already usesNSLock/fallbackLockto protect all mutable state, so the invariant holdsIOSAtomicBlobStore—baseURLis immutable after init;FileManager.defaultisn'tSendableitself but is safe to use concurrentlyIOSStorageProvider— all fields are set once ininitand never mutated
Went with @unchecked Sendable over actor since making the protocol methods async would break the UniFFI-generated call site.
|
Let me look at the PR and the relevant comments to understand the full context. The request to "update this test to match the iOS implementation" sounds straightforward, but the two implementations are fundamentally different:
Switching would be a breaking change (old ciphertext can't be opened), and ECIES doesn't natively support the
|
makeDefaultProvider previously resolved to Library/Application Support/<bundleId>/ which differs from the existing iOS app's Documents/worldid/ layout. Switch to .documentDirectory (no bundle ID subdirectory needed — iOS sandboxing already isolates each app's Documents folder) so that the resulting blob store base path is Documents/worldid/, matching the existing app. Also drops the unused parameter.
|
Let me read the full Let me make that change on the branch.
The |
IOSDeviceKeystore, IOSAtomicBlobStore, and IOSStorageProvider all use internal synchronisation (NSLock / FileManager) or are effectively immutable after init, so @unchecked Sendable is the right approach rather than converting to actors (which would make every method async and break the UniFFI-generated protocol surface).
|
All three classes need the fix.
Chose |
|
The subagent did the same file-fetching work I'd already done in parallel — no further action needed from it. I'll leave it as complete. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 577d51a. Configure here.
| kSecClass as String: kSecClassGenericPassword, | ||
| kSecAttrService as String: service, | ||
| kSecAttrAccount as String: account, | ||
| kSecAttrAccessible as String: kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly |
There was a problem hiding this comment.
Keychain accessibility allows access when device is locked
High Severity
The keychain query uses kSecAttrAccessibleAfterFirstUnlockThisDeviceOnly, which allows the encryption key to be read even while the device is locked (after the first unlock since boot). The PR discussion explicitly confirms the intent is that "the data should definitely not be accessible if the device is locked," and the existing implementation uses kSecAttrAccessibleWhenUnlockedThisDeviceOnly. This weaker accessibility level exposes credential key material to attack scenarios where the device is locked but powered on.
Reviewed by Cursor Bugbot for commit 577d51a. Configure here.
| .setEncryptionPaddings(KeyProperties.ENCRYPTION_PADDING_NONE) | ||
| .setKeySize(256) | ||
| .setRandomizedEncryptionRequired(true) | ||
| .build() |
There was a problem hiding this comment.
Android keystore key accessible when device is locked
High Severity
The KeyGenParameterSpec does not call setUnlockedDeviceRequired(true) (available on API 28+), meaning the AES key can be used for encrypt/decrypt operations even when the device is locked. The PR owner confirmed "the data should definitely not be accessible if the device is locked." While minSdk is 23 and this API requires 28, a runtime version check could conditionally enable this protection on supported devices. Currently no lock-screen restriction is applied on any API level.
Reviewed by Cursor Bugbot for commit 577d51a. Configure here.
Guardiola31337
left a comment
Guardiola31337
left a comment
There was a problem hiding this comment.
Android/Kotlin consumer questions from the World App side. I skipped the lock-state and atomic-replace points already covered in existing threads.
| try { | ||
| val keyStore = KeyStore.getInstance("AndroidKeyStore") | ||
| keyStore.load(null) | ||
| val existing = keyStore.getKey(alias, null) as? SecretKey |
There was a problem hiding this comment.
Question: if we tighten this key spec, how should we handle an existing key already stored under this alias?
Right now we silently reuse whatever walletkit_device_key resolves to. Should the alias be versioned, or should we inspect KeyInfo and rotate/delete keys that do not meet the expected lock-state / hardware-backed requirements?
| return existing | ||
| } | ||
|
|
||
| val keyGenerator = KeyGenerator.getInstance( |
There was a problem hiding this comment.
Question: is this intended to be a production-grade Android default, or a minimal reference implementation?
As a consumer, I’d expect the default to either attempt/verify StrongBox or TEE backing, or clearly document that apps needing those guarantees should inject their own DeviceKeystore. Otherwise this may look like a safe default while being weaker than existing app integrations. For reference, World App’s current Android wrapper goes through SecureEnvironmentService, which attempts StrongBox/TEE and verifies the resulting key security level.
| } | ||
|
|
||
| override fun writeAtomic(path: String, bytes: ByteArray) { | ||
| val file = File(baseDir, path) |
There was a problem hiding this comment.
Question: should this resolve and canonicalize path before using it?
Even if the Rust core currently only passes known filenames like account_keys.bin, this is a public Android blob-store implementation. Without checking that the canonical target stays under baseDir, ../ or absolute paths could escape the storage root.
| "${file.name}.tmp-${UUID.randomUUID()}" | ||
| ) | ||
| try { | ||
| temp.writeBytes(bytes) |
There was a problem hiding this comment.
Question: separate from the existing atomic-replace discussion, do we need to fsync the temp file before renaming it?
Since this can store account_keys.bin, I’m wondering whether a successful writeAtomic should mean the bytes are durable across process/device crash, or only that readers will not observe a partial file.
|
|
||
| object WalletKitStorage { | ||
| fun defaultProvider(context: Context): AndroidStorageProvider { | ||
| val root = File(context.filesDir, "walletkit") |
There was a problem hiding this comment.
Question: should this default root and keystore alias be called out as new storage, not a migration path for existing Android consumers?
This uses filesDir/walletkit/worldid plus the walletkit_device_key alias. Existing clients may already use a different root and keystore alias, so switching to WalletKitStorage.defaultProvider(context) could make existing credentials appear missing.
4 participants
Note
Medium Risk
Introduces new platform keystore and blob-store implementations (AES-GCM + AndroidKeyStore/iOS Keychain) and exercises persistence in tests, which can affect confidentiality and data durability if incorrect.
Overview
Adds end-to-end Swift and Kotlin tests for credential storage, covering
CredentialStoreinit requirements, credential persistence across reopen, and Merkle proof cache TTL behavior, plus basic atomic blob store read/write/delete semantics.Introduces platform storage implementations: Android
AndroidDeviceKeystore,AndroidAtomicBlobStore, andAndroidStorageProvider(withWalletKitStorage.defaultProvider), and iOSIOSDeviceKeystore,IOSAtomicBlobStore, andIOSStorageProvider(withWalletKitStorage.makeDefaultProvider). Also updates Swift test harness (test_swift.sh) to stage local support Swift sources and renames logging test classes toLoggingTests.Reviewed by Cursor Bugbot for commit 54a1188. Bugbot is set up for automated code reviews on this repo. Configure here.