RFR: 8213952: Relax DNSName restriction as per RFC 1123
Seán Coffey
sean.coffey at oracle.com
Mon Dec 3 17:10:26 UTC 2018
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/c470ba8b/attachment.htm>
More information about the security-dev
mailing list