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 {