API Key storage using AES encryption in Java

This class is responsible for storing an API Key and Secret pair. The secret is encrypted with AES in CFB mode, using a key derived from a passphrase, a random salt and a number of rounds of SHA1.

An additional hash of the plaintext and the salt is stored to allow for detecting incorrect passphrase entry.

This is tested and working, but I would love feedback on correctness or improvements, especially areas where the code can be simplified or replaced with built-in functionality I am unaware of.

The key id is a string UUID format, and the secret is provided as a base-64 encoded string.

```
package com.bitgrind.mtgox;

import java.io.FileInputStream;
import java.io.FileNotFoundException;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
import java.security.InvalidKeyException;
import java.security.NoSuchAlgorithmException;
import java.security.SecureRandom;
import java.util.Arrays;
import java.util.Properties;

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

import com.google.common.base.Charsets;
import com.google.common.hash.HashCode;
import com.google.common.hash.HashFunction;
import com.google.common.hash.Hashing;
import com.google.common.io.BaseEncoding;
import com.google.common.io.Closer;
import com.google.common.primitives.Bytes;

public class ApiKey {
private static final String PROP_HASH = "hash";
private static final String PROP_SALT = "salt";
private static final String PROP_SECRET = "secret";
private static final String PROP_KEY = "key";

private static final BaseEncoding BASE64 = BaseEncoding.base64();
private static final BaseEncoding HEX = BaseEncoding.base16().lowerCase();
private static final byte[] IV = new byte[] {
0x10, 0x10, 0x01, 0x04, 0x01, 0x01, 0x01, 0x02,
0x14, 0x1c, 0x11, 0x44, 0x21, 0x4a, 0x22, 0x31
};
private static final i

-

key.write(passphrase, new FileOutputStream(filename));

I see that the stream is flushed in the write method but it will be cleaner to close it too.

(Guideline 1-2: Release resources in all cases in the Secure Coding Guidelines for the Java Programming Language, Version 4.0).

-

return new SecretKeySpec(code.asBytes(), 0, 16, "AES");

I'd create local variables for 0 and 16. What are they supposed to be? Currently they're magic numbers. It would be readable not to force readers to check the implementation or javadoc to figure out their purpose.

The code has them issue with the null parameter here:

p.store(out, null);

-

HashCode code = sha1.hashBytes(Bytes.concat(salt, passphrase.getBytes(Charsets.UTF_8)));
int len = code.writeBytesTo(hash, 0, hash.length);
System.arraycopy(salt, 0, hash, len, salt.length);
for (int i = 0; i < iterations; i++) {
    code = sha1.hashBytes(hash);
    len = code.writeBytesTo(hash, 0, hash.length);
    System.arraycopy(salt, 0, hash, len, salt.length);
}

The code and len variables are used for two different purposes. It would be easier to read if they were separate variables. (Anyway, len is too general, whose length is it? I'd put that information into the name.)

The same is true here for ciphertext:

byte[] ciphertext = BASE64.omitPadding().decode(p.getProperty(PROP_ENC_KEY, "0"));
byte[] salt = Arrays.copyOfRange(ciphertext, 0, SALT_LENGTH);
ciphertext = Arrays.copyOfRange(ciphertext, SALT_LENGTH, ciphertext.length);

I guess the first array should be called saltedCiphertext.

-
The read method does more than one thing (as well as the write):

• handles the Properties object

• checks the checksum

• parse input data

I'd put these functions to separate methods (or classes). Actually, the class is also responsible for more than one thing which violates the single responsibility principle.

-

private final String key;
final String secret;

I guess secret should be also private here.

-

ApiKey(String key, String secret) {
    this.key = key;
    this.secret = secret;
}

It's a good practice to validate input parameters. Does it make sense to create an ApiKey instance with null or empty string key or secret? If not, check it and throw a NullPointerException or IllegalArgumentException than postponing the error which is harder to debug.

If someone creates an ApiKey with null key getKeyBytes() will throw an NPE. The stacktrace of this NPE will contain a stacktrace but it won't be the stacktrace of the real error (the consructor call).

(Effective Java, Second Edition, Item 38: Check parameters for validity; The Pragmatic Programmer: From Journeyman to Master by Andrew Hunt and David Thomas: Dead Programs Tell No Lies.)

-
HmacSHA512 could be a named constant, it is used multiple times.

try {
    mac = Mac.getInstance("HmacSHA512");
} catch (NoSuchAlgorithmException e) {
    throw new AssertionError(e);
}
SecretKeySpec secretKey = new SecretKeySpec(BASE64.decode(secret), "HmacSHA512");

-
I would use longer variable names than p or in. Longer names would make the code more readable since readers don't have to decode the abbreviations every time when they write/maintain the code and don't have to guess which abbreviation the author uses.

(Clean Code by Robert C. Martin, Avoid Mental Mapping, p25)

-

byte[] secret;
secret = Arrays.copyOfRange(plaintext, 0, plaintext.length - 4);
byte[] checksum = Arrays.copyOfRange(plaintext, plaintext.length - 4, plaintext.length);

I'd create a local variable for plaintext.length - 4. I think the following tells more about the intent of code:

int checksumLength = 4;
int secretLength = plaintext.length - checksumLength;
byte[] secret = Arrays.copyOfRange(plaintext, 0, secretLength);
byte[] checksum = Arrays.copyOfRange(plaintext, secretLength, plaintext.length);

It even deserves a separate method:

private static byte[] getVerifiedSecret(final byte[] plaintext) throws ApiKeyException {
    final int checksumLength = 4;
    final int secretLength = plaintext.length - checksumLength;
    final byte[] secret = Arrays.copyOfRange(plaintext, 0, secretLength);
    final byte[] checksum = Arrays.copyOfRange(plaintext, secretLength, plaintext.length);
    if (!Arrays.equals(CRC32.hashBytes(secret).asBytes(), checksum)) {
        throw new ApiKeyException("Wrong passphrase");
    }
    return secret;
}

-
The main method should be in a separate class (because of the single responsibility principle again) and it should catch the exceptions. Instead of printing the full stacktrace (which is hardly useful for a user), print only the message and log (with a logger) the details to a log file.

StackExchange Code Review Q#25851, answer score: 6