> On Nov 26, 2018, at 11:15 AM, Weijun Wang <weijun.w...@oracle.com> wrote:
>
>
>
>> 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?
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.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.
>>>>>>>
>>>>
>>
>
