This is an automated email from the ASF dual-hosted git repository. remm pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push: new 1f56910 Partially revert 590877b 1f56910 is described below commit 1f56910cf16b895857d3313f79ddd149774f5dc8 Author: remm <r...@apache.org> AuthorDate: Tue Nov 23 21:39:43 2021 +0100 Partially revert 590877b This causes a regression crash with the TestSSLHostConfigCompat test [which I had stopped running since the previous Tomcat release]. I don't understand the difference between the two (an implicit scope is a shared scope with a cleaner), but no reason to leave this in. --- .../util/net/openssl/panama/OpenSSLContext.java | 143 +++++++++++---------- 1 file changed, 76 insertions(+), 67 deletions(-) diff --git a/modules/openssl-java17/src/main/java/org/apache/tomcat/util/net/openssl/panama/OpenSSLContext.java b/modules/openssl-java17/src/main/java/org/apache/tomcat/util/net/openssl/panama/OpenSSLContext.java index 15b075a..69f1f71 100644 --- a/modules/openssl-java17/src/main/java/org/apache/tomcat/util/net/openssl/panama/OpenSSLContext.java +++ b/modules/openssl-java17/src/main/java/org/apache/tomcat/util/net/openssl/panama/OpenSSLContext.java @@ -30,6 +30,8 @@ import java.io.File; import java.lang.invoke.MethodHandle; import java.lang.invoke.MethodHandles; import java.lang.invoke.MethodType; +import java.lang.ref.Cleaner; +import java.lang.ref.Cleaner.Cleanable; import java.nio.charset.StandardCharsets; import java.security.PrivateKey; import java.security.SecureRandom; @@ -147,6 +149,8 @@ public class OpenSSLContext implements org.apache.tomcat.util.net.SSLContext { } } + private static final Cleaner cleaner = Cleaner.create(); + private final SSLHostConfig sslHostConfig; private final SSLHostConfigCertificate certificate; private final boolean alpn; @@ -167,7 +171,7 @@ public class OpenSSLContext implements org.apache.tomcat.util.net.SSLContext { } private final ContextState state; - private final ResourceScope scope; + private final Cleanable cleanable; private static String[] getCiphers(MemoryAddress sslCtx) { MemoryAddress sk = SSL_CTX_get_ciphers(sslCtx); @@ -198,7 +202,7 @@ public class OpenSSLContext implements org.apache.tomcat.util.net.SSLContext { this.sslHostConfig = certificate.getSSLHostConfig(); this.certificate = certificate; - scope = ResourceScope.newImplicitScope(); + ResourceScope scope = ResourceScope.newSharedScope(); MemoryAddress sslCtx = MemoryAddress.NULL; MemoryAddress confCtx = MemoryAddress.NULL; @@ -335,7 +339,7 @@ public class OpenSSLContext implements org.apache.tomcat.util.net.SSLContext { } catch(Exception e) { throw new SSLException(sm.getString("openssl.errorSSLCtxInit"), e); } finally { - state = new ContextState(sslCtx, confCtx, negotiableProtocolsBytes); + state = new ContextState(scope, sslCtx, confCtx, negotiableProtocolsBytes); /* * When an SSLHostConfig is replaced at runtime, it is not possible to * call destroy() on the associated OpenSSLContext since it is likely @@ -344,11 +348,11 @@ public class OpenSSLContext implements org.apache.tomcat.util.net.SSLContext { * OpenSSLSessionContext) to ensure that the OpenSSLContext remains * ineligible for GC while those connections are alive. Once those * connections complete, the OpenSSLContext will become eligible for GC - * and the implicit scope will ensure that the associated native - * resources are cleaned up. + * and this method will ensure that the associated native resources are + * cleaned up. */ states.put(Long.valueOf(sslCtx.address().toRawLongValue()), state); - scope.addCloseAction(state); + cleanable = cleaner.register(this, state); if (!success) { destroy(); @@ -370,6 +374,7 @@ public class OpenSSLContext implements org.apache.tomcat.util.net.SSLContext { @Override public synchronized void destroy() { states.remove(Long.valueOf(state.sslCtx.address().toRawLongValue())); + cleanable.clean(); } @@ -556,7 +561,7 @@ public class OpenSSLContext implements org.apache.tomcat.util.net.SSLContext { } // List the ciphers that the client is permitted to negotiate - if (SSL_CTX_set_cipher_list(state.sslCtx, CLinker.toCString(sslHostConfig.getCiphers(), scope)) <= 0) { + if (SSL_CTX_set_cipher_list(state.sslCtx, CLinker.toCString(sslHostConfig.getCiphers(), state.scope)) <= 0) { log.warn(sm.getString("engine.failedCipherSuite", sslHostConfig.getCiphers())); } @@ -592,67 +597,65 @@ public class OpenSSLContext implements org.apache.tomcat.util.net.SSLContext { // Set int verify_callback(int preverify_ok, X509_STORE_CTX *x509_ctx) callback MemoryAddress openSSLCallbackVerify = CLinker.getInstance().upcallStub(openSSLCallbackVerifyHandle, - openSSLCallbackVerifyFunctionDescriptor, scope); + openSSLCallbackVerifyFunctionDescriptor, state.scope); // Leave this just in case but in Tomcat this is always set again by the engine SSL_CTX_set_verify(state.sslCtx, value, openSSLCallbackVerify); // Trust and certificate verification - try (var scope = ResourceScope.newConfinedScope()) { - var allocator = SegmentAllocator.ofScope(scope); - if (tms != null) { - // Client certificate verification based on custom trust managers - state.x509TrustManager = chooseTrustManager(tms); - MemoryAddress openSSLCallbackCertVerify = - CLinker.getInstance().upcallStub(openSSLCallbackCertVerifyHandle, - openSSLCallbackCertVerifyFunctionDescriptor, this.scope); - SSL_CTX_set_cert_verify_callback(state.sslCtx, openSSLCallbackCertVerify, state.sslCtx); - - // Pass along the DER encoded certificates of the accepted client - // certificate issuers, so that their subjects can be presented - // by the server during the handshake to allow the client choosing - // an acceptable certificate - for (X509Certificate caCert : state.x509TrustManager.getAcceptedIssuers()) { - //SSLContext.addClientCACertificateRaw(state.ctx, caCert.getEncoded()); - var rawCACertificate = allocator.allocateArray(CLinker.C_CHAR, caCert.getEncoded()); - var rawCACertificatePointer = allocator.allocate(CLinker.C_POINTER, rawCACertificate); - var x509CACert = d2i_X509(MemoryAddress.NULL, rawCACertificatePointer, rawCACertificate.byteSize()); - if (MemoryAddress.NULL.equals(x509CACert)) { - logLastError(allocator, "openssl.errorLoadingCertificate"); - } else if (SSL_CTX_add_client_CA(state.sslCtx, x509CACert) <= 0) { - logLastError(allocator, "openssl.errorAddingCertificate"); - } else if (log.isDebugEnabled()) { - log.debug(sm.getString("openssl.addedClientCaCert", caCert.toString())); - } + var allocator = SegmentAllocator.ofScope(state.scope); + if (tms != null) { + // Client certificate verification based on custom trust managers + state.x509TrustManager = chooseTrustManager(tms); + MemoryAddress openSSLCallbackCertVerify = + CLinker.getInstance().upcallStub(openSSLCallbackCertVerifyHandle, + openSSLCallbackCertVerifyFunctionDescriptor, state.scope); + SSL_CTX_set_cert_verify_callback(state.sslCtx, openSSLCallbackCertVerify, state.sslCtx); + + // Pass along the DER encoded certificates of the accepted client + // certificate issuers, so that their subjects can be presented + // by the server during the handshake to allow the client choosing + // an acceptable certificate + for (X509Certificate caCert : state.x509TrustManager.getAcceptedIssuers()) { + //SSLContext.addClientCACertificateRaw(state.ctx, caCert.getEncoded()); + var rawCACertificate = allocator.allocateArray(CLinker.C_CHAR, caCert.getEncoded()); + var rawCACertificatePointer = allocator.allocate(CLinker.C_POINTER, rawCACertificate); + var x509CACert = d2i_X509(MemoryAddress.NULL, rawCACertificatePointer, rawCACertificate.byteSize()); + if (MemoryAddress.NULL.equals(x509CACert)) { + logLastError(allocator, "openssl.errorLoadingCertificate"); + } else if (SSL_CTX_add_client_CA(state.sslCtx, x509CACert) <= 0) { + logLastError(allocator, "openssl.errorAddingCertificate"); + } else if (log.isDebugEnabled()) { + log.debug(sm.getString("openssl.addedClientCaCert", caCert.toString())); } - } else if (sslHostConfig.getCaCertificateFile() != null || sslHostConfig.getCaCertificatePath() != null) { - // Client certificate verification based on trusted CA files and dirs - //SSLContext.setCACertificate(state.ctx, - // SSLHostConfig.adjustRelativePath(sslHostConfig.getCaCertificateFile()), - // SSLHostConfig.adjustRelativePath(sslHostConfig.getCaCertificatePath())); - MemorySegment caCertificateFileNative = sslHostConfig.getCaCertificateFile() != null - ? CLinker.toCString(SSLHostConfig.adjustRelativePath(sslHostConfig.getCaCertificateFile()), scope) : null; - MemorySegment caCertificatePathNative = sslHostConfig.getCaCertificatePath() != null - ? CLinker.toCString(SSLHostConfig.adjustRelativePath(sslHostConfig.getCaCertificatePath()), scope) : null; - if (SSL_CTX_load_verify_locations(state.sslCtx, - caCertificateFileNative == null ? MemoryAddress.NULL : caCertificateFileNative, - caCertificatePathNative == null ? MemoryAddress.NULL : caCertificatePathNative) <= 0) { - logLastError(allocator, "openssl.errorConfiguringLocations"); - } else { - var caCerts = SSL_CTX_get_client_CA_list(state.sslCtx); - if (MemoryAddress.NULL.equals(caCerts)) { - caCerts = SSL_load_client_CA_file(caCertificateFileNative); - if (!MemoryAddress.NULL.equals(caCerts)) { - SSL_CTX_set_client_CA_list(state.sslCtx, caCerts); - } - } else { - if (SSL_add_file_cert_subjects_to_stack(caCerts, caCertificateFileNative) <= 0) { - caCerts = MemoryAddress.NULL; - } + } + } else if (sslHostConfig.getCaCertificateFile() != null || sslHostConfig.getCaCertificatePath() != null) { + // Client certificate verification based on trusted CA files and dirs + //SSLContext.setCACertificate(state.ctx, + // SSLHostConfig.adjustRelativePath(sslHostConfig.getCaCertificateFile()), + // SSLHostConfig.adjustRelativePath(sslHostConfig.getCaCertificatePath())); + MemorySegment caCertificateFileNative = sslHostConfig.getCaCertificateFile() != null + ? CLinker.toCString(SSLHostConfig.adjustRelativePath(sslHostConfig.getCaCertificateFile()), state.scope) : null; + MemorySegment caCertificatePathNative = sslHostConfig.getCaCertificatePath() != null + ? CLinker.toCString(SSLHostConfig.adjustRelativePath(sslHostConfig.getCaCertificatePath()), state.scope) : null; + if (SSL_CTX_load_verify_locations(state.sslCtx, + caCertificateFileNative == null ? MemoryAddress.NULL : caCertificateFileNative, + caCertificatePathNative == null ? MemoryAddress.NULL : caCertificatePathNative) <= 0) { + logLastError(allocator, "openssl.errorConfiguringLocations"); + } else { + var caCerts = SSL_CTX_get_client_CA_list(state.sslCtx); + if (MemoryAddress.NULL.equals(caCerts)) { + caCerts = SSL_load_client_CA_file(caCertificateFileNative); + if (!MemoryAddress.NULL.equals(caCerts)) { + SSL_CTX_set_client_CA_list(state.sslCtx, caCerts); } - if (MemoryAddress.NULL.equals(caCerts)) { - log.warn(sm.getString("openssl.noCACerts")); + } else { + if (SSL_add_file_cert_subjects_to_stack(caCerts, caCertificateFileNative) <= 0) { + caCerts = MemoryAddress.NULL; } } + if (MemoryAddress.NULL.equals(caCerts)) { + log.warn(sm.getString("openssl.noCACerts")); + } } } @@ -661,7 +664,7 @@ public class OpenSSLContext implements org.apache.tomcat.util.net.SSLContext { // MemoryAddress in, int inlen, MemoryAddress arg MemoryAddress openSSLCallbackAlpnSelectProto = CLinker.getInstance().upcallStub(openSSLCallbackAlpnSelectProtoHandle, - openSSLCallbackAlpnSelectProtoFunctionDescriptor, scope); + openSSLCallbackAlpnSelectProtoFunctionDescriptor, state.scope); SSL_CTX_set_alpn_select_cb(state.sslCtx, openSSLCallbackAlpnSelectProto, state.sslCtx); // Skip NPN (annoying and likely not useful anymore) //SSLContext.setNpnProtos(state.ctx, protocolsArray, SSL.SSL_SELECTOR_FAILURE_NO_ADVERTISE); @@ -1125,7 +1128,7 @@ public class OpenSSLContext implements org.apache.tomcat.util.net.SSLContext { } // Set callback for DH parameters MemoryAddress openSSLCallbackTmpDH = CLinker.getInstance().upcallStub(openSSLCallbackTmpDHHandle, - openSSLCallbackTmpDHFunctionDescriptor, scope); + openSSLCallbackTmpDHFunctionDescriptor, state.scope); SSL_CTX_set_tmp_dh_callback(state.sslCtx, openSSLCallbackTmpDH); // Set certificate chain file if (certificate.getCertificateChainFile() != null) { @@ -1213,7 +1216,7 @@ public class OpenSSLContext implements org.apache.tomcat.util.net.SSLContext { } // Set callback for DH parameters MemoryAddress openSSLCallbackTmpDH = CLinker.getInstance().upcallStub(openSSLCallbackTmpDHHandle, - openSSLCallbackTmpDHFunctionDescriptor, scope); + openSSLCallbackTmpDHFunctionDescriptor, state.scope); SSL_CTX_set_tmp_dh_callback(state.sslCtx, openSSLCallbackTmpDH); for (int i = 1; i < chain.length; i++) { //SSLContext.addChainCertificateRaw(state.ctx, chain[i].getEncoded()); @@ -1359,13 +1362,15 @@ public class OpenSSLContext implements org.apache.tomcat.util.net.SSLContext { private static class ContextState implements Runnable { + final ResourceScope scope; private final MemoryAddress sslCtx; private final MemoryAddress confCtx; private final List<byte[]> negotiableProtocols; private X509TrustManager x509TrustManager = null; - private ContextState(MemoryAddress sslCtx, MemoryAddress confCtx, List<byte[]> negotiableProtocols) { + private ContextState(ResourceScope scope, MemoryAddress sslCtx, MemoryAddress confCtx, List<byte[]> negotiableProtocols) { + this.scope = scope; this.sslCtx = sslCtx; this.confCtx = confCtx; this.negotiableProtocols = negotiableProtocols; @@ -1373,9 +1378,13 @@ public class OpenSSLContext implements org.apache.tomcat.util.net.SSLContext { @Override public void run() { - SSL_CTX_free(sslCtx); - if (!MemoryAddress.NULL.equals(confCtx)) { - SSL_CONF_CTX_free(confCtx); + try { + SSL_CTX_free(sslCtx); + if (!MemoryAddress.NULL.equals(confCtx)) { + SSL_CONF_CTX_free(confCtx); + } + } finally { + scope.close(); } } } --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org