Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Remove a thread blocking behaviour #547

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Remove a thread blocking behaviour #547

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5d7bc4d
Make decryption asynchronous
savelichalex Jun 8, 2022
abfffa4
Merge pull request #1 from EverSurf/make-decrypt-async
sertony Jun 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
90 changes: 52 additions & 38 deletions android/src/main/java/com/oblador/keychain/KeychainModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@
import java.util.HashSet;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CompletionException;
import java.util.concurrent.TimeUnit;

import javax.crypto.Cipher;
Expand Down Expand Up @@ -303,23 +305,35 @@ protected void getGenericPassword(@NonNull final String alias,
cipher = getCipherStorageByName(storageName);
}

final DecryptionResult decryptionResult = decryptCredentials(alias, cipher, resultSet, rules, promptInfo);

final WritableMap credentials = Arguments.createMap();
credentials.putString(Maps.SERVICE, alias);
credentials.putString(Maps.USERNAME, decryptionResult.username);
credentials.putString(Maps.PASSWORD, decryptionResult.password);
credentials.putString(Maps.STORAGE, cipher.getCipherStorageName());
CipherStorage finalCipher = cipher;
decryptCredentials(alias, finalCipher, resultSet, rules, promptInfo).handle((decryptionResult, e) -> {
if (e == null) {
final WritableMap credentials = Arguments.createMap();
credentials.putString(Maps.SERVICE, alias);
credentials.putString(Maps.USERNAME, decryptionResult.username);
credentials.putString(Maps.PASSWORD, decryptionResult.password);
credentials.putString(Maps.STORAGE, finalCipher.getCipherStorageName());

promise.resolve(credentials);
return null;
}
if(e instanceof CompletionException)
e = e.getCause();
if (e instanceof KeyStoreAccessException) {
Log.e(KEYCHAIN_MODULE, e.getMessage());

promise.resolve(credentials);
} catch (KeyStoreAccessException e) {
Log.e(KEYCHAIN_MODULE, e.getMessage());
promise.reject(Errors.E_KEYSTORE_ACCESS_ERROR, e);
} else if (e instanceof CryptoFailedException) {
Log.e(KEYCHAIN_MODULE, e.getMessage());

promise.reject(Errors.E_KEYSTORE_ACCESS_ERROR, e);
} catch (CryptoFailedException e) {
Log.e(KEYCHAIN_MODULE, e.getMessage());
promise.reject(Errors.E_CRYPTO_FAILED, e);
} else {
Log.e(KEYCHAIN_MODULE, e.getMessage(), e);

promise.reject(Errors.E_CRYPTO_FAILED, e);
promise.reject(Errors.E_UNKNOWN_ERROR, e);
}
return null;
});
} catch (Throwable fail) {
Log.e(KEYCHAIN_MODULE, fail.getMessage(), fail);

Expand Down Expand Up @@ -513,7 +527,7 @@ private static String getSecurityRulesOrDefault(@Nullable final ReadableMap opti
String rules = null;

if (null != options && options.hasKey(Maps.RULES)) {
rules = options.getString(Maps.RULES);
rules = options.getString(Maps.ACCESS_CONTROL);
}

if (null == rules) return rule;
Expand Down Expand Up @@ -633,7 +647,7 @@ private static PromptInfo getPromptInfo(@Nullable final ReadableMap options) {
* results set then executed migration.
*/
@NonNull
private DecryptionResult decryptCredentials(@NonNull final String alias,
private CompletableFuture<DecryptionResult> decryptCredentials(@NonNull final String alias,
@NonNull final CipherStorage current,
@NonNull final ResultSet resultSet,
@Rules @NonNull final String rules,
Expand All @@ -654,37 +668,37 @@ private DecryptionResult decryptCredentials(@NonNull final String alias,
}

// decrypt using the older cipher storage
final DecryptionResult decryptionResult = decryptToResult(alias, oldStorage, resultSet, promptInfo);

if (Rules.AUTOMATIC_UPGRADE.equals(rules)) {
try {
// encrypt using the current cipher storage
migrateCipherStorage(alias, current, oldStorage, decryptionResult);
} catch (CryptoFailedException e) {
Log.w(KEYCHAIN_MODULE, "Migrating to a less safe storage is not allowed. Keeping the old one");
return decryptToResult(alias, oldStorage, resultSet, promptInfo).thenApply(decryptionResult -> {
if (Rules.AUTOMATIC_UPGRADE.equals(rules)) {
try {
// encrypt using the current cipher storage
migrateCipherStorage(alias, current, oldStorage, decryptionResult);
} catch (CryptoFailedException e) {
Log.w(KEYCHAIN_MODULE, "Migrating to a less safe storage is not allowed. Keeping the old one");
} catch (KeyStoreAccessException e) {
throw new CompletionException(e);
}
}
}

return decryptionResult;
return decryptionResult;
});
}

/** Try to decrypt with provided storage. */
@NonNull
private DecryptionResult decryptToResult(@NonNull final String alias,
@NonNull final CipherStorage storage,
@NonNull final ResultSet resultSet,
@NonNull final PromptInfo promptInfo)
private CompletableFuture<DecryptionResult> decryptToResult(@NonNull final String alias,
@NonNull final CipherStorage storage,
@NonNull final ResultSet resultSet,
@NonNull final PromptInfo promptInfo)
throws CryptoFailedException {
final DecryptionResultHandler handler = getInteractiveHandler(storage, promptInfo);
storage.decrypt(handler, alias, resultSet.username, resultSet.password, SecurityLevel.ANY);

CryptoFailedException.reThrowOnError(handler.getError());

if (null == handler.getResult()) {
throw new CryptoFailedException("No decryption results and no error. Something deeply wrong!");
}
return storage.decrypt(handler, alias, resultSet.username, resultSet.password, SecurityLevel.ANY).thenApply((decryptionResult) -> {
if (null == decryptionResult) {
throw new CompletionException(new CryptoFailedException("No decryption results and no error. Something deeply wrong!"));
}

return handler.getResult();
return decryptionResult;
});
}

/** Get instance of handler that resolves access to the keystore on system request. */
Expand Down
27 changes: 15 additions & 12 deletions android/src/main/java/com/oblador/keychain/cipherStorage/CipherStorage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import java.security.Key;
import java.util.Set;
import java.util.concurrent.CompletableFuture;

@SuppressWarnings({"unused", "WeakerAccess"})
public interface CipherStorage {
Expand Down Expand Up @@ -92,19 +93,21 @@ EncryptionResult encrypt(@NonNull final String alias,
* That can happens during migration from one version of library to another.
*/
@NonNull
DecryptionResult decrypt(@NonNull final String alias,
@NonNull final byte[] username,
@NonNull final byte[] password,
@NonNull final SecurityLevel level)
throws CryptoFailedException;
CompletableFuture<DecryptionResult> decrypt(@NonNull final String alias,
@NonNull final byte[] username,
@NonNull final byte[] password,
@NonNull final SecurityLevel level)
throws CryptoFailedException;

/** Decrypt the credentials but redirect results of operation to handler. */
void decrypt(@NonNull final DecryptionResultHandler handler,
@NonNull final String alias,
@NonNull final byte[] username,
@NonNull final byte[] password,
@NonNull final SecurityLevel level)
throws CryptoFailedException;
/**
* Decrypt the credentials but redirect results of operation to handler.
*/
CompletableFuture<DecryptionResult> decrypt(@NonNull final DecryptionResultHandler handler,
@NonNull final String alias,
@NonNull final byte[] username,
@NonNull final byte[] password,
@NonNull final SecurityLevel level)
throws CryptoFailedException;

/** Remove key (by alias) from storage. */
void removeKey(@NonNull final String alias) throws KeyStoreAccessException;
Expand Down
36 changes: 16 additions & 20 deletions android/src/main/java/com/oblador/keychain/cipherStorage/CipherStorageFacebookConceal.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

import java.security.GeneralSecurityException;
import java.security.Key;
import java.util.concurrent.CompletableFuture;

/**
* @see <a href="https://github.com/facebook/conceal">Conceal Project</a>
Expand Down Expand Up @@ -96,10 +97,10 @@ public EncryptionResult encrypt(@NonNull final String alias,

@NonNull
@Override
public DecryptionResult decrypt(@NonNull final String alias,
@NonNull final byte[] username,
@NonNull final byte[] password,
@NonNull final SecurityLevel level)
public CompletableFuture<DecryptionResult> decrypt(@NonNull final String alias,
@NonNull final byte[] username,
@NonNull final byte[] password,
@NonNull final SecurityLevel level)
throws CryptoFailedException {

throwIfInsufficientLevel(level);
Expand All @@ -112,30 +113,25 @@ public DecryptionResult decrypt(@NonNull final String alias,
final byte[] decryptedUsername = crypto.decrypt(username, usernameEntity);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't these 2 decrypt calls also be part of supplyAsync?

final byte[] decryptedPassword = crypto.decrypt(password, passwordEntity);

return new DecryptionResult(
return CompletableFuture.supplyAsync(() -> new DecryptionResult(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This call requires API level 24. (CompletableFuture was introduced in 24.)
Current minimum is 21. There is no desugaring for this API to support older versions of android, which means bumping min level => bumping min level for all apps using this sdk. Any alternative? Rx and kotlin come to my mind.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think to include CompletableFuture compat version? Like what is suggested here: https://stackoverflow.com/a/38375991 , or maybe I can try to write my own wrapper (because it seems that multithreading that CompletableFuture provide is not that important for the task)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This compat version seems to be not official one. We will have maintenance issues later on.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@savelichalex We tested this patch and observed it fixes the crash issue, writing own wrapper sounds good to me will you be able to update this pr.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ankitsingh08 when I tried to make the wrapper turned out it's not a 20 minutes ride :D I have an idea to make it with callbacks, unfortunately I don't have much time for this right now, but I'll try to allocate some time for it

new String(decryptedUsername, UTF8),
new String(decryptedPassword, UTF8),
SecurityLevel.ANY);
SecurityLevel.ANY));
} catch (Throwable fail) {
throw new CryptoFailedException("Decryption failed for alias: " + alias, fail);
}
}

/** redirect call to default {@link #decrypt(String, byte[], byte[], SecurityLevel)} method. */
/**
* redirect call to default {@link #decrypt(String, byte[], byte[], SecurityLevel)} method.
*/
@Override
public void decrypt(@NonNull DecryptionResultHandler handler,
@NonNull String service,
@NonNull byte[] username,
@NonNull byte[] password,
@NonNull final SecurityLevel level) {

try {
final DecryptionResult results = decrypt(service, username, password, level);

handler.onDecrypt(results, null);
} catch (Throwable fail) {
handler.onDecrypt(null, fail);
}
public CompletableFuture<DecryptionResult> decrypt(@NonNull DecryptionResultHandler handler,
@NonNull String service,
@NonNull byte[] username,
@NonNull byte[] password,
@NonNull final SecurityLevel level) throws CryptoFailedException {
return decrypt(service, username, password, level);
}

@Override
Expand Down
52 changes: 26 additions & 26 deletions android/src/main/java/com/oblador/keychain/cipherStorage/CipherStorageKeystoreAesCbc.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
import java.security.GeneralSecurityException;
import java.security.Key;
import java.security.spec.KeySpec;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CompletionException;
import java.util.concurrent.atomic.AtomicInteger;

import javax.crypto.Cipher;
Expand Down Expand Up @@ -128,46 +130,44 @@ public EncryptionResult encrypt(@NonNull final String alias,

@Override
@NonNull
public DecryptionResult decrypt(@NonNull final String alias,
@NonNull final byte[] username,
@NonNull final byte[] password,
@NonNull final SecurityLevel level)
public CompletableFuture<DecryptionResult> decrypt(@NonNull final String alias,
@NonNull final byte[] username,
@NonNull final byte[] password,
@NonNull final SecurityLevel level)
throws CryptoFailedException {

throwIfInsufficientLevel(level);

final String safeAlias = getDefaultAliasIfEmpty(alias, getDefaultAliasServiceName());
final AtomicInteger retries = new AtomicInteger(1);

try {
final Key key = extractGeneratedKey(safeAlias, level, retries);

return new DecryptionResult(
decryptBytes(key, username),
decryptBytes(key, password),
getSecurityLevel(key));
} catch (GeneralSecurityException e) {
throw new CryptoFailedException("Could not decrypt data with alias: " + alias, e);
} catch (Throwable fail) {
throw new CryptoFailedException("Unknown error with alias: " + alias +
", error: " + fail.getMessage(), fail);
}
return CompletableFuture.supplyAsync(() -> {
try {
return extractGeneratedKey(safeAlias, level, retries);
} catch (GeneralSecurityException e) {
throw new CompletionException(new CryptoFailedException("Could not decrypt data with alias: " + alias, e));
}
}).thenApply(key -> {
try {
return new DecryptionResult(
decryptBytes(key, username),
decryptBytes(key, password),
getSecurityLevel(key));
} catch (Throwable fail) {
throw new CompletionException(new CryptoFailedException("Unknown error with alias: " + alias +
", error: " + fail.getMessage(), fail));
}
});
}

/** Redirect call to {@link #decrypt(String, byte[], byte[], SecurityLevel)} method. */
@Override
public void decrypt(@NonNull final DecryptionResultHandler handler,
public CompletableFuture<DecryptionResult> decrypt(@NonNull final DecryptionResultHandler handler,
@NonNull final String service,
@NonNull final byte[] username,
@NonNull final byte[] password,
@NonNull final SecurityLevel level) {
try {
final DecryptionResult results = decrypt(service, username, password, level);

handler.onDecrypt(results, null);
} catch (Throwable fail) {
handler.onDecrypt(null, fail);
}
@NonNull final SecurityLevel level) throws CryptoFailedException {
return decrypt(service, username, password, level);
}
//endregion

Expand Down