ZOOKEEPER-5058: Remove special characters from ensemble name before logging in EnsembleAuthenticationProvider

[anmolnar]

cc @phunt @ztzg

[PDavid]

Thanks for this fix. 👍

Just a question: Would it be a enough to not log the client input itself - e.g. the unexpected ensemble name here?
What is the advantage of logging the wrong ensemble name?

[anmolnar]

Thanks for this fix. 👍

Just a question: Would it be a enough to not log the client input itself - e.g. the unexpected ensemble name here? What is the advantage of logging the wrong ensemble name?

Thanks for the review. That's actually a very good point!
Anybody has objection against removing the logging of the ensemble name?

[phunt]

My 0.02 - it seems like ensemble name could be useful for troubleshooting purposes. Given we have this issue for logging of other externally controlled data perhaps we could map to eg "*" (via subroutine used across the codebase) and indicate in the log string itself that the name may be masked? Perhaps a general such note in the documentation somewhere that says along the lines of "We may mask certain logged externally controled fields with '*' to protect against log corruption" (not sure the technical term to be used here... ).

[PDavid]

I see that this is a trade-off: it can be useful if we log it. If we don't log it, it is more secure.

We could use the replace conversion pattern in logback:
https://logback.qos.ch/manual/layouts.html#replace

Or how about integrating org.owasp:security-logging-logback and define a custom conversionRule for a CRLFConverter in our logback XML to sanitize input? This would be a general solution for the whole project.

<dependency>
    <groupId>org.owasp</groupId>
    <artifactId>security-logging-logback</artifactId>
    <version>1.1.6</version>
</dependency>   

<configuration>
    <conversionRule conversionWord="crlf" 
                    converterClass="org.owasp.security.logging.mask.CRLFConverter" />
    
    <appender name="STDOUT" class="ch.qos.logback.core.ConsoleAppender">
        <encoder>
            <!-- Use %crlf to sanitize the message -->
            <pattern>%d{HH:mm:ss.SSS} [%thread] %-5level %logger{36} - %crlf(%msg)%n</pattern>
        </encoder>
    </appender>
    
    <root level="debug">
        <appender-ref ref="STDOUT" />
    </root>
</configuration>   

https://github.com/augustd/owasp-security-logging/wiki/Log-Forging

[anmolnar]

I like the logback approach too.

[anmolnar]

Made the changes. I think the downside of that is we don't have control over it, needs additional administrative task if user doesn't use our sample config and it introduces a new dependency. Apart from those, it works fine.

[anmolnar]

Looks like the change causes some problems in other unit tests too.