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

[Sean Mullan]

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.

[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;

[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.

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

[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?

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

[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?

[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.

--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.
>>>>
>>>
>>
>