On Thu, Aug 21, 2014 at 1:04 PM, Justin Erenkrantz
<[email protected]> wrote:
> On Tue, Aug 19, 2014 at 2:53 PM, Lieven Govaerts <[email protected]> wrote:
>> However, when the client certificate is requested for a resource
>> deeper in the repository, it's likely that say during a large
>> checkout, many (pipelined) requests will already be sent by the client
>> before the request for the protected resource. This is the scenario
>> that'll lead to the problem.
>
> I know we already do something similar in a number of other places -
> what if we can flag that we have sent the client cert, see an error
> with pipelining, and then retry the requests/connections without
> pipelining? It'd mean the performance would suffer for those with
> renegotiations - and if there is a real failure, it'd force us to fail
> twice - but, not require a config option.
>
> I also wonder if we retry the first request that triggered
> renegotiation and then turn back on pipelining...
>
> WDYT? -- justin
Attached patch implements a slightly different approach but I think
it's a valid workaround for the problem.
What it does is detect when a server triggers a renegotiation, and
when that happens switch from a pipelined to a non-pipelined
connection. It doesn't check if the application is actually using the
pipelining, just if it's enabled or not.
I'm not convinced this completely removes the need for the
"http-pipelining" option, because I'm not convinced this will cover
all possible situations, but it should at least cover the ones
reported to the serf dev list.
Lieven
[[[
Add a workaround for issue #135: SSL renegotiation over a connection that
uses HTTP pipelining will fail with OpenSSL.
The workaround: when a connection has pipelining enabled, detect when a server
initiates a SSL renegotiation via the SSL alert info callback, reset the
connection, disable pipelining on the connection and reconnect to the server.
* serf.h
(SERF_ERROR_SSL_NEGOTIATE_IN_PROGRESS): New error code.
(SERF_CONFIG_CONN_PIPELINING): New config key, its value is "Y" or "N" to
indicate if HTTP pipelining is enabled on the connection.
(SERF_CONFIG_*) Renumber to clarify that their code is per category.
* buckets/ssl_buckets.c
(struct serf_ssl_context_t): New flag renegotiation.
(detect_renegotiate): New OpenSSL state callback, listens for the
"SSL renegotiate ciphers" alert and raises an error flag.
(bio_bucket_read,
bio_bucket_write): When the renegotiation flag is raised bailed out
immediately.
(ssl_init_context): Initiate the renegotiation flag.
(serf_ssl_set_config): When the SERF_CONFIG_CONN_PIPELINING key is set to "Y",
detect renegotiation events.
* outgoing.c
(serf__open_connections): Set the SERF_CONFIG_CONN_PIPELINING config value to
"Y" if HTTP pipelining is enabled on the connection.
(read_from_connection): Handle SERF_ERROR_SSL_NEGOTIATE_IN_PROGRESS errors.
TODO: we probably have to do this in write_to_connection as well.
* test/test_ssl.c
(test_ssl_renegotiate): Enable and finish the already written test for this
issue.
* test/MockHTTPinC/MockHTTP_server.c
(setupTCPServer,
_mhRunServerLoop): Handle sudden connection aborts initiated by the client.
(initSSLCtx): Add some logging.
]]]
Index: outgoing.c
===================================================================
--- outgoing.c (revision 2419)
+++ outgoing.c (working copy)
@@ -446,6 +446,13 @@ apr_status_t serf__open_connections(serf_context_t
return status;
}
+ status = serf_config_set_string(conn->config,
+ SERF_CONFIG_CONN_PIPELINING,
+ (conn->max_outstanding_requests != 1 &&
+ conn->pipelining == 1) ? "Y" : "N");
+ if (status)
+ return status;
+
/* Flag our pollset as dirty now that we have a new socket. */
conn->dirty_conn = 1;
ctx->dirty_pollset = 1;
@@ -1247,6 +1254,20 @@ static apr_status_t read_from_connection(serf_conn
goto error;
}
+ /* This connection uses HTTP pipelining and the server asked for a
+ renegotiation (e.g. to access the requested resource a specific
+ client certificate is required).
+ Because of a known problem in OpenSSL this won't work most of the
+ time, so as a workaround, when the server asks for a renegotiation
+ on a connection using HTTP pipelining, we reset the connection,
+ disable pipelining and reconnect to the server. */
+ if (status == SERF_ERROR_SSL_NEGOTIATE_IN_PROGRESS) {
+ serf__connection_set_pipelining(conn, 0);
+ reset_connection(conn, 1);
+ status = APR_SUCCESS;
+ goto error;
+ }
+
/* If our response handler says it can't do anything more, we now
* treat that as a success.
*/
Index: serf.h
===================================================================
--- serf.h (revision 2419)
+++ serf.h (working copy)
@@ -110,6 +110,10 @@ typedef struct serf_config_t serf_config_t;
/* SSL handshake failed */
#define SERF_ERROR_SSL_SETUP_FAILED (SERF_ERROR_START + 72)
+/* Serf-internal error code, raised when the server initiates SSL renegotiation
+ on a connection that uses HTTP pipelining. */
+#define SERF_ERROR_SSL_NEGOTIATE_IN_PROGRESS (SERF_ERROR_START + 73)
+
/* General authentication related errors */
#define SERF_ERROR_AUTHN_FAILED (SERF_ERROR_START + 90)
@@ -1190,9 +1194,10 @@ typedef enum {
#define SERF_CONFIG_HOST_NAME SERF_CONFIG_PER_HOST | 0x000001
#define SERF_CONFIG_HOST_PORT SERF_CONFIG_PER_HOST | 0x000002
-#define SERF_CONFIG_CONN_LOCALIP SERF_CONFIG_PER_CONNECTION | 0x000003
-#define SERF_CONFIG_CONN_REMOTEIP SERF_CONFIG_PER_CONNECTION | 0x000004
-#define SERF_CONFIG_CTX_LOGBATON SERF_CONFIG_PER_CONTEXT | 0x000005
+#define SERF_CONFIG_CONN_LOCALIP SERF_CONFIG_PER_CONNECTION | 0x000001
+#define SERF_CONFIG_CONN_REMOTEIP SERF_CONFIG_PER_CONNECTION | 0x000002
+#define SERF_CONFIG_CONN_PIPELINING SERF_CONFIG_PER_CONNECTION | 0x000003
+#define SERF_CONFIG_CTX_LOGBATON SERF_CONFIG_PER_CONTEXT | 0x000001
/* Configuration values stored in the configuration store:
Index: test/MockHTTPinC/MockHTTP_server.c
===================================================================
--- test/MockHTTPinC/MockHTTP_server.c (revision 2419)
+++ test/MockHTTPinC/MockHTTP_server.c (working copy)
@@ -214,7 +214,7 @@ static apr_status_t setupTCPServer(mhServCtx_t *ct
pfd.desc_type = APR_POLL_SOCKET;
pfd.desc.s = ctx->skt;
- pfd.reqevents = APR_POLLIN;
+ pfd.reqevents = APR_POLLIN | APR_POLLHUP | APR_POLLERR;
STATUSERR(apr_pollset_add(ctx->pollset, &pfd));
}
@@ -1552,7 +1552,7 @@ apr_status_t _mhRunServerLoop(mhServCtx_t *ctx)
cctx = initClientCtx(ctx->pool, ctx, cskt, ctx->type);
pfd.desc_type = APR_POLL_SOCKET;
pfd.desc.s = cskt;
- pfd.reqevents = APR_POLLIN | APR_POLLOUT;
+ pfd.reqevents = APR_POLLIN | APR_POLLOUT | APR_POLLHUP |
APR_POLLERR;
pfd.client_data = cctx;
STATUSERR(apr_pollset_add(ctx->pollset, &pfd));
cctx->reqevents = pfd.reqevents;
@@ -1588,6 +1588,7 @@ apr_status_t _mhRunServerLoop(mhServCtx_t *ctx)
if (status == APR_EOF) {
/* Close the socket and an associated proxy skt */
closeAndRemoveClientCtx(ctx, cctx);
+ break;
}
}
}
@@ -2434,6 +2435,8 @@ static apr_status_t initSSLCtx(_mhClientCtx_t *cct
cctx->ssl_ctx = ssl_ctx;
ssl_ctx->bio_read_status = APR_SUCCESS;
+ _mhLog(MH_VERBOSE, cctx->skt, "Initializing SSL context.\n");
+
/* Init OpenSSL globally */
if (!init_done)
{
Index: test/test_ssl.c
===================================================================
--- test/test_ssl.c (revision 2419)
+++ test/test_ssl.c (working copy)
@@ -1755,10 +1755,23 @@ static void test_ssl_renegotiate(CuTest *tc)
create_new_request(tb, &handler_ctx[0], "GET", "/index1.html", 1);
- run_client_and_mock_servers_loops_expect_ok(tc, tb, num_requests,
- handler_ctx, tb->pool);
+ status = run_client_and_mock_servers_loops(tb, 1, handler_ctx,
+ tb->pool);
+ CuAssertIntEquals(tc, APR_SUCCESS, status);
create_new_request(tb, &handler_ctx[1], "GET", "/index2.html", 2);
+
+ status = run_client_and_mock_servers_loops(tb, num_requests, handler_ctx,
+ tb->pool);
+ CuAssertIntEquals(tc, APR_SUCCESS, status);
+
+ /* Check that the requests were sent and reveived by the server */
+ /* Note: the test server will have received the first request twice, so
+ we can't check for VerifyAllRequestsReceivedInOrder here. */
+ Verify(tb->mh)
+ CuAssert(tc, ErrorMessage, VerifyAllRequestsReceived);
+ EndVerify
+ CuAssertIntEquals(tc, num_requests, tb->handled_requests->nelts);
}
static void test_ssl_missing_client_certificate(CuTest *tc)
@@ -2136,10 +2149,7 @@ CuSuite *test_ssl(void)
SUITE_ADD_TEST(suite, test_ssl_server_cert_with_san_nul_byte);
SUITE_ADD_TEST(suite, test_ssl_server_cert_with_cnsan_nul_byte);
SUITE_ADD_TEST(suite, test_ssl_server_cert_with_san_and_empty_cb);
-#if 0
- /* WIP: Test hangs */
SUITE_ADD_TEST(suite, test_ssl_renegotiate);
-#endif
return suite;
}
Index: buckets/ssl_buckets.c
===================================================================
--- buckets/ssl_buckets.c (revision 2419)
+++ buckets/ssl_buckets.c (working copy)
@@ -179,6 +179,9 @@ struct serf_ssl_context_t {
requests. */
apr_status_t fatal_err;
+ /* Flag is set to 1 when a renegotiation is in progress. */
+ int renegotiation;
+
serf_config_t *config;
};
@@ -278,6 +281,25 @@ apps_ssl_info_callback(const SSL *s, int where, in
}
#endif
+
+/* Listens for the SSL renegotiate ciphers alert and report it back to the
+ serf context. */
+static void
+detect_renegotiate(const SSL *s, int where, int ret)
+{
+#ifdef SERF_LOGGING_ENABLED
+ apps_ssl_info_callback(s, where, ret);
+#endif
+
+ /* The server asked to renegotiate the SSL session. */
+ if (SSL_state(s) == SSL_ST_RENEGOTIATE) {
+ serf_ssl_context_t *ssl_ctx = SSL_get_app_data(s);
+
+ ssl_ctx->renegotiation = 1;
+ ssl_ctx->fatal_err = SERF_ERROR_SSL_NEGOTIATE_IN_PROGRESS;
+ }
+}
+
static void log_ssl_error(serf_ssl_context_t *ctx)
{
unsigned long e = ERR_get_error();
@@ -294,6 +316,11 @@ static int bio_bucket_read(BIO *bio, char *in, int
apr_status_t status;
apr_size_t len;
+ /* The server initiated a renegotiation and we were instructed to report
+ that as an error asap. */
+ if (ctx->renegotiation)
+ return -1;
+
serf__log(LOGLVL_DEBUG, LOGCOMP_SSL, __FILE__, ctx->config,
"bio_bucket_read called for %d bytes\n", inlen);
@@ -328,6 +355,11 @@ static int bio_bucket_write(BIO *bio, const char *
serf_ssl_context_t *ctx = bio->ptr;
serf_bucket_t *tmp;
+ /* The server initiated a renegotiation and we were instructed to report
+ that as an error asap. */
+ if (ctx->renegotiation)
+ return -1;
+
serf__log(LOGLVL_DEBUG, LOGCOMP_SSL, __FILE__, ctx->config,
"bio_bucket_write called for %d bytes\n", inl);
@@ -1446,6 +1478,7 @@ static serf_ssl_context_t *ssl_init_context(serf_b
ssl_ctx->cached_cert_pw = 0;
ssl_ctx->pending_err = APR_SUCCESS;
ssl_ctx->fatal_err = APR_SUCCESS;
+ ssl_ctx->renegotiation = 0;
ssl_ctx->cert_callback = NULL;
ssl_ctx->cert_pw_callback = NULL;
@@ -2092,6 +2125,8 @@ static apr_status_t serf_ssl_set_config(serf_bucke
ssl_context_t *ctx = bucket->data;
serf_ssl_context_t *ssl_ctx = ctx->ssl_ctx;
apr_status_t err_status = APR_SUCCESS;
+ const char *pipelining;
+ apr_status_t status;
ssl_ctx->config = config;
@@ -2111,6 +2146,15 @@ static apr_status_t serf_ssl_set_config(serf_bucke
}
}
+ status = serf_config_get_string(config, SERF_CONFIG_CONN_PIPELINING,
+ &pipelining);
+ if (status)
+ return status;
+
+ if (strcmp(pipelining, "Y") == 0) {
+ SSL_CTX_set_info_callback(ssl_ctx->ctx, detect_renegotiate);
+ }
+
return err_status;
}
