Re: [patch] httpd: add tls client certificate authentication

2018-05-19 Thread Joel Sing
On Wednesday 16 May 2018 17:32:56 Jack Burton wrote:
> My attempts to get this accepted last year stalled.
> 
> As best I recall, the main stumbling block was agreeing on how much /
> exactly which client cert data to pass through to fastcgi (my view was
> that passing the whole client cert chain would be ideal).
> 
> So, here's a stripped down version that passes *no* client cert
> data through to fastcgi at all (but still passes the relevant status
> flags in TLS_PEER_VERIFY).
> 
> This diff provides everything necessary for tls client *authentication*,
> but *none* of what's required to use tls client certs for authorisation
> or accounting.
> 
> I figured that if we can agree on this much, so httpd can be used for
> the authentication-only case (which is all non-fastcgi sites would want)
> straight away, that's be a good first step -- then we can come back and
> argue the toss over how much client cert data is necessary/sufficient
> to pass through for authorisation/accounting purposes.
> 
> There's also a trivial regression test (unchanged from last year), which
> I'll post again separately next.

Thanks - I've just committed this, with some minor tweaks and minus one chunk:

> Index: server.c
> ===
> RCS file: /cvs/src/usr.sbin/httpd/server.c,v
> retrieving revision 1.113
> diff -u -p -r1.113 server.c
> --- server.c  29 Nov 2017 16:55:08 -  1.113
> +++ server.c  16 May 2018 07:59:11 -
> @@ -264,6 +300,27 @@ server_tls_init(struct server *srv)
>   return (-1);
>   }
> 
> + if (srv->srv_conf.tls_ca != NULL) {
> + if (tls_config_set_ca_mem(srv->srv_tls_config,
> + srv->srv_conf.tls_ca, srv->srv_conf.tls_ca_len) != 0) {
> + log_warnx("%s: failed to add ca cert(s)", __func__);
> + return (-1);
> + }
> + if (srv->srv_conf.tls_flags & TLSFLAG_OPTIONAL)
> + tls_config_verify_client_optional(srv->srv_tls_config);
> + else
> + tls_config_verify_client(srv->srv_tls_config);
> +
> + if (srv->srv_conf.tls_crl != NULL) {
> + if (tls_config_set_crl_mem(srv->srv_tls_config,
> + srv->srv_conf.tls_crl,
> + srv->srv_conf.tls_crl_len) != 0) {
> + log_warnx("%s: failed to add crl(s)", __func__);
> + return (-1);
> + }
> + }
> + }
> +
>   TAILQ_FOREACH(srv_conf, &srv->srv_hosts, entry) {
>   if (srv_conf->tls_cert == NULL || srv_conf->tls_key == NULL)
>   continue;
> @@ -277,6 +334,26 @@ server_tls_init(struct server *srv)
>   log_warnx("%s: failed to add tls keypair", __func__);
>   return (-1);
>   }
> +
> + if (srv->srv_conf.tls_ca == NULL)
> + continue;
> + log_debug("%s: adding ca cert(s) for server %s", __func__,
> + srv->srv_conf.name);
> + if (tls_config_set_ca_mem(srv->srv_tls_config,
> + srv_conf->tls_ca, srv_conf->tls_ca_len) != 0) {
> + log_warnx("%s: failed to add ca cert(s)", __func__);
> + return (-1);
> + }
> +
> + if (srv->srv_conf.tls_crl == NULL)
> + continue;
> +
> + log_debug("%s: adding crl(s) for server %s", __func__,
> + srv->srv_conf.name);
> + if (tls_config_set_crl_mem(srv->srv_tls_config,
> + srv_conf->tls_crl, srv_conf->tls_crl_len) != 0) {
> + return (-1);
> + }
>   }
> 
>   /* set common session ID among all processes */

The above chunk does not make sense, since in the case of multiple httpd 
servers configured on the same port, we'll just keep on setting the CA and CRL 
which overwrites the one set in the previous chunk (which means the last 
configured CA and CRL win). The SNI support in libtls does not currently allow 
for multiple CAs/CRLs to be provided - if we wanted to support this we'd need 
to add support their first. For the time being we should add the CA and CRL 
configuration to the server_tls_cmp() check so that we force it to be identical 
across HTTPS servers configured on the same address/port.



