]> git.ipfire.org Git - thirdparty/FORT-validator.git/commitdiff
Misc. RRDP Review draft-spaghetti-sidrops-rrdp-desynchronization
authorAlberto Leiva Popper <ydahhrk@gmail.com>
Mon, 11 Mar 2024 20:22:09 +0000 (14:22 -0600)
committerAlberto Leiva Popper <ydahhrk@gmail.com>
Mon, 11 Mar 2024 20:22:09 +0000 (14:22 -0600)
Resove FIXMEs, add some comments and unit tests.

src/rrdp.c
src/types/uri.c
src/types/uri.h
test/rrdp_test.c

index 7015e4e1a488066cd418e33ee7bbe286ec3d6b3e..7f3d52dfaf24feb244dc0d07f4fabfb27161d328 100644 (file)
@@ -164,12 +164,18 @@ update_notification_init(struct update_notification *notif,
 }
 
 static void
-update_notification_cleanup(struct update_notification *file)
+__update_notification_cleanup(struct update_notification *notif)
 {
-       metadata_cleanup(&file->snapshot);
-       session_cleanup(&file->session);
-       notification_deltas_cleanup(&file->deltas, notification_delta_cleanup);
-       uri_refput(file->uri);
+       metadata_cleanup(&notif->snapshot);
+       notification_deltas_cleanup(&notif->deltas, notification_delta_cleanup);
+       uri_refput(notif->uri);
+}
+
+static void
+update_notification_cleanup(struct update_notification *notif)
+{
+       session_cleanup(&notif->session);
+       __update_notification_cleanup(notif);
 }
 
 static int
@@ -968,15 +974,18 @@ handle_deltas(struct update_notification *notif, struct rrdp_serial *serial)
        return 0;
 }
 
+/*
+ * Initializes @old by extracting relevant data from @new.
+ * Consumes @new.
+ */
 static void
-init_notif(struct update_notification *new, struct cachefile_notification *old)
+init_notif(struct cachefile_notification *old, struct update_notification *new)
 {
        size_t dn;
        size_t i;
        struct rrdp_hash *hash;
 
        old->session = new->session;
-       memset(&new->session, 0, sizeof(new->session));
        STAILQ_INIT(&old->delta_hashes);
 
        dn = config_get_rrdp_delta_threshold();
@@ -988,6 +997,8 @@ init_notif(struct update_notification *new, struct cachefile_notification *old)
                memcpy(hash->bytes, new->deltas.array[i].meta.hash, RRDP_HASH_LEN);
                STAILQ_INSERT_TAIL(&old->delta_hashes, hash, hook);
        }
+
+       __update_notification_cleanup(new);
 }
 
 static void
@@ -1003,38 +1014,42 @@ drop_notif(struct cachefile_notification *notif)
        }
 }
 
