From: Florian Obser <florian@openbsd.org>
Subject: Re: [PATCH] relayd client certificate validation again
To: Theo Buehler <tb@theobuehler.org>
Cc: Sören Tempel <soeren@soeren-tempel.net>, tech@openbsd.org, markus.l2ll@gmail.com,  rivo@elnit.ee,  brian@planetunix.net
Date: Mon, 28 Oct 2024 20:12:49 +0100

On 2024-10-28 19:22 +01, Theo Buehler <tb@theobuehler.org> wrote:
> On Mon, Oct 28, 2024 at 07:17:12PM +0100, Sören Tempel wrote:
>> Hi Theo,
>> 
>> Sorry to bother you again about this, but as far as I can tell the patch
>> hasn't been merged yet. Did you want me to send an updated patch with
>> the man page change? I thought that you would just change that on
>> commit. If there is anything else that needs doing, just let me know.
>
> I can't commit this without getting the ok from another committer.
> Here's the diff I have in my tree. It is
>
> ok tb
>
> if anyone wants to commit or I'm happy to take an ok.
>

reads correct to me. I trust you know what you are doing with the TLS
bits.

OK florian

> Index: usr.sbin/relayd/config.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/relayd/config.c,v
> diff -u -p -r1.45 config.c
> --- usr.sbin/relayd/config.c	17 Jan 2024 10:01:24 -0000	1.45
> +++ usr.sbin/relayd/config.c	21 Oct 2024 07:14:24 -0000
> @@ -953,6 +953,15 @@ config_setrelay(struct relayd *env, stru
>  					    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, stru
>  		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, stru
>  			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, stru
>  	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)
> @@ -1161,6 +1179,9 @@ config_getrelayfd(struct relayd *env, st
>  		break;
>  	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;
>  	}
>  
> Index: usr.sbin/relayd/parse.y
> ===================================================================
> RCS file: /cvs/src/usr.sbin/relayd/parse.y,v
> diff -u -p -r1.257 parse.y
> --- usr.sbin/relayd/parse.y	10 Aug 2024 05:47:29 -0000	1.257
> +++ usr.sbin/relayd/parse.y	21 Oct 2024 07:14:24 -0000
> @@ -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->tick
>  			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 r
>  	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);
>  
> Index: usr.sbin/relayd/relay.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/relayd/relay.c,v
> diff -u -p -r1.259 relay.c
> --- usr.sbin/relayd/relay.c	17 Jan 2024 10:01:24 -0000	1.259
> +++ usr.sbin/relayd/relay.c	21 Oct 2024 07:14:24 -0000
> @@ -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;
>  			}
> @@ -2250,6 +2249,26 @@ relay_tls_ctx_create(struct relay *rlay)
>  				goto err;
>  		}
>  		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) {
> Index: usr.sbin/relayd/relayd.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/relayd/relayd.c,v
> diff -u -p -r1.191 relayd.c
> --- usr.sbin/relayd/relayd.c	25 Jun 2023 08:07:38 -0000	1.191
> +++ usr.sbin/relayd/relayd.c	21 Oct 2024 07:14:24 -0000
> @@ -1359,6 +1359,14 @@ relay_load_certfiles(struct relayd *env,
>  	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;
> Index: usr.sbin/relayd/relayd.conf.5
> ===================================================================
> RCS file: /cvs/src/usr.sbin/relayd/relayd.conf.5,v
> diff -u -p -r1.210 relayd.conf.5
> --- usr.sbin/relayd/relayd.conf.5	21 Sep 2024 05:37:26 -0000	1.210
> +++ usr.sbin/relayd/relayd.conf.5	28 Oct 2024 18:20:51 -0000
> @@ -954,6 +954,9 @@ will be used (strong crypto cipher suite
>  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 that can be verified against the CA
> +certificates in the specified file.
>  .It Ic client-renegotiation
>  Allow client-initiated renegotiation.
>  To mitigate a potential DoS risk,
> Index: usr.sbin/relayd/relayd.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/relayd/relayd.h,v
> diff -u -p -r1.275 relayd.h
> --- usr.sbin/relayd/relayd.h	8 Oct 2024 05:28:11 -0000	1.275
> +++ usr.sbin/relayd/relayd.h	21 Oct 2024 07:14:24 -0000
> @@ -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;
> Index: regress/usr.sbin/relayd/Client.pm
> ===================================================================
> RCS file: /cvs/src/regress/usr.sbin/relayd/Client.pm,v
> diff -u -p -r1.14 Client.pm
> --- regress/usr.sbin/relayd/Client.pm	22 Dec 2021 11:50:28 -0000	1.14
> +++ regress/usr.sbin/relayd/Client.pm	21 Oct 2024 07:14:24 -0000
> @@ -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;
> Index: regress/usr.sbin/relayd/Makefile
> ===================================================================
> RCS file: /cvs/src/regress/usr.sbin/relayd/Makefile,v
> diff -u -p -r1.21 Makefile
> --- regress/usr.sbin/relayd/Makefile	30 Dec 2021 20:51:34 -0000	1.21
> +++ regress/usr.sbin/relayd/Makefile	21 Oct 2024 07:14:24 -0000
> @@ -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
> Index: regress/usr.sbin/relayd/Relayd.pm
> ===================================================================
> RCS file: /cvs/src/regress/usr.sbin/relayd/Relayd.pm,v
> diff -u -p -r1.19 Relayd.pm
> --- regress/usr.sbin/relayd/Relayd.pm	12 Oct 2021 05:42:39 -0000	1.19
> +++ regress/usr.sbin/relayd/Relayd.pm	21 Oct 2024 07:14:24 -0000
> @@ -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;
>

-- 
In my defence, I have been left unsupervised.