iOS: Error adding record: DuplicateItem thrown when setting secure value

[kensykora]

Description

One time, I experienced an exception being thrown when trying to set a secure value. This value was an updated refresh/oauth token which only gets updated occasionally, and it happened on an iOS simulator when I deployed an updated version of the app to the simulator after it sat idle overnight. Additionally I had done a full git clean git clean -xdf and rebuild after switching branches.

I'm unclear if this is a normal use case that I need to handle or if it is just a "Development Artifact" that I can safely ignore for production... right now I'm leaning toward it might be a rare scenario I need to handle... possibly during app updates?

Steps to Reproduce

I don't have reliable steps to reproduce unfortunately, nor a minimum reproduction sample. If it happens again I'll try to make one. The rough steps are

1. Set something into secure storage in an app on an iOS simulator
2. Clean the app in git, full clean -- git clean -xdf
3. Deploy an updated version of the app

Expected Behavior

Secure storage continues to work

Actual Behavior

Exception is thrown:

System.Exception: Error adding record: DuplicateItem
  at Xamarin.Essentials.KeyChain.SetValueForKey (System.String value, System.String key, System.String service) [0x00071] in <80b8ae4e16374c93b74637a8a9a92836>:0 
  at Xamarin.Essentials.SecureStorage.SetAsync (System.String key, System.String value, Security.SecAccessible accessible) [0x00027] in <80b8ae4e16374c93b74637a8a9a92836>:0 
  at Xamarin.Essentials.SecureStorage.PlatformSetAsync (System.String key, System.String data) [0x00007] in <80b8ae4e16374c93b74637a8a9a92836>:0 
  at Xamarin.Essentials.SecureStorage.SetAsync (System.String key, System.String value) [0x00021] in <80b8ae4e16374c93b74637a8a9a92836>:0 
  at App.MyApp.Services.LocalStorageServiceBase.set_IdToken (System.String value) [0x0001a] in /Users/kensykora/Workspace/App.myapp/src/App.MyApp/Services/LocalStorageServiceBase.cs:178 
  at App.MyApp.Services.Login.LoginService+<RefreshCurrentLogin>d__16.MoveNext () [0x0020d] in /Users/kensykora/Workspace/App.myapp/src/App.MyApp/Services/Login/LoginService.cs:123

Basic Information

• Version with issue: 1.0.0
• Last known good version: n/a
• IDE: VS for Mac
• Platform Target Frameworks:
  • iOS: 12.1
• Nuget Packages: 1.0.0
• Affected Devices:

Screenshots

n/a

Reproduction Link

LMK if you need one, I can try and spend some time and find more detail here.

[jamesmontemagno]

I'm not sure I understand the git clean part, that seems like it is your code... It seems like something else though. We can detect this and remove it and then re-add it. Have you tried to do that?

[jamesmontemagno]

Looking at the code we check to see if it exists and if so then we remove and re-add it. Maybe this is a threading issue? hmmmm

[kensykora]

I’ve added tracking to AppCenter for my app for this issue, but I appreciate the pull request #705 to try and address it. I’ll update when it’s released and see if it continues to pop up.

[jamesmontemagno]

Yeah, you could search the error message and then remove and re-add it probably, but it is very strange that it didn't remove it because we check that...

[kensykora]

Hmmm, I’ll check for a race condition. Since it’s an oauth token we were trying to refresh, it’s entirely possible 2 API calls were trying to happen at once.

I’m guessing the storage methods for encryption aren’t designed with thread safety?

[HadiHas]

Hello, we have the latest version of xamarin.essentials (1.5.1) and we still getting this error
System.Exception: Error adding record: DuplicateItem at Xamarin.Essentials.KeyChain.SetValueForKey (System.String value, System.String key, System.String service)
Were you able to resolve this issue?

[francis2]

Same here using Essentials 1.5.3.2. Error does not always occur. Last occurrence was on iPad 6th gen with iOS 13.4.1.

