On Wed, Jan 26, 2022 at 10:06:37AM +0100, Claudio Jeker wrote:
> This diff removes the valid/ subdir in favor of a more direct directory
> layout for all valid CA repository files.
> It moves rrdp and rsync to .rsync and .rrdp but keeps ta/ because trust
> anchors are special.
>
> The biggest change is probably in the FTS code to cleanup the repo since
> the traversing now starts at ".". Apart from that removing valid results
> in some simplified code (esp. in -f mode).
>
> This diff is mostly from job@, I just did fixed the FTS bits so the
> cleanup works again.
If I do a full run with this diff, I run into
rpki-client: both possibilities of file present
Instrumenting this error (should it include fn?) it turns out to be
pki-repo.registro.br/repo/A2x6icaKpVWP5uUBzJQzRvEpjvS7PV2qt1Ct/1/3230302e3138392e34302e302f32332d3234203d3e20313roa
I have not yet figured out why.
One tiny nit inline.
> --
> :wq Claudio
>
> Index: parser.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
> retrieving revision 1.54
> diff -u -p -r1.54 parser.c
> --- parser.c 24 Jan 2022 17:29:37 -0000 1.54
> +++ parser.c 25 Jan 2022 16:36:12 -0000
> @@ -737,9 +737,9 @@ parse_entity(struct entityq *q, struct m
> * verification.
> */
> static void
> -parse_load_crl(const char *uri)
> +parse_load_crl(char *uri)
> {
> - char *nfile, *f;
> + char *f;
> size_t flen;
>
> if (uri == NULL)
> @@ -750,19 +750,14 @@ parse_load_crl(const char *uri)
> }
> uri += strlen("rsync://");
>
> - if (asprintf(&nfile, "valid/%s", uri) == -1)
> - err(1, NULL);
> -
> - f = load_file(nfile, &flen);
> + f = load_file(uri, &flen);
> if (f == NULL) {
> - warn("parse file %s", nfile);
> - goto done;
> + warn("parse file %s", uri);
> + return;
> }
>
> - proc_parser_crl(nfile, f, flen);
> + proc_parser_crl(uri, f, flen);
>
> -done:
> - free(nfile);
> free(f);
> }
>
> @@ -773,10 +768,10 @@ done:
> * necessary certs were loaded. Returns NULL on failure.
> */
> static struct cert *
> -parse_load_cert(const char *uri)
> +parse_load_cert(char *uri)
> {
> struct cert *cert = NULL;
> - char *nfile, *f;
> + char *f;
> size_t flen;
>
> if (uri == NULL)
> @@ -788,33 +783,28 @@ parse_load_cert(const char *uri)
> }
> uri += strlen("rsync://");
>
> - if (asprintf(&nfile, "valid/%s", uri) == -1)
> - err(1, NULL);
> -
> - f = load_file(nfile, &flen);
> + f = load_file(uri, &flen);
> if (f == NULL) {
> - warn("parse file %s", nfile);
> + warn("parse file %s", uri);
> goto done;
> }
>
> - cert = cert_parse(nfile, f, flen);
> + cert = cert_parse(uri, f, flen);
> free(f);
>
> if (cert == NULL)
> goto done;
> if (cert->purpose != CERT_PURPOSE_CA) {
> - warnx("AIA reference to bgpsec cert %s", nfile);
> + warnx("AIA reference to bgpsec cert %s", uri);
> goto done;
> }
> /* try to load the CRL of this cert */
> parse_load_crl(cert->crl);
>
> - free(nfile);
> return cert;
>
> -done:
> + done:
> cert_free(cert);
> - free(nfile);
> return NULL;
> }
>
> Index: repo.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/repo.c,v
> retrieving revision 1.27
> diff -u -p -r1.27 repo.c
> --- repo.c 24 Jan 2022 15:50:34 -0000 1.27
> +++ repo.c 26 Jan 2022 08:39:07 -0000
> @@ -224,8 +224,14 @@ repo_dir(const char *uri, const char *di
> local = uri;
> }
>
> - if (asprintf(&out, "%s/%s", dir, local) == -1)
> - err(1, NULL);
> + if (dir == NULL) {
> + if ((out = strdup(local)) == NULL)
> + err(1, NULL);
> + } else {
> + if (asprintf(&out, "%s/%s", dir, local) == -1)
> + err(1, NULL);
> + }
> +
> free(hdir);
> return out;
> }
> @@ -436,7 +442,7 @@ rsync_get(const char *uri, const char *v
> SLIST_INSERT_HEAD(&rsyncrepos, rr, entry);
>
> rr->repouri = repo;
> - rr->basedir = repo_dir(repo, "rsync", 0);
> + rr->basedir = repo_dir(repo, ".rsync", 0);
>
> /* create base directory */
> if (mkpath(rr->basedir) == -1) {
> @@ -484,13 +490,16 @@ rrdp_filename(const struct rrdprepo *rr,
> char *nfile;
> const char *dir = rr->basedir;
>
> - if (valid)
> - dir = "valid";
> if (!valid_uri(uri, strlen(uri), "rsync://"))
> errx(1, "%s: bad URI %s", rr->basedir, uri);
> uri += strlen("rsync://"); /* skip proto */
> - if (asprintf(&nfile, "%s/%s", dir, uri) == -1)
> - err(1, NULL);
> + if (valid) {
> + if ((nfile = strdup(uri)) == NULL)
> + err(1, NULL);
> + } else {
> + if (asprintf(&nfile, "%s/%s", dir, uri) == -1)
> + err(1, NULL);
> + }
> return nfile;
> }
>
> @@ -734,7 +743,7 @@ rrdp_get(const char *uri)
>
> if ((rr->notifyuri = strdup(uri)) == NULL)
> err(1, NULL);
> - rr->basedir = repo_dir(uri, "rrdp", 1);
> + rr->basedir = repo_dir(uri, ".rrdp", 1);
>
> RB_INIT(&rr->deleted);
>
> @@ -1058,7 +1067,7 @@ repo_lookup(int talid, const char *uri,
> }
>
> rp = repo_alloc(talid);
> - rp->basedir = repo_dir(repouri, "valid", 0);
> + rp->basedir = repo_dir(repouri, NULL, 0);
> rp->repouri = repouri;
> if (notify != NULL)
> if ((rp->notifyuri = strdup(notify)) == NULL)
> @@ -1270,40 +1279,38 @@ static void
> repo_move_valid(struct filepath_tree *tree)
> {
> struct filepath *fp, *nfp;
> - size_t rsyncsz = strlen("rsync/");
> - size_t rrdpsz = strlen("rrdp/");
> + size_t rsyncsz = strlen(".rsync/");
> + size_t rrdpsz = strlen(".rrdp/");
> char *fn, *base;
>
> RB_FOREACH_SAFE(fp, filepath_tree, tree, nfp) {
> - if (strncmp(fp->file, "rsync/", rsyncsz) != 0 &&
> - strncmp(fp->file, "rrdp/", rrdpsz) != 0)
> + if (strncmp(fp->file, ".rsync/", rsyncsz) != 0 &&
> + strncmp(fp->file, ".rrdp/", rrdpsz) != 0)
> continue; /* not a temporary file path */
>
> - if (strncmp(fp->file, "rsync/", rsyncsz) == 0) {
> - if (asprintf(&fn, "valid/%s", fp->file + rsyncsz) == -1)
> - err(1, NULL);
> + if (strncmp(fp->file, ".rsync/", rsyncsz) == 0) {
> + fn = fp->file + rsyncsz;
> } else {
> base = strchr(fp->file + rrdpsz, '/');
> assert(base != NULL);
> - if (asprintf(&fn, "valid/%s", base + 1) == -1)
> - err(1, NULL);
> + fn = base + 1;
> }
>
> if (repo_mkpath(fn) == -1) {
> - free(fn);
> continue;
> }
>
> if (rename(fp->file, fn) == -1) {
> warn("rename %s", fp->file);
> - free(fn);
> continue;
> }
>
> /* switch filepath node to new path */
> RB_REMOVE(filepath_tree, tree, fp);
> - free(fp->file);
> - fp->file = fn;
> + base = fp->file;
> + if ((fp->file = strdup(fn)) == NULL)
> + err(1, NULL);
> + free(base);
The dance with base is not necessary:
free(fp->file)
if ((fp->file = strdup(fn)) == NULL)
err(1, NULL);
> if (RB_INSERT(filepath_tree, tree, fp) != NULL)
> errx(1, "both possibilities of file present");
> }
> @@ -1313,12 +1320,20 @@ repo_move_valid(struct filepath_tree *tr
> #define RSYNC_DIR (void *)0x73796e63
> #define RRDP_DIR (void *)0x52524450
>
> +static inline char *
> +skip_dotslash(char *in)
> +{
> + if (memcmp(in, "./", 2) == 0)
> + return in + 2;
> + return in;
> +}
> +
> void
> repo_cleanup(struct filepath_tree *tree)
> {
> size_t i, cnt, delsz = 0, dirsz = 0;
> char **del = NULL, **dir = NULL;
> - char *argv[5] = { "ta", "rsync", "rrdp", "valid", NULL };
> + char *argv[2] = { ".", NULL };
> FTS *fts;
> FTSENT *e;
>
> @@ -1331,38 +1346,38 @@ repo_cleanup(struct filepath_tree *tree)
> err(1, "fts_open");
> errno = 0;
> while ((e = fts_read(fts)) != NULL) {
> + char *path = skip_dotslash(e->fts_path);
> switch (e->fts_info) {
> case FTS_NSOK:
> - /* handle rrdp .state files explicitly */
> - if (e->fts_parent->fts_pointer == RRDP_DIR &&
> - e->fts_level == 2 &&
> - strcmp(e->fts_name, ".state") == 0) {
> + if (filepath_exists(tree, path)) {
> e->fts_parent->fts_number++;
> - } else if (filepath_exists(tree, e->fts_path)) {
> + break;
> + }
> + if (e->fts_parent->fts_pointer == RRDP_DIR) {
> e->fts_parent->fts_number++;
> - } else if (e->fts_parent->fts_pointer == RRDP_DIR) {
> + /* handle rrdp .state files explicitly */
> + if (e->fts_level == 3 &&
> + strcmp(e->fts_name, ".state") == 0)
> + break;
> /* can't delete these extra files */
> - e->fts_parent->fts_number++;
> stats.extra_files++;
> if (verbose > 1)
> - logx("superfluous %s", e->fts_path);
> - } else {
> - if (e->fts_parent->fts_pointer == RSYNC_DIR) {
> - /* no need to keep rsync files */
> - stats.extra_files++;
> - if (verbose > 1)
> - logx("superfluous %s",
> - e->fts_path);
> - }
> - del = add_to_del(del, &delsz,
> - e->fts_path);
> + logx("superfluous %s", path);
> + break;
> + }
> + if (e->fts_parent->fts_pointer == RSYNC_DIR) {
> + /* no need to keep rsync files */
> + stats.extra_files++;
> + if (verbose > 1)
> + logx("superfluous %s", path);
> }
> + del = add_to_del(del, &delsz, path);
> break;
> case FTS_D:
> - if (e->fts_level == FTS_ROOTLEVEL) {
> - if (strcmp("rsync", e->fts_path) == 0)
> + if (e->fts_level == 1) {
> + if (strcmp(".rsync", e->fts_name) == 0)
> e->fts_pointer = RSYNC_DIR;
> - else if (strcmp("rrdp", e->fts_path) == 0)
> + else if (strcmp(".rrdp", e->fts_name) == 0)
> e->fts_pointer = RRDP_DIR;
> else
> e->fts_pointer = BASE_DIR;
> @@ -1375,8 +1390,8 @@ repo_cleanup(struct filepath_tree *tree)
> * only if rrdp is active.
> */
> if (e->fts_pointer == RRDP_DIR && !noop && rrdpon &&
> - e->fts_level == 1) {
> - if (!rrdp_is_active(e->fts_path))
> + e->fts_level == 2) {
> + if (!rrdp_is_active(path))
> e->fts_pointer = NULL;
> }
> break;
> @@ -1384,26 +1399,26 @@ repo_cleanup(struct filepath_tree *tree)
> if (e->fts_level == FTS_ROOTLEVEL)
> break;
> if (e->fts_number == 0)
> - dir = add_to_del(dir, &dirsz, e->fts_path);
> + dir = add_to_del(dir, &dirsz, path);
>
> e->fts_parent->fts_number += e->fts_number;
> break;
> case FTS_SL:
> case FTS_SLNONE:
> - warnx("symlink %s", e->fts_path);
> - del = add_to_del(del, &delsz, e->fts_path);
> + warnx("symlink %s", path);
> + del = add_to_del(del, &delsz, path);
> break;
> case FTS_NS:
> case FTS_ERR:
> if (e->fts_errno == ENOENT &&
> e->fts_level == FTS_ROOTLEVEL)
> continue;
> - warnx("fts_read %s: %s", e->fts_path,
> + warnx("fts_read %s: %s", path,
> strerror(e->fts_errno));
> break;
> default:
> - warnx("unhandled[%x] %s", e->fts_info,
> - e->fts_path);
> + warnx("fts_read %s: unhandled[%x]", path,
> + e->fts_info);
> break;
> }
>
>
