eliminate double books for session expiration

Session expiration is now handled only by security
module. That simplifies the logic significantly.

src/cookies.c

@@ -169,141 +169,3 @@ cleanup:
 
 }
 
-void cookie_db_deinit(struct cookie_entry_db_st* db)
-{
-struct cookie_entry_st * e;
-struct htable_iter iter;
-
-	e = htable_first(db->db, &iter);
-	while(e != NULL) {
-		if (e->proc)
-			e->proc->cookie_ptr = NULL;
-
-		e->proc = NULL;
-		talloc_free(e);
-
-		e = htable_next(db->db, &iter);
-	}
-	htable_clear(db->db);
-	talloc_free(db->db);
-
-	return;
-}
-
-void expire_cookies(struct cookie_entry_db_st* db)
-{
-struct cookie_entry_st * e;
-struct htable_iter iter;
-time_t now = time(0);
-
-	e = htable_first(db->db, &iter);
-	while(e != NULL) {
-		if (e->expiration == -1 || now < e->expiration)
-			goto cont;
-
-		if (e->proc) {
-			syslog(LOG_ERR, "found proc that references expired cookie!");
-			e->proc->cookie_ptr = NULL;
-		}
-
-		htable_delval(db->db, &iter);
-		db->total--;
-
-		talloc_free(e);
- cont:
-		e = htable_next(db->db, &iter);
-	}
-
-	return;
-}
-
-static size_t rehash(const void* _e, void* unused)
-{
-const struct cookie_entry_st * e = _e;
-
-	return hash_any(e->cookie_hash, sizeof(e->cookie_hash), 0);
-}
-
-void cookie_db_init(void *pool, struct cookie_entry_db_st* db)
-{
-	db->db = talloc(pool, struct htable);
-	htable_init(db->db, rehash, NULL);
-	db->total = 0;
-}
-
-static bool cookie_entry_cmp(const void* _c1, void* _c2)
-{
-const struct cookie_entry_st* c1 = _c1;
-struct cookie_entry_st* c2 = _c2;
-
-	if (memcmp(c1->cookie_hash, c2->cookie_hash, sizeof(c1->cookie_hash)) == 0)
-		return 1;
-
-	return 0;
-}
-
-struct cookie_entry_st *find_cookie_entry(struct cookie_entry_db_st* db, void *cookie, unsigned cookie_size)
-{
-	struct cookie_entry_st *e;
-	struct cookie_entry_st t;
-	int ret;
-
-	ret = gnutls_hash_fast(COOKIE_HASH, cookie, cookie_size, t.cookie_hash);
-	if (ret < 0) {
-		return NULL;
-	}
-
-	e = htable_get(db->db, hash_any(t.cookie_hash, sizeof(t.cookie_hash), 0), cookie_entry_cmp, &t);
-	if (e == NULL)
-		return NULL;
-
-	if (e->expiration != -1 && e->expiration < time(0)) {
-		delete_cookie(db, e);
-		return NULL;
-	}
-
-	return e;
-}
-
-struct cookie_entry_st *new_cookie_entry(struct cookie_entry_db_st* db, proc_st *proc, void *cookie, unsigned cookie_size)
-{
-	struct cookie_entry_st *t;
-	int ret;
-
-	t = talloc(db->db, struct cookie_entry_st);
-	if (t == NULL)
-		return NULL;
-
-	t->expiration = -1;
-
-	ret = gnutls_hash_fast(COOKIE_HASH, cookie, cookie_size, t->cookie_hash);
-	if (ret < 0) {
-		goto fail;
-	}
-
-	t->proc = proc;
-
-	if (htable_add(db->db, rehash(t, NULL), t) == 0) {
-		goto fail;
-	}
-
-	db->total++;
-
-	return t;
-
- fail:
- 	talloc_free(t);
- 	return NULL;
-}
-
-void delete_cookie(struct cookie_entry_db_st* db, struct cookie_entry_st *e)
-{
-	if (e->proc) {
-		syslog(LOG_ERR, "found proc that references cookie to be deleted!");
-		e->proc->cookie_ptr = NULL;
-	}
-	htable_del(db->db, rehash(e, NULL), e);
-	db->total--;
-	talloc_free(e);
-	return;
-}

src/cookies.h

