> On Nov 24, 2018, at 12:45 AM, Seán Coffey <sean.cof...@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?
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.cof...@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.cof...@oracle.com>
>>>>> Gesendet: Freitag, November 16, 2018 5:15 PM
>>>>> An: Bernd Eckenfels; security-dev@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-boun...@openjdk.java.net> im Auftrag
>>>>>> von Seán Coffey <sean.cof...@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.
>>>>>>
>>>
>
