xamarin / Essentials

Xamarin.Essentials is no longer supported. Migrate your apps to .NET MAUI, which includes Maui.Essentials.
https://aka.ms/xamarin-upgrade
Other
1.52k stars 505 forks source link

Specify iOS Keychain Access Group for SecureStorage #712

Open tedrog36 opened 5 years ago

tedrog36 commented 5 years ago

For iOS, I need the ability to specify the keychain access group for SecureStorage so that the information stored there can be accessed by the main host app as well as app extensions. This is primary reason I am not using SecureStorage.

This could be implemented similar to Preferences/Xam.Settings where a shareName can be specified.

jamesmontemagno commented 5 years ago

This would be a good addition. I think that on Android we could pass this in and store it in specific preferences and pass it down. We can have a default which most people will use and then this as an optional parameter. Thanks for the feature request.

tedrog36 commented 5 years ago

No problem. Let me know if you need anything for the implementation since I have already done this in my own KeyChain class.

jamesmontemagno commented 5 years ago

If you want to send down a PR that would be cool ;)

sasivishnu commented 5 years ago

If the issue is still open, can I give a try to add this enhancement?

How do we like to pass down the value of KeyChain Access Group value to be used in SecRecord? Should we add constructor that takes this value?

tedrog36 commented 5 years ago

I haven't looked at the Keychain object in SecureStorage in a while but I can tell you that is how I handled it in my Keychain classs.

public Keychain(string serviceName, string accessGroup)
{
    _serviceName = serviceName;
    _accessGroup = accessGroup;
}
Johan-dutoit commented 5 years ago

I created a local implementation last night.

I didn't go with the accessGroup in the constructor approach, as I figured someone might want to change it/not use it between calls. i.e. something keychain items should be sharable and others shouldn't.

That being said, my knowledge is somewhat limited to how the keychain works, as I remember reading somewhere that it if you don't specify an accessGroup it picks the first one from the list (not sure how accurate this is).

jamesmontemagno commented 5 years ago

PRs are accepted :)

Johan-dutoit commented 5 years ago

@jamesmontemagno I was about to submit a PR, however it's for iOS only and would put the other platforms in an unsupported state. Any ideas on how that should be implemented?

Android we could pass this in and store it in specific preferences and pass it down

When you said we can pass it in and store it in a specific preference, do you mean prefixing the key with the accessGroup value?

jamesmontemagno commented 4 years ago

I think that they are just ignored on Android/UWP, we can just document it.

kenny128 commented 4 years ago

I might be misunderstanding what this issue is about but wasn't this feature supported in Xamarin.Auth?

Andrec-Dxs commented 4 years ago

Is this issue still pending to be solved?

GMCristiano commented 4 years ago

any news @jamesmontemagno ?

mshuf commented 4 years ago

+1

I'm already invested in using Xamarin Essential's SecureStorage in my project and have just hit the snag of needing to share keychain data with an iOS extension that is bundled with the app.

@jamesmontemagno @Johan-dutoit - Is there any progress updates with this feature request? Contemplating whether to wait for this or pivot to another solution, which I'd rather not do since the library has been great for my needs thus far!

Thanks guys.

UPDATE: I ended up forking the repo and made the required changes for my project. I'll try tidy this up and do a pull request for the benefit of everyone else. Unless one of you guys have already done the work and wish to officially submit the PR? I'll leave this open for a little bit and check back in a week or so.

kiddailey commented 4 years ago

I ended up forking the repo and made the required changes for my project. I'll try tidy this up and do a pull request for the benefit of everyone else. Unless one of you guys have already done the work and wish to officially submit the PR? I'll leave this open for a little bit and check back in a week or so.

