RFR: 8213952: Relax DNSName restriction as per RFC 1123

Sean Mullan sean.mullan at oracle.com
Wed Nov 28 22:35:20 UTC 2018


Just a nit:

77             throw new IOException("DNSNames with blank components are 
not permitted");
79             throw new IOException("DNSNames may not begin or end with 
a .");

Use the singular "DNSName" to be consistent with the other exception 
messages.

--Sean

On 11/23/18 11:45 AM, Seán Coffey wrote:
> Thanks for your review Max. I've made the suggested edits.
> http://cr.openjdk.java.net/~coffeys/webrev.8213952.v3/webrev/
> 
> 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.
> 
> 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.
>>>>>>
>>>
> 



More information about the security-dev mailing list