You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "rajinisivaram (via GitHub)" <gi...@apache.org> on 2023/08/14 09:01:28 UTC
[GitHub] [kafka] rajinisivaram commented on a diff in pull request #14130: KAFKA-15273: Log common name of expired client certificates
rajinisivaram commented on code in PR #14130:
URL: https://github.com/apache/kafka/pull/14130#discussion_r1293192482
##########
clients/src/test/resources/log4j.properties:
##########
@@ -19,3 +19,5 @@ log4j.appender.stdout.layout=org.apache.log4j.PatternLayout
log4j.appender.stdout.layout.ConversionPattern=[%d] %p %m (%c:%L)%n
log4j.logger.org.apache.kafka=ERROR
+# We are testing for a particular INFO log message in DefaultSslEngineFactory
+log4j.logger.org.apache.kafka.common.security.ssl.DefaultSslEngineFactory=INFO
Review Comment:
You should be able to set the log level in the test that relies on this instead of setting it here in the file used for all tests.
##########
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:
As @omkreddy mentioned, it would be better to do this change under a config, so that we have the ability to disable this path. Earlier, we we using the Java implementation, but now we have a wrapper in a security-critical code path. It will be useful to disable it in case there is some scenario where it doesn't work in future. Caching also adds a slight risk. And as already mentioned, there is the performance impact. Instead of a new config, you could also add a new implementation of the existing config `ssl.engine.factory.class`, which provides the extra logging.
##########
clients/src/main/java/org/apache/kafka/common/security/ssl/CommonNameLoggingTrustManagerFactoryWrapper.java:
##########
@@ -0,0 +1,423 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.common.security.ssl;
+
+import org.apache.kafka.common.KafkaException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.math.BigInteger;
+import java.security.InvalidKeyException;
+import java.security.KeyStore;
+import java.security.KeyStoreException;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
+import java.security.NoSuchProviderException;
+import java.security.Principal;
+import java.security.PublicKey;
+import java.security.SignatureException;
+import java.security.cert.CertificateEncodingException;
+import java.security.cert.CertificateException;
+import java.security.cert.CertificateExpiredException;
+import java.security.cert.CertificateNotYetValidException;
+import java.security.cert.X509Certificate;
+import java.util.Date;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Set;
+
+import javax.net.ssl.TrustManager;
+import javax.net.ssl.TrustManagerFactory;
+import javax.net.ssl.X509TrustManager;
+import javax.security.auth.x500.X500Principal;
+/**
+ * A wrapper around the original trust manager factory for creating common name logging trust managers.
+ * These trust managers log the common name of an expired but otherwise valid (client) certificate before rejecting the connection attempt.
+ * This allows to identify misconfigured clients in complex network environments, where the IP address is not sufficient.
+ */
+class CommonNameLoggingTrustManagerFactoryWrapper {
+
+ private static final Logger log = LoggerFactory.getLogger(DefaultSslEngineFactory.class);
+
+ private TrustManagerFactory origTmf;
+
+ /**
+ * Create a wrapped trust manager factory
+ * @param kmfAlgorithm the algorithm
+ * @return A wrapped trust manager factory
+ * @throws NoSuchAlgorithmException
+ */
+ protected CommonNameLoggingTrustManagerFactoryWrapper(String kmfAlgorithm) throws NoSuchAlgorithmException {
+ this.origTmf = TrustManagerFactory.getInstance(kmfAlgorithm);
+ }
+ /**
+ * Factory for creating a wrapped trust manager factory
+ * @param kmfAlgorithm the algorithm
+ * @return A wrapped trust manager factory
+ * @throws NoSuchAlgorithmException
+ */
+ public static CommonNameLoggingTrustManagerFactoryWrapper getInstance(String kmfAlgorithm) throws NoSuchAlgorithmException {
+ return new CommonNameLoggingTrustManagerFactoryWrapper(kmfAlgorithm);
+ }
+
+ public TrustManagerFactory getOriginalTrustManagerFactory() {
+ return this.origTmf;
+ }
+
+ public String getAlgorithm() {
+ return this.origTmf.getAlgorithm();
+ }
+
+ public void init(KeyStore ts) throws KeyStoreException {
+ this.origTmf.init(ts);
+ }
+
+ public TrustManager[] getTrustManagers() {
+ TrustManager[] origTrustManagers = this.origTmf.getTrustManagers();
+ TrustManager[] wrappedTrustManagers = new TrustManager[origTrustManagers.length];
+ for (int i = 0; i < origTrustManagers.length; i++) {
+ TrustManager tm = origTrustManagers[i];
+ if (tm instanceof X509TrustManager) {
+ // Wrap only X509 trust managers
+ wrappedTrustManagers[i] = new CommonNameLoggingTrustManager((X509TrustManager) tm, 2000);
+ } else {
+ wrappedTrustManagers[i] = tm;
+ }
+ }
+ return wrappedTrustManagers;
+ }
+ /**
+ * A trust manager which logs the common name of an expired but otherwise valid (client) certificate before rejecting the connection attempt.
+ * This allows to identify misconfigured clients in complex network environments, where the IP address is not sufficient.
+ * this class wraps a standard trust manager and delegates almost all requests to it, except for cases where an invalid certificate is reported by the
+ * standard trust manager. In this cases this manager checks whether the provided certificate is invalid only due to being expired and logs the common
+ * name if that is the case. This trust manager will always return the results of the wrapped standard trust manager, i.e. return if the certificate is valid
+ * or rethrow the original exception if it is not.
+ */
+ static class CommonNameLoggingTrustManager implements X509TrustManager {
+
+ final private X509TrustManager origTm;
+ final int nrOfRememberedBadCerts;
+ final private LinkedHashMap<String, String> previouslyRejectedClientCertChains;
+
+ public CommonNameLoggingTrustManager(X509TrustManager originalTrustManager, int nrOfRememberedBadCerts) {
+ this.origTm = originalTrustManager;
+ this.nrOfRememberedBadCerts = nrOfRememberedBadCerts;
+ // Restrict maximal size of the LinkedHashMap to avoid security attacks causing OOM
+ this.previouslyRejectedClientCertChains = new LinkedHashMap<String, String>() {
+ @Override
+ protected boolean removeEldestEntry(final Map.Entry<String, String> eldest) {
+ return size() > nrOfRememberedBadCerts;
+ }
+ };
+ }
+
+ public X509TrustManager getOriginalTrustManager() {
+ return this.origTm;
+ }
+
+ @Override
+ public void checkClientTrusted(X509Certificate[] chain, String authType)
+ throws CertificateException {
+ CertificateException origException = null;
+ String chainDigest = calcDigestForCertificateChain(chain);
+ if (chainDigest != null) {
+ String errorMessage = this.previouslyRejectedClientCertChains.get(chainDigest);
+ if (errorMessage != null) {
+ // Reinsert the digest, to remember that this is the most recent digest we've seen
+ addRejectedClientCertChains(chainDigest, errorMessage, true);
+ // Then throw with the original error Message
+ throw new CertificateException(errorMessage);
+ }
+ }
+ try {
+ this.origTm.checkClientTrusted(chain, authType);
+ // If the last line did not throw, the chain is valid (including that none of the certificates is expired)
+ } catch (CertificateException e) {
+ origException = e;
+ try {
+ X509Certificate[] wrappedChain = sortChainAnWrapEndCertificate(chain);
+ this.origTm.checkClientTrusted(wrappedChain, authType);
+ // No exception occurred this time. The certificate is either not yet valid or already expired
+ Date now = new Date();
+ // Check if the certificate was valid in the past
+ if (wrappedChain[0].getNotBefore().before(now)) {
+ String commonName = wrappedChain[0].getSubjectX500Principal().toString();
+ String notValidAfter = wrappedChain[0].getNotAfter().toString();
+ log.info("Certificate with common name \"" + commonName + "\" expired on " + notValidAfter);
Review Comment:
Have we considered returning this information - i.e. if the broker rejects a client certificate, we now log in the broker, but the client doesn't get this information?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org