RFR 8058778: New APIs for creating certificates and certificate requests

[Wang Weijun]

> On Jan 8, 2016, at 6:06 AM, Sean Mullan <sean.mullan at oracle.com> wrote:
> 
> This is looking really nice. Here are some comments on the API portion, will review the impl later ...
> 
> 407     public final Certificate.Builder<?,?> getCertificateBuilder() {
> 
> Should this be <? extends Certificate,? extends Builder> or something like that?

I can write it into <? extends Certificate,? extends Certificate.Builder<?,?>> to avoid a warning. But it still seems incomplete because it does not show that the 1st ? should be the same as the 3rd one.

I'll do some experiment with a simple program. Might need to ask langtools.

> 
> * CertificateFactorySpi
> 
> Need more details on how inStream is parsed.

I thought a "@see CertificateFactory#generateCertificateRequest" is enough. I do noticed that CertificateFactorySpi#engineGenerateCertificate copies all spec from CertificateFactory#generateCertificate.

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

IMO no one wants v1 or v2 now. Or are you thinking of a future version?

That said, I can add a version(Enum) method. If it's v1 or v2 extensions will be ignored, if >3, UOE.

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

We know the OID is more precise than the Name, and it's the actual value encoded in a cert, therefore I wondered if we should instead make a getDefaultSigAlgOId().

My opinion now is to keep using getDefaultSigAlgName(). As long as a name is a standard one, it is equivalent to an OID and more people understand it.

So, just remove the TODO words.

> 
> 772         String getDefaultSigAlgName(PrivateKey key);
> 
> This seems like it should just be a static utility method, and not something every subclass has to implement.

But only the provider (X509Factory here) knows about the return values, and another provider can return different values.

> 
> 1064     public static class GeneralName {
> 
> This should be a standalone class, since we may leverage it in other APIs that are not X509Certificate specific.

Honestly the current GeneralName is not quite useful. It is only used as a *parameter* in building certificates. A real GeneralName needs getEncoded().

Or, we can do it like

 interface GeneralName {
   byte[] getEncoded();
 }

 class X509Certificate.Builder {
   GeneralName newGeneralName(int/Enum type, String svalue);
   GeneralName newGeneralName(int/Enum type, byte[] value);
 }

> 
> 1075         public int getType() {
> 
> Should this be an Enum?

Yes.

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

Aha, I copied most from X509Certificate, where getTBSCertificate() throws such an exception.

In a public API view, you can only create a Certificate from CertificateFactory#generateCertificate, therefore the cert is always encoded and signed. But we do have X509CertImpl that extends X509Certificate that can be unfilled, unsigned and unencoded, and looks like this is the reason that getTBSCertificate() and getEncoded() could throw an exception. The PKCS10 class has the same problem.

I consider this not a feature but a bug. Every public available Certificate and CertificateRequest objects should be encoded and signed and immutable. I'll remove that exception from getCertificationRequestInfo(). We need to make sure that unsigned and unencoded Certificate or CertificateRequest should never be revealed to an application.

Thanks
Max


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