On Mon, Oct 26, 2009 at 12:44:17AM +0100, Ruediger Pluem wrote:
> On 10/25/2009 06:21 PM, [email protected] wrote:
> > Author: jorton
> > Date: Sun Oct 25 17:21:10 2009
> > New Revision: 829619
> >
> > URL: http://svn.apache.org/viewvc?rev=829619&view=rev
> > Log:
> > Add support for OCSP "stapling":
...
> Will there be documentation patches for the new directives?
Yes, I will try to get that done, I might not have time until after
ApacheCon though.
Thanks greatly for the review!
> > + timeout += time(NULL);
>
> Shouldn't we use apr_time_now here?
Yup, fixed in r830551.
> > +
> > + i2d_OCSP_RESPONSE(rsp, &p);
>
> Is this correct? p is already char *. So &p would be char **.
> If p gets changed by i2d_OCSP_RESPONSE our flag set above would get lost ???
This is correct. The preceding lines:
p = resp_der;
if (ok == TRUE) {
*p++ = 1;
timeout = mctx->stapling_cache_timeout;
}
else {
*p++ = 0;
timeout = mctx->stapling_errcache_timeout;
}
set the flag value in the first byte of the data block. The i2d_*
macros all take a char ** pointer, and move the pointer to the next byte
after the serialized data. (see e.g. the man page for i2d_X509)
> > + p++;
> > + resp_derlen--;
> > + rsp = d2i_OCSP_RESPONSE(NULL, &p, resp_derlen);
>
> Why &p and not p?
As above, the d2i_ macros also take a char ** pointer and move it to the
first byte after the encoded OCSP_RESPONSE.
...
> I guess we are missing some indention here.
Sorry about that - Guenter has fixed it all, thanks Guenter!
> > +done:
> > + if (id)
> > + OCSP_CERTID_free(id);
> > + if (req)
> > + OCSP_REQUEST_free(req);
> > + return rv;
> > +err:
> > + rv = FALSE;
> > + goto done;
>
> Looks a little bit like spaghetti code :-).
It's a bit annoying; it could be done using pool cleanups but that adds
significant complexity, or by in-lining the _free calls everywhere,
which is a bit error-pront. Not really sure either alternative is
better. C sucks ;)
> > + * Check for cached responses in session cache. If valid send back to
> > + * client. If absent or no longer valid query responder and update
> > + * cache. */
> > +static int stapling_cb(SSL *ssl, void *arg)
> > +{
> > + conn_rec *conn = (conn_rec *)SSL_get_app_data(ssl);
> > + server_rec *s = conn->base_server;
>
> Shouldn't we use mySrvFromConn(conn) here instead of conn->base_server?
Yup, fixed in r830546.
Thanks again!
Regards, Joe
