AES Encryption Service

[David Sawyer]

I'm working on an AES Encryption service. I was wondering in you wouldn't mind taking a look at the design and look for any security holes:

    
import java.security.AlgorithmParameters;
import java.security.SecureRandom;
import java.security.spec.KeySpec;

import javax.crypto.Cipher;
import javax.crypto.SecretKey;
import javax.crypto.SecretKeyFactory;
import javax.crypto.spec.IvParameterSpec;
import javax.crypto.spec.PBEKeySpec;
import javax.crypto.spec.SecretKeySpec;

import org.apache.commons.codec.binary.Base64;
import org.apache.log4j.Logger;

/**
 * Provides AES 256 Bit Encryption
 * Uses a random 8-byte salt and IV
 * 
 * The salt and IV are concatenated into the actual encryption string.
 * Which can then be parsed during decryption.
 *
 */

class AESEncryptionServiceImpl implements AESEncryptionService {
	
	private static final Logger LOGGER = Logger.getLogger(AESEncryptionServiceImpl.class);
	private static final String PASSWORD = "]e@h,]IrMH~.%c5cb#FUf|;HNwSpTH:|";
	private static final String SEPARATOR = "~";
	
	/**
	 * Encrypt a plainText String using AES 256 bit encryption
	 * with an IV and Salt for good measure
	 * 
	 * @param plainTextString
	 * @return encryptedString
	 */
	public String encrypt(String plainTextString) {
		
		String encryptedString = null;
		
		try {
			
			SecureRandom secureRandom = SecureRandom.getInstance("SHA1PRNG", "SUN");
			byte[] salt = new byte[8];
			secureRandom.nextBytes(salt);
			
			SecretKeyFactory factory = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA1");
			KeySpec spec = new PBEKeySpec(PASSWORD.toCharArray(), salt, 1024, 256);
			SecretKey tmp = factory.generateSecret(spec);
			SecretKey secret = new SecretKeySpec(tmp.getEncoded(), "AES");
	
			Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
			cipher.init(Cipher.ENCRYPT_MODE, secret);
			AlgorithmParameters params = cipher.getParameters();
			
			byte[] iv = params.getParameterSpec(IvParameterSpec.class).getIV();
			byte[] ciphertext = cipher.doFinal(plainTextString.getBytes("UTF-8"));
			
			Base64 base64 = new org.apache.commons.codec.binary.Base64();		
			
			encryptedString = new String(base64.encode(iv)) 
					+ SEPARATOR + new String(base64.encode(ciphertext)) 
					+ SEPARATOR + new String(base64.encode(salt));
			
		} catch(Exception ex) {
			LOGGER.error("Error Encrypting data", ex);
		}		
		
		return encryptedString;
	}
	
	/**
	 * Decrypt an encrypted String which was encrypted using AESEncryptionServiceImpl.encrypt(String)
	 * 
	 * @param encryptedString
	 * @return plainTextString
	 */
	public String decrypt(String encryptedString) {	
		
		String plainTextString = null;		
		Base64 base64 = new org.apache.commons.codec.binary.Base64();	
		String[] encryptedStringParts = encryptedString.split(SEPARATOR);
		
		if (encryptedStringParts.length == 3) {
			
			byte[] iv = base64.decode(encryptedStringParts[0].getBytes());
			byte[] encryptedBytes = base64.decode(encryptedStringParts[1].getBytes());
			byte[] salt = base64.decode(encryptedStringParts[2].getBytes());
			
			try {
				
				SecretKeyFactory factory = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA1");
				KeySpec spec = new PBEKeySpec(PASSWORD.toCharArray(), salt, 1024, 256);
				SecretKey tmp = factory.generateSecret(spec);
				SecretKey secret = new SecretKeySpec(tmp.getEncoded(), "AES");
				
				Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
				cipher.init(Cipher.DECRYPT_MODE, secret, new IvParameterSpec(iv));
				plainTextString = new String(cipher.doFinal(encryptedBytes));
			
			} catch(Exception ex) {
				LOGGER.error("Error Encrypting data", ex);
			}
		
		}
		
		return plainTextString;
	}

}

[James Sabre]

1) Why are you using PBE rather than loading the key from some form of key store? It is certainly insecure to hold the password in the source code.
2) Any exception in either encryption or decryption is fatal so you cannot continue. Either the exception must be passed back to the caller or you should wrap the exception in an Error or your own exception and throw that.
3) In the decryption, if the split() does not create three sections then you cannot continue so you should throw an exception. I would follow the same approach as in (2).
4) Since you use utf-8 when converting the cleartext String to bytes you should use utf-8 when converting the decrypted bytes to a String.

[David Sawyer]

