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@

3 files changed, 49 insertions, 17 deletions

diff --git a/usr.sbin/rpki-client/extern.h b/usr.sbin/rpki-client/extern.h
index b811b485e3e..3f74142e432 100644
--- 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 780f40e7c35..f722f9e95bc 100644
--- 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 0dd47cd0bb0..ce97015aedf 100644
--- 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;
 }
@@ -180,6 +182,19 @@ filepath_exists(struct filepath_tree *tree, char *file)
 }
 
 /*
+ * 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.
  */
 static void
@@ -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);
 	}
 }