You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2007/11/29 21:02:43 UTC
Re: svn commit: r599385 - in /httpd/httpd/trunk: ./ modules/ssl/
On 11/29/2007 12:18 PM, jorton@apache.org wrote:
> Author: jorton
> Date: Thu Nov 29 03:18:40 2007
> New Revision: 599385
>
> URL: http://svn.apache.org/viewvc?rev=599385&view=rev
> Log:
> mod_ssl: Add support for OCSP validation of client certificates:
>
> * modules/ssl/ssl_engine_config.c (modssl_ctx_init,
> modssl_ctx_cfg_merge): Initialize and merge OCSP config options.
> (ssl_cmd_SSLOCSPOverrideResponder, ssl_cmd_SSLOCSPDefaultResponder,
> ssl_cmd_SSLOCSPEnable): Add functions.
>
> * modules/ssl/mod_ssl.c (ssl_config_cmds): Add config options.
>
> * modules/ssl/ssl_private.h: Add prototypes, config options to
> modssl_ctx_t.
>
> * modules/ssl/ssl_util_ocsp.c: New file, utility interface for
> dispatching OCSP requests.
>
> * modules/ssl/ssl_engine_ocsp.c: New file, interface for performing
> OCSP validation.
>
> * modules/ssl/ssl_engine_kernel.c (ssl_callback_SSLVerify): Perform
> OCSP validation if configured, and the cert is so-far verified to be
> trusted. Fail if OCSP validation is configured an the optional-no-ca
> check tripped.
>
> * modules/ssl/config.m4: Check for OCSP support, build new files.
>
> * modules/ssl/mod_ssl.dsp: Build new files.
>
> * modules/ssl/ssl_toolkit_compat.h: Include headers for OCSP
> interfaces.
>
> PR: 41123
> Submitted by: Marc Stern <marc.stern approach.be>, Joe Orton
> Reviewed by: Steve Henson <steve openssl.org>
>
> Added:
> httpd/httpd/trunk/modules/ssl/ssl_engine_ocsp.c (with props)
> httpd/httpd/trunk/modules/ssl/ssl_util_ocsp.c (with props)
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/modules/ssl/config.m4
> httpd/httpd/trunk/modules/ssl/mod_ssl.c
> httpd/httpd/trunk/modules/ssl/mod_ssl.dsp
> httpd/httpd/trunk/modules/ssl/ssl_engine_config.c
> httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c
> httpd/httpd/trunk/modules/ssl/ssl_private.h
> httpd/httpd/trunk/modules/ssl/ssl_toolkit_compat.h
>
> Modified: httpd/httpd/trunk/CHANGES
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=599385&r1=599384&r2=599385&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/CHANGES [utf-8] (original)
> +++ httpd/httpd/trunk/CHANGES [utf-8] Thu Nov 29 03:18:40 2007
> @@ -2,6 +2,9 @@
> Changes with Apache 2.3.0
> [ When backported to 2.2.x, remove entry from this file ]
>
> + *) mod_ssl: Add support for OCSP validation of client certificates.
> + PR 41123. [Marc Stern <marc.stern approach.be>, Joe Orton]
> +
Shouldn't we add Steve to this? As far as I followed the discussion in Bugzilla
he contributed very valuable points and we have credited people for less in the past :-).
> Added: httpd/httpd/trunk/modules/ssl/ssl_util_ocsp.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_util_ocsp.c?rev=599385&view=auto
> ==============================================================================
> --- httpd/httpd/trunk/modules/ssl/ssl_util_ocsp.c (added)
> +++ httpd/httpd/trunk/modules/ssl/ssl_util_ocsp.c Thu Nov 29 03:18:40 2007
> @@ -0,0 +1,299 @@
> +/* Licensed to the Apache Software Foundation (ASF) under one or more
> + * contributor license agreements. See the NOTICE file distributed with
> + * this work for additional information regarding copyright ownership.
> + * The ASF licenses this file to You under the Apache License, Version 2.0
> + * (the "License"); you may not use this file except in compliance with
> + * the License. You may obtain a copy of the License at
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +/* This file implements an OCSP client including a toy HTTP/1.0
> + * client. Once httpd depends on a real HTTP client library, most of
> + * this can be thrown away. */
> +
> +#include "ssl_private.h"
> +
> +#ifdef HAVE_OCSP
> +
> +#include "apr_buckets.h"
> +#include "apr_uri.h"
> +
> +/* Serialize an OCSP request which will be sent to the responder at
> + * given URI to a memory BIO object, which is returned. */
> +static BIO *serialize_request(OCSP_REQUEST *req, const apr_uri_t *uri)
> +{
> + BIO *bio;
> + int len;
> +
> + len = i2d_OCSP_REQUEST(req, NULL);
> +
> + bio = BIO_new(BIO_s_mem());
> +
> + BIO_printf(bio, "POST %s%s HTTP/1.0\r\n"
> + "Host: %s:%d\r\n"
> + "Content-Length: %d\r\n"
> + "\r\n",
> + uri->path, uri->query ? uri->query : "",
> + uri->hostname, uri->port, len);
> +
> + if (i2d_OCSP_REQUEST_bio(bio, req) != 1) {
> + BIO_free(bio);
> + return NULL;
> + }
> +
> + return bio;
> +}
> +
> +/* Send the OCSP request serialized into BIO 'request' to the
> + * responder at given server given by URI. Returns socket object or
> + * NULL on error. */
> +static apr_socket_t *send_request(BIO *request, const apr_uri_t *uri,
> + conn_rec *c, apr_pool_t *p)
> +{
> + apr_status_t rv;
> + apr_sockaddr_t *sa;
> + apr_socket_t *sd;
> + char buf[HUGE_STRING_LEN];
Is it really a good idea to store this on the stack? Shouldn't we allocate
this from the pool?
> + int len;
> +
> + rv = apr_sockaddr_info_get(&sa, uri->hostname, APR_UNSPEC, uri->port, 0, p);
> + if (rv) {
> + ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, c,
> + "could not resolve address of OCSP responder %s",
> + uri->hostinfo);
> + return NULL;
> + }
> +
> + /* establish a connection to the OCSP responder */
> + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, c,
> + "connecting to OCSP responder '%s'", uri->hostinfo);
> +
> + /* Cycle through address until a connect() succeeds. */
> + for (; sa; sa = sa->next) {
> + rv = apr_socket_create(&sd, sa->family, SOCK_STREAM, APR_PROTO_TCP, p);
> + if (rv == APR_SUCCESS) {
> + /* Inherit the default I/O timeout. */
> + apr_socket_timeout_set(sd, c->base_server->timeout);
> +
> + rv = apr_socket_connect(sd, sa);
> + if (rv == APR_SUCCESS) {
> + break;
> + }
> + apr_socket_close(sd);
> + }
> + }
> +
> + if (sa == NULL) {
> + ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, c,
> + "could not connect to OCSP responder '%s'",
> + uri->hostinfo);
> + apr_socket_close(sd);
> + return NULL;
> + }
> +
> + /* send the request and get a response */
> + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, c,
> + "sending request to OCSP responder");
> +
> + while ((len = BIO_read(request, buf, sizeof buf)) > 0) {
> + char *wbuf = buf;
> + apr_size_t remain = len;
> +
> + do {
> + apr_size_t wlen = remain;
> +
> + rv = apr_socket_send(sd, wbuf, &wlen);
> + wbuf += remain;
> + remain -= wlen;
> + } while (rv == APR_SUCCESS && remain > 0);
Why do we need remain here and do not use len directly?
> +
> + if (rv) {
> + apr_socket_close(sd);
> + ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, c,
> + "failed to send request to OCSP responder '%s'",
> + uri->hostinfo);
> + return NULL;
> + }
> + }
> +
> + return sd;
> +}
> +
> +/* Return a pool-alocated NUL-terminated line, with CRLF stripped,
> + * read from brigade 'bbin' using 'bbout' as temporary storage. */
> +static char *get_line(apr_bucket_brigade *bbout, apr_bucket_brigade *bbin,
> + conn_rec *c, apr_pool_t *p)
> +{
> + apr_status_t rv;
> + apr_size_t len;
> + char *line;
> +
> + apr_brigade_cleanup(bbout);
> +
> + rv = apr_brigade_split_line(bbout, bbin, APR_BLOCK_READ, 8192);
> + if (rv) {
> + ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, c,
> + "failed reading line from OCSP server");
> + return NULL;
> + }
> +
> + rv = apr_brigade_pflatten(bbout, &line, &len, p);
> + if (rv) {
> + ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, c,
> + "failed reading line from OCSP server");
> + return NULL;
> + }
> +
> + if (len && line[len-1] != APR_ASCII_LF) {
> + ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, c,
> + "response header line too long from OCSP server");
> + return NULL;
> + }
> +
> + line[len-1] = '\0';
> + if (len > 1 && line[len-2] == APR_ASCII_CR) {
> + line[len-2] = '\0';
> + }
> +
> + return line;
> +}
> +
> +/* Maximum values to prevent eating RAM forever. */
> +#define MAX_HEADERS (256)
> +#define MAX_CONTENT (2048 * 1024)
> +
> +/* Read the OCSP response from the socket 'sd', using temporary memory
> + * BIO 'bio', and return the decoded OCSP response object, or NULL on
> + * error. */
> +static OCSP_RESPONSE *read_response(apr_socket_t *sd, BIO *bio, conn_rec *c,
> + apr_pool_t *p)
> +{
> + apr_bucket_brigade *bb, *tmpbb;
> + OCSP_RESPONSE *response;
> + char *line;
> + apr_size_t count;
> + apr_int64_t code;
> +
> + /* Using brigades for response parsing is much simpler than using
> + * apr_socket_* directly. */
> + bb = apr_brigade_create(p, c->bucket_alloc);
> + tmpbb = apr_brigade_create(p, c->bucket_alloc);
> + APR_BRIGADE_INSERT_TAIL(bb, apr_bucket_socket_create(sd, c->bucket_alloc));
> +
> + line = get_line(tmpbb, bb, c, p);
> + if (!line || strncmp(line, "HTTP/", 5)
> + || (line = ap_strchr(line, ' ')) == NULL
> + || (code = apr_atoi64(++line)) < 200 || code > 299) {
> + ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, c,
> + "bad response from OCSP server: %s",
> + line ? line : "(none)");
> + return NULL;
> + }
> +
> + /* Read till end of headers; don't have to even bother parsing the
> + * Content-Length since the server is obliged to close the
> + * connection after the response anyway for HTTP/1.0. */
> + count = 0;
> + while ((line = get_line(tmpbb, bb, c, p)) != NULL && line[0]
> + && ++count < MAX_HEADERS) {
> + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, c,
> + "OCSP response header: %s", line);
> + }
Why don't we exit with an error message if count > MAX_HEADERS?
> +
> + if (!line) {
> + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, c,
> + "could not read response header from OCSP server");
> + return NULL;
> + }
> +
> + /* Read the response body into the memory BIO. */
> + count = 0;
> + while (!APR_BRIGADE_EMPTY(bb)) {
> + const char *data;
> + apr_size_t len;
> + apr_status_t rv;
> + apr_bucket *e = APR_BRIGADE_FIRST(bb);
> +
> + rv = apr_bucket_read(e, &data, &len, APR_BLOCK_READ);
> + if (rv == APR_EOF || (rv == APR_SUCCESS && len == 0)) {
> + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, c,
> + "OCSP response: got EOF");
> + break;
> + }
> + if (rv != APR_SUCCESS) {
> + ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, c,
> + "error reading response from OCSP server");
> + return NULL;
> + }
> + count += len;
> + if (count > MAX_CONTENT) {
> + ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, c,
> + "OCSP response size exceeds %u byte limit",
> + MAX_CONTENT);
> + return NULL;
> + }
> + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, c,
> + "OCSP response: got %" APR_SIZE_T_FMT
> + " bytes, %" APR_SIZE_T_FMT " total", len, count);
> +
> + BIO_write(bio, data, (int)len);
> + apr_bucket_delete(e);
> + }
> +
> + apr_brigade_destroy(bb);
> + apr_brigade_destroy(tmpbb);
We miss to destroy the brigades in the cases above where we return NULL.
Regards
Rüdiger
Re: svn commit: r599385 - in /httpd/httpd/trunk: ./ modules/ssl/
Posted by Dr Stephen Henson <sh...@oss-institute.org>.
Ruediger Pluem wrote:
>
> On 11/29/2007 10:03 PM, Joe Orton wrote:
>> On Thu, Nov 29, 2007 at 09:02:43PM +0100, Ruediger Pluem wrote:
>>>> ==============================================================================
>>>> --- httpd/httpd/trunk/CHANGES [utf-8] (original)
>>>> +++ httpd/httpd/trunk/CHANGES [utf-8] Thu Nov 29 03:18:40 2007
>>>> @@ -2,6 +2,9 @@
>>>> Changes with Apache 2.3.0
>>>> [ When backported to 2.2.x, remove entry from this file ]
>>>>
>>>> + *) mod_ssl: Add support for OCSP validation of client certificates.
>>>> + PR 41123. [Marc Stern <marc.stern approach.be>, Joe Orton]
>>>> +
>>> Shouldn't we add Steve to this? As far as I followed the discussion in
>>> Bugzilla he contributed very valuable points and we have credited
>>> people for less in the past :-).
>> Discussing this stuff makes me slightly uncomfortable, but I try to
>> follow a strict rule: credit in CHANGES exactly the set of people who
>> contributed actual lines of code. Steve was credited in the commit
>> messages as a reviewer. Such review is invaluable, no argument at all
>> :)
>
> This rule is fine with me. I guess everybody has its own ruleset here which
> might be slightly different.
> I guess Steve is famous enough such that he can live without a CHANGES entry
> and "only" a commit message :).
>
>
Can one be famous enough? ;-)
Don't mind at all either way to be honest. I haven't had the opportunity
to study mod_ssl much until now and I hope to do considerably more in
future to at least merit the occasional CHANGES credit.
Steve.
--
Dr Stephen N. Henson.
Core developer of the OpenSSL project: http://www.openssl.org/
Freelance consultant see: http://www.drh-consultancy.co.uk/
Email: shenson@drh-consultancy.co.uk, PGP key: via homepage.
Re: svn commit: r599385 - in /httpd/httpd/trunk: ./ modules/ssl/
Posted by Ruediger Pluem <rp...@apache.org>.
On 11/29/2007 10:03 PM, Joe Orton wrote:
> On Thu, Nov 29, 2007 at 09:02:43PM +0100, Ruediger Pluem wrote:
>>> ==============================================================================
>>> --- httpd/httpd/trunk/CHANGES [utf-8] (original)
>>> +++ httpd/httpd/trunk/CHANGES [utf-8] Thu Nov 29 03:18:40 2007
>>> @@ -2,6 +2,9 @@
>>> Changes with Apache 2.3.0
>>> [ When backported to 2.2.x, remove entry from this file ]
>>>
>>> + *) mod_ssl: Add support for OCSP validation of client certificates.
>>> + PR 41123. [Marc Stern <marc.stern approach.be>, Joe Orton]
>>> +
>> Shouldn't we add Steve to this? As far as I followed the discussion in
>> Bugzilla he contributed very valuable points and we have credited
>> people for less in the past :-).
>
> Discussing this stuff makes me slightly uncomfortable, but I try to
> follow a strict rule: credit in CHANGES exactly the set of people who
> contributed actual lines of code. Steve was credited in the commit
> messages as a reviewer. Such review is invaluable, no argument at all
> :)
This rule is fine with me. I guess everybody has its own ruleset here which
might be slightly different.
I guess Steve is famous enough such that he can live without a CHANGES entry
and "only" a commit message :).
>
> ...
>>> +/* Send the OCSP request serialized into BIO 'request' to the
>>> + * responder at given server given by URI. Returns socket object or
>>> + * NULL on error. */
>>> +static apr_socket_t *send_request(BIO *request, const apr_uri_t *uri,
>>> + conn_rec *c, apr_pool_t *p)
>>> +{
>>> + apr_status_t rv;
>>> + apr_sockaddr_t *sa;
>>> + apr_socket_t *sd;
>>> + char buf[HUGE_STRING_LEN];
>> Is it really a good idea to store this on the stack? Shouldn't we allocate
>> this from the pool?
>
> Hmmm, "shrug" - stack is cheaper than heap.
Just thought of platforms with small default stack sizes.
So lets stick to it until someone reports problems.
>
> ...
>>> + while ((len = BIO_read(request, buf, sizeof buf)) > 0) {
>>> + char *wbuf = buf;
>>> + apr_size_t remain = len;
>>> +
>>> + do {
>>> + apr_size_t wlen = remain;
>>> +
>>> + rv = apr_socket_send(sd, wbuf, &wlen);
>>> + wbuf += remain;
>>> + remain -= wlen;
>>> + } while (rv == APR_SUCCESS && remain > 0);
>> Why do we need remain here and do not use len directly?
>
> Really only to make the types match; len is an int but wlen must be an
> apr_size_t.
Thanks for explaining.
> ...
>>> + apr_brigade_destroy(bb);
>>> + apr_brigade_destroy(tmpbb);
>> We miss to destroy the brigades in the cases above where we return NULL.
>
> They are allocated out of the pool, so it Shouldn't Matter (TM).
Ok. They are taken from the newly created vpool. So it shouldn't really matter.
Regards
Rüdiger
Re: svn commit: r599385 - in /httpd/httpd/trunk: ./ modules/ssl/
Posted by Joe Orton <jo...@redhat.com>.
On Thu, Nov 29, 2007 at 09:02:43PM +0100, Ruediger Pluem wrote:
> > ==============================================================================
> > --- httpd/httpd/trunk/CHANGES [utf-8] (original)
> > +++ httpd/httpd/trunk/CHANGES [utf-8] Thu Nov 29 03:18:40 2007
> > @@ -2,6 +2,9 @@
> > Changes with Apache 2.3.0
> > [ When backported to 2.2.x, remove entry from this file ]
> >
> > + *) mod_ssl: Add support for OCSP validation of client certificates.
> > + PR 41123. [Marc Stern <marc.stern approach.be>, Joe Orton]
> > +
>
> Shouldn't we add Steve to this? As far as I followed the discussion in
> Bugzilla he contributed very valuable points and we have credited
> people for less in the past :-).
Discussing this stuff makes me slightly uncomfortable, but I try to
follow a strict rule: credit in CHANGES exactly the set of people who
contributed actual lines of code. Steve was credited in the commit
messages as a reviewer. Such review is invaluable, no argument at all
:)
...
> > +/* Send the OCSP request serialized into BIO 'request' to the
> > + * responder at given server given by URI. Returns socket object or
> > + * NULL on error. */
> > +static apr_socket_t *send_request(BIO *request, const apr_uri_t *uri,
> > + conn_rec *c, apr_pool_t *p)
> > +{
> > + apr_status_t rv;
> > + apr_sockaddr_t *sa;
> > + apr_socket_t *sd;
> > + char buf[HUGE_STRING_LEN];
>
> Is it really a good idea to store this on the stack? Shouldn't we allocate
> this from the pool?
Hmmm, "shrug" - stack is cheaper than heap.
...
> > + while ((len = BIO_read(request, buf, sizeof buf)) > 0) {
> > + char *wbuf = buf;
> > + apr_size_t remain = len;
> > +
> > + do {
> > + apr_size_t wlen = remain;
> > +
> > + rv = apr_socket_send(sd, wbuf, &wlen);
> > + wbuf += remain;
> > + remain -= wlen;
> > + } while (rv == APR_SUCCESS && remain > 0);
>
> Why do we need remain here and do not use len directly?
Really only to make the types match; len is an int but wlen must be an
apr_size_t.
...
> > + count = 0;
> > + while ((line = get_line(tmpbb, bb, c, p)) != NULL && line[0]
> > + && ++count < MAX_HEADERS) {
> > + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, c,
> > + "OCSP response header: %s", line);
> > + }
>
> Why don't we exit with an error message if count > MAX_HEADERS?
Good catch, will fix.
...
> > + apr_brigade_destroy(bb);
> > + apr_brigade_destroy(tmpbb);
>
> We miss to destroy the brigades in the cases above where we return NULL.
They are allocated out of the pool, so it Shouldn't Matter (TM).
Thanks for the review!
joe