[security-dev 00579]: Re: Code review request: 6780416: New keytool commands/options: -gencert, -printcertreq, -ext

[Max (Weijun) Wang]

Hi Andrew

First and quite important, please add my name into the To: field of  
the mail. The filter in my office computer would move this mail into  
the very corner of the mailbox. Fortunately I'm at home now and still  
have a chance to catch it.

On Feb 19, 2009, at 12:05 AM, Xuelei Fan wrote:

>> If you find the webrev too long, you might only review a part of it.
>
> 1. src/share/classes/sun/security/x509/ 
> IssuerAlternativeNameExtension.java
>
> Adding a new constructor which allow mark this extension as critical.
> The spec requires "Where present, conforming CAs SHOULD mark this
> extension as non-critical. Do you really want to mark it critical  
> freely as the request?

For every non-MUST extension, I leave a chance for the user to provide  
its criticality. In fact, this class already have another constructor  
public IssuerAlternativeNameExtension(Boolean critical, Object value),  
so it's not so evil to add another constructor which also accept the  
critical argument.

>
> 2. src/share/classes/sun/security/x509/CertificateExtensions.java
> I have no reading the keytool class, so I don't know why you have to  
> add
> a getNameByOid(ObjectIdentifier) method here. The name of an oid could
> be get from OIDMap by static. Or this name is not refer to that name  
> in OIDMap?

This method is used in "-ext honored=KU,-BC", CertificateExtensions  
stores extensions in a Map<String,Extension> so I need to find the  
name for a given type of Extension to update/delete it.

I remember the name in OIDMap and name in CertificateExtensions are  
different. The former is full class name for reflection, latter is a  
simple name. I'll check it today.

>
> 3. src/share/classes/sun/security/x509/CertAndKeyGen.java
> Why remove the SKID extension from getSelfCertificate()? Are you  
> sure the remove has no impact on other models.

Yes I'm sure.

After the fix, all extensions are added inside the  
createV3Extensions() method, and SKID is always added there. Also, the  
createV3Extensions() method is always called even if there's no -ext  
option at all.

>
>
> I will look at KeyTool.java tomorrow, others looks fine for me by now.

Good.

Thanks
Max

>
>
> Xuelei
>
> Max (Weijun) Wang wrote:
>> Hi All
>>
>> Can you take a review of this RFE?
>>
>>  6780416: New keytool commands/options: -gencert, -printcertreq, -ext
>>  bug: http://bugs.sun.com/view_bug.do?bug_id=6780416
>>  webrev: http://hgrev.appspot.com/show?id=3077
>>
>> The spec of the 3 new commands/options is inside the evaluation  
>> section of the bug report.
>>
>> The fix is mainly on KeyTool.java, with changes in Resources.java  
>> for l10n strings. Some X.509 files are changed to provide new  
>> constructor, new constants etc. A new class  
>> SubjectInfoAccessExtension.java is created for the extension. The  
>> KeyToolTest.java regression test are updated to cover the new  
>> commands/options.
>>
>> If you find the webrev too long, you might only review a part of it.
>>
>> Thanks
>> Max
>>
>>
>
>