Fix for issue #711: The specified item already exists in the keychain, iOS Code -25299, -25300

[frankteller-de]

My suggestion for fixing bugs 25299 and 25300, which have existed on iOS since version greater than 9.0.0.

Original issue: The specified item already exists in the keychain, IOS #711

The problem occurred when the parameter kSecAttrAccessible was used to check whether a certain key already exists. The parameters kSecAttrAccount and kSecAttrAccessible together are not unique. The key kSecAttrAccount can therefore only occur once, even if kSecAttrAccessible is different.

In addition, kSecAttrAccessible cannot be changed by SecItemUpdate. This requires the key to be deleted and recreated.

Another problem arises with the kSecAttrSynchronizable parameter. To check whether a key already exists, it must be checked with and without synchronize. Not setting kSecAttrSynchronizable in order to ignore it in the query does not seem to help here. The current workaround is to read the value with and without sync.

[dballance]

This is the correct PR to merge. Please do NOT merge #734 as it will exacerbate the issue.

The parameters that were optional (kSecAttrAccessible) should be made optional again. This PR does that, and should be merged to fix the issues in #711

[paulking86]

Excellent work @frankteller-de 👏

[dballance]

I think this PR may also be missing the ability to set the accessibility key to null via dart. Otherwise, the value is still always set.

[frankteller-de]

I think this PR may also be missing the ability to set the accessibility key to null via dart. Otherwise, the value is still always set.

Hi, why do you think that? In this PR, accessibility should be optional.

if (accessibility != nil) {
    keychainQuery[kSecAttrAccessible] = parseAccessibleAttr(accessibility: accessibility)
}

If it is not specified in flutter, the attribute is ignored.
But at the latest apple will set kSecAttrAccessibleWhenUnlocked as the default attribute.

Or have I missed something?

[dballance]

@frankteller-de - apologies for my late reply - I'm on paternity leave with a new little one so I've been a bit behind!

I believe we also need to update the Dart side of things in IOSOptions and AppleOptions to allow setting accessibility to null.

Here is the current state:

class IOSOptions extends AppleOptions {
  const IOSOptions({
    String? groupId,
    String? accountName = AppleOptions.defaultAccountName,
    KeychainAccessibility accessibility = KeychainAccessibility.unlocked,
    bool synchronizable = false,
  })

With the above, it's impossible to set accessibility to null on calls. Accessibility will always be KeychainAccessibility.unlocked.

Previously, this wasn't an issue because this value was only utilized on write calls on the Swift side, but now that it's included in every call, and there is no way to set it to null on the Dart side, the nil changes on the iOS side won't have any impact. It'll still always be kSecAttrAccessibleWhenUnlocked.

[juliansteenbakker]

Hi all, if i read everything correctly, this PR is done for merge, however the only thing on the dart side AppleOptions that should change is this:

    KeychainAccessibility? accessibility,
    // KeychainAccessibility accessibility = KeychainAccessibility.unlocked,

I will make this change in a new major version since it would change the current default behavior.

[paulking86]

@dballance Has kindly done some work on this already @juliansteenbakker. This is the commit you want I believe (although I am unsure if we still want the default values there)

Tango-Tango@400f6b4

[juliansteenbakker]

Thank you for pointing me in the good direction @paulking86 ! Since that change is not breaking, i will merge it right away.