From 87b3d7b7e5c1232f319dd78f5d0feb3ab4e3eeef Mon Sep 17 00:00:00 2001 From: Tom Meadows Date: Wed, 8 Jul 2026 13:34:40 +0100 Subject: [PATCH] ipn/localapi,client/local: honour Retry-After on cert rate-limit (#20315) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 * 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 --------- Signed-off-by: chaosinthecrd --- client/local/cert.go | 54 +++++++++++++++++++++++++++++++++++++++ client/local/local.go | 3 ++- feature/acme/acme.go | 12 ++++++++- feature/acme/cert.go | 34 ++++++++++++------------ feature/acme/cert_test.go | 8 +++--- feature/acme/certstore.go | 8 +++--- feature/acme/errors.go | 36 ++++++++++++++++++++++++++ ipn/ipnlocal/cert.go | 4 +++ ipn/localapi/cert.go | 17 ++++++++---- tsnet/depaware.txt | 8 +++--- tsweb/tsweb.go | 15 +++++++++++ 11 files changed, 163 insertions(+), 36 deletions(-) create mode 100644 feature/acme/errors.go diff --git a/client/local/cert.go b/client/local/cert.go index 701bfe026..0ce56b74f 100644 --- a/client/local/cert.go +++ b/client/local/cert.go @@ -10,13 +10,55 @@ "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 diff --git a/client/local/local.go b/client/local/local.go index 72793c9df..e26ee3c32 100644 --- a/client/local/local.go +++ b/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) { diff --git a/feature/acme/acme.go b/feature/acme/acme.go index 9947bc7f1..a1f1537a7 100644 --- a/feature/acme/acme.go +++ b/feature/acme/acme.go @@ -28,6 +28,7 @@ "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) { diff --git a/feature/acme/cert.go b/feature/acme/cert.go index 2d8b89f9a..42b1472ff 100644 --- a/feature/acme/cert.go +++ b/feature/acme/cert.go @@ -27,7 +27,7 @@ "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 @@ // 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 @@ func (e *extension) domainRenewalTimeByARI(b *ipnlocal.LocalBackend, cs certStor case err == nil: // Great, already registered. logf("already had ACME account.") - case err == acme.ErrNoAccount: - a, err = ac.Register(ctx, new(acme.Account), acme.AcceptTOS) - if err == acme.ErrAccountAlreadyExists { + case err == xacme.ErrNoAccount: + a, err = ac.Register(ctx, new(xacme.Account), xacme.AcceptTOS) + if err == xacme.ErrAccountAlreadyExists { // Potential race. Double check. a, err = ac.GetReg(ctx, "" /* pre-RFC param */) } @@ -364,7 +364,7 @@ func (e *extension) domainRenewalTimeByARI(b *ipnlocal.LocalBackend, cs certStor 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 @@ func (e *extension) domainRenewalTimeByARI(b *ipnlocal.LocalBackend, cs certStor // 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 diff --git a/feature/acme/cert_test.go b/feature/acme/cert_test.go index bf7cdc330..c28a5e83c 100644 --- a/feature/acme/cert_test.go +++ b/feature/acme/cert_test.go @@ -31,7 +31,7 @@ "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") } diff --git a/feature/acme/certstore.go b/feature/acme/certstore.go index 8e62a2cfc..834bbc381 100644 --- a/feature/acme/certstore.go +++ b/feature/acme/certstore.go @@ -32,7 +32,7 @@ "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 } diff --git a/feature/acme/errors.go b/feature/acme/errors.go new file mode 100644 index 000000000..c77b38f3b --- /dev/null +++ b/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, + } +} diff --git a/ipn/ipnlocal/cert.go b/ipn/ipnlocal/cert.go index 247fc2568..ad36957a8 100644 --- a/ipn/ipnlocal/cert.go +++ b/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) diff --git a/ipn/localapi/cert.go b/ipn/localapi/cert.go index 95817f801..132f7bd1e 100644 --- a/ipn/localapi/cert.go +++ b/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 - // GetCertPEM (and everywhere) should carry info to get whether - // they're 400 vs 403 vs 500 at minimum. And then we should have helpers - // (in tsweb probably) to return an error that looks at the error value - // to determine the HTTP status code. + if hs, ok := errors.AsType[tsweb.HTTPStatuser](err); ok { + resp := hs.HTTPStatus() + maps.Copy(w.Header(), resp.Header) + http.Error(w, resp.Msg, resp.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 } diff --git a/tsnet/depaware.txt b/tsnet/depaware.txt index ee4c7b748..a553a9e93 100644 --- a/tsnet/depaware.txt +++ b/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+ diff --git a/tsweb/tsweb.go b/tsweb/tsweb.go index 101512b89..a391110ad 100644 --- a/tsweb/tsweb.go +++ b/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.