On Wed, Feb 09, 2022 at 02:59:41PM +0100, Theo Buehler wrote:
> We should not use CRLs if now isn't between thisUpdate and nextUpdate.
> This also ensures that thisUpdate <= nextUpdate. While the verifier will
> catch all this, doing this early will often remove one of the two
> possible choices of a CRL to use for a MFT since these are typically
> short-lived. While there, let's simplify the exit of crl_parse().
>
> I was pondering whether we should mark such CRLs stale and add them to
> the statistics as we do for MFTs, but I think it's not super
> interesting.
I'm not fully convinced by this. Mainly not returning a CRL will alter the
error reported by X509_verify_cert() and make it more confusing
(especially since the warnings in crl_parse are only if verbose > 1.
I would not mind to do this check in parse_load_crl_from_mft().
Another thing we should consider is that the CRL used to validate the MFT
should also be the one used to validate the rest. This is currently not
enforced.
Will need to think about this more.
> Index: crl.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/crl.c,v
> retrieving revision 1.13
> diff -u -p -r1.13 crl.c
> --- crl.c 8 Feb 2022 14:53:03 -0000 1.13
> +++ crl.c 9 Feb 2022 06:23:30 -0000
> @@ -34,7 +34,7 @@ crl_parse(const char *fn, const unsigned
> struct crl *crl;
> const ASN1_TIME *at;
> struct tm issued_tm, expires_tm;
> - int rc = 0;
> + time_t now;
>
> /* just fail for empty buffers, the warning was printed elsewhere */
> if (der == NULL)
> @@ -66,7 +66,6 @@ crl_parse(const char *fn, const unsigned
> if ((crl->issued = mktime(&issued_tm)) == -1)
> errx(1, "%s: mktime failed", fn);
>
> - /* extract expire time for later use */
> at = X509_CRL_get0_nextUpdate(crl->x509_crl);
> if (at == NULL) {
> warnx("%s: X509_CRL_get0_nextUpdate failed", fn);
> @@ -80,13 +79,25 @@ crl_parse(const char *fn, const unsigned
> if ((crl->expires = mktime(&expires_tm)) == -1)
> errx(1, "%s: mktime failed", fn);
>
> - rc = 1;
> - out:
> - if (rc == 0) {
> - crl_free(crl);
> - crl = NULL;
> + now = time(NULL);
> + if (now < crl->issued) {
> + if (verbose > 1)
> + warnx("%s: crl not yet valid %s", fn,
> + time2str(crl->issued));
> + goto out;
> + }
> + if (now > crl->expires) {
> + if (verbose > 1)
> + warnx("%s: crl expired on %s", fn,
> + time2str(crl->expires));
> + goto out;
> }
> +
> return crl;
> +
> + out:
> + crl_free(crl);
> + return NULL;
> }
>
> static inline int
> Index: extern.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.118
> diff -u -p -r1.118 extern.h
> --- extern.h 8 Feb 2022 14:53:03 -0000 1.118
> +++ extern.h 9 Feb 2022 06:21:49 -0000
> @@ -502,6 +502,7 @@ void entity_free(struct entity *);
> void entity_read_req(struct ibuf *, struct entity *);
> void entityq_flush(struct entityq *, struct repo *);
> void proc_parser(int) __attribute__((noreturn));
> +char *time2str(time_t);
>
> /* Rsync-specific. */
>
> Index: parser.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
> retrieving revision 1.63
> diff -u -p -r1.63 parser.c
> --- parser.c 8 Feb 2022 14:53:03 -0000 1.63
> +++ parser.c 9 Feb 2022 06:19:40 -0000
> @@ -94,7 +94,7 @@ repo_add(unsigned int id, char *path, ch
> errx(1, "repository already added: id %d, %s", id, path);
> }
>
> -static char *
> +char *
> time2str(time_t t)
> {
> static char buf[64];
>
--
:wq Claudio
