Hi again, Checking in again to see if there's any appetite for this.
Best, Ashe On Thu, Oct 15, 2020, at 5:52 PM, Ashe Connor wrote: > Hi there, > > A year or two ago I submitted a patch for adding TLS client certificate > validation to relayd. At the time it didn't make it in, and I stopped > pursuing it further. > (https://marc.info/?l=openbsd-tech&m=154509330608643&w=2) > > I'd still like to see this landed, if at all possible. I'm continuing to use > this feature on my own personal websites, and it works well. > > The latest diff is attached, or can be viewed online here: > https://github.com/openbsd/src/compare/master...kivikakk:relayd-client-verification.patch > > I've added a test that confirms client failure to connect without a > certificate at regress/usr.sbin/relayd/args-ssl-client-verify-fail.pl -- it's > a bit awkward. Let me know if I can redo it better. > > Best, > > Ashe > > --- > > From c63bca7ba7889b43e0a9317e807499eb8ca0db55 Mon Sep 17 00:00:00 2001 > From: Asherah Connor <a...@kivikakk.ee> > Date: Thu, 15 Oct 2020 17:23:15 +1100 > Subject: [PATCH] TLS client certificate validation > > --- > regress/usr.sbin/relayd/Client.pm | 13 ++++++++++ > regress/usr.sbin/relayd/Makefile | 18 ++++++++++++- > regress/usr.sbin/relayd/Relayd.pm | 3 +++ > .../relayd/args-ssl-client-verify-fail.pl | 25 +++++++++++++++++++ > .../usr.sbin/relayd/args-ssl-client-verify.pl | 19 ++++++++++++++ > usr.sbin/relayd/config.c | 21 ++++++++++++++++ > usr.sbin/relayd/parse.y | 15 ++++++++++- > usr.sbin/relayd/relay.c | 21 ++++++++++++++++ > usr.sbin/relayd/relayd.c | 9 +++++++ > usr.sbin/relayd/relayd.conf.5 | 4 +++ > usr.sbin/relayd/relayd.h | 14 +++++++---- > 11 files changed, 155 insertions(+), 7 deletions(-) > create mode 100644 regress/usr.sbin/relayd/args-ssl-client-verify-fail.pl > create mode 100644 regress/usr.sbin/relayd/args-ssl-client-verify.pl > > diff --git a/regress/usr.sbin/relayd/Client.pm > b/regress/usr.sbin/relayd/Client.pm > index b3e011de13d..ec6fa64274e 100644 > --- a/regress/usr.sbin/relayd/Client.pm > +++ b/regress/usr.sbin/relayd/Client.pm > @@ -57,6 +57,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} ? > + "client.crt" : "", > + SSL_key_file => $self->{offertlscert} ? > + "client.key" : "", > ) or die ref($self), " $iosocket socket connect failed: $!,$SSL_ERROR"; > if ($self->{sndbuf}) { > setsockopt($cs, SOL_SOCKET, SO_SNDBUF, > @@ -86,6 +91,14 @@ sub child { > print STDERR "ssl cipher: ",$cs->get_cipher(),"\n"; > print STDERR "ssl peer certificate:\n", > $cs->dump_peer_certificate(); > + > + if ($self->{offertlscert}) { > + print STDERR "ssl client certificate:\n"; > + print STDERR "Subject Name: ", > + "${\$cs->sock_certificate('subject')}\n"; > + print STDERR "Issuer Name: ", > + "${\$cs->sock_certificate('issuer')}\n"; > + } > } > > *STDIN = *STDOUT = $self->{cs} = $cs; > diff --git a/regress/usr.sbin/relayd/Makefile > b/regress/usr.sbin/relayd/Makefile > index cd01aa3fb63..f2198f43cc9 100644 > --- a/regress/usr.sbin/relayd/Makefile > +++ b/regress/usr.sbin/relayd/Makefile > @@ -96,7 +96,23 @@ server.req: > 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*ssl*} ${REGRESS_TARGETS:M*https*}: server.crt > +client-ca.crt: > + openssl req -batch -new \ > + -subj /L=OpenBSD/O=relayd-regress/OU=client-ca/CN=root/ \ > + -nodes -newkey rsa -keyout client-ca.key -x509 \ > + -out client-ca.crt > + > +client.req: > + openssl req -batch -new \ > + -subj /L=OpenBSD/O=relayd-regress/OU=client/CN=localhost/ \ > + -nodes -newkey rsa -keyout client.key \ > + -out client.req > + > +client.crt: client-ca.crt client.req > + openssl x509 -CAcreateserial -CAkey client-ca.key -CA client-ca.crt \ > + -req -in client.req -out client.crt > + > +${REGRESS_TARGETS:M*ssl*} ${REGRESS_TARGETS:M*https*}: server.crt client.crt > .if empty (REMOTE_SSH) > ${REGRESS_TARGETS:M*ssl*} ${REGRESS_TARGETS:M*https*}: 127.0.0.1.crt > .else > diff --git a/regress/usr.sbin/relayd/Relayd.pm > b/regress/usr.sbin/relayd/Relayd.pm > index 98f2ada5db9..896c0b401be 100644 > --- a/regress/usr.sbin/relayd/Relayd.pm > +++ b/regress/usr.sbin/relayd/Relayd.pm > @@ -84,6 +84,9 @@ sub new { > print $fh "\n\ttls ca cert ca.crt"; > print $fh "\n\ttls ca key ca.key password ''"; > } > + if ($self->{verifyclient}) { > + print $fh "\n\ttls client ca client-ca.crt"; > + } > # substitute variables in config file > foreach (@protocol) { > s/(\$[a-z]+)/$1/eeg; > diff --git a/regress/usr.sbin/relayd/args-ssl-client-verify-fail.pl > b/regress/usr.sbin/relayd/args-ssl-client-verify-fail.pl > new file mode 100644 > index 00000000000..93c9b22a63d > --- /dev/null > +++ b/regress/usr.sbin/relayd/args-ssl-client-verify-fail.pl > @@ -0,0 +1,25 @@ > +# test client ssl certificate verification > + > +use strict; > +use warnings; > + > +our %args = ( > + client => { > + ssl => 1, > + offertlscert => 0, > + up => 'failed', > + down => 'connect attempt failed', > + dryrun => 1, > + nocheck => 1, > + }, > + relayd => { > + listenssl => 1, > + verifyclient => 1, > + }, > + server => { > + noserver => 1, > + nocheck => 1, > + }, > +); > + > +1; > diff --git a/regress/usr.sbin/relayd/args-ssl-client-verify.pl > b/regress/usr.sbin/relayd/args-ssl-client-verify.pl > new file mode 100644 > index 00000000000..c5cc3110724 > --- /dev/null > +++ b/regress/usr.sbin/relayd/args-ssl-client-verify.pl > @@ -0,0 +1,19 @@ > +# test client ssl certificate verification > + > +use strict; > +use warnings; > + > +our %args = ( > + client => { > + ssl => 1, > + offertlscert => 1, > + }, > + relayd => { > + listenssl => 1, > + verifyclient => 1, > + }, > + len => 251, > + md5 => "bc3a3f39af35fe5b1687903da2b00c7f", > +); > + > +1; > diff --git a/usr.sbin/relayd/config.c b/usr.sbin/relayd/config.c > index 3e60d63ef52..5b409aa1134 100644 > --- a/usr.sbin/relayd/config.c > +++ b/usr.sbin/relayd/config.c > @@ -956,6 +956,15 @@ config_setrelay(struct relayd *env, struct relay *rlay) > rlay->rl_conf.name); > return (-1); > } > + if (rlay->rl_tls_client_ca_fd != -1 && > + config_setrelayfd(ps, id, n, 0, > + rlay->rl_conf.id, RELAY_FD_CLIENTCACERT, > + rlay->rl_tls_client_ca_fd) == -1) { > + log_warn("%s: fd passing failed for " > + "`%s'", __func__, > + rlay->rl_conf.name); > + return (-1); > + } > /* Prevent fd exhaustion in the parent. */ > if (proc_flush_imsg(ps, id, n) == -1) { > log_warn("%s: failed to flush " > @@ -989,6 +998,10 @@ config_setrelay(struct relayd *env, struct relay *rlay) > close(rlay->rl_s); > rlay->rl_s = -1; > } > + if (rlay->rl_tls_client_ca_fd != -1) { > + close(rlay->rl_tls_client_ca_fd); > + rlay->rl_tls_client_ca_fd = -1; > + } > if (rlay->rl_tls_cacert_fd != -1) { > close(rlay->rl_tls_cacert_fd); > rlay->rl_tls_cacert_fd = -1; > @@ -1014,6 +1027,10 @@ config_setrelay(struct relayd *env, struct relay *rlay) > cert->cert_ocsp_fd = -1; > } > } > + if (rlay->rl_tls_client_ca_fd != -1) { > + close(rlay->rl_tls_client_ca_fd); > + rlay->rl_tls_client_ca_fd = -1; > + } > > return (0); > } > @@ -1036,6 +1053,7 @@ config_getrelay(struct relayd *env, struct imsg *imsg) > rlay->rl_s = imsg->fd; > rlay->rl_tls_ca_fd = -1; > rlay->rl_tls_cacert_fd = -1; > + rlay->rl_tls_client_ca_fd = -1; > > if (ps->ps_what[privsep_process] & CONFIG_PROTOS) { > if (rlay->rl_conf.proto == EMPTY_ID) > @@ -1165,6 +1183,9 @@ config_getrelayfd(struct relayd *env, struct imsg *imsg) > case RELAY_FD_CAFILE: > rlay->rl_tls_cacert_fd = imsg->fd; > break; > + case RELAY_FD_CLIENTCACERT: > + rlay->rl_tls_client_ca_fd = imsg->fd; > + break; > } > > DPRINTF("%s: %s %d received relay fd %d type %d for relay %s", __func__, > diff --git a/usr.sbin/relayd/parse.y b/usr.sbin/relayd/parse.y > index ce219245731..a524447aa72 100644 > --- a/usr.sbin/relayd/parse.y > +++ b/usr.sbin/relayd/parse.y > @@ -179,7 +179,7 @@ typedef struct { > %token TIMEOUT TLS TO ROUTER RTLABEL TRANSPARENT TRAP URL WITH TTL RTABLE > %token MATCH PARAMS RANDOM LEASTSTATES SRCHASH KEY CERTIFICATE PASSWORD ECDHE > %token EDH TICKETS CONNECTION CONNECTIONS CONTEXT ERRORS STATE CHANGES CHECKS > -%token WEBSOCKETS > +%token WEBSOCKETS CLIENT > %token <v.string> STRING > %token <v.number> NUMBER > %type <v.string> context hostname interface table value optstring path > @@ -1373,6 +1373,16 @@ tlsflags : SESSION TICKETS { proto->tickets = 1; } > name->name = $2; > TAILQ_INSERT_TAIL(&proto->tlscerts, name, entry); > } > + | CLIENT CA STRING { > + if (strlcpy(proto->tlsclientca, $3, > + sizeof(proto->tlsclientca)) >= > + sizeof(proto->tlsclientca)) { > + yyerror("tlsclientca truncated"); > + free($3); > + YYERROR; > + } > + free($3); > + } > | NO flag { proto->tlsflags &= ~($2); } > | flag { proto->tlsflags |= $1; } > ; > @@ -1830,6 +1840,7 @@ relay : RELAY STRING { > r->rl_conf.dstretry = 0; > r->rl_tls_ca_fd = -1; > r->rl_tls_cacert_fd = -1; > + r->rl_tls_client_ca_fd = -1; > TAILQ_INIT(&r->rl_tables); > if (last_relay_id == INT_MAX) { > yyerror("too many relays defined"); > @@ -2423,6 +2434,7 @@ lookup(char *s) > { "check", CHECK }, > { "checks", CHECKS }, > { "ciphers", CIPHERS }, > + { "client", CLIENT }, > { "code", CODE }, > { "connection", CONNECTION }, > { "context", CONTEXT }, > @@ -3408,6 +3420,7 @@ relay_inherit(struct relay *ra, struct relay *rb) > if (!(rb->rl_conf.flags & F_TLS)) { > rb->rl_tls_cacert_fd = -1; > rb->rl_tls_ca_fd = -1; > + rb->rl_tls_client_ca_fd = -1; > } > TAILQ_INIT(&rb->rl_tables); > > diff --git a/usr.sbin/relayd/relay.c b/usr.sbin/relayd/relay.c > index 43b5c377fa5..2f9f293368c 100644 > --- a/usr.sbin/relayd/relay.c > +++ b/usr.sbin/relayd/relay.c > @@ -2259,6 +2259,27 @@ relay_tls_ctx_create(struct relay *rlay) > } > rlay->rl_tls_cacert_fd = -1; > > + if (rlay->rl_tls_client_ca_fd != -1) { > + if ((buf = relay_load_fd(rlay->rl_tls_client_ca_fd, > + &len)) == > + NULL) { > + log_warn( > + "failed to read tls client CA certificate"); > + goto err; > + } > + > + if (tls_config_set_ca_mem(tls_cfg, buf, len) != 0) { > + log_warnx( > + "failed to set tls client CA cert: %s", > + tls_config_error(tls_cfg)); > + goto err; > + } > + purge_key(&buf, len); > + > + tls_config_verify_client(tls_cfg); > + } > + rlay->rl_tls_client_ca_fd = -1; > + > tls = tls_server(); > if (tls == NULL) { > log_warnx("unable to allocate TLS context"); > diff --git a/usr.sbin/relayd/relayd.c b/usr.sbin/relayd/relayd.c > index 9390b2e4606..52093f6cdc9 100644 > --- a/usr.sbin/relayd/relayd.c > +++ b/usr.sbin/relayd/relayd.c > @@ -1359,6 +1359,15 @@ relay_load_certfiles(struct relayd *env, struct relay > *rlay, const char *name) > if ((rlay->rl_conf.flags & F_TLS) == 0) > return (0); > > + if (strlen(proto->tlsclientca) && > + rlay->rl_tls_client_ca_fd == -1) { > + if ((rlay->rl_tls_client_ca_fd = > + open(proto->tlsclientca, O_RDONLY)) == -1) > + return (-1); > + log_debug("%s: using client ca %s", __func__, > + proto->tlsclientca); > + } > + > if (name == NULL && > print_host(&rlay->rl_conf.ss, hbuf, sizeof(hbuf)) == NULL) > goto fail; > diff --git a/usr.sbin/relayd/relayd.conf.5 b/usr.sbin/relayd/relayd.conf.5 > index 491fc216b71..fe6dff92174 100644 > --- a/usr.sbin/relayd/relayd.conf.5 > +++ b/usr.sbin/relayd/relayd.conf.5 > @@ -946,6 +946,10 @@ will be used (strong crypto cipher suites without > anonymous DH). > See the CIPHERS section of > .Xr openssl 1 > for information about SSL/TLS cipher suites and preference lists. > +.It Ic client ca Ar path > +Require TLS client certificates whose authenticity can be verified > +against the CA certificate(s) in the specified file in order to > +proceed beyond the TLS handshake. > .It Ic client-renegotiation > Allow client-initiated renegotiation. > To mitigate a potential DoS risk, > diff --git a/usr.sbin/relayd/relayd.h b/usr.sbin/relayd/relayd.h > index 354c4e7d01f..5d7c71dcbff 100644 > --- a/usr.sbin/relayd/relayd.h > +++ b/usr.sbin/relayd/relayd.h > @@ -139,11 +139,12 @@ struct ctl_relaytable { > }; > > enum fd_type { > - RELAY_FD_CERT = 1, > - RELAY_FD_CACERT = 2, > - RELAY_FD_CAFILE = 3, > - RELAY_FD_KEY = 4, > - RELAY_FD_OCSP = 5 > + RELAY_FD_CERT = 1, > + RELAY_FD_CACERT = 2, > + RELAY_FD_CAFILE = 3, > + RELAY_FD_KEY = 4, > + RELAY_FD_OCSP = 5, > + RELAY_FD_CLIENTCACERT = 6 > }; > > struct ctl_relayfd { > @@ -401,6 +402,7 @@ union hashkey { > #define F_TLSINSPECT 0x04000000 > #define F_HASHKEY 0x08000000 > #define F_AGENTX_TRAPONLY 0x10000000 > +#define F_TLSVERIFY 0x20000000 > > #define F_BITS \ > "\10\01DISABLE\02BACKUP\03USED\04DOWN\05ADD\06DEL\07CHANGED" \ > @@ -744,6 +746,7 @@ struct protocol { > char tlscacert[PATH_MAX]; > char tlscakey[PATH_MAX]; > char *tlscapass; > + char tlsclientca[PATH_MAX]; > struct keynamelist tlscerts; > char name[MAX_NAME_SIZE]; > int tickets; > @@ -833,6 +836,7 @@ struct relay { > > int rl_tls_ca_fd; > int rl_tls_cacert_fd; > + int rl_tls_client_ca_fd; > EVP_PKEY *rl_tls_pkey; > X509 *rl_tls_cacertx509; > char *rl_tls_cakey; > >
