RFR: 8208583: Better management of internal KeyStore buffers

[Seán Coffey]

Thanks for your review Weijun. I'll update with your comments [1] and 
push shortly.

regards,
Sean.

[1]

com/sun/crypto/provider/KeyProtector.java:

(reverted to as was)

     187:         KeyFactory kFac = KeyFactory.getInstance(oidName);
                      return kFac.generatePrivate(new 
PKCS8EncodedKeySpec(plain));


======

JavaKeyStore.java:

@@ -133,18 +133,20 @@
              throw new UnrecoverableKeyException("Password must not be 
null");
          }

-        KeyProtector keyProtector = new KeyProtector(password);
+        byte[] passwordBytes = convertToBytes(password);
+        KeyProtector keyProtector = new KeyProtector(passwordBytes);

Regards,
Sean.

On 03/08/18 10:05, Weijun Wang wrote:
> com/sun/crypto/provider/KeyProtector.java:
>
>   187             Key k = kFac.generatePrivate(new PKCS8EncodedKeySpec(plain));
>   188             return k;
>
>   No need to create a local variable "k" anymore.
>
> JavaKeyStore.java:
>
>   128         byte[] passwordBytes = null;
>
>   No need to define so early and assigned null.
>
> No other comments.
>
>> On Aug 3, 2018, at 12:37 AM, Seán Coffey <sean.coffey at oracle.com> wrote:
>>
>> All valid comments Max. Changes made. Webrev at
>>
>> http://cr.openjdk.java.net/~coffeys/webrev.8208583.v4/webrev/index.html
>>
>>> I wonder why DestroyedFailedException was a checked exception, what can we do if it's thrown?
>> I guess we could log a message, but given the limited usage case here, I don't see a need.
> I was not clear, I meant I cannot find a proper action to do in this case, thus it might be better to defined as an unchecked exception.
>
> Thanks
> Max
>
>> regards,
>> Sean.
>>
>>
>> On 02/08/2018 17:05, Weijun Wang wrote:
>>> KeyProtector.java:
>>>
>>>   113         pbeKeySpec.clearPassword();
>>>
>>>   You can also put this into the finally block.
>>>
>>>   189             Arrays.fill(plain, (byte) 0x00);
>>>
>>>   Can this be in finally?
>>>
>>> JavaKeyStore.java:
>>>
>>>   149         Arrays.fill(passwordBytes, (byte) 0x00);
>>>
>>>   In other cases, you call it in a finally block. Unnecessary here?
>>>
>>>   (Oops, every comment is about finally.)
>>>
>>> KeyProtector.java:
>>>
>>>   123     public KeyProtector(byte[] password)
>>>
>>>   How about just "public KeyProtector(byte[] passwordBytes)"?
>>>
>>>> On Aug 2, 2018, at 7:38 PM, Seán Coffey <sean.coffey at oracle.com> wrote:
>>>>
>>>> No - no problem at all. Some extra exception handling but probably best for the long run.
>>> I wonder why DestroyedFailedException was a checked exception, what can we do if it's thrown?
>>>
>>> Thanks
>>> Max
>>>
>>>> http://cr.openjdk.java.net/~coffeys/webrev.8208583.v3/webrev/index.html
>>>>
>>>> regards,
>>>> Sean.
>>>>
>>>> On 02/08/2018 02:13, Weijun Wang wrote:
>>>>>> 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?
>>>>>
>>>>>