From: Alberto Leiva Popper <ydahhrk@gmail.com>
Date: Tue, 5 Dec 2023 13:01:33 +0000 (-0300)
Subject: Stop trying to cage non-notification URLs during cleanup
X-Git-Tag: 1.6.1~7
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=96fba7ec237f4b9a16b3676b749a3826c43dfef2;p=thirdparty%2FFORT-validator.git

Stop trying to cage non-notification URLs during cleanup

Fixes the new issue reported in #103.
---

diff --git a/src/abbreviations.txt b/src/abbreviations.txt
index bbb5b147..5024c4d8 100644
--- a/src/abbreviations.txt
+++ b/src/abbreviations.txt
@@ -21,7 +21,7 @@ len: length
 max: maximum
 min: minimum
 msg: message
-notif: Notification
+notif: (RRDP) Notification
 pdu: Protocol Data Unit (RFC 6810)
 pp: Publication Point
 pr: print
diff --git a/src/cache/local_cache.c b/src/cache/local_cache.c
index 436f73c1..6cdb6e6b 100644
--- a/src/cache/local_cache.c
+++ b/src/cache/local_cache.c
@@ -44,6 +44,7 @@ struct rpki_cache {
 #define TAGNAME_ATTEMPT_TS "attempt-timestamp"
 #define TAGNAME_ATTEMPT_ERR "attempt-result"
 #define TAGNAME_SUCCESS_TS "success-timestamp"
+#define TAGNAME_IS_NOTIF "is-rrdp-notification"
 
 static char *
 get_json_filename(struct rpki_cache *cache)
@@ -58,6 +59,7 @@ json2node(struct rpki_cache *cache, json_t *json)
 {
 	struct cache_node *node;
 	char const *url;
+	bool is_notif;
 	enum uri_type type;
 	int error;
 
@@ -78,7 +80,16 @@ json2node(struct rpki_cache *cache, json_t *json)
 		pr_op_err("Unknown protocol: %s", url);
 		goto fail;
 	}
-	error = uri_create(&node->url, cache->tal, type, NULL, url);
+
+	error = json_get_bool(json, TAGNAME_IS_NOTIF, &is_notif);
+	if (error < 0)
+		goto fail;
+
+	error = uri_create(&node->url, cache->tal, type, is_notif, NULL, url);
+	if (error) {
+		pr_op_err("Cannot parse '%s' into a URI.", url);
+		goto fail;
+	}
 
 	error = json_get_ts(json, TAGNAME_ATTEMPT_TS, &node->attempt.ts);
 	if (error) {
@@ -191,6 +202,9 @@ node2json(struct cache_node *node)
 
 	if (json_add_str(json, TAGNAME_URL, uri_get_global(node->url)))
 		goto cancel;
+	if (uri_is_notif(node->url))
+		if (json_add_bool(json, TAGNAME_IS_NOTIF, true))
+			goto cancel;
 	if (json_add_date(json, TAGNAME_ATTEMPT_TS, node->attempt.ts))
 		goto cancel;
 	if (json_add_int(json, TAGNAME_ATTEMPT_ERR, node->attempt.result))
@@ -315,15 +329,15 @@ get_url(struct rpki_uri *uri, const char *tal, struct rpki_uri **url)
 		if (*c == '/') {
 			slashes++;
 			if (slashes == 4)
-				return __uri_create(url, tal, UT_RSYNC, NULL,
-				    guri, c - guri + 1);
+				return __uri_create(url, tal, UT_RSYNC, false,
+				    NULL, guri, c - guri + 1);
 		}
 	}
 
 	if (slashes == 3 && *(c - 1) != '/') {
 		guri2 = pstrdup(guri); /* Remove const */
 		guri2[c - guri] = '/';
-		error = __uri_create(url, tal, UT_RSYNC, NULL, guri2,
+		error = __uri_create(url, tal, UT_RSYNC, false, NULL, guri2,
 		    c - guri + 1);
 		free(guri2);
 		return error;
@@ -604,12 +618,9 @@ static void
 delete_node_and_cage(struct rpki_cache *cache, struct cache_node *node)
 {
 	struct rpki_uri *cage;
-	int error;
 
-	if (uri_get_type(node->url) == UT_HTTPS) {
-		error = __uri_create(&cage, cache->tal, UT_CAGED, node->url,
-		    "", 0);
-		if (!error) {
+	if (uri_is_notif(node->url)) {
+		if (uri_create_cage(&cage, cache->tal, node->url) == 0) {
 			pr_op_debug("Deleting cage %s.", uri_get_local(cage));
 			file_rm_rf(uri_get_local(cage));
 			uri_refput(cage);
@@ -656,6 +667,7 @@ cleanup_node(struct rpki_cache *cache, struct cache_node *node,
 		break;
 	case ENOENT:
 		/* Node exists but file doesn't: Delete node */
+		pr_op_debug("Node exists but file doesn't: %s", path);
 		delete_node_and_cage(cache, node);
 		return;
 	default:
@@ -739,9 +751,10 @@ delete_unknown_files(struct rpki_cache *cache)
 		uri_refget(node->url);
 		uris_add(&dnc, node->url);
 
-		error = __uri_create(&cage, cache->tal, UT_CAGED, node->url,
-		    "", 0);
-		if (error) {
+		if (!uri_is_notif(node->url))
+			continue;
+
+		if (uri_create_cage(&cage, cache->tal, node->url) != 0) {
 			pr_op_err("Cannot generate %s's cage. I'm probably going to end up deleting it from the cache.",
 			    uri_op_get_printable(node->url));
 			continue;
@@ -771,10 +784,12 @@ cache_cleanup(struct rpki_cache *cache)
 	struct cache_node *node, *tmp;
 	time_t last_week;
 
+	pr_op_debug("Cleaning up old abandoned cache files.");
 	last_week = get_days_ago(7);
 	HASH_ITER(hh, cache->ht, node, tmp)
 		cleanup_node(cache, node, last_week);
 
+	pr_op_debug("Cleaning up unknown cache files.");
 	delete_unknown_files(cache);
 }
 
diff --git a/src/object/certificate.c b/src/object/certificate.c
index a5cc53b3..0724cae6 100644
--- a/src/object/certificate.c
+++ b/src/object/certificate.c
@@ -1481,12 +1481,13 @@ end:
  * Create @uri from the @ad
  */
 static int
-uri_create_ad(struct rpki_uri **uri, ACCESS_DESCRIPTION *ad, enum uri_type type)
+uri_create_ad(struct rpki_uri **uri, ACCESS_DESCRIPTION *ad, enum uri_type type,
+    bool is_notif)
 {
-	ASN1_STRING *asn1_string;
+	ASN1_STRING *asn1str;
 	int ptype;
 
-	asn1_string = GENERAL_NAME_get0_value(ad->location, &ptype);
+	asn1str = GENERAL_NAME_get0_value(ad->location, &ptype);
 
 	/*
 	 * RFC 6487: "This extension MUST have an instance of an
@@ -1524,9 +1525,9 @@ uri_create_ad(struct rpki_uri **uri, ACCESS_DESCRIPTION *ad, enum uri_type type)
 	 * But ask the testers to keep an eye on it anyway.
 	 */
 	return __uri_create(uri,
-	    tal_get_file_name(validation_tal(state_retrieve())), type, NULL,
-	    ASN1_STRING_get0_data(asn1_string),
-	    ASN1_STRING_length(asn1_string));
+	    tal_get_file_name(validation_tal(state_retrieve())), type,
+	    is_notif, NULL,
+	    ASN1_STRING_get0_data(asn1str), ASN1_STRING_length(asn1str));
 }
 
 /**
@@ -1560,7 +1561,8 @@ handle_ad(int nid, struct ad_metadata const *meta, SIGNATURE_INFO_ACCESS *ia,
 	for (i = 0; i < sk_ACCESS_DESCRIPTION_num(ia); i++) {
 		ad = sk_ACCESS_DESCRIPTION_value(ia, i);
 		if (OBJ_obj2nid(ad->method) == nid) {
-			error = uri_create_ad(&uri, ad, meta->type);
+			error = uri_create_ad(&uri, ad, meta->type,
+			    meta == &RPKI_NOTIFY);
 			switch (error) {
 			case 0:
 				break;
diff --git a/src/object/manifest.c b/src/object/manifest.c
index f1a81ac8..3d1739d8 100644
--- a/src/object/manifest.c
+++ b/src/object/manifest.c
@@ -23,10 +23,9 @@ cage(struct rpki_uri **uri, struct rpki_uri *notif)
 		return 0;
 	}
 
-	return __uri_create(uri,
-	    tal_get_file_name(validation_tal(state_retrieve())),
-	    UT_CAGED, notif, uri_get_global(*uri),
-	    uri_get_global_len(*uri));
+	return uri_create_caged(uri,
+	    tal_get_file_name(validation_tal(state_retrieve())), notif,
+	    uri_get_global(*uri), uri_get_global_len(*uri));
 }
 
 static int
diff --git a/src/object/tal.c b/src/object/tal.c
index fa8e6341..2d0fe8d2 100644
--- a/src/object/tal.c
+++ b/src/object/tal.c
@@ -55,9 +55,9 @@ add_uri(struct uri_list *uris, char const *tal, char *uri)
 	int error;
 
 	if (str_starts_with(uri, "rsync://"))
-		error = uri_create(&new, tal, UT_RSYNC, NULL, uri);
+		error = uri_create(&new, tal, UT_RSYNC, false, NULL, uri);
 	else if (str_starts_with(uri, "https://"))
-		error = uri_create(&new, tal, UT_HTTPS, NULL, uri);
+		error = uri_create(&new, tal, UT_HTTPS, false, NULL, uri);
 	else
 		return pr_op_err("TAL has non-RSYNC/HTTPS URI: %s", uri);
 	if (error)
diff --git a/src/rrdp.c b/src/rrdp.c
index 2c09fef8..7a522e9b 100644
--- a/src/rrdp.c
+++ b/src/rrdp.c
@@ -479,7 +479,7 @@ parse_file_metadata(xmlTextReaderPtr reader, struct rpki_uri *notif,
 	error = uri_create(&meta->uri,
 	    tal_get_file_name(validation_tal(state_retrieve())),
 	    (notif != NULL) ? UT_CAGED : UT_HTTPS,
-	    notif, (char const *)uri);
+	    false, notif, (char const *)uri);
 	xmlFree(uri);
 	if (error)
 		return error;
diff --git a/src/types/uri.c b/src/types/uri.c
index bc691659..cc7fb74e 100644
--- a/src/types/uri.c
+++ b/src/types/uri.c
@@ -1,6 +1,5 @@
 #include "types/uri.h"
 
-
 #include "alloc.h"
 #include "common.h"
 #include "config.h"
@@ -64,6 +63,7 @@ struct rpki_uri {
 	/* "local_len" is never needed right now. */
 
 	enum uri_type type;
+	bool is_notif; /* Does it point to an RRDP Notification? */
 
 	unsigned int references;
 };
@@ -393,7 +393,7 @@ autocomplete_local(struct rpki_uri *uri, char const *tal,
  */
 int
 __uri_create(struct rpki_uri **result, char const *tal, enum uri_type type,
-    struct rpki_uri *notif, void const *guri, size_t guri_len)
+    bool is_notif, struct rpki_uri *notif, void const *guri, size_t guri_len)
 {
 	struct rpki_uri *uri;
 	int error;
@@ -407,6 +407,7 @@ __uri_create(struct rpki_uri **result, char const *tal, enum uri_type type,
 	}
 
 	uri->type = type;
+	uri->is_notif = is_notif;
 
 	error = autocomplete_local(uri, tal, notif);
 	if (error) {
@@ -421,13 +422,6 @@ __uri_create(struct rpki_uri **result, char const *tal, enum uri_type type,
 	return 0;
 }
 
-int
-uri_create(struct rpki_uri **result, char const *tal, enum uri_type type,
-    struct rpki_uri *notif, char const *guri)
-{
-	return __uri_create(result, tal, type, notif, guri, strlen(guri));
-}
-
 /*
  * Manifest fileList entries are a little special in that they're just file
  * names. This function will infer the rest of the URL.
@@ -541,6 +535,12 @@ uri_is_certificate(struct rpki_uri *uri)
 	return uri_has_extension(uri, ".cer");
 }
 
+bool
+uri_is_notif(struct rpki_uri *uri)
+{
+	return uri->is_notif;
+}
+
 enum uri_type
 uri_get_type(struct rpki_uri *uri)
 {
diff --git a/src/types/uri.h b/src/types/uri.h
index d4d8188c..4730ec48 100644
--- a/src/types/uri.h
+++ b/src/types/uri.h
@@ -1,6 +1,7 @@
 #ifndef SRC_TYPES_URI_H_
 #define SRC_TYPES_URI_H_
 
+#include <stdbool.h>
 #include "asn1/asn1c/IA5String.h"
 #include "data_structure/array_list.h"
 
@@ -19,13 +20,18 @@ enum uri_type {
 struct rpki_uri;
 
 int __uri_create(struct rpki_uri **, char const *, enum uri_type,
-    struct rpki_uri *, void const *, size_t);
-int uri_create(struct rpki_uri **, char const *, enum uri_type,
-    struct rpki_uri *, char const *);
+    bool, struct rpki_uri *, void const *, size_t);
 int uri_create_mft(struct rpki_uri **, char const *, struct rpki_uri *,
     struct rpki_uri *, IA5String_t *);
 struct rpki_uri *uri_create_cache(char const *);
 
+#define uri_create(uri, tal, type, is_notif, notif, guri) \
+	__uri_create(uri, tal, type, is_notif, notif, guri, strlen(guri))
+#define uri_create_caged(uri, tal, notif, guri, guri_len) \
+	__uri_create(uri, tal, UT_CAGED, false, notif, guri, guri_len)
+#define uri_create_cage(uri, tal, notif) \
+	uri_create_caged(uri, tal, notif, "", 0)
+
 struct rpki_uri *uri_refget(struct rpki_uri *);
 void uri_refput(struct rpki_uri *);
 
@@ -40,6 +46,7 @@ size_t uri_get_global_len(struct rpki_uri *);
 bool uri_equals(struct rpki_uri *, struct rpki_uri *);
 bool uri_has_extension(struct rpki_uri *, char const *);
 bool uri_is_certificate(struct rpki_uri *);
+bool uri_is_notif(struct rpki_uri *);
 
 enum uri_type uri_get_type(struct rpki_uri *);
 bool uri_is_rsync(struct rpki_uri *);
diff --git a/test/cache/local_cache_test.c b/test/cache/local_cache_test.c
index 1ac44079..0491c7d4 100644
--- a/test/cache/local_cache_test.c
+++ b/test/cache/local_cache_test.c
@@ -124,7 +124,7 @@ run_cache_download(char const *url, int expected_error,
 	rsync_counter = 0;
 	https_counter = 0;
 
-	ck_assert_int_eq(0, uri_create(&uri, TAL_FILE, type, NULL, url));
+	ck_assert_int_eq(0, uri_create(&uri, TAL_FILE, type, false, NULL, url));
 	ck_assert_int_eq(expected_error, cache_download(cache, uri, NULL));
 	ck_assert_uint_eq(rsync_calls, rsync_counter);
 	ck_assert_uint_eq(https_calls, https_counter);
@@ -133,7 +133,8 @@ run_cache_download(char const *url, int expected_error,
 }
 
 static struct cache_node *
-node(char const *url, time_t attempt, int err, bool succeeded, time_t success)
+node(char const *url, time_t attempt, int err, bool succeeded, time_t success,
+    bool is_notif)
 {
 	enum uri_type type;
 	struct cache_node *result;
@@ -146,7 +147,8 @@ node(char const *url, time_t attempt, int err, bool succeeded, time_t success)
 		ck_abort_msg("Bad protocol: %s", url);
 
 	result = pzalloc(sizeof(struct cache_node));
-	ck_assert_int_eq(0, uri_create(&result->url, TAL_FILE, type, NULL, url));
+	ck_assert_int_eq(0, uri_create(&result->url, TAL_FILE, type, is_notif,
+	    NULL, url));
 	result->attempt.ts = attempt;
 	result->attempt.result = err;
 	result->success.happened = succeeded;
@@ -156,7 +158,7 @@ node(char const *url, time_t attempt, int err, bool succeeded, time_t success)
 }
 
 #define NODE(url, err, succeeded, has_file) \
-	node(url, has_file, err, succeeded, 0)
+	node(url, has_file, err, succeeded, 0, 0)
 
 static void
 reset_visiteds(void)
@@ -724,13 +726,12 @@ START_TEST(test_metadata_json)
 	add_node(cache, NODE("rsync://a.b.c/e", 1, 0, 0));
 	add_node(cache, NODE("rsync://x.y.z/e", 0, 1, 0));
 	add_node(cache, NODE("https://a/b", 1, 1, 0));
-	add_node(cache, NODE("https://a/c", 0, 1, 0));
+	add_node(cache, node("https://a/c", 0, 0, 1, 0, 1));
 
 	json = build_metadata_json(cache);
 	ck_assert_int_eq(0, json_dump_file(json, "tmp/" TAL_FILE "/metadata.json", JSON_COMPACT));
 
 	str = json_dumps(json, /* JSON_INDENT(4) */ JSON_COMPACT);
-	/* printf("%s\n", str); */
 	json_decref(json);
 
 	ck_assert_str_eq(
@@ -738,9 +739,8 @@ START_TEST(test_metadata_json)
 	    "{\"url\":\"rsync://a.b.c/e\",\"attempt-timestamp\":\"1970-01-01T00:00:00Z\",\"attempt-result\":1},"
 	    "{\"url\":\"rsync://x.y.z/e\",\"attempt-timestamp\":\"1970-01-01T00:00:00Z\",\"attempt-result\":0,\"success-timestamp\":\"1970-01-01T00:00:00Z\"},"
 	    "{\"url\":\"https://a/b\",\"attempt-timestamp\":\"1970-01-01T00:00:00Z\",\"attempt-result\":1,\"success-timestamp\":\"1970-01-01T00:00:00Z\"},"
-	    "{\"url\":\"https://a/c\",\"attempt-timestamp\":\"1970-01-01T00:00:00Z\",\"attempt-result\":0,\"success-timestamp\":\"1970-01-01T00:00:00Z\"}]",
+	    "{\"url\":\"https://a/c\",\"is-rrdp-notification\":true,\"attempt-timestamp\":\"1970-01-01T00:00:00Z\",\"attempt-result\":0,\"success-timestamp\":\"1970-01-01T00:00:00Z\"}]",
 	    str);
-	printf("%s", str);
 	free(str);
 
 	cache_reset(cache);
@@ -778,7 +778,8 @@ prepare_uri_list(struct uri_list *uris, ...)
 			type = UT_RSYNC;
 		else
 			ck_abort_msg("Bad protocol: %s", str);
-		ck_assert_int_eq(0, uri_create(&uri, TAL_FILE, type, NULL, str));
+		ck_assert_int_eq(0, uri_create(&uri, TAL_FILE, type, false,
+		    NULL, str));
 		uris_add(uris, uri);
 	}
 	va_end(args);
@@ -838,18 +839,18 @@ START_TEST(test_recover)
 	 */
 	cache_reset(cache);
 
-	add_node(cache, node("rsync://a/1", 100, 0, 1, 100));
-	add_node(cache, node("rsync://a/2", 100, 1, 1, 100));
-	add_node(cache, node("rsync://a/3", 200, 0, 1, 100));
-	add_node(cache, node("rsync://a/4", 200, 1, 1, 100));
-	add_node(cache, node("rsync://a/5", 100, 0, 1, 200));
-	add_node(cache, node("rsync://a/6", 100, 1, 1, 200));
-	add_node(cache, node("rsync://b/1", 100, 0, 0, 100));
-	add_node(cache, node("rsync://b/2", 100, 1, 0, 100));
-	add_node(cache, node("rsync://b/3", 200, 0, 0, 100));
-	add_node(cache, node("rsync://b/4", 200, 1, 0, 100));
-	add_node(cache, node("rsync://b/5", 100, 0, 0, 200));
-	add_node(cache, node("rsync://b/6", 100, 1, 0, 200));
+	add_node(cache, node("rsync://a/1", 100, 0, 1, 100, 0));
+	add_node(cache, node("rsync://a/2", 100, 1, 1, 100, 0));
+	add_node(cache, node("rsync://a/3", 200, 0, 1, 100, 0));
+	add_node(cache, node("rsync://a/4", 200, 1, 1, 100, 0));
+	add_node(cache, node("rsync://a/5", 100, 0, 1, 200, 0));
+	add_node(cache, node("rsync://a/6", 100, 1, 1, 200, 0));
+	add_node(cache, node("rsync://b/1", 100, 0, 0, 100, 0));
+	add_node(cache, node("rsync://b/2", 100, 1, 0, 100, 0));
+	add_node(cache, node("rsync://b/3", 200, 0, 0, 100, 0));
+	add_node(cache, node("rsync://b/4", 200, 1, 0, 100, 0));
+	add_node(cache, node("rsync://b/5", 100, 0, 0, 200, 0));
+	add_node(cache, node("rsync://b/6", 100, 1, 0, 200, 0));
 
 	/* Multiple successful caches: Prioritize the most recent one */
 	PREPARE_URI_LIST(&uris, "rsync://a/1", "rsync://a/3", "rsync://a/5");
diff --git a/test/rrdp_test.c b/test/rrdp_test.c
index 40b7f79a..cab2b21e 100644
--- a/test/rrdp_test.c
+++ b/test/rrdp_test.c
@@ -24,8 +24,6 @@ MOCK_ABORT_INT(hash_validate_file, struct rpki_uri *uri,
 MOCK_ABORT_INT(relax_ng_parse, const char *path, xml_read_cb cb, void *arg)
 MOCK_ABORT_PTR(state_retrieve, validation, void)
 __MOCK_ABORT(tal_get_file_name, char const *, NULL, struct tal *tal)
-MOCK_ABORT_INT(uri_create, struct rpki_uri **result, char const *tal,
-    enum uri_type type, struct rpki_uri *notif, char const *guri)
 __MOCK_ABORT(uri_get_global, char const *, NULL, struct rpki_uri *uri)
 __MOCK_ABORT(uri_get_local, char const *, NULL, struct rpki_uri *uri)
 __MOCK_ABORT(uri_get_rrdp_workspace, char *, NULL, char const *tal,
diff --git a/test/types/uri_test.c b/test/types/uri_test.c
index 73858667..d8196093 100644
--- a/test/types/uri_test.c
+++ b/test/types/uri_test.c
@@ -20,8 +20,8 @@ MOCK_ABORT_INT(rrdp_update, struct rpki_uri *uri)
 
 /* Tests */
 
-#define URI_CREATE_HTTP(uri, str) uri_create(&uri, "test.tal", UT_HTTPS, NULL, str)
-#define URI_CREATE_RSYNC(uri, str) uri_create(&uri, "test.tal", UT_RSYNC, NULL, str)
+#define URI_CREATE_HTTP(uri, str) uri_create(&uri, "test.tal", UT_HTTPS, false, NULL, str)
+#define URI_CREATE_RSYNC(uri, str) uri_create(&uri, "test.tal", UT_RSYNC, false, NULL, str)
 
 START_TEST(test_constructor)
 {
@@ -147,14 +147,14 @@ START_TEST(check_caged)
 {
 	struct rpki_uri *uri;
 
-	ck_assert_int_eq(0, uri_create(&notif, "test.tal", UT_HTTPS, NULL, "https://a.b.c/d/e.xml"));
-	ck_assert_int_eq(0, uri_create(&uri, "test.tal", UT_CAGED, notif, "rsync://x.y.z/v/w.cer"));
+	ck_assert_int_eq(0, uri_create(&notif, "test.tal", UT_HTTPS, true, NULL, "https://a.b.c/d/e.xml"));
+	ck_assert_int_eq(0, uri_create(&uri, "test.tal", UT_CAGED, false, notif, "rsync://x.y.z/v/w.cer"));
 	ck_assert_str_eq("tmp/test.tal/rrdp/a.b.c/d/e.xml/x.y.z/v/w.cer", uri_get_local(uri));
 	uri_refput(uri);
 	uri_refput(notif);
 
-	ck_assert_int_eq(0, uri_create(&notif, "test.tal", UT_HTTPS, NULL, "https://a.b.c"));
-	ck_assert_int_eq(0, uri_create(&uri, "test.tal", UT_CAGED, notif, "rsync://w"));
+	ck_assert_int_eq(0, uri_create(&notif, "test.tal", UT_HTTPS, true, NULL, "https://a.b.c"));
+	ck_assert_int_eq(0, uri_create(&uri, "test.tal", UT_CAGED, false, notif, "rsync://w"));
 	ck_assert_str_eq("tmp/test.tal/rrdp/a.b.c/w", uri_get_local(uri));
 	uri_refput(uri);
 	uri_refput(notif);