On Sun, Feb 19, 2023 at 10:36:28AM +0000, Job Snijders wrote:
> Hi,
>
> I wasn't entirely happy about how parse_load_crl_from_mft() behaved and
> refactored the function.
>
> The good: if the MFT at hand was located in DIR_TEMP and no matching CRL
> could be found in DIR_TEMP, it would additionally attempt to find a CRL
> in DIR_VALID.
> The bad: if the MFT at hand was located in DIR_VALID, no attempt would
> be made to search for a matching CRL in DIR_TEMP; resulting in less
> opportunity to potentially salvage a broken situation at a future point
> in time with the help of locally cached artefacts.
>
> If the following 5 commands are run (before and after applying the below
> changeset), one can observe that with this diff rpki-client's behaviour
> becomes more idempotent.
>
> rm -rf /var/cache/rpki-client/{*,.rrdp,.rsync}
>
> rpki-client -t /etc/rpki/lacnic.tal 2>&1 | fgrep
> 6QvnUnEXe5JTf7VhQHnUFRwdzeEpbF4rt3b5PLrvdeyy
>
> ls -lahtr
> /var/cache/rpki-client/{.rsync,.,.rrdp/*}/rpki-repo.registro.br/repo/6QvnUnEXe5JTf7VhQHnUFRwdzeEpbF4rt3b5PLrvdeyy/0/
>
> rpki-client -t /etc/rpki/lacnic.tal 2>&1 | fgrep
> 6QvnUnEXe5JTf7VhQHnUFRwdzeEpbF4rt3b5PLrvdeyy
>
> ls -lahtr
> /var/cache/rpki-client/{.rsync,.,.rrdp/*}/rpki-repo.registro.br/repo/6QvnUnEXe5JTf7VhQHnUFRwdzeEpbF4rt3b5PLrvdeyy/0/
>
> With the diff applied, the second invocation of rpki-client won't delete
> CDD9973303E25E7554D25F5703FB347389D59326.crl & friends from DIR_TEMP;
> without this diff, we lose sight of some files. Losing the files hampers
> our ability to (re)construct the publication point if a future RRDP
> delta publish the correct ROAs (because by then we'd be missing the
> CRL).
>
> Since SHA256 hashes are used to confirm the correct object is loaded, it
> doesn't matter whether the CRL comes from DIR_VALID, DIR_TEMP, USB
> stick, or pigeon carrier.
>
> OK?
Fine with this change. As we discussed offlist this is the right approach
since the hash will protect us from loding a CRL that does not match with
the MFT.
One minor complaint below.
> Index: parser.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
> retrieving revision 1.82
> diff -u -p -r1.82 parser.c
> --- parser.c 6 Jan 2023 16:06:43 -0000 1.82
> +++ parser.c 19 Feb 2023 10:17:09 -0000
> @@ -210,43 +210,47 @@ proc_parser_mft_check(const char *fn, st
> }
>
> /*
> - * Load the correct CRL using the info from the MFT.
> + * Load the correct CRL using the SHA256 info from the MFT.
> + * Returns NULL if no valid matching CRL was found in either the staging area
> + * or the validated cache area.
> */
> static struct crl *
> -parse_load_crl_from_mft(struct entity *entp, struct mft *mft, enum location
> loc)
> +parse_load_crl_from_mft(struct entity *entp, struct mft *mft)
> {
> - struct crl *crl = NULL;
> - unsigned char *f = NULL;
> - char *fn = NULL;
> - size_t flen;
> + char *fn = NULL;
> + unsigned char *f = NULL;
> + struct crl *res = NULL;
Why did you rename *crl to *res? For me res is normally more like an
integer result. I would prefer if you keep that as crl.
Still OK claudio@
> + const enum location loc[2] = { DIR_TEMP, DIR_VALID };
> + size_t flen;
> + int i;
>
> - while (1) {
> - fn = parse_filepath(entp->repoid, entp->path, mft->crl, loc);
> + for (i = 0; i < 2; i++) {
> + fn = parse_filepath(entp->repoid, entp->path, mft->crl, loc[i]);
> if (fn == NULL)
> - goto next;
> + continue;
>
> f = load_file(fn, &flen);
> if (f == NULL && errno != ENOENT)
> warn("parse file %s", fn);
> - if (f == NULL)
> - goto next;
> - if (!valid_hash(f, flen, mft->crlhash, sizeof(mft->crlhash)))
> - goto next;
> - crl = crl_parse(fn, f, flen);
> + if (f == NULL) {
> + free(fn);
> + continue;
> + }
> +
> + if (valid_hash(f, flen, mft->crlhash, sizeof(mft->crlhash))) {
> + res = crl_parse(fn, f, flen);
> + break;
> + }
>
> -next:
> free(f);
> free(fn);
> f = NULL;
> fn = NULL;
> -
> - if (crl != NULL)
> - return crl;
> - if (loc == DIR_TEMP)
> - loc = DIR_VALID;
> - else
> - return NULL;
> }
> +
> + free(f);
> + free(fn);
> + return res;
> }
>
> /*
> @@ -268,7 +272,7 @@ proc_parser_mft_pre(char *file, const un
> *errstr = NULL;
> if ((mft = mft_parse(&x509, file, der, len)) == NULL)
> return NULL;
> - *crl = parse_load_crl_from_mft(entp, mft, loc);
> + *crl = parse_load_crl_from_mft(entp, mft);
>
> a = valid_ski_aki(file, &auths, mft->ski, mft->aki);
> if (!valid_x509(file, ctx, x509, a, *crl, errstr)) {
>
--
:wq Claudio