Re: [patch] httpd: add tls client certificate authentication

2018-05-16 Thread Reyk Floeter
It will! The diff is nice and OK.

> Am 16.05.2018 um 22:33 schrieb Jan Klemkow :
> 
> Hi Jack,
> 
>> On Wed, May 16, 2018 at 05:32:56PM +0930, Jack Burton wrote:
>> I figured that if we can agree on this much, so httpd can be used for
>> the authentication-only case (which is all non-fastcgi sites would want)
>> straight away, that's be a good first step -- then we can come back and
>> argue the toss over how much client cert data is necessary/sufficient
>> to pass through for authorisation/accounting purposes.
> 
> I agree with you.  Lets commit this first step.
> 
> I tested your diff on current amd64 and sparc64 with an own PKI [1] and
> the firefox browser.  Every thing works for me.  I also looked down your
> source code which also seams fine to me.
> 
> I tested the optional and non-optional "client ca" configuration, as
> well as the revocation and the fastcgi environment feature on both
> architectures.  Everything works so far.
> 
> Hopefully, it will committed this time! :-)
> 
> Thanks!
> Jan
> 
> [1]: https://github.com/younix/ca
> 
>> There's also a trivial regression test (unchanged from last year), which
>> I'll post again separately next.
>> 
>> 
>> Index: config.c
>> ===
>> RCS file: /cvs/src/usr.sbin/httpd/config.c,v
>> retrieving revision 1.53
>> diff -u -p -r1.53 config.c
>> --- config.c19 Jul 2017 17:36:25 -1.53
>> +++ config.c16 May 2018 07:59:10 -
>> @@ -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->tls_flags = parent->tls_flags;
>> 
>>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, &tls_conf, "ca", p, len,
>> +&srv_conf->tls_ca, &srv_conf->tls_ca_len) != 0)
>> +goto fail;
>> +break;
>> +
>>case TLS_CFG_CERT:
>>if (config_gettls(env, srv_conf, &tls_conf, "cert", p, len,
>>&srv_conf->tls_cert, &srv_conf->tls_cert_len) != 0)
>> +goto fail;
>> +break;
>> +
>> +case TLS_CFG_CRL:
>> +if (config_gettls(env, srv_conf, &tls_conf, "crl", p, len,
>> +&srv_conf->tls_crl, &srv_conf->tls_crl_len) != 0)
>>goto fail;
>>break;
>> 
>> Index: httpd.conf.5
>> ===
>> RCS file: /cvs/src/usr.sbin/httpd/httpd.conf.5,v
>> retrieving revision 1.90
>> diff -u -p -r1.90 httpd.conf.5
>> --- httpd.conf.511 Apr 2018 15:50:46 -1.90
>> +++ httpd.conf.516 May 2018 07:59:10 -
>> @@ -342,6 +342,10 @@ The revision of the HTTP specification u
>> .It Ic SERVER_SOFTWARE
>> The server software name of
>> .Xr httpd 8 .
>> +.It Ic TLS_PEER_VERIFY
>> +A variable that is set to a comma separated list of TLS client verification
>> +features in use
>> +.Pq omitted when TLS client verification is not in use .
>> .El
>> .It Ic hsts Oo Ar option Oc
>> Enable HTTP Strict Transport Security.
>> @@ -526,6 +530,23 @@ will be used (strong crypto cipher suite
>> See the CIPHERS section of
>> .Xr openssl 1
>> for information about SSL/TLS cipher suites and preference lists.
>> +.It Ic client ca Ar cafile Oo Ic crl Ar crlfile Oc Op Ic optional
>> +Require
>> +.Po
>> +or, if
>> +.Ic optional
>> +is specified, request but do not require
>> +.Pc
>> +TLS client certificates whose authenticity can be verified
>> +against the CA certificate(s) in
>> +.Ar cafile
>> +in order to proceed beyond the TLS handshake.
>> +With
>> +.Ic crl
>> +specified, additionally require that no certificate in the client chain be
>> +listed as revoked in the CRL(s) in
>> +.Ar crlfile .
>> +CA certificates and CRLs should be PEM encoded.
>> .It Ic dhe Ar params
>> Specify the DHE parameters to use for DHE cipher suites.
>> Valid parameter values are none, legacy and auto.
>> Index: httpd.h
>> ===
>> RCS file: /cvs/src/usr.sbin/httpd/httpd.h,v
>> retrieving revision 1.136
>> diff -u -p -r1.136 httpd.h
>> ---

Re: [patch] httpd: add tls client certificate authentication

2018-05-16 Thread Jan Klemkow
Hi Jack,

On Wed, May 16, 2018 at 05:32:56PM +0930, Jack Burton wrote:
> I figured that if we can agree on this much, so httpd can be used for
> the authentication-only case (which is all non-fastcgi sites would want)
> straight away, that's be a good first step -- then we can come back and
> argue the toss over how much client cert data is necessary/sufficient
> to pass through for authorisation/accounting purposes.

I agree with you.  Lets commit this first step.

I tested your diff on current amd64 and sparc64 with an own PKI [1] and
the firefox browser.  Every thing works for me.  I also looked down your
source code which also seams fine to me.

I tested the optional and non-optional "client ca" configuration, as
well as the revocation and the fastcgi environment feature on both
architectures.  Everything works so far.

Hopefully, it will committed this time! :-)

