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....
> > + 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.
> > +};
> > +#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.
>
> > #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.
> > +
> > +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.
> > +
> > + 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
