Re: rpki-client: refactor parse_load_crl_from_mft()
On Tue, Feb 21, 2023 at 11:10:33AM +0100, Theo Buehler wrote: > > 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@ > > I would prefer to keep the refactor/cleanup separate from the behavior > change. This change is incomplete and not easy to follow. For example, > there's no point in passing down location to proc_parser_mft_pre() > anymore. Sure. Just flip the input to always go for both dirs and do the cleanup afterwards. I just don't want to add more on top of this (like registring CRLs in a different way). This should be a seperate step. -- :wq Claudio
Re: rpki-client: refactor parse_load_crl_from_mft()
> 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@ I would prefer to keep the refactor/cleanup separate from the behavior change. This change is incomplete and not easy to follow. For example, there's no point in passing down location to proc_parser_mft_pre() anymore.
Re: rpki-client: refactor parse_load_crl_from_mft()
On Sun, Feb 19, 2023 at 10:36:28AM +, 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 - 1.82
> +++ parser.c 19 Feb 2023 10:17:09 -
> @@ -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)
>
rpki-client: refactor parse_load_crl_from_mft()
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?
Kind regards,
Job
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.c6 Jan 2023 16:06:43 - 1.82
+++ parser.c19 Feb 2023 10:17:09 -
@@ -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;
+ 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)) {
