feat: attempt to check ARI unless explicitly disabled

[beautifulentropy]

With draft-ietf-acme-ari now in IETF Working Group Last Call—the final step before becoming an RFC—it is reasonable to enable ARI checks by default.

[ldez]

I don't merge the PR because I have doubts about the impact of this PR: I don't see any problem with doing this with LE, but I wonder about the ACME servers. 🤔

I'm torn between having a better default value, but that can introduce a change that may upset a large number of users, or just keeping the current option.

[beautifulentropy]

I don't merge the PR because I have doubts about the impact of this PR: I don't see any problem with doing this with LE, but I wonder about the ACME servers. 🤔

I'm torn between having a better default value, but that can introduce a change that may upset a large number of users, or just keeping the current option.

Oh, interesting! I figured that this was reasonably safe given that use of ARI is actually gated on whether or not the CA includes a renewalInfo field in its directory object per draft-ietf-acme-ari-06#section-3. I'd be curious to hear your concerns @ldez.

[ldez]

You are right, I missed that api.ErrNoARI is managed but there are several elements related to the flag:

lego/cmd/cmd_renew.go

Lines 212 to 217 in 19b535c

	if ctx.Bool(flgARIEnable) {
	request.ReplacesCertID, err = certificate.MakeARICertID(cert)
	if err != nil {
	log.Fatalf("Error while construction the ARI CertID for domain %s\n\t%v", domain, err)
	}
	}

lego/cmd/cmd_renew.go

Lines 282 to 287 in 19b535c

	if ctx.Bool(flgARIEnable) {
	request.ReplacesCertID, err = certificate.MakeARICertID(cert)
	if err != nil {
	log.Fatalf("Error while construction the ARI CertID for domain %s\n\t%v", domain, err)
	}
	}

I don't want to have the same problem as before: #2138

[beautifulentropy]

If we change these to log.Warnf and continue using the default renewal logic would that be sufficient?

[ldez]

🤔 maybe we can move the call to MakeARICertID inside the first if

lego/cmd/cmd_renew.go

Lines 154 to 164 in 19b535c

	if ctx.Bool(flgARIEnable) {
	ariRenewalTime = getARIRenewalTime(ctx, cert, domain, client)
	if ariRenewalTime != nil {
	now := time.Now().UTC()
	// Figure out if we need to sleep before renewing.
	if ariRenewalTime.After(now) {
	log.Infof("[%s] Sleeping %s until renewal time %s", domain, ariRenewalTime.Sub(now), ariRenewalTime)
	time.Sleep(ariRenewalTime.Sub(now))
	}
	}
	}

lego/cmd/cmd_renew.go

Lines 253 to 263 in 19b535c

	if ctx.Bool(flgARIEnable) {
	ariRenewalTime = getARIRenewalTime(ctx, cert, domain, client)
	if ariRenewalTime != nil {
	now := time.Now().UTC()
	// Figure out if we need to sleep before renewing.
	if ariRenewalTime.After(now) {
	log.Infof("[%s] Sleeping %s until renewal time %s", domain, ariRenewalTime.Sub(now), ariRenewalTime)
	time.Sleep(ariRenewalTime.Sub(now))
	}
	}
	}

[ldez]

something like that:

	var replacesCertID string
	var ariRenewalTime *time.Time
	if ctx.Bool(flgARIEnable) {
		ariRenewalTime = getARIRenewalTime(ctx, cert, domain, client)
		if ariRenewalTime != nil {
			now := time.Now().UTC()
			// Figure out if we need to sleep before renewing.
			if ariRenewalTime.After(now) {
				log.Infof("[%s] Sleeping %s until renewal time %s", domain, ariRenewalTime.Sub(now), ariRenewalTime)
				time.Sleep(ariRenewalTime.Sub(now))
			}
		}

		replacesCertID, err = certificate.MakeARICertID(cert)
		if err != nil {
			log.Fatalf("Error while construction the ARI CertID for domain %s\n\t%v", domain, err)
		}
	}

	if ariRenewalTime == nil && !needRenewal(cert, domain, ctx.Int(flgDays)) {
		return nil
	}
...

	if replacesCertID != "" {
		request.ReplacesCertID = replacesCertID
	}

[ldez]

LGTM