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

Xuelei Fan xuelei.fan at oracle.com
Wed Nov 9 05:36:42 UTC 2011 

>>>>>> Fortunately, our PKCS11 implementation do returns "x.509"
>>>>>> format for RSA, DSA, DH and EC public key.
Please go with your latest update. Please pay attention that
publicKey.getFormat() may be null.

Xuelei

On 11/9/2011 1:15 PM, Weijun Wang wrote:
> 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
>>>>>>>>>
>>>>>
>>>>>
>>>
>>

More information about the security-dev mailing list