The OpenSSL devs intended this to be a breaking change - but it's not
documented anywhere. Sigh.
I've got a WIP patch against trunk that causes test_ssl to pass - see
below. It also seems to work with OpenSSL 1.1.1h as well as OpenSSL 1.1.1i
/ 1.1.1-stable, AFAICT.
James: can you please give it a try as well?
We've been on the threshold of releasing serf 1.4 for quite some time
now...maybe we should just do that... If this looks reasonable, I'll try
to clean this up and get it into trunk and 1.4.x.
Cheers. -- justin
Index: test/test_serf.h
===================================================================
--- test/test_serf.h (revision 1884847)
+++ test/test_serf.h (working copy)
@@ -296,6 +296,14 @@
handler_baton_t handler_ctx[],
apr_pool_t *pool);
+/* Helper function, runs the client and server context loops and validates
+ that errors were encountered. */
+void
+run_client_and_mock_servers_loops_expect_fail(CuTest *tc, test_baton_t *tb,
+ int num_requests,
+ handler_baton_t
handler_ctx[],
+ apr_pool_t *pool);
+
/* Logs a test suite error with its code location, and return status
SERF_ERROR_ISSUE_IN_TESTSUITE. */
#define REPORT_TEST_SUITE_ERROR()\
Index: test/test_ssl.c
===================================================================
--- test/test_ssl.c (revision 1884847)
+++ test/test_ssl.c (working copy)
@@ -465,9 +465,10 @@
tb->result_flags |= TEST_RESULT_SERVERCERTCB_CALLED;
+ test__log(TEST_VERBOSE, __FILE__, "Cert failure received: %d ;
expected failure mask: %d\n", failures, expected_failures);
/* We expect an error from the certificate validation function. */
if (failures & expected_failures)
- return APR_SUCCESS;
+ return APR_EGENERAL;
else
return REPORT_TEST_SUITE_ERROR();
}
@@ -532,8 +533,8 @@
create_new_request(tb, &handler_ctx[0], "GET", "/", 1);
- run_client_and_mock_servers_loops_expect_ok(tc, tb, num_requests,
- handler_ctx, tb->pool);
+ run_client_and_mock_servers_loops_expect_fail(tc, tb, num_requests,
+ handler_ctx, tb->pool);
}
/* Validate that connecting to a SSLv2 only server fails. */
@@ -1121,8 +1122,8 @@
create_new_request(tb, &handler_ctx[0], "GET", "/", 1);
- run_client_and_mock_servers_loops_expect_ok(tc, tb, num_requests,
- handler_ctx, tb->pool);
+ run_client_and_mock_servers_loops_expect_fail(tc, tb, num_requests,
+ handler_ctx, tb->pool);
CuAssertTrue(tc, tb->result_flags & TEST_RESULT_SERVERCERTCB_CALLED);
}
@@ -1165,8 +1166,8 @@
create_new_request(tb, &handler_ctx[0], "GET", "/", 1);
- run_client_and_mock_servers_loops_expect_ok(tc, tb, num_requests,
- handler_ctx, tb->pool);
+ run_client_and_mock_servers_loops_expect_fail(tc, tb, num_requests,
+ handler_ctx, tb->pool);
CuAssertTrue(tc, tb->result_flags & TEST_RESULT_SERVERCERTCB_CALLED);
}
@@ -2095,8 +2096,8 @@
create_new_request(tb, &handler_ctx[0], "GET", "/", 1);
- run_client_and_mock_servers_loops_expect_ok(tc, tb, num_requests,
- handler_ctx, tb->pool);
+ run_client_and_mock_servers_loops_expect_fail(tc, tb, num_requests,
+ handler_ctx, tb->pool);
CuAssertTrue(tc, tb->result_flags & TEST_RESULT_SERVERCERTCB_CALLED);
}
@@ -2138,8 +2139,8 @@
create_new_request(tb, &handler_ctx[0], "GET", "/", 1);
- run_client_and_mock_servers_loops_expect_ok(tc, tb, num_requests,
- handler_ctx, tb->pool);
+ run_client_and_mock_servers_loops_expect_fail(tc, tb, num_requests,
+ handler_ctx, tb->pool);
CuAssertTrue(tc, tb->result_flags & TEST_RESULT_SERVERCERTCB_CALLED);
}
@@ -2181,8 +2182,8 @@
create_new_request(tb, &handler_ctx[0], "GET", "/", 1);
- run_client_and_mock_servers_loops_expect_ok(tc, tb, num_requests,
- handler_ctx, tb->pool);
+ run_client_and_mock_servers_loops_expect_fail(tc, tb, num_requests,
+ handler_ctx, tb->pool);
CuAssertTrue(tc, tb->result_flags & TEST_RESULT_SERVERCERTCB_CALLED);
}
@@ -2310,8 +2311,8 @@
create_new_request(tb, &handler_ctx[0], "GET", "/", 1);
- run_client_and_mock_servers_loops_expect_ok(tc, tb, num_requests,
- handler_ctx, tb->pool);
+ run_client_and_mock_servers_loops_expect_fail(tc, tb, num_requests,
+ handler_ctx, tb->pool);
#endif /* OPENSSL_NO_TLSEXT */
}
Index: test/test_util.c
===================================================================
--- test/test_util.c (revision 1884847)
+++ test/test_util.c (working copy)
@@ -561,6 +561,19 @@
CuAssertIntEquals(tc, num_requests, tb->handled_requests->nelts);
}
+void
+run_client_and_mock_servers_loops_expect_fail(CuTest *tc, test_baton_t *tb,
+ int num_requests,
+ handler_baton_t
handler_ctx[],
+ apr_pool_t *pool)
+{
+ apr_status_t status;
+
+ status = run_client_and_mock_servers_loops(tb, num_requests,
handler_ctx,
+ pool);
+ CuAssertIntEquals_Msg(tc, serf_error_string(status), APR_EGENERAL,
status);
+}
+
void setup_test_mock_server(test_baton_t *tb)
{
if (!tb->mh) /* TODO: move this to test_setup */
On Mon, Dec 28, 2020 at 5:22 PM Justin Erenkrantz <jus...@erenkrantz.com>
wrote:
> On Mon, Dec 28, 2020 at 5:00 PM Justin Erenkrantz <jus...@erenkrantz.com>
> wrote:
>
>> It's not clear to me if OpenSSL authors intended to make this breaking
>> change. On the serf side, we would need to think through what it would
>> mean to have our app callback return false upon failure in order to
>> short-circuit the check.
>>
>> I probably won't get a chance to open an upstream OpenSSL issue today (or
>> even tomorrow)...
>>
>
> I found the original issue where they changed the behavior and added a
> comment there:
>
> https://github.com/openssl/openssl/issues/11297
>
> Cheers. -- justin
>
