From fffa61ced9fcc618bc8172af6d31f41426f01577 Mon Sep 17 00:00:00 2001 From: agustin-conductor Date: Wed, 26 Aug 2026 17:48:05 -0300 Subject: [PATCH] fix: surface DPoP proof rejections instead of "unknown error" (CXP-1002) Okta answers a bad DPoP proof with a 400, a zero-length body, and the reason in a DPoP-scheme WWW-Authenticate header. The vendored SDK's CheckResponseForError reads that header only for 401/403 responses whose scheme is Bearer, then discards the decode failure on the empty body, so every such rejection reached the caller as "the API returned an unknown error" and failed the whole sync with no way to tell what happened. Confirmed live against a DPoP-enabled tenant. A replayed proof jti, an iat more than five minutes old, and an iat in the future all produce that exact shape; an unsolicited nonce is accepted, and no resource response ever offers a DPoP-Nonce, so the resource server issues no nonce challenges at all. oktaauth: - restate the WWW-Authenticate challenge as the error body the SDK expects, so it renders what Okta actually said. The challenge is passed through whole rather than parsed into auth-params: RFC 9110 allows several challenges in one header, attributing a param to the right scheme needs a real parser, and quoting the header drops nothing and cannot misattribute one scheme's error to another. Bounded to client errors with a genuinely empty body, so a real body is never replaced -- including a body that failed partway through being read, whose bytes are reported rather than mistaken for an empty body. - retry once with a fresh proof when Okta reports a replayed proof jti; uhttp's transport retries a failed request with its headers untouched, so a stale-connection retry resends the same proof. Matched on the full DPoP-specific phrase, not on "already been used" alone, which another scheme's challenge in the same header could carry. A generic invalid_dpop_proof is not retried, since a skewed clock would only triple the traffic. - log any DPoP-related 4xx at Warn with the full challenge, before the SDK discards it, logarithmically sampled with a total_occurrences field: a skewed clock is not retried, so an unsampled line would fire once per request for a whole sync - guard the retry drain against a nil response body. net/http always sets one, but io.Copy on a nil ReadCloser panics and the two neighbouring helpers already check it. - match a nonce challenge on 400 as well as 401, and in the JSON-body shape as well as the header, with a bounded retry loop. Defensive: unreachable on the tenants observed, but correct per RFC 9449 and consistent with the token endpoint's existing handling connector: - getError now names the HTTP status, excerpts an undecodable body, and carries a grpc code derived from that status. Its five call sites return its error verbatim, so an empty body surfaced as a bare "unexpected end of JSON input" with no status and no prefix, and an unparseable body reached the sync as codes.Unknown while a parseable one became PermissionDenied for the same upstream failure. #200 rewrote handleOktaResponseError but left getError untouched. - the event feed no longer returns the SDK error bare, so it picks up a grpc code and the status line like every other call site. #200 did not touch event_log.go. Both truncation paths back up to a rune boundary rather than cutting at a byte offset, so an excerpt reaching an error message or a log field stays valid UTF-8. Status codes and grpc codes for everything routed through handleOktaResponseError, including 429, come from #200's GrpcCodeFromHTTPStatus fallback; no duplicate handling is added here. Validated with four clean full syncs against a live tenant (119 actions, no errors) plus unit coverage of each path using the captured challenge strings. The panic guard, the partial-read fix, and both rune-boundary cuts are verified by mutation. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/connector/event_log.go | 4 +- pkg/connector/get_error_test.go | 123 +++++ pkg/connector/helpers.go | 46 +- pkg/oktaauth/nonce_challenge_test.go | 654 +++++++++++++++++++++++++++ pkg/oktaauth/round_tripper.go | 272 ++++++++++- 5 files changed, 1083 insertions(+), 16 deletions(-) create mode 100644 pkg/connector/get_error_test.go create mode 100644 pkg/oktaauth/nonce_challenge_test.go diff --git a/pkg/connector/event_log.go b/pkg/connector/event_log.go index 36e16e4c..390aab11 100644 --- a/pkg/connector/event_log.go +++ b/pkg/connector/event_log.go @@ -73,7 +73,9 @@ func (connector *Okta) ListEvents( logs, resp, err := connector.client.LogEvent.GetLogs(ctx, qp) if err != nil { - return nil, nil, nil, err + // Route through the shared handler like every other call site; bare, this + // returned an SDK error carrying no grpc code, no status, and no prefix. + return nil, nil, nil, fmt.Errorf("okta-connectorv2: failed to list system log events: %w", handleOktaResponseError(resp, err)) } // MJP each log is not guaranteed to result in a v2.Event anymore, but it's still likely? diff --git a/pkg/connector/get_error_test.go b/pkg/connector/get_error_test.go new file mode 100644 index 00000000..2b9f54dc --- /dev/null +++ b/pkg/connector/get_error_test.go @@ -0,0 +1,123 @@ +package connector + +import ( + "io" + "net/http" + "strings" + "testing" + "unicode/utf8" + + "github.com/okta/okta-sdk-golang/v2/okta" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +// getError's five call sites all return its error verbatim, so it has to carry +// the status and the connector prefix itself. An empty body used to surface as a +// bare "unexpected end of JSON input". +func TestGetError(t *testing.T) { + t.Parallel() + + oktaResp := func(statusCode int, body string) *okta.Response { + return &okta.Response{Response: &http.Response{ + StatusCode: statusCode, + Status: http.StatusText(statusCode), + Body: io.NopCloser(strings.NewReader(body)), + }} + } + + t.Run("empty body reports the status", func(t *testing.T) { + t.Parallel() + _, err := getError(oktaResp(http.StatusBadRequest, "")) + if err == nil { + t.Fatal("expected an error") + } + for _, want := range []string{"okta-connectorv2:", http.StatusText(http.StatusBadRequest)} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error = %q, want it to contain %q", err, want) + } + } + }) + + t.Run("non-JSON body is excerpted, not dropped", func(t *testing.T) { + t.Parallel() + _, err := getError(oktaResp(http.StatusBadGateway, "gateway")) + if err == nil { + t.Fatal("expected an error") + } + if !strings.Contains(err.Error(), "gateway") { + t.Errorf("error = %q, want the body excerpt included", err) + } + }) + + t.Run("decodable body still parses", func(t *testing.T) { + t.Parallel() + got, err := getError(oktaResp(http.StatusForbidden, `{"errorCode":"E0000006","errorSummary":"denied"}`)) + if err != nil { + t.Fatalf("getError: %v", err) + } + if got.ErrorCode != AccessDeniedErrorCode { + t.Errorf("ErrorCode = %q, want %q", got.ErrorCode, AccessDeniedErrorCode) + } + }) +} + +// A fixed byte offset can split a multi-byte rune; the excerpt lands in an error +// message and in log fields, so it has to stay valid UTF-8. +func TestBodyExcerpt_TruncatesOnRuneBoundary(t *testing.T) { + t.Parallel() + + // A three-byte rune so the byte limit does not divide evenly and the cut lands + // mid-rune. Two-byte runes would tile it exactly and prove nothing. + body := strings.Repeat("€", errorBodyExcerptLimit) + got := bodyExcerpt([]byte(body)) + + if !utf8.ValidString(got) { + t.Errorf("excerpt is not valid UTF-8: %q", got) + } + if !strings.HasSuffix(got, "...") { + t.Errorf("excerpt = %q, want it marked as truncated", got) + } + if short := bodyExcerpt([]byte("€")); short != "€" { + t.Errorf("short body = %q, want it returned whole", short) + } +} + +// Both getError paths must classify by HTTP status, so the same upstream failure +// gets the same gRPC code whether or not Okta sent a parseable body. The five call +// sites return this error verbatim, so an unparseable body used to reach the sync +// as codes.Unknown while a parseable one became PermissionDenied. +func TestGetError_ClassifiesByHTTPStatus(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + statusCode int + want codes.Code + }{ + {name: "forbidden", statusCode: http.StatusForbidden, want: codes.PermissionDenied}, + {name: "rate limited", statusCode: http.StatusTooManyRequests, want: codes.Unavailable}, + {name: "not found", statusCode: http.StatusNotFound, want: codes.NotFound}, + {name: "bad request", statusCode: http.StatusBadRequest, want: codes.InvalidArgument}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + resp := &okta.Response{Response: &http.Response{ + StatusCode: tc.statusCode, + Status: http.StatusText(tc.statusCode), + Body: io.NopCloser(strings.NewReader("not json")), + }} + _, err := getError(resp) + if status.Code(err) != tc.want { + t.Errorf("code = %s, want %s (error: %v)", status.Code(err), tc.want, err) + } + // The message still has to carry the prefix and status for the logs. + for _, sub := range []string{"okta-connectorv2:", http.StatusText(tc.statusCode)} { + if !strings.Contains(err.Error(), sub) { + t.Errorf("error = %q, want it to contain %q", err, sub) + } + } + }) + } +} diff --git a/pkg/connector/helpers.go b/pkg/connector/helpers.go index 3e3789ca..3f1b9220 100644 --- a/pkg/connector/helpers.go +++ b/pkg/connector/helpers.go @@ -8,6 +8,7 @@ import ( "io" "net/url" "strings" + "unicode/utf8" "github.com/conductorone/baton-sdk/pkg/pagination" "github.com/conductorone/baton-sdk/pkg/uhttp" @@ -103,21 +104,41 @@ func responseToContext(token *pagination.Token, resp *okta.Response) (*responseC }, nil } +// errorBodyExcerptLimit caps how much of an unparseable error body is echoed +// back into the returned error. +const errorBodyExcerptLimit = 200 + func getError(response *okta.Response) (okta.Error, error) { var errOkta okta.Error bytes, err := io.ReadAll(response.Body) if err != nil { - return okta.Error{}, err + return okta.Error{}, bodyReadError(response, "read error body", err) } + // An empty or non-JSON body used to surface as a bare "unexpected end of JSON + // input" with no status code, which made an empty 400 indistinguishable from + // an empty 403 in logs. err = json.Unmarshal(bytes, &errOkta) if err != nil { - return okta.Error{}, err + return okta.Error{}, bodyReadError(response, fmt.Sprintf("unparseable error body %q", bodyExcerpt(bytes)), err) } return errOkta, nil } +// bodyReadError builds the error getError's callers return verbatim. The gRPC code +// comes from the HTTP status so that an unreadable body and a readable one produce +// the same classification for the same upstream failure -- a parseable body reaches +// handleOktaResponseError and gets a code, and without this an unparseable one +// would surface as codes.Unknown. +func bodyReadError(response *okta.Response, what string, err error) error { + return uhttp.WrapErrors( + uhttp.GrpcCodeFromHTTPStatus(response.StatusCode), + fmt.Sprintf("okta-connectorv2: %s: %s", response.Status, what), + err, + ) +} + // https://developer.okta.com/docs/reference/error-codes/ var oktaErrToGRPCError = map[string]codes.Code{ "E0000006": codes.PermissionDenied, @@ -126,6 +147,27 @@ var oktaErrToGRPCError = map[string]codes.Code{ "E0000011": codes.Unauthenticated, } +func bodyExcerpt(body []byte) string { + excerpt := strings.TrimSpace(string(body)) + if len(excerpt) > errorBodyExcerptLimit { + return truncateAtRuneBoundary(excerpt, errorBodyExcerptLimit) + "..." + } + return excerpt +} + +// truncateAtRuneBoundary cuts s to at most maxBytes without splitting a rune, so +// the excerpt reaching an error message or a log field stays valid UTF-8. +func truncateAtRuneBoundary(s string, maxBytes int) string { + if len(s) <= maxBytes { + return s + } + cut := maxBytes + for cut > 0 && !utf8.RuneStart(s[cut]) { + cut-- + } + return s[:cut] +} + func handleOktaResponseError(resp *okta.Response, err error) error { if err == nil { return nil diff --git a/pkg/oktaauth/nonce_challenge_test.go b/pkg/oktaauth/nonce_challenge_test.go new file mode 100644 index 00000000..99030f3d --- /dev/null +++ b/pkg/oktaauth/nonce_challenge_test.go @@ -0,0 +1,654 @@ +package oktaauth + +import ( + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + "testing/iotest" + "unicode/utf8" + + "github.com/conductorone/dpop/integrations/dpop_oauth2" + "github.com/okta/okta-sdk-golang/v2/okta" +) + +// Okta's resource server can deliver the DPoP nonce challenge as an HTTP 400 +// rather than the 401 RoundTrip used to gate on. Unretried, the empty-body 400 +// went straight to the caller and failed the sync. +func TestRoundTripper_ResourceNonceRetryOn400(t *testing.T) { + key := generateRSAKey(t) + var calls atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if calls.Add(1) == 1 { + w.Header().Set("WWW-Authenticate", `DPoP error="use_dpop_nonce", error_description="Authorization server requires nonce in DPoP proof"`) + w.Header().Set("DPoP-Nonce", "res-nonce-400") + w.WriteHeader(http.StatusBadRequest) // the whole bug: 400, not 401 + return + } + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + ns := dpop_oauth2.NewNonceStore() + rt := newRoundTripperForTest(t, key, dpopAccessToken("tok"), ns, http.DefaultTransport) + c := &http.Client{Transport: rt} + req, _ := http.NewRequestWithContext(t.Context(), http.MethodGet, srv.URL+"/api/v1/groups/00g1/users", nil) + resp, err := c.Do(req) + if err != nil { + t.Fatalf("Do: %v", err) + } + defer resp.Body.Close() + + if got := calls.Load(); got != 2 { + t.Errorf("server calls = %d, want 2 (400 nonce challenge was not retried)", got) + } + if resp.StatusCode != http.StatusOK { + t.Errorf("caller saw status %d, want 200 (the 400 challenge leaked to the caller)", resp.StatusCode) + } + // The nonce IS captured even on the unretried failure -- which is why the + // very next call succeeds and failures look random and never repeat. + if ns.GetNonce() != "res-nonce-400" { + t.Errorf("nonce store = %q, want res-nonce-400", ns.GetNonce()) + } +} + +// Guard for the fix: a plain 400 with no nonce challenge must never be retried. +func TestRoundTripper_NoRetryOnPlain400(t *testing.T) { + key := generateRSAKey(t) + var calls atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls.Add(1) + w.WriteHeader(http.StatusBadRequest) + })) + defer srv.Close() + + rt := newRoundTripperForTest(t, key, dpopAccessToken("tok"), dpop_oauth2.NewNonceStore(), http.DefaultTransport) + c := &http.Client{Transport: rt} + req, _ := http.NewRequestWithContext(t.Context(), http.MethodGet, srv.URL+"/api/v1/groups", nil) + resp, err := c.Do(req) + if err != nil { + t.Fatalf("Do: %v", err) + } + _ = resp.Body.Close() + if got := calls.Load(); got != 1 { + t.Errorf("server calls = %d, want 1", got) + } +} + +// The retry is now a bounded loop, not single-shot: Okta can rotate the nonce +// again on the retry, and a strictly sequential sync then failed anyway. +func TestRoundTripper_RetriesRepeatedNonceChallenges(t *testing.T) { + key := generateRSAKey(t) + var calls atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if n := calls.Add(1); n < int32(maxDPoPRetrySends) { + w.Header().Set("WWW-Authenticate", `DPoP error="use_dpop_nonce"`) + w.Header().Set("DPoP-Nonce", "rotated") + w.WriteHeader(http.StatusBadRequest) + return + } + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + rt := newRoundTripperForTest(t, key, dpopAccessToken("tok"), dpop_oauth2.NewNonceStore(), http.DefaultTransport) + c := &http.Client{Transport: rt} + req, _ := http.NewRequestWithContext(t.Context(), http.MethodGet, srv.URL+"/api/v1/groups", nil) + resp, err := c.Do(req) + if err != nil { + t.Fatalf("Do: %v", err) + } + defer resp.Body.Close() + + if got, want := calls.Load(), int32(maxDPoPRetrySends); got != want { + t.Errorf("server calls = %d, want %d", got, want) + } + if resp.StatusCode != http.StatusOK { + t.Errorf("final status = %d, want 200", resp.StatusCode) + } +} + +// The loop must terminate: an endlessly challenging server gets exactly +// maxDPoPRetrySends attempts, then the response goes to the caller. +func TestRoundTripper_NonceRetryIsBounded(t *testing.T) { + key := generateRSAKey(t) + var calls atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls.Add(1) + w.Header().Set("WWW-Authenticate", `DPoP error="use_dpop_nonce"`) + w.Header().Set("DPoP-Nonce", "never-good-enough") + w.WriteHeader(http.StatusBadRequest) + })) + defer srv.Close() + + rt := newRoundTripperForTest(t, key, dpopAccessToken("tok"), dpop_oauth2.NewNonceStore(), http.DefaultTransport) + c := &http.Client{Transport: rt} + req, _ := http.NewRequestWithContext(t.Context(), http.MethodGet, srv.URL+"/api/v1/groups", nil) + resp, err := c.Do(req) + if err != nil { + t.Fatalf("Do: %v", err) + } + defer resp.Body.Close() + + if got, want := calls.Load(), int32(maxDPoPRetrySends); got != want { + t.Errorf("server calls = %d, want %d", got, want) + } + if resp.StatusCode != http.StatusBadRequest { + t.Errorf("final status = %d, want 400 handed back to the caller", resp.StatusCode) + } +} + +// A challenge with no DPoP-Nonce header has nothing to retry with; it must not +// spin, and it now logs rather than falling through silently. +func TestRoundTripper_NoRetryOn400ChallengeWithoutNonceHeader(t *testing.T) { + key := generateRSAKey(t) + var calls atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls.Add(1) + w.Header().Set("WWW-Authenticate", `DPoP error="use_dpop_nonce"`) + w.WriteHeader(http.StatusBadRequest) + })) + defer srv.Close() + + rt := newRoundTripperForTest(t, key, dpopAccessToken("tok"), dpop_oauth2.NewNonceStore(), http.DefaultTransport) + c := &http.Client{Transport: rt} + req, _ := http.NewRequestWithContext(t.Context(), http.MethodGet, srv.URL+"/api/v1/groups", nil) + resp, err := c.Do(req) + if err != nil { + t.Fatalf("Do: %v", err) + } + _ = resp.Body.Close() + if got := calls.Load(); got != 1 { + t.Errorf("server calls = %d, want 1", got) + } +} + +// Okta's own nonce challenge (observed at its token endpoint) sends no +// WWW-Authenticate at all and puts the code in a JSON body. Matching only the +// header would miss it entirely. +func TestRoundTripper_NonceChallengeInBodyWithoutHeader(t *testing.T) { + key := generateRSAKey(t) + var calls atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if calls.Add(1) == 1 { + w.Header().Set("DPoP-Nonce", "from-body-challenge") + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{"error":"use_dpop_nonce","error_description":"Authorization server requires nonce in DPoP proof."}`)) + return + } + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + rt := newRoundTripperForTest(t, key, dpopAccessToken("tok"), dpop_oauth2.NewNonceStore(), http.DefaultTransport) + c := &http.Client{Transport: rt} + req, _ := http.NewRequestWithContext(t.Context(), http.MethodGet, srv.URL+"/api/v1/groups", nil) + resp, err := c.Do(req) + if err != nil { + t.Fatalf("Do: %v", err) + } + defer resp.Body.Close() + + if got := calls.Load(); got != 2 { + t.Errorf("server calls = %d, want 2 (body-carried challenge was not retried)", got) + } + if resp.StatusCode != http.StatusOK { + t.Errorf("final status = %d, want 200", resp.StatusCode) + } +} + +// Sniffing the body to look for a challenge must not consume it: a caller that +// declines to retry still has to read the full error payload. +func TestRoundTripper_ErrorBodySurvivesSniff(t *testing.T) { + key := generateRSAKey(t) + const payload = `{"errorCode":"E0000006","errorSummary":"You do not have permission to perform the requested action"}` + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("WWW-Authenticate", `DPoP error="invalid_dpop_proof"`) + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(payload)) + })) + defer srv.Close() + + rt := newRoundTripperForTest(t, key, dpopAccessToken("tok"), dpop_oauth2.NewNonceStore(), http.DefaultTransport) + c := &http.Client{Transport: rt} + req, _ := http.NewRequestWithContext(t.Context(), http.MethodGet, srv.URL+"/api/v1/groups", nil) + resp, err := c.Do(req) + if err != nil { + t.Fatalf("Do: %v", err) + } + defer resp.Body.Close() + + got, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("read restored body: %v", err) + } + if string(got) != payload { + t.Errorf("body = %q, want %q", got, payload) + } +} + +// A body larger than the sniff limit must also come back whole. +func TestRoundTripper_LargeErrorBodySurvivesSniff(t *testing.T) { + key := generateRSAKey(t) + payload := `{"errorCode":"E0000006","pad":"` + strings.Repeat("x", errorBodySniffLimit*2) + `"}` + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(payload)) + })) + defer srv.Close() + + rt := newRoundTripperForTest(t, key, dpopAccessToken("tok"), dpop_oauth2.NewNonceStore(), http.DefaultTransport) + c := &http.Client{Transport: rt} + req, _ := http.NewRequestWithContext(t.Context(), http.MethodGet, srv.URL+"/api/v1/groups", nil) + resp, err := c.Do(req) + if err != nil { + t.Fatalf("Do: %v", err) + } + defer resp.Body.Close() + + got, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("read restored body: %v", err) + } + if len(got) != len(payload) { + t.Errorf("body length = %d, want %d", len(got), len(payload)) + } +} + +// A 200 must never be buffered -- only client errors are sniffed. +func TestRoundTripper_SuccessBodyNotBuffered(t *testing.T) { + resp := &http.Response{StatusCode: http.StatusOK, Body: http.NoBody} + if got := sniffClientErrorBody(resp); got != nil { + t.Errorf("sniffed a 200 body: %q", got) + } + if resp.Body != http.NoBody { + t.Error("sniffClientErrorBody replaced the body of a success response") + } +} + +// The exact challenge Okta returns for a rejected proof, captured live. +const liveReplayChallenge = `DPoP algs="RS256 RS384 RS512 ES256 ES384 ES512", ` + + `authorization_uri="http://tenant.okta.com/oauth2/v1/authorize", realm="http://tenant.okta.com", ` + + `scope="okta.users.read.self", error="invalid_dpop_proof", ` + + `error_description="The DPoP proof JWT has already been used.", resource="/api/v1/users"` + +const liveSkewChallenge = `DPoP algs="RS256", error="invalid_dpop_proof", ` + + `error_description="The DPoP proof JWT is issued more than five minutes in the past.", resource="/api/v1/users"` + +// A replayed proof is resent once with a fresh proof: uhttp's transport retries +// with headers untouched, so the same jti can reach Okta twice. +func TestRoundTripper_RetriesProofReplay(t *testing.T) { + key := generateRSAKey(t) + var calls atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if calls.Add(1) == 1 { + w.Header().Set("WWW-Authenticate", liveReplayChallenge) + w.WriteHeader(http.StatusBadRequest) + return + } + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + rt := newRoundTripperForTest(t, key, dpopAccessToken("tok"), dpop_oauth2.NewNonceStore(), http.DefaultTransport) + c := &http.Client{Transport: rt} + req, _ := http.NewRequestWithContext(t.Context(), http.MethodGet, srv.URL+"/api/v1/users", nil) + resp, err := c.Do(req) + if err != nil { + t.Fatalf("Do: %v", err) + } + defer resp.Body.Close() + + if got := calls.Load(); got != 2 { + t.Errorf("server calls = %d, want 2 (replayed proof was not resent)", got) + } + if resp.StatusCode != http.StatusOK { + t.Errorf("final status = %d, want 200", resp.StatusCode) + } +} + +// A skewed clock is NOT resent -- retrying cannot fix it and would triple traffic. +func TestRoundTripper_DoesNotRetryClockSkewRejection(t *testing.T) { + key := generateRSAKey(t) + var calls atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls.Add(1) + w.Header().Set("WWW-Authenticate", liveSkewChallenge) + w.WriteHeader(http.StatusBadRequest) + })) + defer srv.Close() + + rt := newRoundTripperForTest(t, key, dpopAccessToken("tok"), dpop_oauth2.NewNonceStore(), http.DefaultTransport) + c := &http.Client{Transport: rt} + req, _ := http.NewRequestWithContext(t.Context(), http.MethodGet, srv.URL+"/api/v1/users", nil) + resp, err := c.Do(req) + if err != nil { + t.Fatalf("Do: %v", err) + } + defer resp.Body.Close() + + if got := calls.Load(); got != 1 { + t.Errorf("server calls = %d, want 1 (clock skew must not be retried)", got) + } +} + +// The empty-bodied rejection must come back with a body the Okta SDK can decode, +// so the reason reaches the caller instead of "the API returned an unknown error". +func TestRoundTripper_EmptyDPoPErrorGetsDecodableBody(t *testing.T) { + key := generateRSAKey(t) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("WWW-Authenticate", liveSkewChallenge) + w.WriteHeader(http.StatusBadRequest) + // no body at all, exactly as Okta answers + })) + defer srv.Close() + + rt := newRoundTripperForTest(t, key, dpopAccessToken("tok"), dpop_oauth2.NewNonceStore(), http.DefaultTransport) + c := &http.Client{Transport: rt} + req, _ := http.NewRequestWithContext(t.Context(), http.MethodGet, srv.URL+"/api/v1/users", nil) + resp, err := c.Do(req) + if err != nil { + t.Fatalf("Do: %v", err) + } + defer resp.Body.Close() + + got, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("read synthesized body: %v", err) + } + var payload struct { + ErrorSummary string `json:"errorSummary"` + } + if err := json.Unmarshal(got, &payload); err != nil { + t.Fatalf("synthesized body is not JSON (%q): %v", got, err) + } + if !strings.Contains(payload.ErrorSummary, "five minutes in the past") { + t.Errorf("errorSummary = %q, want Okta's reason", payload.ErrorSummary) + } + if !strings.Contains(payload.ErrorSummary, "invalid_dpop_proof") { + t.Errorf("errorSummary = %q, want the error code kept", payload.ErrorSummary) + } + if resp.Header.Get("Content-Type") != "application/json" { + t.Errorf("Content-Type = %q", resp.Header.Get("Content-Type")) + } +} + +// A rejection that already has a body must be left exactly as it arrived. +func TestRoundTripper_NonEmptyBodyNotOverwritten(t *testing.T) { + key := generateRSAKey(t) + const original = `{"errorCode":"E0000006","errorSummary":"real okta error"}` + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("WWW-Authenticate", liveSkewChallenge) + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(original)) + })) + defer srv.Close() + + rt := newRoundTripperForTest(t, key, dpopAccessToken("tok"), dpop_oauth2.NewNonceStore(), http.DefaultTransport) + c := &http.Client{Transport: rt} + req, _ := http.NewRequestWithContext(t.Context(), http.MethodGet, srv.URL+"/api/v1/users", nil) + resp, err := c.Do(req) + if err != nil { + t.Fatalf("Do: %v", err) + } + defer resp.Body.Close() + + got, _ := io.ReadAll(resp.Body) + if string(got) != original { + t.Errorf("body = %q, want it untouched (%q)", got, original) + } +} + +// End of the chain: feed the synthesized response to the real Okta SDK and check +// it now renders the reason Okta gave. The baseline assertion pins the bug -- the +// same response without a body yields the ticket's opaque message. +func TestAnnotateEmptyDPoPError_SDKRendersRealReason(t *testing.T) { + header := http.Header{} + header.Set(wwwAuthenticateHdr, liveReplayChallenge) + + baseline := okta.CheckResponseForError(&http.Response{ + StatusCode: http.StatusBadRequest, + Status: "400 Bad Request", + Header: header.Clone(), + Body: http.NoBody, + }) + if baseline == nil || !strings.Contains(baseline.Error(), "the API returned an unknown error") { + t.Fatalf("baseline should be the opaque SDK error, got %v", baseline) + } + + resp := &http.Response{ + StatusCode: http.StatusBadRequest, + Status: "400 Bad Request", + Header: header.Clone(), + Body: http.NoBody, + } + annotateEmptyDPoPError(resp, nil) + + fixed := okta.CheckResponseForError(resp) + if fixed == nil { + t.Fatal("expected an error from the annotated response") + } + if strings.Contains(fixed.Error(), "unknown error") { + t.Errorf("SDK still renders the opaque message: %q", fixed.Error()) + } + if !strings.Contains(fixed.Error(), "already been used") { + t.Errorf("SDK error = %q, want Okta's actual reason", fixed.Error()) + } + t.Logf("before: %v", baseline) + t.Logf("after: %v", fixed) +} + +// finish() runs for every response and body is only populated for client errors, +// so annotateEmptyDPoPError must range-check the status itself rather than trust +// an empty body argument -- otherwise a real body outside 4xx gets discarded. +func TestAnnotateEmptyDPoPError_OnlyTouchesClientErrors(t *testing.T) { + for _, statusCode := range []int{http.StatusOK, http.StatusFound, http.StatusInternalServerError} { + t.Run(http.StatusText(statusCode), func(t *testing.T) { + const original = `{"real":"payload"}` + header := http.Header{} + header.Set(wwwAuthenticateHdr, liveReplayChallenge) + resp := &http.Response{ + StatusCode: statusCode, + Header: header, + Body: io.NopCloser(strings.NewReader(original)), + } + // nil body, exactly as sniffClientErrorBody returns outside 4xx. + annotateEmptyDPoPError(resp, nil) + + got, err := io.ReadAll(resp.Body) + if err != nil { + t.Fatalf("read body: %v", err) + } + if string(got) != original { + t.Errorf("body = %q, want it untouched (%q)", got, original) + } + }) + } +} + +// The replay retry keys on two DPoP-specific markers, so a neighbouring Bearer +// challenge in the same header cannot trigger it. +func TestRoundTripper_BearerChallengeDoesNotTriggerProofRetry(t *testing.T) { + key := generateRSAKey(t) + var calls atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls.Add(1) + w.Header().Set("WWW-Authenticate", + `Bearer error="invalid_token", error_description="the token has already been used"`) + w.WriteHeader(http.StatusBadRequest) + })) + defer srv.Close() + + rt := newRoundTripperForTest(t, key, dpopAccessToken("tok"), dpop_oauth2.NewNonceStore(), http.DefaultTransport) + c := &http.Client{Transport: rt} + req, _ := http.NewRequestWithContext(t.Context(), http.MethodGet, srv.URL+"/api/v1/users", nil) + resp, err := c.Do(req) + if err != nil { + t.Fatalf("Do: %v", err) + } + defer resp.Body.Close() + + if got := calls.Load(); got != 1 { + t.Errorf("server calls = %d, want 1 (a Bearer challenge must not be retried)", got) + } +} + +// The challenge is restated verbatim, so a multi-challenge header cannot have one +// scheme's error reported as the other's -- everything the server sent is kept. +func TestAnnotateEmptyDPoPError_KeepsWholeChallenge(t *testing.T) { + const challenge = `DPoP algs="RS256", error="invalid_dpop_proof", ` + + `error_description="The DPoP proof JWT has already been used.", ` + + `Bearer error="invalid_token", error_description="unrelated"` + header := http.Header{} + header.Set(wwwAuthenticateHdr, challenge) + resp := &http.Response{ + StatusCode: http.StatusBadRequest, + Status: "400 Bad Request", + Header: header, + Body: http.NoBody, + } + annotateEmptyDPoPError(resp, nil) + + rendered := okta.CheckResponseForError(resp) + if rendered == nil { + t.Fatal("expected an error") + } + if !strings.Contains(rendered.Error(), "already been used") { + t.Errorf("error = %q, want the DPoP reason present", rendered) + } + // Nothing is attributed, so the neighbouring challenge survives too rather + // than being silently swapped in as the DPoP reason. + if !strings.Contains(rendered.Error(), "invalid_token") { + t.Errorf("error = %q, want the full challenge preserved", rendered) + } +} + +// nilBodyTransport mimics a RoundTripper that hands back a 4xx with no body. +// net/http never does, but io.Copy on a nil ReadCloser panics, so the drain path +// must not assume one. +type nilBodyTransport struct{ calls atomic.Int32 } + +func (t *nilBodyTransport) RoundTrip(req *http.Request) (*http.Response, error) { + t.calls.Add(1) + header := http.Header{} + header.Set(wwwAuthenticateHdr, `DPoP error="use_dpop_nonce"`) + header.Set(dpopNonceHdr, "fresh") + return &http.Response{ + StatusCode: http.StatusBadRequest, + Status: "400 Bad Request", + Header: header, + Body: nil, // the whole point + Request: req, + }, nil +} + +func TestRoundTripper_NilResponseBodyDoesNotPanic(t *testing.T) { + key := generateRSAKey(t) + inner := &nilBodyTransport{} + rt := newRoundTripperForTest(t, key, dpopAccessToken("tok"), dpop_oauth2.NewNonceStore(), inner) + c := &http.Client{Transport: rt} + req, _ := http.NewRequestWithContext(t.Context(), http.MethodGet, "https://tenant.okta.com/api/v1/users", nil) + + resp, err := c.Do(req) + if err != nil { + t.Fatalf("Do: %v", err) + } + if resp.Body != nil { + _ = resp.Body.Close() + } + // The nonce challenge is retryable, so it drains and resends up to the bound. + if got := inner.calls.Load(); got != int32(maxDPoPRetrySends) { + t.Errorf("inner calls = %d, want %d", got, maxDPoPRetrySends) + } +} + +func TestTruncateAtRuneBoundary(t *testing.T) { + // A three-byte rune so the limit does not divide evenly. + s := strings.Repeat("€", 10) // 30 bytes + for _, limit := range []int{29, 28, 27, 20, 1} { + got := truncateAtRuneBoundary(s, limit) + if !utf8.ValidString(got) { + t.Errorf("limit %d: %q is not valid UTF-8", limit, got) + } + if len(got) > limit { + t.Errorf("limit %d: got %d bytes", limit, len(got)) + } + } + if got := truncateAtRuneBoundary(s, 30); got != s { + t.Errorf("exact-length input was truncated to %q", got) + } + if got := truncateAtRuneBoundary("abc", 10); got != "abc" { + t.Errorf("short input = %q, want abc", got) + } +} + +// The replay marker has to be specific enough that a neighbouring challenge in the +// same header cannot trip it: "already been used" alone is plain English. +func TestRoundTripper_MixedChallengeDoesNotTriggerProofRetry(t *testing.T) { + key := generateRSAKey(t) + var calls atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls.Add(1) + // DPoP rejects for clock skew; a Bearer challenge separately mentions reuse. + w.Header().Set("WWW-Authenticate", + `DPoP error="invalid_dpop_proof", error_description="The DPoP proof JWT is issued in the future.", `+ + `Bearer error="invalid_token", error_description="that token has already been used"`) + w.WriteHeader(http.StatusBadRequest) + })) + defer srv.Close() + + rt := newRoundTripperForTest(t, key, dpopAccessToken("tok"), dpop_oauth2.NewNonceStore(), http.DefaultTransport) + c := &http.Client{Transport: rt} + req, _ := http.NewRequestWithContext(t.Context(), http.MethodGet, srv.URL+"/api/v1/users", nil) + resp, err := c.Do(req) + if err != nil { + t.Fatalf("Do: %v", err) + } + defer resp.Body.Close() + + if got := calls.Load(); got != 1 { + t.Errorf("server calls = %d, want 1 (clock skew must not be read as replay)", got) + } +} + +// truncatedBodyTransport returns a body that yields some bytes and then fails, the +// shape a connection dying mid-body produces. +type truncatedBodyTransport struct{ payload string } + +func (t truncatedBodyTransport) RoundTrip(req *http.Request) (*http.Response, error) { + header := http.Header{} + header.Set(wwwAuthenticateHdr, liveSkewChallenge) + return &http.Response{ + StatusCode: http.StatusBadRequest, + Status: "400 Bad Request", + Header: header, + Body: io.NopCloser(iotest.TimeoutReader(strings.NewReader(t.payload))), + Request: req, + }, nil +} + +// A partial read must not be reported as an empty body: the bytes that arrived are +// restored onto resp.Body, and claiming empty would let the challenge restatement +// overwrite real content. +func TestRoundTripper_PartialBodyReadIsNotTreatedAsEmpty(t *testing.T) { + key := generateRSAKey(t) + const payload = `{"errorCode":"E0000006","errorSummary":"real content"}` + rt := newRoundTripperForTest(t, key, dpopAccessToken("tok"), dpop_oauth2.NewNonceStore(), + truncatedBodyTransport{payload: payload}) + c := &http.Client{Transport: rt} + req, _ := http.NewRequestWithContext(t.Context(), http.MethodGet, "https://tenant.okta.com/api/v1/users", nil) + + resp, err := c.Do(req) + if err != nil { + t.Fatalf("Do: %v", err) + } + defer resp.Body.Close() + + got, _ := io.ReadAll(resp.Body) + if !strings.Contains(string(got), "real content") { + t.Errorf("body = %q, want the arrived bytes preserved rather than overwritten", got) + } +} diff --git a/pkg/oktaauth/round_tripper.go b/pkg/oktaauth/round_tripper.go index 64d752a8..211a3d36 100644 --- a/pkg/oktaauth/round_tripper.go +++ b/pkg/oktaauth/round_tripper.go @@ -1,14 +1,51 @@ package oktaauth import ( + "bytes" + "context" + "encoding/json" "errors" "fmt" "io" "net/http" + "strconv" "strings" + "sync/atomic" + "unicode/utf8" "github.com/conductorone/dpop/integrations/dpop_oauth2" "github.com/conductorone/dpop/pkg/dpop" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" + "go.uber.org/zap" +) + +const ( + // maxDPoPRetrySends bounds how many times one logical request may be sent while + // Okta keeps rejecting the DPoP layer: the initial send plus retries. + maxDPoPRetrySends = 3 + // errorBodySniffLimit caps how much of a 4xx body is buffered while looking for + // a challenge code. Okta's challenge payload is a few hundred bytes. + errorBodySniffLimit = 4096 + // maxChallengeSummaryLen bounds the challenge text restated as the error body. + // Okta's runs about 250 bytes. + maxChallengeSummaryLen = 1024 + // proofReplayDesc is the distinctive part of the error_description Okta returns + // when a proof's jti has been seen before ("The DPoP proof JWT has already been + // used."). uhttp's transport retries a failed request with its headers + // untouched, so a stale-connection retry resends the same proof; minting a + // fresh one and retrying once clears it. Matched in full rather than on "already + // been used" alone, which is plain English another scheme's challenge could + // carry. + proofReplayDesc = "DPoP proof JWT has already been used" +) + +// dpopRetryReason names the DPoP-layer rejections worth resending. +type dpopRetryReason int + +const ( + dpopRetryNone dpopRetryReason = iota + dpopRetryNonce + dpopRetryProofReplay ) type dpopRoundTripper struct { @@ -16,6 +53,10 @@ type dpopRoundTripper struct { proofer *dpop.Proofer tokenSource tokenGetter resourceNonceStore *dpop_oauth2.NonceStore + // dpopFailureCount drives logarithmic sampling of the DPoP failure log: a + // skewed clock rejects every request in a sync, so an unsampled line per + // request would flood the log. + dpopFailureCount atomic.Int64 } func (rt *dpopRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { @@ -29,22 +70,59 @@ func (rt *dpopRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) return nil, err } - if resp.StatusCode == http.StatusUnauthorized && isResourceNonceChallenge(resp) && isReplayable(req) { - nonce := resp.Header.Get(dpopNonceHdr) - if nonce != "" { + // Okta rejects the DPoP layer in two resendable ways: a stale nonce, and a + // replayed proof jti. Both clear on a fresh proof, and either can recur on the + // retry, so loop -- mirroring the token endpoint's bounded retry in exchange(). + for send := 1; ; send++ { + body := sniffClientErrorBody(resp) + reason := retryableDPoPFailure(resp, body) + if reason == dpopRetryNone || !isReplayable(req) { + return rt.finish(req, resp, body), nil + } + if send >= maxDPoPRetrySends { + ctxzap.Extract(req.Context()).Warn( + "oktaauth: dpop rejection persisted across retries"+proxyStripHintResponse, + zap.Int("status_code", resp.StatusCode), + zap.String("path", req.URL.Path), + zap.Int("sends", send), + ) + return rt.finish(req, resp, body), nil + } + if reason == dpopRetryNonce { + nonce := resp.Header.Get(dpopNonceHdr) + if nonce == "" { + ctxzap.Extract(req.Context()).Warn( + "oktaauth: dpop nonce challenge carried no DPoP-Nonce header; not retrying"+proxyStripHintResponse, + zap.Int("status_code", resp.StatusCode), + zap.String("path", req.URL.Path), + ) + return rt.finish(req, resp, body), nil + } rt.resourceNonceStore.SetNonce(nonce) + } + ctxzap.Extract(req.Context()).Debug("oktaauth: retrying after dpop rejection", + zap.Int("status_code", resp.StatusCode), + zap.String("path", req.URL.Path), + zap.Int("send", send), + zap.Bool("proof_replay", reason == dpopRetryProofReplay), + ) + // net/http always sets a body, but sniffClientErrorBody and + // annotateEmptyDPoPError both guard it, and io.Copy on a nil ReadCloser + // panics -- so guard here too rather than trust the transport. + if resp.Body != nil { _, _ = io.Copy(io.Discard, resp.Body) _ = resp.Body.Close() - // Refresh: the original token may have expired mid-roundtrip; a stale ath would be rejected. - freshTok, terr := rt.tokenSource.Token(req.Context()) - if terr != nil { - return nil, fmt.Errorf("oktaauth: refresh token before nonce retry: %w", terr) - } - return rt.send(req, freshTok) + } + // Refresh: the original token may have expired mid-roundtrip; a stale ath would be rejected. + freshTok, terr := rt.tokenSource.Token(req.Context()) + if terr != nil { + return nil, fmt.Errorf("oktaauth: refresh token before dpop retry: %w", terr) + } + resp, err = rt.send(req, freshTok) + if err != nil { + return nil, err } } - - return resp, nil } // Concurrent requests can fail with a 401. The retry handles it. Don't add a lock. @@ -112,8 +190,176 @@ func htuForProof(req *http.Request) (string, error) { return u.String(), nil } -func isResourceNonceChallenge(resp *http.Response) bool { - return strings.Contains(resp.Header.Get(wwwAuthenticateHdr), "use_dpop_nonce") +// finish is the single exit for a response the DPoP layer will not resend: it +// gives an empty-bodied rejection a decodable body and reports what it saw. +func (rt *dpopRoundTripper) finish(req *http.Request, resp *http.Response, body []byte) *http.Response { + rt.logDPoPFailure(req.Context(), req, resp, body) + annotateEmptyDPoPError(resp, body) + return resp +} + +// retryableDPoPFailure classifies a response as a DPoP rejection worth resending +// with a fresh proof, or not worth resending at all. Deliberately narrow: a +// generic invalid_dpop_proof (a skewed clock, say) is not resent, since retrying +// would only triple the requests without changing the outcome. +func retryableDPoPFailure(resp *http.Response, body []byte) dpopRetryReason { + switch resp.StatusCode { + case http.StatusBadRequest, http.StatusUnauthorized: + default: + return dpopRetryNone + } + challenge := resp.Header.Get(wwwAuthenticateHdr) + if isResourceNonceChallenge(resp, body) { + return dpopRetryNonce + } + // Both markers are DPoP-specific, so finding them anywhere in the header is + // unambiguous; no need to work out which challenge they belong to. + if strings.Contains(challenge, invalidDPoPProofErrorCode) && strings.Contains(challenge, proofReplayDesc) { + return dpopRetryProofReplay + } + return dpopRetryNone +} + +// isResourceNonceChallenge reports whether resp is Okta demanding a fresh DPoP +// nonce, in either shape RFC 9449 allows: a resource server answers 401 with the +// code in WWW-Authenticate, an authorization server answers 400 with the code in +// a JSON body. Okta's token endpoint was observed using the latter and sending no +// WWW-Authenticate at all, so matching only the header would miss it. body may be +// nil when the response was not a client error. +func isResourceNonceChallenge(resp *http.Response, body []byte) bool { + switch resp.StatusCode { + case http.StatusBadRequest, http.StatusUnauthorized: + default: + return false + } + if strings.Contains(resp.Header.Get(wwwAuthenticateHdr), useDPoPNonceErrorCode) { + return true + } + return jsonErrorCode(body) == useDPoPNonceErrorCode +} + +// annotateEmptyDPoPError gives an empty-bodied DPoP rejection a body the Okta SDK +// can decode. Okta answers a bad proof with a 400, an empty body, and the reason +// in a DPoP-scheme WWW-Authenticate; the SDK's CheckResponseForError reads that +// header only for 401/403 responses whose scheme is Bearer, then discards the +// decode failure on the empty body and yields a zero-valued error rendering as +// "the API returned an unknown error". Restating the header as the JSON the SDK +// expects turns that back into the reason Okta actually gave. +func annotateEmptyDPoPError(resp *http.Response, body []byte) { + // Guard the status range here rather than relying on the caller: body is only + // populated for client errors, so outside that range an empty body argument + // says nothing about the real body and rewriting it would discard content. + if resp.StatusCode < 400 || resp.StatusCode >= 500 { + return + } + if len(bytes.TrimSpace(body)) > 0 { + return + } + challenge := resp.Header.Get(wwwAuthenticateHdr) + if challenge == "" { + return + } + if len(challenge) > maxChallengeSummaryLen { + challenge = truncateAtRuneBoundary(challenge, maxChallengeSummaryLen) + "..." + } + // Passed through whole rather than parsed into auth-params: RFC 9110 allows + // several challenges in one header, and attributing a param to the right scheme + // needs a real parser. Quoting the header drops nothing and cannot misattribute + // one scheme's error to another. + payload, err := json.Marshal(struct { + ErrorSummary string `json:"errorSummary"` + }{ErrorSummary: challenge}) + if err != nil { + return + } + if resp.Body != nil { + _ = resp.Body.Close() + } + resp.Body = io.NopCloser(bytes.NewReader(payload)) + resp.ContentLength = int64(len(payload)) + resp.Header.Set("Content-Type", "application/json") + resp.Header.Set("Content-Length", strconv.Itoa(len(payload))) +} + +// logDPoPFailure reports a DPoP-related client error while the detail is still +// visible, since the SDK is about to discard the WWW-Authenticate header. Warn per +// the upstream-4xx convention, but logarithmically sampled: a generic +// invalid_dpop_proof is deliberately not retried, so a skewed clock would +// otherwise emit one line per request for a whole sync. +func (rt *dpopRoundTripper) logDPoPFailure(ctx context.Context, req *http.Request, resp *http.Response, body []byte) { + if resp.StatusCode < 400 || resp.StatusCode >= 500 { + return + } + challenge := resp.Header.Get(wwwAuthenticateHdr) + errCode := jsonErrorCode(body) + if !strings.Contains(strings.ToLower(challenge), "dpop") && !strings.Contains(errCode, "dpop") { + return + } + count := rt.dpopFailureCount.Add(1) + if count != 1 && count != 10 && count != 100 && count%1000 != 0 { + return + } + ctxzap.Extract(ctx).Warn("oktaauth: dpop failure on resource request", + zap.Int64("total_occurrences", count), + zap.Int("status_code", resp.StatusCode), + zap.String("method", req.Method), + zap.String("path", req.URL.Path), + zap.String("www_authenticate", challenge), + zap.String("body_error", errCode), + zap.Bool("nonce_offered", resp.Header.Get(dpopNonceHdr) != ""), + ) +} + +// jsonErrorCode pulls the OAuth-style "error" field out of an error body. +func jsonErrorCode(body []byte) string { + if len(body) == 0 { + return "" + } + var payload struct { + Error string `json:"error"` + } + if json.Unmarshal(body, &payload) != nil { + return "" + } + return payload.Error +} + +// truncateAtRuneBoundary cuts s to at most maxBytes without splitting a rune, so +// the result stays valid UTF-8. A raw byte slice can leave a partial rune behind. +func truncateAtRuneBoundary(s string, maxBytes int) string { + if len(s) <= maxBytes { + return s + } + cut := maxBytes + for cut > 0 && !utf8.RuneStart(s[cut]) { + cut-- + } + return s[:cut] +} + +// restoredBody re-attaches a peeked prefix ahead of the unread remainder, so a +// body can be inspected and still handed to the caller intact. +type restoredBody struct { + io.Reader + io.Closer +} + +// sniffClientErrorBody buffers the start of a 4xx body and puts it back. Only +// client errors are buffered, so success responses stream through untouched. +func sniffClientErrorBody(resp *http.Response) []byte { + if resp.StatusCode < 400 || resp.StatusCode >= 500 || resp.Body == nil { + return nil + } + // The read error is dropped deliberately: whatever arrived is restored onto the + // body, so the caller will meet the error again when it reads. Reporting nil + // here would instead claim an empty body, and annotateEmptyDPoPError would + // overwrite the partial payload that did arrive. + prefix, _ := io.ReadAll(io.LimitReader(resp.Body, errorBodySniffLimit)) + resp.Body = restoredBody{ + Reader: io.MultiReader(bytes.NewReader(prefix), resp.Body), + Closer: resp.Body, + } + return prefix } func isReplayable(req *http.Request) bool {