I'll definitely be moving the password to a keystore as we need different keys for each environment (dev, staging, production). This was more illustrative of what i was doing in one chunk of code. Mostly I wanted to confirm that there were no serious flaws in the encryption/decryption routine.

I will definitely take your advice and add throws rather than catching those exceptions.

[David Sawyer]

Here's the updated version with the throws suggestion implemented and the character encoding fixed:

    
import java.io.UnsupportedEncodingException;
import java.security.AlgorithmParameters;
import java.security.InvalidAlgorithmParameterException;
import java.security.InvalidKeyException;
import java.security.NoSuchAlgorithmException;
import java.security.NoSuchProviderException;
import java.security.SecureRandom;
import java.security.spec.InvalidKeySpecException;
import java.security.spec.InvalidParameterSpecException;
import java.security.spec.KeySpec;

import javax.crypto.BadPaddingException;
import javax.crypto.Cipher;
import javax.crypto.IllegalBlockSizeException;
import javax.crypto.NoSuchPaddingException;
import javax.crypto.SecretKey;
import javax.crypto.SecretKeyFactory;
import javax.crypto.spec.IvParameterSpec;
import javax.crypto.spec.PBEKeySpec;
import javax.crypto.spec.SecretKeySpec;

import org.apache.commons.codec.binary.Base64;
import org.apache.log4j.Logger;

/**
 * Provides AES 256 Bit Encryption
 * Uses a random 8-byte salt and IV
 * 
 * The salt and IV are concatenated into the actual encryption string.
 * Which can then be parsed during decryption.
 *
 */

class AESEncryptionServiceImpl implements AESEncryptionService {
	
	@SuppressWarnings("unused")
	private static final Logger LOGGER = Logger.getLogger(AESEncryptionServiceImpl.class);
	private static final String ENCODING = "UTF-8";
	private static final String PASSWORD = "]e@h,]IrMH~.%c5cb#FUf|;HNwSpTH:|"; //this will be moved to ks once the service has been peer reviewed
	private static final String SEPARATOR = "~";
	
	/**
	 * Encrypt a plainText String using AES 256 bit encryption
	 * with an IV and Salt for good measure.
	 * 
	 * @param plainTextString
	 * @return encryptedString
	 * @throws NoSuchProviderException 
	 * @throws NoSuchAlgorithmException 
	 * @throws InvalidKeySpecException 
	 * @throws NoSuchPaddingException 
	 * @throws InvalidKeyException 
	 * @throws InvalidParameterSpecException 
	 * @throws UnsupportedEncodingException 
	 * @throws BadPaddingException 
	 * @throws IllegalBlockSizeException 
	 */
        public String encrypt(String plainTextString) throws NoSuchAlgorithmException, NoSuchProviderException,
                                                             InvalidKeySpecException, NoSuchPaddingException,
                                                             InvalidKeyException, InvalidParameterSpecException,
                                                             IllegalBlockSizeException, BadPaddingException,
                                                             UnsupportedEncodingException {
		
		String encryptedString = null;
				
		SecureRandom secureRandom = SecureRandom.getInstance("SHA1PRNG", "SUN");
		byte[] salt = new byte[8];
		secureRandom.nextBytes(salt);
		
		SecretKeyFactory factory = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA1");
		KeySpec spec = new PBEKeySpec(PASSWORD.toCharArray(), salt, 1024, 256);
		SecretKey tmp = factory.generateSecret(spec);
		SecretKey secret = new SecretKeySpec(tmp.getEncoded(), "AES");

		Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
		cipher.init(Cipher.ENCRYPT_MODE, secret);
		AlgorithmParameters params = cipher.getParameters();
		
		byte[] iv = params.getParameterSpec(IvParameterSpec.class).getIV();
		byte[] ciphertext = cipher.doFinal(plainTextString.getBytes(ENCODING));
		
		Base64 base64 = new org.apache.commons.codec.binary.Base64();		
		
		encryptedString = new String(base64.encode(iv)) 
				+ SEPARATOR + new String(base64.encode(ciphertext)) 
				+ SEPARATOR + new String(base64.encode(salt));
		
		return encryptedString;
	}
	
