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

[Xuelei Fan]

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