Ability to set accessgroup on secure storage. Fixes #712

[jamesmontemagno]

Description of Change

Initial test run at adding AccessGroup for Secure storage. will pass this down to Apple Platform Keychain.

Android/UWP just update the Alias for a special group, which seems legit.

Tizen, nothing different.

Bugs Fixed

• Related to issue Specify iOS Keychain Access Group for SecureStorage #712

Provide links to issues here. Ensure that a GitHub issue was created for your feature or bug fix before sending PR.

API Changes

Added new parameters to all methods to pass down a string. Still backwards compatible.

Behavioral Changes

None from previous versions.

PR Checklist

• Has tests (if omitted, state reason in description)
• Has samples (if omitted, state reason in description)
• Rebased on top of master at time of PR
• Changes adhere to coding standard
• Updated documentation (see walkthrough)

[mattleibow]

Looks good at first glance. Will re review tomorrow 😁

[jamesmontemagno]

Lovely! Haven't tested it... so probably need a real test on it :)

[albernazf]

Hi guys,
Can I help with this in any way? Such an important feature. Otherwise I'll have to fork and apply the changes locally but would love to see that in the main branch.

[kiddailey]

I have been trying to get this to work without luck. A couple of weird issues.

• In the iOS device tests project, adding tests for this fails with a complaint that the entitlement isn't assigned (even though it is).

• When I try it in my own project, I can successfully save and retrieve a secure storage item, but only if I use App Groups (and not keychain access) and within a specific function -- if I try to retrieve a previously set secure key, I always get a null value.

I feel like I'm missing something, but haven't figured it out yet. I had to apply these changes manually, because the branch doesn't appear for me, so maybe I missed something :/

Edit: The entitlement issue may be due to my provisioning profile getting set to wildcard somehow.

Yes. For some reason, I could not get VS provisioning to properly select the correct profile and it constantly reverted to wildcard with the Xamarin Essentials project. After re-creating the provisioning stack from scratch and using manual provisioning, I was able to get the tests to pass, but only on a physical device. Even with the proper entitlements and provisioning profile set, you cannot test this in the simulator (which I kind of assumed).

I am still trying to get the second issue resolved -- where accessing a previously set key returns the expected result and not a null. Will update my post if/when I figure it out.

[kiddailey]

Can anyone else incorporate these changes and see if they can set and get a value from two separate code sections?

I'm thinking right now that it's my building of Essentials that is at fault. When I use SecureStorage without the acccessGroup I have the same issue. I reverted these changes and had the same issue. But when I grab essentials from Nuget, it works correctly.

Would be great to confirm if it's just me.

[jamesmontemagno]

It did build a nuget -> https://dev.azure.com/xamarin/public/_build/results?buildId=24379&view=logs&j=7868e7c0-c859-51b7-c3be-08c48f6833d0

That you could test. Although like I said I haven't tested it at all yet.

[kiddailey]

Thank you for replying! My preliminary tests with the nuget are looking good. So far, I have confirmed that I am able to:

• Set and Get with only a keychain access group entitlement (using the format: TeamID.KeyChainAccessGroupID)
• Set and Get with App Group access group entitlement

This was in a simple, single test app. My next test is to try it out with a container app/extension combination and see if they can share the data. Will post again when that is complete. Thanks again.

[kiddailey]

Can't seem to get the prerelease to install on my iOS extension project. I'm getting the dreaded "System.drawing/common not in GAC" error and haven't been able to find a resolution that works. Will try again later.

[jamesmontemagno]

On your extension can you convert it to use package references instead of packages.config?

https://docs.microsoft.com/en-us/nuget/consume-packages/migrate-packages-config-to-package-reference

[kiddailey]

Thanks. I couldn't -- the option was missing. I recreated the project and all is well. All of my other projects were already package references, so not sure what was going on. /shrugs

In any case, my container app test appears to work correctly. Just finishing up testing the extension now and will hopefully update with results later today.

Edit: For testing, I have a function that is the same identical function that is called in the container app and the extension app. It does a single call to SecureStorage.GetAsync() with an access group. When ran by the container app, it works great. When called by the extension app, it appears that the call is failing for some reason. Still digging...

Edit: When using SecureStorage in the ViewDidLoad() method of my extension, it doesn't crash. For some reason though, it crashes when it's used deeper down (specifically, when I use it in an instantiated object from another referenced project). The error is interesting:

2020-09-04 01:31:26.032957-0400 XXXXX.XXXXX.XXXXX.XXXXXExtension[231:3510] [User Defaults] Couldn't read values in CFPrefsPlistSource<0x281e4fb80> (Domain: group.XXXXX.XXXXX, User: kCFPreferencesAnyUser, ByHost: Yes, Container: (null), Contents Need Refresh: Yes): Using kCFPreferencesAnyUser with a container is only allowed for System Containers, detaching from cfprefsd

Still testing, but I think I'll see if keychain sharing with the team ID prefix works instead of app groups first. I get the same error whether I use keychain access groups or app groups :( Still digging...

Edit: I did some tests moving the GetAsync() up the chain a bit in my iOS extension app. Interestingly, when I call it a little earlier, I get a NULL value instead of a crash/error. All of this works great in my main container app. To be a little more clear:

• Extension view controller (null value)
  • Shared code called by extension view controller (null value)
    • Shared code called by previously shared code (crash/error)

