ethaden commented on code in PR #14130:
URL: https://github.com/apache/kafka/pull/14130#discussion_r1285195172
##########
clients/src/main/java/org/apache/kafka/common/security/ssl/DefaultSslEngineFactory.java:
##########
@@ -255,7 +271,7 @@ private SSLContext createSSLContext(SecurityStore keystore,
SecurityStore trusts
}
String tmfAlgorithm = this.tmfAlgorithm != null ?
this.tmfAlgorithm : TrustManagerFactory.getDefaultAlgorithm();
- TrustManagerFactory tmf =
TrustManagerFactory.getInstance(tmfAlgorithm);
+ CommonNameLoggingTrustManagerFactoryWrapper tmf =
CommonNameLoggingTrustManagerFactoryWrapper.getInstance(tmfAlgorithm);
Review Comment:
I have an alternative idea which might be worse discussing as it even speed
up the handling of such faulty clients compared to the current implementation
in trunk. We could cache negative decisions (i.e. rejected cert chains) and
reject further client connections based on the cache without revalidating them.
The idea is based on two assumptions:
Assumption 1: As long as the truststore is unmodified, any invalid
certificate chain will never become valid, except if certificates are not yet
valid (NotBefore constraint violated). The latter should rarely occur.
Assumption 2: Consulting an efficiently implemented decision cache would be
significantly faster than validating a certificate chain.
The algorithm would be like this:
1. For any client certificate chain to be checked, we would first check if
the truststore has been modified.
2. If this is the case, we invalidate our cache.
3. Otherwise, we calculate a hash over the whole cert chain and check if it
is contained in our cache.
4. If this is the case, we reject the chain.
5. Otherwise we do the regular validation and accept the certificate if the
validation is successful.
6. If the validation fails, we identify if this is due to the end certifcate
being expired and log this.
7. We calculate the hash over the whole chain (see above) and add it to the
cache, but only if this cert might not become valid in the future (NotBefore
constraint violated).
The cache should be implemented using a hash set (lookup with ~ O(1)) which
is bounded in size (to prevent attackers from causing OOM) by keeping only the
latest N entries. The number N might be configurable.
Advantage: We speed up handling of all invalid certificates as soon as they
have been found invalid once. Any valid certificate will just be affected by a
very quick extra step (calc hash, lookup hash in reject cache).
What do you think about this approach?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