	/**
	 * Decrypt an encrypted String which was encrypted using AESEncryptionServiceImpl.encrypt(String)
	 * 
	 * @param encryptedString
	 * @return plainTextString
	 * @throws NoSuchAlgorithmException 
	 * @throws InvalidKeySpecException 
	 * @throws NoSuchPaddingException 
	 * @throws InvalidAlgorithmParameterException 
	 * @throws InvalidKeyException 
	 * @throws BadPaddingException 
	 * @throws IllegalBlockSizeException 
	 * @throws UnsupportedEncodingException 
	 */
        public String decrypt(String encryptedString) throws NoSuchAlgorithmException, InvalidKeySpecException,
                                                             NoSuchPaddingException, InvalidKeyException,
                                                             InvalidAlgorithmParameterException,
                                                             IllegalBlockSizeException, BadPaddingException,
                                                             UnsupportedEncodingException { 
		
		String plainTextString = null;		
		Base64 base64 = new org.apache.commons.codec.binary.Base64();	
		String[] encryptedStringParts = encryptedString.split(SEPARATOR);
		
		if (encryptedStringParts.length == 3) {
			
			byte[] iv = base64.decode(encryptedStringParts[0].getBytes());
			byte[] encryptedBytes = base64.decode(encryptedStringParts[1].getBytes());
			byte[] salt = base64.decode(encryptedStringParts[2].getBytes());		
				
			SecretKeyFactory factory = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA1");
			KeySpec spec = new PBEKeySpec(PASSWORD.toCharArray(), salt, 1024, 256);
			SecretKey tmp = factory.generateSecret(spec);
			SecretKey secret = new SecretKeySpec(tmp.getEncoded(), "AES");
			
			Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
			cipher.init(Cipher.DECRYPT_MODE, secret, new IvParameterSpec(iv));
			plainTextString = new String(cipher.doFinal(encryptedBytes), ENCODING);
				
		} else {
			throw new RuntimeException("Invalid hash passed to " 
					+ this.getClass().getSimpleName() + ": " + encryptedString);
		}
		
		return plainTextString;
	}

}

[James Sabre]

I still don't like the fact that you are using PBE. If you are going to use some form of key store then it is not needed.

The users of your methods don't care about the exceptions that are thrown. What can they do if they get a NoSuchAlgorithmException or any of the other exceptions you throw? Nothing since they are ALL fatal and would represent a system error. In your position I would catch any exception and wrap it in an exception I derived from RuntimeException and throw that.

[David Sawyer]

I understand that from the perspective of not checking in the key etc. that a keystore is more secure, but is there a technical security issue with PBE?

    
import java.security.AlgorithmParameters;
import java.security.spec.KeySpec;
import java.util.Random;

import javax.crypto.Cipher;
import javax.crypto.SecretKey;
import javax.crypto.SecretKeyFactory;
import javax.crypto.spec.IvParameterSpec;
import javax.crypto.spec.PBEKeySpec;
import javax.crypto.spec.SecretKeySpec;

import org.apache.commons.codec.binary.Base64;
import org.apache.log4j.Logger;

/**
 * Provides AES 256 Bit Encryption
 * Uses a random 8-byte salt and IV
 * 
 * The salt and IV are concatenated into the actual encryption string.
 * Which can then be parsed during decryption.
 *
 */

class AESEncryptionServiceImpl implements AESEncryptionService {
	
	private static final Logger LOGGER = Logger.getLogger(AESEncryptionServiceImpl.class);
	private static final String ENCODING = "UTF-8";
	private static final String PASSWORD = "]e@h,]IrMH~.%c5cb#FUf|;HNwSpTH:|";
	private static final String SEPARATOR = "~";
	
	/**
	 * Encrypt a plainText String using AES 256 bit encryption
	 * with an IV and Salt for good measure.
	 * 
	 * @param plainTextString
	 * @return encryptedString
	 * @throws AESEncryptionException 
	 */
	public String encrypt(String plainTextString) throws AESEncryptionException {
		
		String encryptedString = null;
				
		try {
			Random random = new Random();
			byte[] salt = new byte[8];
			random.nextBytes(salt);
			
			SecretKeyFactory factory = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA1");
			KeySpec spec = new PBEKeySpec(PASSWORD.toCharArray(), salt, 1024, 256);
			SecretKey tmp = factory.generateSecret(spec);
			SecretKey secret = new SecretKeySpec(tmp.getEncoded(), "AES");
	
			Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
			cipher.init(Cipher.ENCRYPT_MODE, secret);
			AlgorithmParameters params = cipher.getParameters();
			
			byte[] iv = params.getParameterSpec(IvParameterSpec.class).getIV();
			byte[] ciphertext = cipher.doFinal(plainTextString.getBytes(ENCODING));
			
			Base64 base64 = new org.apache.commons.codec.binary.Base64();		
			
			encryptedString = new String(base64.encode(iv)) 
					+ SEPARATOR + new String(base64.encode(ciphertext)) 
					+ SEPARATOR + new String(base64.encode(salt));
		
		} catch (Exception ex) {
			throw new AESEncryptionException(ex.getMessage());
		}
		
		return encryptedString;
	}
	
