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

[Seán Coffey]

http://cr.openjdk.java.net/~juh/8054037/02/

Looks good.

regards,
Sean.

On 03/03/2015 18:25, Jason Uh wrote:
> 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
>>>>
>>