On Thu, 27 Jul 2017 13:10:14 +0200
Jan Klemkow <j.klem...@wemelug.de> wrote:
> Hi Jack,
>
> On Fri, Jul 21, 2017 at 06:33:43PM +0930, Jack Burton wrote:
> > Thoughts?
>
> I've tested your diff. The main feature looks fine to me. TLS
> connections with and with out Clients certs, as well as with and
> without certificate revocation lists seams to work. Also, the tests
> are passing.
Thanks Jan -- that's good to hear.
> But, I found a bug in the part of the FastCGI variables. The
> following condition is always false.
>
> > Index: usr.sbin/httpd/server_fcgi.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/httpd/server_fcgi.c,v
> > retrieving revision 1.74
> > diff -u -p -r1.74 server_fcgi.c
> > --- usr.sbin/httpd/server_fcgi.c 21 Jan 2017 11:32:04
> > -0000 1.74 +++ usr.sbin/httpd/server_fcgi.c 21 Jul
> > 2017 08:25:57 -0000 @@ -282,11 +283,57 @@ server_fcgi(struct httpd
> > *env, struct cl
> ...
> > + if (srv_conf->tls_ca != NULL) {
> ...
That's odd -- I'm not seeing that behaviour here -- in my tests
srv_conf->tls_ca always behaved just as expected (it's NULL iff the "tls
client ca" directive is not given for that server).
Just to show that I'm not hallucinating... here's exactly what I see
when I double check that using my last diff, in each of the three
relevant cases ("no tls" isn't relevant, as that won't ever hit line
291):
1. tls without any tls client directive. condition is false as expected:
Breakpoint 1, server_fcgi (env=0x5ecd5d74400, clt=0x5ec299cf000)
at /usr/src/usr.sbin/httpd/server_fcgi.c:291
291 if (srv_conf->tls_ca != NULL) {
Current language: auto; currently c
(gdb) p srv_conf->tls_ca
$1 = (uint8_t *) 0x0
2. with tls client ca "/etc/ssl/test_ca.crt". condition is true as
expected:
Breakpoint 1, server_fcgi (env=0x114682474800, clt=0x1145e23b3800)
at /usr/src/usr.sbin/httpd/server_fcgi.c:291
291 if (srv_conf->tls_ca != NULL) {
Current language: auto; currently c
(gdb) p srv_conf->tls_ca
$1 = (
uint8_t *) 0x11468a6d9000 "-----BEGIN
CERTIFICATE-----\nMIIF2DCCA8CgAwIBAgICEAAwDQYJKoZIhvcNAQELBQAwfjELMAkGA1UEBhMCQVUx\nGDAWBgNVBAgMD1NvdXRoIEF1c3RyYWxpYTERMA8GA1UEBwwIQWRlbGFpZGUxFzAV\nBgNVBAoMDlNhb3NjZSBQdHkgTHRkMRAwDgYDVQQLDA"...
3. with tls client ca "/etc/ssl/test_ca.crt" crl "/etc/ssl/crl.pem".
condition is true as expected:
Breakpoint 2, server_fcgi (env=0x13cf0dcb3400, clt=0x13cf7981d800)
at /usr/src/usr.sbin/httpd/server_fcgi.c:291
291 if (srv_conf->tls_ca != NULL) {
(gdb) p srv_conf->tls_ca
$8 = (
uint8_t *) 0x13cf64321000 "-----BEGIN
CERTIFICATE-----\nMIIF2DCCA8CgAwIBAgICEAAwDQYJKoZIhvcNAQELBQAwfjELMAkGA1UEBhMCQVUx\nGDAWBgNVBAgMD1NvdXRoIEF1c3RyYWxpYTERMA8GA1UEBwwIQWRlbGFpZGUxFzAV\nBgNVBAoMDlNhb3NjZSBQdHkgTHRkMRAwDgYDVQQLDA"...
...which, in each case, is exactly what I'd expect.
I'm puzzled as to what could be different between your test environment
and mine that gives you different results -- could you provide any more
details?
> It has to be replaced with the following condition:
>
> + if (clt->clt_srv->srv_conf.tls_ca != NULL) {
Why?
srv_conf is initialised to clt->clt_srv_conf at the top of
server_fcgi().
clt->clt_srv_conf gets initialised to srv->srv_conf (which is
exactly what clt->clt_srv->srv_conf ends up pointing to) way back in
server_accept()... and in the absence of anything out of the ordinary
(like a redirect or an error, which would end up taking different code
paths anyway), it remains so.
So why take the more circuitous route to get to the same place?
Happy to be corrected if I'm wrong, but my understanding from reading
the httpd sources was that whenever you have both a struct client and a
struct server in flight, the client's clt_srv_conf and the server's
srv_conf point to the same thing... whichever one you're not
passed gets initialised to the other.
Or have I missed something obvious?
> To make this working I have to change two void pointers by more
> specific types. Below, you find the whole diff:
Well yes, obviously if you're going to deference clt_srv without even
an implicit cast then you'll need to clarify it's definition first, but
I still don't see why we can't just use clt_srv_conf instead -- that
strikes me as exactly the sort of situation it's provided for.
Maybe I'm reading too much into the definition of struct client in
httpd.h here, but when I see two void * members in amongst a long list
of declared member structs, the message I automatically infer from
that is "at some point in the future, there might be more than one
structure that each of these two members can take on" (after all,
why else would you declare some members fully and others opaquely,
in an *internal* header file?)...
...which is why I'd rather not change those two definitions unless it's
*clear* how & why we might end up in server_fcgi() with clt_srv
initialised and clt_srv_conf uninitialised -- a condition I haven't yet
managed to induce here, try as I might.
Checking the cvs history, clt_srv has been a void * since its
introduction in r1.9 (as was its ancestor clt_server since r1.1). It
may well be that any possible original intention for clt_srv to have
multiple types fell by the wayside a long time ago. But again I don't
think that's your or my call to make, unless it's made clear why
clt_srv_conf can't be used (or unless one of the original authors comes
out and says no I've misunderstood their intent, or no that's not
important any more).
Assuming the problem you were seeing is reproducible, can you trace it
back and let me know where clt_srv_conf is getting unset? Or
alternatively share an httpd.conf & sequence of requests that reliably
reproduces the problem?
Many thanks.