@mshuf : Would love to know when you commit to your fork (even if you don't get to the pull request). I'm trying to get an extension to share SecureStorage with it's parent app and ran into this snag as well. Would be nice to test it out and see if your solution works with my use case.

jamesmontemagno commented 4 years ago

Would probably look something like this -> https://github.com/xamarin/Essentials/pull/1351

kiddailey commented 4 years ago

Awesome jamesmontemagno, thanks!

As noted in my post on #1351 , this doesn't seem to work. I'm testing with a container app and extension and neither can access values stored by the other with the same access group applied.

Andrec-Dxs commented 4 years ago

Any progress on this issue @kiddailey @jamesmontemagno

kiddailey commented 3 years ago

Sorry for the slow response. I unfortunately haven't gotten around to looking at this more, but I hope to in the coming week or so. In a nutshell, it doesn't seem to work as-is and there is an outstanding question (posted on #1351) about how to treat this feature in the case of Android.

The app/extension I am developing requires this functionality, so I'm planning to either figure out how to make Essentials work or develop my own implementation. Will post when I can.

Andrec-Dxs commented 3 years ago

Ok ok. @kiddailey I don't know if this might help but I don't think you'll have an issue while doing this for Android because in iOS in order to access a app extension and have the stored information from the main application you need to have the apps on the same app group. On android this doesn't exist so I don't believe that will be an issue. I found this project on Android that tries to handle the secure storage for Android devices. Take a look if that might help you....

https://github.com/adorsys/secure-storage-android/issues

Andrec-Dxs commented 3 years ago

@kiddailey any news?

kiddailey commented 3 years ago

@kiddailey any news?

Yes, the apps have to be part of the same app group on iOS. That's the part that doesn't appear to be working for whatever reason. Not sure if it's user error on my part or something else.

I'm going to spend some time today/tomorrow getting everything set up again (because new versions). Will let you know if I discover anything.

Edit: Finally got Essentials compiling/building/packaging on my Mac and starting to get back into the swing. Having an issue getting the Extension project in my test app to accept the new package, but will get there eventually. More to come.

Edit: Got the extension project working with my Essentials build but having same results in that the extension always gets no value back. Slowly working my way through things to see if I can find out anything new...

kiddailey commented 3 years ago

Had my first successful test! I implemented jamesmontemagno's changes into Essentials, packaged and created a new test application consisting of a container app and a single action extension (apparently, I made some changes that fixed the issues, see below).

After (much) playing around with all the entitlements, it works. I was able to set a key in the container app and access it both in the container AND the extension!

I need to figure out the specifics about the entitlement requirements, because I had to setup keychain access and app groups despite what the documentation seems to say and the app prefix isn't required.

Next step -- figure out those specifics and then try this in a much more sophisticated app and see if the results are the same. Will post when I have that complete. <crossing fingers>

Edit: I should note that this was in the iPhone simulator. I haven't confirmed it's the same on an actual device, but will definitely test that as well.

Edit: In the iPhone simulator, I updated a far more complicated app with a different extension type to use my custom Essentials build and went through the same motions of making sure entitlements are correct, and I have success in the iPhone simulator! Will update again when I test on actual device.

Edit: Actual device working as well! I also realize that I may have made some minor updates to jamesmontemango's original changes, so I need to review those to make sure I didn't accidentally fix things myself :D

Edit: Yes, I made two key changes that I'm researching to determine which, if any, is the reason it is working now

kiddailey commented 3 years ago

I've figured out the issue for why this wasn't working across apps and/or extensions on iOS and gave a detailed explanation and the required code changes on @jamesmontemagno 's pull request referenced above: https://github.com/xamarin/Essentials/pull/1351#issuecomment-748457139

gsgou commented 3 years ago

@kiddailey any update on this PR?

kiddailey commented 3 years ago

I submitted a pull request a few days ago to @jamesmontemagno 's original pull request that fixes the issues with iOS (I haven't yet looked at Android). It works well and I've been testing it for some time now with a container app and extension without any issues. Hopefully he'll have some free time to review and incorporate it, but I'm not sure when/if it'll make it into the mainline.

mnxamdev commented 3 years ago

Oh I could so use this right now! Any word on it being released soon? Thanks for all your hard work!

kiddailey commented 3 years ago

@jamesmontemagno Is there any status update on the possibility of your xamarin:secure-storage-group branch being merged into main?

I've been using it with the changes you merged back in February in development and testing on iOS for a number of months now without issue. It works great, but I'm trying to decide if I'm going to wait for Essentials to have this merged into main, or if I should just roll my solution.

If there's something I can do to help, let me know. Thanks!

yevgeniy-seleznev commented 2 years ago

Hi there, just wanted to leave a +1 on this! Had to implement this in my own layer recently in order to be able to share keychain items between main app and action extension. Would be really great if we could extend Xamarin Essentials to support this scenario. Thank you!

mnxamdev commented 2 years ago

Any word on this pull request?

Giorgi commented 2 years ago

Is this ever going to be merged?

mikequ-taggysoft commented 2 years ago

We're running into this very issue, too. Would be great if this issue can receive some love and attention @jamesmontemagno !

Dids commented 2 years ago

This is a very essential feature (pun intended), so getting this merged would be highly appreciated!

idenardi commented 2 years ago

Any news on this issue/PR?

kiddailey commented 2 years ago

Has anyone tried extracting the (up-to-date) secure storage from Essentials into a standalone package? I really would like to stick with this code base and have been maintaining my own version of Essentials with my pull request integrated, but it's annoying. I think I'm going to have to do it myself if it hasn't already been done.