Delete keys and stored Credentials on unrecoverable use cases #218

lbalmaceda · 2019-01-15T20:33:38Z

Whenever the Android Keystore stored keys are deemed unrecoverable (e.g. when the Android Lock Screen method or password is changed), instruct to delete the keys in order to regenerate them the next time the method is called; additionally throw a new exception so that the SecureCredentialsManager instance is aware of this scenario and can clear the existing stored encrypted credentials or just fail gracefully.

There's no breaking change. Users can now distinguish an "incompatible device" scenario when a exception is thrown by calling exception.isIncompatibleDevice(). This would mean the device does not support the SecureCredentialsManager class and will need to use some different solution.

Will fix #167

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

lbalmaceda · 2019-01-16T20:02:43Z

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

            deleteKeys();
            Log.e(TAG, "The input contained unexpected content, probably because it was encrypted using a different key. " +
                    "The existing keys have been deleted and a new pair will be created next time. Please try to encrypt the content again.", e);
-            return new byte[]{};


This should be the fix. Instead of returning an "empty key" byte array, throw an exception that the manager (outside of this class) can capture and handle gracefully

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

lbalmaceda · 2019-01-16T20:06:45Z

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

+            if (e instanceof UnrecoverableContentException) {
+                //If keys were invalidated, existing credentials will not be recoverable.
+                clearCredentials();
+            }
            callback.onFailure(new CredentialsManagerException("An error occurred while decrypting the existing credentials.", e));


Like the comment says. In the case of "reading a saved pair of credentials" when the existing keys got corrupted (and so removed), the content signed with those keys would no longer be decryptable (since keys now no longer exist). The operation will fail gracefully via the callback, prior removing any "saved credentials encrypted content" so next time user calls hasValidCredentials it will tell them nothing is saved.

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

cocojoe

In general the principal seems fine, just a query on possible infinite recursion. I do not know enough Java to comment on per line quality. However, the tests seem extensive.

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

lbalmaceda

Please, while reviewing pay attention to the different comments I added explaining at a per-method level which Exceptions can be thrown and when. Almost all of them are never going to be thrown, I'm catching them because compiler would complain otherwise, and I'm treating those cases as "catastrophic" declaring that the device is not compatible with crypto stuff at all. Users can check this with the introduced flag in the CredentialsManagerException class and decide whether to fallback to CredentialsManager (the regular one) or go with their own implementation.

In general, the CryptoUtil methods will bubble up a CryptoException in case the keys were rotated and/or the operation can be retried, or an IncompatibleDeviceException if this catastrophic event happened.

lbalmaceda · 2019-01-22T20:22:15Z

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

+     *
+     * @return whether this device is compatible with {@link SecureCredentialsManager} or not.
+     */
+    public boolean isDeviceIncompatible() {


When users receive a CredentialsManagerException after calling "save" or "get" credentials methods, they can check whether the exception is recoverable or not by checking this isDeviceIncompatible flag.

lbalmaceda · 2019-01-22T20:23:41Z

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

                if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.LOLLIPOP) {
                    //The next call can return null when the LockScreen is not configured
-                    authIntent = kManager.createConfirmDeviceCredentialIntent(null, null);
-                }
-                boolean keyguardEnabled = Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP && kManager.isKeyguardSecure() && authIntent != null;


removed this check Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP since it's already done a few lines above

lbalmaceda · 2019-01-22T20:25:16Z

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

            deleteKeys();
