RFR: 8255557: Decouple GCM from CipherCore

[Valerie Peng]

On Mon, 17 May 2021 18:13:46 GMT, Anthony Scarpino <ascarpino at openjdk.org> 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