Fix traversal result code

handle_tal_uri() was returning 0 on soft errors and positive on success.
It's supposed to be the other way around.

This resulted in the main loop dropping successful tree traversals.

It also resulted in TA public key mismatches causing traversal
termination, which was a violation of RFC 8630:

> If the connection to the preferred URI fails or the retrieved CA
> certificate public key does not match the TAL public key, the RP
> SHOULD retrieve the CA certificate from the next URI, according to
> the local preference ranking of URIs.

This bug was introduced by 604f845ce6b9eb596dc9f1fdff7a7fa5752fcc87.

diff --git a/src/object/certificate.c b/src/object/certificate.c
index 8fe0a1db86e4c85b3f8845dff1f26c63db1be08c..0a77064c4df268ad290c8642064028abda985d49 100644 (file)
--- a/src/object/certificate.c
+++ b/src/object/certificate.c
@@ -1919,15 +1919,27 @@ certificate_validate_aia(struct rpki_uri *caIssuers, X509 *cert)
        return 0;
 }
 
+static int
+retrieve_uri(struct rpki_uri *uri, void *arg)
+{
+       struct rpki_uri **result = arg;
+       *result = uri;
+       return 0;
+}
+
 static struct rpki_uri *
 download_rpp(struct sia_uris *uris)
 {
+       struct rpki_uri *uri;
+       int error;
+
        if (uris->rpp.len == 0) {
                pr_val_err("SIA lacks both caRepository and rpkiNotify.");
                return NULL;
        }
 
-       return uris_download(&uris->rpp, true);
+       error = uris_download(&uris->rpp, true, retrieve_uri, &uri);
+       return error ? NULL : uri;
 }
 
 /** Boilerplate code for CA certificate validation and recursive traversal. */

