From: Alberto Leiva Popper Date: Mon, 11 Mar 2024 20:22:09 +0000 (-0600) Subject: Misc. RRDP Review X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=refs%2Fheads%2Fdraft-spaghetti-sidrops-rrdp-desynchronization;p=thirdparty%2FFORT-validator.git Misc. RRDP Review Resove FIXMEs, add some comments and unit tests. --- diff --git a/src/rrdp.c b/src/rrdp.c index 7015e4e1..7f3d52df 100644 --- a/src/rrdp.c +++ b/src/rrdp.c @@ -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(¬if->snapshot); + notification_deltas_cleanup(¬if->deltas, notification_delta_cleanup); + uri_refput(notif->uri); +} + +static void +update_notification_cleanup(struct update_notification *notif) +{ + session_cleanup(¬if->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(¬if->session.serial.num, notif->session.serial.str)) { error = pr_op_err("Not a serial number: %s", notif->session.serial.str); goto revert_serial; diff --git a/src/types/uri.c b/src/types/uri.c index d4ee1a09..919eef99 100644 --- a/src/types/uri.c +++ b/src/types/uri.c @@ -13,57 +13,37 @@ #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) diff --git a/src/types/uri.h b/src/types/uri.h index 3ba86704..ea770692 100644 --- a/src/types/uri.h +++ b/src/types/uri.h @@ -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; diff --git a/test/rrdp_test.c b/test/rrdp_test.c index 7aeec251..b752646f 100644 --- a/test/rrdp_test.c +++ b/test/rrdp_test.c @@ -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(¬if->session, serial); + STAILQ_INIT(¬if->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(¬if->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(¬if->session, serial); + notification_deltas_init(¬if->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(¬if->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(¬if->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);