SSLLogger.java (was Re: Code Review Request: TLS 1.3 Implementation)

[Seán Coffey]

Some comments on SSLLogger also :

formatCaller() uses getStackTrace() to walk the stack. It's probably 
more expensive than using the newer Stackwalker class. Could it be 
replaced with something like :

>          return StackWalker.getInstance().walk(s ->
>                  s.dropWhile((f -> 
> f.getClassName().startsWith("sun.security.ssl.SSLLogger")))
>                          .map(f -> f.getClassName() + ":" + 
> f.getLineNumber())
>                          .findFirst().orElse("unknown caller"));

Looks like this method is not used :

>   406         private static boolean isLoggerImpl(StackTraceElement ste) {
>   407             String cn = ste.getClassName();
>   408             System.out.println("class name: " + cn);
>   409             return cn.equals("sun.security.ssl.SSLLogger");
>   410         }

Also the SSLSimpleFormatter class has dateFormat variable stored in a 
ThreadLocal. Is there a reason for that ?

regards,
Sean.


On 08/06/2018 02:40, Xuelei Fan wrote:
> I got your ideas.  Let's see if we can make a change, a little bit later.
>
> Thanks,
> Xuelei
>
> On 6/7/2018 6:23 PM, Weijun Wang wrote:
>> My final comment on this class:
>>
>> Since SSLConsoleLogger is a Logger child, its log(..., params) method 
>> should follow the Logger::log spec, which has
>>
>>          /**
>>           * Logs a message with resource bundle and an optional list of
>>           * parameters.
>>           * ...
>>           * @param format the string message format in {@link
>>           * java.text.MessageFormat} format, (or a key in the message
>>           * catalog if {@code bundle} is not {@code null}); can be 
>> {@code null}.
>>           * @param params an optional list of parameters to the 
>> message (may be
>>           * none).
>>           * ...
>>           */
>>          public void log(Level level, ResourceBundle bundle, String 
>> format,
>>                  Object... params);
>>
>> Especially, the String parameter is in MessageFormat format (that's 
>> why it's named "format" but not "message"). However, 
>> SSLConsoleLogger::log is always called with format being a plain 
>> string and params are stacked in the output separated by command and 
>> newline.
>>
>> My opinion is that this is not the normal style of using a Logger and 
>> we should not allow system logger (through -Djavax.net.debug=). If 
>> you still see benefit in that, can we make this change at the moment?
>>
>> -                    logger.log(level, msg, formatted);
>> +                    if (logger instanceof SSLConsoleLogger) {
>> +                        logger.log(level, msg, formatted);
>> +                    } else {
>> +                        logger.log(level, msg + ": " + formatted);
>> +                    }
>>
>> Thanks
>> Max
>>
>>> On Jun 7, 2018, at 11:24 AM, Weijun Wang <weijun.wang at oracle.com> 
>>> wrote:
>>>
>>> I think I understand. So if a user sets it to empty, they will also 
>>> need to create a customized logger/formatter like SSLLogger that is 
>>> able to output a parameter without a placeholder in msg.
>>>
>>> --Max
>>>
>>>> On Jun 7, 2018, at 9:51 AM, Xuelei Fan <xuelei.fan at oracle.com> wrote:
>>>>
>>>> In this implementation, the SunJSSE logging is updated to (the 
>>>> class comment lines):
>>>>
>>>> /**
>>>> * Implementation of SSL logger.
>>>> *
>>>> * If the system property "javax.net.debug" is not defined, the 
>>>> debug logging
>>>> * is turned off.  If the system property "javax.net.debug" is 
>>>> defined as
>>>> * empty, the debug logger is specified by 
>>>> System.getLogger("javax.net.ssl"),
>>>> * and applications can customize and configure the logger or use 
>>>> external
>>>> * logging mechanisms.  If the system property "javax.net.debug" is 
>>>> defined
>>>> * and non-empty, a private debug logger implemented in this class 
>>>> is used.
>>>> */
>>>>
>>>> On 6/6/2018 6:37 PM, Weijun Wang wrote:
>>>>> But will any application use the logger named "javax.net.debug"?  
>>>>> I think that's only for JSSE.
>>>> If using System Logger, the "javax.net.debug" system property will 
>>>> be used to turn on/off the logger (define with empty or not defined).
>>>>
>>>> Xuelei
>>>>
>>>>>> On Jun 7, 2018, at 9:35 AM, Xuelei Fan<xuelei.fan at oracle.com>  
>>>>>> wrote:
>>>>>>
>>>>>> I see your concerns now.  Currently, we don't fine the format if 
>>>>>> using System logger.  Applications can define the format and 
>>>>>> output style for themselves if needed. That's also the purpose 
>>>>>> why we introduce the System Logger in the provider.
>>>
>>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20180609/50222977/attachment.htm>