Improve duplicate detection and repo_move_valid

Only trigger a duplicate error if a valid filepath is revisted. It is
possible that a bad CA references somebody else's files and if that
happens first it would block the valid access.

To make this work, pass the ok flag to filepath_add() and only set the
talmask bit if the file was ok. Since we need to do the duplicate check
before processing the entity introduce filepath_valid() which checks
if the path is in the tree and has its talmask bit set.

In repo_move_valid() handle conflicts more gracefully. When both a valid
and temporary file are present assume that one of the files was never ok
(talmask == 0) and silently remove that file from the filepath tree.

OK tb@

diff --git a/usr.sbin/rpki-client/extern.h b/usr.sbin/rpki-client/extern.h
index b811b48..3f74142 100644 (file)
--- a/usr.sbin/rpki-client/extern.h
+++ b/usr.sbin/rpki-client/extern.h
@@ -1,4 +1,4 @@
-/*     $OpenBSD: extern.h,v 1.224 2024/06/08 13:30:35 tb Exp $ */
+/*     $OpenBSD: extern.h,v 1.225 2024/07/12 09:27:32 claudio Exp $ */
 /*
  * Copyright (c) 2019 Kristaps Dzonsons <kristaps@bsd.lv>
  *
@@ -831,7 +831,8 @@ void                 proc_http(char *, int) __attribute__((noreturn));
 void            proc_rrdp(int) __attribute__((noreturn));
 
 /* Repository handling */
-int             filepath_add(struct filepath_tree *, char *, int, time_t);
+int             filepath_add(struct filepath_tree *, char *, int, time_t, int);
+int             filepath_valid(struct filepath_tree *, char *, int);
 void            rrdp_clear(unsigned int);
 void            rrdp_session_save(unsigned int, struct rrdp_session *);
 void            rrdp_session_free(struct rrdp_session *);

diff --git a/usr.sbin/rpki-client/main.c b/usr.sbin/rpki-client/main.c
index 780f40e..f722f9e 100644 (file)
--- a/usr.sbin/rpki-client/main.c
+++ b/usr.sbin/rpki-client/main.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: main.c,v 1.261 2024/07/12 08:54:48 claudio Exp $ */
+/*     $OpenBSD: main.c,v 1.262 2024/07/12 09:27:32 claudio Exp $ */
 /*
  * Copyright (c) 2021 Claudio Jeker <claudio@openbsd.org>
  * Copyright (c) 2019 Kristaps Dzonsons <kristaps@bsd.lv>
@@ -595,7 +595,7 @@ entity_process(struct ibuf *b, struct stats *st, struct vrp_tree *tree,
        if (filemode)
                goto done;
 
-       if (filepath_add(&fpt, file, talid, mtime) == 0) {
+       if (filepath_valid(&fpt, file, talid)) {
                warnx("%s: File already visited", file);
                goto done;
        }
@@ -697,6 +697,9 @@ entity_process(struct ibuf *b, struct stats *st, struct vrp_tree *tree,
                break;
        }
 
+       if (filepath_add(&fpt, file, talid, mtime, ok) == 0)
+               errx(1, "%s: File already in tree", file);
+
 done:
        free(file);
        entity_queue--;

diff --git a/usr.sbin/rpki-client/repo.c b/usr.sbin/rpki-client/repo.c
index 0dd47cd..ce97015 100644 (file)
--- a/usr.sbin/rpki-client/repo.c
+++ b/usr.sbin/rpki-client/repo.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: repo.c,v 1.60 2024/06/07 08:22:53 claudio Exp $ */
+/*     $OpenBSD: repo.c,v 1.61 2024/07/12 09:27:32 claudio Exp $ */
 /*
  * Copyright (c) 2021 Claudio Jeker <claudio@openbsd.org>
  * Copyright (c) 2019 Kristaps Dzonsons <kristaps@bsd.lv>
@@ -133,7 +133,8 @@ RB_PROTOTYPE(filepath_tree, filepath, entry, filepathcmp);
  * Functions to lookup which files have been accessed during computation.
  */
 int
-filepath_add(struct filepath_tree *tree, char *file, int id, time_t mtime)
+filepath_add(struct filepath_tree *tree, char *file, int id, time_t mtime,
+    int ok)
 {
        struct filepath *fp, *rfp;
 
@@ -154,7 +155,8 @@ filepath_add(struct filepath_tree *tree, char *file, int id, time_t mtime)
                        return 0;
                fp = rfp;
        }
-       fp->talmask |= (1 << id);
+       if (ok)
+               fp->talmask |= (1 << id);
 
        return 1;
 }
@@ -179,6 +181,19 @@ filepath_exists(struct filepath_tree *tree, char *file)
        return filepath_find(tree, file) != NULL;
 }
 
+/*
+ * Returns true if file exists and the id bit is set and ok flag is true.
+ */
+int
+filepath_valid(struct filepath_tree *tree, char *file, int id)
+{
+       struct filepath *fp;
+
+       if ((fp = filepath_find(tree, file)) == NULL)
+               return 0;
+       return (fp->talmask & (1 << id)) != 0;
+}
+
 /*
  * Remove entry from tree and free it.
  */
@@ -922,7 +937,7 @@ rrdp_handle_file(unsigned int id, enum publish_type pt, char *uri,
 
        /* write new content or mark uri as deleted. */
        if (pt == PUB_DEL) {
-               filepath_add(&rr->deleted, uri, 0, 0);
+               filepath_add(&rr->deleted, uri, 0, 0, 1);
        } else {
                fp = filepath_find(&rr->deleted, uri);
                if (fp != NULL) {
@@ -1630,7 +1645,7 @@ repo_cleanup_rrdp(struct filepath_tree *tree)
 static void
 repo_move_valid(struct filepath_tree *tree)
 {
-       struct filepath *fp, *nfp;
+       struct filepath *fp, *nfp, *ofp;
        size_t rsyncsz = strlen(".rsync/");
        size_t rrdpsz = strlen(".rrdp/");
        size_t tasz = strlen(".ta/");
@@ -1677,20 +1692,33 @@ repo_move_valid(struct filepath_tree *tree)
                if (repo_mkpath(AT_FDCWD, fn) == -1)
                        continue;
 
-               if (rename(fp->file, fn) == -1) {
-                       warn("rename %s", fp->file);
-                       continue;
-               }
-
                /* switch filepath node to new path */
                RB_REMOVE(filepath_tree, tree, fp);
                base = fp->file;
                if ((fp->file = strdup(fn)) == NULL)
                        err(1, NULL);
+
+ again:
+               if ((ofp = RB_INSERT(filepath_tree, tree, fp)) != NULL) {
+                       if (ofp->talmask == 0) {
+                               /* conflicting path is not valid, drop it */
+                               filepath_put(tree, ofp);
+                               goto again;
+                       }
+                       if (fp->talmask != 0) {
+                               warnx("%s: file already present in "
+                                   "validated cache", fp->file);
+                       }
+                       free(fp->file);
+                       free(fp);
+                       free(base);
+                       continue;
+               }
+
+               if (rename(base, fp->file) == -1)
+                       warn("rename to %s", fp->file);
+
                free(base);
-               if (RB_INSERT(filepath_tree, tree, fp) != NULL)
-                       errx(1, "%s: both possibilities of file present",
-                           fp->file);
        }
 }