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