	/**
	 * Decrypt an encrypted String which was encrypted using AESEncryptionServiceImpl.encrypt(String)
	 * 
	 * @param encryptedString
	 * @return plainTextString
	 * @throws AESEncryptionException
	 */
	public String decrypt(String encryptedString) throws AESEncryptionException {	
		
		String plainTextString = null;		
		Base64 base64 = new org.apache.commons.codec.binary.Base64();	
		String[] encryptedStringParts = encryptedString.split(SEPARATOR);
		
		try {
		
			if (encryptedStringParts.length == 3) {
				
				byte[] iv = base64.decode(encryptedStringParts[0].getBytes());
				byte[] encryptedBytes = base64.decode(encryptedStringParts[1].getBytes());
				byte[] salt = base64.decode(encryptedStringParts[2].getBytes());		
					
				SecretKeyFactory factory = SecretKeyFactory.getInstance("PBKDF2WithHmacSHA1");
				KeySpec spec = new PBEKeySpec(PASSWORD.toCharArray(), salt, 1024, 256);
				SecretKey tmp = factory.generateSecret(spec);
				SecretKey secret = new SecretKeySpec(tmp.getEncoded(), "AES");
				
				Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
				cipher.init(Cipher.DECRYPT_MODE, secret, new IvParameterSpec(iv));
				plainTextString = new String(cipher.doFinal(encryptedBytes), ENCODING);
					
			} else {
				throw new RuntimeException("Invalid hash passed to " 
						+ this.getClass().getSimpleName() + ": " + encryptedString);
			}
		
		} catch (Exception ex) {
			LOGGER.error(ex.getMessage(), ex);
			throw new AESEncryptionException(ex.getMessage());
		}
		
		return plainTextString;
	}

}

[James Sabre]

David Sawyer wrote:I understand that from the perspective of not checking in the key etc. that a keystore is more secure, but is there a technical security issue with PBE?

PBE is meant for use where a user provides the password that only he knows. Your code does not allow a user to do do this.

I still don't like your exception handling since you are not actually wrapping the exception; you just wrap the message. My encryption exception is

public class EncryptionException extends Error
{
    public EncryptionException(String text, Exception chain)
    {
        super(text, chain);
    }
    
    public EncryptionException(String text)
    {
        super(text);
    }
}

and I would use it as

         } catch (Exception ex) {  
             throw new EncryptionException("Encryption problem or some such message", ex);  
         }  

so that all the information available in the wrapped exception is retained.

P.S. There is no point in generating a 256 bit key from your password if the entropy of your password is less than 256 bits and almost nobody will be able to remember a 256 bit (32 byte) password or type it in without error. Note, to get an entropy of 256 requires about 42 ASCII printable characters.

[David Sawyer]

James, thank you so much for the time. Here's what I've come up with:

First a script to generate the keystore/key:

    

import java.io.File;
import java.io.FileOutputStream;
import java.io.InputStream;
import java.security.KeyStore;

import javax.crypto.KeyGenerator;
import javax.crypto.SecretKey;

public class Generate256BitKey {
	
	public static void main(String[] args) throws Exception {
		new Generate256BitKey().run();
	}
	
	public void run() {
		File file = new File("256BitAESEncryptionKeyStore");
		this.generateKeyStore(file, "changeit", "strongencryption");
	}
	
	/**
	 * Generate a 256 bit AES Key and write it to the file given
	 * using the alias provided
	 * 
	 * @param file
	 * @param keyStorePassword keyStorePassword
	 * @param keyStoreEntryAlias alias to refer to this keystore entry by
	 */
	public void generateKeyStore(File file, String keyStorePassword, String keyStoreEntryAlias) {
		
		try {

			// Get the KeyGenerator
			KeyGenerator keyGenerator = KeyGenerator.getInstance("AES");
			keyGenerator.init(256);

			// Generate the secret key
			SecretKey secretKey = keyGenerator.generateKey();

			// Get the KeyStore
			KeyStore keyStore = KeyStore.getInstance("JCEKS");

			// Create the KeyStore
			keyStore.load((InputStream) null, keyStorePassword.toCharArray());

			// Put key into store
			keyStore.setKeyEntry(keyStoreEntryAlias, secretKey, keyStorePassword.toCharArray(), null);

			// Write KeyStore to disk
			final FileOutputStream fileOutputStream = new FileOutputStream(file);
			
			try {
				keyStore.store(fileOutputStream, keyStorePassword.toCharArray());
			} finally {
				fileOutputStream.close();
			}

		} catch (Exception ex) {
			ex.printStackTrace();
		}

	}

}

Next the revised AESEncryptionException:

    
public class AESEncryptionException extends Error {

