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

[Michael StJohns]

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