Adding SecureStorage.RemoveAll() before calling SetAsync() to see if it will help.

[munkii]

Same here using Essentials 1.5.3.2. Error does not always occur. Last occurrence was on iPad 6th gen with iOS 13.4.1.

Adding SecureStorage.RemoveAll() before calling SetAsync() to see if it will help.

Did it help? We get this a lot. Our AppCenter logs are full of KeyChain.SetValueForKey exceptions

[francis2]

It appears to have solved the issue for us, but it's a new app with few users so it's too soon to tell for sure.

[munkii]

@francis2 arw you only storing a single value in SecureStorage? If not surely RemoveAll would just remove all your values

[francis2]

yes I'm only storing a serialized token. Teh trick above fixed the issue for me.

[somoreingold]

@francis2 Interesting fix. What about just calling SecureStorage.Remove("YOUR_KEY")? Would that have been enough, or did you have to specifically do RemoveAll() to make the trick work?

[francis2]

Just Remove() did not work, I had to use RemoveAll.

…

On Thu, Dec 30, 2021 at 11:50 AM somoreingold ***@***.***> wrote: @francis2 <https://github.com/francis2> Interesting fix. What about just calling SecureStorage.Remove("YOUR_KEY")? Would that have been enough, or did you have to specifically do RemoveAll() to make the trick work? — Reply to this email directly, view it on GitHub <#704 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACUMTECBY22TUUTVIGNMWP3UTSZXJANCNFSM4GWASYZQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[somoreingold]

Ok thanks @francis2 . Can this be reopened since this issue still happens in the latest Xamarin and Xamarin.Essentials, given the only workaround is to delete all entries in SecureStorage @jamesmontemagno ?

[somoreingold]

Pretty please @jamesmontemagno? Seems like this is still reproducible in the latest XCT when 2 threads are trying to SetAsync at the same time for the same key. Since deep down, Xamarin.Essentials.SecureStorage.SetAsync isn't really an "update," it's a "remove" followed by an "add," something like this may be happening:

1. ThreadA starts SetAsync
2. ThreadB starts SetAsync
3. ThreadA deletes the old value
4. ThreadB tries to delete the old value but it doesn't exist anymore, so it adds the new value
5. ThreadA tries to add the new value but the DuplicateItem error happens instead.

[Axemasta]

This is something thats going to be addressed before the Xamarin sunset? Will this be brought forward to Maui?

[L-PLake]

I'm still having a lot these errors. Please open this reopen this issue.

[mphill]

Still occurring.

[mphill]

@jamesmontemagno Pretty sure a lock would resolve this:

		internal void SetValueForKey(string value, string key, string service)
		{
			lock (string.Intern(key)) 
			{
				using (var record = ExistingRecordForKey(key, service))
				{
					if (string.IsNullOrEmpty(value))
					{
						if (!string.IsNullOrEmpty(ValueForKey(key, service)))
							RemoveRecord(record);

						return;
					}

					// if the key already exists, remove it
					if (!string.IsNullOrEmpty(ValueForKey(key, service)))
						RemoveRecord(record);
				}

				using (var newRecord = CreateRecordForNewKeyValue(key, value, service))
				{
					var result = SecKeyChain.Add(newRecord);

					switch (result)
					{
						case SecStatusCode.DuplicateItem:
							{
								Debug.WriteLine("Duplicate item found. Attempting to remove and add again.");

								// try to remove and add again
								if (Remove(key, service))
								{
									result = SecKeyChain.Add(newRecord);
									if (result != SecStatusCode.Success)
										throw new Exception($"Error adding record: {result}");
								}
								else
								{
									Debug.WriteLine("Unable to remove key.");
								}
							}
							break;
						case SecStatusCode.Success:
							return;
						default:
							throw new Exception($"Error adding record: {result}");
					}
				}
			}
		}