	private static final long serialVersionUID = 6258646710240714329L;
	
	public AESEncryptionException(String message, Exception exception) {
		super(message, exception);
	}
	
	public AESEncryptionException(String message) {
		super(message);
	}

}

The AESEncryptionService Interface:

    
public interface AESEncryptionService {

	/**
	 * Encrypt a plainText String using AES 256 bit encryption
	 * with an IV and Salt for good measure.
	 * 
	 * @param plainTextString
	 * @return encryptedString
	 * @throws AESEncryptionException 
	 */
	public String encrypt(String plainTextString) throws AESEncryptionException;
	
	/**
	 * Decrypt an encrypted String which was encrypted using AESEncryptionServiceImpl.encrypt(String)
	 * 
	 * @param encryptedString
	 * @return plainTextString
	 * @throws AESEncryptionException
	 */
	public String decrypt(String encryptedString) throws AESEncryptionException;
	
}

The AESEncryptionServiceImpl:

    
import java.io.File;
import java.io.FileInputStream;
import java.net.URL;
import java.security.AlgorithmParameters;
import java.security.Key;
import java.security.KeyStore;

import javax.crypto.Cipher;
import javax.crypto.spec.IvParameterSpec;
import javax.crypto.spec.SecretKeySpec;

import org.apache.commons.codec.binary.Base64;
import org.apache.log4j.Logger;

/**
 * Provides AES 256 Bit Encryption
 * 
 * The IV is concatenated into the actual encryption string.
 * Which can then be parsed during decryption.
 *
 */

class AESEncryptionServiceImpl implements AESEncryptionService {
	
	private static final Logger LOGGER = Logger.getLogger(AESEncryptionServiceImpl.class);
	private static final String ENCODING = "UTF-8";
	private static final String SEPARATOR = "~";
	private static final URL KEYSTORE_URL = ClassLoader.getSystemResource("encryption/256BitAESEncryptionKeyStore");
	private static final String KEYSTORE_PASSWORD = "changeit";
	private static final String KEYSTORE_KEY_ALIAS = "strongencryption";
	
	/**
	 * Encrypt a plainText String using AES 256 bit encryption
	 * with an IV and Salt for good measure.
	 * 
	 * @param plainTextString
	 * @return encryptedString
	 * @throws AESEncryptionException 
	 */
	public String encrypt(String plainTextString) throws AESEncryptionException {
		
		String encryptedString = null;
				
		try {
			
			Key key = getKeyFromStore(KEYSTORE_URL, KEYSTORE_PASSWORD, KEYSTORE_KEY_ALIAS);
	
			byte[] raw = key.getEncoded();
			SecretKeySpec secretKeySpec = new SecretKeySpec(raw, "AES");

			Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
			cipher.init(Cipher.ENCRYPT_MODE, secretKeySpec);
			AlgorithmParameters params = cipher.getParameters();
			
			byte[] iv = params.getParameterSpec(IvParameterSpec.class).getIV();
			byte[] ciphertext = cipher.doFinal(plainTextString.getBytes(ENCODING));
			
			Base64 base64 = new org.apache.commons.codec.binary.Base64();		
			
			encryptedString = new String(base64.encode(iv)) 
					+ SEPARATOR + new String(base64.encode(ciphertext));
		
		} catch (Exception ex) {
			LOGGER.debug(ex.getMessage(), ex);
			throw new AESEncryptionException(ex.getMessage(), ex);
		}
		
		return encryptedString;
	}
	
	/**
	 * Decrypt an encrypted String which was encrypted using AESEncryptionServiceImpl.encrypt(String)
	 * 
	 * @param encryptedString
	 * @return plainTextString
	 * @throws AESEncryptionException
	 */
	public String decrypt(String encryptedString) throws AESEncryptionException {	
		
		String plainTextString = null;		
		Base64 base64 = new org.apache.commons.codec.binary.Base64();	
		String[] encryptedStringParts = encryptedString.split(SEPARATOR);
		
		try {
		
			if (encryptedStringParts.length == 2) {
				
				byte[] iv = base64.decode(encryptedStringParts[0].getBytes());
				byte[] encryptedBytes = base64.decode(encryptedStringParts[1].getBytes());	
				
				Key key = getKeyFromStore(KEYSTORE_URL, KEYSTORE_PASSWORD, KEYSTORE_KEY_ALIAS);
				
				byte[] raw = key.getEncoded();
				SecretKeySpec secretKeySpec = new SecretKeySpec(raw, "AES");
				
				Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
				cipher.init(Cipher.DECRYPT_MODE, secretKeySpec, new IvParameterSpec(iv));
				plainTextString = new String(cipher.doFinal(encryptedBytes), ENCODING);
					
			} else {
				throw new AESEncryptionException("Invalid hash passed to " 
						+ this.getClass().getSimpleName() + ": " + encryptedString);
			}
		
		} catch (Exception ex) {
			LOGGER.debug(ex.getMessage(), ex);
			throw new AESEncryptionException(ex.getMessage(), ex);
		}
		
		return plainTextString;
	}
	
