-
Notifications
You must be signed in to change notification settings - Fork 14
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
Encrypt credentials in memory #1349
base: master
Are you sure you want to change the base?
Changes from 20 commits
49c6586
09d9900
c95b7c8
676442b
93f33c8
cd6c546
f20986c
7968312
c5d3f94
7b0a54d
a22ff5a
3647d76
c485e5a
b075ac9
eff308b
99e70fd
efdc30a
448f074
0c39c5a
5b66cc4
8feef57
659f47e
f6ac37d
c67c1df
e151f5f
1069a52
87f6a93
aae5ff2
5f561fb
92cef42
fcf749d
276c050
0a32085
0b39e88
03f0ac0
eb4b3ed
409a6f5
5fe6f00
20b2068
4c429d4
dbb1983
0b771c7
db3d264
7c9f89d
fb70f9b
acd23a9
41cb1f2
dd45f8d
19fc7d8
3767176
5b8f281
2f269e8
df1575a
f7ab55b
10352e1
c3b79b9
94b0062
f176952
7a82c35
96f0b04
ffc0c96
58ffb32
b2a8da9
8c7363a
188e22b
a2928bd
bd512d6
4716bda
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,49 +1,112 @@ | ||
package org.commcare.util; | ||
|
||
import java.io.IOException; | ||
import java.nio.ByteBuffer; | ||
import java.nio.charset.Charset; | ||
import java.security.InvalidAlgorithmParameterException; | ||
import java.security.InvalidKeyException; | ||
import java.security.Key; | ||
import java.security.KeyFactory; | ||
import java.security.KeyStore; | ||
import java.security.KeyStoreException; | ||
import java.security.NoSuchAlgorithmException; | ||
import java.security.Security; | ||
import java.security.UnrecoverableEntryException; | ||
import java.security.cert.CertificateException; | ||
import java.security.spec.InvalidKeySpecException; | ||
import java.security.spec.PKCS8EncodedKeySpec; | ||
import java.security.spec.X509EncodedKeySpec; | ||
|
||
import javax.crypto.BadPaddingException; | ||
import javax.crypto.Cipher; | ||
import javax.crypto.IllegalBlockSizeException; | ||
import javax.crypto.NoSuchPaddingException; | ||
import javax.crypto.SecretKey; | ||
import javax.crypto.spec.GCMParameterSpec; | ||
import javax.crypto.spec.IvParameterSpec; | ||
import javax.crypto.spec.SecretKeySpec; | ||
|
||
import static org.commcare.util.CommCarePlatform.getPlatformKeyStoreName; | ||
|
||
public class EncryptionUtils { | ||
|
||
public static final String USER_CREDENTIALS_KEY_ALIAS = "user-credentials-key-alias"; | ||
|
||
public static final String RSA_ALGORITHM_KEY = "RSA"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we already have tests for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes |
||
public static final String AES_ALGORITHM_KEY = "AES"; | ||
|
||
|
||
private static KeyStore platformKeyStore; | ||
|
||
private enum CryptographicOperation {Encryption, Decryption} | ||
|
||
public static KeyStore getPlatformKeyStore() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can be private ? |
||
if (platformKeyStore == null) { | ||
try { | ||
platformKeyStore = KeyStore.getInstance(getPlatformKeyStoreName()); | ||
platformKeyStore.load(null); | ||
} catch (KeyStoreException | IOException | NoSuchAlgorithmException | | ||
CertificateException e) { | ||
throw new RuntimeException(e); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we throw a specific exception that callers can handle for and take appropriate fallbacks. |
||
} | ||
} | ||
return platformKeyStore; | ||
} | ||
|
||
public static String encryptUsingKeyFromKeyStore(String message, String alias) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
same comments for decrypt methods as well |
||
throws EncryptionException { | ||
Key key; | ||
try { | ||
key = retrieveKeyFromKeyStore(alias, CryptographicOperation.Encryption); | ||
} catch (KeyStoreException | UnrecoverableEntryException | NoSuchAlgorithmException e) { | ||
throw new RuntimeException(e); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should bubble up the specific exceptions to caller. |
||
} | ||
return encrypt(key.getAlgorithm(), message, key); | ||
} | ||
|
||
public static String encryptUsingBase64EncodedKey(String algorithm, String message, String key) | ||
throws EncryptionException { | ||
Key secret; | ||
try { | ||
secret = getKey(algorithm, key, CryptographicOperation.Encryption); | ||
} catch (InvalidKeySpecException e) { | ||
throw new EncryptionException("Invalid Key specifications", e); | ||
} | ||
return encrypt(algorithm, message, secret); | ||
} | ||
|
||
/** | ||
* Encrypts a message using the AES encryption and produces a base64 encoded payload containing the ciphertext, and a random IV which was used to encrypt the input. | ||
* Encrypts a message using the AES or RAS algorithms and produces a base64 encoded payload | ||
* containing the ciphertext, and, when applicable, a random IV which was used to encrypt | ||
* the input. | ||
* | ||
* @param message a UTF-8 encoded message to be encrypted | ||
* @param key A base64 encoded 256 bit symmetric key | ||
* @return A base64 encoded payload containing the IV and AES encrypted ciphertext, which can be decoded by this utility's decrypt method and the same symmetric key | ||
* @param algorithm the algotithm to be used to encrypt the data | ||
* @param message a UTF-8 encoded message to be encrypted | ||
* @param key depending on the algorithm, a SecretKey or PublicKey to be used to | ||
* encrypt the message | ||
* @return A base64 encoded payload containing the IV and AES or RSA encrypted ciphertext, | ||
* which can be decoded by this utility's decrypt method and the same symmetric key | ||
*/ | ||
public static String encrypt(String message, String key) throws EncryptionException { | ||
final String ENCRYPT_ALGO = "AES/GCM/NoPadding"; | ||
public static String encrypt(String algorithm, String message, Key key) | ||
throws EncryptionException { | ||
final int MIN_IV_LENGTH_BYTE = 1; | ||
final int MAX_IV_LENGTH_BYTE = 255; | ||
SecretKey secret = getSecretKeySpec(key); | ||
|
||
try { | ||
Cipher cipher = Cipher.getInstance(ENCRYPT_ALGO); | ||
cipher.init(Cipher.ENCRYPT_MODE, secret); | ||
Cipher cipher = Cipher.getInstance(getCryptographicTransformation(algorithm)); | ||
cipher.init(Cipher.ENCRYPT_MODE, key); | ||
byte[] encryptedMessage = cipher.doFinal(message.getBytes(Charset.forName("UTF-8"))); | ||
byte[] iv = cipher.getIV(); | ||
if (iv.length < MIN_IV_LENGTH_BYTE || iv.length > MAX_IV_LENGTH_BYTE) { | ||
int ivSize = (iv == null ? 0 : iv.length); | ||
if (ivSize == 0) { | ||
iv = new byte[0]; | ||
} else if (ivSize < MIN_IV_LENGTH_BYTE || ivSize > MAX_IV_LENGTH_BYTE) { | ||
throw new EncryptionException("Initialization vector should be between " + | ||
MIN_IV_LENGTH_BYTE + " and " + MAX_IV_LENGTH_BYTE + | ||
" bytes long, but it is " + iv.length + " bytes"); | ||
" bytes long, but it is " + ivSize + " bytes"); | ||
} | ||
// The conversion of iv.length to byte takes the low 8 bits. To | ||
// convert back, cast to int and mask with 0xFF. | ||
byte[] ivPlusMessage = ByteBuffer.allocate(1 + iv.length + encryptedMessage.length) | ||
.put((byte)iv.length) | ||
byte[] ivPlusMessage = ByteBuffer.allocate(1 + ivSize + encryptedMessage.length) | ||
.put((byte)ivSize) | ||
.put(iv) | ||
.put(encryptedMessage) | ||
.array(); | ||
|
@@ -53,33 +116,120 @@ public static String encrypt(String message, String key) throws EncryptionExcept | |
} | ||
} | ||
|
||
|
||
private static SecretKey getSecretKeySpec(String key) throws EncryptionException { | ||
final int KEY_LENGTH_BIT = 256; | ||
/** | ||
* Converts a Base64 encoded key into a SecretKey, PrivateKey or PublicKey, depending on the | ||
* algorithm and cryptographic operation | ||
* | ||
* @param algorithm the algorithm to be used to encrypt/decrypt | ||
* @param base64encodedKey key in String format | ||
* @param cryptographicOperation Cryptographic operation where the key is to be used, relevant | ||
* to the RSA algorithm | ||
* @return Secret key, Public key or Private Key to be used | ||
*/ | ||
private static Key getKey(String algorithm, | ||
String base64encodedKey, | ||
CryptographicOperation cryptographicOperation) | ||
throws EncryptionException, InvalidKeySpecException { | ||
byte[] keyBytes; | ||
try { | ||
keyBytes = Base64.decode(key); | ||
keyBytes = Base64.decode(base64encodedKey); | ||
} catch (Base64DecoderException e) { | ||
throw new EncryptionException("Encryption key base 64 encoding is invalid", e); | ||
} | ||
if (8 * keyBytes.length != KEY_LENGTH_BIT) { | ||
throw new EncryptionException("Key should be " + KEY_LENGTH_BIT + | ||
" bits long, not " + 8 * keyBytes.length); | ||
|
||
if (algorithm.equals(AES_ALGORITHM_KEY)) { | ||
final int KEY_LENGTH_BIT = 256; | ||
|
||
if (8 * keyBytes.length != KEY_LENGTH_BIT) { | ||
throw new EncryptionException("Key should be " + KEY_LENGTH_BIT + | ||
" bits long, not " + 8 * keyBytes.length); | ||
} | ||
return new SecretKeySpec(keyBytes, AES_ALGORITHM_KEY); | ||
} else if (algorithm.equals(RSA_ALGORITHM_KEY)) { | ||
// This is not very relevant at the moment as the RSA algorithm is only used to encrypt | ||
// user credentials on devices runnning Android 5.0 - 5.1.1 for the KeyStore | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. unnecessary comment, It's still relevant even if it's getting used on a small set of devices. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not necessarily, we only use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True, you can just say - |
||
KeyFactory keyFactory = null; | ||
try { | ||
keyFactory = KeyFactory.getInstance(RSA_ALGORITHM_KEY); | ||
} catch (NoSuchAlgorithmException e) { | ||
throw new EncryptionException("There is no Provider for the selected algorithm", e); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use |
||
} | ||
|
||
if (cryptographicOperation == CryptographicOperation.Encryption) { | ||
X509EncodedKeySpec keySpec = new X509EncodedKeySpec(keyBytes); | ||
return keyFactory.generatePublic(keySpec); | ||
} else { | ||
PKCS8EncodedKeySpec keySpec = new PKCS8EncodedKeySpec(keyBytes); | ||
return keyFactory.generatePrivate(keySpec); | ||
} | ||
} | ||
// This should cause an error | ||
return null; | ||
} | ||
|
||
private static String getCryptographicTransformation(String algorithm) { | ||
if (algorithm.equals(AES_ALGORITHM_KEY)) { | ||
return "AES/GCM/NoPadding"; | ||
} else if (algorithm.equals(RSA_ALGORITHM_KEY)) { | ||
return "RSA/ECB/PKCS1Padding"; | ||
} else { | ||
// This will cause an error | ||
return null; | ||
} | ||
} | ||
|
||
private static Key retrieveKeyFromKeyStore(String keyAlias, CryptographicOperation operation) | ||
throws KeyStoreException, UnrecoverableEntryException, NoSuchAlgorithmException { | ||
if (getPlatformKeyStore().containsAlias(keyAlias)) { | ||
KeyStore.Entry keyEntry = getPlatformKeyStore().getEntry(keyAlias, null); | ||
if (keyEntry instanceof KeyStore.PrivateKeyEntry) { | ||
if (operation == CryptographicOperation.Encryption) { | ||
return ((KeyStore.PrivateKeyEntry)keyEntry).getCertificate().getPublicKey(); | ||
} else { | ||
return ((KeyStore.PrivateKeyEntry)keyEntry).getPrivateKey(); | ||
} | ||
} else { | ||
return ((KeyStore.SecretKeyEntry)keyEntry).getSecretKey(); | ||
} | ||
} else { | ||
throw new KeyStoreException("Key not found in KeyStore"); | ||
} | ||
return new SecretKeySpec(keyBytes, "AES"); | ||
} | ||
|
||
public static String decryptUsingKeyFromKeyStore(String message, String alias) | ||
throws EncryptionException { | ||
Key key; | ||
try { | ||
key = retrieveKeyFromKeyStore(alias, CryptographicOperation.Decryption); | ||
} catch (KeyStoreException | UnrecoverableEntryException | NoSuchAlgorithmException e) { | ||
throw new RuntimeException(e); | ||
} | ||
return decrypt(key.getAlgorithm(), message, key); | ||
} | ||
|
||
public static String decryptUsingBase64EncodedKey(String algorithm, String message, String key) | ||
throws EncryptionException { | ||
Key secret = null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: |
||
try { | ||
secret = getKey(algorithm, key, CryptographicOperation.Decryption); | ||
} catch (InvalidKeySpecException e) { | ||
throw new EncryptionException("Invalid Key specifications", e); | ||
} | ||
return decrypt(algorithm, message, secret); | ||
} | ||
|
||
/** | ||
* Decrypts a base64 payload containing an IV and AES encrypted ciphertext using the provided key | ||
* Decrypts a base64 payload containing an IV and AES or RSA encrypted ciphertext using the | ||
* provided key | ||
* | ||
* @param message a message to be decrypted | ||
* @param key key that should be used for decryption | ||
* @return Decrypted message for the given AES encrypted message | ||
* @param algorithm the algorithm to be used to decrypt the message | ||
* @param message a message to be decrypted | ||
* @param key dependening on the algorithm, a Secret key or Private key to be used for | ||
* decryption | ||
* @return Decrypted message for the given encrypted message | ||
*/ | ||
public static String decrypt(String message, String key) throws EncryptionException { | ||
final String ENCRYPT_ALGO = "AES/GCM/NoPadding"; | ||
private static String decrypt(String algorithm, String message, Key key) throws EncryptionException { | ||
final int TAG_LENGTH_BIT = 128; | ||
SecretKey secret = getSecretKeySpec(key); | ||
|
||
try { | ||
byte[] messageBytes = Base64.decode(message); | ||
|
@@ -91,17 +241,25 @@ public static String decrypt(String message, String key) throws EncryptionExcept | |
byte[] cipherText = new byte[bb.remaining()]; | ||
bb.get(cipherText); | ||
|
||
|
||
Cipher cipher = Cipher.getInstance(ENCRYPT_ALGO); | ||
cipher.init(Cipher.DECRYPT_MODE, secret, new GCMParameterSpec(TAG_LENGTH_BIT, iv)); | ||
Cipher cipher = Cipher.getInstance(getCryptographicTransformation(algorithm)); | ||
if (algorithm.equals(AES_ALGORITHM_KEY)) { | ||
cipher.init(Cipher.DECRYPT_MODE, key, new GCMParameterSpec(TAG_LENGTH_BIT, iv)); | ||
} else { | ||
cipher.init(Cipher.DECRYPT_MODE, key); | ||
} | ||
byte[] plainText = cipher.doFinal(cipherText); | ||
return new String(plainText, Charset.forName("UTF-8")); | ||
} catch (NoSuchAlgorithmException | BadPaddingException | NoSuchPaddingException | | ||
IllegalBlockSizeException | InvalidAlgorithmParameterException | InvalidKeyException | Base64DecoderException e) { | ||
IllegalBlockSizeException | InvalidKeyException | Base64DecoderException | | ||
InvalidAlgorithmParameterException e) { | ||
throw new EncryptionException("Error encountered while decrypting the message", e); | ||
} | ||
} | ||
|
||
public static boolean isPlatformKeyStoreAvailable() { | ||
return Security.getProvider(getPlatformKeyStoreName()) != null; | ||
} | ||
|
||
public static class EncryptionException extends Exception { | ||
|
||
public EncryptionException(String message) { | ||
|
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.
there is no reason to make a variable for something constant. You can just return
null
from here directly instead.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.
So, this is kind of a hack,
platformKeyStoreName
isstatic
butnot final
, so not a constant. Reason why is that we wantchild cases
to initialize this with the KeyStore name they support. I don't think we can override static methods. What do you think?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.
In this commit is where I set the Android Key Store name.
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.
ahh it should not really be static, thinking again probably the easiest way to handle this is pass this as an argument directly to the encryptionutils methods
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.
@shubham1g5 wouldn't that mean having platform specific terminology on
core
? In this case, the EncryptionUtils method are called from theUser
class, adding as a parameter means that we still need to pull that from some place, right?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.
Hmm I do see your issue. I think it's fine for now. I will see if I can find a better solution here.
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.
How about we make new methods in
user
class asgetUsername(keystoreName)
andsetUsername(keystoreName)
and use those instead if it's not too complicates to implement and change.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.
The problem with that is that some of the calls are made from within
core
, so we would still have the problem of having to pass on the name from there. But also, I find it more elegant to reference the KeyStore of the platform, instead of having to include in each call, it feels redundant.