ethaden commented on code in PR #14130: URL: https://github.com/apache/kafka/pull/14130#discussion_r1283754255
########## clients/src/main/java/org/apache/kafka/common/security/ssl/DefaultSslEngineFactory.java: ########## @@ -578,4 +594,335 @@ private List<byte[]> pemEntries(String pem) { return entries; } } + + /** + * 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. + */ + static class CommonNameLoggingTrustManagerFactoryWrapper { + + 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); + } 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 { + + private X509TrustManager origTm; + + public CommonNameLoggingTrustManager(X509TrustManager originalTrustManager) { + this.origTm = originalTrustManager; + } + + public X509TrustManager getOriginalTrustManager() { + return this.origTm; + } + + @Override + public void checkClientTrusted(X509Certificate[] chain, String authType) + throws CertificateException { + CertificateException origException = null; + 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 invalid due to being expired, but otherwise valid. + String commonName = wrappedChain[0].getSubjectX500Principal().toString(); + String notValidAfter = wrappedChain[0].getNotAfter().toString(); + log.info("Certificate with common name \"" + commonName + "\" expired on " + notValidAfter); Review Comment: I'm not sure whether or not the log message should have severity "WARN". I'd argue that the original (still existing) log message that reports the IP address from which the client tried to connect with the expired certificate has severity "INFO", too. Pragmatically, both options would work for the users, though (as "INFO" is considered the recommended log level for running brokers with). -- 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