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

[Weijun Wang]

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.
>