@@ -37,16 +37,4 @@ int decrypt_cookie(ProtobufCAllocator *pa, gnutls_datum_t *key,
 			uint8_t *cookie, unsigned cookie_size, 
 			Cookie **msg);
 
-void cookie_db_init(void *pool, struct cookie_entry_db_st* db);
-void cookie_db_deinit(struct cookie_entry_db_st* db);
-void expire_cookies(struct cookie_entry_db_st* db);
-struct cookie_entry_st *new_cookie_entry(struct cookie_entry_db_st* db, proc_st *proc, void *cookie, unsigned cookie_size);
-struct cookie_entry_st *find_cookie_entry(struct cookie_entry_db_st* db, void *cookie, unsigned cookie_len);
-void delete_cookie(struct cookie_entry_db_st* db, struct cookie_entry_st *e);
-
-inline static void revive_cookie(struct cookie_entry_st * e)
-{
-	e->expiration = -1;
-}
-
 #endif

src/main-auth.c

@@ -170,11 +170,10 @@ int handle_auth_cookie_req(main_server_st* s, struct proc_st* proc,
 {
 int ret;
 Cookie *cmsg;
-time_t now = time(0);
 gnutls_datum_t key = {s->cookie_key, sizeof(s->cookie_key)};
 char str_ip[MAX_IP_STR+1];
 PROTOBUF_ALLOCATOR(pa, proc);
-struct cookie_entry_st *old = NULL;
+struct proc_st *old_proc;
 
 	if (req->cookie.len == 0) {
 		mslog(s, proc, LOG_INFO, "error in cookie size");
@@ -243,43 +242,25 @@ struct cookie_entry_st *old = NULL;
 		}
 	}
 
-	/* check for a valid stored cookie */
-	if ((old=find_cookie_entry(&s->cookies, req->cookie.data, req->cookie.len)) != NULL) {
-		mslog(s, proc, LOG_DEBUG, "reusing cookie for '%s' (%u)", proc->username, (unsigned)proc->pid);
-		if (old->proc != NULL) {
-			mslog(s, old->proc, LOG_DEBUG, "disconnecting (%u) due to new cookie connection",
-				(unsigned)old->proc->pid);
+	/* check for a user with the same sid as in the cookie */
+	old_proc = proc_search_sid(s, cmsg->sid.data, cmsg->sid.len);
+	if (old_proc != NULL) {
+		mslog(s, old_proc, LOG_DEBUG, "disconnecting (%u) due to new cookie session",
+			(unsigned)old_proc->pid);
 
-			if (strcmp(proc->username, old->proc->username) != 0) {
-				mslog(s, old->proc, LOG_ERR, "the user of the cookie don't match (new: %s)",
-					proc->username);
-				return -1;
-			}
-
-			/* steal its leases */
-			steal_ip_leases(old->proc, proc);
-
-			/* steal its cookie */
-			old->proc->cookie_ptr = NULL;
-
-			if (old->proc->pid > 0)
-				kill(old->proc->pid, SIGTERM);
-		} else {
-			revive_cookie(old);
+		if (strcmp(proc->username, old_proc->username) != 0) {
+			mslog(s, old_proc, LOG_ERR, "the user of the cookie doesn't match (new: %s)",
+				proc->username);
+			return -1;
 		}
-		proc->cookie_ptr = old;
-		old->proc = proc;
+
+		/* steal its leases */
+		steal_ip_leases(old_proc, proc);
+
+		if (old_proc->pid > 0)
+			kill(old_proc->pid, SIGTERM);
 	} else {
-		if (cmsg->expiration < now) {
-			mslog(s, proc, LOG_INFO, "ignoring expired cookie");
-			return -1;
-		}
-
-		mslog(s, proc, LOG_DEBUG, "new cookie for (%u)", (unsigned)proc->pid);
-
-		proc->cookie_ptr = new_cookie_entry(&s->cookies, proc, req->cookie.data, req->cookie.len);
-		if (proc->cookie_ptr == NULL)
-			return -1;
+		mslog(s, proc, LOG_DEBUG, "new cookie session for (%u)", (unsigned)proc->pid);
 	}
 
 	if (proc->config.require_cert != 0 && cmsg->tls_auth_ok == 0) {

src/main-misc.c

@@ -369,12 +369,6 @@ void remove_proc(main_server_st * s, struct proc_st *proc, unsigned k)
 	if (proc->ipv4 || proc->ipv6)
 		remove_ip_leases(s, proc);
 
-	/* expire any available cookies */
-	if (proc->cookie_ptr) {
-		proc->cookie_ptr->proc = NULL;
-		proc->cookie_ptr->expiration = time(0) + s->config->cookie_timeout;
-	}
-
 	close_tun(s, proc);
 	proc_table_del(s, proc);
 

src/main.c

@@ -606,8 +606,6 @@ void clear_lists(main_server_st *s)
 			close(ctmp->fd);
 		if (ctmp->tun_lease.fd >= 0)
 			close(ctmp->tun_lease.fd);
-		if (ctmp->cookie_ptr)
-			ctmp->cookie_ptr->proc = NULL;
 		list_del(&ctmp->list);
 		safe_memset(ctmp, 0, sizeof(*ctmp));
 		talloc_free(ctmp);
@@ -623,7 +621,6 @@ void clear_lists(main_server_st *s)
 	ip_lease_deinit(&s->ip_leases);
 	proc_table_deinit(s);
 	ctl_handler_deinit(s);
-	cookie_db_deinit(&s->cookies);
 }
 
 static void kill_children(main_server_st* s)
@@ -859,7 +856,6 @@ unsigned total = 10;
 		need_maintenance = 0;
 		mslog(s, NULL, LOG_DEBUG, "performing maintenance");
 		expire_tls_sessions(s);
-		expire_cookies(&s->cookies);
 		alarm(MAINTAINANCE_TIME(s));
 	}
 }
@@ -932,7 +928,6 @@ int main(int argc, char** argv)
 	list_head_init(&s->proc_list.head);
 	list_head_init(&s->script_list.head);
 	tls_cache_init(s, &s->tls_db);
-	cookie_db_init(s, &s->cookies);
 	ip_lease_init(&s->ip_leases);
 	proc_table_init(s);
 

src/main.h

@@ -80,19 +80,6 @@ enum {
 	PS_AUTH_COMPLETED, /* successful authentication */
 };
 
-#define COOKIE_HASH_SIZE 20
-#define COOKIE_HASH GNUTLS_DIG_SHA1
-
-typedef struct cookie_entry_st {
-	struct proc_st *proc; /* may be null, otherwise the proc that uses that cookie */
-	time_t expiration; /* -1 or the time it should expire */
-
-	/* We store the hash of the cookie that is associated with a particular session.
-	 * The reason is to avoid a memory leak to an unprivileged process to expose
-	 * data that can be used to authenticate as another user */
-	uint8_t cookie_hash[COOKIE_HASH_SIZE];
-} cookie_st;
-
 /* Each worker process maps to a unique proc_st structure.
  */
 typedef struct proc_st {
@@ -136,9 +123,6 @@ typedef struct proc_st {
 	char dtls_compr[8];
 	unsigned mtu;
 
-	/* pointer to the cookie used by this session */
-	struct cookie_entry_st *cookie_ptr;
-
 	/* if the session is initiated by a cookie the following two are set
 	 * and are considered when generating an IP address. That is used to
 	 * generate the same address as previously allocated.

src/sec-mod-auth.c

@@ -329,8 +329,10 @@ int handle_sec_auth_session_cmd(int cfd, sec_mod_st * sec, const SecAuthSessionM
 	if (cmd == SM_CMD_AUTH_SESSION_OPEN) {
 		SecAuthSessionReplyMsg rep = SEC_AUTH_SESSION_REPLY_MSG__INIT;
 
-		/* we don't check for expiration of session here. This is
-		 * done at the main server. */
+		if (e->time != -1 && time(0) > e->time + sec->config->cookie_timeout) {
+			seclog(sec, LOG_ERR, "session is expired");
+			return -1;
+		}
 
 		if (module != NULL && module->open_session != NULL) {
 			ret = module->open_session(e->auth_ctx, req->sid.data, req->sid.len);
@@ -371,6 +373,8 @@ int handle_sec_auth_session_cmd(int cfd, sec_mod_st * sec, const SecAuthSessionM
 
 		if (rep.reply != AUTH__REP__OK)
 			del_client_entry(sec, e);
+		else /* set expiration time to unlimited (until someone closes the session) */
+			e->time = -1;
 
 	} else { /* CLOSE */
 		if (req->has_uptime && req->uptime > e->stats.uptime) {