[8] Code review request for 8005408: KeyStore API enhancements

[Vincent Ryan]

Thanks for those comments Sean.

On 22 Jan 2013, at 21:22, Sean Mullan wrote:

> Final set of comments in this webrev:
> 
> PKCS12KeyStore.java
> 
> [214] Minor comment, but I think it would be cleaner to create separate SecretKeyEntry and PrivateKeyEntry classes.
> 

I did consider that originally; it's worth doing.


> [224, 235-238, 519, 533-536]
> 
> For the attributes, I think it would be cleaner (and faster) to pass Collections.emptySet() instead of null, and then replace lines 235-238 and 533-536 with:
> 
> entry.attributes = attributes;

Done.


> 
> [518] You should destroy the PasswordProtection object before returning to the caller (probably with a try-finally) as they don't have a way to clear the password that has been cloned by the PasswordProtection ctor.
> 

Done.


> [634-678] Why doesn't this method also support SecretKeys like the other engineSetKeyEntry method?
> 

I cannot determine whether the encoded key is a SecretKey or a PrivateKey so
only PrivateKey is supported.


> [801-806] You should probably add a TODO comment that this should be first checking the security property, and if not set then falling back to "PBEWithSHA1AndDESede". Shouldn't we fallback to something stronger (SHA256/AES?) though?

I've added code to check the security property


> 
> [910] what if this is a SecretKey? Should you still decrement this?
> 

Corrected.


> [1484-1522] Why don't you just write out the bytes from the PKCS12Attribute.getEncoded() method rather than parsing and converting the Strings for the OID and value?
> 

Right. This code had not been updated to use PKCS12Attribute.


> [2216 -] It seems it would be better to create the PKCS12Attributes with the byte[] ctor, as that retains the full encoding information, and you could return arbitrary attributes we don't recognize. Also, the String ctor will cause it to be re-encoded again, when we already have the encoding.
> 

Same as above.


> --Sean
> 
> On 01/22/2013 01:22 PM, Sean Mullan wrote:
>> More comments ...
>> 
>> PKCS12Attribute.java:
>> 
>> [213-215] You can replace this with Arrays.hashCode(byte[]).
>> 
>> --Sean
>> 
>> On 01/22/2013 11:50 AM, Sean Mullan wrote:
>>> More comments:
>>> 
>>> KeyStore.java
>>> 
>>> [551, 670, 753] Needs to make a copy (clone) according to javadoc, so
>>> should be:
>>> 
>>> this.attributes = Collections.unmodifiableSet(new HashSet<>(attributes));
>>> 
>>> --Sean
>>> 
>>> On 01/22/2013 11:24 AM, Sean Mullan wrote:
>>>> Comments so far, will send more as I review more:
>>>> 
>>>> AlgorithmId.java
>>>> 
>>>> * Update copyright
>>>> 
>>>> KeyStore.java
>>>> 
>>>> [296] I think you want to say:
>>>> 
>>>> If none was set then null is returned.
>>>> 
>>>> As I understand it, if none is set, then the KeyStore provider will use
>>>> a default algorithm as specified by the Security property. This needs to
>>>> be made clearer in the javadoc, as it reads it says it returns the value
>>>> of this property, which is not possible since this class doesn't know
>>>> what keystore type is being used at this point.
>>>> 
>>>> [304] specify that null can be returned -
>>>> 
>>>> @return the algorithm name, or null if none was set
>>>> 
>>>> --Sean
>>>> 
>>>> 
>>>> On 01/21/2013 07:18 PM, Vincent Ryan wrote:
>>>>> 
>>>>> Updated webrev to include java.security.PKCS12Attribute:
>>>>>   http://cr.openjdk.java.net/~vinnie/8005408/webrev.01/
>>>>> 
>>>>> 
>>>>> 
>>>>> On 21/01/2013 15:18, Vincent Ryan wrote:
>>>>>> Hello,
>>>>>> 
>>>>>> Please review the fix for 8005408. It adds support for associating
>>>>>> attributes with keystore entries.
>>>>>> It is yet another component of the JEP-166 delivery.
>>>>>> 
>>>>>> This new API permits several enhancements to the PKCS12 keystore
>>>>>> implementation: the storage of
>>>>>> trusted certificates, storage of secret keys and support for entry
>>>>>> metadata. Currently, only the
>>>>>> PKCS12 keystore takes advantage of these new KeyStore APIs.
>>>>>> 
>>>>>> Webrev: http://cr.openjdk.java.net/~vinnie/8005408/webrev.00/
>>>>>> 
>>>>>> 
>>>>>> For storing trusted certificates in PKCS12 a new SafeBag attribute
>>>>>> (with
>>>>>> a familiar syntax) is introduced
>>>>>> to indicate a trust usage:
>>>>>> 
>>>>>> |trustedKeyUsage ATTRIBUTE ::= {|
>>>>>> |||WITH SYNTAX ExtKeyUsageSyntax|
>>>>>> |||ID id-at-trustedKeyUsage  -- object identifier from an Oracle arc|
>>>>>> |}|
>>>>>> |-- from RFC ||5832||, Section ||4.2||.||1.12|
>>>>>> |||ExtKeyUsageSyntax ::= SEQUENCE SIZE (||1||..MAX) OF KeyPurposeId|
>>>>>> |||KeyPurposeId ::= OBJECT IDENTIFIER|
>>>>>> |||anyExtendedKeyUsage OBJECT IDENTIFIER ::= { id-ce-extKeyUsage ||0|
>>>>>> |}|
>>>>>> 
>>>>>> Note that this approach does not preclude the storage of a Trust
>>>>>> Anchor
>>>>>> List (as defined in RFC 5914)
>>>>>> which was proposed earlier on this list.
>>>>>> 
>>>>>> 
>>>>>> There is one omission from the webrev above: the
>>>>>> java.security.PKCS12Attribute class needs some
>>>>>> additional changes and will be posted shortly.
>>>>>> 
>>>>>> Again, JEP-166 is on a tight schedule for M6 so your early comments
>>>>>> are
>>>>>> appreciated.
>>>>>> 
>>>>>> Thanks.
>>>>> 
>>>> 
>>> 
>> 
>