From: Job Snijders <job@openbsd.org>
Subject: Re: acme-client(8): Adapt renewal calculation for shortlived certificates.
To: tech <tech@openbsd.org>
Date: Tue, 16 Sep 2025 23:27:21 +0000

On Tue, Sep 16, 2025 at 05:31:23PM +0200, Florian Obser wrote:
>  /*
> - * Convert the X509's expiration time into a time_t value.
> + * Convert the X509's notAfter time into a time_t value.
>   */
>  static time_t
> -X509expires(X509 *x)
> +X509notafter(X509 *x)
>  {
>  	ASN1_TIME	*atim;
>  	struct tm	 t;
> @@ -58,6 +56,30 @@ X509expires(X509 *x)
>  	return timegm(&t);
>  }
>  
> +/*
> + * Convert the X509's notBefore time into a time_t value.
> + */
> +static time_t
> +X509notbefore(X509 *x)
> +{
> +	ASN1_TIME	*atim;
> +	struct tm	 t;
> +
> +	if ((atim = X509_getm_notBefore(x)) == NULL) {
> +		warnx("missing notBefore");
> +		return -1;
> +	}

As the objective isn't to change anything inside the X509 structure *x
points to, in X509notafter(() and X509notbefore(), instead of the
X509_getm_* variants, I'd maybe use X509_get0_notAfter() and
X509_get0_notBefore() which both return a pointer to a const ASN1_TIME
object.

A stylistic suggestion: rename X509notafter() along the lines of
x509_get_notafter() to distinguish these getters function names a bit
more from libcrypto API.

> +	memset(&t, 0, sizeof(t));
> +
> +	if (!ASN1_TIME_to_tm(atim, &t)) {
> +		warnx("invalid ASN1_TIME");
> +		return -1;
> +	}
> +
> +	return timegm(&t);
> +}

Maybe it's cleaner to not conflate the error indicator and return value,
perhaps use this function signature instead:

int
x509_get_notafter(X509 *x, time_t *t) ...

>  int
>  revokeproc(int fd, const char *certfile, int force,
>      int revocate, const char *const *alts, size_t altsz)
> @@ -70,7 +92,8 @@ revokeproc(int fd, const char *certfile, int force,
>  	X509				*x = NULL;
>  	long				 lval;
>  	enum revokeop			 op, rop;
> -	time_t				 t;
> +	time_t				 notafter, notbefore, cert_validity;
> +	time_t				 remaining_validity, renew_allow;
>  	size_t				 j;
>  
>  	/*
> @@ -125,8 +148,13 @@ revokeproc(int fd, const char *certfile, int force,
>  
>  	/* Read out the expiration date. */
>  
> -	if ((t = X509expires(x)) == -1) {
> -		warnx("X509expires");
> +	if ((notafter = X509notafter(x)) == -1) {
> +		warnx("X509notafter");
> +		goto out;
> +	}
> +
> +	if ((notbefore = X509notbefore(x)) == -1) {
> +		warnx("X509notbefore");
>  		goto out;
>  	}
>  
> @@ -252,14 +280,35 @@ revokeproc(int fd, const char *certfile, int force,
>  		goto out;
>  	}
>  
> -	rop = time(NULL) >= (t - RENEW_ALLOW) ? REVOKE_EXP : REVOKE_OK;
> +	cert_validity = notafter - notbefore;
> +
> +	if (cert_validity < 0) {

maybe 'cert_validity <= 0' ?