The CertAndKeyGen class is for generating a keypair and create a
certificate or cert request. The normal usage for it will be
1. create an object
2. call generate() to generate keypair
3. call getSelfCertificate or getCertRequest
Both methods in step 3 assume the publicKey to be X.509 encoded.
Of course, one can only call the first 2 steps and do not go on. But in
order to do that, a simple KeyPairGenerator is enough.
I still think the current webrev.02 is correct. Yes, I can move the
check back to getPublicKeyAnyway(). Or you can say the check is
completely useless, because at least we are very sure in KeyTool that if
the format is not X.509, something bad will happen anyway when we call
getSelfCertificate. Maybe an assert statement is better.
-Max
On 11/09/2011 12:53 PM, Xuelei Fan wrote:
One more comment that I'm not sure why you come to the conclusion that
CertAndKeyGen has already assumed that the public key is of X.509 format.
And what did you want to show with the example of getSelfCertificate()
method? I did not find it is useful to come to the above conclusion.
I'm not sure who are the caller of getPublicKey() and generate().
Conservatively, I think it would be safer to make the check in
getPublicKeyAnyway() instead of generate().
Just my 0.2 cents.
Xuelei
On 11/9/2011 12:01 PM, Xuelei Fan wrote:
I'm also working on some other CR that need to considering the key
format. The key.getFormat() may return null, so the safer comparing
should be:
+ 160 if (!"X.509".equalsIgnoreCase(publicKey.getFormat())) {
- 160 if (!publicKey.getFormat().equalsIgnoreCase("X.509")) {
Otherwise, looks fine to me.
Thanks,
Xuelei
On 11/9/2011 11:32 AM, Weijun Wang wrote:
Well, in fact the whole CertAndKeyGen class already assumes that the
public key has an X.509 encoding format.
public X509Certificate getSelfCertificate (
X500Name myname, Date firstDate, long validity) ...
{
....
info.set(X509CertInfo.KEY, new CertificateX509Key(publicKey));
....
and CertificateX509Key.encode() simply calls key.getEncoded() without
checking its format.
So here is an updated webrev:
http://cr.openjdk.java.net/~weijun/7109096/webrev.02/
BTW, I didn't get external report on the failure. But the -selfcert
command is now failing on MSCAPI because of some other instanceof
checks. Before that gets fixed, I want to take -selfcert out of
-genkeypair. -selfcert is an obsolete command but -genkeypair is almost
the most important one for keytool.
Thanks
Max
On 11/09/2011 03:24 AM, Michael StJohns wrote:
Looking at the API definitions, it's possible for an RSAPublicKey
implementation to have an encoding that is not "X.509". So, the
right check really is "publicKey.getFormat().equalsIgnoreCase("X.509")
and not "publicKey instanceof RSAPublicKey". No need for the "or"
check. Or maybe the instance check is "publicKey instanceof PublicKey"
Mike
At 06:57 AM 11/8/2011, Xuelei Fan wrote:
Did you get any failure report about the CR?
I asked the question because I concern about the format of the encoded
public key. I'm not sure whether it is always be X.509 or not. If it is
not of X.509, we properly cannot calculate the KID properly, and then
would be in the risk to chain the AKID, SKID incorrectly. Or if it is
not of X.509, we may get another exception during parse public key.
If we got complains about the issue, I would suggest you check the
encoded format:
179 public PublicKey getPublicKeyAnyway() {
+ if ((publicKey instanceof X509Key) ||
+ publicKey.getFormat().equalsIgnoreCases("X.509")) {
180 return publicKey;
+ }
+
+ return null;
181 }
If we cannot get the expected public key as expected, I think we proper
have to resign it in order to get the public key from signed
certificate. Fortunately, our PKCS11 implementation do returns "x.509"
format for RSA, DSA, DH and EC public key.
And a minor comment:
178 // Used by KeyTool, where publicKey is not a X509Key on Solaris.
I think is is not because of Solaris platform, but because of the PKCS11
provider, so that publicKey is not an instance of X509Key. How about:
/**
* Always returns the public key of the generated key pair. Used
* by KeyTool only.
*
* The publicKey is not necessarily to be an instance of
* X509Key in some JCA/JCE providers, for example SunPKCS11.
*/
BTW, I just noticed that KeyTool does not use AKID in self-signed
certificated. It's fine, but personally, I prefer to use both AKID and
SKID in all certificates.
Xuelei
On 11/8/2011 4:29 PM, Weijun Wang wrote:
webrev updated at
http://cr.openjdk.java.net/~weijun/7109096/webrev.01/
This time JPRT tests jdk_security3 passes on all platforms.
Thanks
Max
On 11/08/2011 03:18 PM, Weijun Wang wrote:
I only run tests on my Linux before posting the webrev. Then, in the
pre-push JPRT run, it fails on all Solaris!
Turns out that CertAndKeyGen has
public X509Key getPublicKey()
{
if (!(publicKey instanceof X509Key)) {
return null;
}
return (X509Key)publicKey;
}
So the public key, which I guess is a P11RSAPublicKey, is now null.
I'll
try to find a workaround.
Thanks
Max
On 11/08/2011 11:19 AM, Xuelei Fan wrote:
Looks fine in general. Please make sure all regression tests are
passed.
Thanks,
Xuelei
On 11/7/2011 7:34 PM, Weijun Wang wrote:
Description:
keytool uses CertAndKeyGen to generate a basic self-signed
certificate
with no extensions. When -ext option was introduced, -genkeypair was
implemented as original -genkeypair plus -selfcert, and
extensions info
was added in the -selfcert step.
This means the keystore object is modified twice in this single
operation. In the case of PKCS11 or MSCAPI, it is actually
written to
the token twice. If a token can only be written once, the action
will
fail.
Webrev:
http://cr.openjdk.java.net/~weijun/7109096/webrev.00/
No new regression test (noreg-cleanup).
Note: NetBeans consolidates the multiple import lines in
CertAndKeyGen
into one. I'm not against that.
Thanks
Max
