Re: [diff] httpd: tls client cert & CRL checks

[Jack Burton]

On Tue, 8 Aug 2017 15:10:36 +0930
Jack Burton  wrote:
> On Tue, 08 Aug 2017 14:23:02 +1000
> Joel Sing  wrote:
> > On Saturday 29 July 2017 20:49:18 Jan Klemkow wrote:
> > > In the End, I found and fixed the real bug here:
> > > 
> > > @@ -430,7 +438,11 @@ config_getserver_config(struct httpd *en
> > > }
> > > 
> > > f = SRVFLAG_TLS;
> > > -   srv_conf->flags |= parent->flags & f;
> > > +   if ((srv_conf->flags & f) == 0) {
> > > +   srv_conf->flags |= parent->flags & f;
> > > +   srv_conf->tls_ca = parent->tls_ca;
> > > +   srv_conf->tls_crl = parent->tls_crl;
> > 
> > I'd have to double check, however I'm pretty sure that this will
> > result in a double-free since you're copying the pointer (without a
> > reference count) across server config structs. Both will likely be
> > passed to serverconfig_free(), which means free() will then be
> > passed the same pointer twice.  
> 
> You're absolutely right. That happens when purging config on reload,
> if the old config for a server doing tls client verify has two or more
> location blocks...
> 
> ...and my 30 July diff suffers from the same problem.
<...>
> Perhaps a better alternative would be to free the CA & CRL mem at the
> end of server_tls_init() -- like we do already for the key pair --
> then add flags for CA & CRL use instead of just checking for null
> pointers everywhere (as both my & Jan's diffs have been doing so far).
> 
> It seems I have a couple of other changes to make too, so will roll
> something like that in with them & post a revised diff later today or
> tomorrow.

Okay, here's a further revised diff that takes the approach I proposed
above to fixing the bug that Joel pointed out and also makes a few
other small changes that were suggested to me off list:
* the tls client ca directive now has an "optional" option (to request
  a client cert but not require it);
* the extra bit string I got rid off a diff or two ago is back again,
  now passed through to fcgi as TLS_PEER_VERIFY (since there are now
  multiple possible modes of operation for tls client certs, so fcgi
  responders should have some way of knowing which are in use);
* reworded the descriptions of the TLS_PEER_* fcgi vars in
  httpd.conf(5); and
* fixed a minor typo in httpd.conf(5) that my earlier diff
  had introduced.

Thoughts?

Index: usr.sbin/httpd/config.c
===
RCS file: /cvs/src/usr.sbin/httpd/config.c,v
retrieving revision 1.53
diff -u -p -r1.53 config.c
--- usr.sbin/httpd/config.c 19 Jul 2017 17:36:25 -  1.53
+++ usr.sbin/httpd/config.c 8 Aug 2017 12:36:19 -
@@ -304,10 +304,18 @@ config_setserver_tls(struct httpd *env, 
 
log_debug("%s: configuring tls for %s", __func__, srv_conf->name);
 
+   if (config_settls(env, srv, TLS_CFG_CA, "ca", srv_conf->tls_ca,
+   srv_conf->tls_ca_len) != 0)
+   return (-1);
+
if (config_settls(env, srv, TLS_CFG_CERT, "cert", srv_conf->tls_cert,
srv_conf->tls_cert_len) != 0)
return (-1);
 
+   if (config_settls(env, srv, TLS_CFG_CRL, "crl", srv_conf->tls_crl,
+   srv_conf->tls_crl_len) != 0)
+   return (-1);
+
if (config_settls(env, srv, TLS_CFG_KEY, "key", srv_conf->tls_key,
srv_conf->tls_key_len) != 0)
return (-1);
@@ -431,6 +439,7 @@ config_getserver_config(struct httpd *en
 
f = SRVFLAG_TLS;
srv_conf->flags |= parent->flags & f;
+   srv_conf->tlsflags = parent->tlsflags;
 
f = SRVFLAG_ACCESS_LOG;
if ((srv_conf->flags & f) == 0) {
@@ -655,9 +664,21 @@ config_getserver_tls(struct httpd *env, 
}
 
switch (tls_conf.tls_type) {
+   case TLS_CFG_CA:
+   if (config_gettls(env, srv_conf, _conf, "ca", p, len,
+   _conf->tls_ca, _conf->tls_ca_len) != 0)
+   goto fail;
+   break;
+
case TLS_CFG_CERT:
if (config_gettls(env, srv_conf, _conf, "cert", p, len,
_conf->tls_cert, _conf->tls_cert_len) != 0)
+   goto fail;
+   break;
+
+   case TLS_CFG_CRL:
+   if (config_gettls(env, srv_conf, _conf, "crl", p, len,
+   _conf->tls_crl, _conf->tls_crl_len) != 0)
goto fail;
break;
 
Index: usr.sbin/httpd/httpd.conf.5
===
RCS file: /cvs/src/usr.sbin/httpd/httpd.conf.5,v
retrieving revision 1.82
diff -u -p -r1.82 httpd.conf.5
--- usr.sbin/httpd/httpd.conf.5 9 Apr 2017 09:13:28 -   1.82
+++ usr.sbin/httpd/httpd.conf.5 8 Aug 2017 12:36:19 -
@@ -342,7 +342,37 @@ The revision of the HTTP specification u
 .It Ic SERVER_SOFTWARE

Re: [diff] httpd: tls client cert & CRL checks

[Jack Burton]

On Tue, 08 Aug 2017 14:23:02 +1000
Joel Sing  wrote:
> On Saturday 29 July 2017 20:49:18 Jan Klemkow wrote:
> > Hi Jack,
> > 
> > On Fri, Jul 28, 2017 at 02:05:34AM +0930, Jack Burton wrote:  
> > > On Thu, 27 Jul 2017 13:10:14 +0200
> > >   
> > > > 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
> > > > > - 1.74 +++ usr.sbin/httpd/server_fcgi.c   21
> > > > > Jul 2017 08:25:57 - @@ -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).  
> > 
> > In the End, I found and fixed the real bug here:
> > 
> > @@ -430,7 +438,11 @@ config_getserver_config(struct httpd *en
> > }
> > 
> > f = SRVFLAG_TLS;
> > -   srv_conf->flags |= parent->flags & f;
> > +   if ((srv_conf->flags & f) == 0) {
> > +   srv_conf->flags |= parent->flags & f;
> > +   srv_conf->tls_ca = parent->tls_ca;
> > +   srv_conf->tls_crl = parent->tls_crl;  
> 
> I'd have to double check, however I'm pretty sure that this will
> result in a double-free since you're copying the pointer (without a
> reference count) across server config structs. Both will likely be
> passed to serverconfig_free(), which means free() will then be passed
> the same pointer twice.

You're absolutely right. That happens when purging config on reload,
if the old config for a server doing tls client verify has two or more
location blocks...

...and my 30 July diff suffers from the same problem.

Didn't show up in my testing, as initially I used no location blocks
and after Jan's problem report only used one location block per
server ... and server_purge() already makes sure it doesn't free the
default location twice.

I don't think it makes much sense to copy the contents of tls_ca &
tls_crl for each location, since they'll always be identical within
their common parent server.

So that leaves as obvious alternatives either reference counting (as you
alluded to above) or dereferencing only via the parent struct (as Jan's
earlier diff did).

I'm not a fan of the latter, as it's likely to cause confusion
later on (won't be obvious why we're not using the seemingly identical
member of the struct we're handed) ... and the former probably adds
more complexity than we really need.

Perhaps a better alternative would be to free the CA & CRL mem at the
end of server_tls_init() -- like we do already for the key pair -- then
add flags for CA & CRL use instead of just checking for null pointers
everywhere (as both my & Jan's diffs have been doing so far).

It seems I have a couple of other changes to make too, so will roll
something like that in with them & post a revised diff later today or
tomorrow.

Thanks for spotting this Joel.

Re: [diff] httpd: tls client cert & CRL checks

[Joel Sing]

On Saturday 29 July 2017 20:49:18 Jan Klemkow wrote:
> Hi Jack,
> 
> On Fri, Jul 28, 2017 at 02:05:34AM +0930, Jack Burton wrote:
> > On Thu, 27 Jul 2017 13:10:14 +0200
> > 
> > > 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.c21 Jan 2017 11:32:04
> > > > -   1.74 +++ usr.sbin/httpd/server_fcgi.c   21 Jul
> > > > 2017 08:25:57 - @@ -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).
> 
> In the End, I found and fixed the real bug here:
> 
> @@ -430,7 +438,11 @@ config_getserver_config(struct httpd *en
> }
> 
> f = SRVFLAG_TLS;
> -   srv_conf->flags |= parent->flags & f;
> +   if ((srv_conf->flags & f) == 0) {
> +   srv_conf->flags |= parent->flags & f;
> +   srv_conf->tls_ca = parent->tls_ca;
> +   srv_conf->tls_crl = parent->tls_crl;

I'd have to double check, however I'm pretty sure that this will result in a 
double-free since you're copying the pointer (without a reference count) 
across server config structs. Both will likely be passed to 
serverconfig_free(), 
which means free() will then be passed the same pointer twice.

> +   }
> 
> f = SRVFLAG_ACCESS_LOG;
> if ((srv_conf->flags & f) == 0) {
> 
> This additional copy fixes the bug I have seen by this config:
> 
> server "default" {
> listen on 127.0.0.1 tls port 443
> 
> # TLS certificate and key files created with acme-client(1)
> tls certificate "/root/ca/server.crt"
> tls key "/root/ca/server.key"
> #tls client ca "/root/ca/ca.crt" crl "/root/ca/ca.crl"
> tls client ca "/root/ca/ca.crt"
> 
> location "*.cgi" {
> fastcgi
> root "/var/www/cgi-bin/env.cgi"
> }
> 
> root "/htdocs/"
> }
> 
> You find the whole diff below.
> I tested:
>  - TLS without client certs
>  - TLS with client certs and without CRL
>  - TLS with client certs and with CRL
>  - as well as environment variables in CGI-Scripts
> 
> Everything should work now.
> 
> Bye,
> Jan

Re: [diff] httpd: tls client cert & CRL checks

[Jack Burton]

On Sat, 29 Jul 2017 20:49:18 +0200
Jan Klemkow  wrote:
> On Fri, Jul 28, 2017 at 02:05:34AM +0930, Jack Burton wrote:
> > On Thu, 27 Jul 2017 13:10:14 +0200
> >   
> > > 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.c21 Jan 2017 11:32:04
> > > > -   1.74 +++ usr.sbin/httpd/server_fcgi.c   21 Jul
> > > > 2017 08:25:57 - @@ -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).  
> 
> In the End, I found and fixed the real bug here:
> 
> @@ -430,7 +438,11 @@ config_getserver_config(struct httpd *en
> }
> 
> f = SRVFLAG_TLS;
> -   srv_conf->flags |= parent->flags & f;
> +   if ((srv_conf->flags & f) == 0) {
> +   srv_conf->flags |= parent->flags & f;
> +   srv_conf->tls_ca = parent->tls_ca;
> +   srv_conf->tls_crl = parent->tls_crl;
> +   }
> 
> f = SRVFLAG_ACCESS_LOG;
> if ((srv_conf->flags & f) == 0) {

Thanks Jan -- well caught!

I didn't notice that.

Perhaps we also need to change this comment then:

/*
 * Get variable-length values for the virtual host.  The tls_*
ones
 * aren't needed in the virtual hosts unless we implement SNI.
 */

...since at least two of the tls_* ones *are* needed when there's a
location block involved...

...and SNI *has* been implemented (by jsing@ last year) without needing
to make any changes to config_getserver_config().

> This additional copy fixes the bug I have seen by this config:
> 
> server "default" {
> listen on 127.0.0.1 tls port 443
> 
> # TLS certificate and key files created with acme-client(1)
> tls certificate "/root/ca/server.crt"
> tls key "/root/ca/server.key"
> #tls client ca "/root/ca/ca.crt" crl "/root/ca/ca.crl"
> tls client ca "/root/ca/ca.crt"
> 
> location "*.cgi" {
> fastcgi
> root "/var/www/cgi-bin/env.cgi"
> }
> 
> root "/htdocs/"
> }

Thanks. Yes that, together with your short diff above, helps me
understand exactly why I wasn't seeing the issue that you were: clearly
the problem was in the extra srv_conf instance created by the location
block.

Where that gets created in parse.y there's no reference to tls_* at
all (presumably for the same reason as given by the comment in
config.c), which might explain why I missed it in my first cut back
in March...

...well, that and none of my tests involved location blocks (force of
habit -- in production I tend to keep configs as constant as possible
within each server for simplicity's sake, so don't often need to use
location blocks).

Still, it's something I should have tested but didn't.

Thanks for finding & fixing it.

> You find the whole diff below.
> I tested:
>  - TLS without client certs
>  - TLS with client certs and without CRL
>  - TLS with client certs and with CRL
>  - as well as environment variables in CGI-Scripts
> 
> Everything should work now.

It does, but I'm going to suggest one more set of tweaks...

<...>
> Index: usr.sbin/httpd/httpd.h
> ===
> RCS file: /mount/openbsd/cvs/src/usr.sbin/httpd/httpd.h,v
> retrieving revision 1.133
> diff -u -p -r1.133 httpd.h
> --- usr.sbin/httpd/httpd.h19 Jul 2017 17:36:25 -
> 1.133 +++ usr.sbin/httpd/httpd.h  28 Jul 2017 11:28:25 -
> @@ -324,8 +324,8 @@ struct range_data {
>  struct client {
>   uint32_t clt_id;
>   pid_tclt_pid;
> - void*clt_srv;
> - void*clt_srv_conf;
> + struct server   *clt_srv;
> + struct server_config*clt_srv_conf;

Those declaration changes aren't necessary. The code changes in the
relevant part of your diff consist only of two assignment statements,
where the implicit casts are obvious.

Because I don't *know* the original authors' intentions re planning
for other possible types of config structure in future (see my last
post), I'm suggesting we leave those declarations as-is for now.

If it turns out that they *do* need changing, I'd suggest that might be
better done in a separate commit, with its own message explaining why
(as that change isn't 

Re: [diff] httpd: tls client cert & CRL checks

[Jan Klemkow]

Hi Jack,

On Fri, Jul 28, 2017 at 02:05:34AM +0930, Jack Burton wrote:
> On Thu, 27 Jul 2017 13:10:14 +0200
> 
> > 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
> > > - 1.74 +++ usr.sbin/httpd/server_fcgi.c   21 Jul
> > > 2017 08:25:57 - @@ -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).

In the End, I found and fixed the real bug here:

@@ -430,7 +438,11 @@ config_getserver_config(struct httpd *en
}

f = SRVFLAG_TLS;
-   srv_conf->flags |= parent->flags & f;
+   if ((srv_conf->flags & f) == 0) {
+   srv_conf->flags |= parent->flags & f;
+   srv_conf->tls_ca = parent->tls_ca;
+   srv_conf->tls_crl = parent->tls_crl;
+   }

f = SRVFLAG_ACCESS_LOG;
if ((srv_conf->flags & f) == 0) {

This additional copy fixes the bug I have seen by this config:

server "default" {
listen on 127.0.0.1 tls port 443

# TLS certificate and key files created with acme-client(1)
tls certificate "/root/ca/server.crt"
tls key "/root/ca/server.key"
#tls client ca "/root/ca/ca.crt" crl "/root/ca/ca.crl"
tls client ca "/root/ca/ca.crt"

location "*.cgi" {
fastcgi
root "/var/www/cgi-bin/env.cgi"
}

root "/htdocs/"
}

You find the whole diff below.
I tested:
 - TLS without client certs
 - TLS with client certs and without CRL
 - TLS with client certs and with CRL
 - as well as environment variables in CGI-Scripts

Everything should work now.

Bye,
Jan

Index: usr.sbin/httpd/config.c
===
RCS file: /mount/openbsd/cvs/src/usr.sbin/httpd/config.c,v
retrieving revision 1.53
diff -u -p -r1.53 config.c
--- usr.sbin/httpd/config.c 19 Jul 2017 17:36:25 -  1.53
+++ usr.sbin/httpd/config.c 29 Jul 2017 18:14:36 -
@@ -304,10 +304,18 @@ config_setserver_tls(struct httpd *env, 
 
log_debug("%s: configuring tls for %s", __func__, srv_conf->name);
 
+   if (config_settls(env, srv, TLS_CFG_CA, "ca", srv_conf->tls_ca,
+   srv_conf->tls_ca_len) != 0)
+   return (-1);
+
if (config_settls(env, srv, TLS_CFG_CERT, "cert", srv_conf->tls_cert,
srv_conf->tls_cert_len) != 0)
return (-1);
 
+   if (config_settls(env, srv, TLS_CFG_CRL, "crl", srv_conf->tls_crl,
+   srv_conf->tls_crl_len) != 0)
+   return (-1);
+
if (config_settls(env, srv, TLS_CFG_KEY, "key", srv_conf->tls_key,
srv_conf->tls_key_len) != 0)
return (-1);
@@ -430,7 +438,11 @@ config_getserver_config(struct httpd *en
}
 
f = SRVFLAG_TLS;
-   srv_conf->flags |= parent->flags & f;
+   if ((srv_conf->flags & f) == 0) {
+   srv_conf->flags |= parent->flags & f;
+   srv_conf->tls_ca = parent->tls_ca;
+   srv_conf->tls_crl = parent->tls_crl;
+   }
 
f = SRVFLAG_ACCESS_LOG;
if ((srv_conf->flags & f) == 0) {
@@ -655,9 +667,21 @@ config_getserver_tls(struct httpd *env, 
}
 
switch (tls_conf.tls_type) {
+   case TLS_CFG_CA:
+   if (config_gettls(env, srv_conf, _conf, "ca", p, len,
+   _conf->tls_ca, _conf->tls_ca_len) != 0)
+   goto fail;
+   break;
+
case TLS_CFG_CERT:
if (config_gettls(env, srv_conf, _conf, "cert", p, len,
_conf->tls_cert, _conf->tls_cert_len) != 0)
+   goto fail;
+   break;
+
+   case TLS_CFG_CRL:
+   if (config_gettls(env, srv_conf, _conf, "crl", p, len,
+   _conf->tls_crl, _conf->tls_crl_len) != 0)
goto fail;
break;
 
Index: usr.sbin/httpd/httpd.conf.5
===
RCS file: /mount/openbsd/cvs/src/usr.sbin/httpd/httpd.conf.5,v
retrieving revision 1.82
diff -u -p -r1.82 httpd.conf.5
--- usr.sbin/httpd/httpd.conf.5 9 Apr 2017 09:13:28 -   1.82
+++ usr.sbin/httpd/httpd.conf.5 28 Jul 2017 11:28:25 -
@@ -342,6 +342,28 @@ The revision of 

Re: [diff] httpd: tls client cert & CRL checks

[Jack Burton]

On Thu, 27 Jul 2017 13:10:14 +0200
Jan Klemkow  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.c21 Jan 2017 11:32:04
> > -   1.74 +++ usr.sbin/httpd/server_fcgi.c   21 Jul
> > 2017 08:25:57 - @@ -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 

Re: [diff] httpd: tls client cert & CRL checks

[Jan Klemkow]

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.

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 -  1.74
> +++ usr.sbin/httpd/server_fcgi.c  21 Jul 2017 08:25:57 -
> @@ -282,11 +283,57 @@ server_fcgi(struct httpd *env, struct cl
...
> + if (srv_conf->tls_ca != NULL) {
...

It has to be replaced with the following condition:

+   if (clt->clt_srv->srv_conf.tls_ca != NULL) {

To make this working I have to change two void pointers by more specific
types.  Below, you find the whole diff:

bye,
Jan

Index: regress/usr.sbin/httpd/tests/Client.pm
===
RCS file: /mount/openbsd/cvs/src/regress/usr.sbin/httpd/tests/Client.pm,v
retrieving revision 1.1
diff -u -p -r1.1 Client.pm
--- regress/usr.sbin/httpd/tests/Client.pm  16 Jul 2015 16:35:57 -  
1.1
+++ regress/usr.sbin/httpd/tests/Client.pm  21 Jul 2017 11:20:24 -
@@ -59,6 +59,11 @@ sub child {
PeerAddr=> $self->{connectaddr},
PeerPort=> $self->{connectport},
SSL_verify_mode => SSL_VERIFY_NONE,
+   SSL_use_cert=> $self->{offertlscert} ? 1 : 0,
+   SSL_cert_file   => $self->{offertlscert} ?
+   $self->{chroot}."/client.crt" : "",
+   SSL_key_file=> $self->{offertlscert} ?
+   $self->{chroot}."/client.key" : "",
) or die ref($self), " $iosocket socket connect failed: $!,$SSL_ERROR";
print STDERR "connect sock: ",$cs->sockhost()," ",$cs->sockport(),"\n";
print STDERR "connect peer: ",$cs->peerhost()," ",$cs->peerport(),"\n";
Index: regress/usr.sbin/httpd/tests/Httpd.pm
===
RCS file: /mount/openbsd/cvs/src/regress/usr.sbin/httpd/tests/Httpd.pm,v
retrieving revision 1.2
diff -u -p -r1.2 Httpd.pm
--- regress/usr.sbin/httpd/tests/Httpd.pm   30 Jan 2017 21:18:24 -  
1.2
+++ regress/usr.sbin/httpd/tests/Httpd.pm   21 Jul 2017 11:20:24 -
@@ -72,6 +72,8 @@ sub new {
print $fh "\n";
print $fh "\ttls certificate \"".$args{chroot}."/server.crt\"\n";
print $fh "\ttls key \"".$args{chroot}."/server.key\"";
+   $self->{verifytls}
+   and print $fh "\ntls client ca \"".$args{chroot}."/ca.crt\"";
}
print $fh "\n\troot \"/\"";
print $fh "\n\tlog style combined";
Index: regress/usr.sbin/httpd/tests/Makefile
===
RCS file: /mount/openbsd/cvs/src/regress/usr.sbin/httpd/tests/Makefile,v
retrieving revision 1.8
diff -u -p -r1.8 Makefile
--- regress/usr.sbin/httpd/tests/Makefile   14 Jul 2017 13:31:44 -  
1.8
+++ regress/usr.sbin/httpd/tests/Makefile   21 Jul 2017 11:20:24 -
@@ -77,10 +77,16 @@ ca.crt:
 server.req:
openssl req -batch -new -subj 
/L=OpenBSD/O=httpd-regress/OU=server/CN=localhost/ -nodes -newkey rsa -keyout 
server.key -out server.req
 
+client.req:
+   openssl req -batch -new -subj 
/L=OpenBSD/O=httpd-regress/OU=client/CN=localhost/ -nodes -newkey rsa -keyout 
client.key -out $@
+
 server.crt: ca.crt server.req
openssl x509 -CAcreateserial -CAkey ca.key -CA ca.crt -req -in 
server.req -out server.crt
 
-${REGRESS_TARGETS:M*tls*} ${REGRESS_TARGETS:M*https*}: server.crt
+client.crt: ca.crt client.req
+   openssl x509 -CAcreateserial -CAkey ca.key -CA ca.crt -req -in 
client.req -out $@
+
+${REGRESS_TARGETS:M*tls*} ${REGRESS_TARGETS:M*https*}: server.crt client.crt
 
 # make perl syntax check for all args files
 
Index: usr.sbin/httpd/config.c
===
RCS file: /mount/openbsd/cvs/src/usr.sbin/httpd/config.c,v
retrieving revision 1.53
diff -u -p -r1.53 config.c
--- usr.sbin/httpd/config.c 19 Jul 2017 17:36:25 -  1.53
+++ usr.sbin/httpd/config.c 21 Jul 2017 11:20:24 -
@@ -304,10 +304,18 @@ config_setserver_tls(struct httpd *env, 
 
log_debug("%s: configuring tls for %s", __func__, srv_conf->name);
 
+   if (config_settls(env, srv, TLS_CFG_CA, "ca", srv_conf->tls_ca,
+   srv_conf->tls_ca_len) != 0)
+   return (-1);
+
if (config_settls(env, srv, TLS_CFG_CERT, "cert", srv_conf->tls_cert,