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.w...@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.mul...@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
