From 9de06b67f7da2c54aa97a7d8c93eb2859baaecb9 Mon Sep 17 00:00:00 2001 From: Alberto Leiva Popper Date: Mon, 16 Sep 2024 12:43:21 -0600 Subject: [PATCH] Design 3 development checkpoint Sigh. The testing is forcing me to realize several misconceptions I've been carrying. Status: This code is design 3. There are three refactors that clearly need to be addressed immediately: 1. Design 3 is vulnerable to notification stack URL hacking, which has to be fixed by shifting to designs 3.1 or (unlikely) 4. 2. Fall back to rsync only on RRDP download failure; it's wasteful to also do it on RRDP validation failure. 3. Must commit repository content at the RPP level, not at the module level. (One failing RPP should not invalidate the whole module.) --- src/Makefile.am | 2 +- src/cache.c | 14 ++++-- src/cachent.c | 17 ++++--- src/cachent.h | 2 +- src/common.c | 13 ++--- src/common.h | 4 +- src/config.c | 13 ++++- src/config.h | 5 +- src/file.c | 23 ++++++--- src/hash.c | 13 ++++- src/http.c | 2 +- src/log.c | 106 ++++++++++++++++++++++----------------- src/log.h | 11 +++- src/main.c | 2 +- src/object/certificate.c | 3 +- src/object/tal.c | 7 ++- src/print_file.c | 2 +- src/rrdp.c | 37 +++++++------- src/rsync.c | 2 +- src/types/map.c | 13 ++--- src/types/path.c | 7 +++ src/types/path.h | 1 + test/Makefile.am | 4 +- test/cache_test.c | 96 ++++++++++++++++++++++++++++++----- test/cache_util.c | 2 +- test/mock.c | 45 +++++++++-------- test/rrdp_update_test.c | 18 +------ test/rrdp_util.h | 21 ++++++++ 28 files changed, 311 insertions(+), 174 deletions(-) create mode 100644 test/rrdp_util.h diff --git a/src/Makefile.am b/src/Makefile.am index 62f74736..83ae4720 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -96,7 +96,7 @@ fort_SOURCES += validation_handler.h validation_handler.c include asn1/asn1c/Makefile.include fort_SOURCES += $(ASN_MODULE_SRCS) $(ASN_MODULE_HDRS) -#fort_CFLAGS = -Wall -Wpedantic -Werror +# -Werror -Wno-unused fort_CFLAGS = -Wall -Wpedantic #fort_CFLAGS += $(GCC_WARNS) fort_CFLAGS += -std=c99 -D_DEFAULT_SOURCE=1 -D_XOPEN_SOURCE=700 -D_BSD_SOURCE=1 diff --git a/src/cache.c b/src/cache.c index ef15fceb..25d9514d 100644 --- a/src/cache.c +++ b/src/cache.c @@ -173,7 +173,7 @@ init_tmp_dir(void) dirname = get_cache_filename(CACHE_TMPDIR, true); - if (mkdir(dirname, 0777) < 0) { + if (mkdir(dirname, CACHE_FILEMODE) < 0) { error = errno; if (error != EEXIST) return pr_op_err("Cannot create '%s': %s", @@ -568,13 +568,16 @@ get_tmppath(struct cache_node *node) ancestor = node; do { ancestor = ancestor->parent; - if (ancestor == NULL) - pr_crit("aaaaa"); // XXX This should never happen, but maybe don't crash anyway + if (ancestor == NULL) { + /* May warrant pr_crit(), but I'm chickening out. */ + pr_val_err("The download function did not set any tmppaths."); + return NULL; + } } while (ancestor->tmppath == NULL); skiplen = strlen(ancestor->path); if (strncmp(ancestor->path, node->path, skiplen) != 0) - return NULL; // XXX + pr_crit("???"); // XXX pb_init(&pb); if (pb_append(&pb, ancestor->tmppath) != 0) @@ -751,7 +754,7 @@ commit_rpp_delta(struct cache_node *node) pr_op_debug("Branch."); goto branch; } else { - pr_op_debug("Not changed; nothing to commit."); + pr_op_debug("Unchanged leaf; nothing to commit."); return true; } } @@ -898,6 +901,7 @@ nftw_remove_abandoned(const char *path, const struct stat *st, goto unknown; } if (!pm && !(msm->flags & CNF_RSYNC)) { + /* XXX what about HTTP? */ pr_op_debug("RRDP and no perfect match; unknown."); goto unknown; /* The traversal is depth-first */ } diff --git a/src/cachent.c b/src/cachent.c index 3f51f822..72aa2bd8 100644 --- a/src/cachent.c +++ b/src/cachent.c @@ -15,7 +15,7 @@ cachent_root(char const *schema, char const *dir) root = pzalloc(sizeof(struct cache_node)); root->url = (char *)schema; root->path = join_paths(config_get_local_repository(), dir); - root->name = strrchr(root->path, '/') + 1; + root->name = path_filename(root->path); root->flags = CNF_FREE_PATH; return root; @@ -48,7 +48,7 @@ cachent_traverse(struct cache_node *root, bool (*cb)(struct cache_node *)) return; parent = root; - iter_start = parent->children; + iter_start = parent->children; /* aka "first child" */ if (iter_start == NULL) return; @@ -61,7 +61,6 @@ reloop: /* iter_start must not be NULL */ } } - parent = iter_start->parent; do { if (parent == NULL) return; @@ -83,7 +82,6 @@ find_child(struct cache_node *parent, char const *name, size_t namelen) /* * Returns perfect match or NULL. @msm will point to the Most Specific Match. * Assumes @path is normalized. - * XXX if root doesn't match path, will return garbage */ struct cache_node * cachent_find(struct cache_node *root, char const *path, struct cache_node **msm) @@ -102,8 +100,15 @@ cachent_find(struct cache_node *root, char const *path, struct cache_node **msm) for (parent = child = root; token_next(&tkn); parent = child) { child = find_child(parent, tkn.str, tkn.len); if (!child) { - *msm = parent; - return NULL; + if (parent->flags & CNF_NOTIFICATION) { + child = find_child(parent->rrdp.subtree, + tkn.str, tkn.len); + if (!child) + goto nochild; + } else { +nochild: *msm = parent; + return NULL; + } } } diff --git a/src/cachent.h b/src/cachent.h index 63a1c1a9..8d297101 100644 --- a/src/cachent.h +++ b/src/cachent.h @@ -9,7 +9,7 @@ #include "rrdp.h" #include "types/uthash.h" -/* XXX rename "touched" and "validated" into "preserve"? */ +/* XXX rename "touched" and "valid" into "preserve"? */ // XXX trees now separated; consider removing this flag #define CNF_RSYNC (1 << 0) diff --git a/src/common.c b/src/common.c index c3c2db7d..f986673e 100644 --- a/src/common.c +++ b/src/common.c @@ -251,7 +251,7 @@ stat_dir(char const *path) } static int -ensure_dir(char const *path, mode_t mode) +ensure_dir(char const *path) { int error; @@ -263,7 +263,7 @@ ensure_dir(char const *path, mode_t mode) if (stat_dir(path) == ENOTDIR && remove(path) < 0) return errno; - if (mkdir(path, mode) < 0) { + if (mkdir(path, CACHE_FILEMODE) < 0) { error = errno; return (error == EEXIST) ? 0 : error; } @@ -274,7 +274,7 @@ ensure_dir(char const *path, mode_t mode) /* mkdir -p $_path */ /* XXX Maybe also short-circuit by parent? */ int -mkdir_p(char const *_path, bool include_basename, mode_t mode) +mkdir_p(char const *_path, bool include_basename) { char *path, *last_slash; int i, result = 0; @@ -297,13 +297,10 @@ mkdir_p(char const *_path, bool include_basename, mode_t mode) goto end; } - if (result == ENOTDIR) - pr_op_err_st("stack tracing for '%s'...", path); - for (i = 1; path[i] != '\0'; i++) { if (path[i] == '/') { path[i] = '\0'; - result = ensure_dir(path, mode); + result = ensure_dir(path); path[i] = '/'; if (result != 0) { pr_op_err_st("Error during mkdir(%s): %s", @@ -312,7 +309,7 @@ mkdir_p(char const *_path, bool include_basename, mode_t mode) } } } - result = ensure_dir(path, mode); + result = ensure_dir(path); end: free(path); return result; diff --git a/src/common.h b/src/common.h index eff5ee24..8685814e 100644 --- a/src/common.h +++ b/src/common.h @@ -39,13 +39,15 @@ int rwlock_read_lock(pthread_rwlock_t *); void rwlock_write_lock(pthread_rwlock_t *); void rwlock_unlock(pthread_rwlock_t *); +#define CACHE_FILEMODE 0755 + typedef int (*foreach_file_cb)(char const *, void *); int foreach_file(char const *, char const *, bool, foreach_file_cb, void *); // XXX bool valid_file_or_dir(char const *, bool); -int mkdir_p(char const *, bool, mode_t); +int mkdir_p(char const *, bool); int delete_dir_recursive_bottom_up(char const *); time_t time_nonfatal(void); diff --git a/src/config.c b/src/config.c index 0320af7d..6e25cea0 100644 --- a/src/config.c +++ b/src/config.c @@ -22,6 +22,7 @@ #include "state.h" #include "thread_pool.h" #include "types/array.h" +#include "types/path.h" /** * To add a member to this structure, @@ -1292,7 +1293,7 @@ config_get_op_log_color_output(void) } enum filename_format -config_get_op_log_filename_format(void) +config_get_op_log_file_format(void) { return rpki_config.log.filename_format; } @@ -1334,11 +1335,19 @@ config_get_val_log_color_output(void) } enum filename_format -config_get_val_log_filename_format(void) +config_get_val_log_file_format(void) { return rpki_config.validation_log.filename_format; } +char const * +logv_filename(char const *path) +{ + return (rpki_config.validation_log.filename_format == FNF_NAME) + ? path_filename(path) + : path; +} + uint8_t config_get_val_log_level(void) { diff --git a/src/config.h b/src/config.h index 4933f433..710cbd25 100644 --- a/src/config.h +++ b/src/config.h @@ -65,7 +65,7 @@ char const *config_get_payload(void); bool config_get_op_log_enabled(void); char const * config_get_op_log_tag(void); bool config_get_op_log_color_output(void); -enum filename_format config_get_op_log_filename_format(void); +enum filename_format config_get_op_log_file_format(void); uint8_t config_get_op_log_level(void); enum log_output config_get_op_log_output(void); uint32_t config_get_op_log_facility(void); @@ -73,7 +73,8 @@ uint32_t config_get_op_log_facility(void); bool config_get_val_log_enabled(void); char const * config_get_val_log_tag(void); bool config_get_val_log_color_output(void); -enum filename_format config_get_val_log_filename_format(void); +enum filename_format config_get_val_log_file_format(void); +char const *logv_filename(char const *); uint8_t config_get_val_log_level(void); enum log_output config_get_val_log_output(void); uint32_t config_get_val_log_facility(void); diff --git a/src/file.c b/src/file.c index 27e16d12..d992ea8a 100644 --- a/src/file.c +++ b/src/file.c @@ -68,7 +68,7 @@ file_write_full(char const *path, unsigned char const *content, size_t written; int error; - error = mkdir_p(path, false, 0777); + error = mkdir_p(path, false); if (error) return error; @@ -157,6 +157,7 @@ file_free(struct file_contents *fc) } /* Wrapper for stat(), mostly for the sake of unit test mocking. */ +/* XXX needs a rename, because it returns errno. */ int file_exists(char const *path) { @@ -181,23 +182,23 @@ merge_into(const char *src, const struct stat *st, int typeflag, if (S_ISDIR(st->st_mode)) { pr_op_debug("mkdir -p %s", dst); - if (mkdir_p(dst, true, st->st_mode)) { + if (mkdir_p(dst, true)) { PR_DEBUG_MSG("Failed: %s", strerror(errno)); goto end; } times[0] = st->st_atim; times[1] = st->st_mtim; - if (utimensat(AT_FDCWD, dst, times, AT_SYMLINK_NOFOLLOW)) + if (utimensat(AT_FDCWD, dst, times, AT_SYMLINK_NOFOLLOW) < 0) PR_DEBUG_MSG("utimensat: %s", strerror(errno)); } else { pr_op_debug("rename: %s -> %s", src, dst); - if (rename(src, dst)) { + if (rename(src, dst) < 0) { if (errno == EISDIR) { /* XXX stacked nftw()s */ if (file_rm_rf(dst) != 0) PR_DEBUG_MSG("%s", "AAAAAAAAAAA"); - if (rename(src, dst)) + if (rename(src, dst) < 0) PR_DEBUG_MSG("rename: %s", strerror(errno)); } } @@ -221,10 +222,20 @@ file_merge_into(char const *src, char const *dst) { int error; - error = mkdir_p(dst, false, 0777); + error = mkdir_p(dst, false); if (error) return error; + if (file_exists(dst) == ENOENT) { + if (rename(src, dst) < 0) { + error = errno; + pr_op_err("Could not move %s to %s: %s", + src, dst, strerror(error)); + return error; + } + return 0; + } + src_offset = strlen(src); merge_dst = dst; /* TODO (performance) optimize that 32 */ diff --git a/src/hash.c b/src/hash.c index 43887b5c..21e5d837 100644 --- a/src/hash.c +++ b/src/hash.c @@ -178,7 +178,18 @@ hash_validate_file(struct hash_algorithm const *algorithm, char const *path, return 0; fail: - return pr_val_err("File '%s' does not match its expected hash.", path); + error = pr_val_err("File '%s' does not match its expected hash.", path); +#ifdef UNIT_TESTING + size_t i; + printf("Expected: "); + for (i = 0; i < expected_len; i++) + printf("%02x", expected[i]); + printf("\nActual: "); + for (i = 0; i < actual_len; i++) + printf("%02x", actual[i]); + printf("\n"); +#endif + return error; } static int diff --git a/src/http.c b/src/http.c index 3d3d6a4e..ef7d3275 100644 --- a/src/http.c +++ b/src/http.c @@ -389,7 +389,7 @@ http_download(char const *url, char const *path, curl_off_t ims, bool *changed) pr_val_info("HTTP GET: %s -> %s", url, path); /* XXX might not be needed anymore */ - error = mkdir_p(path, false, 0777); + error = mkdir_p(path, false); if (error) return error; diff --git a/src/log.c b/src/log.c index f15264b0..741d508b 100644 --- a/src/log.c +++ b/src/log.c @@ -13,6 +13,7 @@ #include "config.h" #include "thread_var.h" +#include "types/path.h" struct level { char const *label; @@ -20,22 +21,22 @@ struct level { FILE *stream; }; -static struct level DBG = { "DBG", "\x1B[36m" }; /* Cyan */ -static struct level INF = { "INF", "\x1B[37m" }; /* White */ -static struct level WRN = { "WRN", "\x1B[33m" }; /* Yellow */ -static struct level ERR = { "ERR", "\x1B[31m" }; /* Red */ -static struct level CRT = { "CRT", "\x1B[35m" }; /* Purple */ +static struct level DBG = { "DBG", PR_COLOR_DBG }; +static struct level INF = { "INF", PR_COLOR_INF }; +static struct level WRN = { "WRN", PR_COLOR_WRN }; +static struct level ERR = { "ERR", PR_COLOR_ERR }; +static struct level CRT = { "CRT", PR_COLOR_CRT }; static struct level UNK = { "UNK", "" }; -#define COLOR_RESET "\x1B[0m" struct log_config { bool fprintf_enabled; /* Print on the standard streams? */ bool syslog_enabled; /* Print on syslog? */ uint8_t level; - char const *prefix; + char const *tag; bool color; int facility; + bool rm_filepath; }; /* Configuration for the operation logs. */ @@ -44,10 +45,8 @@ static struct log_config op_config; static struct log_config val_config; /* - * Note: Normally, fprintf and syslog would have separate locks. - * - * However, fprintf and syslog are rarely enabled at the same time, so I don't - * think it's worth it. So I'm reusing the lock. + * fprintf and syslog are rarely enabled at the same time, so I reused the + * mutex. * * "log" + "lock" = "logck" */ @@ -103,12 +102,12 @@ print_stack_trace(char const *title) #endif /* BACKTRACE_ENABLED */ } -static void init_config(struct log_config *cfg, bool unit_tests) +static void init_config(struct log_config *cfg) { cfg->fprintf_enabled = true; - cfg->syslog_enabled = !unit_tests; + cfg->syslog_enabled = true; cfg->level = LOG_DEBUG; - cfg->prefix = NULL; + cfg->tag = NULL; cfg->color = false; cfg->facility = LOG_DAEMON; } @@ -207,7 +206,7 @@ register_signal_handlers(void) } int -log_setup(bool unit_tests) +log_setup(void) { /* * Remember not to use any actual logging functions until logging has @@ -223,25 +222,21 @@ log_setup(bool unit_tests) CRT.stream = stderr; UNK.stream = stdout; - if (unit_tests) - openlog("fort", LOG_CONS | LOG_PID, LOG_DAEMON); - - init_config(&op_config, unit_tests); - init_config(&val_config, unit_tests); + init_config(&op_config); + init_config(&val_config); error = pthread_mutex_init(&logck, NULL); if (error) { - fprintf(ERR.stream, "pthread_mutex_init() returned %d: %s\n", + fprintf(ERR.stream, + "pthread_mutex_init() returned %d: %s\n", + error, strerror(error)); + syslog(LOG_ERR | op_config.facility, + "pthread_mutex_init() returned %d: %s", error, strerror(error)); - if (!unit_tests) - syslog(LOG_ERR | op_config.facility, - "pthread_mutex_init() returned %d: %s", - error, strerror(error)); return error; } - if (!unit_tests) - register_signal_handlers(); + register_signal_handlers(); return 0; } @@ -294,13 +289,15 @@ log_start(void) } op_config.level = config_get_op_log_level(); - op_config.prefix = config_get_op_log_tag(); + op_config.tag = config_get_op_log_tag(); op_config.color = config_get_op_log_color_output(); op_config.facility = config_get_op_log_facility(); + op_config.rm_filepath = config_get_op_log_file_format() == FNF_NAME; val_config.level = config_get_val_log_level(); - val_config.prefix = config_get_val_log_tag(); + val_config.tag = config_get_val_log_tag(); val_config.color = config_get_val_log_color_output(); val_config.facility = config_get_val_log_facility(); + val_config.rm_filepath = config_get_val_log_file_format() == FNF_NAME; } void @@ -398,24 +395,28 @@ __vfprintf(int level, struct log_config *cfg, char const *format, va_list args) now = time(NULL); if (now != ((time_t) -1)) { + // XXX not catching any errors localtime_r(&now, &stm_buff); strftime(time_buff, sizeof(time_buff), "%b %e %T", &stm_buff); fprintf(lvl->stream, "%s ", time_buff); } fprintf(lvl->stream, "%s", lvl->label); - if (cfg->prefix) - fprintf(lvl->stream, " [%s]", cfg->prefix); + if (cfg->tag) + fprintf(lvl->stream, " [%s]", cfg->tag); fprintf(lvl->stream, ": "); file_name = fnstack_peek(); - if (file_name != NULL) + if (file_name != NULL) { + if (cfg->rm_filepath) + file_name = path_filename(file_name); fprintf(lvl->stream, "%s: ", file_name); + } vfprintf(lvl->stream, format, args); if (cfg->color) - fprintf(lvl->stream, COLOR_RESET); + fprintf(lvl->stream, PR_COLOR_RST); fprintf(lvl->stream, "\n"); /* Force flush */ @@ -425,35 +426,46 @@ __vfprintf(int level, struct log_config *cfg, char const *format, va_list args) unlock_mutex(); } -#define MSG_LEN 1024 +/* + * TODO (fine) Optimize. Notice the buffer is static, which seems to be the + * reason why it's (probably ill-advisedly) mutexing. + */ +#define MSG_LEN 512 static void __syslog(int level, struct log_config *cfg, const char *format, va_list args) { static char msg[MSG_LEN]; - char const *file_name; + char const *file; + int res; - file_name = fnstack_peek(); + level |= cfg->facility; + file = fnstack_peek(); + if (file && cfg->rm_filepath) + file = path_filename(file); lock_mutex(); /* Can't use vsyslog(); it's not portable. */ - vsnprintf(msg, MSG_LEN, format, args); - if (file_name != NULL) { - if (cfg->prefix != NULL) - syslog(level | cfg->facility, "[%s] %s: %s", - cfg->prefix, file_name, msg); + res = vsnprintf(msg, MSG_LEN, format, args); + if (res < 0) + goto end; + if (res >= MSG_LEN) + msg[MSG_LEN - 1] = '\0'; + + if (file != NULL) { + if (cfg->tag != NULL) + syslog(level, "[%s] %s: %s", cfg->tag, file, msg); else - syslog(level | cfg->facility, "%s: %s", file_name, msg); + syslog(level, "%s: %s", file, msg); } else { - if (cfg->prefix != NULL) - syslog(level | cfg->facility, "[%s] %s", - cfg->prefix, msg); + if (cfg->tag != NULL) + syslog(level, "[%s] %s", cfg->tag, msg); else - syslog(level | cfg->facility, "%s", msg); + syslog(level, "%s", msg); } - unlock_mutex(); +end: unlock_mutex(); } #define PR_SIMPLE(lvl, config) \ diff --git a/src/log.h b/src/log.h index cde4010e..5c94dd43 100644 --- a/src/log.h +++ b/src/log.h @@ -6,6 +6,13 @@ #include "incidence.h" +#define PR_COLOR_DBG "\x1B[36m" /* Cyan */ +#define PR_COLOR_INF "\x1B[37m" /* White */ +#define PR_COLOR_WRN "\x1B[33m" /* Yellow */ +#define PR_COLOR_ERR "\x1B[31m" /* Red */ +#define PR_COLOR_CRT "\x1B[35m" /* Purple */ +#define PR_COLOR_RST "\x1B[0m" + /* * According to BSD style, __dead is supposed to be defined in sys/cdefs.h, * but it doesn't exist in Linux. @@ -40,7 +47,7 @@ #endif /* - * Only call this group of functions when you know there's only one thread. + * Only call this group of functions while you know there's only one thread. * * log_setup() is an incomplete initialization meant to be called when the * program starts. Logging can be performed after log_setup(), but it will use @@ -48,7 +55,7 @@ * log_init() finishes initialization by loading the user's intended config. * log_teardown() reverts initialization. */ -int log_setup(bool); +int log_setup(void); void log_start(void); void log_teardown(void); diff --git a/src/main.c b/src/main.c index 4d63ee81..0184d209 100644 --- a/src/main.c +++ b/src/main.c @@ -116,7 +116,7 @@ main(int argc, char **argv) /* Initializations */ - error = log_setup(false); + error = log_setup(); if (error) goto just_quit; diff --git a/src/object/certificate.c b/src/object/certificate.c index 9a09a8ef..1e1e5b1e 100644 --- a/src/object/certificate.c +++ b/src/object/certificate.c @@ -24,6 +24,7 @@ #include "object/manifest.h" #include "thread_var.h" #include "types/path.h" +#include "types/url.h" /* * The X509V3_EXT_METHOD that references NID_sinfo_access uses the AIA item. @@ -1853,7 +1854,7 @@ check_rpp(struct cache_mapping *map_rpp, void *rpkiManifest) int error; mft.url = rpkiManifest; - mft.path = join_paths(map_rpp->path, strrchr(mft.url, '/')); // XXX + mft.path = join_paths(map_rpp->path, mft.url + RPKI_SCHEMA_LEN); error = handle_manifest(&mft, &pp); if (error) diff --git a/src/object/tal.c b/src/object/tal.c index 02af11a0..489d2a89 100644 --- a/src/object/tal.c +++ b/src/object/tal.c @@ -11,6 +11,7 @@ #include "file.h" #include "log.h" #include "thread_var.h" +#include "types/path.h" struct tal { char const *file_name; @@ -110,7 +111,6 @@ premature: static int tal_init(struct tal *tal, char const *file_path) { - char const *file_name; struct file_contents file; int error; @@ -118,9 +118,7 @@ tal_init(struct tal *tal, char const *file_path) if (error) return error; - file_name = strrchr(file_path, '/'); - file_name = (file_name != NULL) ? (file_name + 1) : file_path; - tal->file_name = file_name; + tal->file_name = path_filename(file_path); strlist_init(&tal->urls); error = read_content((char *)file.buffer, tal); @@ -219,6 +217,7 @@ handle_ta(struct cache_mapping *ta, void *arg) */ certificate_traverse(deferred.pp, &deferred.map); + map_cleanup(&deferred.map); rpp_refput(deferred.pp); } while (true); diff --git a/src/print_file.c b/src/print_file.c index ed23c79f..7d0f534d 100644 --- a/src/print_file.c +++ b/src/print_file.c @@ -53,7 +53,7 @@ rsync2bio_tmpdir(char const *src) error = pb_append(&pb, tmpdir); if (error) goto end; - error = pb_append(&pb, strrchr(src, '/') + 1); + error = pb_append(&pb, path_filename(src)); if (error) goto end; diff --git a/src/rrdp.c b/src/rrdp.c index 5723a151..ab87db9f 100644 --- a/src/rrdp.c +++ b/src/rrdp.c @@ -13,6 +13,7 @@ #include "relax_ng.h" #include "thread_var.h" #include "types/arraylist.h" +#include "types/path.h" #include "types/url.h" /* RRDP's XML namespace */ @@ -486,13 +487,12 @@ handle_publish(xmlTextReaderPtr reader, struct cache_node *notif) error = parse_publish(reader, &tag); if (error) goto end; - pr_val_debug("- publish %s", tag.meta.uri); if (!notif->rrdp.subtree) { subtree = pzalloc(sizeof(struct cache_node)); subtree->url = "rsync://"; subtree->path = notif->path; - subtree->name = strrchr(subtree->path, '/') + 1; + subtree->name = path_filename(subtree->path); subtree->tmppath = notif->tmppath; notif->rrdp.subtree = subtree; } @@ -525,7 +525,7 @@ handle_publish(xmlTextReaderPtr reader, struct cache_node *notif) goto end; } - pr_val_debug("Caching file: %s", node->tmppath); + pr_val_debug("Publish %s", logv_filename(node->tmppath)); error = file_write_full(node->tmppath, tag.content, tag.content_len); end: metadata_cleanup(&tag.meta); @@ -544,8 +544,6 @@ handle_withdraw(xmlTextReaderPtr reader, struct cache_node *notif) if (error) goto end; - pr_val_debug("- withdraw: %s", tag.meta.uri); - node = cachent_provide(notif->rrdp.subtree, tag.meta.uri); if (!node) { // XXX outdated msg @@ -570,6 +568,7 @@ handle_withdraw(xmlTextReaderPtr reader, struct cache_node *notif) goto end; node->flags |= CNF_WITHDRAWN; + pr_val_debug("Withdraw %s", logv_filename(tag.meta.uri)); end: metadata_cleanup(&tag.meta); return error; @@ -857,7 +856,7 @@ handle_snapshot(struct update_notification *new, struct cache_node *notif) char *tmppath; int error; - pr_val_debug("Processing snapshot '%s'.", new->snapshot.uri); + pr_val_debug("Processing snapshot."); fnstack_push(new->snapshot.uri); error = dl_tmp(new->snapshot.uri, &tmppath); @@ -1112,20 +1111,18 @@ dl_notif(struct cache_node *notif, struct update_notification *new) if (notif->dlerr) goto end; -// if (remove(tmppath) < 0) { -// notif->dlerr = errno; -// pr_val_err("Can't remove notification's temporal file: %s", -// strerror(notif->dlerr)); -// update_notification_cleanup(new); -// goto end; -// } -// if (mkdir(tmppath, 0777) < 0) { -// notif->dlerr = errno; -// pr_val_err("Can't create notification's temporal directory: %s", -// strerror(notif->dlerr)); -// update_notification_cleanup(new); -// goto end; -// } + if (remove(tmppath) < 0) { + /* + * Note, this could be ignored if we weren't planning on reusing + * the path. This is going to stop being an issue once streaming + * is implemented. + */ + notif->dlerr = errno; + pr_val_err("Can't remove notification's temporal file: %s", + strerror(notif->dlerr)); + update_notification_cleanup(new); + goto end; + } notif->flags |= CNF_FREE_TMPPATH; notif->tmppath = tmppath; diff --git a/src/rsync.c b/src/rsync.c index 0ab21925..36aa596b 100644 --- a/src/rsync.c +++ b/src/rsync.c @@ -299,7 +299,7 @@ rsync_download(char const *src, char const *dst, char const *cmpdst) pr_val_debug(" %s", args[i]); } - error = mkdir_p(dst, true, 0777); + error = mkdir_p(dst, true); if (error) return error; diff --git a/src/types/map.c b/src/types/map.c index 3f0ea814..2a6c13af 100644 --- a/src/types/map.c +++ b/src/types/map.c @@ -5,13 +5,6 @@ #include "log.h" #include "types/path.h" -static char const * -get_filename(char const *file_path) -{ - char *slash = strrchr(file_path, '/'); - return (slash != NULL) ? (slash + 1) : file_path; -} - static char const * map_get_printable(struct cache_mapping *map, enum filename_format format) { @@ -21,7 +14,7 @@ map_get_printable(struct cache_mapping *map, enum filename_format format) case FNF_LOCAL: return map->path; case FNF_NAME: - return get_filename(map->url); + return path_filename(map->url); } pr_crit("Unknown file name format: %u", format); @@ -31,13 +24,13 @@ map_get_printable(struct cache_mapping *map, enum filename_format format) char const * map_val_get_printable(struct cache_mapping *map) { - return map_get_printable(map, config_get_val_log_filename_format()); + return map_get_printable(map, config_get_val_log_file_format()); } char const * map_op_get_printable(struct cache_mapping *map) { - return map_get_printable(map, config_get_op_log_filename_format()); + return map_get_printable(map, config_get_op_log_file_format()); } void diff --git a/src/types/path.c b/src/types/path.c index 364ed6d2..33235cfa 100644 --- a/src/types/path.c +++ b/src/types/path.c @@ -238,6 +238,13 @@ path_childn(char const *p1, char const *p2, size_t p2len) return pb.string; } +char const * +path_filename(char const *path) +{ + char *slash = strrchr(path, '/'); + return slash ? (slash + 1) : path; +} + /* * Cannot return NULL. * diff --git a/src/types/path.h b/src/types/path.h index 54369a1b..7fcc54a8 100644 --- a/src/types/path.h +++ b/src/types/path.h @@ -50,6 +50,7 @@ void pb_cleanup(struct path_builder *); char *path_parent(char const *); char *path_childn(char const *, char const *, size_t); +char const *path_filename(char const *); char *join_paths(char const *, char const *); #endif /* SRC_TYPES_PATH_H_ */ diff --git a/test/Makefile.am b/test/Makefile.am index 54a04640..49ba65ba 100644 --- a/test/Makefile.am +++ b/test/Makefile.am @@ -12,7 +12,7 @@ if USE_TESTS # _CFLAGS is not defined. # Otherwise it must be included manually: # mumble_mumble_CFLAGS = ${AM_CFLAGS} flag1 flag2 flag3 ... -AM_CFLAGS = -pedantic -Wall -Wno-unused +AM_CFLAGS = -Wall -Wpedantic AM_CFLAGS += -std=c99 -D_DEFAULT_SOURCE=1 -D_XOPEN_SOURCE=700 -D_BSD_SOURCE=1 AM_CFLAGS += -I../src -DUNIT_TESTING ${CHECK_CFLAGS} ${XML2_CFLAGS} ${JANSSON_CFLAGS} # Reminder: As opposed to AM_CFLAGS, "AM_LDADD" is not idiomatic automake, and @@ -58,7 +58,7 @@ cachent_test_SOURCES = cachent_test.c cachent_test_LDADD = ${MY_LDADD} ${JANSSON_LIBS} cache_test_SOURCES = cache_test.c -cache_test_LDADD = ${MY_LDADD} ${JANSSON_LIBS} +cache_test_LDADD = ${MY_LDADD} ${JANSSON_LIBS} ${XML2_LIBS} db_table_test_SOURCES = rtr/db/db_table_test.c db_table_test_LDADD = ${MY_LDADD} diff --git a/test/cache_test.c b/test/cache_test.c index 232f1375..6c47f9fd 100644 --- a/test/cache_test.c +++ b/test/cache_test.c @@ -4,14 +4,20 @@ #include #include "alloc.c" +#include "base64.c" #include "common.c" #include "cache.c" #include "cachent.c" #include "cachetmp.c" #include "cache_util.c" #include "file.c" +#include "hash.c" +#include "json_util.c" #include "mock.c" #include "mock_https.c" +#include "rrdp_util.h" +#include "relax_ng.c" +#include "rrdp.c" #include "types/path.c" #include "types/str.c" #include "types/url.c" @@ -20,29 +26,28 @@ static unsigned int rsync_counter; /* Times the rsync function was called */ -int -rsync_download(char const *src, char const *dst, char const *cmpdir) +static void +touch_file(char const *dir) { char cmd[64]; + ck_assert(snprintf(cmd, sizeof(cmd), "touch %s/file", dir) < sizeof(cmd)); + ck_assert_int_eq(0, system(cmd)); +} +int +rsync_download(char const *src, char const *dst, char const *cmpdir) +{ rsync_counter++; if (dl_error) return dl_error; - ck_assert_int_eq(0, mkdir_p(dst, true, 0777)); - - ck_assert(snprintf(cmd, sizeof(cmd), "touch %s/file", dst) < sizeof(cmd)); - ck_assert_int_eq(0, system(cmd)); + ck_assert_int_eq(0, mkdir_p(dst, true)); + touch_file(dst); return 0; } -MOCK_ABORT_INT(rrdp_update, struct cache_node *notif) -__MOCK_ABORT(rrdp_notif2json, json_t *, NULL, struct cachefile_notification *notif) -MOCK_ABORT_VOID(rrdp_notif_cleanup, struct cachefile_notification *notif) -MOCK_VOID(rrdp_notif_free, struct cachefile_notification *notif) -MOCK_ABORT_INT(rrdp_json2notif, json_t *json, struct cachefile_notification **result) MOCK_VOID(__delete_node_cb, struct cache_node const *node) /* Helpers */ @@ -790,6 +795,70 @@ START_TEST(test_cache_cleanup_https_error) } END_TEST +START_TEST(test_collisions) +{ + /* + * Request + * + * 1. rsync://a.b.c/d/e/f/ + * 2. https://x.y.z/m/n/o.notification -> rsync://a.b.c/d/e/f/ + * + * - None validates + * - rsync validates + * - https validates + * - Both validate + */ + + struct sia_uris sias; + + ck_assert_int_eq(0, hash_setup()); + ck_assert_int_eq(0, relax_ng_init()); + + setup_test(); + + printf("==== 1 ====\n"); + + /* Context: rsync */ + sias.caRepository = "rsync://a.b.c/mod/rpp1"; + sias.rpkiManifest = "rsync://a.b.c/mod/rpp1/m.mft"; + sias.rpkiNotify = NULL; + + rsync_counter = https_counter = 0; + ck_assert_int_eq(0, cache_download_alt(&sias, okay, NULL)); + ck_assert_uint_eq(1, rsync_counter); + ck_assert_uint_eq(0, https_counter); + + cache_print(); + + /* + * Context: notification "https://a.b.c/d/notification.xml". + * Both point to the same caRepository (RPP). + * + * This is either two benign RPPs coexisting (likely because of key + * rollover), or one malicious RPP is trying to overwrite the other. + * + * So they need to be cached separately. We cannot reuse the RPP simply + * because the caRepositories are identical. + */ + sias.rpkiNotify = "https://a.b.c/d/notification.xml"; + + dls[0] = NHDR("12") NSS("https://a.b.c/d/snapshot.xml", + "d880c0e3136695636f73f8fb6340245182f4b19bd4b092679b9002ad427dc380") + NTAIL; + dls[1] = SHDR("12") PBLSH("rsync://a.b.c/mod/rpp1/m.mft", + "ZXhhbXBsZTE=") STAIL; + dls[2] = NULL; + + rsync_counter = https_counter = 0; + ck_assert_int_eq(0, cache_download_alt(&sias, okay, NULL)); + ck_assert_uint_eq(0, rsync_counter); + ck_assert_uint_eq(2, https_counter); + + cache_print(); + + // XXX +} + START_TEST(test_dots) { setup_test(); @@ -1016,7 +1085,7 @@ END_TEST static Suite *thread_pool_suite(void) { Suite *suite; - TCase *rsync, *https, *dot, *meta, *recover; + TCase *rsync, *https, *mix, *dot, *meta, *recover; rsync = tcase_create("rsync"); tcase_add_test(rsync, test_cache_download_rsync); @@ -1030,6 +1099,9 @@ static Suite *thread_pool_suite(void) tcase_add_test(https, test_cache_cleanup_https); tcase_add_test(https, test_cache_cleanup_https_error); + mix = tcase_create("mix"); + tcase_add_test(https, test_collisions); + dot = tcase_create("dot"); tcase_add_test(dot, test_dots); diff --git a/test/cache_util.c b/test/cache_util.c index 08a588db..8bfb7a84 100644 --- a/test/cache_util.c +++ b/test/cache_util.c @@ -54,7 +54,7 @@ vnode(char const *url, char const *path, int flags, char const *tmppath, result->url = (char *)url; result->path = (char *)path; - result->name = strrchr(result->path, '/') + 1; + result->name = path_filename(result->path); ck_assert_ptr_ne(NULL, result->name); result->flags = flags; result->tmppath = (char *)tmppath; diff --git a/test/mock.c b/test/mock.c index 44c70736..83dda331 100644 --- a/test/mock.c +++ b/test/mock.c @@ -4,6 +4,7 @@ #include #include "config.h" #include "incidence.h" +#include "log.h" #include "state.h" #include "thread_var.h" @@ -14,50 +15,51 @@ MOCK_TRUE(log_op_enabled, unsigned int l) /* CFLAGS=-DPRINT_PRS make check */ #ifdef PRINT_PRS -#define MOCK_PRINT \ +#define MOCK_PRINT(color) \ do { \ va_list args; \ + printf(color); \ va_start(args, format); \ vfprintf(stdout, format, args); \ va_end(args); \ - printf("\n"); \ + printf(PR_COLOR_RST "\n"); \ } while (0) #else -#define MOCK_PRINT +#define MOCK_PRINT(color) #endif -#define MOCK_VOID_PRINT(name) \ +#define MOCK_VOID_PRINT(name, color) \ void \ name(const char *format, ...) \ { \ - MOCK_PRINT; \ + MOCK_PRINT(color); \ } -#define MOCK_INT_PRINT(name, result) \ +#define MOCK_INT_PRINT(name, color, result) \ int \ name(const char *format, ...) \ { \ - MOCK_PRINT; \ + MOCK_PRINT(color); \ return result; \ } -MOCK_VOID_PRINT(pr_op_debug) -MOCK_VOID_PRINT(pr_op_info) -MOCK_INT_PRINT(pr_op_warn, 0) -MOCK_INT_PRINT(pr_op_err, -EINVAL) -MOCK_INT_PRINT(pr_op_err_st, -EINVAL) -MOCK_INT_PRINT(op_crypto_err, -EINVAL) +MOCK_VOID_PRINT(pr_op_debug, PR_COLOR_DBG) +MOCK_VOID_PRINT(pr_op_info, PR_COLOR_INF) +MOCK_INT_PRINT(pr_op_warn, PR_COLOR_WRN, 0) +MOCK_INT_PRINT(pr_op_err, PR_COLOR_ERR, -EINVAL) +MOCK_INT_PRINT(pr_op_err_st, PR_COLOR_ERR, -EINVAL) +MOCK_INT_PRINT(op_crypto_err, PR_COLOR_ERR, -EINVAL) -MOCK_VOID_PRINT(pr_val_debug) -MOCK_VOID_PRINT(pr_val_info) -MOCK_INT_PRINT(pr_val_warn, 0) -MOCK_INT_PRINT(pr_val_err, -EINVAL) -MOCK_INT_PRINT(val_crypto_err, -EINVAL) +MOCK_VOID_PRINT(pr_val_debug, PR_COLOR_DBG) +MOCK_VOID_PRINT(pr_val_info, PR_COLOR_INF) +MOCK_INT_PRINT(pr_val_warn, PR_COLOR_WRN, 0) +MOCK_INT_PRINT(pr_val_err, PR_COLOR_ERR, -EINVAL) +MOCK_INT_PRINT(val_crypto_err, PR_COLOR_ERR, -EINVAL) int incidence(enum incidence_id id, const char *format, ...) { - MOCK_PRINT; + MOCK_PRINT(PR_COLOR_ERR); return -EINVAL; } @@ -118,8 +120,9 @@ MOCK_TRUE(config_get_http_enabled, void) MOCK_UINT(config_get_http_priority, 60, void) MOCK_NULL(config_get_output_roa, char const *, void) MOCK_NULL(config_get_output_bgpsec, char const *, void) -MOCK(config_get_op_log_filename_format, enum filename_format, FNF_NAME, void) -MOCK(config_get_val_log_filename_format, enum filename_format, FNF_NAME, void) +MOCK(config_get_op_log_file_format, enum filename_format, FNF_NAME, void) +MOCK(config_get_val_log_file_format, enum filename_format, FNF_NAME, void) +MOCK(logv_filename, char const *, path, char const *path) MOCK_VOID(fnstack_init, void) MOCK_VOID(fnstack_push, char const *file) diff --git a/test/rrdp_update_test.c b/test/rrdp_update_test.c index 17cb8239..69ff2fb3 100644 --- a/test/rrdp_update_test.c +++ b/test/rrdp_update_test.c @@ -13,6 +13,7 @@ #include "mock_https.c" #include "relax_ng.c" #include "rrdp.c" +#include "rrdp_util.h" #include "types/path.c" #include "types/url.c" @@ -53,23 +54,6 @@ ck_file(char const *path) ck_assert_str_eq("Fort\n", buffer); } -#define NHDR(serial) "\n" -#define NSS(u, h) "\t\n" -#define NTAIL "" - -#define SHDR(serial) "\n" -#define STAIL "" - -#define PBLSH(u, c) "" c "" - /* Tests */ START_TEST(startup) diff --git a/test/rrdp_util.h b/test/rrdp_util.h new file mode 100644 index 00000000..64bfe7bb --- /dev/null +++ b/test/rrdp_util.h @@ -0,0 +1,21 @@ +#ifndef TEST_RRDP_UTIL_H_ +#define TEST_RRDP_UTIL_H_ + +#define NHDR(serial) "\n" +#define NSS(u, h) "\t\n" +#define NTAIL "" + +#define SHDR(serial) "\n" +#define STAIL "" + +#define PBLSH(u, c) "" c "" + +#endif /* TEST_RRDP_UTIL_H_ */ -- 2.47.2