-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Unify TLS configuration #3902
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 2.x
Are you sure you want to change the base?
Unify TLS configuration #3902
Conversation
- Introduced TlsConfiguration interface - Defaulted verifyHostname to true - Repurposed Http.verifyHostname as a toggle for backward compatibility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jhl221123, thanks so much for the PR! 🙇 I've a question: In @ppkarwasz's original proposal, there is also the following item:
Currently if the user configures an
SSL
element, all thelog4j2.ssl*
configuration properties are ignored. I think those properties should still be used to provide default values forSSL
.
@jhl221123, do you plan to update this PR to reflect this too?
@ppkarwasz, I'm not able to see the value of a new TlsConfiguration
interface here. The fact that it contains a getSslContext
method (note the Ssl
in the name!) makes it even more clumsy. I know that SSL keyword is superseded by TLS, but from an API point of view in the Java ecosystem, is it really? What will be the benefit of introducing TlsFoo
as long as we will have SslFoo
and SslBar
around for functionality, backward compatibility, etc. reasons?
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/HttpAppender.java
Outdated
Show resolved
Hide resolved
log4j-core/src/main/java/org/apache/logging/log4j/core/appender/HttpAppender.java
Outdated
Show resolved
Hide resolved
@vy Thanks for the feedback!
Absolutely! But I noticed in #2792 you mentioned that it might be better to handle this in a separate PR. Which approach would you prefer: should I continue updating this PR, or would it be better to open a separate one for it? |
- Update HttpAppender deprecation warning message - Make isVerifyHostname() simple
I don't see the point in calling it Where I do see value, though, is in introducing an interface rather than tying everything to the concrete Most popular Java HTTP clients ( public interface TlsConfiguration {
SSLContext getSslContext();
HostnameVerifier getHostnameVerifier();
/** Optional **/
SSLParameters getSslParameters();
} Jakarta Mail is the only outlier that still needs an With such an abstraction, you could, for example, plug in Spring Boot’s Each host would only need to declare which @rgoers: does this kind of abstraction line up with what you’d find useful in practice? |
@ppkarwasz, thanks for clarifying the use case in mind. That certainly helps with setting the feature's intended context.
I understand and share your preference for an interface, yet given the current state of the code, wouldn't it be less intrusive to provide a similar adapter pattern support by adding a sufficient |
TlsConfiguration
interfaceverifyHostname
totrue
Http.verifyHostname
as a toggle for backward compatibilityRelated to #2792