ipn/localapi,client/local: honour Retry-After on cert rate-limit (#20315)

* ipn/localapi,ipnlocal,feature/acme,client/local: honour Retry-After on cert rate-limit

serveCert now responds with 429 + Retry-After when the underlying ACME
error is a rate limit, instead of a generic 500. client/local surfaces
this as a typed RateLimitedError with the parsed hint so callers can
back off intelligently.

Updates tailscale/corp#42164

Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>

* tsweb,feature/acme,ipn/localapi,ipnlocal: generalise cert error → HTTP mapping via tsweb.HTTPStatuser

Introduces a tsweb.HTTPStatuser interface, any error can implement
to describe its intended HTTP response (code, message, headers).
Moves CertRateLimitedError from ipnlocal to feature/acme where it's
constructed, and it now uses HTTPStatuser to return 429 + Retry-After.

serveCert now checks for tsweb.HTTPStatuser rather than the specific
error type, so it no longer needs to know about the ACME rate-limit
type.

Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>

---------

Signed-off-by: chaosinthecrd <tom@tmlabs.co.uk>

client/local/cert.go

@@ -10,13 +10,55 @@ import (
 	"crypto/tls"
 	"errors"
 	"fmt"
+	"net/http"
 	"net/url"
+	"strconv"
 	"strings"
 	"time"
 
 	"go4.org/mem"
 )
 
+// rateLimitedError is returned from cert-fetching methods when the
+// upstream ACME CA reported a rate limit. Callers should unpack it via
+// [RateLimitRetryAfter].
+type rateLimitedError struct {
+	retryAfter time.Duration
+	underlying error
+}
+
+func (e rateLimitedError) Error() string { return e.underlying.Error() }
+func (e rateLimitedError) Unwrap() error { return e.underlying }
+
+// RateLimitRetryAfter reports whether err was a rate-limit failure from
+// the upstream ACME CA and, if so, returns the CA's suggested wait
+// (zero if none was provided).
+func RateLimitRetryAfter(err error) (retryAfter time.Duration, ok bool) {
+	var rl rateLimitedError
+	if errors.As(err, &rl) {
+		return rl.retryAfter, true
+	}
+	return 0, false
+}
+
+// retryAfterFromHeader parses a Retry-After header, matching the
+// delta-seconds + HTTP-date pattern in tempfork/acme/http.go.
+func retryAfterFromHeader(h http.Header) time.Duration {
+	v := h.Get("Retry-After")
+	if i, err := strconv.Atoi(v); err == nil {
+		return time.Duration(i) * time.Second
+	}
+	t, err := http.ParseTime(v)
+	if err != nil {
+		return 0
+	}
+	d := time.Until(t)
+	if d < 0 {
+		return 0
+	}
+	return d
+}
+
 // SetDNS adds a DNS TXT record for the given domain name, containing
 // the provided TXT value. The intended use case is answering
 // LetsEncrypt/ACME dns-01 challenges.
@@ -43,6 +85,8 @@ func (lc *Client) SetDNS(ctx context.Context, name, value string) error {
 //
 // It returns a cached certificate from disk if it's still valid.
 //
+// Rate-limit failures can be identified via [RateLimitRetryAfter].
+//
 // Deprecated: use [Client.CertPair].
 func CertPair(ctx context.Context, domain string) (certPEM, keyPEM []byte, err error) {
 	return defaultClient.CertPair(ctx, domain)
@@ -52,6 +96,8 @@ func CertPair(ctx context.Context, domain string) (certPEM, keyPEM []byte, err e
 //
 // It returns a cached certificate from disk if it's still valid.
 //
+// Rate-limit failures can be identified via [RateLimitRetryAfter].
+//
 // API maturity: this is considered a stable API.
 func (lc *Client) CertPair(ctx context.Context, domain string) (certPEM, keyPEM []byte, err error) {
 	return lc.CertPairWithValidity(ctx, domain, 0)
@@ -65,10 +111,18 @@ func (lc *Client) CertPair(ctx context.Context, domain string) (certPEM, keyPEM
 // least the given duration, if permitted by the CA. If the certificate is
 // valid, but for less than minValidity, it will be synchronously renewed.
 //
+// Rate-limit failures can be identified via [RateLimitRetryAfter].
+//
 // API maturity: this is considered a stable API.
 func (lc *Client) CertPairWithValidity(ctx context.Context, domain string, minValidity time.Duration) (certPEM, keyPEM []byte, err error) {
 	res, err := lc.send(ctx, "GET", fmt.Sprintf("/localapi/v0/cert/%s?type=pair&min_validity=%s", domain, minValidity), 200, nil)
 	if err != nil {
+		if hse, ok := errors.AsType[httpStatusError](err); ok && hse.HTTPStatus == http.StatusTooManyRequests {
+			return nil, nil, rateLimitedError{
+				retryAfter: retryAfterFromHeader(hse.Header),
+				underlying: err,
+			}
+		}
 		return nil, nil, err
 	}
 	// with ?type=pair, the response PEM is first the one private

client/local/local.go

@@ -280,7 +280,7 @@ func (lc *Client) sendWithHeaders(
 	}
 	if res.StatusCode != wantStatus {
 		err = fmt.Errorf("%v: %s", res.Status, bytes.TrimSpace(slurp))
-		return nil, nil, httpStatusError{bestError(err, slurp), res.StatusCode}
+		return nil, nil, httpStatusError{bestError(err, slurp), res.StatusCode, res.Header}
 	}
 	return slurp, res.Header, nil
 }
@@ -288,6 +288,7 @@ func (lc *Client) sendWithHeaders(
 type httpStatusError struct {
 	error
 	HTTPStatus int
+	Header     http.Header
 }
 
 func (lc *Client) get200(ctx context.Context, path string) ([]byte, error) {

feature/acme/acme.go

@@ -28,6 +28,7 @@ import (
 	"tailscale.com/ipn/ipnext"
 	"tailscale.com/ipn/ipnlocal"
 	"tailscale.com/syncs"
+	xacme "tailscale.com/tempfork/acme"
 	"tailscale.com/tsconst"
 	"tailscale.com/types/logger"
 	"tailscale.com/util/clientmetric"
@@ -171,7 +172,16 @@ func getCertPEMHook(ctx context.Context, b *ipnlocal.LocalBackend, domain string
 	if err != nil {
 		return nil, err
 	}
-	return e.getCertPEMWithValidity(ctx, b, domain, minValidity)
+	pair, err := e.getCertPEMWithValidity(ctx, b, domain, minValidity)
+	if err != nil {
+		if ae, ok := errors.AsType[*xacme.Error](err); ok {
+			if d, ok := xacme.RateLimit(ae); ok {
+				return nil, certRateLimitedError{retryAfter: d, underlying: err}
+			}
+		}
+		return nil, err
+	}
+	return pair, nil
 }
 
 func getACMETLSALPNCertHook(b *ipnlocal.LocalBackend, hi *tls.ClientHelloInfo) (*tls.Certificate, bool) {

feature/acme/cert.go

@@ -27,7 +27,7 @@ import (
 	"tailscale.com/ipn"
 	"tailscale.com/ipn/ipnlocal"
 	"tailscale.com/tailcfg"
-	"tailscale.com/tempfork/acme"
+	xacme "tailscale.com/tempfork/acme"
 	"tailscale.com/types/logger"
 	"tailscale.com/util/mak"
 	"tailscale.com/util/set"
@@ -49,7 +49,7 @@ var acmeDebug = envknob.RegisterBool("TS_DEBUG_ACME")
 // and an ACME order is actively waiting on that challenge for
 // hi.ServerName.
 func (e *extension) getACMETLSALPNCert(hi *tls.ClientHelloInfo) (cert *tls.Certificate, ok bool) {
-	if hi == nil || hi.ServerName == "" || !slices.Contains(hi.SupportedProtos, acme.ALPNProto) {
+	if hi == nil || hi.ServerName == "" || !slices.Contains(hi.SupportedProtos, xacme.ALPNProto) {
 		return nil, false
 	}
 	cert, ok = e.pendingACMETLSALPNCerts.Load(hi.ServerName)
@@ -62,7 +62,7 @@ func (e *extension) getACMETLSALPNProto(hi *tls.ClientHelloInfo) (proto string,
 	if _, ok := e.getACMETLSALPNCert(hi); !ok {
 		return "", false
 	}
-	return acme.ALPNProto, true
+	return xacme.ALPNProto, true
 }
 
 // storeACMETLSALPNCert publishes cert to Serve TLS handshakes for domain
@@ -252,7 +252,7 @@ func (e *extension) isBYOFunnelDomain(b *ipnlocal.LocalBackend, domain string) b
 	return b.HasFunnelForHostPort(domain, 443)
 }
 
-func challengeByType(challenges []*acme.Challenge, typ string) *acme.Challenge {
+func challengeByType(challenges []*xacme.Challenge, typ string) *xacme.Challenge {
 	for _, ch := range challenges {
 		if ch.Type == typ {
 			return ch
@@ -349,9 +349,9 @@ var getCertPEM = func(ctx context.Context, e *extension, b *ipnlocal.LocalBacken
 	case err == nil:
 		// Great, already registered.
 		logf("already had ACME account.")
-	case err == acme.ErrNoAccount:
+	case err == xacme.ErrNoAccount:
-		a, err = ac.Register(ctx, new(acme.Account), acme.AcceptTOS)
+		a, err = ac.Register(ctx, new(xacme.Account), xacme.AcceptTOS)
-		if err == acme.ErrAccountAlreadyExists {
+		if err == xacme.ErrAccountAlreadyExists {
 			// Potential race. Double check.
 			a, err = ac.GetReg(ctx, "" /* pre-RFC param */)
 		}
@@ -364,7 +364,7 @@ var getCertPEM = func(ctx context.Context, e *extension, b *ipnlocal.LocalBacken
 		return nil, fmt.Errorf("acme.GetReg: %w", err)
 
 	}
-	if a.Status != acme.StatusValid {
+	if a.Status != xacme.StatusValid {
 		return nil, fmt.Errorf("unexpected ACME account status %q", a.Status)
 	}
 
@@ -374,11 +374,11 @@ var getCertPEM = func(ctx context.Context, e *extension, b *ipnlocal.LocalBacken
 	// Note that this order extension will fail renewals if the ACME account key has changed
 	// since the last issuance, see
 	// https://github.com/tailscale/tailscale/issues/18251
-	var opts []acme.OrderOption
+	var opts []xacme.OrderOption
 	if previous != nil && !envknob.Bool("TS_DEBUG_ACME_FORCE_RENEWAL") {
 		prevCrt, err := parseCertificate(previous)
 		if err == nil {
-			opts = append(opts, acme.WithOrderReplacesCert(prevCrt))
+			opts = append(opts, xacme.WithOrderReplacesCert(prevCrt))
 		}
 	}
 
@@ -415,14 +415,14 @@ type acmeCertIssueArgs struct {
 	logf          logger.Logf        // logs ACME progress and failures
 	traceACME     func(any)          // optional hook for logging ACME messages
 	domain        string             // certificate domain being issued
-	opts          []acme.OrderOption // ACME order options
+	opts          []xacme.OrderOption // ACME order options
 	challengeType acmeChallengeType  // challenge type to fulfill
 }
 
 func (args acmeCertIssueArgs) baseDomain() string { return strings.TrimPrefix(args.domain, "*.") }
 func (args acmeCertIssueArgs) isWildcard() bool   { return isWildcardDomain(args.domain) }
 
-func (e *extension) issueACMECert(ctx context.Context, b *ipnlocal.LocalBackend, ac *acme.Client, args acmeCertIssueArgs) (ret *ipnlocal.TLSCertKeyPair, err error) {
+func (e *extension) issueACMECert(ctx context.Context, b *ipnlocal.LocalBackend, ac *xacme.Client, args acmeCertIssueArgs) (ret *ipnlocal.TLSCertKeyPair, err error) {
 	if args.traceACME == nil {
 		args.traceACME = func(any) {}
 	}
@@ -451,14 +451,14 @@ func (e *extension) issueACMECert(ctx context.Context, b *ipnlocal.LocalBackend,
 	}
 
 	// For wildcards, we need to authorize both the wildcard and base domain.
-	var authzIDs []acme.AuthzID
+	var authzIDs []xacme.AuthzID
 	if args.isWildcard() {
-		authzIDs = []acme.AuthzID{
+		authzIDs = []xacme.AuthzID{
 			{Type: "dns", Value: args.domain},
 			{Type: "dns", Value: args.baseDomain()},
 		}
 	} else {
-		authzIDs = []acme.AuthzID{{Type: "dns", Value: args.domain}}
+		authzIDs = []xacme.AuthzID{{Type: "dns", Value: args.domain}}
 	}
 	order, err := ac.AuthorizeOrder(ctx, authzIDs, args.opts...)
 	if err != nil {
@@ -504,7 +504,7 @@ func (e *extension) issueACMECert(ctx context.Context, b *ipnlocal.LocalBackend,
 		if ctx.Err() != nil {
 			return nil, ctx.Err()
 		}
-		if oe, ok := err.(*acme.OrderError); ok {
+		if oe, ok := err.(*xacme.OrderError); ok {
 			args.logf("acme: WaitOrder: OrderError status %q", oe.Status)
 		} else {
 			args.logf("acme: WaitOrder error: %v", err)
@@ -550,7 +550,7 @@ func (e *extension) issueACMECert(ctx context.Context, b *ipnlocal.LocalBackend,
 	return &ipnlocal.TLSCertKeyPair{CertPEM: certPEM.Bytes(), KeyPEM: privPEM.Bytes()}, nil
 }
 
-func fulfillACMEDNS01Challenge(ctx context.Context, b *ipnlocal.LocalBackend, ac *acme.Client, az *acme.Authorization, logf logger.Logf, traceACME func(any)) error {
+func fulfillACMEDNS01Challenge(ctx context.Context, b *ipnlocal.LocalBackend, ac *xacme.Client, az *xacme.Authorization, logf logger.Logf, traceACME func(any)) error {
 	for _, ch := range az.Challenges {
 		if ch.Type != string(acmeChallengeDNS01) {
 			continue

feature/acme/cert_test.go

@@ -31,7 +31,7 @@ import (
 	"tailscale.com/ipn/ipnlocal/ipnlocaltest"
 	"tailscale.com/ipn/store/mem"
 	"tailscale.com/tailcfg"
-	"tailscale.com/tempfork/acme"
+	xacme "tailscale.com/tempfork/acme"
 	"tailscale.com/tsconst"
 	"tailscale.com/tstest"
 	"tailscale.com/types/logger"
@@ -263,7 +263,7 @@ func TestACMETLSALPNCertHook(t *testing.T) {
 
 	if got, ok := b.ForTest().GetACMETLSALPNCert(&tls.ClientHelloInfo{
 		ServerName:      "example.com",
-		SupportedProtos: []string{acme.ALPNProto},
+		SupportedProtos: []string{xacme.ALPNProto},
 	}); !ok || got != cert {
 		t.Fatalf("getACMETLSALPNCert = %v, %v; want stored cert, true", got, ok)
 	}
@@ -275,7 +275,7 @@ func TestACMETLSALPNCertHook(t *testing.T) {
 	}
 	if _, ok := b.ForTest().GetACMETLSALPNCert(&tls.ClientHelloInfo{
 		ServerName:      "other.example.com",
-		SupportedProtos: []string{acme.ALPNProto},
+		SupportedProtos: []string{xacme.ALPNProto},
 	}); ok {
 		t.Fatal("getACMETLSALPNCert for other name = ok, want false")
 	}
@@ -283,7 +283,7 @@ func TestACMETLSALPNCertHook(t *testing.T) {
 	otherBackend := ipnlocaltest.NewBackend(t)
 	if _, ok := otherBackend.ForTest().GetACMETLSALPNCert(&tls.ClientHelloInfo{
 		ServerName:      "example.com",
-		SupportedProtos: []string{acme.ALPNProto},
+		SupportedProtos: []string{xacme.ALPNProto},
 	}); ok {
 		t.Fatal("getACMETLSALPNCert on different LocalBackend = ok, want false")
 	}

feature/acme/certstore.go

@@ -32,7 +32,7 @@ import (
 	"tailscale.com/ipn/store"
 	"tailscale.com/ipn/store/mem"
 	"tailscale.com/net/bakedroots"
-	"tailscale.com/tempfork/acme"
+	xacme "tailscale.com/tempfork/acme"
 	"tailscale.com/util/testenv"
 	"tailscale.com/version"
 	"tailscale.com/version/distro"
@@ -354,7 +354,7 @@ func acmeKey(cs certStore) (crypto.Signer, error) {
 	return privKey, nil
 }
 
-func acmeClient(cs certStore) (*acme.Client, error) {
+func acmeClient(cs certStore) (*xacme.Client, error) {
 	key, err := acmeKey(cs)
 	if err != nil {
 		return nil, fmt.Errorf("acmeKey: %w", err)
@@ -362,7 +362,7 @@ func acmeClient(cs certStore) (*acme.Client, error) {
 	// Note: if we add support for additional ACME providers (other than
 	// LetsEncrypt), we should make sure that they support ARI extension (see
 	// shouldStartDomainRenewalARI).
-	return &acme.Client{
+	return &xacme.Client{
 		Key:          key,
 		UserAgent:    "tailscaled/" + version.Long(),
 		DirectoryURL: envknob.String("TS_DEBUG_ACME_DIRECTORY_URL"),
@@ -440,5 +440,5 @@ func validateLeaf(leaf *x509.Certificate, intermediates *x509.CertPool, domain s
 }
 
 func isDefaultDirectoryURL(u string) bool {
-	return u == "" || u == acme.LetsEncryptURL
+	return u == "" || u == xacme.LetsEncryptURL
 }

feature/acme/errors.go

@@ -0,0 +1,36 @@
+// Copyright (c) Tailscale Inc & contributors
+// SPDX-License-Identifier: BSD-3-Clause
+
+package acme
+
+import (
+	"net/http"
+	"strconv"
+	"time"
+
+	"tailscale.com/tsweb"
+)
+
+// certRateLimitedError is returned when the upstream ACME CA rate-limited
+// the issuance. It exists so cert-fetching failures can surface as HTTP
+// 429 responses via [tsweb.HTTPStatuser].
+type certRateLimitedError struct {
+	retryAfter time.Duration
+	underlying error
+}
+
+func (e certRateLimitedError) Error() string { return e.underlying.Error() }
+func (e certRateLimitedError) Unwrap() error { return e.underlying }
+
+// HTTPStatus implements [tsweb.HTTPStatuser].
+func (e certRateLimitedError) HTTPStatus() tsweb.HTTPError {
+	h := http.Header{}
+	if e.retryAfter > 0 {
+		h.Set("Retry-After", strconv.Itoa(int(e.retryAfter.Seconds())))
+	}
+	return tsweb.HTTPError{
+		Code:   http.StatusTooManyRequests,
+		Msg:    e.Error(),
+		Header: h,
+	}
+}

ipn/ipnlocal/cert.go

@@ -90,6 +90,10 @@ func (b *LocalBackend) GetCertPEM(ctx context.Context, domain string) (*TLSCertK
 //   - A bring-your-own Funnel domain referenced by the local serve
 //     config (e.g., "foo.com" when ServeConfig.AllowFunnel has
 //     "foo.com:443").
+//
+// On an ACME rate-limit failure the returned error implements
+// [tsweb.HTTPStatuser] with a 429 + Retry-After response; other errors
+// are returned unchanged.
 func (b *LocalBackend) GetCertPEMWithValidity(ctx context.Context, domain string, minValidity time.Duration) (*TLSCertKeyPair, error) {
 	if f, ok := HookGetCertPEM.GetOk(); ok {
 		return f(ctx, b, domain, minValidity)

ipn/localapi/cert.go

@@ -6,12 +6,15 @@
 package localapi
 
 import (
+	"errors"
 	"fmt"
+	"maps"
 	"net/http"
 	"strings"
 	"time"
 
 	"tailscale.com/ipn/ipnlocal"
+	"tailscale.com/tsweb"
 )
 
 func init() {
@@ -39,11 +42,15 @@ func (h *Handler) serveCert(w http.ResponseWriter, r *http.Request) {
 	}
 	pair, err := h.b.GetCertPEMWithValidity(r.Context(), domain, minValidity)
 	if err != nil {
-		// TODO(bradfitz): 500 is a little lazy here. The errors returned from
+		if hs, ok := errors.AsType[tsweb.HTTPStatuser](err); ok {
-		// GetCertPEM (and everywhere) should carry info to get whether
+			resp := hs.HTTPStatus()
-		// they're 400 vs 403 vs 500 at minimum. And then we should have helpers
+			maps.Copy(w.Header(), resp.Header)
-		// (in tsweb probably) to return an error that looks at the error value
+			http.Error(w, resp.Msg, resp.Code)
-		// to determine the HTTP status code.
+			return
+		}
+		// TODO(bradfitz): 500 is a little lazy. Other errors from GetCertPEM
+		// should also implement tsweb.HTTPStatuser (400 vs 403 vs 500 vs …)
+		// rather than falling through here.
 		http.Error(w, fmt.Sprint(err), 500)
 		return
 	}

tsnet/depaware.txt

@@ -232,7 +232,7 @@ tailscale.com/tsnet dependencies: (generated by github.com/tailscale/depaware)
         tailscale.com/tstime                                         from tailscale.com/control/controlclient+
         tailscale.com/tstime/mono                                    from tailscale.com/net/tstun+
         tailscale.com/tstime/rate                                    from tailscale.com/wgengine/filter
- LDWI    tailscale.com/tsweb                                          from tailscale.com/util/eventbus+
+        tailscale.com/tsweb                                          from tailscale.com/util/eventbus+
         tailscale.com/tsweb/varz                                     from tailscale.com/tsweb+
         tailscale.com/types/appctype                                 from tailscale.com/ipn/ipnlocal+
         tailscale.com/types/bools                                    from tailscale.com/tsnet+
@@ -500,7 +500,7 @@ tailscale.com/tsnet dependencies: (generated by github.com/tailscale/depaware)
         internal/nettrace                                            from net+
         internal/oserror                                             from internal/syscall/windows+
         internal/poll                                                from net+
- LDWI    internal/profile                                             from net/http/pprof
+        internal/profile                                             from net/http/pprof
         internal/profilerecord                                       from runtime+
         internal/race                                                from internal/poll+
         internal/reflectlite                                         from context+
@@ -552,7 +552,7 @@ tailscale.com/tsnet dependencies: (generated by github.com/tailscale/depaware)
         net/http/internal                                            from net/http+
         net/http/internal/ascii                                      from net/http+
         net/http/internal/httpcommon                                 from net/http
- LDWI    net/http/pprof                                               from tailscale.com/ipn/localapi+
+        net/http/pprof                                               from tailscale.com/ipn/localapi+
         net/netip                                                    from crypto/x509+
         net/textproto                                                from github.com/coder/websocket+
         net/url                                                      from crypto/x509+
@@ -567,7 +567,7 @@ tailscale.com/tsnet dependencies: (generated by github.com/tailscale/depaware)
         runtime                                                      from crypto/internal/fips140+
         runtime/debug                                                from github.com/klauspost/compress/zstd+
         runtime/pprof                                                from net/http/pprof+
- LDWI    runtime/trace                                                from net/http/pprof
+        runtime/trace                                                from net/http/pprof
         slices                                                       from crypto/tls+
         sort                                                         from compress/flate+
         strconv                                                      from compress/flate+

tsweb/tsweb.go

@@ -784,6 +784,12 @@ func (h errorHandler) handleError(w http.ResponseWriter, r *http.Request, lw *lo
 	// Extract a presentable, loggable error.
 	var hOK bool
 	hErr, hAsOK := errors.AsType[HTTPError](err)
+	if !hAsOK {
+		if hs, ok := errors.AsType[HTTPStatuser](err); ok {
+			hErr = hs.HTTPStatus()
+			hAsOK = true
+		}
+	}
 	if hAsOK {
 		hOK = true
 		if hErr.Code == 0 {
@@ -919,6 +925,15 @@ func WriteHTTPError(w http.ResponseWriter, r *http.Request, e HTTPError) {
 	}
 }
 
+// HTTPStatuser is an optional interface implemented by errors that
+// carry an intended HTTP response. Handlers translating errors to
+// HTTP should honour the returned HTTPError rather than defaulting to
+// 500.
+type HTTPStatuser interface {
+	error
+	HTTPStatus() HTTPError
+}
+
 // HTTPError is an error with embedded HTTP response information.
 //
 // It is the error type to be (optionally) used by Handler.ServeHTTPReturn.