Code Review Request for 6996769: support AEAD ciphers

Fri Jan 4 02:21:37 UTC 2013

Max,

Forgot to mention that the latest webrev has been updated:
http://cr.openjdk.java.net/~valeriep/6996769/webrev.05/

I shall proceed with putbacks if you have no further comments tomorrow.

Thanks,
Valerie

On 01/03/13 18:17, Valerie (Yu-Ching) Peng wrote:
> Michael,
>
> I thought this some more. I think a cleaner way to do what you wanted 
> may be achieved by adding an AlgorithmParameterGenerator impl for GCM.
> I've read through your email multiple times but yet I can't think of a 
> good way to fit all the functionality that you wanted into the 
> GCMParameterSpec class while maintaining the contract of 
> AlgorithmParameterSpec and AlgorithmParameters. The 
> GCMParameterSpec.generateNextIV() method also seems a bit strange to 
> me since you need a random source and GCMParameterSpec is just a spec 
> and isn't associated with any provider. This also means that the 
> conversion between GCMParameterSpec and GCM AlgorithmParameters object 
> and vice versa, may not return you the same spec. I guess I have a 
> hard time accepting the idea of an AlgorithmParameterSpec object with 
> changing values. This are just my opinion, however.
>
> I will file a separate RFE to keep track of this as I think more 
> discussion are needed.
> JDK8 feature deadline is approaching and we can address this as an RFE 
> separately.
> Regards,
> Valerie
>
> On 12/17/12 10:54, Michael StJohns wrote:
>> Hi Valerie -
>>
>> Comments in line.
>>
>> At 08:32 PM 12/14/2012, Valerie (Yu-Ching) Peng wrote:
>>> Mike,
>>>
>>> Thanks for the comments...
>>> With the current API, it is still possible to have the IV generated 
>>> at the provider level by calling Cipher.init(...) without GCM 
>>> parameters.
>>> This means that provider will generate IVs using provider-default iv 
>>> length as well as provider-default value for tag length.
>>> Of course, things may be a little awkward if you need to use custom 
>>> tag length and a different iv length from what the provider uses as 
>>> default.
>> Right.  And that may be required by specific protocols.  Not saying 
>> it is now, but there needs to be support for this in the API.
>>
>>
>>> SunJCE provider uses 128-bit tag length as well as 96-bit IVs as 
>>> default. If there are demands to use other non-default values, then 
>>> we should see if something can be done to facilitate these scenarios.
>> It's perfectly acceptable for any given implementation to only permit 
>> 96 bit IVs,   But the AEAD algorithms have the possibility of some 
>> really unique initialization variables.
>>
>> Going back to just the 96 bit IV you've got two ways of constructing 
>> the IV - a deterministic and a random (8.2.1 and 8.2.2).  For the 
>> both of these you have a "fixed field" (aka the "free field" which 
>> may be empty in 8.2.2) which is specified by the user. For 8.2.1 you 
>> also have an invocation field which is either a linear counter or an 
>> LFSR based value.  For 8.2.2 you have a random field.
>>
>> You really need an API for the GCMParameterSpec which allows you to 
>> specify either the IV value or the generation parameters.
>>
>>
>>
>>> As for preventing the same IV from being re-used, I think it takes 
>>> more than just adding a new constructor for GCMParameterSpec.
>>
>> Right - but the enforcement has to be in the cipher engine, and only 
>> for the ENCRYPT operation.
>>
>>> Generally, AlgorithmParameterSpec objects can be converted to 
>>> AlgorithmParameters objects and vice versa. If the GCMParameterSpec 
>>> only specifies iv length, would you expect the provider to 
>>> automatically generate a new IV w/o explicit re-init? If yes, then 
>>> what should the Cipher.getParameters() return, the new one or the 
>>> old one?
>> Depends on whether there is a contract of immutability for 
>> AlgorithmParameterSpec's - if no, then you update the state in the 
>> existing object and return that object.  If yes, you create a new 
>> object from the existing object with the generated IV.
>>
>>
>>> Regardless which way we choose, it introduces a window on when 
>>> Cipher.getParameters() should be called.
>>
>> If you pass in the GCMParameterSpec with just the IV format info, 
>> then when you call init, you cause that object to do a generate of 
>> the IV.  The GCMParameterSpec object should indicate whether the IV 
>> was passed or generated.
>>
>> Thinking this through, the GCMParameterSpec needs  "generateNextIV()" 
>> method.  If the GCMParameterSpec is an IV generator rather than an IV 
>> container, then generateNextIV() updates the internal state of the 
>> parameter spec and IV.  Every time you pass this is as an init() 
>> argument, you call generateNextIV() - the spec will either ignore the 
>> call (IV container) or update the IV (IV generator).  (Hmm... 
>> possibly a common interface method for AEAD and CTR mode parameter 
>> spec object).
>>
>> For the pass back and forth between the GCMAlgorithmParameter object 
>> and this, just reflect the state at the time called.
>>
>> I think this works for the ordering specified in the GCM spec.
>>
>> This isn't perfect - what you really want is some state to continue 
>> from Cipher.init to Cipher.init call without reference to an 
>> externally twiddleable object.  Cipher.updateIV which would do an 
>> internal reset and then update the parameter counter appropriately to 
>> get a new IV for the next message?
>>
>> Something similar will probably be needed for CCM as well.
>>
>>
>>> Valerie
>>>
>>> On 12/13/12 20:07, Michael StJohns wrote:
>>>> Sorry for the late comment -
>>>>
>>>> You might want to consider section 9.1, first paragraph of 
>>>> SP800-38D which defines the GCM mode.  Basically, for FIPS 
>>>> validated implementations, to prevent accidental reuse, the IV 
>>>> needs to be generated inside the cryptographic boundary using one 
>>>> of the defined mechanism.   Obviously, that's on the ENCRYPT side 
>>>> since you need to match the IV on the DECRYPT side.
>>>>
>>>> I think you may need to add a GCMParameterSpec that allows you to 
>>>> specify either the IV or the IV length as a subclass of 
>>>> IvParameterSpec.
>>>>
>>>> Mike
>>>>
>>>>
>>>>
>>>>
>>>> At 08:23 PM 12/7/2012, Valerie (Yu-Ching) Peng wrote:
>>>>> Max,
>>>>>
>>>>> The webrev has been updated so that different key + iv values have 
>>>>> to be used for AES/GCM encryption.
>>>>> Latest version at: 
>>>>> http://cr.openjdk.java.net/~valeriep/6996769/webrev.03/
>>>>>
>>>>> Please review and send me comments.
>>>>> Thanks!
>>>>> Valerie
>>>>>
>>>>> On 11/07/12 21:50, Valerie (Yu-Ching) Peng wrote:
>>>>>> Max,
>>>>>>
>>>>>> Update: I removed the block (starting line 580 in 
>>>>>> CipherCore.java) for handling RC2 since it's never used.
>>>>>>
>>>>>> It turns out that the current impl in RC2Cipher always convert 
>>>>>> the AlgorithmParameters object to RC2ParameterSpec and only uses 
>>>>>> CipherCore.init(..., AlgorithmParameterSpec,...) method. Thus, I 
>>>>>> won't be adding a regression test, but rather simply document the 
>>>>>> current RC2Cipher behavior in CipherCore.java to clarify things up.
>>>>>>
>>>>>> The updated webrev is at:
>>>>>> http://cr.openjdk.java.net/~valeriep/6996769/webrev.01/
>>>>>>
>>>>>> Xuelei brought up the issue of enforcing (Key+IV) uniqueness for 
>>>>>> GCM mode during this afternoon's meeting.
>>>>>> I think more changes may be made after we decide what to do.
>>>>>> So, there may be a webrev.02 coming... Just a heads up.
>>>>>>
>>>>>> Thanks!
>>>>>> Valerie
>>>>>>
>>>>>> On 11/07/12 14:48, Valerie (Yu-Ching) Peng wrote:
>>>>>>>> 580                  } else if 
>>>>>>>> (key.getAlgorithm().equals("RC2")) {
>>>>>>>>
>>>>>>>> This seems a bug fix. Is there a regression test for it?
>>>>>>> I just noticed this problem when make the enhancement for GCM mode.
>>>>>>> I will add a regression test for this.
>>
>