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.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.
