Index | Thread | Search

From:
Theo Buehler <tb@theobuehler.org>
Subject:
Re: tcpbench + tls
To:
Jan Klemkow <jan@openbsd.org>
Cc:
tech@openbsd.org
Date:
Wed, 24 Jul 2024 10:34:19 +0200

Download raw body.

Thread
On Wed, Jul 24, 2024 at 09:19:47AM +0200, Jan Klemkow wrote:
> On Wed, Jul 24, 2024 at 06:47:27AM +0100, Jason McIntyre wrote:
> > On Tue, Jul 23, 2024 at 07:16:09PM +0200, Jan Klemkow wrote:
> > > This diff adds TLS support via libtls to tcpbench.  So, we are able to
> > > measure TLS throughput performance.
> > > 
> > > Its based on the tls code of nc(1).  But, I just took the essentials
> > > which is needed in the context of tcpbench.  We just transfer test data
> > > and have no need for authentication nor key handling.  In client mode,
> > > it just accepts every certificate.  In server mode, it creates a self
> > > signed certificate on the fly.  Thus, its easy to use.

While I understand the desire of benchmarking TLS and making this as
easy as possible for the benchmarker, I'm not a fan of generating an
(invalid per RFC 5280) certificate on the fly. Doing this right feels
out of scope for tcpbench and what you do will break when we decide to
raise the bar on what a cert has to satisfy in order to be accepted by
our TLS stack just a little.

I think a first step should require the operator to point the server at
cert/key PEM files (which could be generated per ssl(8), easyrsa/mkcert/...
from acme-client or whatever other source).

I get your argument about no need for authentication, but I still wonder
if a client shouldn't opt into ignoring certificate and name validation
rather than doing it by default.

some comments inline.

