> On Jan 8, 2016, at 6:06 AM, Sean Mullan <sean.mul...@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
