RFR: 8293779: redundant checking in AESCrypt.makeSessionKey() method [v3]

[Daniel Jeliński]

On Wed, 14 Sep 2022 21:04:58 GMT, Sean Mullan <mullan at openjdk.org> wrote:

>>> If the key is null, the following condition could bypass the checking, and result in NPE.
>>> 
>>> ` if (!MessageDigest.isEqual(key, lastKey)) {`
>>> 
>>> Although it is unlikely to happen as the caller should has already been checked that the key cannot be null, but the code logic here is not that clear to read. In the new patch, I have the null checking in the init() method, and the validity checking in the makeSessionKey() method.
>> 
>> I agree, the provider layer should have rejected a null `Key` or a null `Key.getEncoded` prior to this, but best to not change what this code previously did (at least not for this change).
>
>> > Actually, NM, init still has to call MessageDigest.isEqual so eliminating keys of invalid length before that is probably more efficient.
>> 
>> The key should be valid for common cases. For valid key, it is more efficient to have the checking in makeSessionKey() as there is less checking. For invalid key, it is more efficient to have the checking before calling MessageDigest.isEqual(). Exception itself is costly, I would prefer to have better performance for common cases (valid key).
>> 
>> I updated the patch before I read the comment. Please let me know your preference. I'm fine to rollback if you prefer the old patch.
> 
> Yes, I think your current fix should be fine too. No need to rollback.

Speaking of MessageDigest.isEqual, we don't need constant time comparison here. We could use Arrays.equals for some extra performance.

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

PR: https://git.openjdk.org/jdk/pull/10263