Skip to content

Commit

Permalink
Remove old encryption keys from KeyStore & storage when cleaning up o…
Browse files Browse the repository at this point in the history 
…n failure. (#619)

* Remove old encryption keys from KeyStore & storage when cleaning up on failure.
* Update tests, including various tests that were failing to verify what they thought they were verifying.
  • Loading branch information
@paulyoungtt
paulyoungtt authored Dec 5, 2022
1 parent 2148663 commit be00da8
Show file tree
Hide file tree
Showing 2 changed files with 105 additions and 46 deletions.
3 changes: 3 additions & 0 deletions auth0/src/main/java/com/auth0/android/authentication/storage/CryptoUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,7 @@ private void deleteRSAKeys() {
KeyStore keyStore = KeyStore.getInstance(ANDROID_KEY_STORE);
keyStore.load(null);
keyStore.deleteEntry(KEY_ALIAS);
keyStore.deleteEntry(OLD_KEY_ALIAS);
Log.d(TAG, "Deleting the existing RSA key pair from the KeyStore.");
} catch (KeyStoreException | CertificateException | IOException | NoSuchAlgorithmException e) {
Log.e(TAG, "Failed to remove the RSA KeyEntry from the Android KeyStore.", e);
Expand All @@ -252,6 +253,8 @@ private void deleteRSAKeys() {
private void deleteAESKeys() {
storage.remove(KEY_ALIAS);
storage.remove(KEY_IV_ALIAS);
storage.remove(OLD_KEY_ALIAS);
storage.remove(OLD_KEY_IV_ALIAS);
}

/**
Expand Down
148 changes: 102 additions & 46 deletions auth0/src/test/java/com/auth0/android/authentication/storage/CryptoUtilTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -519,7 +519,7 @@ public void shouldUseExistingRSAKeyPairOnAPI27AndDown() throws Exception {
}

@Test
public void shouldThrowOnUnrecoverableEntryExceptionWhenTryingToObtainRSAKeys() {
public void shouldDeleteRSAAndAESKeysAndThrowOnUnrecoverableEntryExceptionWhenTryingToObtainRSAKeys() throws Exception {
Assert.assertThrows("The existing RSA key pair could not be recovered and has been deleted. " +
"This occasionally happens when the Lock Screen settings are changed. You can safely retry this operation.", CryptoException.class, () -> {
KeyStore.PrivateKeyEntry entry = PowerMockito.mock(KeyStore.PrivateKeyEntry.class);
Expand All @@ -530,20 +530,52 @@ public void shouldThrowOnUnrecoverableEntryExceptionWhenTryingToObtainRSAKeys()

cryptoUtil.getRSAKeyEntry();
});

Mockito.verify(keyStore).deleteEntry(KEY_ALIAS);
Mockito.verify(keyStore).deleteEntry(OLD_KEY_ALIAS);
Mockito.verify(storage).remove(KEY_ALIAS);
Mockito.verify(storage).remove(KEY_ALIAS + "_iv");
Mockito.verify(storage).remove(OLD_KEY_ALIAS);
Mockito.verify(storage).remove(OLD_KEY_ALIAS + "_iv");
}

@Test
public void shouldDeleteRSAAndAESKeysAndThrowOnIOExceptionWhenTryingToObtainRSAKeys() {
public void shouldDeleteRSAAndAESKeysAndThrowOnSingleIOExceptionWhenTryingToObtainRSAKeys() throws Exception {
Assert.assertThrows("The existing RSA key pair could not be recovered and has been deleted. " +
"This occasionally happens when the Lock Screen settings are changed. You can safely retry this operation.", CryptoException.class, () -> {
doThrow(new IOException()).when(keyStore).load(nullable(KeyStore.LoadStoreParameter.class));
//In this variant, the first call to keyStore.load() - when we're reading the key - throws an
//exception, but the second one - when we delete the entry on cleanup - does not. Though
//unlikely, this test provides coverage of that scenario.
doThrow(new IOException()).doNothing().when(keyStore).load(nullable(KeyStore.LoadStoreParameter.class));

cryptoUtil.getRSAKeyEntry();
});

Mockito.verify(keyStore).deleteEntry(KEY_ALIAS);
Mockito.verify(keyStore).deleteEntry(OLD_KEY_ALIAS);
Mockito.verify(storage).remove(KEY_ALIAS);
Mockito.verify(storage).remove(KEY_ALIAS + "_iv");
Mockito.verify(storage).remove(OLD_KEY_ALIAS);
Mockito.verify(storage).remove(OLD_KEY_ALIAS + "_iv");
}

@Test
public void shouldDeleteAESKeysAndThrowOnDoubleIOExceptionWhenTryingToObtainRSAKeys() throws Exception {
Assert.assertThrows("The existing RSA key pair could not be recovered and has been deleted. " +
"This occasionally happens when the Lock Screen settings are changed. You can safely retry this operation.", CryptoException.class, () -> {
//In this variant, the first call to keyStore.load() - when we're reading the key - throws an
//exception, and the second one - when we delete the entry on cleanup - does as well.
//This would seem to be the more likely scenario. In this case we don't have any way
//to clean up the RSA key.
doThrow(new IOException()).when(keyStore).load(nullable(KeyStore.LoadStoreParameter.class));

Mockito.verify(keyStore).deleteEntry(KEY_ALIAS);
Mockito.verify(storage).remove(KEY_ALIAS);
Mockito.verify(storage).remove(KEY_ALIAS + "_iv");
cryptoUtil.getRSAKeyEntry();
});

Mockito.verify(storage).remove(KEY_ALIAS);
Mockito.verify(storage).remove(KEY_ALIAS + "_iv");
Mockito.verify(storage).remove(OLD_KEY_ALIAS);
Mockito.verify(storage).remove(OLD_KEY_ALIAS + "_iv");
}

@Test
Expand Down Expand Up @@ -751,7 +783,7 @@ public void shouldThrowOnInvalidKeyExceptionWhenTryingToRSAEncrypt() {
}

@Test
public void shouldDeleteAESKeysAndThrowOnBadPaddingExceptionWhenTryingToRSAEncrypt() {
public void shouldDeleteAESKeysAndThrowOnBadPaddingExceptionWhenTryingToRSAEncrypt() throws Exception {
Assert.assertThrows("The RSA decrypted input is invalid.", CryptoException.class, () -> {

byte[] sampleBytes = new byte[0];
Expand All @@ -764,15 +796,18 @@ public void shouldDeleteAESKeysAndThrowOnBadPaddingExceptionWhenTryingToRSAEncry
PowerMockito.when(rsaCipher.doFinal(sampleBytes)).thenThrow(new BadPaddingException());

cryptoUtil.RSAEncrypt(sampleBytes);

Mockito.verify(keyStore, never()).deleteEntry(KEY_ALIAS);
Mockito.verify(storage).remove(KEY_ALIAS);
Mockito.verify(storage).remove(KEY_ALIAS + "_iv");
});

Mockito.verify(keyStore, never()).deleteEntry(KEY_ALIAS);
Mockito.verify(keyStore, never()).deleteEntry(OLD_KEY_ALIAS);
Mockito.verify(storage).remove(KEY_ALIAS);
Mockito.verify(storage).remove(KEY_ALIAS + "_iv");
Mockito.verify(storage).remove(OLD_KEY_ALIAS);
Mockito.verify(storage).remove(OLD_KEY_ALIAS + "_iv");
}

@Test
public void shouldDeleteAESKeysAndThrowOnIllegalBlockSizeExceptionWhenTryingToRSAEncrypt() {
public void shouldDeleteAESKeysAndThrowOnIllegalBlockSizeExceptionWhenTryingToRSAEncrypt() throws Exception {
Assert.assertThrows("The RSA decrypted input is invalid.", CryptoException.class, () -> {
Certificate certificate = PowerMockito.mock(Certificate.class);
KeyStore.PrivateKeyEntry privateKeyEntry = PowerMockito.mock(KeyStore.PrivateKeyEntry.class);
Expand All @@ -783,11 +818,14 @@ public void shouldDeleteAESKeysAndThrowOnIllegalBlockSizeExceptionWhenTryingToRS
PowerMockito.when(rsaCipher.doFinal(any(byte[].class))).thenThrow(new IllegalBlockSizeException());

cryptoUtil.RSAEncrypt(new byte[0]);

Mockito.verify(keyStore, never()).deleteEntry(KEY_ALIAS);
Mockito.verify(storage).remove(KEY_ALIAS);
Mockito.verify(storage).remove(KEY_ALIAS + "_iv");
});

Mockito.verify(keyStore, never()).deleteEntry(KEY_ALIAS);
Mockito.verify(keyStore, never()).deleteEntry(OLD_KEY_ALIAS);
Mockito.verify(storage).remove(KEY_ALIAS);
Mockito.verify(storage).remove(KEY_ALIAS + "_iv");
Mockito.verify(storage).remove(OLD_KEY_ALIAS);
Mockito.verify(storage).remove(OLD_KEY_ALIAS + "_iv");
}

@Test
Expand Down Expand Up @@ -884,7 +922,7 @@ public void shouldThrowOnNoSuchPaddingExceptionWhenTryingToRSADecrypt() {
}

@Test
public void shouldDeleteAESKeysAndThrowOnBadPaddingExceptionWhenTryingToRSADecrypt() {
public void shouldDeleteAESKeysAndThrowOnBadPaddingExceptionWhenTryingToRSADecrypt() throws Exception {
Assert.assertThrows("The RSA encrypted input is corrupted and cannot be recovered. Please discard it.", CryptoException.class, () -> {
PrivateKey privateKey = PowerMockito.mock(PrivateKey.class);
KeyStore.PrivateKeyEntry privateKeyEntry = PowerMockito.mock(KeyStore.PrivateKeyEntry.class);
Expand All @@ -893,15 +931,18 @@ public void shouldDeleteAESKeysAndThrowOnBadPaddingExceptionWhenTryingToRSADecry

doThrow(new BadPaddingException()).when(rsaCipher).doFinal(any(byte[].class));
cryptoUtil.RSADecrypt(new byte[0]);

Mockito.verify(keyStore, never()).deleteEntry(KEY_ALIAS);
Mockito.verify(storage).remove(KEY_ALIAS);
Mockito.verify(storage).remove(KEY_ALIAS + "_iv");
});

Mockito.verify(keyStore, never()).deleteEntry(KEY_ALIAS);
Mockito.verify(keyStore, never()).deleteEntry(OLD_KEY_ALIAS);
Mockito.verify(storage).remove(KEY_ALIAS);
Mockito.verify(storage).remove(KEY_ALIAS + "_iv");
Mockito.verify(storage).remove(OLD_KEY_ALIAS);
Mockito.verify(storage).remove(OLD_KEY_ALIAS + "_iv");
}

@Test
public void shouldDeleteAESKeysAndThrowOnIllegalBlockSizeExceptionWhenTryingToRSADecrypt() {
public void shouldDeleteAESKeysAndThrowOnIllegalBlockSizeExceptionWhenTryingToRSADecrypt() throws Exception {
Assert.assertThrows("The RSA encrypted input is corrupted and cannot be recovered. Please discard it.", CryptoException.class, () -> {
PrivateKey privateKey = PowerMockito.mock(PrivateKey.class);
KeyStore.PrivateKeyEntry privateKeyEntry = PowerMockito.mock(KeyStore.PrivateKeyEntry.class);
Expand All @@ -910,11 +951,14 @@ public void shouldDeleteAESKeysAndThrowOnIllegalBlockSizeExceptionWhenTryingToRS

doThrow(new IllegalBlockSizeException()).when(rsaCipher).doFinal(any(byte[].class));
cryptoUtil.RSADecrypt(new byte[0]);

Mockito.verify(keyStore, never()).deleteEntry(KEY_ALIAS);
Mockito.verify(storage).remove(KEY_ALIAS);
Mockito.verify(storage).remove(KEY_ALIAS + "_iv");
});

Mockito.verify(keyStore, never()).deleteEntry(KEY_ALIAS);
Mockito.verify(keyStore, never()).deleteEntry(OLD_KEY_ALIAS);
Mockito.verify(storage).remove(KEY_ALIAS);
Mockito.verify(storage).remove(KEY_ALIAS + "_iv");
Mockito.verify(storage).remove(OLD_KEY_ALIAS);
Mockito.verify(storage).remove(OLD_KEY_ALIAS + "_iv");
}

/*
Expand Down Expand Up @@ -1037,33 +1081,39 @@ public void shouldThrowOnInvalidKeyExceptionWhenTryingToAESEncrypt() throws Exce


@Test
public void shouldDeleteAESKeysAndThrowOnBadPaddingExceptionWhenTryingToAESEncrypt() {
public void shouldThrowButNotDeleteAESKeysOnBadPaddingExceptionWhenTryingToAESEncrypt() throws Exception {
Assert.assertThrows("The AES decrypted input is invalid.", CryptoException.class, () -> {
doReturn(new byte[]{11, 22, 33}).when(cryptoUtil).getAESKey();
doThrow(new BadPaddingException()).when(aesCipher)
.doFinal(any(byte[].class));

cryptoUtil.encrypt(new byte[0]);

Mockito.verify(keyStore, never()).deleteEntry(KEY_ALIAS);
Mockito.verify(storage, never()).remove(KEY_ALIAS);
Mockito.verify(storage, never()).remove(KEY_ALIAS + "_iv");
});

Mockito.verify(keyStore, never()).deleteEntry(KEY_ALIAS);
Mockito.verify(keyStore, never()).deleteEntry(OLD_KEY_ALIAS);
Mockito.verify(storage, never()).remove(KEY_ALIAS);
Mockito.verify(storage, never()).remove(KEY_ALIAS + "_iv");
Mockito.verify(storage, never()).remove(OLD_KEY_ALIAS);
Mockito.verify(storage, never()).remove(OLD_KEY_ALIAS + "_iv");
}

@Test
public void shouldDeleteAESKeysAndThrowOnIllegalBlockSizeExceptionWhenTryingToAESEncrypt() {
public void shouldThrowButNotDeleteAESKeysOnIllegalBlockSizeExceptionWhenTryingToAESEncrypt() throws Exception {
Assert.assertThrows("The AES decrypted input is invalid.", CryptoException.class, () -> {
doReturn(new byte[]{11, 22, 33}).when(cryptoUtil).getAESKey();
doThrow(new IllegalBlockSizeException()).when(aesCipher)
.doFinal(any(byte[].class));

cryptoUtil.encrypt(new byte[0]);

Mockito.verify(keyStore, never()).deleteEntry(KEY_ALIAS);
Mockito.verify(storage, never()).remove(KEY_ALIAS);
Mockito.verify(storage, never()).remove(KEY_ALIAS + "_iv");
});

Mockito.verify(keyStore, never()).deleteEntry(KEY_ALIAS);
Mockito.verify(keyStore, never()).deleteEntry(OLD_KEY_ALIAS);
Mockito.verify(storage, never()).remove(KEY_ALIAS);
Mockito.verify(storage, never()).remove(KEY_ALIAS + "_iv");
Mockito.verify(storage, never()).remove(OLD_KEY_ALIAS);
Mockito.verify(storage, never()).remove(OLD_KEY_ALIAS + "_iv");
}


Expand Down Expand Up @@ -1237,7 +1287,7 @@ public void shouldThrowOnInvalidAlgorithmParameterExceptionWhenTryingToAESDecryp
}

@Test
public void shouldDeleteAESKeysAndThrowOnBadPaddingExceptionWhenTryingToAESDecrypt() {
public void shouldThrowButNotDeleteAESKeysOnBadPaddingExceptionWhenTryingToAESDecrypt() throws Exception {
Assert.assertThrows("The AES encrypted input is corrupted and cannot be recovered. Please discard it.", CryptoException.class, () -> {
byte[] aesKeyBytes = new byte[]{11, 22, 33};
byte[] ivBytes = new byte[]{99, 22};
Expand All @@ -1253,15 +1303,18 @@ public void shouldDeleteAESKeysAndThrowOnBadPaddingExceptionWhenTryingToAESDecry
doThrow(new BadPaddingException()).when(aesCipher).doFinal(any(byte[].class));

cryptoUtil.decrypt(new byte[0]);

Mockito.verify(keyStore, never()).deleteEntry(KEY_ALIAS);
Mockito.verify(storage, never()).remove(KEY_ALIAS);
Mockito.verify(storage, never()).remove(KEY_ALIAS + "_iv");
});

Mockito.verify(keyStore, never()).deleteEntry(KEY_ALIAS);
Mockito.verify(keyStore, never()).deleteEntry(OLD_KEY_ALIAS);
Mockito.verify(storage, never()).remove(KEY_ALIAS);
Mockito.verify(storage, never()).remove(KEY_ALIAS + "_iv");
Mockito.verify(storage, never()).remove(OLD_KEY_ALIAS);
Mockito.verify(storage, never()).remove(OLD_KEY_ALIAS + "_iv");
}

@Test
public void shouldDeleteAESKeysAndThrowOnIllegalBlockSizeExceptionWhenTryingToAESDecrypt() {
public void shouldThrowButNotDeleteAESKeysOnIllegalBlockSizeExceptionWhenTryingToAESDecrypt() throws Exception {
Assert.assertThrows("The AES encrypted input is corrupted and cannot be recovered. Please discard it.", CryptoException.class, () -> {
byte[] aesKeyBytes = new byte[]{11, 22, 33};
doReturn(aesKeyBytes).when(cryptoUtil).getAESKey();
Expand All @@ -1276,11 +1329,14 @@ public void shouldDeleteAESKeysAndThrowOnIllegalBlockSizeExceptionWhenTryingToAE
doThrow(new IllegalBlockSizeException()).when(aesCipher).doFinal(any(byte[].class));

cryptoUtil.decrypt(new byte[0]);

Mockito.verify(keyStore, never()).deleteEntry(KEY_ALIAS);
Mockito.verify(storage, never()).remove(KEY_ALIAS);
Mockito.verify(storage, never()).remove(KEY_ALIAS + "_iv");
});

Mockito.verify(keyStore, never()).deleteEntry(KEY_ALIAS);
Mockito.verify(keyStore, never()).deleteEntry(OLD_KEY_ALIAS);
Mockito.verify(storage, never()).remove(KEY_ALIAS);
Mockito.verify(storage, never()).remove(KEY_ALIAS + "_iv");
Mockito.verify(storage, never()).remove(OLD_KEY_ALIAS);
Mockito.verify(storage, never()).remove(OLD_KEY_ALIAS + "_iv");
}


Expand Down

0 comments on commit be00da8

Please sign in to comment.