RFR: 8255557: Decouple GCM from CipherCore [v3]

[Valerie Peng]

On Mon, 24 May 2021 16:37:05 GMT, Anthony Scarpino <ascarpino at openjdk.org> wrote:

>> src/java.base/share/classes/com/sun/crypto/provider/GHASH.java line 189:
>> 
>>> 187:             ct.position(ct.position());
>>> 188:             return processed;
>>> 189:         } else if (!ct.isReadOnly()) {
>> 
>> Maybe you meant "ct.hasArray()" instead of "!ct.isReadOnly()"?
>
> hasArray() checks both isReadonly() and isDirect().  Since I already did isDirect() in the previous if, I just need to check readonly here

I see.

>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 259:
>> 
>>> 257:     @Override
>>> 258:     protected void engineInit(int opmode, Key key,
>>> 259:         AlgorithmParameterSpec params, SecureRandom random)
>> 
>> The specified "random" is not saved to internal "random". Perhaps an oversight?
>
> Yeah, I changed this code long ago, i suspect I didn't realize engineGetParameters() used it

It seems that you still have not saved the specified 'random' into the instance 'random' field? Did I miss something?

>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 279:
>> 
>>> 277:             if (iv.length == 0) {
>>> 278:                 throw new InvalidAlgorithmParameterException("IV is empty");
>>> 279:             }
>> 
>> Why separating the validation of iv and tag length in separate methods, i.e. engineInit() vs init()? Consider consolidating them?
>
> I think I had a reason for this where I didn't know what the exact error was.  I'd rather keep them separate so it throws the right message for the error.  It's not a performance critical area.

Ok, it's more for code robustness, not for performance.

-------------

PR: https://git.openjdk.java.net/jdk/pull/4072