RFR 8058778: New APIs for creating certificates and certificate requests

[Sean Mullan]

This is looking really nice. Here are some comments on the API portion, 
will review the impl later ...

* Certificate

315      * can provider more builder methods.

s/provider/provide/

329          * Builds a certificate by signing a request.

Technically, this signs the certificate, not the request, so I think 
this should say: "Builds a certificate from a request and signs it with 
the issuer's key."

343          * Builds a certificate request of the specified type.

This also signs the request, so I think it is important to say that, ex: 
"Builds and signs a certificate request of the specified type."

* CertificateFactory

44  * their encodings. A factory can also be used to build a certificate 
request

I would probably reword this: "A factory can also return a certificate 
builder that can create certificates and certificate requests."

105  * (if {@link #generateCertificateRequest} and {@link 
#getCertificateBuilder}
106  * are implemented):

I think you can remove these lines.

407     public final Certificate.Builder<?,?> getCertificateBuilder() {

Should this be <? extends Certificate,? extends Builder> or something 
like that?

* CertificateFactorySpi

Need more details on how inStream is parsed.
Missing an @implSpec on engineGenerateCertificateRequest

* X509Certificate

657      * A builder that builds X.509 v3 certificates and certificate 
requests.

Why only v3? I think we need to allow this to support other versions, if 
there ever are any.

  673      * Implementations of {@link #buildCertificateRequest} and various
  674      * overloaded {@code #buildCertificate} must support the
  675      * {@link PKCS10CertificateRequest} type.

I don't think we should mandate this for every impl, only SE 
implementations. I would remove this.

756          * SHA384withECDSA for a 384-bit EC key. TODO: return OID?

TODO needs to be addressed.

772         String getDefaultSigAlgName(PrivateKey key);

This seems like it should just be a static utility method, and not 
something every subclass has to implement.

1064     public static class GeneralName {

This should be a standalone class, since we may leverage it in other 
APIs that are not X509Certificate specific.

1075         public int getType() {

Should this be an Enum?

* CertificateRequest

   58     private int hash = -1; // Default to -1

should be volatile.

   64      * See the CertificateFactory section in the <a href=
   65      * 
"{@docRoot}/../technotes/guides/security/StandardNames.html#CertificateFactory">

There should be a new section for CR encodings like there is one for 
CertPath encodings.

* PKCS10CertificateRequest

Methods should state whether they return or create new copies/clones.

  105      * @exception CertificateEncodingException if an encoding 
error occurs.

Use @throws. Also, do we really need to throw this exception? When 
wouldn't we be able to create a valid encoding?

--Sean

On 01/07/2016 07:33 AM, Wang Weijun wrote:
> An updated webrev
>
>    http://cr.openjdk.java.net/~weijun/8058778/webrev.07/
>    http://cr.openjdk.java.net/~weijun/8058778/webrev.07/specdiff/java/security/cert/package-summary.html
>
> with more changes:
>
> 1. Certificate.Builder and X509Certificate.Builder are now interfaces
>
> 2. Implementation is in a provider through CertificateFactory.getCertificateBuilder
>
> 3. New class CertificateRequest and PKCS10CertificateRequest, and CertificateFactory#generateCertificateRequest
>
> 4. New class X509Certificate.GeneralName
>
> 5. X509Certificate.Builder#setSigAlg(Name|OID|Params)
>
> 6. X509Certificate.Builder#serialNumber
>
> Thanks
> Max
>
>
>> On Dec 16, 2015, at 10:26 AM, Wang Weijun <weijun.wang at oracle.com> wrote:
>>
>> Hi All
>>
>> Here is an updated webrev
>>
>>   http://cr.openjdk.java.net/~weijun/8058778/webrev.05/
>>
>> Spec change is at
>>
>>   http://cr.openjdk.java.net/~weijun/8058778/webrev.05/specdiff/java/security/cert/package-summary.html
>>
>> These changes are made:
>>
>> 1. The Builder is moved into java.security.cert.X509Certificate as an inner class
>>
>> 2. There is no more addExtension(String,String,boolean) that tries to parse input value strings (leave them to keytool). Each supported extension has its own addXXXExtension() method in java.security.cert.X509Extension. The input format is the same as the output format of X509Certificate.getXXX() for each extension type. This relieves the requirement to define interfaces for GeneralNames etc at the moment.
>>
>> 3. keytool directly calls X509Certificate.Builder now.
>>
>> No CertificateRequest at the moment. Builder still using byte[] which is PKCS #10 encoded.
>>
>> Many thanks to Mandy, Larry, and Sean for your comments. Mike, we will add more methods later when they are needed.
>>
>> --Max
>>
>>> On Dec 15, 2015, at 11:53 PM, Sean Mullan <sean.mullan at oracle.com> wrote:
>>>
>>> On 12/03/2015 09:07 PM, Wang Weijun wrote:
>>>> Or if this is too much, we can at least do the X509Extension part. If
>>>> CertificateRequest is needed one day, we can create a new method
>>>> Builder.certificateRequest() that returns it and deprecate the
>>>> current request() method.
>>>>
>>>> Or use certificateRequest() to return byte[] and save request() for
>>>> the future. :-)
>>>
>>> I agree with this approach. I like the idea of moving the creation of Extensions to X509Extension so that they could be used independently of the X509Certificate.Builder API. Let's defer a CertificateRequest API for later.
>>>
>>> --Sean
>>
>