Quite Frankly, we're happy to support what's needed in relayd,
But first relayd needs to actually convert to use libtls instead of bare
knuckles shit
Until then we're just making the problem worse.
IMO, we should convert relayd to use libtls - (add what we need to libtls
to support it)
before adding *more* stuff to relayd using bareknuckles OpenSSL shit.. or
we are just making the problem worse.
We should convert it (and deal with whatever needs it has in libtls) before
adding
more functionality to it
-Bob
On Tue, Aug 30, 2016 at 8:11 AM, Reyk Floeter <r...@openbsd.org> wrote:
> 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 <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?
> > >
> >
> > 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
>
>
