Download raw body.
[PATCH] relayd client certificate validation again
Hello Theo,
Thanks for your detailed comments and input so far!
Please find the updated patch and additional comments below.
Theo Buehler <tb@theobuehler.org> 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 <v.string> STRING
%token <v.number> NUMBER
%type <v.string> 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;
[PATCH] relayd client certificate validation again