Visitar URL original
fix(auth/http): bound one retry by an overall fetch deadline · modelcontextprotocol/registry@2fdcd62 · GitHub
Skip to content

Commit 2fdcd62

Browse files
committed
fix(auth/http): bound one retry by an overall fetch deadline
1 parent 17f440f commit 2fdcd62

4 files changed

Lines changed: 245 additions & 16 deletions

File tree

‎docs/modelcontextprotocol-io/authentication.mdx‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -346,8 +346,8 @@ The fetch is a plain HTTPS request with these properties:
346346
| Request | `GET https://<domain>/.well-known/mcp-registry-auth`, sent with `Accept: text/plain` and `User-Agent: mcp-registry/1.0` |
347347
| Status | `200 OK`. Redirects are not followed, so a `3xx` fails verification |
348348
| Body | one or more proof records, at most 4096 bytes in total. Use one record per line; surrounding whitespace is ignored |
349-
| Timeout | 10 seconds for each request attempt |
350-
| Retries | up to 3 attempts, 500 ms apart, when an attempt fails transiently (connection failure, `408`, `429`, `5xx`). Other `4xx` and `3xx` responses fail immediately |
349+
| Timeout | 15 seconds overall, including retries and backoff; each attempt is limited to 10 seconds or the remaining overall budget, whichever is shorter |
350+
| Retries | at most one retry after 500 ms for connection failures, `408`, `429`, or `5xx`, while time remains. Other `4xx`, `3xx`, invalid TLS certificates, blocked addresses, nonexistent domains, and oversized bodies fail immediately |
351351
| Origin | the hosted Registry runs on Google Cloud, so the request arrives from Google Cloud addresses rather than a fixed address you can allowlist per request |
352352