-static void
-update_notif(struct update_notification *new, struct cachefile_notification *old)
+/*
+ * Updates @old with the new information carried by @new.
+ * Consumes @new on success.
+ */
+static int
+update_notif(struct cachefile_notification *old, struct update_notification *new)
 {
-       BIGNUM *delta_bn;
-       BN_ULONG delta;
-       size_t d, dn;
+       BIGNUM *diff_bn;
+       BN_ULONG diff; /* difference between the old and new serials */
+       size_t d, dn; /* delta counter, delta "num" (total) */
        struct rrdp_hash *hash;
 
-       delta_bn = BN_new();
-       if (!BN_sub(delta_bn, new->session.serial.num, old->session.serial.num)) {
-               // FIXME
-       }
-       if (BN_is_negative(delta_bn)) {
-               // FIXME
-       }
+       diff_bn = BN_create();
+       if (!BN_sub(diff_bn, new->session.serial.num, old->session.serial.num))
+               return val_crypto_err("OUCH! libcrypto cannot subtract %s - %s",
+                   new->session.serial.str, old->session.serial.str);
+       if (BN_is_negative(diff_bn))
+               /* The validation was the BN_cmp() in the caller. */
+               pr_crit("%s - %s < 0 despite validations.",
+                   new->session.serial.str, old->session.serial.str);
 
-       delta = BN_get_word(delta_bn);
-       if (delta > new->deltas.len) {
-               // FIXME
-       }
+       diff = BN_get_word(diff_bn);
+       if (diff > new->deltas.len)
+               /* Should be <= because it was already compared to the delta threshold. */
+               pr_crit("%lu > %zu despite validations.",
+                   diff, new->deltas.len);
 
        BN_free(old->session.serial.num);
        free(old->session.serial.str);
        old->session.serial = new->session.serial;
-       new->session.serial.num = NULL;
-       new->session.serial.str = NULL;
 
-       dn = delta;
+       dn = diff;
        STAILQ_FOREACH(hash, &old->delta_hashes, hook)
                dn++;
 
-       for (d = new->deltas.len - delta; d < new->deltas.len; d++) {
+       for (d = new->deltas.len - diff; d < new->deltas.len; d++) {
                hash = pmalloc(sizeof(struct rrdp_hash));
                memcpy(hash->bytes, new->deltas.array[d].meta.hash, RRDP_HASH_LEN);
                STAILQ_INSERT_TAIL(&old->delta_hashes, hash, hook);
@@ -1046,6 +1061,10 @@ update_notif(struct update_notification *new, struct cachefile_notification *old
                free(hash);
                dn--;
        }
+
+       free(new->session.session_id);
+       __update_notification_cleanup(new);
+       return 0;
 }
 
 /*
@@ -1058,16 +1077,17 @@ update_notif(struct update_notification *new, struct cachefile_notification *old
 int
 rrdp_update(struct rpki_uri *uri)
 {
-       struct cachefile_notification **__old, *old;
+       struct cachefile_notification **cached, *old;
        struct update_notification new;
        bool changed;
+       int serial_cmp;
        int error;
 
        fnstack_push_uri(uri);
        pr_val_debug("Processing notification.");
 
        error = cache_download(validation_cache(state_retrieve()), uri,
-           &changed, &__old);
+           &changed, &cached);
        if (error)
                goto end;
        if (!changed) {
@@ -1081,49 +1101,63 @@ rrdp_update(struct rpki_uri *uri)
        pr_val_debug("New session/serial: %s/%s", new.session.session_id,
            new.session.serial.str);
 
-       old = *__old;
+       old = *cached;
        if (old == NULL) {
                pr_val_debug("This is a new Notification.");
                error = handle_snapshot(&new);
-               if (!error) {
-                       *__old = pmalloc(sizeof(struct cachefile_notification));
-                       init_notif(&new, *__old);
-               }
-               goto revert_notification;
-       }
+               if (error)
+                       goto clean_notif;
 
-       error = validate_session_desync(old, &new);
-       if (error) {
-               pr_val_debug("Falling back to snapshot.");
-               error = handle_snapshot(&new);
-               if (!error) {
-                       drop_notif(old);
-                       init_notif(&new, old);
-               }
-               goto revert_notification;
+               *cached = pmalloc(sizeof(struct cachefile_notification));
+               init_notif(*cached, &new);
+               goto end;
        }
 
-       if (BN_cmp(old->session.serial.num, new.session.serial.num) != 0) {
+       serial_cmp = BN_cmp(old->session.serial.num, new.session.serial.num);
+       if (serial_cmp < 0) {
                pr_val_debug("The Notification's serial changed.");
+               error = validate_session_desync(old, &new);
+               if (error)
+                       goto snapshot_fallback;
                error = handle_deltas(&new, &old->session.serial);
-               if (!error) {
-                       update_notif(&new, old);
-               } else {
-                       /* Error msg already printed. */
-                       pr_val_debug("Falling back to snapshot.");
-                       error = handle_snapshot(&new);
-                       if (!error) {
-                               drop_notif(old);
-                               init_notif(&new, old);
-                       }
-               }
-               goto revert_notification;
+               if (error)
+                       goto snapshot_fallback;
+               error = update_notif(old, &new);
+               if (!error)
+                       goto end;
+               /*
+                * The files are exploded and usable, but @cached is not
+                * updatable. So drop and create it anew.
+                * We might lose some delta hashes, but it's better than
+                * re-snapshotting the next time the notification changes.
+                * Not sure if it matters. This looks so unlikely, it's
+                * practically dead code.
+                */
+               goto reset_notif;
+
+       } else if (serial_cmp > 0) {
+               pr_val_debug("Cached serial is higher than notification serial.");
+               goto snapshot_fallback;
+
+       } else {
+               pr_val_debug("The Notification changed, but the session ID and serial didn't, and no session desync was detected.");
+               goto clean_notif;
        }
 
-       pr_val_debug("The Notification changed, but the session ID and serial didn't.");
+snapshot_fallback:
+       pr_val_debug("Falling back to snapshot.");
+       error = handle_snapshot(&new);
+       if (error)
+               goto clean_notif;
+
+reset_notif:
+       drop_notif(old);
+       init_notif(old, &new);
+       goto end;
 
