On Tue, Aug 30, 2016 at 03:51:04PM +0200, Claudio Jeker wrote:
> On Tue, Aug 30, 2016 at 02:44:17PM +0200, Reyk Floeter wrote:
> > 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 */
> >
>
> Done.
>
> > > + 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.
> >
>
> Since I only wanted to have sid initialized in the relay process I came to
> the conclusion that this is the better place to do the initialisation.
>
> > 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).
>
> I agree fork+exec will need some extra work. Will think about it... guess
> the rekey could move to the parent.
>
> > > + 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.
> >
>
> Hmm. This is more or less a verbatim copy from ca.c. Since the id and
> session buffers are dynamic length and their size is not known it is not
> easily possible to wrap this into a struct. I renamed tls_session_hdr
> to ctl_tlsc_hdr.
>
> > > +
> > > + 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 ;)
> >
>
> Done as well.
>
> > > + 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?
> >
>
> This is what LibreSSL does. Since this is lots of magic I tried to keep it
> as close to LibreSSL code as possible.
> See lib/libssl/src/ssl/t1_lib.c::tls_decrypt_ticket()
> I'm not sure if it is save to use different crypt/mac pairs for this.
>
> This is dark undocumented territory of OpenSSL. I wonder if
> SSL_CTX_set_tlsext_ticket_keys() would be another option to do this. Will
> talk to bob@ and maybe make him scream....
>
OK, that makes sense.
I'm fine with copying it for now, but it should be considered and
discussed with the TLS people (including beck@ and jsing@).
> > > + 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.
> >
>
> LibreSSL code is using 16 and it has to be 16 or shit fails.
> I can call it TLS_MAGIC_SIZE but I doubt it will help. I will add a
> comment for this though.
>
All the black magic from libssl ;)
OK, TLS_MAGIC_SIZE or TLS_MAGIC_NAME_SIZE and a comment would be the best.
> > > +};
> > > +#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.
>
> Hmm. Can we do the move / cleanup after it is in? At least then you can
> decide where to put it.
>
OK
> >
> > > #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 <[email protected]>
> > > + *
> > > + * 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?
> >
>
> Killed it.
>
> > 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).
> >
>
> Done.
>
> > > +};
> > > +
> > > +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".
> >
>
> I think this is not helping here. These timeouts are used in one place
> only and adding them to struct relayd make that struct just fatter for no
> real benefit. I don't mind sharing common data structures and config that
> way but event structs are something I see no benefit to be in there.
>
But struct relayd gives me a context and prefix and I can see what is
done where. All these events all over the place are easy to lose...
But, OK, if you insist. The global struct relayd / env / conf will
get some cleanup anyway (eg. for fork+exec) and we might just tweak it
later.
> > > +
> > > +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.
> >
>
> Oups. This is actually a bad debug leftover. Yes this should be
> TLS_CACHE_GC_INTERVAL and rekey needs to use TLS_TICKET_LIFE_TIME.
> Fixed.
>
> > > + 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".
> >
>
> It is not realy possible because the id length is unknown.
>
I see.
> > > +
> > > + 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.
>
> Hmm. I think I stole this example from LibreSSL. Anyway easy change to do.
>
> >
> > > + 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));
> > > +}
> > >
> >
> > --
> >
>
> Will send out an update diff later.
> --
> :wq Claudio
Thanks.
As I'm in the plane soon - I think you'll have my OK on it.
Reyk