	/**
	 * Returns a key from the url provided 
	 * 
	 * @param url 
	 * @param keyStorePassword
	 * @param alias
	 * @return key
	 */
	private static Key getKeyFromStore(URL url, String keyStorePassword, String alias) {

		Key keyFromStore = null;
		File file = new File(url.getFile());

		try {

			final KeyStore keyStore = KeyStore.getInstance("JCEKS");

			if (file.exists()) { 
				final FileInputStream input = new FileInputStream(file);
				keyStore.load(input, keyStorePassword.toCharArray());
				input.close();
			}

			keyFromStore = keyStore.getKey(alias, keyStorePassword.toCharArray());

		} catch (Exception ex) {[code]
			LOGGER.error(ex);
		}

		return keyFromStore;
	}

}

And Unit Test to make sure it all works:

import junit.framework.Assert;

import org.apache.log4j.Logger;
import org.junit.Test;

public class AesEncryptionServiceTest {
	
	private static final Logger LOGGER = Logger.getLogger(AesEncryptionServiceTest.class);
	
	@Test
	public void testEncryptDecrypt() {
		
		String testString = "I like to swim in the pond";
		String encryptedString = null;
		String decryptedString = null;
		
		try {
			encryptedString = AESEncryptionServiceFactory.getInstance().encrypt(testString);
			LOGGER.info(encryptedString);
			decryptedString = AESEncryptionServiceFactory.getInstance().decrypt(encryptedString); 
			LOGGER.info(decryptedString);
		} catch (AESEncryptionException ex) {
			LOGGER.error(ex.getMessage(), ex);
		}
		
		Assert.assertEquals(testString, decryptedString);
	}
	
	@Test
	public void testTwoEncryptionRoutinesYieldDifferentResults() {
		
		String testString = "I like to swim in the pond";
		String encryptedString = null;
		String encryptedString2 = null;
		
		try {
			encryptedString = AESEncryptionServiceFactory.getInstance().encrypt(testString);
			LOGGER.info(encryptedString);
			encryptedString2 = AESEncryptionServiceFactory.getInstance().encrypt(testString);
			LOGGER.info(encryptedString2);
		} catch (AESEncryptionException ex) {
			LOGGER.error(ex.getMessage(), ex);
		}
		
		Assert.assertNotSame(encryptedString, encryptedString2);		
	}

}

[James Sabre]

Much much better David. The only significant change I would make is that I would pass the key (as a SecretKey object) into the AESEncryptionServiceImpl when it was constructed so that the encryption methods were isolated from the key storage method (separation of concerns). In your code there are some 'tweeks' I would make

1) I would make the getKeyFromStore() method return a SecretKey (separation of concerns again) then the code

  byte[] raw = key.getEncoded();  
                 SecretKeySpec secretKeySpec = new SecretKeySpec(raw, "AES");  
                

would not be needed. When you generate the key store you specified the key as being AES so you would not need to specify or convert it to AES when you read it from the key store.
2) In your decryption method I would either catch the AESEncryptionException before the Exception and then just re-throw it since there is no point in re-wrapping the wrapped exception OR I would put the exception handling code only round the actual decryption code and not to include the code that is involved in splitting the IV from the ciphertext.
3) You don't handle the exceptions in generateKeyStore() and getKeyFromStore() in the same way as you do elsewhere.

I like the fact that you have test code.

[David Sawyer]

Nice. I like that much better. Here's the current setup:

Interface:

    
public interface AESEncryptionService {

	/**
	 * Encrypt a plainText String using AES 256 bit encryption
	 * with an IV and Salt for good measure.
	 * 
	 * @param plainTextString
	 * @return encryptedString
	 * @throws AESEncryptionException 
	 */
	public String encrypt(String plainTextString) throws AESEncryptionException;
	
	/**
	 * Decrypt an encrypted String which was encrypted using AESEncryptionServiceImpl.encrypt(String)
	 * 
	 * @param encryptedString
	 * @return plainTextString
	 * @throws AESEncryptionException
	 */
	public String decrypt(String encryptedString) throws AESEncryptionException;
	
}

Impl:

import java.security.AlgorithmParameters;

import javax.crypto.Cipher;
import javax.crypto.SecretKey;
import javax.crypto.spec.IvParameterSpec;

