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