On Mon, 17 May 2021 18:13:46 GMT, Anthony Scarpino <[email protected]>
wrote:
> Hi,
>
> I need a review of this rather large change to GCM. GCM will no longer use
> CipherCore, and AESCrypt to handle it's buffers and other objects. It is
> also a major code redesign limits the amount of data copies and make some
> performance-based decisions.
>
> Thanks
>
> Tony
Here are some comments, will continue looking at the rest of changes.
Thanks,
Valerie
src/java.base/share/classes/com/sun/crypto/provider/AESCipher.java line 155:
> 153: super(32, new AESCrypt());
> 154: }
> 155: }
These should be removed since SunJCE registers
com.sun.crypto.provider.GaloisCounterMode$AES128/AES192/AES256 instead of these?
src/java.base/share/classes/com/sun/crypto/provider/CipherCore.java line 128:
> 126: private static final int CTS_MODE = 6;
> 127:
> 128: private byte[] lastEncKey = null;
lastEncKey is also used only by GCM and should be removed with 'requireReinit'
and 'lastEncIv'
src/java.base/share/classes/com/sun/crypto/provider/CipherCore.java line 495:
> 493: String algorithm = key.getAlgorithm();
> 494: cipher.init(decrypting, algorithm, keyBytes, ivBytes);
> 495: // skip checking key+iv from now on until after doFinal()
Outdated comment, remove?
src/java.base/share/classes/com/sun/crypto/provider/FeedbackCipher.java line
213:
> 211: */
> 212: int getBufferedLength() {
> 213: return 0;
Since you are removing all GCM related things from FeedbackCipher, why not
remove this method as well. Based on the current change, this method is not
called at all, right?
src/java.base/share/classes/com/sun/crypto/provider/SunJCE.java line 238:
> 236:
> 237: // ps("Cipher", "GCM",
> 238: // "com.sun.crypto.provider.GaloisCounterMode$AESGCM", null,
> attrs);
Clean these two lines up?
src/java.base/share/classes/com/sun/crypto/provider/SunJCE.java line 240:
> 238: // "com.sun.crypto.provider.GaloisCounterMode$AESGCM", null,
> attrs);
> 239: ps("Cipher", "AES/GCM/NoPadding",
> 240: "com.sun.crypto.provider.GaloisCounterMode$AESGCM", null,
> attrs);
Why this one uses AESGCM but the rest uses AES128, AES192, AES256? Maybe just
AES?
src/java.base/share/classes/com/sun/crypto/provider/SunJCE.java line 249:
> 247: psA("Cipher", "AES_256/GCM/NoPadding",
> 248: "com.sun.crypto.provider.GaloisCounterMode$AES256",
> 249: attrs);
Can we please follow the same indentation style for consistency sake? This
would also eliminate the unnecessary change such as the one to DESedeWrap and
AESWrap cipher registration below.
src/java.base/share/classes/sun/security/util/SecurityProviderConstants.java
line 221:
> 219:
> 220: store("AES/GCM", KnownOIDs.AES_128$GCM$NoPadding,
> 221: "AES/GCM/NoPadding");
In SunJCE class, the registration is done using "AES/GCM/NoPadding" instead of
"AES/GCM". I am not sure why is this store() call using AES/GCM is necessary?
-------------
PR: https://git.openjdk.java.net/jdk/pull/4072
