Hi On Thu, Oct 15, 2020 at 05:52:40PM +1100, 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.
This looks pretty good to me and appears to work in basic testing. I'd be willing to get this in provided you address the tiny nits below. The diff in its current form applies with a little bit of fuzz, it would be nice if you could rebase it on top of -current. > > 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. The problem with this is that it will make the relayd regress fail: ==== run-args-ssl-client-verify-fail.pl ==== time SUDO= KTRACE= RELAYD= perl -I/usr/src/regress/usr.sbin/relayd /usr/src/regress/usr.sbin/relayd/relayd.pl copy /usr/src/regress/usr.sbin/relayd/args-ssl-client-verify-fail.pl Client no 'connect attempt failed' in client.log after 30 seconds at /usr/src/regress/usr.sbin/relayd/relayd.pl line 84. *** Error 255 in . (Makefile:65 'run-args-ssl-client-verify-fail.pl') FAILED This will need to be done in such a way that the test passes. I don't really understand this perl contraption, so I can't really give advice on this. > > Best, > > Ashe > > --- > > From c63bca7ba7889b43e0a9317e807499eb8ca0db55 Mon Sep 17 00:00:00 2001 > From: Asherah Connor <[email protected]> > 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; > + } Previous line has one tab too many. > + 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) { no line break after == > + 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) { unwrap above line. > + 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 Like F_AGENTX_TRAPONLY this is unused. Since we're slowly running out of flags, I'm not sure it should be added. > > #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; >
