implement @wcawijngaards' review comments

services/cache/infra.c

@@ -393,8 +393,7 @@ data_entry_init(struct infra_cache* infra, struct lruhash_entry* e,
 	uint8_t cookie[8] = {0,0,0,0,0,0,0,0};
 
 	for (i = 0; i < 8; i++) {
-		cookie[i] = ub_random_max(infra->random_state,
-			255); // @TODO is 255 correct? macro-ify it?
+		cookie[i] = ub_random_max(infra->random_state, 256);
 	}
 
 	data = (struct infra_data*)e->data;
@@ -405,7 +404,7 @@ data_entry_init(struct infra_cache* infra, struct lruhash_entry* e,
 	data->probedelay = 0;
 	/* set EDNS cookie to zero, as this also sets the starting state*/
 	memset(&data->cookie, 0, sizeof(struct edns_cookie));
-	memcpy(data->cookie.data.components.client, cookie, 8);
+	memcpy(data->cookie.data.cookie, cookie, 8);
 	data->isdnsseclame = 0;
 	data->rec_lame = 0;
 	data->lame_type_A = 0;
@@ -706,10 +705,10 @@ infra_edns_update(struct infra_cache* infra, struct sockaddr_storage* addr,
 	return 1;
 }
 
-struct edns_cookie*
+int
 infra_get_cookie(struct infra_cache* infra, struct sockaddr_storage* addr,
 	socklen_t addrlen, uint8_t* name, size_t namelen,
-	time_t timenow)
+	time_t timenow, struct edns_cookie* cookie)
 {
 	struct lruhash_entry* e = infra_lookup_nottl(infra, addr, addrlen,
 		name, namelen, 1);
@@ -718,29 +717,30 @@ infra_get_cookie(struct infra_cache* infra, struct sockaddr_storage* addr,
 
 	if(!e) {
 		if(!(e = new_entry(infra, addr, addrlen, name, namelen, timenow))) {
-			return NULL;
+			return 0;
 		}
 		needtoinsert = 1;
 	} else if(((struct infra_data*)e->data)->ttl < timenow) {
 		/* EDNS cookies have their own timeout logic controlled by the
 		 * upstream, so we just copy the cookie from the old cache entry */
 		struct edns_cookie c = ((struct infra_data*)e->data)->cookie;
-		/* @TODO create logic for saving server cookie? -> not sure, find out */
 
-		/* create new cookie if the cache TTL expired */
+		/* create new cookie if the cache TTL expired, keep the cookie */
 		data_entry_init(infra, e, timenow);
 		((struct infra_data*)e->data)->cookie = c;
 	}
 
 	data = (struct infra_data*) e->data;
 
+	memcpy(cookie, &data->cookie, sizeof(struct edns_cookie));
+
 	if(needtoinsert) {
 		slabhash_insert(infra->hosts, e->hash, e, e->data, NULL);
 	} else {
 		lock_rw_unlock(&e->lock);
 	}
 
-	return &data->cookie;
+	return 1;
 }
 
 int
@@ -755,8 +755,13 @@ infra_set_server_cookie(struct infra_cache* infra, struct sockaddr_storage* addr
 	assert(cookie->opt_len == 24);
 
 	/* the client cookie was set on the outgoing upstream, so the entry
-	 * must exists here */
-	assert(e);
+	 * should exists here. This can be false if the cookie has fallen
+	 * out of cache */
+	if (!(e)) {
+		/* No need to insert a new cookie/entry here, this will be
+		 * done with an outgoing request */
+		return 0;
+	}
 
 	data = (struct infra_data*) e->data;
 
@@ -764,25 +769,25 @@ infra_set_server_cookie(struct infra_cache* infra, struct sockaddr_storage* addr
 		/* we known this upstream doesn't support cookies; the state
 		 * remains unchanged */
 		lock_rw_unlock(&e->lock);
-		return 0;
+		return 1;
 	} else if (data->cookie.state == SERVER_COOKIE_LEARNED) {
 		/* wrong client cookie; don't store the server cookie */
-		if (!(memcmp(data->cookie.data.components.client,
+		if (!(memcmp(data->cookie.data.cookie,
 			cookie->opt_data+4, 8))) {
 			/* the state of the cookie remains unchanged as we will
 			 * drop this upstream response */
 
-			verbose(VERB_ALGO, "wrong client cookie from upstream with "
-				"previously seen cookie");
+			verbose(VERB_ALGO, "wrong client cookie from upstream"
+				" with previously seen cookie");
 			lock_rw_unlock(&e->lock);
 			return -1;
 		}
 
 		/* the server cookie has changed, but the client cookie has not
 		 * so we update the server cookie */
-		if (memcmp(data->cookie.data.complete+8,
+		if (memcmp(data->cookie.data.cookie+8,
 				cookie->opt_data+12, 16) != 0) {
-			memcpy(data->cookie.data.complete, cookie->opt_data, 24);
+			memcpy(data->cookie.data.cookie, cookie->opt_data, 24);
 			/* the cookie state remains unchanged*/
 
 			verbose(VERB_ALGO, "update new server cookie from upstream");
@@ -792,13 +797,14 @@ infra_set_server_cookie(struct infra_cache* infra, struct sockaddr_storage* addr
 
 		/* both the complete cookies are identical, so the state
 		 * remains unchanged */
-		verbose(VERB_ALGO, "correctly received indentical cookie from upstream; don't update");
+		verbose(VERB_ALGO, "correctly received indentical cookie from"
+			" upstream; don't update");
 		lock_rw_unlock(&e->lock);
-		return 0;
+		return 1;
 	} else { /* cookie state == SERVER_COOKIE_UNKNOWN */
 
 		/* wrong client cookie; don't store the server cookie */
-		if (!(memcmp(data->cookie.data.components.client,
+		if (!(memcmp(data->cookie.data.cookie,
 			cookie->opt_data+4, 8))) {
 			/* the state of the cookie remains unchanged as we will
 			 * drop this upstream response */
@@ -808,8 +814,8 @@ infra_set_server_cookie(struct infra_cache* infra, struct sockaddr_storage* addr
 			return -1;
 		}
 
-		/* else; store the client cookie */
-		memcpy(data->cookie.data.complete, cookie->opt_data, 24);
+		/* store the server cookie */
+		memcpy(data->cookie.data.cookie, cookie->opt_data, 24);
 		data->cookie.state = SERVER_COOKIE_LEARNED;
 
 		verbose(VERB_QUERY, "storing received server cookie from upstream");

services/cache/infra.h

@@ -55,17 +55,21 @@ struct config_file;
 /* COOKIE @TODO move this to correct spot */
 
 /**
- * The actual EDNS cookie data
+ * The actual EDNS cookie data. Note that the cookie can be filled with the 
+ * just 'client' section, or with the 'complete' cookie depending on the state
+ * governed by the edns_cookie_state.
+ * The commented struct provides insight on how the bytes in the struct are
+ * structured.
  */
-union edns_cookie_data {
-        uint8_t complete[24];
-        struct {
+struct edns_cookie_data {
+        uint8_t cookie[24];
+        /* struct {
                 uint8_t client[8];
                 uint8_t version;
                 uint8_t reserved[3];
                 uint32_t timestamp;
                 uint8_t hash[8];
-        } components;
+        } components; */
 };
 
 /**
@@ -83,7 +87,7 @@ enum edns_cookie_state
  */
 struct edns_cookie {
         enum edns_cookie_state state;
-        union edns_cookie_data data;
+        struct edns_cookie_data data;
 };
 
 
@@ -368,18 +372,18 @@ int infra_edns_update(struct infra_cache* infra,
  * @param name: name of zone
  * @param namelen: length of name
  * @param timenow: what time it is now.
- * @return: struct ends_cookie*
+ * @param cookie: the cookie that is retrieved from cache on success.
+ * @return: 0 on error, cookie pointer remains unchanged then.
  */
-struct edns_cookie*
-infra_get_cookie(struct infra_cache* infra, struct sockaddr_storage* addr,
+int infra_get_cookie(struct infra_cache* infra, struct sockaddr_storage* addr,
         socklen_t addrlen, uint8_t* name, size_t namelen,
-        time_t timenow);
+        time_t timenow, struct edns_cookie* cookie);
 
 /**
- * Find the cookie entry in the cache and update it with to make a complete
- * cookie (RFC9018). This function asserts that the cookie param contains a
- * complete cookie with a length of 24 bytes. It also asserts that a previous
- * entry in the cache already exists, as it will update the entry.
+ * Find the cookie entry in the cache and update it with to make a 'complete cookie'
+ * (client+server) (RFC9018). This function asserts that the cookie param contains
+ * a complete cookie with a length of 24 bytes. If the cache entry isn't found
+ * a new one will be inserted.
  * @param infra: infrastructure cache.
  * @param addr: host address.
  * @param addrlen: length of addr.
@@ -387,7 +391,9 @@ infra_get_cookie(struct infra_cache* infra, struct sockaddr_storage* addr,
  * @param namelen: length of name
  * @param timenow: what time it is now.
  * @param cookie: the EDNS cookie option we want to store.
- * @return true if the complete cookie is stored, and false if it is not
+ * @return -1 if the wrong client cookie is found, 0 if the entry isn't found in
+ *      the cache and a new one is inserted, 1 if the complete cookie is inserted
+ *      or unchanged.
  */
 int infra_set_server_cookie(struct infra_cache* infra, struct sockaddr_storage* addr,
         socklen_t addrlen, uint8_t* name, size_t namelen, struct edns_option* cookie);

services/outside_network.c

@@ -3349,7 +3349,7 @@ outnet_serviced_query(struct outside_network* outnet,
 	struct service_callback* cb;
 	struct edns_string_addr* client_string_addr;
 	struct regional* region;
-	struct edns_cookie* cookie;
+	struct edns_cookie cookie;
 	struct edns_option* backed_up_opt_list = qstate->edns_opts_back_out;
 	struct edns_option* per_upstream_opt_list = NULL;
 	time_t timenow = 0;
@@ -3389,18 +3389,18 @@ outnet_serviced_query(struct outside_network* outnet,
 			client_string_addr->string, region);
 	}
 
-	if (env->cfg->upstream_cookies) {
-		cookie = infra_get_cookie(env->infra_cache, addr, addrlen, zone,
-			zonelen, *env->now);
+	if (env->cfg->upstream_cookies &&
+		infra_get_cookie(env->infra_cache, addr, addrlen, zone, zonelen,
+			*env->now, &cookie)) {
 
-		if (cookie->state == SERVER_COOKIE_LEARNED) {
+		if (cookie.state == SERVER_COOKIE_LEARNED) {
 			/* We known the complete cookie, so we attach it */
 			edns_opt_list_append(&per_upstream_opt_list, LDNS_EDNS_COOKIE,
-				24, cookie->data.complete, region);
-		} else if (cookie->state == SERVER_COOKIE_UNKNOWN) {
+				24, cookie.data.cookie, region);
+		} else if (cookie.state == SERVER_COOKIE_UNKNOWN) {
 			/* We know just client cookie, so we attach it */
 			edns_opt_list_append(&per_upstream_opt_list, LDNS_EDNS_COOKIE,
-				8, cookie->data.components.client, region);
+				8, cookie.data.cookie, region);
 		} /* We ignore COOKIE_NOT_SUPPORTED */
 	}
 

testcode/fake_event.c

@@ -1226,7 +1226,7 @@ struct serviced_query* outnet_serviced_query(struct outside_network* outnet,
 		struct edns_option* backed_up_opt_list =
 			qstate->edns_opts_back_out;
 		struct edns_option* per_upstream_opt_list = NULL;
-		struct edns_cookie* cookie;
+		struct edns_cookie cookie;
 		/* If we have an already populated EDNS option list make a copy
 		 * since we may now add upstream specific EDNS options. */
 		if(qstate->edns_opts_back_out) {
@@ -1256,20 +1256,20 @@ struct serviced_query* outnet_serviced_query(struct outside_network* outnet,
 				client_string_addr->string, qstate->region);
 		}
 
-		if (qstate->env->cfg->upstream_cookies) {
-			cookie = infra_get_cookie(env->infra_cache, addr, addrlen,
-				zone, zonelen, *env->now);
+		if (qstate->env->cfg->upstream_cookies &&
+			infra_get_cookie(env->infra_cache, addr, addrlen,
+				zone, zonelen, *env->now, &cookie)) {
 
-			if (cookie->state == SERVER_COOKIE_LEARNED) {
+			if (cookie.state == SERVER_COOKIE_LEARNED) {
 				/* We known the complete cookie, so we attach it */
 				edns_opt_list_append(&per_upstream_opt_list,
-					LDNS_EDNS_COOKIE, 24, cookie->data.complete,
+					LDNS_EDNS_COOKIE, 24, cookie.data.cookie,
 					qstate->region);
-			} else if (cookie->state == SERVER_COOKIE_UNKNOWN) {
+			} else if (cookie.state == SERVER_COOKIE_UNKNOWN) {
 				/* We know just client cookie, so we attach it */
 				edns_opt_list_append(&per_upstream_opt_list,
 					LDNS_EDNS_COOKIE, 8,
-					cookie->data.components.client,
+					cookie.data.cookie,
 					qstate->region);
 			} /* We ignore COOKIE_NOT_SUPPORTED */
 		}

testcode/testpkts.c

@@ -1529,8 +1529,14 @@ match_random_client_cookie(uint8_t* query, size_t query_len)
 	}
 
 	/* class + ttl + rdlen = 8 */
-	if(walk_query_len <= 8) {
-		verbose(3, "No edns opt, no cookie");
+	if (walk_query_len <= 8) {
+		verbose(3, "No correct EDNS record found, so no cookie");
+		return 0;
+	}
+
+	/* class + ttl + rdlen + opt_code + opt_len = 12 */
+	if (walk_query_len < 12) {
+		verbose(3, "No EDNS opt found, so no cookie");
 		return 0;
 	}
 
@@ -1561,13 +1567,19 @@ match_random_complete_cookie(uint8_t* query, size_t query_len, struct entry* p)
 
 	/* class + ttl + rdlen = 8 */
 	if(walk_query_len <= 8) {
-		verbose(3, "No edns opt, no cookie");
+		verbose(3, "No correct EDNS record , so no cookie");
 		return 0;
 	}
 
 	walk_query += 8;
 	walk_query_len -= 8;
 
+	/* opt_code + opt_len = 4 */
+	if (walk_query_len < 4) {
+		verbose(3, "No EDNS opt found, so no cookie");
+		return 0;
+	}
+
 	if (sldns_read_uint16(walk_query) != 10 /* LDNS_EDNS_COOKIE */) {
 		verbose(3, "EDNS option is not a cookie");
 		return 0;
@@ -1577,6 +1589,12 @@ match_random_complete_cookie(uint8_t* query, size_t query_len, struct entry* p)
 		return 0;
 	}
 
+	/* opt_code + opt_len + cookie_data = 28 */
+	if (walk_query_len < 28) {
+		verbose(3, "No complete cookie found in the packet");
+		return 0;
+	}
+
 	if (p->match_random_complete_cookie_renewed) {
 		uint8_t renewed_cookie[16];
 
@@ -1881,18 +1899,16 @@ adjust_packet(struct entry* match, uint8_t** answer_pkt, size_t *answer_len,
 			if (sldns_read_uint16(walk_query+2) == 8) {
 				/* copy the EDNS client cookie from the query
 				 * packet to the response */
-				memcpy(walk_response, walk_query, 12);
+				memmove(walk_response, walk_query, 12);
 
 				/* add the server cookie to the client cookie to make it
 				 * 'complete'. we fake the siphash specified in RFC9018
 				 * by hardcoding the server cookie */
-				memcpy(walk_response+12, hardcoded_server_cookie, 16);
+				memmove(walk_response+12, hardcoded_server_cookie, 16);
 
 				/* update the RDLEN and OPTLEN */
 				sldns_write_uint16(rdlen_ptr_response, 28);
 				sldns_write_uint16(walk_response+2, 24);
-
-				reslen = origlen + 28;
 			} else if (sldns_read_uint16(walk_query+2) == 24) {
 				/* update the RDLEN */
 				sldns_write_uint16(rdlen_ptr_response, 28);
@@ -1903,11 +1919,11 @@ adjust_packet(struct entry* match, uint8_t** answer_pkt, size_t *answer_len,
 				if (match->server_cookie_renew) {
 					/* copy the cookie from the response but add a
 					 * different cookie (by reshuffeling server cookie) */
-					memcpy(walk_response, walk_query, 12);
-					memcpy(walk_response+12, walk_query+12+8, 8);
-					memcpy(walk_response+12+8, walk_query+12, 8);
+					memmove(walk_response, walk_query, 12);
+					memmove(walk_response+12, walk_query+12+8, 8);
+					memmove(walk_response+12+8, walk_query+12, 8);
 				} else {
-					memcpy(walk_response, walk_query, 28);
+					memmove(walk_response, walk_query, 28);
 				}
 			} else {
 				log_err("testbound: the incoming EDNS cookie has the wrong length");

testcode/unitmain.c

@@ -529,6 +529,7 @@ infra_test(void)
 
 	infra_delete(slab);
 	config_delete(cfg);
+	ub_randfree(rnd);
 }
 
 #include "util/random.h"