-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Android][libraries] Throw PNSE for PersistKeySet flags #57494
[Android][libraries] Throw PNSE for PersistKeySet flags #57494
Conversation
Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks Issue DetailsFixes #52434 Until support is added for Android with regards to Exportable and PersistKeySet flags, throw PNSE on Android.
|
if ((keyStorageFlags & X509KeyStorageFlags.Exportable) == X509KeyStorageFlags.Exportable) | ||
{ | ||
throw new PlatformNotSupportedException(SR.Cryptography_X509_PKCS12_ExportableNotSupported); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that the Android implementation currently makes everything exportable. Can someone confirm? (@elinor-fung)
That makes it somewhat odd that the two mobile platforms would have totally opposite behavior on the supported flags and when it throws. I am not feeling good about that and not sure how to proceed. /cc @bartonjs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If Android imports are already exportable then the flag can just be ignored. (If non-exportability makes sense then it'd be great if "Exportable wasn't set, ensure non-Exportable" worked now, but I understand the calendar is out of pages).
I actually don't know what state the keys on iOS are in, guess I took it as a given that they weren't exportable.
That makes it somewhat odd that the two mobile platforms would have totally opposite behavior on the supported flags and when it throws.
It's unfortunate, but not unheard of. EphemeralKeySet is preferred for transient operations on Windows and Linux, but throws on macOS. Our Linux stack has no notion of non-exportability, so it's keys are exportable even without Exportable being set. Windows "Exportable" keys aren't always (plain-text) exportable. The only true consistency in this area is how inconsistent things are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually don't know what state the keys on iOS are in, guess I took it as a given that they weren't exportable.
That's fair question and even though I wrote it I don't have a definitive answer. Technically there are two limitations:
- No API to export PKCS#12 (or any other certificate format with private key) exists on mobile Apple platforms
- No support for storing private keys in secure enclave on the .NET side
When I started writing the code I simply assumed no export is possible at all due to lack of the API but that's not always true. We reuse the managed PKCS#12 export and hence depend on exporting the raw private keys themselves. That is certainly possible in some circumstances (eg. ephemeral keys) but I never actually verified what happens for persisted keys now. I am not sure if there is some test covering it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If Android imports are already exportable then the flag can just be ignored.
Will remove this block of code to effectively ignore the flag
Fixes #52435
Until support is added for Android with regards to PersistKeySet flags, throw PNSE on Android.