RFR: 8213952: Relax DNSName restriction as per RFC 1123

Seán Coffey sean.coffey at oracle.com
Mon Dec 3 17:11:27 UTC 2018 

whoops:

latest webrev : 
http://cr.openjdk.java.net/~coffeys/webrev.8213952.v4/webrev/

Regards,
Sean.

On 03/12/18 17:10, Seán Coffey wrote:
>
> The CSR related to this change has been approved.
>
> I made further edits to update the DNSName comment code to reference 
> RFC 5280 rather than the obsoleted RFC 2459. I also updated the test 
> case with a few extra tests per suggestion from Chris and others. 
> Moved the dataprovider into a good and bad data set also.
>
> A follow on bug has been logged to update all references of RFC 2459 
> to RFC 5280 (JDK-8214532)
>
> Regards,
> Sean.
> On 27/11/18 14:27, Seán Coffey wrote:
>> Thanks for the feedback Max. I'll ping Joe Darcy and check if a CSR 
>> is required for this type of change. I'll revert back to this list 
>> once I have an answer.
>>
>> keytool works well with the new change. I've modified the test as per 
>> your suggestion :
>>
>> --- a/test/jdk/sun/security/tools/keytool/KeyToolTest.java
>> +++ b/test/jdk/sun/security/tools/keytool/KeyToolTest.java
>> @@ -1436,6 +1436,7 @@
>>          testOK("", pre+"san3 -ext san=dns:me.org");
>>          testOK("", pre+"san4 -ext san=ip:192.168.0.1");
>>          testOK("", pre+"san5 -ext san=oid:1.2.3.4");
>> +        testOK("", pre+"san6 -ext san=dns:1abc.com"); //begin with 
>> digit
>>          testOK("", pre+"san235 -ext 
>> san=uri:http://me.org,dns:me.org,oid:1.2.3.4");
>>
>> Regards,
>> Sean.
>>
>> On 27/11/18 01:29, Weijun Wang wrote:
>>>
>>>> On Nov 26, 2018, at 11:15 AM, Weijun Wang <weijun.wang at oracle.com> 
>>>> wrote:
>>>>
>>>>
>>>>
>>>>> On Nov 24, 2018, at 12:45 AM, Seán Coffey <sean.coffey at oracle.com> 
>>>>> wrote:
>>>>>
>>>>> Thanks for your review Max. I've made the suggested edits.
>>>>> http://cr.openjdk.java.net/~coffeys/webrev.8213952.v3/webrev/
>>>> The change looks fine. Thanks.
>>>>
>>>>> I've also submitted a CSR for this change just to cover the 
>>>>> behavioural aspect of the edit. Will push if/once that's approved 
>>>>> by CSR team.
>>>> The CSR focuses on keytool but this is actually a library change. 
>>>> There is no change with "add/remove/modify command line option" at 
>>>> all.
>>>>
>>>> I would suggest talking about the DNSName class instead of keytool. 
>>>> I understand it's internal but we can describe this change as a 
>>>> non-minimal change on behavior so it's still worth a CSR. Or, you 
>>>> can consult Joe what the best way is. Maybe he can spare you from 
>>>> filing a CSR at all.
>>>>
>>>> BTW, you did try out keytool after this code change and "-ext 
>>>> dns=1abc.com" is working now, right?
>>> You can add an extra line after line 1437 of 
>>> test/jdk/sun/security/tools/keytool/KeyToolTest.java.
>>>
>>>> Thanks
>>>> Max
>>>>
>>>>> Regards,
>>>>> Sean.
>>>>>
>>>>> On 22/11/18 14:49, Weijun Wang wrote:
>>>>>> * DNSName.java:
>>>>>>
>>>>>>   91             if ((endIndex - startIndex) < 1)
>>>>>>
>>>>>> No need for inner parentheses.
>>>>>>
>>>>>> And, is there really a need to define alphaDigitsAndHyphen? How 
>>>>>> about just (== '-' || inside alphaDigits)?
>>>>>>
>>>>>> * DNSNameTest.java:
>>>>>>
>>>>>> Space cannot appear anywhere, please add a test case like "a c.com".
>>>>>>
>>>>>> BTW, I assume you want to reuse this test for other sub-tasks of 
>>>>>> JDK-8054380? I see the @summary is more general.
>>>>>>
>>>>>> No other comments.
>>>>>>
>>>>>> Thanks
>>>>>> Max
>>>>>>
>>>>>>> On Nov 22, 2018, at 12:51 AM, Seán Coffey 
>>>>>>> <sean.coffey at oracle.com> wrote:
>>>>>>>
>>>>>>> p.s I've updated the testcase to test the IOException message 
>>>>>>> for presence of "DNSName". Webrev updated in place.
>>>>>>>
>>>>>>> Regards,
>>>>>>> Sean.
>>>>>>>
>>>>>>> On 21/11/18 15:42, Seán Coffey wrote:
>>>>>>>> Thanks for the comments Bernd. Comments inline..
>>>>>>>>
>>>>>>>> On 16/11/18 21:27, Bernd Eckenfels wrote:
>>>>>>>>> Hello Sean,
>>>>>>>>>
>>>>>>>>> I was only looking at the inspected DNSName class, it still 
>>>>>>>>> has some variations (but start now with DNSNames which is good 
>>>>>>>>> already):
>>>>>>>>>
>>>>>>>>>   76 throw new IOException("DNSName must not be null or empty");
>>>>>>>>>   78 throw new IOException("DNSNames or NameConstraints with 
>>>>>>>>> blank components are not permitted");
>>>>>>>>>   80 throw new IOException("DNSNames or NameConstraints may 
>>>>>>>>> not begin or end with a .");
>>>>>>>>>   92 throw new IOException("DNSName SubjectAltNames with empty 
>>>>>>>>> components are not permitted");
>>>>>>>>> 96 throw new IOException("DNSName components must begin with a 
>>>>>>>>> letter or digit");
>>>>>>>>> 101 throw new IOException("DNSName components must consist of 
>>>>>>>>> letters, digits, and hyphens");
>>>>>>>>>
>>>>>>>>> I did not check if those exceptions are catched and rethrown 
>>>>>>>>> with something like „while validating the SubjectAltName 
>>>>>>>>> Extension <num> of certificate <subject>...“, in my experience 
>>>>>>>>> it helps if you do not have to find and review the actual 
>>>>>>>>> certificates (in this case it might not be a problem if SAN is 
>>>>>>>>> only in the end entity). You can probably remove „or 
>>>>>>>>> NameConstraints“ and „SubjectAltNames“ from lines 78-92 (this 
>>>>>>>>> is good if DNsNa
>>>>>>>> Ok - I've cleaned up the exception messages on line 78, 80, 92.
>>>>>>>> webrev updated in place : 
>>>>>>>> http://cr.openjdk.java.net/~coffeys/webrev.8213952.v2/webrev/
>>>>>>>>
>>>>>>>>
>>>>>>>>> me applies to SAN or NameConstrained context and the 
>>>>>>>>> validation logic does not know — so it’s not only more unified 
>>>>>>>>> but also less missleading)
>>>>>>>>>
>>>>>>>>> BTW: probably not inthe scope of this fix but a subtype for 
>>>>>>>>> validation errors which have getters for context/location and 
>>>>>>>>> maybe error code and getValue() would allow callers to print 
>>>>>>>>> nicer validation reports without relying on the message or 
>>>>>>>>> Stacktraces.
>>>>>>>> That's a nice idea and one that should be followed up in 
>>>>>>>> separate enhancement. We could lean on the new 
>>>>>>>> `jdk.includeInExceptions` Security property which other 
>>>>>>>> component areas have started to use lately.
>>>>>>>>
>>>>>>>> e.g. https://bugs.openjdk.java.net/browse/JDK-8207768
>>>>>>>>
>>>>>>>> regards,
>>>>>>>> Sean.
>>>>>>>>> Gruss
>>>>>>>>> Bernd
>>>>>>>>> -- 
>>>>>>>>> http://bernd.eckenfels.net
>>>>>>>>> Von: Seán Coffey <sean.coffey at oracle.com>
>>>>>>>>> Gesendet: Freitag, November 16, 2018 5:15 PM
>>>>>>>>> An: Bernd Eckenfels; security-dev at openjdk.java.net
>>>>>>>>> Betreff: Re: RFR: 8213952: Relax DNSName restriction as per 
>>>>>>>>> RFC 1123
>>>>>>>>> Thanks for the comments Bernd. comments inline..
>>>>>>>>>
>>>>>>>>> On 16/11/18 12:40, Bernd Eckenfels wrote:
>>>>>>>>>> You could also add (a..b, false) and (.a, false), (a., false) 
>>>>>>>>>> to the testcases.
>>>>>>>>> good point. test updated.
>>>>>>>>>> I noticed that there are different types of Exception 
>>>>>>>>>> messages (DNS name, DNSName, DNS Name or name constrained, 
>>>>>>>>>> DNS name and SAN), would be good if all of them have the same 
>>>>>>>>>> keyword?
>>>>>>>>> I cleaned up some other references to DNSName in the 
>>>>>>>>> sun.security.x509 package. I'm not sure what classes you were 
>>>>>>>>> referencing the above examples from.
>>>>>>>>>
>>>>>>>>> new webrev : 
>>>>>>>>> http://cr.openjdk.java.net/~coffeys/webrev.8213952.v2/webrev/
>>>>>>>>>
>>>>>>>>> regards,
>>>>>>>>> Sean.
>>>>>>>>>> Gruss
>>>>>>>>>> Bernd
>>>>>>>>>> -- 
>>>>>>>>>> http://bernd.eckenfels.net
>>>>>>>>>> Von: security-dev <security-dev-bounces at openjdk.java.net> im 
>>>>>>>>>> Auftrag von Seán Coffey <sean.coffey at oracle.com>
>>>>>>>>>> Gesendet: Freitag, November 16, 2018 12:25 PM
>>>>>>>>>> An: OpenJDK Dev list
>>>>>>>>>> Betreff: RFR: 8213952: Relax DNSName restriction as per RFC 1123
>>>>>>>>>> Looking to make an adjustment to DNSName constructor to help 
>>>>>>>>>> comply with
>>>>>>>>>> RFC 1123
>>>>>>>>>>
>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8213952
>>>>>>>>>> http://cr.openjdk.java.net/~coffeys/webrev.8213952/webrev/
>>>>>>>>>>
>>>>>>>>>> regards,
>>>>>>>>>> Sean.
>>>>>>>>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20181203/662a16ec/attachment.htm>

More information about the security-dev mailing list