Re: RFR: 8213952: Relax DNSName restriction as per RFC 1123

2018-11-23 Thread Seán Coffey 

Thanks for your review Max. I've made the suggested edits.
http://cr.openjdk.java.net/~coffeys/webrev.8213952.v3/webrev/

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.


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  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  of certificate ...“, 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 

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  im Auftrag von Seán Coffey 

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.

RFR 8214262: SunEC native code does not compile with debug on

2018-11-23 Thread Weijun Wang 
I was curious in some SunEC internals and turned EC_DEBUG on, and found out the 
library cannot be built. It looks like back when we switched to 
warnings-as-errors some codes are not updated. Here is the patch:

diff --git a/src/jdk.crypto.ec/share/native/libsunec/impl/ec.c 
b/src/jdk.crypto.ec/share/native/libsunec/impl/ec.c
--- a/src/jdk.crypto.ec/share/native/libsunec/impl/ec.c
+++ b/src/jdk.crypto.ec/share/native/libsunec/impl/ec.c
@@ -109,16 +109,16 @@
 printf("\n");
 
 if (k1 != NULL) {
-mp_tohex(k1, mpstr);
+mp_tohex((mp_int*)k1, mpstr);
 printf("ec_points_mul: scalar k1: %s\n", mpstr);
-mp_todecimal(k1, mpstr);
+mp_todecimal((mp_int*)k1, mpstr);
 printf("ec_points_mul: scalar k1: %s (dec)\n", mpstr);
 }
 
 if (k2 != NULL) {
-mp_tohex(k2, mpstr);
+mp_tohex((mp_int*)k2, mpstr);
 printf("ec_points_mul: scalar k2: %s\n", mpstr);
-mp_todecimal(k2, mpstr);
+mp_todecimal((mp_int*)k2, mpstr);
 printf("ec_points_mul: scalar k2: %s (dec)\n", mpstr);
 }
 
diff --git a/src/jdk.crypto.ec/share/native/libsunec/impl/ecdecode.c 
b/src/jdk.crypto.ec/share/native/libsunec/impl/ecdecode.c
--- a/src/jdk.crypto.ec/share/native/libsunec/impl/ecdecode.c
+++ b/src/jdk.crypto.ec/share/native/libsunec/impl/ecdecode.c
@@ -55,6 +55,9 @@
 #include "ecl-curve.h"
 #include "ecc_impl.h"
 
+#if EC_DEBUG
+#include 
+#endif
 #define MAX_ECKEY_LEN   72
 #define SEC_ASN1_OBJECT_ID  0x06

In the first file, k1 and k2 are defined as const mp_int * but mp_tohex accepts 
a mp_int* as its 1st argument.

In the second file, there is a printf call in a EC_DEBUG block.

Noreg-cleanup.

Thanks
Max