On Thu, 16 Oct 2025 00:18:08 GMT, Valerie Peng <[email protected]> wrote:
>> Shawn M Emery has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Updates for code review comments from @valeriepeng
>
> src/java.base/share/classes/com/sun/crypto/provider/AES_Crypt.java line 896:
>
>> 894: if (key.length == AESConstants.AES_KEYSIZES[0]) {
>> 895: rounds = AES_128_ROUNDS;
>> 896: nk = AESConstants.AES_KEYSIZES[0]/WB;
>
> Looks like we can get rid of `nk` as the `genRKeys(byte[])` method can
> calculate/derive it based on the specified `key` argument, i.e. `key.length
> >> 2` or `key.length / WB`
Sounds reasonable. Fixed.
> src/java.base/share/classes/com/sun/crypto/provider/AES_Crypt.java line 932:
>
>> 930: * @return w the cipher round keys.
>> 931: */
>> 932: private int[] genRKeys(byte[] key, int nk) {
>
> nit: The spec names this "keyExpansion" and documents it under section 5.2.
> Could we include this in the method javadoc? Also, how about "genRoundKeys"
> or "doKeyExpansion" as method name? `nk` can be derived from `key` and maybe
> no need for extra argument?
Actually I used Stallings' cryptography book specifically for the round key
concepts, hence the variable name mismatch at least for 128 bit keys. But
after your suggestions is does look more like FIPS 192-upd 1 so I will
reference the section ;) Fixed.
> src/java.base/share/classes/com/sun/crypto/provider/AES_Crypt.java line 932:
>
>> 930: * @return w the cipher round keys.
>> 931: */
>> 932: private int[] genRKeys(byte[] key, int nk) {
>
> This method can be static if you pass the `rounds` value as a method argument.
Yes and subWord() would also need to be static for this change. Fixed.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27807#discussion_r2434497086
PR Review Comment: https://git.openjdk.org/jdk/pull/27807#discussion_r2434497821
PR Review Comment: https://git.openjdk.org/jdk/pull/27807#discussion_r2434498001
