On Tue, Aug 30, 2016 at 01:22:49PM +0200, Claudio Jeker wrote:
> Here is the latest version of the ticket and tls session cache support.
> Tickets can be disabled and also the session timeout is configurable.
> Same code as before with man page diff
>
Nice work! I'm curious how this impact production, do you have any
experience to share?
> Will commit this soonish unless somebody complains
I do complain, but just a bit. nit-picking below.
Reyk
> --
> :wq Claudio
>
> Index: Makefile
> ===================================================================
> RCS file: /cvs/src/usr.sbin/relayd/Makefile,v
> retrieving revision 1.29
> diff -u -p -r1.29 Makefile
> --- Makefile 21 Nov 2015 12:37:42 -0000 1.29
> +++ Makefile 19 Jul 2016 08:33:26 -0000
> @@ -6,7 +6,7 @@ SRCS+= agentx.c ca.c carp.c check_icmp.
> check_tcp.c config.c control.c hce.c log.c name2id.c \
> pfe.c pfe_filter.c pfe_route.c proc.c \
> relay.c relay_http.c relay_udp.c relayd.c \
> - shuffle.c snmp.c ssl.c util.c
> + shuffle.c snmp.c ssl.c tlsc.c util.c
> MAN= relayd.8 relayd.conf.5
>
> LDADD= -levent -lssl -lcrypto -lutil
> Index: ca.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/relayd/ca.c,v
> retrieving revision 1.16
> diff -u -p -r1.16 ca.c
> --- ca.c 5 Dec 2015 13:13:11 -0000 1.16
> +++ ca.c 19 Jul 2016 13:18:33 -0000
> @@ -23,6 +23,7 @@
> #include <unistd.h>
> #include <string.h>
> #include <stdlib.h>
> +#include <poll.h>
> #include <imsg.h>
>
> #include <openssl/bio.h>
> @@ -256,6 +257,7 @@ static int
> rsae_send_imsg(int flen, const u_char *from, u_char *to, RSA *rsa,
> int padding, u_int cmd)
> {
> + struct pollfd pfd[1];
> struct ctl_keyop cko;
> int ret = 0;
> objid_t *id;
> @@ -292,9 +294,21 @@ rsae_send_imsg(int flen, const u_char *f
> * operation in OpenSSL's engine layer.
> */
> imsg_composev(ibuf, cmd, 0, 0, -1, iov, cnt);
> - imsg_flush(ibuf);
> + if (imsg_flush(ibuf) == -1)
> + log_warn("rsae_send_imsg: imsg_flush");
>
> + pfd[0].fd = ibuf->fd;
> + pfd[0].events = POLLIN;
> while (!done) {
> + switch (poll(pfd, 1, 5 * 1000)) {
I don't like this timeout number here, please turn it into a #define
and put it into relayd.h, eg.
#define TLS_PRIV_SEND_TIMEOUT 5000 /* 5 sec */
> + case -1:
> + fatal("rsae_send_imsg: poll");
> + case 0:
> + log_warnx("rsae_send_imsg: poll timeout");
> + break;
> + default:
> + break;
> + }
> if ((n = imsg_read(ibuf)) == -1 && errno != EAGAIN)
> fatalx("imsg_read");
> if (n == 0)
> Index: config.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/relayd/config.c,v
> retrieving revision 1.27
> diff -u -p -r1.27 config.c
> --- config.c 7 Dec 2015 04:03:27 -0000 1.27
> +++ config.c 18 Jul 2016 13:01:35 -0000
> @@ -51,6 +51,7 @@ config_init(struct relayd *env)
> ps->ps_what[PROC_CA] = CONFIG_RELAYS;
> ps->ps_what[PROC_RELAY] = CONFIG_RELAYS|
> CONFIG_TABLES|CONFIG_PROTOS|CONFIG_CA_ENGINE;
> + ps->ps_what[PROC_TLSC] = 0;
> }
>
> /* Other configuration */
> Index: parse.y
> ===================================================================
> RCS file: /cvs/src/usr.sbin/relayd/parse.y,v
> retrieving revision 1.207
> diff -u -p -r1.207 parse.y
> --- parse.y 21 Jun 2016 21:35:25 -0000 1.207
> +++ parse.y 30 Aug 2016 10:50:16 -0000
> @@ -172,13 +172,13 @@ typedef struct {
> %token SOCKET SPLICE SSL STICKYADDR STYLE TABLE TAG TAGGED TCP TIMEOUT
> TLS TO
> %token ROUTER RTLABEL TRANSPARENT TRAP UPDATES URL VIRTUAL WITH TTL
> RTABLE
> %token MATCH PARAMS RANDOM LEASTSTATES SRCHASH KEY CERTIFICATE
> PASSWORD ECDH
> -%token EDH CURVE
> +%token EDH CURVE TICKETS
> %token <v.string> STRING
> %token <v.number> NUMBER
> %type <v.string> hostname interface table value optstring
> %type <v.number> http_type loglevel quick trap
> %type <v.number> dstmode flag forwardmode retry
> -%type <v.number> opttls opttlsclient tlscache
> +%type <v.number> opttls opttlsclient tlscache tlstickets
> tlstimeout
> %type <v.number> redirect_proto relay_proto match
> %type <v.number> action ruleaf key_option
> %type <v.number> tlsdhparams tlsecdhcurve
> @@ -1092,6 +1092,8 @@ tlsflags_l : tlsflags comma tlsflags_l
> ;
>
> tlsflags : SESSION CACHE tlscache { proto->cache = $3; }
> + | SESSION TICKETS tlstickets { proto->tickets = $3; }
> + | SESSION TIMEOUT tlstimeout { proto->tlstimeout = $3; }
> | CIPHERS STRING {
> if (strlcpy(proto->tlsciphers, $2,
> sizeof(proto->tlsciphers)) >=
> @@ -1190,6 +1192,17 @@ tlscache : NUMBER {
> | DISABLE { $$ = -2; }
> ;
>
> +tlstickets : DISABLE { $$ = -1; }
> +
> +tlstimeout : NUMBER {
> + if ($1 < 0) {
> + yyerror("invalid tlstimeout value: %d", $1);
> + YYERROR;
> + }
> + $$ = $1;
> + }
> + ;
> +
> filterrule : action dir quick ruleaf rulesrc ruledst {
> if ((rule = calloc(1, sizeof(*rule))) == NULL)
> fatal("out of memory");
> @@ -2257,6 +2270,7 @@ lookup(char *s)
> { "tag", TAG },
> { "tagged", TAGGED },
> { "tcp", TCP },
> + { "tickets", TICKETS },
> { "timeout", TIMEOUT },
> { "tls", TLS },
> { "to", TO },
> Index: relay.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/relayd/relay.c,v
> retrieving revision 1.206
> diff -u -p -r1.206 relay.c
> --- relay.c 30 Dec 2015 16:00:57 -0000 1.206
> +++ relay.c 30 Aug 2016 10:54:28 -0000
> @@ -28,6 +28,7 @@
> #include <arpa/inet.h>
>
> #include <limits.h>
> +#include <poll.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <errno.h>
> @@ -54,6 +55,8 @@ int relay_dispatch_ca(int, struct priv
> struct imsg *);
> int relay_dispatch_hce(int, struct privsep_proc *,
> struct imsg *);
> +int relay_dispatch_tlsc(int, struct privsep_proc *,
> + struct imsg *);
> void relay_shutdown(void);
>
> void relay_protodebug(struct relay *);
> @@ -84,6 +87,12 @@ void relay_tls_connect(int, short, voi
> void relay_tls_connected(struct ctl_relay_event *);
> void relay_tls_readcb(int, short, void *);
> void relay_tls_writecb(int, short, void *);
> +int relay_tls_new_session(SSL *, SSL_SESSION *);
> +SSL_SESSION *relay_tls_get_session(SSL *, unsigned char *, int, int *);
> +
> +struct tls_ticket_ctx *relay_get_ticket_key(unsigned char [16]);
> +int relay_tls_session_ticket(SSL *, unsigned char [16],
> + unsigned char *, EVP_CIPHER_CTX *, HMAC_CTX *, int);
>
> char *relay_load_file(const char *, off_t *);
> extern void bufferevent_read_pressure_cb(struct evbuffer *, size_t,
> @@ -92,6 +101,8 @@ extern void bufferevent_read_pressure_c
> volatile int relay_sessions;
> volatile int relay_inflight = 0;
> objid_t relay_conid;
> +static u_int8_t sid[SSL_MAX_SID_CTX_LENGTH];
> +static struct tls_ticket_ctx tls_ticket, tls_ticket_bak;
>
Why globals?
I dislike adding more globals because it makes it harder to read the
code. And it makes fork+exec harder in some situations (see below).
If "sid" is a global thing, it should be in "struct relayd" as
something like "sc_tls_sid". I see that tls_ticket_bak and tls_ticket
are used from the libssl callbacks, so you don't have the relay
context without passing it somehow, so it is OK but I'd still prefer
to have them in "struct relay".
> static struct relayd *env = NULL;
> int proc_id;
> @@ -101,12 +112,18 @@ static struct privsep_proc procs[] = {
> { "pfe", PROC_PFE, relay_dispatch_pfe },
> { "ca", PROC_CA, relay_dispatch_ca },
> { "hce", PROC_HCE, relay_dispatch_hce },
> + { "tlsc", PROC_TLSC, relay_dispatch_tlsc },
> };
>
> pid_t
> relay(struct privsep *ps, struct privsep_proc *p)
> {
> pid_t pid;
> +
> + /* initialize the session id to a random key for all relay procs */
> + arc4random_buf(sid, sizeof(sid));
This should be moved to the point when initializing "env" (struct
relayd) in main, as a member of the struct as mentioned above.
This way we can pass it to the child processes later, especially with
fork+exec (which needs further changes, the optimal thing would be to
send and activate the sessions id and ticket with receiving
IMSG_CFG_DONE; but this can be done later).
> + tlsc_create_ticket(&tls_ticket);
> +
> env = ps->ps_env;
> pid = proc_run(ps, p, procs, nitems(procs), relay_init, NULL);
> relay_http(env);
> @@ -262,6 +279,9 @@ relay_protodebug(struct relay *rlay)
> printb_flags(proto->tlsflags, TLSFLAG_BITS));
> if (proto->cache != -1)
> fprintf(stderr, "\ttls session cache: %d\n", proto->cache);
> + fprintf(stderr, "\ttls session tickets: %s\n",
> + (proto->tickets > -1) ? "enabled" : "disabled");
> + fprintf(stderr, "\ttls session timeout: %lu\n", proto->tlstimeout);
> fprintf(stderr, "\ttype: ");
> switch (proto->type) {
> case RELAY_PROTO_TCP:
> @@ -1931,6 +1951,23 @@ relay_dispatch_parent(int fd, struct pri
> return (0);
> }
>
> +int
> +relay_dispatch_tlsc(int fd, struct privsep_proc *p, struct imsg *imsg)
> +{
> + switch (imsg->hdr.type) {
> + case IMSG_TLSC_REKEY:
> + IMSG_SIZE_CHECK(imsg, (&tls_ticket));
> + /* rotate keys */
> + memcpy(&tls_ticket_bak, &tls_ticket, sizeof(tls_ticket));
> + memcpy(&tls_ticket, imsg->data, sizeof(tls_ticket));
> + break;
> + default:
> + return (-1);
> + }
> +
> + return (0);
> +}
> +
> void
> relay_tls_callback_info(const SSL *ssl, int where, int rc)
> {
> @@ -2045,7 +2082,6 @@ relay_tls_ctx_create(struct relay *rlay)
> struct protocol *proto = rlay->rl_proto;
> SSL_CTX *ctx;
> EC_KEY *ecdhkey;
> - u_int8_t sid[SSL_MAX_SID_CTX_LENGTH];
>
> ctx = SSL_CTX_new(SSLv23_method());
> if (ctx == NULL)
> @@ -2054,14 +2090,29 @@ relay_tls_ctx_create(struct relay *rlay)
> /* Modify session timeout and cache size*/
> SSL_CTX_set_timeout(ctx,
> (long)MINIMUM(rlay->rl_conf.timeout.tv_sec, LONG_MAX));
> - if (proto->cache < -1) {
> + /* default timeout is 300, see SSL_get_default_timeout(3) */
> + if (proto->tlstimeout > 0)
> + SSL_CTX_set_timeout(ctx,
> + (long)MINIMUM(proto->tlstimeout, LONG_MAX));
> + if (proto->cache < -1)
> SSL_CTX_set_session_cache_mode(ctx, SSL_SESS_CACHE_OFF);
> - } else if (proto->cache >= -1) {
> + else if (proto->cache >= -1) {
> SSL_CTX_set_session_cache_mode(ctx, SSL_SESS_CACHE_SERVER);
> - if (proto->cache >= 0)
> + if (proto->cache >= 0) {
> SSL_CTX_sess_set_cache_size(ctx, proto->cache);
> + }
> + SSL_CTX_sess_set_new_cb(ctx, relay_tls_new_session);
> + SSL_CTX_sess_set_get_cb(ctx, relay_tls_get_session);
> + /* remove_cb unused because the cache runs a GC */
> }
>
> + /* Callback for TLS session tickets */
> + if (proto->tickets == -1)
> + SSL_CTX_set_options(ctx, SSL_OP_NO_TICKET);
> + else if (!SSL_CTX_set_tlsext_ticket_key_cb(ctx,
> + relay_tls_session_ticket))
> + log_warnx("could not set the TLS ticket callback");
> +
> /* Enable all workarounds and set SSL options */
> SSL_CTX_set_options(ctx, SSL_OP_ALL);
> SSL_CTX_set_options(ctx,
> @@ -2142,11 +2193,9 @@ relay_tls_ctx_create(struct relay *rlay)
> }
>
> /*
> - * Set session ID context to a random value. We don't support
> - * persistent caching of sessions so it is OK to set a temporary
> - * session ID context that is valid during run time.
> + * Set session ID context to a random value. It needs to be the
> + * same accross all relay processes or session caching will fail.
> */
> - arc4random_buf(sid, sizeof(sid));
> if (!SSL_CTX_set_session_id_context(ctx, sid, sizeof(sid)))
> goto err;
>
> @@ -2534,6 +2583,161 @@ relay_tls_writecb(int fd, short event, v
> cre->buflen = 0;
> }
> (*bufev->errorcb)(bufev, what, bufev->cbarg);
> +}
> +
> +int
> +relay_tls_new_session(SSL *ssl, SSL_SESSION *session)
> +{
> + struct iovec iov[3];
> + struct tls_session_hdr th;
> + struct imsgbuf *ibuf;
> + const unsigned char *id;
> + unsigned char *buf, *s;
> + unsigned int idlen;
> + int slen, cnt = 0;
> +
> + ibuf = proc_ibuf(env->sc_ps, PROC_TLSC, -1);
> +
> + id = SSL_SESSION_get_id(session, &idlen);
> +
> + /* Serialise the session. */
> + slen = i2d_SSL_SESSION(session, NULL);
> + if ((buf = s = malloc(slen)) == NULL)
> + fatal("relay_tls_new_session");
> + if (slen != i2d_SSL_SESSION(session, &buf))
> + fatalx("SSL SESSION unexpectly grew beyond buffer");
> +
> + /* build up imsg */
> + th.timeout = SSL_SESSION_get_timeout(session);
> + th.idlen = idlen;
> + th.slen = slen;
> +
> + iov[cnt].iov_base = &th;
> + iov[cnt++].iov_len = sizeof(th);
> + iov[cnt].iov_base = (void *)id;
> + iov[cnt++].iov_len = idlen;
> + iov[cnt].iov_base = (void *)s;
> + iov[cnt++].iov_len = slen;
It would be better to use a "struct ctl_tlsc", fill it in, and send it
at once. This manual filling can lead to errors and it doesn't match the
common style in relayd (which is always very important to me).
See also comments below in tlsc.c.
> +
> + imsg_composev(ibuf, IMSG_TLSC_ADD, 0, 0, -1, iov, cnt);
> + if (imsg_flush(ibuf) == -1)
> + log_warn("tlsc_modify: imsg_flush");
> +
> + free(s);
> + return 0; /* tell SSL to remove the reference on the session */
> +}
> +
> +SSL_SESSION *
> +relay_tls_get_session(SSL *ssl, unsigned char *id, int idlen, int *do_copy)
> +{
> + struct pollfd pfd[1];
> + struct iovec iov[2];
> + SSL_SESSION *session = NULL;
> + struct imsgbuf *ibuf;
> + const unsigned char *data;
> + struct imsg imsg;
> + int proc;
> + int n, done = 0, cnt = 0;
> +
> + ibuf = proc_ibuf(env->sc_ps, PROC_TLSC, -1);
> + proc = proc_id;
> +
> + iov[cnt].iov_base = &proc;
> + iov[cnt++].iov_len = sizeof(proc);
> + iov[cnt].iov_base = (void *)id;
> + iov[cnt++].iov_len = idlen;
> +
> + imsg_composev(ibuf, IMSG_TLSC_GET, 0, 0, -1, iov, cnt);
> + if (imsg_flush(ibuf) == -1)
> + log_warn("tlsc_lookup: imsg_flush");
> +
> + pfd[0].fd = ibuf->fd;
> + pfd[0].events = POLLIN;
> + while (!done) {
> + switch (poll(pfd, 1, 5 * 1000)) {
Same magic timeout number here, so you could even reuse one #define
and it provides a benefit ;)
> + case -1:
> + fatal("rsae_send_imsg: poll");
> + case 0:
> + log_warnx("tlsc_lookup: poll timeout");
> + break;
> + default:
> + break;
> + }
> + if ((n = imsg_read(ibuf)) == -1 && errno != EAGAIN)
> + fatalx("imsg_read");
> + if (n == 0)
> + fatalx("pipe closed");
> +
> + while (!done) {
> + if ((n = imsg_get(ibuf, &imsg)) == -1)
> + fatalx("imsg_get error");
> + if (n == 0)
> + break;
> + if (imsg.hdr.type != IMSG_TLSC_GET)
> + fatalx("invalid response");
> +
> + if (IMSG_DATA_SIZE(&imsg) != 0) {
> + data = imsg.data;
> + session = d2i_SSL_SESSION(NULL, &data,
> + IMSG_DATA_SIZE(&imsg));
> + }
> + done = 1;
> + imsg_free(&imsg);
> + }
> + }
> +
> + *do_copy = 0;
> + return (session);
> +}
> +
> +struct tls_ticket_ctx *
> +relay_get_ticket_key(unsigned char keyname[16])
> +{
> + if (keyname) {
> + if (timingsafe_memcmp(keyname,
> + tls_ticket_bak.key_name, sizeof(keyname)) == 0)
> + return &tls_ticket_bak;
> + if (timingsafe_memcmp(keyname,
> + tls_ticket.key_name, sizeof(keyname)) == 0)
> + return &tls_ticket;
> + return NULL;
> + }
> + return &tls_ticket;
> +}
> +
> +int
> +relay_tls_session_ticket(SSL *ssl, unsigned char keyname[16], unsigned char
> *iv,
> + EVP_CIPHER_CTX *ctx, HMAC_CTX *hctx, int mode)
> +{
> + struct tls_ticket_ctx *key;
> + struct timeval tv;
> +
> + if (mode == 1) {
> + /* create new session */
> + key = relay_get_ticket_key(NULL);
> + memcpy(keyname, key->key_name, 16);
> + arc4random_buf(iv, EVP_MAX_IV_LENGTH);
> + EVP_EncryptInit_ex(ctx, EVP_aes_128_cbc(), NULL,
> + key->aes_key, iv);
> + HMAC_Init_ex(hctx, key->hmac_key, 16, EVP_sha256(), NULL);
Is there a specific reason to use aes128-cbc-hmac-sha256 for the tickets?
> + return 0;
> + } else {
> + getmonotime(&tv);
> +
> + /* get key by name */
> + key = relay_get_ticket_key(keyname);
> + if (!key || key->expire < tv.tv_sec)
> + return 0;
> +
> + EVP_DecryptInit_ex(ctx, EVP_aes_128_cbc(), NULL,
> + key->aes_key, iv);
> + HMAC_Init_ex(hctx, key->hmac_key, 16, EVP_sha256(), NULL);
> +
> + /* time to renew the ticket? */
> + if (key->expire - TLS_TICKET_RENEW_TIME < tv.tv_sec)
> + return 2;
> + return 1;
> + }
> }
>
> int
> Index: relayd.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/relayd/relayd.c,v
> retrieving revision 1.155
> diff -u -p -r1.155 relayd.c
> --- relayd.c 29 Jul 2016 10:09:26 -0000 1.155
> +++ relayd.c 8 Aug 2016 10:04:06 -0000
> @@ -70,7 +70,8 @@ static struct privsep_proc procs[] = {
> { "pfe", PROC_PFE, parent_dispatch_pfe, pfe },
> { "hce", PROC_HCE, parent_dispatch_hce, hce },
> { "relay", PROC_RELAY, parent_dispatch_relay, relay },
> - { "ca", PROC_CA, parent_dispatch_ca, ca }
> + { "ca", PROC_CA, parent_dispatch_ca, ca },
> + { "tlsc", PROC_TLSC, NULL, tlsc }
> };
>
> void
> @@ -635,6 +636,8 @@ purge_relay(struct relayd *env, struct r
> rlay->rl_tls_capkey = NULL;
> }
>
> + if (rlay->rl_ssl_ctx)
> + SSL_CTX_sess_set_remove_cb(rlay->rl_ssl_ctx, NULL);
> SSL_CTX_free(rlay->rl_ssl_ctx);
>
> while ((rlt = TAILQ_FIRST(&rlay->rl_tables))) {
> Index: relayd.conf.5
> ===================================================================
> RCS file: /cvs/src/usr.sbin/relayd/relayd.conf.5,v
> retrieving revision 1.171
> diff -u -p -r1.171 relayd.conf.5
> --- relayd.conf.5 18 Aug 2016 14:12:51 -0000 1.171
> +++ relayd.conf.5 30 Aug 2016 10:50:16 -0000
> @@ -973,6 +973,11 @@ is zero, the default size defined by the
> A positive number will set the maximum size in bytes and the keyword
> .Ic disable
> will disable the TLS session cache.
> +.It Ic session timeout Ar value
> +Set the timeout for TLS session cache entries.
> +If not set, the default defined by the TLS library will be used.
> +.It Ic session tickets disable
> +Disable TLS session tickets.
> .It Xo
> .Op Ic no
> .Ic sslv3
> Index: relayd.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/relayd/relayd.h,v
> retrieving revision 1.225
> diff -u -p -r1.225 relayd.h
> --- relayd.h 29 Jul 2016 10:09:27 -0000 1.225
> +++ relayd.h 30 Aug 2016 10:50:16 -0000
> @@ -711,6 +711,8 @@ struct protocol {
> char *tlscapass;
> char name[MAX_NAME_SIZE];
> int cache;
> + int tickets;
> + unsigned long tlstimeout;
> enum prototype type;
> char *style;
>
> @@ -963,7 +965,10 @@ enum imsg_type {
> IMSG_CA_PRIVENC,
> IMSG_CA_PRIVDEC,
> IMSG_SESS_PUBLISH, /* from relay to hce */
> - IMSG_SESS_UNPUBLISH
> + IMSG_SESS_UNPUBLISH,
> + IMSG_TLSC_ADD,
> + IMSG_TLSC_GET,
> + IMSG_TLSC_REKEY
> };
>
> enum privsep_procid {
> @@ -973,6 +978,7 @@ enum privsep_procid {
> PROC_RELAY,
> PROC_PFE,
> PROC_CA,
> + PROC_TLSC,
> PROC_MAX
> } privsep_process;
>
> @@ -1078,6 +1084,22 @@ struct relayd {
> int sc_reload;
> };
>
> +struct tls_session_hdr {
> + size_t idlen;
> + size_t slen;
> + long timeout;
> +};
> +
> +struct tls_ticket_ctx {
> + time_t expire;
> + unsigned char key_name[16];
> + unsigned char aes_key[16];
> + unsigned char hmac_key[16];
Where are all these 16 from? Please provide it a #define as well, or
use one from libssl if it is coming from there. I don't like such
magic number without explanation.
> +};
> +#define TLS_TICKET_LIFE_TIME (4 * 3600)
> +#define TLS_TICKET_RENEW_TIME 600
> +#define TLS_CACHE_GC_INTERVAL 60
> +
I guess I have to sort relayd.h a bit, but please put such global-like
defines on top in a new block after the RELAY_ ones.
> #define FSNMP_TRAPONLY 0x01
>
> #define RELAYD_OPT_VERBOSE 0x01
> @@ -1239,6 +1261,11 @@ int ssl_ctx_fake_private_key(SSL_CTX *,
> /* ca.c */
> pid_t ca(struct privsep *, struct privsep_proc *);
> void ca_engine_init(struct relayd *);
> +
> +/* tlsc.c */
> +pid_t tlsc(struct privsep *, struct privsep_proc *);
> +void tlsc_engine_init(struct relayd *);
> +void tlsc_create_ticket(struct tls_ticket_ctx *);
>
> /* relayd.c */
> struct host *host_find(struct relayd *, objid_t);
> Index: tlsc.c
> ===================================================================
> RCS file: tlsc.c
> diff -N tlsc.c
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ tlsc.c 24 Jul 2016 13:07:50 -0000
> @@ -0,0 +1,282 @@
> +/* $OpenBSD$ */
> +
> +/*
> + * Copyright (c) 2016 Claudio Jeker <clau...@openbsd.org>
> + *
> + * Permission to use, copy, modify, and distribute this software for any
> + * purpose with or without fee is hereby granted, provided that the above
> + * copyright notice and this permission notice appear in all copies.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> + */
> +
> +#include <sys/types.h>
> +#include <sys/queue.h>
> +#include <sys/tree.h>
> +#include <sys/uio.h>
> +
> +#include <unistd.h>
> +#include <string.h>
> +#include <stdlib.h>
> +#include <poll.h>
> +#include <imsg.h>
> +
> +#include "relayd.h"
> +
> +struct tlsc_get {
> + size_t idlen;
> + int proc;
> +};
This struct doesn't seem to be used anywhere?
As mentioned before, I guess you meant to create something like a
"struct ctl_tlsc", but it is implemented in a different way below.
You should use it, prefix it ctl_ for consistency.
> +
> +struct tlsc {
> + RB_ENTRY(tlsc) entry;
> + size_t idlen;
> + size_t slen;
> + time_t expire;
> + const unsigned char *id;
> + const unsigned char *session;
relayd has a weird mix of un-prefixed and prefixed struct members.
I'd prefer the latter to follow what is being used where (eg. I just
had difficulties below to figure out if x->idlen was from tlsc_get or
tlsc).
> +};
> +
> +void tlsc_init(struct privsep *, struct privsep_proc *p, void *);
> +void tlsc_launch(void);
> +void tlsc_rekey(int, short, void *);
> +int tlsc_dispatch_parent(int, struct privsep_proc *, struct imsg *);
> +int tlsc_dispatch_relay(int, struct privsep_proc *, struct imsg *);
> +
> +int tlsc_compare(struct tlsc *, struct tlsc *);
> +struct tlsc *tlsc_get(const void *, size_t);
> +void tlsc_add(const void *, size_t, const void *, size_t, long);
> +void tlsc_flush(void);
> +void tlsc_gc(int, short, void *);
> +
> +RB_HEAD(tlsc_tree, tlsc) tls_cache = RB_INITIALIZER(&tls_cache);
> +RB_PROTOTYPE(tlsc_tree, tlsc, entry, tlsc_compare)
> +RB_GENERATE(tlsc_tree, tlsc, entry, tlsc_compare)
> +
> +static struct relayd *env = NULL;
> +extern int proc_id;
> +
> +static struct event rekeyev;
> +static struct event gcev;
Most of my comments seem like nit-picking, but I really like
consistency / common style. Accordingly, it would be better to put
them into the global "struct relayd".
> +
> +static struct privsep_proc procs[] = {
> + { "parent", PROC_PARENT, tlsc_dispatch_parent },
> + { "relay", PROC_RELAY, tlsc_dispatch_relay },
> +};
> +
> +pid_t
> +tlsc(struct privsep *ps, struct privsep_proc *p)
> +{
> + env = ps->ps_env;
> +
> + return (proc_run(ps, p, procs, nitems(procs), tlsc_init, NULL));
> +}
> +
> +void
> +tlsc_init(struct privsep *ps, struct privsep_proc *p, void *arg)
> +{
> + if (pledge("stdio", NULL) == -1)
> + fatal("pledge");
> +
> + if (config_init(ps->ps_env) == -1)
> + fatal("failed to initialize configuration");
> +}
> +
> +void
> +tlsc_launch(void)
> +{
> + struct timeval tv;
> +
> + /* flush the cache after reconfigure, the same happens in the relays */
> + tlsc_flush();
> +
> + /* Schedule rekey timer */
> + evtimer_set(&rekeyev, tlsc_rekey, NULL);
> + timerclear(&tv);
> + tv.tv_sec = TLS_CACHE_GC_INTERVAL;
> + evtimer_add(&rekeyev, &tv);
> +
> + /* Schedule GC timer */
> + evtimer_set(&gcev, tlsc_gc, NULL);
> + timerclear(&tv);
> + tv.tv_sec = 60;
I think that would be TLS_CACHE_GC_INTERVAL instead of 60.
> + evtimer_add(&gcev, &tv);
> +}
> +
> +void
> +tlsc_rekey(int fd, short events, void *arg)
> +{
> + struct timeval tv;
> + struct tls_ticket_ctx ticket;
> +
> + log_debug("tlsc_rekey: rekeying tickets");
> +
> + tlsc_create_ticket(&ticket);
> + proc_compose_imsg(env->sc_ps, PROC_RELAY, -1, IMSG_TLSC_REKEY, -1, -1,
> + &ticket, sizeof(ticket));
> +
> + evtimer_set(&rekeyev, tlsc_rekey, NULL);
> + timerclear(&tv);
> + tv.tv_sec = TLS_TICKET_LIFE_TIME;
> + evtimer_add(&rekeyev, &tv);
> +}
> +
> +int
> +tlsc_dispatch_parent(int fd, struct privsep_proc *p, struct imsg *imsg)
> +{
> + switch (imsg->hdr.type) {
> + case IMSG_CTL_START:
> + tlsc_launch();
> + break;
> + case IMSG_CFG_DONE:
> + /* nothing to do here */
> + break;
> + default:
> + return -1;
> + }
> + return 0;
> +}
> +
> +int
> +tlsc_dispatch_relay(int fd, struct privsep_proc *p, struct imsg *imsg)
> +{
> + struct tls_session_hdr th;
> + int proc;
> + unsigned char *id, *s;
> + struct tlsc *t;
> +
> + switch (imsg->hdr.type) {
> + case IMSG_TLSC_ADD:
> + IMSG_SIZE_CHECK(imsg, (&th));
> + memcpy(&th, imsg->data, sizeof(th));
> + if (IMSG_DATA_SIZE(imsg) != (sizeof(th) + th.idlen + th.slen))
> + fatalx("tlsc_dispatch_relay: bad TLS cache imsg");
> + id = (unsigned char *)imsg->data + sizeof(th);
> + s = id + th.idlen;
This would clearly benefit from a single "struct ctl_tlsc".
> +
> + tlsc_add(id, th.idlen, s, th.slen, th.timeout);
> + break;
> + case IMSG_TLSC_GET:
> + IMSG_SIZE_CHECK(imsg, (&proc));
> + memcpy(&proc, imsg->data, sizeof(proc));
> +
> + t = tlsc_get((unsigned char *)imsg->data + sizeof(proc),
> + IMSG_DATA_SIZE(imsg) - sizeof(proc));
> + if (t == NULL)
> + proc_compose_imsg(env->sc_ps, PROC_RELAY, proc,
> + imsg->hdr.type, -1, -1, NULL, 0);
> + else
> + proc_compose_imsg(env->sc_ps, PROC_RELAY, proc,
> + imsg->hdr.type, -1, -1,
> + (void *)t->session, t->slen);
> + break;
> + default:
> + return (-1);
> + }
> +
> + return (0);
> +}
> +
> +int
> +tlsc_compare(struct tlsc *a, struct tlsc *b)
> +{
> + if (a->idlen < b->idlen)
> + return -1;
> + if (a->idlen > b->idlen)
> + return 1;
> + return memcmp(a->id, b->id, a->idlen);
> +}
> +
> +struct tlsc *
> +tlsc_get(const void *id, size_t idlen)
> +{
> + struct tlsc tt;
> +
> + memset(&tt, 0, sizeof(tt));
> + tt.id = id;
> + tt.idlen = idlen;
> +
> + return RB_FIND(tlsc_tree, &tls_cache, &tt);
> +}
> +
> +void
> +tlsc_add(const void *id, size_t idlen, const void *s, size_t slen, long
> timeout)
> +{
> + struct timeval tv;
> + struct tlsc *t;
> + unsigned char *b;
> + size_t l;
> +
> + l = sizeof(*t) + idlen + slen;
> + if ((t = malloc(l)) == NULL)
> + fatal("tlsc_add");
> + memset(t, 0, sizeof(*t));
Better use calloc(1, here. One line, easier to read, common style.
> + b = (unsigned char *)(t + 1);
> + memcpy(b, id, idlen);
> + t->id = b;
> + t->idlen = idlen;
> + b += idlen;
> + memcpy(b, s, slen);
> + t->session = b;
> + t->slen = slen;
> +
> + getmonotime(&tv);
> + t->expire = tv.tv_sec + timeout;
> +
> + if (RB_INSERT(tlsc_tree, &tls_cache, t)) {
> + log_warnx("tlsc_add: session already in cache");
> + free(t);
> + }
> +}
> +
> +void
> +tlsc_flush(void)
> +{
> + struct tlsc *t;
> +
> + while ((t = RB_MIN(tlsc_tree, &tls_cache)) != NULL) {
> + RB_REMOVE(tlsc_tree, &tls_cache, t);
> + free(t);
> + }
> +}
> +
> +void
> +tlsc_gc(int fd, short events, void *arg)
> +{
> + struct timeval tv;
> + struct tlsc *t, *tt;
> +
> + getmonotime(&tv);
> +
> + /* remove expired entries from the cache */
> + RB_FOREACH_SAFE(t, tlsc_tree, &tls_cache, tt) {
> + if (t->expire < tv.tv_sec) {
> + RB_REMOVE(tlsc_tree, &tls_cache, t);
> + free(t);
> + }
> + }
> +
> + evtimer_set(&gcev, tlsc_gc, NULL);
> + timerclear(&tv);
> + tv.tv_sec = TLS_CACHE_GC_INTERVAL;
> + evtimer_add(&gcev, &tv);
> +}
> +
> +void
> +tlsc_create_ticket(struct tls_ticket_ctx *key)
> +{
> + struct timeval tv;
> +
> + getmonotime(&tv);
> + key->expire = tv.tv_sec + TLS_TICKET_LIFE_TIME;
> +
> + arc4random_buf(key->key_name, sizeof(key->key_name));
> + arc4random_buf(key->hmac_key, sizeof(key->hmac_key));
> + arc4random_buf(key->aes_key, sizeof(key->aes_key));
> +}
>
--
