On Wed, Feb 09, 2022 at 08:14:55PM +0100, Claudio Jeker wrote:
> 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.
I agree. Let me drop this patch for now. I just can't shake that gut
feeling that what we're doing with CRLs and MFTs is not quite right.
Sadly, I don't have a good idea or a good reason.
>
> > 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
