RFR: 8208583: Better management of internal KeyStore buffers

Weijun Wang weijun.wang at oracle.com
Thu Aug 2 01:13:14 UTC 2018



> On Aug 1, 2018, at 11:55 PM, Seán Coffey <sean.coffey at oracle.com> wrote:
> 
> Max,
> 
> some updates.
> 
> 1.
> 
> I wasn't able to rename to destroy since that method is reserved for the Destroyable interface. I've gone with destroyKey.

Sorry I wasn't clear but this is exactly what I meant. SecretKey implements Destroyable so you don't need to define sKey as PBEKey. Does it make any problem?

> 
> 3.
> 
> As per offline communication, I've removed the ASCII reference in the s.s.p.KeyProtector constructor comments. I don't believe ASCII requirement was ever enforced (nor needed). We use the 16 bits if presented. If password standards need review, that work should be carried out in another bug report.
> 
> 4.
> 
> I wasn't able to use your suggested method reference syntax. The reference to this would prevent the Object from being cleaned. The PBEKeyCleanupTest testcase seems to confirm that.

Oops.

> 
> http://cr.openjdk.java.net/~coffeys/webrev.8208583.v2/webrev/index.html
> 
> Regards,
> Sean.
> 
> On 01/08/18 09:17, Seán Coffey wrote:
>> Thanks for the comments Max. Comments inline..
>> 
>> 
>> On 01/08/2018 07:59, Weijun Wang wrote:
>>> Some comments:
>>> 
>>> 1. Is it possible to let rename PBEKey::clearKey to PNEKey::destroy?
>> Sure.
>>> 
>>> 2. I am not sure if the newly added "Arrays.fill(thatEncoded, (byte)0x00)" line in PBEKey::equals is safe. What if that key does not return a cloned copy when getEncode() is called? This is possible if the class is only used internally and never escape.
>> This is not new code. I just refactored the java.util.Arrays.fill call to Arrays.fill.
>>> 
>>> 3. You would need to modify the spec of s.s.p.KeyProtector::<init> since password is of a new type. In fact, is this change necessary? Just to prevent the duplication of code in convertToBytes()?
>> Yes, I can add edits to that effect. The class is package private and I only found 2 usages of it. The main advantage is not having to copy or create an extra buffer in this code. The calling code now has full control over when it can null out the bytes in use. That should be a help here and also means we don't place unnecessary load on GC/Cleaner.
>>> 
>>> 4. I found the last line of PBEKey::<init> might be modified to
>>> 
>>>    CleanerFactory.cleaner().register(this, this::clearKey);
>> Sure - I can modify that.
>> 
>> regards,
>> Sean.
>>> 
>>> I have never been a lambda expert so forgive me if this is not correct.
>>> 
>>> Thanks
>>> Max
>>> 
>>>> On Aug 1, 2018, at 3:11 AM, Seán Coffey <sean.coffey at oracle.com> wrote:
>>>> 
>>>> Looking to improve management of internal buffers in KeyStore. The com.sun.crypto.provider.KeyProtector class uses the PBEKey class to protect some keys. Buffers can be freed up earlier if the caller takes responsibility for lifecycle of PBEKey object. (hence no reliance on Cleaner). Some other minor improvements made while visiting this area.
>>>> 
>>>> Other improvements in sun.security.provider.KeyProtector where I believe the password buffer can be managed by the caller.  I only found 2 instances of where this class is used.
>>>> 
>>>> https://bugs.openjdk.java.net/browse/JDK-8208583
>>>> http://cr.openjdk.java.net/~coffeys/webrev.8208583.v1/webrev/index.html
>>>> 
>>>> regards,
>>>> Sean.
>>>> 
>> 
> 




More information about the security-dev mailing list