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.