Thanks!
Jan

[1]: https://github.com/younix/ca

> There's also a trivial regression test (unchanged from last year), which
> I'll post again separately next.
> 
> 
> Index: config.c
> ===
> RCS file: /cvs/src/usr.sbin/httpd/config.c,v
> retrieving revision 1.53
> diff -u -p -r1.53 config.c
> --- config.c  19 Jul 2017 17:36:25 -  1.53
> +++ config.c  16 May 2018 07:59:10 -
> @@ -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->tls_flags = parent->tls_flags;
>  
>   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, &tls_conf, "ca", p, len,
> + &srv_conf->tls_ca, &srv_conf->tls_ca_len) != 0)
> + goto fail;
> + break;
> +
>   case TLS_CFG_CERT:
>   if (config_gettls(env, srv_conf, &tls_conf, "cert", p, len,
>   &srv_conf->tls_cert, &srv_conf->tls_cert_len) != 0)
> + goto fail;
> + break;
> +
> + case TLS_CFG_CRL:
> + if (config_gettls(env, srv_conf, &tls_conf, "crl", p, len,
> + &srv_conf->tls_crl, &srv_conf->tls_crl_len) != 0)
>   goto fail;
>   break;
>  
> Index: httpd.conf.5
> ===
> RCS file: /cvs/src/usr.sbin/httpd/httpd.conf.5,v
> retrieving revision 1.90
> diff -u -p -r1.90 httpd.conf.5
> --- httpd.conf.5  11 Apr 2018 15:50:46 -  1.90
> +++ httpd.conf.5  16 May 2018 07:59:10 -
> @@ -342,6 +342,10 @@ The revision of the HTTP specification u
>  .It Ic SERVER_SOFTWARE
>  The server software name of
>  .Xr httpd 8 .
> +.It Ic TLS_PEER_VERIFY
> +A variable that is set to a comma separated list of TLS client verification
> +features in use
> +.Pq omitted when TLS client verification is not in use .
>  .El
>  .It Ic hsts Oo Ar option Oc
>  Enable HTTP Strict Transport Security.
> @@ -526,6 +530,23 @@ will be used (strong crypto cipher suite
>  See the CIPHERS section of
>  .Xr openssl 1
>  for information about SSL/TLS cipher suites and preference lists.
> +.It Ic client ca Ar cafile Oo Ic crl Ar crlfile Oc Op Ic optional
> +Require
> +.Po
> +or, if
> +.Ic optional
> +is specified, request but do not require
> +.Pc
> +TLS client certificates whose authenticity can be verified
> +against the CA certificate(s) in
> +.Ar cafile
> +in order to proceed beyond the TLS handshake.
> +With
> +.Ic crl
> +specified, additionally require that no certificate in the client chain be
> +listed as revoked in the CRL(s) in
> +.Ar crlfile .
> +CA certificates and CRLs should be PEM encoded.
>  .It Ic dhe Ar params
>  Specify the DHE parameters to use for DHE cipher suites.
>  Valid parameter values are none, legacy and auto.
> Index: httpd.h
> ===
> RCS file: /cvs/src/usr.sbin/httpd/httpd.h,v
> retrieving revision 1.136
> diff -u -p -r1.136 httpd.h
> --- httpd.h   11 Apr 2018 15:50:46 -  1.136
> 

[patch] httpd: add tls client certificate authentication

2018-05-16 Thread Jack Burton
My attempts to get this accepted last year stalled.

As best I recall, the main stumbling block was agreeing on how much /
exactly which client cert data to pass through to fastcgi (my view was
that passing the whole client cert chain would be ideal).

So, here's a stripped down version that passes *no* client cert
data through to fastcgi at all (but still passes the relevant status
flags in TLS_PEER_VERIFY).

This diff provides everything necessary for tls client *authentication*,
but *none* of what's required to use tls client certs for authorisation
or accounting.

I figured that if we can agree on this much, so httpd can be used for
the authentication-only case (which is all non-fastcgi sites would want)
straight away, that's be a good first step -- then we can come back and
argue the toss over how much client cert data is necessary/sufficient
to pass through for authorisation/accounting purposes.

There's also a trivial regression test (unchanged from last year), which
I'll post again separately next.


Index: config.c
===
RCS file: /cvs/src/usr.sbin/httpd/config.c,v
retrieving revision 1.53
diff -u -p -r1.53 config.c
--- config.c19 Jul 2017 17:36:25 -  1.53
+++ config.c16 May 2018 07:59:10 -
@@ -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->tls_flags = parent->tls_flags;
 
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, &tls_conf, "ca", p, len,
+   &srv_conf->tls_ca, &srv_conf->tls_ca_len) != 0)
+   goto fail;
+   break;
+
case TLS_CFG_CERT:
if (config_gettls(env, srv_conf, &tls_conf, "cert", p, len,
&srv_conf->tls_cert, &srv_conf->tls_cert_len) != 0)
+   goto fail;
+   break;
+
+   case TLS_CFG_CRL:
+   if (config_gettls(env, srv_conf, &tls_conf, "crl", p, len,
+   &srv_conf->tls_crl, &srv_conf->tls_crl_len) != 0)
goto fail;
break;
 
