This is an automated email from the ASF dual-hosted git repository.
alexey pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
The following commit(s) were added to refs/heads/master by this push:
new cc646190d [thirdparty] handle OpenSSL errors properly in curl and
squeasel
cc646190d is described below
commit cc646190d3a16bf4f22ac9c32253cf7d4ad05cca
Author: Alexey Serbin <ale...@apache.org>
AuthorDate: Thu May 4 23:43:27 2023 -0700
[thirdparty] handle OpenSSL errors properly in curl and squeasel
This patch improves handling of OpenSSL errors in squeasel and curl,
making the code more robust.
This patch addresses the flakiness in the newly added JWT tests
that were failing in DEBUG-enabled builds because of the assertions
triggered by the SCOPED_OPENSSL_NO_PENDING_ERRORS macro.
I'm planning to submit the patches to the CURL project upstream and
post a PR to the squeasel repo [1] correspondingly. To expedite the
fix and avoid too many unrelated test failures for every CR submitted
to the upstream gerrit, it makes sense to just patch the current source
of these two thirdparty components as an interim solution.
[1] https://github.com/cloudera/squeasel
Change-Id: I209ceb35ded66222c90b82e98e7d191f33573217
Reviewed-on: http://gerrit.cloudera.org:8080/19849
Tested-by: Kudu Jenkins
Reviewed-by: Zoltan Chovan <zcho...@cloudera.com>
Reviewed-by: Abhishek Chennaka <achenn...@cloudera.com>
---
thirdparty/download-thirdparty.sh | 8 +-
.../patches/curl-handle-openssl-errors.patch | 124 +++++++++++++
.../patches/squeasel-handle-openssl-errors.patch | 191 +++++++++++++++++++++
3 files changed, 320 insertions(+), 3 deletions(-)
diff --git a/thirdparty/download-thirdparty.sh
b/thirdparty/download-thirdparty.sh
index 8d68e8e3b..88b4cfd32 100755
--- a/thirdparty/download-thirdparty.sh
+++ b/thirdparty/download-thirdparty.sh
@@ -277,11 +277,12 @@ fetch_and_patch \
"patch -p1 < $TP_DIR/patches/rapidjson-assertions-for-clang-warnings.patch" \
"patch -p1 <
$TP_DIR/patches/rapidjson-avoid-pointer-arithmetic-on-null-pointer.patch"
-SQUEASEL_PATCHLEVEL=0
+SQUEASEL_PATCHLEVEL=1
fetch_and_patch \
squeasel-${SQUEASEL_VERSION}.tar.gz \
$SQUEASEL_SOURCE \
- $SQUEASEL_PATCHLEVEL
+ $SQUEASEL_PATCHLEVEL \
+ "patch -p1 < $TP_DIR/patches/squeasel-handle-openssl-errors.patch"
MUSTACHE_PATCHLEVEL=0
fetch_and_patch \
@@ -302,12 +303,13 @@ fetch_and_patch \
$GCOVR_SOURCE \
$GCOVR_PATCHLEVEL
-CURL_PATCHLEVEL=1
+CURL_PATCHLEVEL=2
fetch_and_patch \
curl-${CURL_VERSION}.tar.gz \
$CURL_SOURCE \
$CURL_PATCHLEVEL \
"patch -p1 < $TP_DIR/patches/curl-custom-openssl-library.patch" \
+ "patch -p1 < $TP_DIR/patches/curl-handle-openssl-errors.patch" \
"autoreconf -fvi"
CRCUTIL_PATCHLEVEL=0
diff --git a/thirdparty/patches/curl-handle-openssl-errors.patch
b/thirdparty/patches/curl-handle-openssl-errors.patch
new file mode 100644
index 000000000..73626dc71
--- /dev/null
+++ b/thirdparty/patches/curl-handle-openssl-errors.patch
@@ -0,0 +1,124 @@
+--- a/lib/vtls/openssl.c 2023-05-03 19:15:43.000000000 -0700
++++ b/lib/vtls/openssl.c 2023-05-04 20:58:10.000000000 -0700
+@@ -405,6 +405,18 @@
+ return buf;
+ }
+
++static void ossl_error_details(struct Curl_easy *data, const char *err_msg) {
++ char err_buf[256];
++ while(true) {
++ unsigned long ssl_err = ERR_get_error();
++ if(!ssl_err) {
++ break;
++ }
++ ossl_strerror(ssl_err, err_buf, sizeof(err_buf));
++ failf(data, "%s: %s", err_msg, err_buf);
++ }
++}
++
+ /* Return an extra data index for the connection data.
+ * This index can be used with SSL_get_ex_data() and SSL_set_ex_data().
+ */
+@@ -796,14 +808,17 @@
+ }
+
+ if(SSL_CTX_use_PrivateKey(ctx, pri) != 1) {
+- failf(data, "unable to use private key from PKCS12 file '%s'",
+- cert_file);
++ failf(data, "unable to use private key from PKCS12 file '%s': %s",
++ cert_file, ossl_strerror(ERR_get_error(), error_buffer,
++ sizeof(error_buffer)));
+ goto fail;
+ }
+
+- if(!SSL_CTX_check_private_key (ctx)) {
++ if(SSL_CTX_check_private_key(ctx) != 1) {
+ failf(data, "private key from PKCS12 file '%s' "
+- "does not match certificate in same file", cert_file);
++ "does not match certificate in same file: %s", cert_file,
++ ossl_strerror(ERR_get_error(), error_buffer,
++ sizeof(error_buffer)));
+ goto fail;
+ }
+ /* Set Certificate Verification chain */
+@@ -860,8 +875,10 @@
+ /* FALLTHROUGH */
+ case SSL_FILETYPE_ASN1:
+ if(SSL_CTX_use_PrivateKey_file(ctx, key_file, file_type) != 1) {
+- failf(data, "unable to set private key file: '%s' type %s",
+- key_file, key_type?key_type:"PEM");
++ failf(data, "unable to set private key file: '%s' type %s: %s",
++ key_file, key_type ? key_type : "PEM",
++ ossl_strerror(ERR_get_error(), error_buffer,
++ sizeof(error_buffer)));
+ return 0;
+ }
+ break;
+@@ -903,7 +920,9 @@
+ return 0;
+ }
+ if(SSL_CTX_use_PrivateKey(ctx, priv_key) != 1) {
+- failf(data, "unable to set private key");
++ failf(data, "unable to set private key: %s",
++ ossl_strerror(ERR_get_error(), error_buffer,
++ sizeof(error_buffer)));
+ EVP_PKEY_free(priv_key);
+ return 0;
+ }
+@@ -975,7 +994,9 @@
+ /* Now we know that a key and cert have been set against
+ * the SSL context */
+ if(!SSL_CTX_check_private_key(ctx)) {
+- failf(data, "Private key does not match the certificate public key");
++ failf(data, "private key does not match the certificate public key:
%s",
++ ossl_strerror(ERR_get_error(), error_buffer,
++ sizeof(error_buffer)));
+ return 0;
+ }
+ }
+@@ -2724,18 +2745,21 @@
+ /* tell SSL where to find CA certificates that are used to verify
+ the servers certificate. */
+ if(!SSL_CTX_load_verify_locations(BACKEND->ctx, ssl_cafile, ssl_capath)) {
++
++ static const char * const err_msg =
++ "error setting certificate verify locations";
+ if(verifypeer) {
+ /* Fail if we insist on successfully verifying the server. */
+- failf(data, "error setting certificate verify locations:\n"
+- " CAfile: %s\n CApath: %s",
++ failf(data, "%s:\n CAfile: %s\n CApath: %s",
++ err_msg,
+ ssl_cafile ? ssl_cafile : "none",
+ ssl_capath ? ssl_capath : "none");
++ ossl_error_details(data, err_msg);
+ return CURLE_SSL_CACERT_BADFILE;
+ }
+ /* Just continue with a warning if no strict certificate verification
+ is required. */
+- infof(data, "error setting certificate verify locations,"
+- " continuing anyway:\n");
++ infof(data, "%s, continuing anyway:\n", err_msg);
+ }
+ else {
+ /* Everything is fine. */
+@@ -2762,7 +2786,9 @@
+ X509_LOOKUP_file());
+ if(!lookup ||
+ (!X509_load_crl_file(lookup, ssl_crlfile, X509_FILETYPE_PEM)) ) {
+- failf(data, "error loading CRL file: %s", ssl_crlfile);
++ static const char * const err_msg = "error loading CRL file";
++ failf(data, "%s: %s", err_msg, ssl_crlfile);
++ ossl_error_details(data, err_msg);
+ return CURLE_SSL_CRL_BADFILE;
+ }
+ /* Everything is fine. */
+@@ -2994,6 +3020,8 @@
+ result = CURLE_SSL_CONNECT_ERROR;
+ ossl_strerror(errdetail, error_buffer, sizeof(error_buffer));
+ }
++ // Clear the rest of the errors as well.
++ ERR_clear_error();
+
+ /* detail is already set to the SSL error above */
+
diff --git a/thirdparty/patches/squeasel-handle-openssl-errors.patch
b/thirdparty/patches/squeasel-handle-openssl-errors.patch
new file mode 100644
index 000000000..23dbfff13
--- /dev/null
+++ b/thirdparty/patches/squeasel-handle-openssl-errors.patch
@@ -0,0 +1,191 @@
+diff --git a/squeasel.c b/squeasel.c
+index d716783..ff40dcc 100644
+--- a/squeasel.c
++++ b/squeasel.c
+@@ -904,11 +904,26 @@ int sq_get_bound_addresses(const struct sq_context *ctx,
struct sockaddr_in ***a
+ return rc;
+ }
+
++#if !defined(NO_SSL)
++
++// Return the last OpenSSL error message and clean the thread's error queue.
++static const char *ssl_error(void) {
++ unsigned long err = ERR_peek_error();
++ if (err != 0) {
++ // Clear the thread's error queue.
++ ERR_clear_error();
++ return ERR_error_string(err, NULL);
++ }
++ return "";
++}
++
++#endif
+
+ // Write data to the IO channel - opened file descriptor, socket or SSL
+-// descriptor. Return number of bytes written.
++// descriptor. Return number of bytes written. The 'conn' parameter is used
++// only for reporting on errors, if any.
+ static int64_t push(FILE *fp, SOCKET sock, SSL *ssl, const char *buf,
+- int64_t len) {
++ int64_t len, struct sq_connection *conn) {
+ int64_t sent;
+ int n, k;
+
+@@ -922,6 +937,12 @@ static int64_t push(FILE *fp, SOCKET sock, SSL *ssl,
const char *buf,
+ #ifndef NO_SSL
+ if (ssl != NULL) {
+ n = SSL_write(ssl, buf + sent, k);
++ if (n <= 0) {
++ int err = SSL_get_error(ssl, n);
++ if (err != 0 && ERR_peek_error() != 0) {
++ cry(conn, "SSL_write error: %s", ssl_error());
++ }
++ }
+ } else
+ #endif
+ if (fp != NULL) {
+@@ -966,6 +987,12 @@ static int pull(FILE *fp, struct sq_connection *conn,
char *buf, int len) {
+ #ifndef NO_SSL
+ } else if (conn->ssl != NULL) {
+ nread = SSL_read(conn->ssl, buf, len);
++ if (nread <= 0) {
++ int err = SSL_get_error(conn->ssl, nread);
++ if (err != 0 && ERR_peek_error() != 0) {
++ cry(conn, "SSL_read error: %s", ssl_error());
++ }
++ }
+ #endif
+ } else {
+ RETRY_ON_EINTR(nread, recv(conn->client.sock, buf, (size_t) len, 0));
+@@ -1050,14 +1077,14 @@ int sq_write(struct sq_connection *conn, const void
*buf, size_t len) {
+ allowed = len;
+ }
+ if ((total = push(NULL, conn->client.sock, conn->ssl, (const char *) buf,
+- (int64_t) allowed)) == allowed) {
++ (int64_t) allowed, conn)) == allowed) {
+ buf = (char *) buf + total;
+ conn->last_throttle_bytes += total;
+ while (total < (int64_t) len && conn->ctx->stop_flag == 0) {
+ allowed = conn->throttle > (int64_t) len - total ?
+ (int64_t) len - total : conn->throttle;
+ if ((n = push(NULL, conn->client.sock, conn->ssl, (const char *) buf,
+- (int64_t) allowed)) != allowed) {
++ (int64_t) allowed, conn)) != allowed) {
+ break;
+ }
+ sleep(1);
+@@ -1069,7 +1096,7 @@ int sq_write(struct sq_connection *conn, const void
*buf, size_t len) {
+ }
+ } else {
+ total = push(NULL, conn->client.sock, conn->ssl, (const char *) buf,
+- (int64_t) len);
++ (int64_t) len, conn);
+ }
+ return (int) total;
+ }
+@@ -2479,7 +2506,7 @@ static int forward_body_data(struct sq_connection *conn,
FILE *fp,
+ if ((int64_t) buffered_len > conn->content_len) {
+ buffered_len = (int) conn->content_len;
+ }
+- push(fp, sock, ssl, body, (int64_t) buffered_len);
++ push(fp, sock, ssl, body, (int64_t) buffered_len, conn);
+ conn->consumed_content += buffered_len;
+ }
+
+@@ -2490,7 +2517,7 @@ static int forward_body_data(struct sq_connection *conn,
FILE *fp,
+ to_read = (int) (conn->content_len - conn->consumed_content);
+ }
+ nread = pull(NULL, conn, buf, to_read);
+- if (nread <= 0 || push(fp, sock, ssl, buf, nread) != nread) {
++ if (nread <= 0 || push(fp, sock, ssl, buf, nread, conn) != nread) {
+ break;
+ }
+ conn->consumed_content += nread;
+@@ -3788,16 +3815,20 @@ static int set_uid_option(struct sq_context *ctx) {
+ static pthread_mutex_t *ssl_mutexes;
+
+ static int sslize(struct sq_connection *conn, SSL_CTX *s, int (*func)(SSL *))
{
+- return (conn->ssl = SSL_new(s)) != NULL &&
+- SSL_set_fd(conn->ssl, conn->client.sock) == 1 &&
+- func(conn->ssl) == 1;
+-}
+-
+-// Return OpenSSL error message
+-static const char *ssl_error(void) {
+- unsigned long err;
+- err = ERR_get_error();
+- return err == 0 ? "" : ERR_error_string(err, NULL);
++ conn->ssl = SSL_new(s);
++ if (conn->ssl == NULL) {
++ cry(conn, "SSL_new failed: %s", ssl_error());
++ return 0;
++ }
++ if (SSL_set_fd(conn->ssl, conn->client.sock) != 1) {
++ cry(conn, "SSL_set_fd failed: %s", ssl_error());
++ return 0;
++ }
++ if (func(conn->ssl) != 1) {
++ cry(conn, "sslize failed: %s", ssl_error());
++ return 0;
++ }
++ return 1;
+ }
+
+ static void ssl_locking_callback(int mode, int mutex_num, const char *file,
+@@ -3899,8 +3930,8 @@ static int set_ssl_option(struct sq_context *ctx) {
+ }
+
+ if ((SSL_CTX_set_options(ctx->ssl_ctx, options) & options) != options) {
+- cry(fc(ctx), "SSL_CTX_set_options (server) error: could not set options
(%d)",
+- options);
++ cry(fc(ctx), "SSL_CTX_set_options (server) error: could not set options
(%d) %s",
++ options, ssl_error());
+ return 0;
+ }
+
+@@ -3913,18 +3944,21 @@ static int set_ssl_option(struct sq_context *ctx) {
+ // set up certificate itself. In this case, skip sertificate setting.
+ if ((ctx->callbacks.init_ssl == NULL ||
+ !ctx->callbacks.init_ssl(ctx->ssl_ctx, ctx->user_data)) &&
+- (SSL_CTX_use_certificate_file(ctx->ssl_ctx, pem, 1) == 0 ||
+- SSL_CTX_use_PrivateKey_file(ctx->ssl_ctx, private_key, 1) == 0)) {
++ (SSL_CTX_use_certificate_file(ctx->ssl_ctx, pem, 1) != 1 ||
++ SSL_CTX_use_PrivateKey_file(ctx->ssl_ctx, private_key, 1) != 1)) {
+ cry(fc(ctx), "%s: cannot open %s: %s", __func__, pem, ssl_error());
+ return 0;
+ }
+
+ if (pem != NULL) {
+- (void) SSL_CTX_use_certificate_chain_file(ctx->ssl_ctx, pem);
++ if (SSL_CTX_use_certificate_chain_file(ctx->ssl_ctx, pem) != 1) {
++ cry(fc(ctx), "%s: cannot open %s: %s", __func__, pem, ssl_error());
++ return 0;
++ }
+ }
+
+ if (ctx->config[SSL_CIPHERS] != NULL) {
+- if (SSL_CTX_set_cipher_list(ctx->ssl_ctx, ctx->config[SSL_CIPHERS]) == 0)
{
++ if (SSL_CTX_set_cipher_list(ctx->ssl_ctx, ctx->config[SSL_CIPHERS]) != 1)
{
+ cry(fc(ctx), "SSL_CTX_set_cipher_list: error setting ciphers (%s): %s",
+ ctx->config[SSL_CIPHERS], ssl_error());
+ return 0;
+@@ -4018,7 +4052,9 @@ static void close_connection(struct sq_connection *conn)
{
+ #ifndef NO_SSL
+ if (conn->ssl != NULL) {
+ // Run SSL_shutdown twice to ensure completly close SSL connection
+- SSL_shutdown(conn->ssl);
++ if (SSL_shutdown(conn->ssl) < 0) {
++ cry(conn, "error shutting down SSL connection: %s", ssl_error());
++ }
+ SSL_free(conn->ssl);
+ conn->ssl = NULL;
+ }
+@@ -4053,7 +4089,7 @@ struct sq_connection *sq_connect(const char *host, int
port, int use_ssl,
+ #ifndef NO_SSL
+ } else if (use_ssl && (conn->client_ssl_ctx =
+ SSL_CTX_new(SSLv23_client_method())) == NULL) {
+- snprintf(ebuf, ebuf_len, "SSL_CTX_new error");
++ snprintf(ebuf, ebuf_len, "SSL_CTX_new error %s", ssl_error());
+ closesocket(sock);
+ free(conn);
+ conn = NULL;
