``` 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`, handle the exception internally or name the method differently. ``` 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. ``` 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. Methods like these should be `private` (without any access specifier, methods and fields are visible from within the package). --- 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.