Index: httpd.conf.5
===
RCS file: /cvs/src/usr.sbin/httpd/httpd.conf.5,v
retrieving revision 1.90
diff -u -p -r1.90 httpd.conf.5
--- httpd.conf.511 Apr 2018 15:50:46 -  1.90
+++ httpd.conf.516 May 2018 07:59:10 -
@@ -342,6 +342,10 @@ The revision of the HTTP specification u
 .It Ic SERVER_SOFTWARE
 The server software name of
 .Xr httpd 8 .
+.It Ic TLS_PEER_VERIFY
+A variable that is set to a comma separated list of TLS client verification
+features in use
+.Pq omitted when TLS client verification is not in use .
 .El
 .It Ic hsts Oo Ar option Oc
 Enable HTTP Strict Transport Security.
@@ -526,6 +530,23 @@ will be used (strong crypto cipher suite
 See the CIPHERS section of
 .Xr openssl 1
 for information about SSL/TLS cipher suites and preference lists.
+.It Ic client ca Ar cafile Oo Ic crl Ar crlfile Oc Op Ic optional
+Require
+.Po
+or, if
+.Ic optional
+is specified, request but do not require
+.Pc
+TLS client certificates whose authenticity can be verified
+against the CA certificate(s) in
+.Ar cafile
+in order to proceed beyond the TLS handshake.
+With
+.Ic crl
+specified, additionally require that no certificate in the client chain be
+listed as revoked in the CRL(s) in
+.Ar crlfile .
+CA certificates and CRLs should be PEM encoded.
 .It Ic dhe Ar params
 Specify the DHE parameters to use for DHE cipher suites.
 Valid parameter values are none, legacy and auto.
Index: httpd.h
===
RCS file: /cvs/src/usr.sbin/httpd/httpd.h,v
retrieving revision 1.136
diff -u -p -r1.136 httpd.h
--- httpd.h 11 Apr 2018 15:50:46 -  1.136
+++ httpd.h 16 May 2018 07:59:10 -
@@ -424,6 +424,11 @@ SPLAY_HEAD(client_tree, client);
 #define HSTSFLAG_PRELOAD   0x02
 #define HSTSFLAG