-revert_notification:
+clean_notif:
        update_notification_cleanup(&new);
+
 end:
        fnstack_pop();
        return error;
@@ -1278,9 +1312,7 @@ rrdp_json2notif(json_t *json, struct cachefile_notification **result)
        }
        notif->session.serial.str = pstrdup(str);
 
-       notif->session.serial.num = BN_new();
-       if (notif->session.serial.num == NULL)
-               enomem_panic();
+       notif->session.serial.num = BN_create();
        if (!BN_dec2bn(&notif->session.serial.num, notif->session.serial.str)) {
                error = pr_op_err("Not a serial number: %s", notif->session.serial.str);
                goto revert_serial;
index d4ee1a0911002098427acdfae2c0761ac4fac6dd..919eef99f0ede5c240de5005cabf40ec52408d1a 100644 (file)
 #include "data_structure/path_builder.h"
 
 /**
- * Design notes:
- *
- * Because we need to generate @local from @global, @global's allowed character
- * set must be a subset of @local. Because this is Unix, @local must never
- * contain NULL (except as a terminating character). Therefore, even though IA5
- * allows NULL, @global won't.
+ * Aside from the reference counter, instances are meant to be immutable.
  *
- * Because we will simply embed @global (minus "rsync://") into @local, @local's
- * encoding must be IA5-compatible. In other words, UTF-16 and UTF-32 are out of
- * the question.
+ * TODO (fine) Needs rebranding. AFAIK, RPKI does not impose significant
+ * restrictions to regular URIs (except for schema, I guess), "global URI" is
+ * pretty much tautologic, and "local URI" is a misnomer. (Because it doesn't
+ * have anything to do with 'interpretation is independent of access'.)
+ * I can't even remember if this nomenclature made sense at some point.
+ * It's more of a mapping than a URI.
  *
- * Aside from the reference counter, instances are meant to be immutable.
+ * TODO (fine) Also, this structure is so intertwined with the cache module,
+ * nowadays it feels like it should be moved there.
  */
 struct rpki_uri {
        /**
         * "Global URI".
         * The one that always starts with "rsync://" or "https://".
-        *
-        * These things are IA5-encoded, which means you're not bound to get
-        * non-ASCII characters.
+        * Normalized, ASCII-only, NULL-terminated.
         */
        char *global;
 
        /**
         * "Local URI".
         * The file pointed by @global, but cached in the local filesystem.
-        *
-        * I can't find a standard that defines this, but lots of complaints on
-        * the Internet imply that Unix file paths are specifically meant to be
-        * C strings.
-        *
-        * So just to clarify: This is a string that permits all characters,
-        * printable or otherwise, except \0. (Because that's the terminating
-        * character.)
-        *
-        * Even though it might contain characters that are non-printable
-        * according to ASCII, we assume that we can just dump it into the
-        * output without trouble, because the input should have the same
-        * encoding as the output.
-        *
-        * Technically, "global" URI "https://a.b.c/d/..///./d" is not the same
-        * identifier as "https://a.b.c/d", but since we're supposed to download
-        * to a filesystem where "https/a.b.c/d" is the same file as
-        * "https/a.b.c/d/..///./d", @local will always be normalized.
+        * Normalized, ASCII-only, NULL-terminated.
+        * Sometimes NULL, depending on @type.
         */
        char *local;
-       /* "local_len" is never needed right now. */
 
        enum uri_type type;
 
-       unsigned int references;
+       unsigned int references; /* Reference counter */
 };
 
 /*
@@ -437,12 +417,6 @@ autocomplete_local(struct rpki_uri *uri, char const *tal,
        pr_crit("Unknown URI type: %u", uri->type);
 }
 
-/*
- * I think the reason why @guri is not a char * is to convey that it doesn't
- * need to be NULL terminated, but I'm not sure.
- *
- * FIXME callers now need to ensure @guri is NULL-terminated.
- */
 int
 uri_create(struct rpki_uri **result, char const *tal, enum uri_type type,
     struct rpki_uri *notif, char const *guri)
index 3ba86704e1b3ec1c019306195b3a39b8d31f3017..ea770692083d208b7ef7b6a40de4ca7d0a78151a 100644 (file)
@@ -4,17 +4,47 @@
 #include "asn1/asn1c/IA5String.h"
 #include "data_structure/array_list.h"
 
+/*
+ * "Long" time = seven days.
+ * Currently hardcoded, but queued for tweakability.
+ */
 enum uri_type {
-       UT_TA_RSYNC, /* TAL's TA URL; downloaded via rsync. */
-       UT_TA_HTTP, /* TAL's TA URL; downloaded via HTTP. */
-       UT_RPP, /* caRepository; downloaded via rsync. */
-       UT_NOTIF, /* rpkiNotify; downloaded via HTTP. */
-       UT_TMP, /* Snapshot or delta; Downloaded via HTTP. */
-       UT_CAGED, /* Endangered species. */
-
-       UT_AIA, /* caIssuers. Not downloaded. */
-       UT_SO, /* signedObject. Not downloaded. */
-       UT_MFT, /* rpkiManifest. Not downloaded. */
+       /*
+        * TAL's TA URL.
+        * The file is cached until it's untraversed for a "long" time.
+        */
+       UT_TA_RSYNC,
+       UT_TA_HTTP,
+
+       /*
+        * (rsync) Repository Publication Point. RFC 6481.
+        * The directory is cached until it's untraversed for a "long" time.
+        */
+       UT_RPP,
+
+       /*
+        * An RRDP notification file; downloaded via HTTP.
+        * The file itself is not cached, but we preserve a handful of metadata
+        * that is needed in subsequent iterations.
+        * The metadata is cached until it's untraversed for a "long" time.
+        */
+       UT_NOTIF,
+
+       /*
+        * RRDP Snapshot or Delta; downloaded via HTTP.
+        * The file itself is not cached, but we preserve some small metadata.
+        * The metadata is destroyed once the iteration finishes.
+        */
+       UT_TMP,
+
+       /*
+        * Endangered species; bound to be removed once RFC 9286 is implemented.
+        */
+       UT_CAGED,
+
+       UT_AIA, /* caIssuers. Not directly downloaded. */
+       UT_SO, /* signedObject. Not directly downloaded. */
+       UT_MFT, /* rpkiManifest. Not directly downloaded. */
 };
 
 struct rpki_uri;
index 7aeec25170bccf6de2c18fbc1f7d30573bdbf61c..b752646f2b564876de5c614ef19b5e152d057e39 100644 (file)
@@ -40,7 +40,7 @@ MOCK_ABORT_PTR(validation_cache, rpki_cache, struct validation *state)
 MOCK(state_retrieve, struct validation *, NULL, void)
 MOCK(validation_tal, struct tal *, NULL, struct validation *state)
 MOCK(tal_get_file_name, char const *, "", struct tal *tal)
-MOCK_UINT(config_get_rrdp_delta_threshold, 64, void)
+MOCK_UINT(config_get_rrdp_delta_threshold, 5, void)
 
 /* Mocks end */
 
@@ -319,6 +319,145 @@ validate_01234_hash(unsigned char *hash)
                ck_assert_uint_eq(expected_hash[i], hash[i]);
 }
 
