On 2 Mar 2015, at 17:26, Seán Coffey <[email protected]> wrote:
> Jason,
>
> thanks for taking this on. Your changes look fine to be and should help the
> debugging experience. Some extra comments from me. Here's some standard
> output that one sees (early in connection) from a standard TLS connection
> attempt with verbose certpath logging :
>
>> certpath: PKIXCertPathValidator.engineValidate()...
>> certpath: AdaptableX509CertSelector.match: subject key IDs don't match
>> certpath: NO - don't try this trustedCert
>
> Can we print the subject key IDs for reference ? I'm conscious of line
> wastage in log files. Bunching the IDs in with the "don't try this
> trustedCert line" would work perhaps.
>
> Shortly after that, one reads :
>
>> certpath: Executing PKIX certification path validation algorithm.
>> certpath: Checking cert1 ...
>> certpath: Set of critical extensions:
>> certpath: 2.5.29.15
>> certpath: 2.5.29.19
> Could we improve the PKIXMasterCertPathValidator.validate printing in this
> case ? Let's print the Subject and/or ID of "cert1"
> I'm not sure why we print the critical extension list here. Could we append
> them after "Set of critical extensions:" to avoid extra lines ?
>
> Shortly after that, we have this in output :
>
>> certpath: ---checking basic constraints...
>> certpath: i = 1
>> certpath: maxPathLength = 2
>> certpath: after processing, maxPathLength = 0
>> certpath: basic constraints verified.
>> certpath: ---checking name constraints...
>> certpath: prevNC = null
>> certpath: newNC = null
>> certpath: mergedNC = null
>> certpath: name constraints verified.
>> certpath: -checker4 validation succeeded
>
> just a tidy up thought, could some of the lines above be concatenated ?
> E.g. : ConstraintsChecker.java
>
> 227 debug.println("---checking " + msg + "...");
> 228 debug.println("i = " + i);
> 229 debug.println("maxPathLength = " + maxPathLength);
>
> Maybe some room for concatenation here also :
> certpath: ---checking timestamp:Mon Mar 02 16:34:47 GMT 2015...
> certpath: timestamp verified.
>
> Finally - another reason for why I logged the enhancement request in the
> first place..
>
> Take this output :
>
>> *** ServerHelloDone
>> [read] MD5 and SHA1 hashes: len = 4
>> 0000: 0E 00 00 00 ....
>> *** Certificate chain
>> ***
>
> The final "***" here indicates that the truststore is empty. It's not very
> obvious to the novice user!
> I believe the output corresponds to :
> jdk/src/java.base/share/classes/sun/security/ssl/HandshakeMessage.java#498
>
> We need to at least print "Empty cert chain" where applicable. This belongs
> in jsse code but it would be great if this can be improved as part of this
> fix.
Jason, I have a patch prepared for this that never got pushed. You could
include it in your changeset.
diff --git a/src/java.base/share/classes/sun/security/ssl/ClientHandshaker.java
b/src/java.base/share/classes/sun/security/ssl/ClientHandshaker.java
--- a/src/java.base/share/classes/sun/security/ssl/ClientHandshaker.java
+++ b/src/java.base/share/classes/sun/security/ssl/ClientHandshaker.java
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 1996, 2013, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1996, 2014, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -750,6 +750,11 @@
} else {
warningSE(Alerts.alert_no_certificate);
}
+ if (debug != null && Debug.isOn("handshake")) {
+ System.out.println(
+ "Warning: no suitable certificate found - " +
+ "continuing without client authentication");
+ }
}
diff --git a/src/java.base/share/classes/sun/security/ssl/HandshakeMessage.java
b/src/java.base/share/classes/sun/security/ssl/HandshakeMessage.java
--- a/src/java.base/share/classes/sun/security/ssl/HandshakeMessage.java
+++ b/src/java.base/share/classes/sun/security/ssl/HandshakeMessage.java
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 1996, 2012, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1996, 2014, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -490,11 +490,14 @@
void print(PrintStream s) throws IOException {
s.println("*** Certificate chain");
- if (debug != null && Debug.isOn("verbose")) {
- for (int i = 0; i < chain.length; i++)
+ if (chain.length == 0) {
+ s.println("<Empty>");
+ } else if (debug != null && Debug.isOn("verbose")) {
+ for (int i = 0; i < chain.length; i++) {
s.println("chain [" + i + "] = " + chain[i]);
- s.println("***");
+ }
}
+ s.println("***");
}
X509Certificate[] getCertificateChain() {
>
> regards,
> Sean.
>
> On 13/02/15 00:05, Jason Uh wrote:
>> Please review this change, which augments some of the debug statements for
>> java.security.debug=certpath.
>>
>> webrev: http://cr.openjdk.java.net/~juh/8054037/00/
>> bug: https://bugs.openjdk.java.net/browse/JDK-8054037
>>
>> Thanks,
>> Jason
>
