From: Theo Buehler <tb@theobuehler.org>
Subject: Re: rpki-client: refactor manifest thisUpdate / nextUpdate parsing
To: Job Snijders <job@openbsd.org>
Cc: tech@openbsd.org
Date: Sun, 4 Feb 2024 00:53:12 +0100

On Sat, Feb 03, 2024 at 10:49:25PM +0000, Job Snijders wrote:
> When d2i_Manifest() was introduced in in 2022, I think we got the check
> whether the thisUpdate/nextUpdate fields are ASN1_GENERALIZEDTIME for
> free with it.

Right if it was a UTCTime it would not parse.

> So it seems that instead of the bespoke mft_parse_time() +
> generalizedtime_to_tm() dance, x509_get_time() can just be used.

Yes.

> @@ -301,8 +247,15 @@ mft_parse_econtent(const unsigned char *
>  	if (p->res->seqnum == NULL)
>  		goto out;
>  
> -	if (!mft_parse_time(mft->thisUpdate, mft->nextUpdate, p))
> +	if (!x509_get_time(mft->thisUpdate, &p->res->thisupdate)) {
> +		warn("%s: parsing manifest thisUpdate failed", p->fn);
>  		goto out;
> +	}
> +
> +	if (!x509_get_time(mft->nextUpdate, &p->res->nextupdate)) {
> +		warn("%s: parsing manifest nextUpdate failed", p->fn);
> +		goto out;
> +	}

I think you should keep the nextupdate >= thisupdate check here for
filemode. I think it is unclear if "MUST be later" means > or >=, so I
would keep the check as it is.

>  
>  	if (OBJ_obj2nid(mft->fileHashAlg) != NID_sha256) {
>  		warnx("%s: RFC 6486 section 4.2.1: fileHashAlg: "
> Index: parser.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
> diff -u -p -r1.128 parser.c
> --- parser.c	3 Feb 2024 14:30:47 -0000	1.128
> +++ parser.c	3 Feb 2024 22:48:24 -0000
> @@ -309,6 +309,11 @@ proc_parser_mft_pre(struct entity *entp,
>  	mft->repoid = entp->repoid;
>  	mft->talid = a->cert->talid;
>  
> +	if (mft->thisupdate >= mft->nextupdate) {
> +		warnx("%s: bad update interval", file);
> +		goto err;
> +	}
> +

Given that equality isn't explicitly excluded, I think this is redundant
with the existing now >= thisupdate and nextupdate >= now checks, so I
would not add this here.

>  	now = get_current_time();
>  	/* check that now is not before from */
>  	if (now < mft->thisupdate) {
>