code review request: 7109096: keytool -genkeypair needn't call -selfcert
Weijun Wang
weijun.wang at oracle.com
Wed Nov 9 05:54:04 UTC 2011
On 11/09/2011 01:36 PM, Xuelei Fan wrote:
>>>>>>> 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.
OK. I'll reverse the order.
Thanks
Max
>
> 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