+static void
+init_serial(struct rrdp_serial *serial, unsigned long num)
+{
+       char *tmp;
+
+       serial->num = BN_create();
+       ck_assert_int_eq(1, BN_add_word(serial->num, num));
+
+       tmp = BN_bn2dec(serial->num);
+       ck_assert_ptr_ne(NULL, tmp);
+       serial->str = pstrdup(tmp);
+       OPENSSL_free(tmp);
+}
+
+static void
+init_rrdp_session(struct rrdp_session *session, unsigned long serial)
+{
+       session->session_id = pstrdup("session");
+       init_serial(&session->serial, serial);
+}
+
+static void
+init_cachefile_notif(struct cachefile_notification **result, unsigned long serial, ...)
+{
+       struct cachefile_notification *notif;
+       va_list args;
+       int hash_byte;
+       struct rrdp_hash *hash;
+       size_t i;
+
+       notif = pmalloc(sizeof(struct cachefile_notification));
+       *result = notif;
+
+       init_rrdp_session(&notif->session, serial);
+       STAILQ_INIT(&notif->delta_hashes);
+
+       va_start(args, serial);
+       while ((hash_byte = va_arg(args, int)) >= 0) {
+               hash = pmalloc(sizeof(struct rrdp_hash));
+               for (i = 0; i < RRDP_HASH_LEN; i++)
+                       hash->bytes[i] = hash_byte;
+               STAILQ_INSERT_TAIL(&notif->delta_hashes, hash, hook);
+       }
+       va_end(args);
+}
+
+static void
+init_regular_notif(struct update_notification *notif, unsigned long serial, ...)
+{
+       va_list args;
+       int hash_byte;
+       struct notification_delta delta;
+       size_t i;
+
+       memset(notif, 0, sizeof(*notif));
+       init_rrdp_session(&notif->session, serial);
+       notification_deltas_init(&notif->deltas);
+
+       va_start(args, serial);
+       while ((hash_byte = va_arg(args, int)) >= 0) {
+               init_serial(&delta.serial, serial--);
+               delta.meta.uri = NULL; /* Not needed for now */
+               delta.meta.hash = pmalloc(RRDP_HASH_LEN);
+               for (i = 0; i < RRDP_HASH_LEN; i++)
+                       delta.meta.hash[i] = hash_byte;
+               delta.meta.hash_len = RRDP_HASH_LEN;
+               notification_deltas_add(&notif->deltas, &delta);
+       }
+       va_end(args);
+}
+
+static void
+validate_cachefile_notif(struct cachefile_notification *notif, unsigned long __serial, ...)
+{
+       struct rrdp_serial serial;
+       va_list args;
+       int hash_byte;
+       struct rrdp_hash *hash;
+       size_t i;
+
+       ck_assert_str_eq("session", notif->session.session_id);
+       init_serial(&serial, __serial);
+       ck_assert_str_eq(serial.str, notif->session.serial.str);
+       ck_assert_int_eq(0, BN_cmp(serial.num, notif->session.serial.num));
+       serial_cleanup(&serial);
+
+       hash = STAILQ_FIRST(&notif->delta_hashes);
+
+       va_start(args, __serial);
+       while ((hash_byte = va_arg(args, int)) >= 0) {
+               ck_assert_ptr_ne(NULL, hash);
+               for (i = 0; i < RRDP_HASH_LEN; i++)
+                       ck_assert_int_eq(hash_byte, hash->bytes[i]);
+               hash = STAILQ_NEXT(hash, hook);
+       }
+       va_end(args);
+
+       ck_assert_ptr_eq(NULL, hash);
+
+       rrdp_notif_free(notif);
+}
+
+START_TEST(test_update_notif)
+{
+       struct cachefile_notification *old;
+       struct update_notification new;
+
+       /* No changes */
+       init_cachefile_notif(&old, 5555, 1, 2, 3, -1);
+       init_regular_notif(&new, 5555, 1, 2, 3, -1);
+       ck_assert_int_eq(0, update_notif(old, &new));
+       validate_cachefile_notif(old, 5555, 1, 2, 3, -1);
+
+       /* Add a few serials */
+       init_cachefile_notif(&old, 5555, 1, 2, 3, -1);
+       init_regular_notif(&new, 5557, 3, 4, 5, -1);
+       ck_assert_int_eq(0, update_notif(old, &new));
+       validate_cachefile_notif(old, 5557, 1, 2, 3, 4, 5, -1);
+
+       /* Add serials, delta threshold exceeded */
+       init_cachefile_notif(&old, 5555, 1, 2, 3, -1);
+       init_regular_notif(&new, 5558, 3, 4, 5, 6, -1);
+       ck_assert_int_eq(0, update_notif(old, &new));
+       validate_cachefile_notif(old, 5558, 2, 3, 4, 5, 6, -1);
+
+       /* All new serials, but no hashes skipped */
+       init_cachefile_notif(&old, 5555, 1, 2, 3, -1);
+       init_regular_notif(&new, 5557, 4, 5, -1);
+       ck_assert_int_eq(0, update_notif(old, &new));
+       validate_cachefile_notif(old, 5557, 1, 2, 3, 4, 5, -1);
+
+       /* 2 previous tests combined */
+       init_cachefile_notif(&old, 5555, 1, 2, 3, 4, 5, -1);
+       init_regular_notif(&new, 5560, 6, 7, 8, 9, 10, -1);
+       ck_assert_int_eq(0, update_notif(old, &new));
+       validate_cachefile_notif(old, 5560, 6, 7, 8, 9, 10, -1);
+}
+END_TEST
+
 START_TEST(test_parse_notification_ok)
 {
        struct rpki_uri uri = { 0 };
@@ -664,6 +803,7 @@ static Suite *xml_load_suite(void)
        tcase_add_test(misc, test_xmlChar_NULL_assumption);
        tcase_add_test(misc, test_hexstr2sha256);
        tcase_add_test(misc, test_sort_deltas);
+       tcase_add_test(misc, test_update_notif);
 
        parse = tcase_create("parse");
        tcase_add_test(parse, test_parse_notification_ok);