From: Sören Tempel Subject: Re: [PATCH] relayd client certificate validation again To: Theo Buehler Cc: tech@openbsd.org, markus.l2ll@gmail.com, rivo@elnit.ee, brian@planetunix.net Date: Mon, 07 Oct 2024 19:03:47 +0200 Hello Theo, Thanks for your detailed comments and input so far! Please find the updated patch and additional comments below. Theo Buehler wrote: > We need a diff that is tested, has regress coverage and applies to > current. Which one it is, is your choice, but it sounds like you want > [2] + some additions in regress. I decided to update the original patch by Ashe Connor, i.e. [1]. Regarding those updates, I made the following changes: 1. Rebased the patch to ensure that it applies cleanly to -current. 2. Incorporated your feedback regarding code style from [2]. 3. Fixed the run-args-ssl-client-verify-fail.pl test case which was failing previously as the default client func (write_char) does not work correctly if we can't establish a connection to the socket. Instead, the test now uses the relayd.log to check for failure/success. With these changes, the entire relayd regress suite now passes successfully on my system. Furthermore, I performed some additional manual tests as well and didn't run into issues with those either. Therefore, I hope the patch is now in a shape where it can be merged. If additional changes are needed, please let me know. Sincerely, Sören [1]: https://marc.info/?l=openbsd-tech&m=160274490821887 [2]: https://marc.info/?l=openbsd-tech&m=161360826025086 diff --git regress/usr.sbin/relayd/Client.pm regress/usr.sbin/relayd/Client.pm index 4edf4cb5bbe..762d6721650 100644 --- regress/usr.sbin/relayd/Client.pm +++ regress/usr.sbin/relayd/Client.pm @@ -58,6 +58,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, @@ -89,6 +94,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 regress/usr.sbin/relayd/Makefile regress/usr.sbin/relayd/Makefile index 885fd1a8406..622efd9fe6d 100644 --- regress/usr.sbin/relayd/Makefile +++ regress/usr.sbin/relayd/Makefile @@ -92,7 +92,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 regress/usr.sbin/relayd/Relayd.pm regress/usr.sbin/relayd/Relayd.pm index 6b5d0e299d0..e01d44570a8 100644 --- regress/usr.sbin/relayd/Relayd.pm +++ regress/usr.sbin/relayd/Relayd.pm @@ -85,6 +85,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 regress/usr.sbin/relayd/args-ssl-client-verify-fail.pl regress/usr.sbin/relayd/args-ssl-client-verify-fail.pl new file mode 100644 index 00000000000..45099a1674b --- /dev/null +++ regress/usr.sbin/relayd/args-ssl-client-verify-fail.pl @@ -0,0 +1,33 @@ +# test client ssl certificate verification + +use strict; +use warnings; + +our %args = ( + client => { + ssl => 1, + offertlscert => 0, + # no-op func as we cannot connect without presenting a client certificate, + # hence the default write_char function won't work here and block forever. + func => sub { + errignore(); + sleep(2); + }, + dryrun => 1, + nocheck => 1, + }, + relayd => { + listenssl => 1, + verifyclient => 1, + loggrep => { + qr/peer did not return a certificate/ => 1, + qr/tls session \d+ established/ => 0, + }, + }, + server => { + noserver => 1, + nocheck => 1, + }, +); + +1; diff --git regress/usr.sbin/relayd/args-ssl-client-verify.pl regress/usr.sbin/relayd/args-ssl-client-verify.pl new file mode 100644 index 00000000000..c5cc3110724 --- /dev/null +++ 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 usr.sbin/relayd/config.c usr.sbin/relayd/config.c index 3024c0768f0..305072fca00 100644 --- usr.sbin/relayd/config.c +++ usr.sbin/relayd/config.c @@ -953,6 +953,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 " @@ -986,6 +995,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; @@ -1011,6 +1024,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); } @@ -1033,6 +1050,7 @@ config_getrelay(struct relayd *env, struct imsg *imsg) rlay->rl_s = imsg_get_fd(imsg); 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) @@ -1162,6 +1180,9 @@ config_getrelayfd(struct relayd *env, struct imsg *imsg) case RELAY_FD_CAFILE: rlay->rl_tls_cacert_fd = imsg_get_fd(imsg); 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 usr.sbin/relayd/parse.y usr.sbin/relayd/parse.y index eea485c4471..39f4773c4c6 100644 --- usr.sbin/relayd/parse.y +++ usr.sbin/relayd/parse.y @@ -179,7 +179,7 @@ typedef struct { %token TIMEOUT TLS TO ROUTER RTLABEL TRANSPARENT 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 PFLOG +%token WEBSOCKETS PFLOG CLIENT %token STRING %token NUMBER %type context hostname interface table value path @@ -1351,6 +1351,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; } ; @@ -1822,6 +1832,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"); @@ -2411,6 +2422,7 @@ lookup(char *s) { "check", CHECK }, { "checks", CHECKS }, { "ciphers", CIPHERS }, + { "client", CLIENT }, { "code", CODE }, { "connection", CONNECTION }, { "context", CONTEXT }, @@ -3397,6 +3409,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 usr.sbin/relayd/relay.c usr.sbin/relayd/relay.c index c29f3917152..8a068aad971 100644 --- usr.sbin/relayd/relay.c +++ usr.sbin/relayd/relay.c @@ -2159,8 +2159,7 @@ relay_tls_ctx_create(struct relay *rlay) tls_config_insecure_noverifyname(tls_client_cfg); if (rlay->rl_tls_ca_fd != -1) { - if ((buf = relay_load_fd(rlay->rl_tls_ca_fd, &len)) == - NULL) { + if ((buf = relay_load_fd(rlay->rl_tls_ca_fd, &len)) == NULL) { log_warn("failed to read root certificates"); goto err; } @@ -2251,6 +2250,26 @@ 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 usr.sbin/relayd/relayd.c usr.sbin/relayd/relayd.c index df93c9527cb..f0f0319cdfa 100644 --- usr.sbin/relayd/relayd.c +++ usr.sbin/relayd/relayd.c @@ -1359,6 +1359,14 @@ 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 usr.sbin/relayd/relayd.conf.5 usr.sbin/relayd/relayd.conf.5 index 50c73cbec15..771d3632398 100644 --- usr.sbin/relayd/relayd.conf.5 +++ usr.sbin/relayd/relayd.conf.5 @@ -954,6 +954,10 @@ will be used (strong crypto cipher suites without anonymous DH). See the CIPHERS section of .Xr openssl 1 for information about 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 usr.sbin/relayd/relayd.h usr.sbin/relayd/relayd.h index ce8cabd68d2..13eca6fca1e 100644 --- usr.sbin/relayd/relayd.h +++ usr.sbin/relayd/relayd.h @@ -137,11 +137,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 { @@ -744,6 +745,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 +835,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;