I'm probably going to try and create a bare-bones extension app to eliminate all variables. I think there's a nuance here that I'm missing, but I haven't yet been able to put my finger on it.

[kiddailey]

I've finally had some time to create a very simple iOS container app and extension project to test and confirm my earlier findings:

• The container app is a single view app that Sets and Gets a SecureStorage key and displays the value for confirmation. The value used is a random GUID. (Screenshot)

• The extension is a share extension that when activated, displays the secure storage value retrieved by GetAsync as the text to be shared. (Screenshot)

As in my previous tests, everything works as expected in the main app (even across sleeps and force quits), but from the extension app it always returns a NULL value. Which is the same behavior as when you don't include the accessGroup.

In reverse it's the same -- secure storage keys created in the extension are only accessible in the extension and not in the container app.

I'm really hoping that someone else can discover something or show that it does work between apps like this, but for now I'm going to have to say that this update sadly doesn't quite work. :(

--

It's a bit of a chore to test this since it has to be on a device, but if anyone else is interested in playing around with the test app/extension, I guess I could put it up on github. You'll have to set up a handful of provisioning stuff (appIDs, appGroupIDs, etc) to get it working though, so it's not as simple as just running it. Let me know.

[Redth]

Hmm, do we really want to use $"{accessGroup}.{Alias}" here? I'm just thinking if I was writing my app I might expect the string to be exactly what I pass in. I'm torn of course because adding the Alias does keep it unique to Essentials' usage, but that might defeat the purpose of specifying an access group in the first place?

[jacob-maristany]

It makes sense to me. Although, perhaps there's a case that Alias should also be configurable. Perhaps if I have a suite of apps written in different tech stacks, yet they should have a common shared storage.

[kiddailey]

Am I understanding correctly that in the case of app groups, this means that you would need to provision a group in the format of:

 <group.groupName>.<packageName>.xamarinessentials

And wouldn't this also mean that sharing secure storage between two apps would be impossible because the package name (distinct between apps) that is added as part of Alias would always be different?

[mattleibow]

@kiddailey is correct... Why do we have the package name in there? Aren't prefs already unique to the app?

Maybe the access group should be the override for that?

Also, since things are shared on ios between the app/extension, what is the equivalent for Android? Does it just use the fact that a watch app is the same as a mobile? Is there a communication mechanism?

[kiddailey]

@kiddailey is correct... Why do we have the package name in there? Aren't prefs already unique to the app?
Maybe the access group should be the override for that?

Thanks for pointing this out @mattleibow. You are correct that the access group should override, and it actually does that in the Android implementation. iOS just needs to as well. And the package name cannot be part of it. See my post below detailing why.

[kiddailey]

Apologies for the slow and long-winded response, but I wanted to make sure I really understood what was going on and document it here for reference. In any case, I've finally had time to determine why this update was not working on iOS and what changes are required. The issue basically boils down to two simple factors:

1. iOS is not using the GetAlias() method and should be
2. The Alias returned by GetAlias() cannot be bundle specific when an access group is provided

For some basic background -- According to the Xamarin SecureStorage Platform Implementation Specifics for iOS:

KeyChain is used to store values securely on iOS devices. The SecRecord used to store the value has a Service value set to [YOUR-APP-BUNDLE-ID].xamarinessentials.

This service value is populated by the Alias key defined in SecureStorage.shared.cs (line 9):

internal static readonly string Alias = $"{AppInfo.PackageName}.xamarinessentials";

Issue 1: iOS is not using the GetAlias() method and should be

In the iOS implementation, the code directly accesses the internal static Alias rather than using the GetAlias() method. I believe this may have just been an oversight and is easily resolved. More on why iOS should be using this method is covered in Issue 2.

Issue 2: The Alias returned by GetAlias() cannot be bundle specific when an access group is provided

iOS Apps and/or Extensions do not share bundle IDs and the static Alias will be different between the apps. Because this app-specific Alias is being used as the SecRecord Service Value on iOS, the apps will not be able to access each others' stored values even if the access groups are the same.

In my tests, as long as all apps have the same SecRecord Service value and matching access groups/keychain access, SecureStorage works across them as expected.

Solution

I have confirmed that changing the GetAlias() method as follows and replacing all instances of direct access to Alias in the iOS platform specific code with calls to it, solves the issues with access groups and shared storage on iOS:

// Special alias needed to use grouping
internal static string GetAlias(string accessGroup)
    => string.IsNullOrWhiteSpace(accessGroup) ? Alias : accessGroup;

It is also worth noting that during the course of figuring this out, I stumbled upon a few places (lines 176, 234, and 258) in the Android platform that also directly access Alias. I'm not sure if that was intentional, but additional review/work may be required there as well.

Let me know if you'd like me to submit any of the code changes for review somehow. If it's easiest, I can simply create my own pull request using this as a baseline. Lemme know.

[kiddailey]

@jamesmontemagno Is there something I can do to help get this moving again? Don't want to step on any toes or do things incorrectly. Any guidance is appreciated.

I've spent a few weeks testing my changes I mentioned above on iOS and it works perfectly between apps and extensions.

[jamesmontemagno]

Do you want to send a PR down to my branch?

[jfversluis]

Unless you think otherwise James, I don't think this will make it in anymore. Thanks everyone for the time and effort on this one!