RFR: 8255557: Decouple GCM from CipherCore [v5]

[Valerie Peng]

On Fri, 21 May 2021 21:18:34 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
>
> Anthony Scarpino has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Review comments update

Here are comments for the test changes.
Thanks,
Valerie

test/jdk/com/sun/crypto/provider/Cipher/AEAD/Encrypt.java line 236:

> 234:             HexFormat hex = HexFormat.of().withUpperCase();
> 235:             if (!Arrays.equals(output, outputTexts.get(k))) {
> 236:                 System.out.println("Combination #" + k + 1 + "\nresult    " +

nit: Why "+1"? The number should match the exception message, otherwise, very confusing.

test/jdk/com/sun/crypto/provider/Cipher/AEAD/GCMBufferTest.java line 673:

> 671: 
> 672:         // Test update-update-doFinal with byte arrays and preset data sizes
> 673:         t = new GCMBufferTest("AES/GCM/NoPadding",

This change seems un-necessary? Why separating the declaration to line 631?

test/jdk/com/sun/crypto/provider/Cipher/AEAD/GCMShortBuffer.java line 34:

> 32: /*
> 33:  * @test
> 34:  * @summary Call decrypt doFinal() with different output values to see if the

Missing bug id?

test/jdk/com/sun/crypto/provider/Cipher/AEAD/GCMShortBuffer.java line 56:

> 54:             throw e;
> 55:         }
> 56:         c.doFinal(cipherText, 1, len, pt, 0);

Should check the return value of doFinal() to match the expected output size?

test/jdk/com/sun/crypto/provider/Cipher/AEAD/GCMShortBuffer.java line 78:

> 76:             throw e;
> 77:         }
> 78:         c.doFinal(ByteBuffer.wrap(cipherText, 1, len), out);

Should check the return value of doFinal() to match the expected output size?

test/jdk/com/sun/crypto/provider/Cipher/AES/TestAESCipher.java line 90:

> 88:             i++;
> 89:         }
> 90:         return b;

The generated data seems too similar, all starts with 0 and increment. Maybe use i = len to start with? Or take some additional argument for starting value?

test/jdk/com/sun/crypto/provider/Cipher/AES/TestAESCipher.java line 98:

> 96: 
> 97:         byte[] iv = makeData(16);
> 98:         AlgorithmParameterSpec aps = new GCMParameterSpec(128, new byte[16]);

Use the already generated iv instead of new byte[16]?

test/jdk/com/sun/crypto/provider/Cipher/AES/TestAESCipher.java line 117:

> 115:             //int offset = ci.update(plainText, 0, plainText.length, cipherText, 0);
> 116:             //ci.doFinal(cipherText, offset);
> 117:             ci.doFinal(plainText, 0, plainText.length, cipherText, 0);

This change seems un-necessary? I think it's better to not change it so multi-part enc/dec are tested.

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

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