import org.apache.commons.codec.binary.Base64;
import org.apache.log4j.Logger;

/**
 * Provides AES 256 Bit Encryption
 * 
 * The IV is concatenated into the actual encryption string
 * which can then be parsed during decryption.
 *
 */

class AESEncryptionServiceImpl implements AESEncryptionService {
	
	private static final Logger LOGGER = Logger.getLogger(AESEncryptionServiceImpl.class);
	private static final String ENCODING = "UTF-8";
	private static final String SEPARATOR = "~";
	private SecretKey secretKey;
	
	AESEncryptionServiceImpl(SecretKey key) {
		this.secretKey = key;
	}
	
	/**
	 * Encrypt a plainText String using AES 256 bit encryption
	 * with an IV and Salt for good measure.
	 * 
	 * @param plainTextString
	 * @return encryptedString
	 * @throws AESEncryptionException 
	 */
	public String encrypt(String plainTextString) throws AESEncryptionException {
		
		String encryptedString = null;
				
		try {

			Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
			cipher.init(Cipher.ENCRYPT_MODE, this.secretKey);
			AlgorithmParameters params = cipher.getParameters();
			
			byte[] iv = params.getParameterSpec(IvParameterSpec.class).getIV();
			byte[] ciphertext = cipher.doFinal(plainTextString.getBytes(ENCODING));
			
			Base64 base64 = new org.apache.commons.codec.binary.Base64();		
			
			encryptedString = new String(base64.encode(iv)) 
					+ SEPARATOR + new String(base64.encode(ciphertext));
		
		} catch (Exception ex) {
			LOGGER.debug(ex.getMessage(), ex);
			throw new AESEncryptionException(ex.getMessage(), ex);
		}
		
		return encryptedString;
	}
	
	/**
	 * Decrypt an encrypted String which was encrypted using AESEncryptionServiceImpl.encrypt(String)
	 * 
	 * @param encryptedString
	 * @return plainTextString
	 * @throws AESEncryptionException
	 */
	public String decrypt(String encryptedString) throws AESEncryptionException {	
		
		String plainTextString = null;		
		Base64 base64 = new org.apache.commons.codec.binary.Base64();	
		String[] encryptedStringParts = encryptedString.split(SEPARATOR);
		
		if (encryptedStringParts.length == 2) {
			
			try {
				
				byte[] iv = base64.decode(encryptedStringParts[0].getBytes());
				byte[] encryptedBytes = base64.decode(encryptedStringParts[1].getBytes());
				
				Cipher cipher = Cipher.getInstance("AES/CBC/PKCS5Padding");
				cipher.init(Cipher.DECRYPT_MODE, this.secretKey, new IvParameterSpec(iv));
				plainTextString = new String(cipher.doFinal(encryptedBytes), ENCODING);
				
			} catch (Exception ex) {
				LOGGER.debug(ex.getMessage(), ex);
				throw new AESEncryptionException(ex.getMessage(), ex);
			}
				
		} else {
			throw new AESEncryptionException("Invalid hash passed to " 
					+ this.getClass().getSimpleName() + ": " + encryptedString);
		}
		
		return plainTextString;
	}

}

Factory:

import java.io.File;
import java.io.FileInputStream;
import java.net.URL;
import java.security.KeyStore;

import javax.crypto.SecretKey;

import org.apache.log4j.Logger;

public class AESEncryptionServiceFactory {
	
	private static final Logger LOGGER = Logger.getLogger(AESEncryptionServiceFactory.class);
	private static final URL KEYSTORE_URL = ClassLoader.getSystemResource("encryption/256BitAESEncryptionKeyStore");
	private static final String KEYSTORE_PASSWORD = "changeit";
	private static final String KEYSTORE_KEY_ALIAS = "strongencryption";
	private volatile static AESEncryptionService aesEncryptionService;
	
	private AESEncryptionServiceFactory() { }
	
	/**
	 * @return aesEncryptionService
	 */
	public static AESEncryptionService getInstance() {
		if (aesEncryptionService == null) {
			synchronized(AESEncryptionService.class) {			
				if (aesEncryptionService == null) {
					SecretKey secretKey = getKeyFromStore(KEYSTORE_URL, KEYSTORE_PASSWORD, KEYSTORE_KEY_ALIAS);
					aesEncryptionService = new AESEncryptionServiceImpl(secretKey);
				}		
			}
		}
		
		return aesEncryptionService;
	}	
	
