[9] RFR: 8054037: Improve tracing for java.security.debug=certpath

[Jason Uh]

Thanks for the comments, Sean; Vinnie, for the patch.

Here's an updated webrev with your suggested changes and Vinnie's patch.
http://cr.openjdk.java.net/~juh/8054037/01/

I've also removed a few unnecessary toString() calls in 
AdaptableX509CertSelector.java and DistributionPointFetcher.java.

Jason


On 03/02/2015 09:26 AM, Seán Coffey 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.
>
> 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
>