diff --git a/src/object/tal.c b/src/object/tal.c
index b3fc82f9a3767c2be4199ffe8a71bd7a7178f668..8ad7477ed7a86fe04d6a2a12df218da62b7c48d1 100644 (file)
--- a/src/object/tal.c
+++ b/src/object/tal.c
@@ -40,11 +40,16 @@ struct validation_thread {
 /* List of threads, one per TAL file */
 SLIST_HEAD(threads_list, validation_thread);
 
-struct tal_param {
+struct tal_thread_args {
        struct db_table *db;
        struct threads_list threads;
 };
 
+struct handle_tal_args {
+       struct tal *tal;
+       struct db_table *db;
+};
+
 static int
 add_uri(struct uri_list *uris, char *uri)
 {
@@ -330,25 +335,11 @@ tal_get_spki(struct tal *tal, unsigned char const **buffer, size_t *len)
 
 /**
  * Performs the whole validation walkthrough on uri @uri, which is assumed to
- * have been extracted from a TAL.
+ * have been extracted from TAL @tal.
  */
 static int
 handle_tal_uri(struct tal *tal, struct rpki_uri *uri, struct db_table *db)
 {
-       /*
-        * Because of the way the foreach iterates, this function must return
-        *
-        * - 0 on soft errors.
-        * - `> 0` on URI handled successfully.
-        * - `< 0` on hard errors.
-        *
-        * A "soft error" is "the connection to the preferred URI fails, or the
-        * retrieved CA certificate public key does not match the TAL public
-        * key." (RFC 8630)
-        *
-        * A "hard error" is any other error.
-        */
-
        struct validation_handler validation_handler;
        struct validation *state;
        struct cert_stack *certstack;
@@ -367,9 +358,10 @@ handle_tal_uri(struct tal *tal, struct rpki_uri *uri, struct db_table *db)
                return ENSURE_NEGATIVE(error);
 
        if (!uri_is_certificate(uri)) {
-               error = pr_op_err("TAL URI does not point to a certificate. (Expected .cer, got '%s')",
+               pr_op_err("TAL URI does not point to a certificate. (Expected .cer, got '%s')",
                    uri_op_get_printable(uri));
-               goto fail;
+               error = EINVAL;
+               goto end;
        }
 
        /* Handle root certificate. */
@@ -377,13 +369,13 @@ handle_tal_uri(struct tal *tal, struct rpki_uri *uri, struct db_table *db)
        if (error) {
                switch (validation_pubkey_state(state)) {
                case PKS_INVALID:
-                       error = 0; /* Try a different TAL URI. */
+                       error = EINVAL;
                        goto end;
                case PKS_VALID:
                case PKS_UNTESTED:
-                       goto fail; /* Reject the TAL. */
+                       error = ENSURE_NEGATIVE(error);
+                       goto end;
                }
-
                pr_crit("Unknown public key state: %u",
                    validation_pubkey_state(state));
        }
@@ -402,8 +394,7 @@ handle_tal_uri(struct tal *tal, struct rpki_uri *uri, struct db_table *db)
        do {
                error = deferstack_pop(certstack, &deferred);
                if (error == -ENOENT) {
-                       /* No more certificates left; we're done. */
-                       error = 1;
+                       error = 0; /* No more certificates left; we're done */
                        goto end;
                } else if (error) /* All other errors are critical, currently */
                        pr_crit("deferstack_pop() returned illegal %d.", error);
@@ -418,18 +409,24 @@ handle_tal_uri(struct tal *tal, struct rpki_uri *uri, struct db_table *db)
                rpp_refput(deferred.pp);
        } while (true);
 
-fail:  error = ENSURE_NEGATIVE(error);
 end:   validation_destroy(state);
        pr_val_debug("}");
        return error;
 }
 
+static int
+__handle_tal_uri(struct rpki_uri *uri, void *arg)
+{
+       struct handle_tal_args *args = arg;
+       return handle_tal_uri(args->tal, uri, args->db);
+}
+
 static void *
 do_file_validation(void *arg)
 {
        struct validation_thread *thread = arg;
        struct tal *tal;
-       struct rpki_uri *ta_uri;
+       struct handle_tal_args handle_args;
        int error;
 
        fnstack_init();
@@ -439,19 +436,15 @@ do_file_validation(void *arg)
        if (error)
                goto end;
 
-       ta_uri = uris_download(&tal->uris, false);
-       if (ta_uri == NULL) {
-               error = pr_op_err("None of the URIs of the TAL '%s' yielded a successful traversal.",
+       handle_args.tal = tal;
+       handle_args.db = thread->db;
+       error = uris_download(&tal->uris, false, __handle_tal_uri, &handle_args);
+       if (error)
+               pr_op_err("None of the URIs of the TAL '%s' yielded a successful traversal.",
                    thread->tal_file);
-               goto destroy_tal;
-       }
-
-       error = handle_tal_uri(tal, ta_uri, thread->db);
 
-destroy_tal:
        tal_destroy(tal);
-end:
-       fnstack_cleanup();
+end:   fnstack_cleanup();
        thread->exit_status = error;
        return NULL;
 }
@@ -467,16 +460,16 @@ thread_destroy(struct validation_thread *thread)
 static int
 spawn_tal_thread(char const *tal_file, void *arg)
 {
-       struct tal_param *param = arg;
+       struct tal_thread_args *thread_args = arg;
        struct validation_thread *thread;
        int error;
 
        thread = pmalloc(sizeof(struct validation_thread));
 
        thread->tal_file = pstrdup(tal_file);
-       thread->db = param->db;
+       thread->db = thread_args->db;
        thread->exit_status = -EINTR;
-       SLIST_INSERT_HEAD(&param->threads, thread, next);
+       SLIST_INSERT_HEAD(&thread_args->threads, thread, next);
 
        error = pthread_create(&thread->pid, NULL, do_file_validation, thread);
        if (error) {
@@ -492,33 +485,33 @@ spawn_tal_thread(char const *tal_file, void *arg)
 int
 perform_standalone_validation(struct db_table *table)
 {
-       struct tal_param param;
+       struct tal_thread_args args;
        struct validation_thread *thread;
        int error, tmperr;
 
-       param.db = table;
-       SLIST_INIT(&param.threads);
+       args.db = table;
+       SLIST_INIT(&args.threads);
 
-       /* TODO (fine) Maybe don't use threads if there's only one TAL */
+       /* TODO (fine) Maybe don't spawn threads if there's only one TAL */
        error = foreach_file(config_get_tal(), ".tal", true, spawn_tal_thread,
-           &param);
+           &args);
        if (error) {
-               while (!SLIST_EMPTY(&param.threads)) {
-                       thread = SLIST_FIRST(&param.threads);
-                       SLIST_REMOVE_HEAD(&param.threads, next);
+               while (!SLIST_EMPTY(&args.threads)) {
+                       thread = SLIST_FIRST(&args.threads);
+                       SLIST_REMOVE_HEAD(&args.threads, next);
                        thread_destroy(thread);
                }
                return error;
        }
 
        /* Wait for all */
-       while (!SLIST_EMPTY(&param.threads)) {
-               thread = SLIST_FIRST(&param.threads);
+       while (!SLIST_EMPTY(&args.threads)) {
+               thread = SLIST_FIRST(&args.threads);
                tmperr = pthread_join(thread->pid, NULL);
                if (tmperr)
                        pr_crit("pthread_join() threw %d (%s) on the '%s' thread.",
                            tmperr, strerror(tmperr), thread->tal_file);
-               SLIST_REMOVE_HEAD(&param.threads, next);
+               SLIST_REMOVE_HEAD(&args.threads, next);
                if (thread->exit_status) {
                        error = thread->exit_status;
                        pr_op_warn("Validation from TAL '%s' yielded error %d (%s); discarding all validation results.",

diff --git a/src/types/uri.c b/src/types/uri.c
index 0b7efd301dc9c0938e1824f4c7918418c8d09f14..6eecbf9b2af3a2c7c734ae7f10c5870ac6ba8f06 100644 (file)
--- a/src/types/uri.c
+++ b/src/types/uri.c
@@ -597,23 +597,35 @@ uris_add(struct uri_list *uris, struct rpki_uri *uri)
 }
 
 static int
-download(struct rpki_uri *uri, bool use_rrdp)
+download(struct rpki_uri *uri, bool use_rrdp, uris_dl_cb cb, void *arg)
 {
-       return (use_rrdp && (uri_get_type(uri) == UT_HTTPS))
+       int error;
+
+       error = (use_rrdp && (uri_get_type(uri) == UT_HTTPS))
            ? rrdp_update(uri)
            : cache_download(uri, NULL);
+       if (error)
+               return 1;
+
+       return cb(uri, arg);
 }
 
-static struct rpki_uri *
-download_uris(struct uri_list *uris, enum uri_type type, bool use_rrdp)
+static int
+download_uris(struct uri_list *uris, enum uri_type type, bool use_rrdp,
+    uris_dl_cb cb, void *arg)
 {
-       struct rpki_uri **cursor, *uri;
-       ARRAYLIST_FOREACH(uris, cursor) {
-               uri = *cursor;
-               if (uri_get_type(uri) == type && download(uri, use_rrdp) == 0)
-                       return uri;
+       struct rpki_uri **uri;
+       int error;
+
+       ARRAYLIST_FOREACH(uris, uri) {
+               if (uri_get_type(*uri) == type) {
+                       error = download(*uri, use_rrdp, cb, arg);
+                       if (error <= 0)
+                               return error;
+               }
        }
-       return NULL;
+
+       return 1;
 }
 
 /**
@@ -622,41 +634,42 @@ download_uris(struct uri_list *uris, enum uri_type type, bool use_rrdp)
  *
  * Sequentially (in the order dictated by their priorities) attempts to update
  * (in the cache) the content pointed by each URL.
- * Stops on the first success, returning the corresponding URI.
- *
- * If there's no successful update, attempts to find one that's already cached.
- * Returns the newest successfully cached URI.
+ * If a download succeeds, calls cb on it. If cb succeeds, returns without
+ * trying more URLs.
  *
- * Does not grab any references.
+ * If none of the URLs download and callback properly, attempts to find one
+ * that's already cached, and callbacks it.
  */
-struct rpki_uri *
-uris_download(struct uri_list *uris, bool use_rrdp)
+int
+uris_download(struct uri_list *uris, bool use_rrdp, uris_dl_cb cb, void *arg)
 {
        struct rpki_uri **cursor, *uri;
+       int error;
 
        if (config_get_http_priority() > config_get_rsync_priority()) {
-               uri = download_uris(uris, UT_HTTPS, use_rrdp);
-               if (uri != NULL)
-                       return uri;
-               uri = download_uris(uris, UT_RSYNC, use_rrdp);
-               if (uri != NULL)
-                       return uri;
+               error = download_uris(uris, UT_HTTPS, use_rrdp, cb, arg);
+               if (error <= 0)
+                       return error;
+               error = download_uris(uris, UT_RSYNC, use_rrdp, cb, arg);
+               if (error <= 0)
+                       return error;
 
        } else if (config_get_http_priority() < config_get_rsync_priority()) {
-               uri = download_uris(uris, UT_RSYNC, use_rrdp);
-               if (uri != NULL)
-                       return uri;
-               uri = download_uris(uris, UT_HTTPS, use_rrdp);
-               if (uri != NULL)
-                       return uri;
+               error = download_uris(uris, UT_RSYNC, use_rrdp, cb, arg);
+               if (error <= 0)
+                       return error;
+               error = download_uris(uris, UT_HTTPS, use_rrdp, cb, arg);
+               if (error <= 0)
+                       return error;
 
        } else {
                ARRAYLIST_FOREACH(uris, cursor) {
-                       uri = *cursor;
-                       if (download(uri, use_rrdp) == 0)
-                               return uri;
+                       error = download(*cursor, use_rrdp, cb, arg);
+                       if (error <= 0)
+                               return error;
                }
        }
 
-       return cache_recover(uris, use_rrdp);
+       uri = cache_recover(uris, use_rrdp);
+       return (uri != NULL) ? cb(uri, arg) : ESRCH;
 }

diff --git a/src/types/uri.h b/src/types/uri.h
index ba03a0e6f2e44526d236ff45bfca50583db18082..493fb01885ae09782036ed3eb814fd13a365da35 100644 (file)
--- a/src/types/uri.h
+++ b/src/types/uri.h
@@ -56,6 +56,15 @@ void uris_init(struct uri_list *);
 void uris_cleanup(struct uri_list *);
 
 void uris_add(struct uri_list *, struct rpki_uri *);
-struct rpki_uri *uris_download(struct uri_list *, bool);
+
+/*
+ * The callback should return
+ *
+ * - 0 on success ("URI handled successfully")
+ * - > 0 on soft errors ("Try another URI")
+ * - < 0 on hard errors ("Abandon foreach")
+ */
+typedef int (*uris_dl_cb)(struct rpki_uri *, void *);
+int uris_download(struct uri_list *, bool, uris_dl_cb, void *);
 
 #endif /* SRC_TYPES_URI_H_ */