> On Aug 1, 2018, at 11:55 PM, Seán Coffey <sean.cof...@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.cof...@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.
>>>>
>>
>