-            return getRSAKeyEntry();
+            throw new CryptoException("The existing RSA key pair could not be recovered and has been deleted. " +


This is the only 2 exceptions scenario where I consider a retry could fix any issue, since I'm deleting any corrupted existing keys if this happens... next time it's called, it will recreate them as explained in the comments.

lbalmaceda · 2019-01-22T20:25:37Z

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

    private void deleteKeys() {
+        storage.remove(KEY_ALIAS);
+        storage.remove(KEY_IV_ALIAS);


moved up to ensure this is called regardless the keystore calls succeed or not.

Separated this into 2 separate calls, since some scenarios required only the AES keys to be recreated

lbalmaceda · 2019-01-22T20:30:09Z

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

        try {
            SecretKey key = new SecretKeySpec(getAESKey(), ALGORITHM_AES);
            Cipher cipher = Cipher.getInstance(AES_TRANSFORMATION);
            String encodedIV = storage.retrieveString(KEY_IV_ALIAS);
            if (TextUtils.isEmpty(encodedIV)) {
-                throw new InvalidAlgorithmParameterException("The AES Key exists but an IV was never stored. Try to encrypt something first.");
+                //AES key was JUST generated. If anything existed before, should be encrypted again first.
+                throw new CryptoException("The encryption keys changed recently. You need to re-encrypt something first.", null);


This is a special scenario where the keys got regenerated but nothing was saved after that.. since we're encrypting with AES in a symmetric manner, the previous stored initialization vector (iv) is no longer related to the now regenerated AES keys. Given that, anything that was already stored cannot be decrypted and should be ignored/removed. The exception message instructs the dev to call "save" credentials first before trying to call "get" again.

cocojoe

Should this be raised up into the README?

README.md

cocojoe · 2019-01-23T14:20:39Z

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

+     *
+     * @return whether this device is compatible with {@link SecureCredentialsManager} or not.
+     */
+    public boolean isDeviceIncompatible() {


Why have a method that checks for the negative outcome? Why not isDeviceCompatible ?

Thought about that but discarded it. The thing is this is already an exception (something that went wrong). The "catastrophic" reason of the error could be a DeviceIncompatibleException, thus the name of the method.

It wouldn't make sense to receive this exception and check "is device compatible" IMO. Maybe we could rename this method (not the exception class) to isDeviceNotCompatible. Thoughts?

cocojoe

In general the principal seems fine, my only concern is I don't have enough platform knowledge to ensure all scenarios are captured. As had I approved this one a week earlier it would have been an issue.

lbalmaceda · 2019-01-28T13:48:54Z

TBH this scenarios are tricky and not easy to catch unless you try different combinations of configuration on an android device. Should be fine now but if something comes up, will open a follow up PR 👌 Thanks for the review @cocojoe

lbalmaceda commented Jan 16, 2019

View reviewed changes

lbalmaceda added this to the v1-Next milestone Jan 16, 2019

lbalmaceda added the CH: Fixed label Jan 16, 2019

cocojoe reviewed Jan 18, 2019

View reviewed changes

auth0/src/main/java/com/auth0/android/authentication/storage/SecureCredentialsManager.java Outdated Show resolved Hide resolved

lbalmaceda force-pushed the crypto-fix branch from 72743fa to 7c6a09b Compare January 22, 2019 20:18

lbalmaceda commented Jan 22, 2019

View reviewed changes

lbalmaceda force-pushed the crypto-fix branch from 606879e to a4665b0 Compare January 22, 2019 20:49

cocojoe requested changes Jan 23, 2019

View reviewed changes

lbalmaceda force-pushed the crypto-fix branch from a4665b0 to cf994d9 Compare January 23, 2019 16:33

lbalmaceda added 6 commits January 24, 2019 15:56

delete keys and stored credentials on unrecoverable use cases

607cb38

fix existing and add new crypto tests

812a9e2

refactor CryptoUtil with better exception handling

bb59b13

refactor tests

c6001da

update README.md and javadoc with exception explanations

d878ec0

separate AES/RSA keys deletion when something goes wrong

af1fbe5

lbalmaceda force-pushed the crypto-fix branch from b72abf6 to af1fbe5 Compare January 24, 2019 18:56

cocojoe approved these changes Jan 28, 2019

View reviewed changes

lbalmaceda merged commit aebfb1b into master Jan 28, 2019

lbalmaceda deleted the crypto-fix branch January 28, 2019 15:29

lbalmaceda modified the milestones: v1-Next, 1.15.1 Jan 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete keys and stored Credentials on unrecoverable use cases #218

Delete keys and stored Credentials on unrecoverable use cases #218

lbalmaceda commented Jan 15, 2019 •

edited

Loading

lbalmaceda Jan 16, 2019

lbalmaceda Jan 16, 2019

cocojoe left a comment

lbalmaceda left a comment

lbalmaceda Jan 22, 2019

lbalmaceda Jan 22, 2019

lbalmaceda Jan 22, 2019

lbalmaceda Jan 22, 2019

lbalmaceda Jan 24, 2019

lbalmaceda Jan 22, 2019

cocojoe left a comment

cocojoe Jan 23, 2019

lbalmaceda Jan 23, 2019

cocojoe left a comment

lbalmaceda commented Jan 28, 2019

Delete keys and stored Credentials on unrecoverable use cases #218

Delete keys and stored Credentials on unrecoverable use cases #218

Conversation

lbalmaceda commented Jan 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cocojoe left a comment

Choose a reason for hiding this comment

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

cocojoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cocojoe left a comment

Choose a reason for hiding this comment

lbalmaceda commented Jan 28, 2019

lbalmaceda commented Jan 15, 2019 •

edited

Loading