code review request: 7109096: keytool -genkeypair needn't call -selfcert

Wed Nov 9 04:53:59 UTC 2011

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