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 4648db89a2 Cleanups 4648db89a2 is described below commit 4648db89a26805ef1ad5212214b7c50d3165c93c Author: remm <r...@apache.org> AuthorDate: Wed Dec 13 11:52:53 2023 +0100 Cleanups Use the log loop when logging errors in the context. Make some methods private. Remove duplicate code. renegotiate does not need sync since it is private and the only caller is synced. --- .../util/net/openssl/panama/OpenSSLContext.java | 43 +++--- .../util/net/openssl/panama/OpenSSLEngine.java | 164 ++++++++++----------- .../tomcat/util/openssl/openssl_h_Macros.java | 5 - 3 files changed, 102 insertions(+), 110 deletions(-) diff --git a/java/org/apache/tomcat/util/net/openssl/panama/OpenSSLContext.java b/java/org/apache/tomcat/util/net/openssl/panama/OpenSSLContext.java index 729635a942..a5aa2ea8a0 100644 --- a/java/org/apache/tomcat/util/net/openssl/panama/OpenSSLContext.java +++ b/java/org/apache/tomcat/util/net/openssl/panama/OpenSSLContext.java @@ -100,6 +100,8 @@ public class OpenSSLContext implements org.apache.tomcat.util.net.SSLContext { public static final int SSL_PROTOCOL_ALL = (SSL_PROTOCOL_TLSV1 | SSL_PROTOCOL_TLSV1_1 | SSL_PROTOCOL_TLSV1_2 | SSL_PROTOCOL_TLSV1_3); + static final int OPTIONAL_NO_CA = 3; + private static final String BEGIN_KEY = "-----BEGIN PRIVATE KEY-----\n"; private static final Object END_KEY = "\n-----END PRIVATE KEY-----"; @@ -136,8 +138,7 @@ public class OpenSSLContext implements org.apache.tomcat.util.net.SSLContext { private final MemorySegment openSSLCallbackPassword; private static final ConcurrentHashMap<Long, ContextState> states = new ConcurrentHashMap<>(); - - static ContextState getState(MemorySegment ctx) { + private static ContextState getState(MemorySegment ctx) { return states.get(Long.valueOf(ctx.address())); } @@ -477,8 +478,6 @@ public class OpenSSLContext implements org.apache.tomcat.util.net.SSLContext { return result; } - private static final int OPTIONAL_NO_CA = 3; - /** * Setup the SSL_CTX. * @@ -564,9 +563,8 @@ public class OpenSSLContext implements org.apache.tomcat.util.net.SSLContext { } // Set int verify_callback(int preverify_ok, X509_STORE_CTX *x509_ctx) callback - // Leave this just in case but in Tomcat this is always set again by the engine SSL_CTX_set_verify(state.sslCtx, value, - SSL_CTX_set_verify$callback.allocate(new VerifyCallback(), contextArena)); + SSL_CTX_set_verify$callback.allocate(new OpenSSLEngine.VerifyCallback(), contextArena)); // Trust and certificate verification if (tms != null) { @@ -778,14 +776,6 @@ public class OpenSSLContext implements org.apache.tomcat.util.net.SSLContext { } - private static class VerifyCallback implements SSL_CTX_set_verify$callback { - @Override - public int apply(int preverify_ok, MemorySegment /*X509_STORE_CTX*/ x509ctx) { - return OpenSSLEngine.openSSLCallbackVerify(preverify_ok, x509ctx); - } - } - - private static class CertVerifyCallback implements SSL_CTX_set_cert_verify_callback$cb { @Override public int apply(MemorySegment /*X509_STORE_CTX*/ x509_ctx, MemorySegment param) { @@ -942,7 +932,6 @@ public class OpenSSLContext implements org.apache.tomcat.util.net.SSLContext { } - @SuppressWarnings("deprecation") private boolean addCertificate(SSLHostConfigCertificate certificate, Arena localArena) throws Exception { int index = getCertificateIndex(certificate); // Load Server key and certificate @@ -1375,10 +1364,26 @@ public class OpenSSLContext implements org.apache.tomcat.util.net.SSLContext { private static void logLastError(SegmentAllocator allocator, String string) { - var buf = allocator.allocate(ValueLayout.JAVA_BYTE, 128); - ERR_error_string(ERR_get_error(), buf); - String err = buf.getString(0); - log.error(sm.getString(string, err)); + String sslError = null; + long error = ERR_get_error(); + if (error != SSL_ERROR_NONE()) { + do { + // Loop until getLastErrorNumber() returns SSL_ERROR_NONE + var buf = allocator.allocate(ValueLayout.JAVA_BYTE, 128); + ERR_error_string(error, buf); + String err = buf.getString(0); + if (sslError == null) { + sslError = err; + } + if (log.isDebugEnabled()) { + log.debug(sm.getString("engine.openSSLError", Long.toString(error), err)); + } + } while ((error = ERR_get_error()) != SSL_ERROR_NONE()); + } + if (sslError == null) { + sslError = ""; + } + log.error(sm.getString(string, sslError)); } diff --git a/java/org/apache/tomcat/util/net/openssl/panama/OpenSSLEngine.java b/java/org/apache/tomcat/util/net/openssl/panama/OpenSSLEngine.java index f02d0b77ea..9f6593cc42 100644 --- a/java/org/apache/tomcat/util/net/openssl/panama/OpenSSLEngine.java +++ b/java/org/apache/tomcat/util/net/openssl/panama/OpenSSLEngine.java @@ -62,6 +62,7 @@ import org.apache.tomcat.util.buf.Asn1Parser; import org.apache.tomcat.util.net.Constants; import org.apache.tomcat.util.net.SSLUtil; import org.apache.tomcat.util.net.openssl.ciphers.OpenSSLCipherConfigurationParser; +import org.apache.tomcat.util.openssl.SSL_CTX_set_verify$callback; import org.apache.tomcat.util.openssl.SSL_set_info_callback$cb; import org.apache.tomcat.util.openssl.SSL_set_verify$callback; import org.apache.tomcat.util.res.StringManager; @@ -101,15 +102,10 @@ public final class OpenSSLEngine extends SSLEngine implements SSLUtil.ProtocolIn private static final int MAX_COMPRESSED_LENGTH = MAX_PLAINTEXT_LENGTH + 1024; private static final int MAX_CIPHERTEXT_LENGTH = MAX_COMPRESSED_LENGTH + 1024; - // Protocols - static final int VERIFY_DEPTH = 10; - // Header (5) + Data (2^14) + Compression (1024) + Encryption (1024) + MAC (20) + Padding (256) - static final int MAX_ENCRYPTED_PACKET_LENGTH = MAX_CIPHERTEXT_LENGTH + 5 + 20 + 256; - - static final int MAX_ENCRYPTION_OVERHEAD_LENGTH = MAX_ENCRYPTED_PACKET_LENGTH - MAX_PLAINTEXT_LENGTH; + private static final int MAX_ENCRYPTED_PACKET_LENGTH = MAX_CIPHERTEXT_LENGTH + 5 + 20 + 256; - enum ClientAuthMode { + private enum ClientAuthMode { NONE, OPTIONAL, REQUIRE, @@ -924,7 +920,7 @@ public final class OpenSSLEngine extends SSLEngine implements SSLUtil.ProtocolIn } } - private synchronized void renegotiate() throws SSLException { + private void renegotiate() throws SSLException { if (log.isDebugEnabled()) { log.debug("Start renegotiate"); } @@ -964,7 +960,7 @@ public final class OpenSSLEngine extends SSLEngine implements SSLUtil.ProtocolIn /** * Clear out any errors, but log a warning. */ - private void clearLastError() { + private static void clearLastError() { getLastError(); } @@ -977,7 +973,7 @@ public final class OpenSSLEngine extends SSLEngine implements SSLUtil.ProtocolIn * zero result. * @return the first error in the stack */ - private String getLastError() { + private static String getLastError() { String sslError = null; long error = ERR_get_error(); if (error != SSL_ERROR_NONE()) { @@ -1118,8 +1114,6 @@ public final class OpenSSLEngine extends SSLEngine implements SSLUtil.ProtocolIn return clientAuth == ClientAuthMode.OPTIONAL; } - private static final int OPTIONAL_NO_CA = 3; - private void setClientAuth(ClientAuthMode mode) { if (clientMode) { return; @@ -1131,7 +1125,7 @@ public final class OpenSSLEngine extends SSLEngine implements SSLUtil.ProtocolIn state.certificateVerifyMode = switch (mode) { case NONE -> SSL_VERIFY_NONE(); case REQUIRE -> SSL_VERIFY_FAIL_IF_NO_PEER_CERT(); - case OPTIONAL -> certificateVerificationOptionalNoCA ? OPTIONAL_NO_CA : SSL_VERIFY_PEER(); + case OPTIONAL -> certificateVerificationOptionalNoCA ? OpenSSLContext.OPTIONAL_NO_CA : SSL_VERIFY_PEER(); }; // SSL.setVerify(state.ssl, value, certificateVerificationDepth); // Set int verify_callback(int preverify_ok, X509_STORE_CTX *x509_ctx) callback @@ -1140,6 +1134,8 @@ public final class OpenSSLEngine extends SSLEngine implements SSLUtil.ProtocolIn case REQUIRE -> SSL_VERIFY_PEER() | SSL_VERIFY_FAIL_IF_NO_PEER_CERT(); case OPTIONAL -> SSL_VERIFY_PEER(); }; + // Note: Since a callback is always set by the context, the callback here could in theory + // be set to NULL (at the time of creation of the SSL, the SSL_CTX will have a non null callback) SSL_set_verify(state.ssl, value, SSL_set_verify$callback.allocate(new VerifyCallback(), engineArena)); clientAuth = mode; } @@ -1159,94 +1155,90 @@ public final class OpenSSLEngine extends SSLEngine implements SSLUtil.ProtocolIn } } - private static class VerifyCallback implements SSL_set_verify$callback { + static class VerifyCallback implements SSL_set_verify$callback, SSL_CTX_set_verify$callback { @Override public int apply(int preverify_ok, MemorySegment /*X509_STORE_CTX*/ x509ctx) { - return openSSLCallbackVerify(preverify_ok, x509ctx); - } - } - - public static int openSSLCallbackVerify(int preverify_ok, MemorySegment /*X509_STORE_CTX*/ x509ctx) { - MemorySegment ssl = X509_STORE_CTX_get_ex_data(x509ctx, SSL_get_ex_data_X509_STORE_CTX_idx()); - EngineState state = getState(ssl); - if (state == null) { - log.warn(sm.getString("engine.noSSL", Long.valueOf(ssl.address()))); - return 0; - } - if (log.isDebugEnabled()) { - log.debug("Verification in engine with mode [" + state.certificateVerifyMode + "] for " + state.ssl); - } - int ok = preverify_ok; - int errnum = X509_STORE_CTX_get_error(x509ctx); - int errdepth = X509_STORE_CTX_get_error_depth(x509ctx); - state.phaState = PHAState.COMPLETE; - if (state.certificateVerifyMode == -1 /*SSL_CVERIFY_UNSET*/ || state.certificateVerifyMode == SSL_VERIFY_NONE()) { - return 1; - } - /*SSL_VERIFY_ERROR_IS_OPTIONAL(errnum) -> ((errnum == X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT) + MemorySegment ssl = X509_STORE_CTX_get_ex_data(x509ctx, SSL_get_ex_data_X509_STORE_CTX_idx()); + EngineState state = getState(ssl); + if (state == null) { + log.warn(sm.getString("engine.noSSL", Long.valueOf(ssl.address()))); + return 0; + } + if (log.isDebugEnabled()) { + log.debug("Verification in engine with mode [" + state.certificateVerifyMode + "] for " + state.ssl); + } + int ok = preverify_ok; + int errnum = X509_STORE_CTX_get_error(x509ctx); + int errdepth = X509_STORE_CTX_get_error_depth(x509ctx); + state.phaState = PHAState.COMPLETE; + if (state.certificateVerifyMode == -1 /*SSL_CVERIFY_UNSET*/ || state.certificateVerifyMode == SSL_VERIFY_NONE()) { + return 1; + } + /*SSL_VERIFY_ERROR_IS_OPTIONAL(errnum) -> ((errnum == X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT) || (errnum == X509_V_ERR_SELF_SIGNED_CERT_IN_CHAIN) || (errnum == X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY) || (errnum == X509_V_ERR_CERT_UNTRUSTED) || (errnum == X509_V_ERR_UNABLE_TO_VERIFY_LEAF_SIGNATURE))*/ - boolean verifyErrorIsOptional = (errnum == X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT()) - || (errnum == X509_V_ERR_SELF_SIGNED_CERT_IN_CHAIN()) - || (errnum == X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY()) - || (errnum == X509_V_ERR_CERT_UNTRUSTED()) - || (errnum == X509_V_ERR_UNABLE_TO_VERIFY_LEAF_SIGNATURE()); - if (verifyErrorIsOptional && (state.certificateVerifyMode == OPTIONAL_NO_CA)) { - ok = 1; - SSL_set_verify_result(state.ssl, X509_V_OK()); - } - /* - * Expired certificates vs. "expired" CRLs: by default, OpenSSL - * turns X509_V_ERR_CRL_HAS_EXPIRED into a "certificate_expired(45)" - * SSL alert, but that's not really the message we should convey to the - * peer (at the very least, it's confusing, and in many cases, it's also - * inaccurate, as the certificate itself may very well not have expired - * yet). We set the X509_STORE_CTX error to something which OpenSSL's - * s3_both.c:ssl_verify_alarm_type() maps to SSL_AD_CERTIFICATE_UNKNOWN, - * i.e. the peer will receive a "certificate_unknown(46)" alert. - * We do not touch errnum, though, so that later on we will still log - * the "real" error, as returned by OpenSSL. - */ - if (ok == 0 && errnum == X509_V_ERR_CRL_HAS_EXPIRED()) { - X509_STORE_CTX_set_error(x509ctx, -1); - } - - // OCSP - if (!state.noOcspCheck && (ok > 0)) { - /* If there was an optional verification error, it's not - * possible to perform OCSP validation since the issuer may be - * missing/untrusted. Fail in that case. + boolean verifyErrorIsOptional = (errnum == X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT()) + || (errnum == X509_V_ERR_SELF_SIGNED_CERT_IN_CHAIN()) + || (errnum == X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY()) + || (errnum == X509_V_ERR_CERT_UNTRUSTED()) + || (errnum == X509_V_ERR_UNABLE_TO_VERIFY_LEAF_SIGNATURE()); + if (verifyErrorIsOptional && (state.certificateVerifyMode == OpenSSLContext.OPTIONAL_NO_CA)) { + ok = 1; + SSL_set_verify_result(state.ssl, X509_V_OK()); + } + /* + * Expired certificates vs. "expired" CRLs: by default, OpenSSL + * turns X509_V_ERR_CRL_HAS_EXPIRED into a "certificate_expired(45)" + * SSL alert, but that's not really the message we should convey to the + * peer (at the very least, it's confusing, and in many cases, it's also + * inaccurate, as the certificate itself may very well not have expired + * yet). We set the X509_STORE_CTX error to something which OpenSSL's + * s3_both.c:ssl_verify_alarm_type() maps to SSL_AD_CERTIFICATE_UNKNOWN, + * i.e. the peer will receive a "certificate_unknown(46)" alert. + * We do not touch errnum, though, so that later on we will still log + * the "real" error, as returned by OpenSSL. */ - if (verifyErrorIsOptional) { - if (state.certificateVerifyMode != OPTIONAL_NO_CA) { - X509_STORE_CTX_set_error(x509ctx, X509_V_ERR_APPLICATION_VERIFICATION()); - errnum = X509_V_ERR_APPLICATION_VERIFICATION(); - ok = 0; - } - } else { - int ocspResponse = processOCSP(x509ctx); - if (ocspResponse == V_OCSP_CERTSTATUS_REVOKED()) { - ok = 0; - errnum = X509_STORE_CTX_get_error(x509ctx); - } else if (ocspResponse == V_OCSP_CERTSTATUS_UNKNOWN()) { - errnum = X509_STORE_CTX_get_error(x509ctx); - if (errnum <= 0) { + if (ok == 0 && errnum == X509_V_ERR_CRL_HAS_EXPIRED()) { + X509_STORE_CTX_set_error(x509ctx, -1); + } + + // OCSP + if (!state.noOcspCheck && (ok > 0)) { + /* If there was an optional verification error, it's not + * possible to perform OCSP validation since the issuer may be + * missing/untrusted. Fail in that case. + */ + if (verifyErrorIsOptional) { + if (state.certificateVerifyMode != OpenSSLContext.OPTIONAL_NO_CA) { + X509_STORE_CTX_set_error(x509ctx, X509_V_ERR_APPLICATION_VERIFICATION()); + errnum = X509_V_ERR_APPLICATION_VERIFICATION(); ok = 0; } + } else { + int ocspResponse = processOCSP(x509ctx); + if (ocspResponse == V_OCSP_CERTSTATUS_REVOKED()) { + ok = 0; + errnum = X509_STORE_CTX_get_error(x509ctx); + } else if (ocspResponse == V_OCSP_CERTSTATUS_UNKNOWN()) { + errnum = X509_STORE_CTX_get_error(x509ctx); + if (errnum <= 0) { + ok = 0; + } + } } } - } - if (errdepth > state.certificateVerificationDepth) { - // Certificate Verification: Certificate Chain too long - ok = 0; + if (errdepth > state.certificateVerificationDepth) { + // Certificate Verification: Certificate Chain too long + ok = 0; + } + return ok; } - return ok; } - static int processOCSP(MemorySegment /*X509_STORE_CTX*/ x509ctx) { + private static int processOCSP(MemorySegment /*X509_STORE_CTX*/ x509ctx) { int ocspResponse = V_OCSP_CERTSTATUS_UNKNOWN(); // ocspResponse = ssl_verify_OCSP(x509_ctx); MemorySegment x509 = X509_STORE_CTX_get_current_cert(x509ctx); diff --git a/java/org/apache/tomcat/util/openssl/openssl_h_Macros.java b/java/org/apache/tomcat/util/openssl/openssl_h_Macros.java index 81435b6781..bcef231520 100644 --- a/java/org/apache/tomcat/util/openssl/openssl_h_Macros.java +++ b/java/org/apache/tomcat/util/openssl/openssl_h_Macros.java @@ -17,18 +17,13 @@ package org.apache.tomcat.util.openssl; -import java.lang.foreign.FunctionDescriptor; -import java.lang.foreign.Linker; import java.lang.foreign.MemorySegment; -import java.lang.foreign.ValueLayout; -import java.lang.invoke.MethodHandle; import static org.apache.tomcat.util.openssl.openssl_h.*; /** * Functional macros not handled by jextract. */ -@SuppressWarnings("javadoc") public class openssl_h_Macros { --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org