From 5cf4e49f3750a985d785c6bd461104582ddc215d Mon Sep 17 00:00:00 2001 From: Ryanmello07 <67509637+Ryanmello07@users.noreply.github.com> Date: Tue, 11 Aug 2026 10:29:22 +0100 Subject: [PATCH] fix(egress-prober): fail loudly when the server rejects the byJwt parseByJwtClientId proves the token decodes and carries a client_id. It does not prove the server accepts it, and the gap between those two is an outage mode the prober cannot currently detect at all. The byJwt authenticates only the provider tunnel. The due queue, attempt reporting and pin fetch all authenticate with the operator secret. So when the jwt stops being accepted -- it expires (jwt.expiryDuration is 24h), or it predates a claim the server has since begun enforcing -- every operator-secret path keeps working. The prober fetches its batch, opens tunnels that carry nothing, and dutifully reports each provider as no_consensus with ok=0/N. Nothing in that output says "credential". It says "the entire fleet is bad", which is convincing enough that on one deployment it ran 8 hours and 870 consecutive failures before anyone suspected the token. The single-shot path (-interval 0) already treats "submitted nothing, recorded failures" as fatal; the long-running loop has no equivalent alarm, which is exactly why it was the loop that stayed silent. checkCredential makes one authenticated request at startup, beside the confinement self-check and for the same reason: a fault invisible at runtime has to be caught here or not at all. It separates three outcomes, and the separation is the point: 200 accepted, log and continue 401/403 rejected -- exit non-zero, naming the remedy 404/5xx/error inconclusive -- WARN and continue The third case is deliberate. A 404 from a server predating the endpoint, or an unreachable host, says nothing about the credential, and stopping on it would convert "we could not ask" into a new outage. This mirrors ingest, which keeps ErrUnauthorized distinct from ErrDueUnsupported so a bad secret cannot hide behind "old server" -- the same principle, applied to the other credential. Also documents -interval honestly: it is a sleep AFTER a pass, so the cycle is pass-duration + interval and throughput is due-limit/(pass-duration+interval). Read as "a pass every interval" it overstates throughput by the pass duration -- at 500 providers per ~30m pass with -interval 1h that is ~390/hour, two thirds of it idle. Tests cover all four outcomes, including that a rejection does NOT also satisfy errCredentialUnverified -- if it did, main's switch would downgrade it to a warning and the prober would start on a dead credential, reintroducing the bug. Verified by simulation: making 401 return errCredentialUnverified fails TestCheckCredentialRejects401. Full suite green under the CI command (go test -race -count=1 -timeout 20m ./...). --- cmd/egress-prober/credential_test.go | 82 ++++++++++++++++++++++++++++ cmd/egress-prober/main.go | 74 ++++++++++++++++++++++++- 2 files changed, 155 insertions(+), 1 deletion(-) create mode 100644 cmd/egress-prober/credential_test.go diff --git a/cmd/egress-prober/credential_test.go b/cmd/egress-prober/credential_test.go new file mode 100644 index 0000000..4c062c2 --- /dev/null +++ b/cmd/egress-prober/credential_test.go @@ -0,0 +1,82 @@ +package main + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "testing" +) + +// The credential check exists to separate three outcomes the prober previously +// could not tell apart: accepted, refused, and unknown. Only the middle one may +// stop the process, so each is asserted on its own. + +func TestCheckCredentialAcceptsOK(t *testing.T) { + var gotAuth string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotAuth = r.Header.Get("Authorization") + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + if err := checkCredential(context.Background(), srv.Client(), srv.URL, "the-jwt"); err != nil { + t.Fatalf("checkCredential: %s, want nil", err) + } + // Without the header the endpoint would answer 401 for a reason that has + // nothing to do with the credential, and the check would condemn a working + // token. + if want := "Bearer the-jwt"; gotAuth != want { + t.Errorf("Authorization = %q, want %q", gotAuth, want) + } +} + +func TestCheckCredentialRejects401(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + })) + defer srv.Close() + + err := checkCredential(context.Background(), srv.Client(), srv.URL, "stale-jwt") + if !errors.Is(err, errCredentialRejected) { + t.Fatalf("checkCredential = %v, want errCredentialRejected", err) + } + // A rejection that also satisfied errCredentialUnverified would be + // downgraded to a warning by main's switch, which is the whole failure this + // change exists to prevent. + if errors.Is(err, errCredentialUnverified) { + t.Errorf("a rejection also matched errCredentialUnverified; main would let the prober start") + } +} + +// A server that predates the endpoint answers 404. That says nothing about the +// credential, so it must not stop the prober -- the same posture ingest takes +// when the due endpoint is missing. +func TestCheckCredentialTreats404AsUnverified(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + })) + defer srv.Close() + + err := checkCredential(context.Background(), srv.Client(), srv.URL, "fine-jwt") + if !errors.Is(err, errCredentialUnverified) { + t.Fatalf("checkCredential = %v, want errCredentialUnverified", err) + } + if errors.Is(err, errCredentialRejected) { + t.Errorf("404 was reported as a rejected credential; an old server would stop the prober") + } +} + +func TestCheckCredentialTreatsTransportErrorAsUnverified(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})) + url := srv.URL + srv.Close() // nothing is listening now + + err := checkCredential(context.Background(), http.DefaultClient, url, "fine-jwt") + if !errors.Is(err, errCredentialUnverified) { + t.Fatalf("checkCredential = %v, want errCredentialUnverified", err) + } + if errors.Is(err, errCredentialRejected) { + t.Errorf("an unreachable server was reported as a rejected credential") + } +} diff --git a/cmd/egress-prober/main.go b/cmd/egress-prober/main.go index 05b70ac..2e3f2c5 100644 --- a/cmd/egress-prober/main.go +++ b/cmd/egress-prober/main.go @@ -66,7 +66,7 @@ func main() { operatorSecret := flag.String("operator-secret", "", "ingest secret, must match ingest_secret in provider_egress.yml; prefer the UR_OPERATOR_SECRET env var, which keeps it out of ps (required)") concurrency := flag.Int("concurrency", 4, "max simultaneous provider tunnels") cacheTTL := flag.Duration("cache-ttl", 24*time.Hour, "do not re-probe a provider within this window. Only applies to the enumeration fallback used against a server with no due endpoint; when the server supplies the due list it owns the schedule") - interval := flag.Duration("interval", time.Hour, "sleep between passes; 0 runs a single pass and exits") + interval := flag.Duration("interval", time.Hour, "sleep AFTER a pass finishes, not a fixed period: the cycle is pass-duration + interval, so throughput is -due-limit / (pass-duration + interval) rather than -due-limit per interval. A 500-provider pass taking ~30m at -interval 1h yields ~390/hour with the prober idle two thirds of every cycle. Size it against how long a pass actually takes; 0 runs a single pass and exits") probeTimeout := flag.Duration("probe-timeout", 60*time.Second, "per-provider probe timeout, and the per-source deadline within a probe") skipConfinementCheck := flag.Bool("skip-confinement-check", false, "DANGEROUS: start even if this host can reach a geolocation api directly. Only for a one-shot manual probe on a host you know is not the operator's; a direct lookup records the OPERATOR's location for the provider and exposes the operator's address to the api") confinementTimeout := flag.Duration("confinement-timeout", 3*time.Second, "per-address deadline for the startup confinement self-check; a timeout counts as blocked. Must be at least "+confinement.MinTimeout.String()) @@ -256,6 +256,21 @@ func main() { log.Fatalf("parse by-jwt client id: %s", err) } + // The credential self-check runs before the first tunnel, for the same + // reason the confinement self-check runs before the first request: a fault + // the prober cannot detect at runtime has to be caught here or not at all. + switch err := checkCredential(ctx, http.DefaultClient, *apiURL, *byJwt); { + case err == nil: + log.Printf("egress-prober: credential self-check passed: the server accepts the byJwt") + case errors.Is(err, errCredentialRejected): + log.Printf("egress-prober: credential self-check FAILED: %s", err) + log.Printf("egress-prober: the byJwt parses but the server refuses it -- it has expired, or it predates a claim the server now enforces. Probes would not fail loudly: the operator-secret calls would keep working while every tunnel carried nothing and every provider was recorded as no_consensus. Mint a fresh network client jwt (POST /network/auth-client) and set UR_PROBER_BY_JWT to it.") + os.Exit(1) + default: + log.Printf("egress-prober: WARNING credential self-check inconclusive: %s", err) + log.Printf("egress-prober: continuing, because being unable to check is not evidence the credential is bad. If every provider comes back no_consensus with ok=0/N, suspect the byJwt first.") + } + // Pins is deliberately NOT set here: it is fetched from the server below // and read afresh on every tunnel Open, so an hourly refresh reaches the // next provider rather than only the next process. @@ -1205,6 +1220,63 @@ func findProvidersAtLocation(ctx context.Context, client *http.Client, apiURL st // is the authority that already validated it when minting a session from it), // and the claim is type-switched rather than unmarshaled into a typed struct, // since some issuers emit client_id as something other than a bare string. +// errCredentialRejected reports that the server refused the prober's byJwt. +// +// This is kept distinct from "could not check" for the same reason +// ingest.ErrUnauthorized is kept distinct from ingest.ErrDueUnsupported: a +// rejected credential is a broken deployment, and anything that lets it look +// like an ordinary runtime hiccup hides the fault behind work that appears to +// continue. +var errCredentialRejected = errors.New("egress-prober: the server rejected the prober's byJwt") + +// errCredentialUnverified reports that the check could not reach a verdict -- +// an old server without the endpoint, a transport error, a 5xx. It is NOT a +// claim that the credential is bad, so it must never stop the prober: doing so +// would turn "we could not ask" into a new outage of its own. +var errCredentialUnverified = errors.New("egress-prober: could not verify the byJwt") + +// checkCredential asks the server whether it ACCEPTS the byJwt, which is not +// what parseByJwtClientId establishes -- that only proves the token decodes and +// carries a client_id. +// +// The gap between those two is a real outage mode, not a hypothetical. A token +// that parses perfectly is still refused once it expires (jwt.expiryDuration is +// 24h) or once the server begins enforcing a claim the token predates, and the +// prober has no way to notice: the byJwt authenticates only the provider +// tunnel, while the due queue, attempt reporting and pin fetch all authenticate +// with the operator secret. So every one of those keeps working, the prober +// goes on reporting attempts and looks healthy, and the tunnel silently carries +// nothing -- every probe fails no_consensus with ok=0/N. +// +// That reads as "the whole fleet is bad", which is a convincing wrong answer. +// On one deployment it ran 8 hours and 870 consecutive failures before the +// credential was suspected. One request at startup turns that into a message. +func checkCredential(ctx context.Context, client *http.Client, apiURL string, byJwt string) error { + url := strings.TrimRight(apiURL, "/") + "/network/clients" + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + if err != nil { + return fmt.Errorf("%w: %w", errCredentialUnverified, err) + } + req.Header.Set("Authorization", "Bearer "+byJwt) + + resp, err := client.Do(req) + if err != nil { + return fmt.Errorf("%w: %w", errCredentialUnverified, err) + } + defer resp.Body.Close() + + switch resp.StatusCode { + case http.StatusOK: + return nil + case http.StatusUnauthorized, http.StatusForbidden: + return errCredentialRejected + default: + // Everything else -- notably 404 from a server that predates this + // endpoint -- is inconclusive by design. + return fmt.Errorf("%w: status %d", errCredentialUnverified, resp.StatusCode) + } +} + func parseByJwtClientId(byJwt string) (connect.Id, error) { claims := gojwt.MapClaims{} if _, _, err := gojwt.NewParser().ParseUnverified(byJwt, claims); err != nil {