> > > 
> > > ok?
> > 
> > hi. some tweaks inline:
> 
> Fixed.
> 
> Thanks,
> Jan
> 
> Index: Makefile
> ===================================================================
> RCS file: /cvs/src/usr.bin/tcpbench/Makefile,v
> diff -u -p -r1.10 Makefile
> --- Makefile	15 Aug 2022 09:06:54 -0000	1.10
> +++ Makefile	24 Jul 2024 07:16:03 -0000
> @@ -1,7 +1,7 @@
>  #	$OpenBSD: Makefile,v 1.10 2022/08/15 09:06:54 claudio Exp $
>  
>  PROG=	tcpbench
> -LDADD=	-lm -levent
> +LDADD=	-lm -levent -ltls -lcrypto
>  DPADD=	${LIBM} ${LIBEVENT}
>  
>  .include <bsd.prog.mk>
> Index: tcpbench.1
> ===================================================================
> RCS file: /cvs/src/usr.bin/tcpbench/tcpbench.1,v
> diff -u -p -r1.30 tcpbench.1
> --- tcpbench.1	15 Aug 2022 09:06:54 -0000	1.30
> +++ tcpbench.1	24 Jul 2024 07:16:18 -0000
> @@ -24,7 +24,7 @@
>  .Nm
>  .Fl l
>  .Nm
> -.Op Fl 46DRUuv
> +.Op Fl 46cDRUuv
>  .Op Fl B Ar buf
>  .Op Fl b Ar sourceaddr
>  .Op Fl k Ar kvars
> @@ -32,20 +32,20 @@
>  .Op Fl p Ar port
>  .Op Fl r Ar interval
>  .Op Fl S Ar space
> -.Op Fl T Ar toskeyword
> +.Op Fl T Ar keyword
>  .Op Fl t Ar secs
>  .Op Fl V Ar rtable
>  .Ar hostname
>  .Nm
>  .Bk -words
>  .Fl s
> -.Op Fl 46DUuv
> +.Op Fl 46cDUuv
>  .Op Fl B Ar buf
>  .Op Fl k Ar kvars
>  .Op Fl p Ar port
>  .Op Fl r Ar interval
>  .Op Fl S Ar space
> -.Op Fl T Ar toskeyword
> +.Op Fl T Ar keyword
>  .Op Fl V Ar rtable
>  .Op Ar hostname
>  .Ek
> @@ -111,6 +111,8 @@ stream.
>  .It Fl b Ar sourceaddr
>  Specify the IP address to send the packets from,
>  which is useful on machines with multiple interfaces.
> +.It Fl c
> +Use TLS to connect or listen.
>  .It Fl D
>  Enable debugging on the socket.
>  .It Fl k Ar kvars
> @@ -143,9 +145,9 @@ connections.
>  It defaults to using TCP if
>  .Fl u
>  is not specified.
> -.It Fl T Ar toskeyword
> +.It Fl T Ar keyword
>  Change the IPv4 TOS or IPv6 TCLASS value.
> -.Ar toskeyword
> +.Ar keyword
>  may be one of
>  .Ar critical ,
>  .Ar inetcontrol ,
> @@ -158,6 +160,22 @@ or one of the DiffServ Code Points:
>  .Ar af11 ... af43 ,
>  .Ar cs0 ... cs7 ;
>  or a number in either hex or decimal.
> +.Pp
> +For TLS options,
> +.Ar keyword
> +specifies a value in the form of a
> +.Ar key Ns = Ns Ar value
> +pair:
> +.Cm ciphers ,
> +which allows the supported TLS ciphers to be specified (see
> +.Xr tls_config_set_ciphers 3
> +for further details) or
> +.Cm protocols ,
> +which allows the supported TLS protocols to be specified (see
> +.Xr tls_config_parse_protocols 3
> +for further details).
> +Specifying TLS options requires
> +.Fl c .
>  .It Fl t Ar secs
>  Stop after
>  .Ar secs
> Index: tcpbench.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/tcpbench/tcpbench.c,v
> diff -u -p -r1.70 tcpbench.c
> --- tcpbench.c	21 Mar 2024 16:46:04 -0000	1.70
> +++ tcpbench.c	24 Jul 2024 07:16:03 -0000
> @@ -51,6 +51,12 @@
>  #include <poll.h>
>  #include <paths.h>
>  #include <math.h>
> +#include <tls.h>
> +
> +#include <openssl/evp.h>
> +#include <openssl/rsa.h>
> +#include <openssl/pem.h>
> +#include <openssl/x509.h>
>  
>  #define DEFAULT_PORT "12345"
>  #define DEFAULT_STATS_INTERVAL 1000 /* ms */
> @@ -74,6 +80,7 @@ struct {
>  	char	**kvars;	/* Kvm enabled vars */
>  	char	 *dummybuf;	/* IO buffer */
>  	size_t	  dummybuf_len;	/* IO buffer len */
> +	struct tls_config *tls_cfg;
>  } tcpbench, *ptb;
>  
>  struct tcpservsock {
> @@ -95,8 +102,12 @@ struct statctx {
>  	struct tcpservsock *tcp_ts;
>  	/* UDP only */
>  	u_long udp_slice_pkts;
> +	/* TLS context */
> +	struct tls *tls;
>  };
>  
> +char *tls_ciphers;
> +char *tls_protocols;
>  struct statctx *udp_sc; /* singleton */
>  
>  static void	signal_handler(int, short, void *);
> @@ -196,11 +207,11 @@ usage(void)
>  {
>  	fprintf(stderr,
>  	    "usage: tcpbench -l\n"
> -	    "       tcpbench [-46DRUuv] [-B buf] [-b sourceaddr] [-k kvars] [-n connections]\n"
> -	    "                [-p port] [-r interval] [-S space] [-T toskeyword]\n"
> +	    "       tcpbench [-46cDRUuv] [-B buf] [-b sourceaddr] [-k kvars] [-n connections]\n"
> +	    "                [-p port] [-r interval] [-S space] [-T keyword]\n"
>  	    "                [-t secs] [-V rtable] hostname\n"
> -	    "       tcpbench -s [-46DUuv] [-B buf] [-k kvars] [-p port] [-r interval]\n"
> -	    "                [-S space] [-T toskeyword] [-V rtable] [hostname]\n");
> +	    "       tcpbench -s [-46cDUuv] [-B buf] [-k kvars] [-p port] [-r interval]\n"
> +	    "                [-S space] [-T keyword] [-V rtable] [hostname]\n");
>  	exit(1);
>  }
>  
> @@ -592,8 +603,13 @@ tcp_server_handle_sc(int fd, short event
>  	struct statctx *sc = v_sc;
>  	ssize_t n;
>  
> -	n = read(sc->fd, sc->buf, sc->buflen);
> +	if (sc->tls)
> +		n = tls_read(sc->tls, sc->buf, sc->buflen);
> +	else
> +		n = read(sc->fd, sc->buf, sc->buflen);
>  	if (n == -1) {
> +		if (sc->tls)
> +			err(1, "tls_read: %s", tls_error(sc->tls));
>  		if (errno != EINTR && errno != EWOULDBLOCK)
>  			warn("fd %d read error", sc->fd);
>  		return;
> @@ -623,6 +639,33 @@ tcp_server_handle_sc(int fd, short event
>  	mainstats.total_bytes += n;
>  }
>  
> +int
> +timeout_tls(int s, struct tls *tls_ctx, int (*func)(struct tls *))
> +{
> +	struct pollfd pfd;
> +	int ret;
> +
> +	while ((ret = (*func)(tls_ctx)) != 0) {

No need need for parens and dereferencing (I know this comes from nc
where this is just as ugly and should be fixed):

	while ((ret = func(tls_ctx)) != 0) {

> +		if (ret == TLS_WANT_POLLIN)
> +			pfd.events = POLLIN;
> +		else if (ret == TLS_WANT_POLLOUT)
> +			pfd.events = POLLOUT;
> +		else
> +			break;
> +		pfd.fd = s;
> +		if ((ret = poll(&pfd, 1, -1)) == 1)
> +			continue;
> +		else if (ret == 0) {
> +			errno = ETIMEDOUT;
> +			ret = -1;
> +			break;
> +		} else
> +			err(1, "poll failed");
> +	}
> +
> +	return ret;
> +}
> +
>  static void
>  tcp_server_accept(int fd, short event, void *arg)
>  {
> @@ -632,6 +675,15 @@ tcp_server_accept(int fd, short event, v
>  	struct sockaddr_storage ss;
>  	socklen_t sslen;
>  	char tmp[NI_MAXHOST + 2 + NI_MAXSERV];
> +	static struct tls *tls = NULL;
> +
> +	if (ptb->tls_cfg && tls == NULL) {
> +		tls = tls_server();
> +		if (tls == NULL)
> +			err(1, "Unable to create TLS context.");
> +		if (tls_configure(tls, ptb->tls_cfg) == -1)
> +			errx(1, "tls_configure: %s", tls_error(tls));
> +	}
>  
>  	sslen = sizeof(ss);
>  
> @@ -672,6 +724,8 @@ tcp_server_accept(int fd, short event, v
>  	sc->tcp_ts = ts;
>  	sc->fd = sock;
>  	stats_prepare(sc);
> +	if (tls && tls_accept_socket(tls, &sc->tls, sc->fd) == -1)
> +		err(1, "tls_accept_socket: %s", tls_error(tls));
>  
>  	event_set(&sc->ev, sc->fd, EV_READ | EV_PERSIST,
>  	    tcp_server_handle_sc, sc);
> @@ -786,7 +840,14 @@ client_handle_sc(int fd, short event, vo
>  
>  	if (ptb->Rflag)
>  		blen = arc4random_uniform(blen) + 1;
> -	if ((n = write(sc->fd, sc->buf, blen)) == -1) {
> +
> +	if (sc->tls)
> +		n = tls_write(sc->tls, sc->buf, blen);
> +	else
> +		n = write(sc->fd, sc->buf, blen);
> +	if (n == -1) {
> +		if (sc->tls)
> +			warn("tls_write: %s", tls_error(sc->tls));
>  		if (errno == EINTR || errno == EWOULDBLOCK ||
>  		    (UDP_MODE && errno == ENOBUFS))
>  			return;
> @@ -885,6 +946,29 @@ client_init(struct addrinfo *aitop, int 
>  			sc = udp_sc;
>  
>  		sc->fd = sock;
> +
> +		if (ptb->tls_cfg) {
> +			sc->tls = tls_client();
> +			if (sc->tls == NULL)
> +				err(1, "Unable to create TLS context.");
> +
> +			if (tls_configure(sc->tls, ptb->tls_cfg) == -1)
> +				errx(1, "tls_configure: %s",
> +				    tls_error(sc->tls));
> +
> +			if (tls_connect_socket(sc->tls, sc->fd,
> +			    mainstats.host) == -1)
> +				errx(1, "tls_connect_socket: %s",
> +				    tls_error(sc->tls));
> +			if (timeout_tls(sc->fd, sc->tls, tls_handshake) == -1) {
> +				const char *errstr;
> +
> +				if ((errstr = tls_error(sc->tls)) == NULL)
> +					errstr = strerror(errno);
> +				errx(1, "tls handshake failed (%s)", errstr);
> +			}
> +		}
> +
>  		stats_prepare(sc);
>  
>  		event_set(&sc->ev, sc->fd, EV_WRITE | EV_PERSIST,
> @@ -982,6 +1066,97 @@ wrapup(int err)
>  }
>  
>  int
> +process_tls_opt(char *s)
> +{
> +	size_t len;
> +	char *v;
> +
> +	const struct tlskeywords {
> +		const char	 *keyword;
> +		char		**value;
> +	} *t, tlskeywords[] = {
> +		{ "ciphers",	&tls_ciphers },
> +		{ "protocols",	&tls_protocols },
> +		{ NULL,		NULL },
> +	};
> +
> +	len = strlen(s);
> +	if ((v = strchr(s, '=')) != NULL) {
> +		len = v - s;
> +		v++;
> +	}
> +
> +	for (t = tlskeywords; t->keyword != NULL; t++) {
> +		if (strlen(t->keyword) == len &&
> +		    strncmp(s, t->keyword, len) == 0) {
> +			if (v == NULL)
> +				errx(1, "invalid tls value `%s'", s);
> +			*t->value = v;
> +			return 1;
> +		}
> +	}
> +	return 0;
> +}
> +
> +void
> +generate_key(uint8_t **key, size_t *key_size, uint8_t **crt, size_t *crt_size)
> +{

calling this generate_cert() would make more sense to me.

> +	EVP_PKEY	*pkey;
> +	BIGNUM		*bne;
> +	RSA		*rsa;
> +	X509		*x509;
> +	BIO		*bio;
> +
> +	/*
> +	 * Generate RSA key.
> +	 */
> +	if ((pkey = EVP_PKEY_new()) == NULL)
> +		err(1, "EVP_PKEY_new");
> +	if ((bne = BN_new()) == NULL)
> +		err(1, "BN_new");
> +	if (BN_set_word(bne, RSA_F4) == 0)
> +		err(1, "BN_set_word");
> +	if ((rsa = RSA_new()) == NULL)
> +		err(1, "RSA_new");
> +	if (RSA_generate_key_ex(rsa, 2048, bne, NULL) == 0)
> +		err(1, "RSA_generate_key_ex");
> +	if (EVP_PKEY_assign_RSA(pkey, rsa) == 0)
> +		err(1, "EVP_PKEY_assign_RSA");

No need for RSA or BIGNUM, using an EVP_PKEY_CTX *ctx, and the exponent
will default to 65535:

	if ((ctx = EVP_PKEY_CTX_new_id(EVP_PKEY_RSA, NULL)) == NULL)
		errx(...);
	if (EVP_PKEY_keygen_init(ctx) <= 0)
		errx(...);
	if (EVP_PKEY_CTX_set_rsa_keygen_bits(ctx, 2048) <= 0)
		errx(...);
	if (EVP_PKEY_keygen(ctx, &pkey) <= 0)
		errx(...);

don't forget to clean up/free pkey and ctx.

> +
> +	/* Get memory pointer of RSA key. */
> +	if ((bio = BIO_new(BIO_s_mem())) == NULL)
> +		err(1, "BIO_new");
> +	if (!PEM_write_bio_PrivateKey(bio, pkey, NULL, NULL, 0, NULL, NULL))
> +		err(1, "PEM_write_bio_PrivateKey");
> +	if ((*key_size = BIO_get_mem_data(bio, key)) <= 0)
> +		err(1, "BIO_get_mem_data");

BIO_get_mem_data() is a macro wrapper around BIO_ctrl(), which can
(in principle) return negative values, which this check won't catch
because *key_size is a size_t.

Using your approach, key and key_size are part of the BIO, which you
will leak (twice) below. You should probably make a copy of this data
or use the mem BIO's BUF_MEM (see the examples in BIO_s_mem(3)).

> +	/*
> +	 * Generate self sign certificate.
> +	 */
> +	if ((x509 = X509_new()) == NULL)
> +		err(1, "X509_new");
> +
> +	/* Expiration date: 30 days (60s * 60m * 24h * 30d) */
> +	X509_gmtime_adj(X509_get_notBefore(x509), 0);
> +	X509_gmtime_adj(X509_get_notAfter(x509), 2592000);
> +
> +	/* Sign the certificate with the key. */
> +	if (X509_set_pubkey(x509, pkey) == 0)
> +		err(1, "X509_set_pubkey");
> +	if (X509_sign(x509, pkey, EVP_sha256()) == 0)
> +		err(1, "X509_sign");

A valid cert also needs a serial number, an issuer and a subject and it
should be version 3 nowadays. But as I said, I'd prefer not to go there,
at least not in the first step.

> +
> +	/* Get memory pointer of certificate. */
> +	if ((bio = BIO_new(BIO_s_mem())) == NULL)
> +		err(1, "BIO_new");
> +	if (!PEM_write_bio_X509(bio, x509))
> +		err(1, "PEM_write_bio_PrivateKey");
> +	if ((*crt_size = BIO_get_mem_data(bio, crt)) <= 0)
> +		err(1, "BIO_get_mem_data");
> +}
> +
> +int
>  main(int argc, char **argv)
>  {
>  	struct timeval tv;
> @@ -990,7 +1165,7 @@ main(int argc, char **argv)
>  	struct addrinfo *aitop, *aib, hints;
>  	const char *errstr;
>  	struct rlimit rl;
> -	int ch, herr, nconn;
> +	int ch, herr, nconn, usetls = 0;
>  	int family = PF_UNSPEC;
>  	const char *host = NULL, *port = DEFAULT_PORT, *srcbind = NULL;
>  	struct event ev_sigint, ev_sigterm, ev_sighup, ev_siginfo, ev_progtimer;
> @@ -1005,11 +1180,12 @@ main(int argc, char **argv)
>  	ptb->kvars = NULL;
>  	ptb->rflag = DEFAULT_STATS_INTERVAL;
>  	ptb->Tflag = -1;
> +	ptb->tls_cfg = NULL;
>  	nconn = 1;
>  	aib = NULL;
>  	secs = 0;
>  
> -	while ((ch = getopt(argc, argv, "46b:B:Dhlk:n:p:Rr:sS:t:T:uUvV:"))
> +	while ((ch = getopt(argc, argv, "46b:B:cDhlk:n:p:Rr:sS:t:T:uUvV:"))
>  	    != -1) {
>  		switch (ch) {
>  		case '4':
> @@ -1021,6 +1197,9 @@ main(int argc, char **argv)
>  		case 'b':
>  			srcbind = optarg;
>  			break;
> +		case 'c':
> +			usetls = 1;
> +			break;
>  		case 'D':
>  			ptb->Dflag = 1;
>  			break;
> @@ -1088,6 +1267,8 @@ main(int argc, char **argv)
>  			ptb->Uflag = 1;
>  			break;
>  		case 'T':
> +			if (process_tls_opt(optarg))
> +				break;
>  			if (map_tos(optarg, &ptb->Tflag))
>  				break;
>  			errstr = NULL;
> @@ -1118,7 +1299,7 @@ main(int argc, char **argv)
>  	argv += optind;
>  	argc -= optind;
>  	if ((argc != (ptb->sflag && !ptb->Uflag ? 0 : 1)) ||
> -	    (UDP_MODE && (ptb->kvars || nconn != 1)))
> +	    (UDP_MODE && (ptb->kvars || nconn != 1 || usetls)))
>  		usage();
>  
>  	if (!ptb->sflag || ptb->Uflag)
> @@ -1127,6 +1308,40 @@ main(int argc, char **argv)
>  	if (ptb->Uflag)
>  		if (unveil(host, "rwc") == -1)
>  			err(1, "unveil %s", host);
> +
> +	if (usetls) {

does any of the below need the unveil or rpath pledge?

> +		uint8_t		*key;
> +		uint8_t		*crt;
> +		size_t		 key_size;
> +		size_t		 crt_size;
> +		uint32_t	 protocols = 0;
> +
> +		if ((ptb->tls_cfg = tls_config_new()) == NULL)
> +			errx(1, "unable to allocate TLS config");
> +
> +		if (ptb->sflag) {
> +			/* Generate key with selfsigned certificate. */
> +			generate_key(&key, &key_size, &crt, &crt_size);
> +
> +			if (tls_config_set_key_mem(ptb->tls_cfg, key,
> +			    key_size) == -1)
> +				errx(1, "%s", tls_config_error(ptb->tls_cfg));
> +			if (tls_config_set_cert_mem(ptb->tls_cfg, crt,
> +			    crt_size) == -1)
> +				errx(1, "%s", tls_config_error(ptb->tls_cfg));
> +		} else {
> +			/* Don't check server certificate. */
> +			tls_config_insecure_noverifyname(ptb->tls_cfg);
> +			tls_config_insecure_noverifycert(ptb->tls_cfg);
> +		}
> +
> +		if (tls_config_parse_protocols(&protocols, tls_protocols) == -1)
> +			errx(1, "invalid TLS protocols `%s'", tls_protocols);
> +		if (tls_config_set_protocols(ptb->tls_cfg, protocols) == -1)
> +			errx(1, "%s", tls_config_error(ptb->tls_cfg));
> +		if (tls_config_set_ciphers(ptb->tls_cfg, tls_ciphers) == -1)
> +			errx(1, "%s", tls_config_error(ptb->tls_cfg));
> +	}
>  
>  	if (pledge("stdio id dns inet unix", NULL) == -1)
>  		err(1, "pledge");
>