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

[Jason Uh]

Thanks for catching that. Here it is with the HandshakeMessage.java changes.

I'll push with both bug IDs.

On 03/03/2015 01:25 AM, Seán Coffey wrote:
> Jason,
>
> I think you're missing the HandshakeMessage.java changes that Vinnie
> proposed. Everything else looks good to me.
> I'd suggest that you push your changeset with both the 8054037 and
> 8055207 bug IDs then.
>
> regards,
> Sean.
>
> On 02/03/2015 23:07, Jason Uh wrote:
>> 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
>>>
>