Copilot commented on code in PR #4075: URL: https://github.com/apache/logging-log4j2/pull/4075#discussion_r2983757068
########## log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/TlsSocketAppenderTest.java: ########## @@ -0,0 +1,322 @@ +/* + * 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.logging.log4j.core.appender; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.io.OutputStream; +import java.net.Socket; +import java.net.URL; +import java.nio.file.Files; +import java.nio.file.Path; +import java.security.KeyPair; +import java.security.KeyStore; +import java.security.PrivateKey; +import java.security.cert.X509Certificate; +import java.util.stream.Stream; +import javax.net.ssl.SSLContext; +import javax.net.ssl.SSLServerSocket; +import org.apache.logging.log4j.Logger; +import org.apache.logging.log4j.core.LoggerContext; +import org.apache.logging.log4j.core.net.SslSocketManager; +import org.apache.logging.log4j.test.TestProperties; +import org.apache.logging.log4j.test.junit.UsingTestProperties; +import org.jspecify.annotations.Nullable; +import org.junit.jupiter.api.io.TempDir; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +@UsingTestProperties +class TlsSocketAppenderTest { + + // Test DNS names and IP addresses + private static final String TARGET_HOSTNAME = "log4j.localhost"; + private static final String TARGET_IP = "::1"; + private static final String ATTACKER_HOSTNAME = "not-log4j.localhost"; + private static final String ATTACKER_IP = "127.0.0.1"; + Review Comment: This test uses IPv6 loopback "::1" as an alias/identifier that is later used to build file names (e.g., keystore/truststore paths). On Windows, ':' is not a valid character in file names, so these tests will fail. Consider sanitizing hostnames when deriving file names (e.g., replace ':' with '_' or hash the identifier) while still connecting to the intended host. ########## log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/TlsSocketAppenderTest.java: ########## @@ -0,0 +1,322 @@ +/* + * 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.logging.log4j.core.appender; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.io.OutputStream; +import java.net.Socket; +import java.net.URL; +import java.nio.file.Files; +import java.nio.file.Path; +import java.security.KeyPair; +import java.security.KeyStore; +import java.security.PrivateKey; +import java.security.cert.X509Certificate; +import java.util.stream.Stream; +import javax.net.ssl.SSLContext; +import javax.net.ssl.SSLServerSocket; +import org.apache.logging.log4j.Logger; +import org.apache.logging.log4j.core.LoggerContext; +import org.apache.logging.log4j.core.net.SslSocketManager; +import org.apache.logging.log4j.test.TestProperties; +import org.apache.logging.log4j.test.junit.UsingTestProperties; +import org.jspecify.annotations.Nullable; +import org.junit.jupiter.api.io.TempDir; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +@UsingTestProperties +class TlsSocketAppenderTest { + + // Test DNS names and IP addresses + private static final String TARGET_HOSTNAME = "log4j.localhost"; + private static final String TARGET_IP = "::1"; + private static final String ATTACKER_HOSTNAME = "not-log4j.localhost"; + private static final String ATTACKER_IP = "127.0.0.1"; + Review Comment: The test hostnames "log4j.localhost" and "not-log4j.localhost" are not guaranteed to resolve in CI environments (only "localhost" is special-cased broadly). This makes the TLS tests environment-dependent and potentially flaky. Consider using universally-resolvable loopback targets (e.g., "localhost", "127.0.0.1", "::1") and decouple the certificate’s SAN/CN values from the connection host to test hostname mismatch cases reliably. ########## log4j-core/src/main/java/org/apache/logging/log4j/core/net/ssl/SslConfiguration.java: ########## @@ -178,15 +178,14 @@ private static TrustManager[] loadTrustManagers(@Nullable final TrustStoreConfig * @param keyStoreConfig The KeyStoreConfiguration. * @param trustStoreConfig The TrustStoreConfiguration. * @return a new SslConfiguration + * @deprecated Since 2.26.0, use {@link #createSSLConfiguration(String, KeyStoreConfiguration, TrustStoreConfiguration, boolean)} instead. */ + @Deprecated @NullUnmarked - @PluginFactory public static SslConfiguration createSSLConfiguration( Review Comment: The new deprecation Javadoc says “Since 2.26.0”, but the project revision is currently 2.25.4-SNAPSHOT. Please update the deprecation version to the release where this change actually lands (or omit the version) to avoid misleading API docs. ########## log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/X509Certificates.java: ########## @@ -0,0 +1,193 @@ +/* + * 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.logging.log4j.core.appender; + +import java.math.BigInteger; +import java.security.KeyPair; +import java.security.KeyPairGenerator; +import java.security.PrivateKey; +import java.security.PublicKey; +import java.security.cert.CertificateException; +import java.security.cert.X509Certificate; +import java.util.ArrayList; +import java.util.Date; +import java.util.List; +import java.util.Random; +import org.bouncycastle.asn1.x500.X500Name; +import org.bouncycastle.asn1.x509.BasicConstraints; +import org.bouncycastle.asn1.x509.ExtendedKeyUsage; +import org.bouncycastle.asn1.x509.Extension; +import org.bouncycastle.asn1.x509.GeneralName; +import org.bouncycastle.asn1.x509.GeneralNames; +import org.bouncycastle.asn1.x509.KeyPurposeId; +import org.bouncycastle.asn1.x509.KeyUsage; +import org.bouncycastle.cert.CertIOException; +import org.bouncycastle.cert.X509CertificateHolder; +import org.bouncycastle.cert.jcajce.JcaX509CertificateConverter; +import org.bouncycastle.cert.jcajce.JcaX509v3CertificateBuilder; +import org.bouncycastle.operator.ContentSigner; +import org.bouncycastle.operator.OperatorCreationException; +import org.bouncycastle.operator.jcajce.JcaContentSignerBuilder; +import org.jspecify.annotations.Nullable; + +/** + * Utility class to generate X.509 certificates for testing purposes. + */ +final class X509Certificates { + + private static final String CA_DN = "CN=Test CA"; + private static final long MINUTE_IN_MILLIS = 60_000L; + private static final long YEAR_IN_MILLIS = 365L * 24 * 60 * MINUTE_IN_MILLIS; + + private static final KeyPairGenerator RSA_GENERATOR; + private static final Random RANDOM = new Random(); + + static { + try { + RSA_GENERATOR = KeyPairGenerator.getInstance("RSA"); + RSA_GENERATOR.initialize(2048); + } catch (Exception e) { + throw new RuntimeException(e); + } + } + + static KeyPair generateKeyPair() { + return RSA_GENERATOR.generateKeyPair(); + } + + static X509Certificate generateCACertificate(KeyPair keyPair) throws Exception { + JcaX509v3CertificateBuilder builder = getCertificateBuilder(keyPair.getPublic(), CA_DN, true); + addKeyUsageExtension(builder, KeyUsage.keyCertSign); + return buildCertificate(builder, keyPair.getPrivate()); + } + + /** + * Create and sign a server X.509 certificate for tests. + * + * <p>The produced certificate complies with {@code sun.security.validator.EndEntityChecker}.</p> + * + * @param keyPair the subject key pair + * @param caKey the private key of the issuing CA used to sign the certificate + * @param subjectDn the subject distinguished name for the certificate (for example {@code CN=example.com}) + * @param dnsAltSubject optional DNS Subject Alternative Name; pass {@code null} to omit + * @param ipAltSubject optional IP Subject Alternative Name; pass {@code null} to omit + * @return a signed X.509 server certificate + * @throws Exception if certificate creation or signing fails + */ + static X509Certificate generateServerCertificate( + KeyPair keyPair, + PrivateKey caKey, + String subjectDn, + @Nullable String dnsAltSubject, + @Nullable String ipAltSubject) + throws Exception { + JcaX509v3CertificateBuilder builder = getCertificateBuilder(keyPair.getPublic(), subjectDn, false); + // The required key usage for the server certificate depends on the key exchange algorithm: + // - keyEncipherment for RSA key exchange (deprecated) + // - digitalSignature for ephemeral Diffie-Hellman key exchange (DHE or ECDHE) + // - keyAgreement for static Diffie-Hellman key exchange (DH or ECDH) + addKeyUsageExtension(builder, KeyUsage.digitalSignature | KeyUsage.keyAgreement); + addExtendedKeyUsageExtension(builder, KeyPurposeId.id_kp_serverAuth); + addSubjectAlternativeName(builder, dnsAltSubject, ipAltSubject); + return buildCertificate(builder, caKey); + } + + /** + * Create and sign a client X.509 certificate for tests. + * + * <p>The produced certificate complies with {@code sun.security.validator.EndEntityChecker}.</p> + * + * @param keyPair the subject key pair + * @param caKey the private key of the issuing CA used to sign the certificate + * @param subjectDn the subject distinguished name for the certificate (for example {@code CN=example.com}) + * @return a signed X.509 server certificate + * @throws Exception if certificate creation or signing fails + */ + static X509Certificate generateClientCertificate(KeyPair keyPair, PrivateKey caKey, String subjectDn) + throws Exception { + JcaX509v3CertificateBuilder builder = getCertificateBuilder(keyPair.getPublic(), subjectDn, false); + // The required key usage for the client certificate + addKeyUsageExtension(builder, KeyUsage.digitalSignature); + addExtendedKeyUsageExtension(builder, KeyPurposeId.id_kp_clientAuth); + return buildCertificate(builder, caKey); + } + + private static JcaX509v3CertificateBuilder getCertificateBuilder( + PublicKey subjectPub, String subjectDn, boolean isCa) throws CertIOException { + long now = System.currentTimeMillis(); + Date notBefore = new Date(now - MINUTE_IN_MILLIS); + Date notAfter = new Date(now + YEAR_IN_MILLIS); + BigInteger serial = BigInteger.valueOf(RANDOM.nextLong()); + + X500Name issuer = new X500Name(CA_DN); + X500Name subject = new X500Name(subjectDn); + + JcaX509v3CertificateBuilder builder = + new JcaX509v3CertificateBuilder(issuer, serial, notBefore, notAfter, subject, subjectPub); + Review Comment: The certificate serial number is derived from Random.nextLong(), which can produce negative values. X.509 serial numbers are expected to be positive (and some validators reject negative serials), so this can make the tests intermittently fail depending on the generated value. Generate a positive serial (e.g., using a bounded positive BigInteger) to keep the test certificates consistently valid. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
