On Thu, Sep 01, 2016 at 11:27:55AM +0200, Claudio Jeker 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.
> > >
>
> After some more talking to people here a new version that drops the
> session cache and only implements session tickets. Everything modern (aka
> the stuff we care about) implements tickets and they are so much easier to
> work with. The system in place now uses a primary key and a backup key and
> will refresh the tickets for everything that was encrypted with the backup
> key. The keys are rotated every 2h so at max a key is valid for 4h.
> We set the tls session timeout also to 2h so that this value is announced
> as TLS session ticket lifetime hint. It is still possible to disable the
> tickets per relay just in case.
>
See comments below, otherwise OK
> --
> :wq Claudio
>
> 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 30 Aug 2016 12:59:36 -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, TLS_PRIV_SEND_TIMEOUT)) {
> + 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 31 Aug 2016 12:53:03 -0000
> @@ -94,7 +94,6 @@ config_init(struct relayd *env)
> bzero(&env->sc_proto_default, sizeof(env->sc_proto_default));
> env->sc_proto_default.id = EMPTY_ID;
> env->sc_proto_default.flags = F_USED;
> - env->sc_proto_default.cache = RELAY_CACHESIZE;
> env->sc_proto_default.tcpflags = TCPFLAG_DEFAULT;
> env->sc_proto_default.tcpbacklog = RELAY_BACKLOG;
> env->sc_proto_default.tlsflags = TLSFLAG_DEFAULT;
> 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 1 Sep 2016 08:25:35 -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 tlstickets
> %type <v.number> redirect_proto relay_proto match
> %type <v.number> action ruleaf key_option
> %type <v.number> tlsdhparams tlsecdhcurve
> @@ -996,7 +996,6 @@ proto : relay_proto PROTO STRING {
> free($3);
> p->id = ++last_proto_id;
> p->type = $1;
> - p->cache = RELAY_CACHESIZE;
> p->tcpflags = TCPFLAG_DEFAULT;
> p->tlsflags = TLSFLAG_DEFAULT;
> p->tcpbacklog = RELAY_BACKLOG;
> @@ -1091,7 +1090,7 @@ tlsflags_l : tlsflags comma tlsflags_l
> | tlsflags
> ;
>
> -tlsflags : SESSION CACHE tlscache { proto->cache = $3; }
> +tlsflags : SESSION TICKETS tlstickets { proto->tickets = $3; }
> | CIPHERS STRING {
> if (strlcpy(proto->tlsciphers, $2,
> sizeof(proto->tlsciphers)) >=
> @@ -1180,15 +1179,7 @@ flag : STRING {
> }
> ;
>
> -tlscache : NUMBER {
> - if ($1 < 0) {
> - yyerror("invalid tlscache value: %d", $1);
> - YYERROR;
> - }
> - $$ = $1;
> - }
> - | DISABLE { $$ = -2; }
> - ;
> +tlstickets : DISABLE { $$ = -1; }
the grammar should match the other options
[no] session tickets
instead of
session tickets disable
see manpage bits below
>
> filterrule : action dir quick ruleaf rulesrc ruledst {
> if ((rule = calloc(1, sizeof(*rule))) == NULL)
> @@ -2257,6 +2248,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 1 Sep 2016 08:54:00 -0000
> @@ -28,6 +28,7 @@
> #include <arpa/inet.h>
>
> #include <limits.h>
> +#include <poll.h>
> #include <stdio.h>
> #include <stdlib.h>
> #include <errno.h>
> @@ -85,6 +86,10 @@ void relay_tls_connected(struct ctl_re
> void relay_tls_readcb(int, short, void *);
> void relay_tls_writecb(int, short, void *);
>
> +struct tls_ticket *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,
> size_t, void *);
> @@ -107,6 +112,7 @@ pid_t
> relay(struct privsep *ps, struct privsep_proc *p)
> {
> pid_t pid;
> +
> env = ps->ps_env;
> pid = proc_run(ps, p, procs, nitems(procs), relay_init, NULL);
> relay_http(env);
> @@ -260,8 +266,8 @@ relay_protodebug(struct relay *rlay)
> if ((rlay->rl_conf.flags & (F_TLS|F_TLSCLIENT)) && proto->tlsflags)
> fprintf(stderr, "\ttls flags: %s\n",
> 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, "\ttype: ");
> switch (proto->type) {
> case RELAY_PROTO_TCP:
> @@ -1924,6 +1930,15 @@ relay_dispatch_parent(int fd, struct pri
> case IMSG_CTL_RESET:
> config_getreset(env, imsg);
> break;
> + case IMSG_TLSTICKET_REKEY:
> + IMSG_SIZE_CHECK(imsg, (&env->sc_tls_ticket));
> + /* rotate keys */
> + memcpy(&env->sc_tls_ticket_bak, &env->sc_tls_ticket,
> + sizeof(env->sc_tls_ticket));
> + env->sc_tls_ticket_bak.tt_backup = 1;
> + memcpy(&env->sc_tls_ticket, imsg->data,
> + sizeof(env->sc_tls_ticket));
> + break;
> default:
> return (-1);
> }
> @@ -2045,21 +2060,27 @@ 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)
> goto err;
>
> - /* 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) {
> - SSL_CTX_set_session_cache_mode(ctx, SSL_SESS_CACHE_OFF);
> - } else if (proto->cache >= -1) {
> - SSL_CTX_set_session_cache_mode(ctx, SSL_SESS_CACHE_SERVER);
> - if (proto->cache >= 0)
> - SSL_CTX_sess_set_cache_size(ctx, proto->cache);
> + /*
> + * Disable the session cache by default.
> + * Everything modern uses tickets
> + */
> + SSL_CTX_set_session_cache_mode(ctx, SSL_SESS_CACHE_OFF);
> +
> +
> + /* Set callback for TLS session tickets if enabled */
> + 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");
> + /* set timeout to the ticket rekey time */
> + SSL_CTX_set_timeout(ctx, TLS_TICKET_REKEY_TIME);
> }
>
> /* Enable all workarounds and set SSL options */
> @@ -2142,12 +2163,11 @@ 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)))
> + if (!SSL_CTX_set_session_id_context(ctx, env->sc_tls_sid,
> + sizeof(env->sc_tls_sid)))
> goto err;
>
> /* The text versions of the keys/certs are not needed anymore */
> @@ -2534,6 +2554,57 @@ relay_tls_writecb(int fd, short event, v
> cre->buflen = 0;
> }
> (*bufev->errorcb)(bufev, what, bufev->cbarg);
> +}
> +
> +struct tls_ticket *
> +relay_get_ticket_key(unsigned char keyname[16])
> +{
> + if (keyname) {
> + if (timingsafe_memcmp(keyname,
> + env->sc_tls_ticket_bak.tt_key_name, sizeof(keyname)) == 0)
> + return &env->sc_tls_ticket_bak;
> + if (timingsafe_memcmp(keyname,
> + env->sc_tls_ticket.tt_key_name, sizeof(keyname)) == 0)
> + return &env->sc_tls_ticket;
> + return NULL;
> + }
> + return &env->sc_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 *key;
> +
> + if (mode == 1) {
> + /* create new session */
> + key = relay_get_ticket_key(NULL);
> + memcpy(keyname, key->tt_key_name, 16);
> + arc4random_buf(iv, EVP_MAX_IV_LENGTH);
> + EVP_EncryptInit_ex(ctx, EVP_aes_128_cbc(), NULL,
> + key->tt_aes_key, iv);
> + HMAC_Init_ex(hctx, key->tt_hmac_key, 16, EVP_sha256(), NULL);
> + log_debug("XXX create tls ticket");
I think you missed some log_debug("XXX debug statements that should
not go in this way.
> + return 0;
> + } else {
> + /* get key by name */
> + key = relay_get_ticket_key(keyname);
> + if (!key)
> + return 0;
> +
> + EVP_DecryptInit_ex(ctx, EVP_aes_128_cbc(), NULL,
> + key->tt_aes_key, iv);
> + HMAC_Init_ex(hctx, key->tt_hmac_key, 16, EVP_sha256(), NULL);
> +
> + /* time to renew the ticket? */
> + if (key->tt_backup) {
> + log_debug("XXX rekey tls ticket");
...
> + return 2;
> + }
> + log_debug("XXX tls ticket primary key");
...
> + 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 1 Sep 2016 08:53:44 -0000
> @@ -63,6 +63,7 @@ int parent_dispatch_relay(int, struct
> int parent_dispatch_ca(int, struct privsep_proc *,
> struct imsg *);
> int bindany(struct ctl_bindany *);
> +void parent_tls_ticket_rekey(int, short, void *);
>
> struct relayd *relayd_env;
>
> @@ -212,6 +213,8 @@ main(int argc, char *argv[])
> TAILQ_INIT(&env->sc_hosts);
> TAILQ_INIT(&env->sc_sessions);
> env->sc_rtable = getrtable();
> + /* initialize the TLS session id to a random key for all relay procs */
> + arc4random_buf(env->sc_tls_sid, sizeof(env->sc_tls_sid));
>
> if (parse_config(env->sc_conffile, env) == -1)
> exit(1);
> @@ -278,6 +281,8 @@ main(int argc, char *argv[])
> if (env->sc_flags & (F_TLS|F_TLSCLIENT))
> ssl_init(env);
>
> + /* rekey the TLS tickets before pushing the config */
> + parent_tls_ticket_rekey(0, 0, env);
> if (parent_configure(env) == -1)
> fatalx("configuration failed");
>
> @@ -1670,4 +1675,28 @@ accept_reserve(int sockfd, struct sockad
> DPRINTF("%s: inflight incremented, now %d",__func__, *counter);
> }
> return (ret);
> +}
> +
> +void
> +parent_tls_ticket_rekey(int fd, short events, void *arg)
> +{
> + static struct event rekeyev;
> + struct relayd *env = arg;
> + struct timeval tv;
> + struct tls_ticket key;
> +
> + log_debug("relayd_tls_ticket_rekey: rekeying tickets");
> +
> + arc4random_buf(key.tt_key_name, sizeof(key.tt_key_name));
> + arc4random_buf(key.tt_hmac_key, sizeof(key.tt_hmac_key));
> + arc4random_buf(key.tt_aes_key, sizeof(key.tt_aes_key));
> + key.tt_backup = 0;
> +
> + proc_compose_imsg(env->sc_ps, PROC_RELAY, -1, IMSG_TLSTICKET_REKEY,
> + -1, -1, &key, sizeof(key));
> +
> + evtimer_set(&rekeyev, parent_tls_ticket_rekey, env);
> + timerclear(&tv);
> + tv.tv_sec = TLS_TICKET_REKEY_TIME;
> + evtimer_add(&rekeyev, &tv);
> }
> 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 1 Sep 2016 08:25:07 -0000
> @@ -965,14 +965,8 @@ TLS clients.
> .It Ic no edh
> Disable EDH support.
> This is the default.
> -.It Ic session cache Ar value
> -Set the maximum size of the TLS session cache.
> -If the
> -.Ar value
> -is zero, the default size defined by the TLS library will be used.
> -A positive number will set the maximum size in bytes and the keyword
> -.Ic disable
> -will disable the TLS session cache.
> +.It Ic session tickets disable
> +Disable TLS session tickets.
After updating the grammar, it would probably also be worth mentioning
that relayd support TLS tickets instead of session caching, something
like:
.It Xo
.Op Ic no
.Ic session tickets
.Xc
Disable TLS session tickets; enabled by default.
.Nm
supports stateless TLS session tickets instead of the older protocol
option of a shared TLS session cache.
Reyk
> .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 1 Sep 2016 08:52:08 -0000
> @@ -693,6 +693,16 @@ TAILQ_HEAD(relay_rules, relay_rule);
> #define TLSDHPARAMS_DEFAULT 0
> #define TLSDHPARAMS_MIN 1024
>
> +struct tls_ticket {
> + /* The key, aes key and hmac key must be 16 bytes / 128bits */
> + unsigned char tt_key_name[16];
> + unsigned char tt_aes_key[16];
> + unsigned char tt_hmac_key[16];
> + int tt_backup;
> +};
> +#define TLS_TICKET_REKEY_TIME (2 * 3600)
> +#define TLS_PRIV_SEND_TIMEOUT 5000 /* 5 sec */
> +
> struct protocol {
> objid_t id;
> u_int32_t flags;
> @@ -710,7 +720,7 @@ struct protocol {
> char tlscakey[PATH_MAX];
> char *tlscapass;
> char name[MAX_NAME_SIZE];
> - int cache;
> + int tickets;
> enum prototype type;
> char *style;
>
> @@ -963,7 +973,8 @@ enum imsg_type {
> IMSG_CA_PRIVENC,
> IMSG_CA_PRIVDEC,
> IMSG_SESS_PUBLISH, /* from relay to hce */
> - IMSG_SESS_UNPUBLISH
> + IMSG_SESS_UNPUBLISH,
> + IMSG_TLSTICKET_REKEY
> };
>
> enum privsep_procid {
> @@ -1076,6 +1087,10 @@ struct relayd {
>
> struct privsep *sc_ps;
> int sc_reload;
> +
> + char sc_tls_sid[SSL_MAX_SID_CTX_LENGTH];
> + struct tls_ticket sc_tls_ticket;
> + struct tls_ticket sc_tls_ticket_bak;
> };
>
> #define FSNMP_TRAPONLY 0x01
--
