hs-v3: Improve accessor semantic of client cached object (18107717) · Commits · The Tor Project / Core / Tor

src/feature/hs/hs_cache.c

+52 −12

Original line number	Diff line number	Diff line
		@@ -27,6 +27,21 @@
		static int cached_client_descriptor_has_expired(time_t now,
		const hs_cache_client_descriptor_t *cached_desc);

		/** Helper function: Return true iff the cache entry has a decrypted
		* descriptor.
		*
		* A NULL desc object in the entry means that we were not able to decrypt the
		* descriptor because we are likely lacking client authorization. It is still
		* a valid entry but some operations can't be done without the decrypted
		* descriptor thus this function MUST be used to safe guard access to the
		* decrypted desc object. */
		static inline bool
		entry_has_decrypted_descriptor(const hs_cache_client_descriptor_t *entry)
		{
		tor_assert(entry);
		return (entry->desc != NULL);
		}

		/******************** Directory HS cache ****************/

		/** Directory descriptor cache. Map indexed by blinded key. */
		@@ -341,8 +356,23 @@ static digest256map_t *hs_cache_client_intro_state;
		static size_t
		cache_get_client_entry_size(const hs_cache_client_descriptor_t *entry)
		{
		return sizeof(*entry) +
		strlen(entry->encoded_desc) + hs_desc_obj_size(entry->desc);
		size_t size = 0;

		if (entry == NULL) {
		goto end;
		}
		size += sizeof(*entry);

		if (entry->encoded_desc) {
		size += strlen(entry->encoded_desc);
		}

		if (entry_has_decrypted_descriptor(entry)) {
		size += hs_desc_obj_size(entry->desc);
		}

		end:
		return size;
		}

		/** Remove a given descriptor from our cache. */
		@@ -659,14 +689,20 @@ cache_store_as_client(hs_cache_client_descriptor_t *client_desc)
		* client authorization. */
		cache_entry = lookup_v3_desc_as_client(client_desc->key.pubkey);
		if (cache_entry != NULL) {
		/* Signalling an undecrypted descriptor. We'll always replace the one we
		* have with the new one just fetched. */
		if (cache_entry->desc == NULL) {
		/* If the current or the new cache entry don't have a decrypted descriptor
		* (missing client authorization), we always replace the current one with
		* the new one. Reason is that we can't inspect the revision counter
		* within the plaintext data so we blindly replace. */
		if (!entry_has_decrypted_descriptor(cache_entry) \|\|
		!entry_has_decrypted_descriptor(client_desc)) {
		remove_v3_desc_as_client(cache_entry);
		cache_client_desc_free(cache_entry);
		goto store;
		}

		/* From this point on, we know that the decrypted descriptor is in the
		* current entry and new object thus safe to access. */

		/* If we have an entry in our cache that has a revision counter greater
		* than the one we just fetched, discard the one we fetched. */
		if (cache_entry->desc->plaintext_data.revision_counter >
		@@ -740,11 +776,15 @@ cache_clean_v3_as_client(time_t now)
		MAP_DEL_CURRENT(key);
		entry_size = cache_get_client_entry_size(entry);
		bytes_removed += entry_size;

		/* We just removed an old descriptor. We need to close all intro circuits
		* so we don't have leftovers that can be selected while lacking a
		* descriptor. We leave the rendezvous circuits opened because they could
		* be in use. */
		* if the descriptor is decrypted so we don't have leftovers that can be
		* selected while lacking a descriptor. Circuits are selected by intro
		* authentication key thus we need the descriptor. We leave the rendezvous
		* circuits opened because they could be in use. */
		if (entry_has_decrypted_descriptor(entry)) {
		hs_client_close_intro_circuits_from_desc(entry->desc);
		}
		/* Entry is not in the cache anymore, destroy it. */
		cache_client_desc_free(entry);
		/* Update our OOM. We didn't use the remove() function because we are in
		@@ -793,7 +833,7 @@ hs_cache_lookup_as_client(const ed25519_public_key_t *key)
		tor_assert(key);

		cached_desc = lookup_v3_desc_as_client(key->pubkey);
		if (cached_desc && cached_desc->desc) {
		if (cached_desc && entry_has_decrypted_descriptor(cached_desc)) {
		return cached_desc->desc;
		}

		@@ -866,7 +906,7 @@ hs_cache_remove_as_client(const ed25519_public_key_t *key)
		/* If we have a decrypted/decoded descriptor, attempt to close its
		* introduction circuit(s). We shouldn't have circuit(s) without a
		* descriptor else it will lead to a failure. */
		if (cached_desc->desc) {
		if (entry_has_decrypted_descriptor(cached_desc)) {
		hs_client_close_intro_circuits_from_desc(cached_desc->desc);
		}
		/* Remove and free. */
		@@ -995,7 +1035,7 @@ hs_cache_client_new_auth_parse(const ed25519_public_key_t *service_pk)
		}

		cached_desc = lookup_v3_desc_as_client(service_pk->pubkey);
		if (cached_desc == NULL \|\| cached_desc->desc != NULL) {
		if (cached_desc == NULL \|\| entry_has_decrypted_descriptor(cached_desc)) {
		/* No entry for that service or the descriptor is already decoded. */
		goto end;
		}

src/test/test_hs_client.c

+46 −0

Original line number	Diff line number	Diff line
		@@ -965,6 +965,7 @@ test_close_intro_circuits_new_desc(void *arg)
		(void) arg;

		hs_init();
		rend_cache_init();

		/* This is needed because of the client cache expiration timestamp is based
		* on having a consensus. See cached_client_descriptor_has_expired(). */
		@@ -989,6 +990,51 @@ test_close_intro_circuits_new_desc(void *arg)
		circ->purpose = CIRCUIT_PURPOSE_C_INTRODUCING;
		ocirc = TO_ORIGIN_CIRCUIT(circ);

		/* Build a descriptor _without_ client authorization and thus not
		* decryptable. Make sure the close circuit code path is not triggered. */
		{
		char *desc_encoded = NULL;
		uint8_t descriptor_cookie[HS_DESC_DESCRIPTOR_COOKIE_LEN];
		curve25519_keypair_t client_kp;
		hs_descriptor_t *desc = NULL;

		tt_int_op(0, OP_EQ, curve25519_keypair_generate(&client_kp, 0));
		crypto_rand((char *) descriptor_cookie, sizeof(descriptor_cookie));

		desc = hs_helper_build_hs_desc_with_client_auth(descriptor_cookie,
		&client_kp.pubkey,
		&service_kp);
		tt_assert(desc);
		ret = hs_desc_encode_descriptor(desc, &service_kp, descriptor_cookie,
		&desc_encoded);
		tt_int_op(ret, OP_EQ, 0);
		/* Associate descriptor intro key with the dummy circuit. */
		const hs_desc_intro_point_t *ip =
		smartlist_get(desc->encrypted_data.intro_points, 0);
		tt_assert(ip);
		ocirc->hs_ident = hs_ident_circuit_new(&service_kp.pubkey);
		ed25519_pubkey_copy(&ocirc->hs_ident->intro_auth_pk,
		&ip->auth_key_cert->signed_key);
		hs_descriptor_free(desc);
		tt_assert(desc_encoded);
		/* Put it in the cache. Should not be decrypted since the client
		* authorization creds were not added to the global map. */
		ret = hs_cache_store_as_client(desc_encoded, &service_kp.pubkey);
		tor_free(desc_encoded);
		tt_int_op(ret, OP_EQ, HS_DESC_DECODE_NEED_CLIENT_AUTH);

		/* Clean cache with a future timestamp. It will trigger the clean up and
		* attempt to close the circuit but only if the descriptor is decryptable.
		* Cache object should be removed and circuit untouched. */
		hs_cache_clean_as_client(mock_ns.valid_after + (60 * 60 * 24));
		tt_assert(!hs_cache_lookup_as_client(&service_kp.pubkey));

		/* Make sure the circuit still there. */
		tt_assert(circuit_get_next_intro_circ(NULL, true));
		/* Get rid of the ident, it will be replaced in the next tests. */
		hs_ident_circuit_free(ocirc->hs_ident);
		}

		/* Build the first descriptor and cache it. */
		{
		char *encoded;