353353
Because the request originates from cloud infrastructure rather than a browser on a residential network, the usual causes of failure are edge rules that block it:
@@ -367,7 +367,7 @@ curl -sS -o - -w '\n%{http_code}\n' \
367367
https://example.com/.well-known/mcp-registry-auth
368368
```
369369

370-
The response body must contain the proof record printed by the login command (`v=MCPv1; k=...; p=...`). When the Registry cannot use the endpoint, the login error carries the Registry's own fetch failure, such as `dial tcp <ip>:443: i/o timeout`, `HTTP 403: failed to fetch key from ...`, or `HTTP 301: failed to fetch key from ...`. Transient fetch failures are retried up to 3 attempts before login fails, and the final error reports how many attempts were made. Allow plain HTTPS clients through for `/.well-known/mcp-registry-auth`, serve the path without redirects, then retry.
370+
The response body must contain the proof record printed by the login command (`v=MCPv1; k=...; p=...`). When the Registry cannot use the endpoint, the login error carries the Registry's own fetch failure, such as `dial tcp <ip>:443: i/o timeout`, `HTTP 403: failed to fetch key from ...`, or `HTTP 301: failed to fetch key from ...`. Transient fetch failures may be retried once within the 15-second overall budget. A retry failure reports the attempt count; cancellation or expiry while waiting for a retry reports that it was stopped. A first attempt that consumes its full 10 seconds leaves about 4.5 seconds for the retry after backoff. Allow plain HTTPS clients through for `/.well-known/mcp-registry-auth`, serve the path without redirects, then retry.
371371

372372
Addresses that resolve to loopback, private, link-local, or carrier-grade NAT ranges are refused by design, so the endpoint must be reachable at a public address.
373373

‎internal/api/handlers/v0/auth/http.go‎

Lines changed: 35 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@ package auth
22

33
import (
44
"context"
5+
"crypto/tls"
6+
"errors"
57
"fmt"
68
"io"
79
"net"
@@ -47,17 +49,24 @@ func NewDefaultHTTPKeyFetcher() *DefaultHTTPKeyFetcher {
4749
// attempt.
4850
const httpKeyFetchTimeout = 10 * time.Second
4951

52+
// httpKeyFetchOverallTimeout includes both attempts, response-body reads and
53+
// backoff. Keep the first attempt's existing budget without doubling the time
54+
// an unauthenticated request can hold resources when an endpoint stalls.
55+
const httpKeyFetchOverallTimeout = 15 * time.Second
56+
5057
// httpKeyFetchAttempts is the total number of attempts FetchKey makes before
5158
// giving up. Transient unreachability of a verification endpoint — a dial
5259
// timeout, a reset connection, a CDN edge answering 5xx — used to fail the
5360
// whole HTTP login on a single attempt (#1625), even though the endpoint
5461
// answers moments later. Permanent failures (client errors, redirects,
5562
// oversized bodies) are never retried and still fail on the first attempt.
56-
const httpKeyFetchAttempts = 3
63+
const httpKeyFetchAttempts = 2
5764

5865
// httpKeyFetchRetryBackoff is the delay between fetch attempts.
5966
const httpKeyFetchRetryBackoff = 500 * time.Millisecond
6067

68+
var errHTTPKeyAddressBlocked = errors.New("refusing to connect to private or loopback address")
69+
6170
// newHTTPKeyFetcherClient builds the HTTP client used to fetch a domain's
6271
// well-known verification key. The transport is a parameter so that tests can
6372
// exercise the production timeout and redirect policy against a local server.
@@ -126,7 +135,7 @@ func safeDialContext(ctx context.Context, network, addr string) (net.Conn, error
126135
lastErr = dialErr
127136
}
128137
if allBlocked {
129-
return nil, fmt.Errorf("dial %s: refusing to connect to private or loopback address", host)
138+
return nil, fmt.Errorf("dial %s: %w", host, errHTTPKeyAddressBlocked)
130139
}
131140
return nil, fmt.Errorf("dial %s: all resolved public addresses failed: %w", host, lastErr)
132141
}
@@ -205,6 +214,9 @@ func NewDefaultHTTPKeyFetcherWithClient(client *http.Client) *DefaultHTTPKeyFetc
205214
// fails, the returned error reports how many attempts were made so fetch
206215
// problems can be distinguished from key problems (#1625).
207216
func (f *DefaultHTTPKeyFetcher) FetchKey(ctx context.Context, domain string) (string, error) {
217+
ctx, cancel := context.WithTimeout(ctx, httpKeyFetchOverallTimeout)
218+
defer cancel()
219+
208220
var lastErr error
209221
for attempt := 1; attempt <= httpKeyFetchAttempts; attempt++ {
210222
if attempt > 1 {
@@ -216,13 +228,22 @@ func (f *DefaultHTTPKeyFetcher) FetchKey(ctx context.Context, domain string) (st
216228
case <-time.After(httpKeyFetchRetryBackoff):
217229
}
218230
}
231+
if err := ctx.Err(); err != nil {
232+
if lastErr != nil {
233+
return "", fmt.Errorf("%w (retry stopped: %w)", lastErr, err)
234+
}
235+
return "", fmt.Errorf("failed to fetch key: %w", err)
236+
}
219237

220238
key, retryable, err := f.fetchKeyOnce(ctx, domain)
221239
if err == nil {
222240
return key, nil
223241
}
224242
lastErr = err
225243
if !retryable {
244+
if attempt > 1 {
245+
return "", fmt.Errorf("%w (after %d attempts)", err, attempt)
246+
}
226247
return "", err
227248
}
228249
}
@@ -245,10 +266,9 @@ func (f *DefaultHTTPKeyFetcher) fetchKeyOnce(ctx context.Context, domain string)
245266

246267
resp, err := f.client.Do(req)
247268
if err != nil {
248-
// Transport-level failures — dial timeouts, resets, stalled TLS
249-
// handshakes — are exactly the transient unreachability reported in
250-
// #1625; retry them unless the caller's context is already done.
251-
return "", ctx.Err() == nil, fmt.Errorf("failed to fetch key: %w", err)
269+
// Retry connection failures while there is time left, but fail fast on
270+
// certificate verification, blocked addresses and nonexistent domains.
271+
return "", ctx.Err() == nil && isRetryableHTTPFetchError(err), fmt.Errorf("failed to fetch key: %w", err)
252272
}
253273
defer resp.Body.Close()
254274

@@ -270,6 +290,14 @@ func (f *DefaultHTTPKeyFetcher) fetchKeyOnce(ctx context.Context, domain string)
270290
return strings.TrimSpace(string(body)), false, nil
271291
}
272292

293+
func isRetryableHTTPFetchError(err error) bool {
294+
var certificateErr *tls.CertificateVerificationError
295+
var dnsErr *net.DNSError
296+
return !errors.Is(err, errHTTPKeyAddressBlocked) &&
297+
!errors.As(err, &certificateErr) &&
298+
(!errors.As(err, &dnsErr) || !dnsErr.IsNotFound)
299+
}
300+
273301
// isTransientHTTPStatus reports whether an unsuccessful response status
274302
// describes a momentary condition worth retrying: request timeouts, rate
275303
// limits, and server-side errors typically clear on their own (overloaded CDN
@@ -279,7 +307,7 @@ func (f *DefaultHTTPKeyFetcher) fetchKeyOnce(ctx context.Context, domain string)
279307
func isTransientHTTPStatus(status int) bool {
280308
return status == http.StatusRequestTimeout ||
281309
status == http.StatusTooManyRequests ||
282-
status >= 500
310+
(status >= 500 && status < 600)
283311
}
284312

285313
// HTTPAuthHandler handles HTTP-based authentication

‎internal/api/handlers/v0/auth/http_internal_test.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -336,18 +336,18 @@ func TestHTTPKeyFetcher_DoesNotRetryPermanentFailures(t *testing.T) {
336336
// attempts and report how many were made — #1625 asks for diagnostics that
337337
// distinguish fetch problems from key problems.
338338
func TestHTTPKeyFetcher_GivesUpAfterBoundedAttempts(t *testing.T) {
339-
srv, requests := newDropFirstConnServer(t, 3)
339+
srv, requests := newDropFirstConnServer(t, 2)
340340
defer srv.Close()
341341

342342
fetcher := newLocalTLSFetcher(srv)
343343
_, err := fetcher.FetchKey(context.Background(), internalTestDomain)
344344
if err == nil {
345345
t.Fatal("expected an error when every attempt drops the connection")
346346
}
347-
if got := atomic.LoadInt32(requests); got != 3 {
348-
t.Errorf("server saw %d requests, want 3 bounded attempts", got)
347+
if got := atomic.LoadInt32(requests); got != 2 {
348+
t.Errorf("server saw %d requests, want 2 bounded attempts", got)
349349
}
350-
if !strings.Contains(err.Error(), "3 attempts") {
350+
if !strings.Contains(err.Error(), "2 attempts") {
351351
t.Errorf("err = %v, want it to report the number of attempts", err)
352352
}
353353
}
@@ -364,7 +364,7 @@ func TestHTTPKeyFetcher_DoesNotRetryAfterContextCancellation(t *testing.T) {
364364
if _, err := fetcher.FetchKey(ctx, internalTestDomain); err == nil {
365365
t.Fatal("expected an error with a cancelled context")
366366
}
367-
if got := atomic.LoadInt32(requests); got > 1 {
368-
t.Errorf("server saw %d requests, want at most 1 (retries must stop once the context is done)", got)
367+
if got := atomic.LoadInt32(requests); got != 0 {
368+
t.Errorf("server saw %d requests, want 0 (a cancelled context must not start a request)", got)
369369
}
370370
}
Lines changed: 201 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,201 @@
1+
package auth
2+
3+
import (
4+
"context"
5+
"errors"
6+
"fmt"
7+
"net"
8+
"net/http"
9+
"net/http/httptest"
10+
"net/http/httptrace"
11+
"strings"
12+
"sync/atomic"
13+
"testing"
14+
"time"
15+
)
16+
17+
func TestHTTPKeyFetcherRetryCancellation(t *testing.T) {
18+
for _, duringBody := range []bool{false, true} {
19+
t.Run(fmt.Sprint(duringBody), func(t *testing.T) {
20+
var count atomic.Int32
21+
started := make(chan struct{})
22+
srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
23+
if count.Add(1) == 1 {
24+
close(started)
25+
}
26+
if duringBody {
27+
w.WriteHeader(http.StatusOK)
28+
w.(http.Flusher).Flush()
29+
<-r.Context().Done()
30+
} else {
31+
w.WriteHeader(http.StatusServiceUnavailable)
32+
}
33+
}))
34+
defer srv.Close()
35+
ctx, cancel := context.WithCancel(context.Background())
36+
defer cancel()
37+
done := make(chan error, 1)
38+
go func() { _, err := newLocalTLSFetcher(srv).FetchKey(ctx, internalTestDomain); done <- err }()
39+
<-started
40+
time.Sleep(50 * time.Millisecond)
41+
cancel()
42+
select {
43+
case err := <-done:
44+
if !errors.Is(err, context.Canceled) {
45+
t.Fatalf("error = %v", err)
46+
}
47+
case <-time.After(time.Second):
48+
t.Fatal("cancellation did not promptly stop fetch")
49+
}
50+
if count.Load() != 1 {
51+
t.Fatalf("requests = %d", count.Load())
52+
}
53+
})
54+
}
55+
}
56+
57+
func TestHTTPKeyFetcherRetryHonorsEarlierCallerDeadline(t *testing.T) {
58+
var count atomic.Int32
59+
srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
60+
count.Add(1)
61+
w.WriteHeader(http.StatusOK)
62+
w.(http.Flusher).Flush()
63+
<-r.Context().Done()
64+
}))
65+
defer srv.Close()
66+
ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond)
67+
defer cancel()
68+
started := time.Now()
69+
_, err := newLocalTLSFetcher(srv).FetchKey(ctx, internalTestDomain)
70+
if !errors.Is(err, context.DeadlineExceeded) || count.Load() != 1 || time.Since(started) > time.Second {
71+
t.Fatalf("err=%v requests=%d elapsed=%s", err, count.Load(), time.Since(started))
72+
}
73+
}
74+
75+
func TestHTTPKeyFetcherRetryRecoversAfterAttemptTimeout(t *testing.T) {
76+
var count atomic.Int32
77+
srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
78+
if count.Add(1) == 1 {
79+
<-r.Context().Done()
80+
return
81+
}
82+
_, _ = w.Write([]byte("recovered"))
83+
}))
84+
defer srv.Close()
85+
fetcher := newLocalTLSFetcher(srv)
86+
fetcher.client.Timeout = 200 * time.Millisecond
87+
key, err := fetcher.FetchKey(context.Background(), internalTestDomain)
88+
if err != nil || key != "recovered" || count.Load() != 2 {
89+
t.Fatalf("key=%q err=%v requests=%d", key, err, count.Load())
90+
}
91+
}
92+
93+
type failingKeyTransport struct {
94+
err error
95+
requests int
96+
}
97+
98+
func (t *failingKeyTransport) RoundTrip(_ *http.Request) (*http.Response, error) {
99+
t.requests++
100+
return nil, t.err
101+
}
102+
103+
func TestHTTPKeyFetcherRetryRejectsPermanentTransportFailures(t *testing.T) {
104+
for _, tc := range []struct {
105+
name string
106+
err error
107+
}{
108+
{"blocked-address", fmt.Errorf("dial example.com: %w", errHTTPKeyAddressBlocked)},
109+
{"nonexistent-domain", &net.DNSError{Err: "no such host", Name: internalTestDomain, IsNotFound: true}},
110+
} {
111+
t.Run(tc.name, func(t *testing.T) {
112+
transport := &failingKeyTransport{err: tc.err}
113+
fetcher := &DefaultHTTPKeyFetcher{client: newHTTPKeyFetcherClient(transport)}
114+
_, err := fetcher.FetchKey(context.Background(), internalTestDomain)
115+
if !errors.Is(err, tc.err) || transport.requests != 1 {
116+
t.Fatalf("err=%v requests=%d", err, transport.requests)
117+
}
118+
})
119+
}
120+
}
121+
122+
func TestHTTPKeyFetcherRetryBodyAndRedirectBoundaries(t *testing.T) {
123+
for _, scenario := range []string{"truncated", "oversized", "redirect-after-retry"} {
124+
t.Run(scenario, func(t *testing.T) {
125+
var count atomic.Int32
126+
var redirected atomic.Int32
127+
target := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { redirected.Add(1); w.WriteHeader(http.StatusOK) }))
128+
defer target.Close()
129+
srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
130+
attempt := count.Add(1)
131+
switch scenario {
132+
case "truncated":
133+
if attempt == 1 {
134+
w.Header().Set("Content-Length", "1000")
135+
_, _ = w.Write([]byte("partial"))
136+
return
137+
}
138+
_, _ = w.Write([]byte("complete"))
139+
case "oversized":
140+
_, _ = w.Write([]byte(strings.Repeat("x", MaxKeyResponseSize+1)))
141+
case "redirect-after-retry":
142+
if attempt == 1 {
143+
w.WriteHeader(http.StatusServiceUnavailable)
144+
return
145+
}
146+
w.Header().Set("Location", target.URL)
147+
w.WriteHeader(http.StatusFound)
148+
}
149+
}))
150+
defer srv.Close()
151+
key, err := newLocalTLSFetcher(srv).FetchKey(context.Background(), internalTestDomain)
152+
want := int32(2)
153+
if scenario == "truncated" {
154+
if err != nil || key != "complete" {
155+
t.Fatalf("key=%q err=%v", key, err)
156+
}
157+
} else if err == nil {
158+
t.Fatal("expected error")
159+
}
160+
if scenario == "oversized" {
161+
want = 1
162+
}
163+
if count.Load() != want || redirected.Load() != 0 {
164+
t.Fatalf("requests=%d redirected=%d", count.Load(), redirected.Load())
165+
}
166+
})
167+
}
168+
}
169+
170+
func TestHTTPKeyFetcherRetryTLSVerificationFailsClosed(t *testing.T) {
171+
var count atomic.Int32
172+
srv := httptest.NewTLSServer(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) { count.Add(1) }))
173+
defer srv.Close()
174+
transport := newLocalTLSTransport(srv)
175+
transport.TLSClientConfig.InsecureSkipVerify = false
176+
var handshakes atomic.Int32
177+
ctx := httptrace.WithClientTrace(context.Background(), &httptrace.ClientTrace{TLSHandshakeStart: func() { handshakes.Add(1) }})
178+
_, err := (&DefaultHTTPKeyFetcher{client: newHTTPKeyFetcherClient(transport)}).FetchKey(ctx, internalTestDomain)
179+
if err == nil || count.Load() != 0 || handshakes.Load() != 1 {
180+
t.Fatalf("err=%v HTTP requests=%d TLS handshakes=%d", err, count.Load(), handshakes.Load())
181+
}
182+
t.Logf("TLS rejection: %v", err)
183+
}
184+
185+
func TestHTTPKeyFetcherRetryProductionTimeBudget(t *testing.T) {
186+
var count atomic.Int32
187+
srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
188+
count.Add(1)
189+
w.WriteHeader(http.StatusOK)
190+
w.(http.Flusher).Flush()
191+
<-r.Context().Done()
192+
}))
193+
defer srv.Close()
194+
started := time.Now()
195+
_, err := newLocalTLSFetcher(srv).FetchKey(context.Background(), internalTestDomain)
196+
elapsed := time.Since(started)
197+
if !errors.Is(err, context.DeadlineExceeded) || count.Load() != 2 || elapsed < 14*time.Second || elapsed > 18*time.Second {
198+
t.Fatalf("err=%v requests=%d elapsed=%s", err, count.Load(), elapsed)
199+
}
200+
t.Logf("requests=%d elapsed=%s error=%v", count.Load(), elapsed, err)
201+
}

0 commit comments

Comments
 (0)