Add Android P support for SecureCredentialsManager #203

shriakhilc · 2018-10-28T12:39:51Z

Fixes #187
Please don't merge while WIP. Only writing proper tests is remaining.

lbalmaceda

Have you tested this solution on devices/emulators already? Address the comments and make sure tests are passing and I'll test it here and merge it.

lbalmaceda · 2018-10-29T18:18:34Z

auth0/src/main/java/com/auth0/android/authentication/storage/CryptoUtil.java

@@ -85,7 +86,15 @@ public CryptoUtil(@NonNull Context context, @NonNull Storage storage, @NonNull S
            keyStore.load(null);
            if (keyStore.containsAlias(KEY_ALIAS)) {
                //Return existing key
-                return (KeyStore.PrivateKeyEntry) keyStore.getEntry(KEY_ALIAS, null);
+
+                if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.P) {


move all this block to a new method since you're using the same one a few lines below.

private KeyStore.PrivateKeyEntry getKeyEntryForAlias(KeyStore keyStore, String alias) { }

I suggested that name since the current method this is in is already named similarly.

@lbalmaceda I named the method getKeyEntryCompat since it is a wrapper that is only checking the API level to call the appropriate methods.

I figured out a method that gets all the tests to run, and gives no warnings on the emulator (I don't have an Android P device, but I believe it should have the same results). However, it is a bit hackish, so I wanted to run it by you to see if it's ok.

I added a condition to check if the PrivateKey is null at this line. Since I couldn't mimic an empty PrivateKey item despite looking into the source codes of some of the classes, I am simply falling back and using getEntry as before in such cases.

From what I observed, the PrivateKey object is null at that point only during the tests. On devices, the getRSAKeyEntry method generates the entry if the alias is not present in the keystore.

lbalmaceda · 2018-10-29T18:53:55Z

Looping you in @sebaslogen since you originally reported the issue. Would you mind testing this PR out and giving us feedback? Thanks

shriakhilc · 2018-10-30T17:43:44Z

All the 647 tests in the suite completed successfully when I ran them locally. I'm not sure why Circle CI is showing some failed tests.

sebaslogen · 2018-10-31T21:20:28Z

Thanks for the fix. I compiled the library and manually replaced the Auth0 dependency in my project, the app works correctly and I don't see anymore the previous exception KeyStore exception android.os.ServiceSpecificException: (code 7)
So this has my approval. 👍

lbalmaceda · 2018-10-31T21:39:35Z

Nice! Thanks for the feedback @sebaslogen. I haven't had time to try this locally on my own, but I have been seeing many failed builds for the android repos. I hope I can look into this before eow @TheGamer007 . 🕐

lbalmaceda

A few code style changes and I'll merge it. Looks and works fine 👌

lbalmaceda · 2018-11-28T19:45:53Z

auth0/src/main/java/com/auth0/android/authentication/storage/CryptoUtil.java

+            Certificate certificate = keyStore.getCertificate(KEY_ALIAS);
+
+            if (privateKey == null) {
+                return (KeyStore.PrivateKeyEntry) keyStore.getEntry(KEY_ALIAS, null);


this line will have the same value as privateKey. Seems like this will always return null, am I correct?

When the privateKey is null, the constructor can't be used to create a PrivateKeyEntry object and it throws a NullPointerException. But the method here returns a PrivateKeyEntry object that contains a null privateKey, and an empty certificate chain without throwing any exceptions or warnings.

lbalmaceda · 2018-11-28T19:50:12Z

auth0/src/main/java/com/auth0/android/authentication/storage/CryptoUtil.java

@@ -145,6 +146,22 @@ public CryptoUtil(@NonNull Context context, @NonNull Storage storage, @NonNull S
        }
    }

+    private KeyStore.PrivateKeyEntry getKeyEntryCompat(KeyStore keyStore)
+            throws KeyStoreException, NoSuchAlgorithmException, UnrecoverableEntryException {
+        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.P) {


Please refactor this to look more like the project's code-style. Read the in-line comments I left you:

if (Build.VERSION.SDK_INT < Build.VERSION_CODES.P) { return (KeyStore.PrivateKeyEntry) keyStore.getEntry(KEY_ALIAS, null); } //Following code is for API 28+ PrivateKey privateKey = (PrivateKey) keyStore.getKey(KEY_ALIAS, null); if (privateKey == null) { //FIXME: See my review comment below return (KeyStore.PrivateKeyEntry) keyStore.getEntry(KEY_ALIAS, null); } //I also moved this since it's used only for the "new" call Certificate certificate = keyStore.getCertificate(KEY_ALIAS); return new KeyStore.PrivateKeyEntry(privateKey, new Certificate[]{certificate});

Refactored to match.

lbalmaceda · 2018-11-28T19:51:13Z

auth0/src/main/java/com/auth0/android/authentication/storage/CryptoUtil.java

+        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.P) {
+            //Following code is for API 28+
+            PrivateKey privateKey = (PrivateKey) keyStore.getKey(KEY_ALIAS, null);
+            Certificate certificate = keyStore.getCertificate(KEY_ALIAS);


this is only used for the new KeyStore.PrivateKeyEntry(privateKey, new Certificate[]{certificate}); call, we could avoid calling this if the if-case doesn't require it.

@lbalmaceda Are you referring to the Certificate object? Since the privateKey check was moved before it, now it is only created when needed. Or did you mean I should create it inline as new Certificate[] {keystore.getCertificate(KEY_ALIAS)}?

lbalmaceda · 2018-12-03T19:01:49Z

Hi ~~@sebaslogen~~ @TheGamer007 . Do you need further guidance on resolving the last comments I left? Please, don't hesitate asking. I'm planning a release this week and would be nice if I can include this. Thanks!

shriakhilc · 2018-12-04T03:14:15Z

@lbalmaceda sorry, I wasn't able to check this out yet due to work. I'll get started on this tomorrow and update the PR as soon as possible.

Fixes #187

shriakhilc · 2018-12-14T05:16:56Z

@lbalmaceda I've made the changes you requested, just confused about that last one.

cocojoe · 2018-12-17T21:23:59Z

Thank you @TheGamer007, to set review expectations, lbalmaceda is on vacation.

lbalmaceda · 2018-12-26T19:29:11Z

@TheGamer007 thanks for sticking up with this!! cheers

MHausBermel · 2019-01-09T22:19:33Z

@lbalmaceda Any idea as to when an official release will come with these changes?

lbalmaceda · 2019-01-10T14:04:43Z

@MHausBermel I'll cut a release today with these changes and then start looking into the issues reported (see open issues) around this class.

MHausBermel · 2019-01-10T18:27:50Z

@lbalmaceda Thank you very much!

lbalmaceda suggested changes Oct 29, 2018

View reviewed changes

shriakhilc changed the title ~~WIP: Add Android P support for SecureCredentialsManager~~ Add Android P support for SecureCredentialsManager Nov 3, 2018

lbalmaceda added this to the v1-Next milestone Nov 28, 2018

lbalmaceda suggested changes Nov 28, 2018

View reviewed changes

Add Android P support for SecureCredentialsManager

9de2822

Fixes #187

lbalmaceda added 2 commits December 26, 2018 16:06

fix old test configuration

a901621

fix old test configuration

567483a

lbalmaceda approved these changes Dec 26, 2018

View reviewed changes

lbalmaceda added the CH: Fixed label Dec 26, 2018

lbalmaceda merged commit 3c74ca6 into auth0:master Dec 26, 2018

shriakhilc deleted the fix_secure_credentials_android_p branch January 2, 2019 09:21

lbalmaceda modified the milestones: v1-Next, 1.15.0 Jan 10, 2019

sebaslogen mentioned this pull request Feb 21, 2019

getKeyEntryCompat crashes with RuntimeException header too long #224

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Android P support for SecureCredentialsManager #203

Add Android P support for SecureCredentialsManager #203

shriakhilc commented Oct 28, 2018

lbalmaceda left a comment

lbalmaceda Oct 29, 2018

shriakhilc Oct 30, 2018

lbalmaceda commented Oct 29, 2018

shriakhilc commented Oct 30, 2018 •

edited

sebaslogen commented Oct 31, 2018 •

edited

lbalmaceda commented Oct 31, 2018

lbalmaceda left a comment

lbalmaceda Nov 28, 2018

shriakhilc Dec 14, 2018

lbalmaceda Nov 28, 2018

shriakhilc Dec 14, 2018

lbalmaceda Nov 28, 2018

shriakhilc Dec 14, 2018

lbalmaceda commented Dec 3, 2018 •

edited

shriakhilc commented Dec 4, 2018

shriakhilc commented Dec 14, 2018

cocojoe commented Dec 17, 2018

lbalmaceda commented Dec 26, 2018

MHausBermel commented Jan 9, 2019

lbalmaceda commented Jan 10, 2019

MHausBermel commented Jan 10, 2019

Add Android P support for SecureCredentialsManager #203

Add Android P support for SecureCredentialsManager #203

Conversation

shriakhilc commented Oct 28, 2018

lbalmaceda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbalmaceda commented Oct 29, 2018

shriakhilc commented Oct 30, 2018 • edited

sebaslogen commented Oct 31, 2018 • edited

lbalmaceda commented Oct 31, 2018

lbalmaceda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbalmaceda commented Dec 3, 2018 • edited

shriakhilc commented Dec 4, 2018

shriakhilc commented Dec 14, 2018

cocojoe commented Dec 17, 2018

lbalmaceda commented Dec 26, 2018

MHausBermel commented Jan 9, 2019

lbalmaceda commented Jan 10, 2019

MHausBermel commented Jan 10, 2019

shriakhilc commented Oct 30, 2018 •

edited

sebaslogen commented Oct 31, 2018 •

edited

lbalmaceda commented Dec 3, 2018 •

edited