``` static SecretKeySpec skeySpec = null; ``` Assigning `null` is never a good idea. Besides that, for fields (static or not) Java will automatically assign `null` anyway. ``` static public byte[] K1 = new byte[16]; // 128 bit sub key ``` ``` static final public byte[] Z16 = new byte[16]; // 128 bit zero ``` Now for the keys you had an excuse, but is this not called `ZERO_VECTOR`? Z16 is not in the specification. ``` public static byte[] getDiversifiedKey() throws IOException, NoSuchAlgorithmException, NoSuchPaddingException, InvalidKeyException, InvalidAlgorithmParameterException, IllegalBlockSizeException, BadPaddingException // make K1 K2 from key ``` A getter should never throw an `IOException`, but as we'll find out, this is not just a getter - the method is badly named at the very least. ``` byte bRb = (byte) 0x87; // Rb for AES128 ``` Completely unclear naming, and seemingly using Hungarian notation, which Java programmers will *never* use. ``` hexStringToByteArray ``` Not sure why you had to program that yourself, but at the naming is spot on at least (let's not be all negative). ``` K1 = cipher.doFinal(Z16); ``` This doesn't make sense, you don't set static fields that way. Just keep the variable local. Assigning the new array to a field just tosses away the previous array reference. ``` System.out.println("KO " + bytesToHex(K1)); ``` Use a logger, or use a field set to `System.out` or something similar, but never print to console from anything other than a UI class. ``` System.out.println("\n error 400 AES " + ex.getMessage()); ``` After catching never print and continue. I default not to `printStackTrace` (like crappy IDE's do) but to > ``` > // TODO better exception > throw new IllegalStateException("Exception not handled", e); > ``` These *runtime* exceptions don't need to be handled and will at least exit the method without running on with invalid state. ``` ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); ``` Consider `ByteBuffer.allocate()` initialized to the correct size instead, then `put` the byte arrays into that, and retrieve the backing array using `array()`. Saves you the IOExceptions. In this case an `IOException` will never happen, so convert it to `IllegalStateException` again. ``` final protected static char[] hexArray = "0123456789ABCDEF".toCharArray(); ``` Always keep to the right order: `protected static final ...`. However, in this case `private static final ...` makes a lot more sense. ``` public static String bytesToHex(byte[] bytes) { ``` Why is this method named rather differently than `hexStringToByteArray`? ``` static byte[] xor16(byte[] ba, byte[] bb) { ``` What's the 16 for? ``` static byte[] shl(byte[] bin) // << 16 byte array ``` Ah, another 16 this time. But in this case it is required. Please document things like the input size in the JavaDoc, and not in an end of line comment. --- General remarks: - If you use names from a specification (which is good) then point out the spec in the documentation of the class - use *clear names* otherwise; - Generally we don't use static anything, especially not for values that will change. None of the static values are necessary (bar the one for the hex decoding, possibly). - If you have static bytes then don't decode them each time, either decode them once and assign them to a `private static final` constant / class field or assign them directly to the class field (using `{ (byte) 0x3B, (byte) 0x17, (byte) 0x35 }`). - Please group the various steps in logical groups and implement them using separate methods.