	/**
	 * Returns a key from the url provided 
	 * 
	 * @param url 
	 * @param keyStorePassword
	 * @param alias
	 * @return key
	 */
	private static SecretKey getKeyFromStore(URL url, String keyStorePassword, String alias) {

		SecretKey keyFromStore = null;
		File file = new File(url.getFile());

		try {

			final KeyStore keyStore = KeyStore.getInstance("JCEKS");

			if (file.exists()) { 
				final FileInputStream input = new FileInputStream(file);
				keyStore.load(input, keyStorePassword.toCharArray());
				input.close();
			}

			keyFromStore = (SecretKey) keyStore.getKey(alias, keyStorePassword.toCharArray());

		} catch (Exception ex) {
			LOGGER.error(ex);
			throw new AESEncryptionException(ex.getMessage(), ex);
		}

		return keyFromStore;
	}

}

Script to generate keystore/key:

import java.io.File;
import java.io.FileOutputStream;
import java.io.InputStream;
import java.security.KeyStore;

import javax.crypto.KeyGenerator;
import javax.crypto.SecretKey;

import org.apache.log4j.Logger;

import com.partnerweekly.cmg.encryption.AESEncryptionException;

public class Generate256BitKey {
	
	private static final Logger LOGGER = Logger.getLogger(Generate256BitKey.class);
	
	public static void main(String[] args) throws Exception {
		new Generate256BitKey().run();
	}
	
	public void run() {
		File file = new File("256BitAESEncryptionKeyStore");
		this.generateKeyStore(file, "changeit", "strongencryption");
	}
	
	/**
	 * Generate a 256 bit AES Key and write it to the file given
	 * using the alias provided
	 * 
	 * @param file
	 * @param keyStorePassword keyStorePassword
	 * @param keyStoreEntryAlias alias to refer to this keystore entry by
	 */
	public void generateKeyStore(File file, String keyStorePassword, String keyStoreEntryAlias) {
		
		try {

			// Get the KeyGenerator
			KeyGenerator keyGenerator = KeyGenerator.getInstance("AES");
			keyGenerator.init(256);

			// Generate the secret key
			SecretKey secretKey = keyGenerator.generateKey();

			// Get the KeyStore
			KeyStore keyStore = KeyStore.getInstance("JCEKS");

			// Create the KeyStore
			keyStore.load((InputStream) null, keyStorePassword.toCharArray());

			// Put key into store
			keyStore.setKeyEntry(keyStoreEntryAlias, secretKey, keyStorePassword.toCharArray(), null);

			// Write KeyStore to disk
			final FileOutputStream fileOutputStream = new FileOutputStream(file);
			
			try {
				keyStore.store(fileOutputStream, keyStorePassword.toCharArray());
			} finally {
				fileOutputStream.close();
			}

		} catch (Exception ex) {
			LOGGER.error(ex);
			throw new AESEncryptionException(ex.getMessage(), ex);
		}

	}

}

Unit Test:

import junit.framework.Assert;

import org.apache.log4j.Logger;
import org.junit.Test;

public class AesEncryptionServiceTest {
	
	private static final Logger LOGGER = Logger.getLogger(AesEncryptionServiceTest.class);
	
	@Test
	public void testEncryptDecrypt() {
		
		String testString = "I like to swim in the pond";
		String encryptedString = null;
		String decryptedString = null;
		
		try {
			encryptedString = AESEncryptionServiceFactory.getInstance().encrypt(testString);
			LOGGER.info(encryptedString);
			decryptedString = AESEncryptionServiceFactory.getInstance().decrypt(encryptedString); 
			LOGGER.info(decryptedString);
		} catch (AESEncryptionException ex) {
			LOGGER.error(ex.getMessage(), ex);
		}
		
		Assert.assertEquals(testString, decryptedString);
	}
	
	@Test
	public void testTwoEncryptionRoutinesYieldDifferentResults() {
		
		String testString = "I like to swim in the pond";
		String encryptedString = null;
		String encryptedString2 = null;
		
		try {
			encryptedString = AESEncryptionServiceFactory.getInstance().encrypt(testString);
			LOGGER.info(encryptedString);
			encryptedString2 = AESEncryptionServiceFactory.getInstance().encrypt(testString);
			LOGGER.info(encryptedString2);
		} catch (AESEncryptionException ex) {
			LOGGER.error(ex.getMessage(), ex);
		}
		
		Assert.assertNotSame(encryptedString, encryptedString2);		
	}

}

[Rob Spoor]

David, I cut up the throws-clauses of your encrypt and decrypt messages in your second post a bit so that the general layout of this thread isn't so messed up anymore. Can you watch it a bit next time? Thanks.

[David Sawyer]

Rob Prime wrote:Can you watch it a bit next time? Thanks.

Definitely. Will do.

[David Sawyer]

Did anyone else have any security concerns with this final implementation or does this seem secure to everyone?