Skip to content

jwtauthccl: check HTTP status code in getHttpResponse#158294

Merged
trunk-io[bot] merged 1 commit intocockroachdb:masterfrom
vxtls:fix/jwt-http-status
Mar 13, 2026
Merged

jwtauthccl: check HTTP status code in getHttpResponse#158294
trunk-io[bot] merged 1 commit intocockroachdb:masterfrom
vxtls:fix/jwt-http-status

Conversation

@vxtls
Copy link
Contributor

@vxtls vxtls commented Nov 24, 2025 •
edited
Loading

check HTTP status in getHttpResponse and include body snippet

Epic: None

Overview

Previously, getHttpResponse ignored non-2xx HTTP responses.
As a result, JWKS/userinfo/discovery requests could return 4xx or 5xx pages,
and the caller would only fail when JSON parsing encountered an error.
This made it difficult to quickly identify the root cause of HTTP failures.

What this change does

  • Adds a check for non-2xx HTTP status codes.
  • Returns a clear error including:
    • HTTP status
    • Requested URL
    • Up to 200 bytes of the response body for context
  • Maintains existing behavior for successful 2xx responses.

Example

A 403 Forbidden response will now return an error like:
GET https://issuer.example.com/.well-known/jwks returned 403 Forbidden: <body snippet>

Testing

  • Added TestGetHTTPResponseNon2xx to cover non-2xx responses.
  • Verified that errors include both status and body snippet.
  • Ran existing tests to ensure no regressions.

Why this is useful

This change improves debuggability and observability for JWT authentication issues.
Telemetry and logs will now clearly show the actual HTTP failure instead of failing later in JSON parsing.

@vxtls vxtls requested review from a team as code owners November 24, 2025 22:24
@blathers-crl
Copy link

blathers-crl bot commented Nov 24, 2025

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

Before a member of our team reviews your PR, I have some potential action items for you:

  • Please ensure your git commit message contains a release note.
  • When CI has completed, please ensure no errors have appeared.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@blathers-crl blathers-crl bot added the O-community Originated from the community label Nov 24, 2025
@cockroach-teamcity
Copy link
Member

cockroach-teamcity commented Nov 24, 2025

This change is Reviewable

@cockroachlabs-cla-agent
Copy link

cockroachlabs-cla-agent bot commented Nov 24, 2025 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

pritesh-lahoti
pritesh-lahoti approved these changes Mar 12, 2026
View reviewed changes
Copy link
Contributor

@pritesh-lahoti pritesh-lahoti left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this contribution and apologies for the delay, @vxtls! This is a really clean change and the PR description is excellent -- the "before and after" is very clear.

The logic looks correct to me, and CI is fully green. A couple of small notes:

  1. Rebase needed: It looks like there are merge conflicts with master. Could you rebase your branch so we can get this mergeable?

  2. Release note: The commit will need a release note line. I'm happy to add that myself when we merge, so don't worry about it unless you'd like to write one.

  3. I left one inline suggestion about the error construction pattern -- not a blocker, and I can amend it myself if you'd prefer.

Really appreciate you improving the debuggability here. The old behavior of silently passing non-2xx responses through to JSON parsing was a real pain point.

@pritesh-lahoti reviewed 2 files and all commit messages, and made 2 comments.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on vxtls).

pkg/ccl/jwtauthccl/authentication_jwt.go line 524 at r1 (raw file):

			bodySnippet = bodySnippet[:200] + "..."
		}
		return nil, errors.Newf("GET %s returned %s: %s", url, resp.Status, bodySnippet)

nit (happy to fix this myself before merging): The rest of this file uses errors.WithDetailf to keep internal details (like URLs and response content) in the error detail rather than the top-level error message. That pattern keeps those details out of client-facing error surfaces while still making them available in logs. Something like:

return nil, errors.WithDetailf(
    errors.Newf("JWT authentication: HTTP request failed"),
    "GET %s returned %s: %s", url, resp.Status, bodySnippet,
)

This would be consistent with how the callers in ValidateJWTLogin and VerifyAndExtractIssuer handle errors from this function. Not a blocker -- I can amend this when merging if you'd like.

@vxtls vxtls force-pushed the fix/jwt-http-status branch from 6ae4d4a to 8c13b52 Compare March 12, 2026 15:10
@blathers-crl
Copy link

blathers-crl bot commented Mar 12, 2026

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

  • We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
  • Please ensure your git commit message contains a release note.
  • When CI has completed, please ensure no errors have appeared.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@vxtls vxtls requested a review from pritesh-lahoti March 12, 2026 15:17
@vxtls
Copy link
Contributor Author

vxtls commented Mar 12, 2026

@pritesh-lahoti
CI appears to have failed, but this seems to be an issue with the CI environment (permission denied).
sample:

INFO: From GoMockReflectProgOnlyGen pkg/ccl/changefeedccl/mocks/kafka_admin_v2_generated_gomock_prog.go [for tool]:
2026/03/02 19:53:50 failed to create modcache index dir: mkdir /home/roach: permission denied
INFO: From GoMockReflectProgOnlyGen pkg/cmd/tef/planners/mock_plan_executor_gomock_prog.go [for tool]:
2026/03/02 19:53:50 failed to create modcache index dir: mkdir /home/roach: permission denied

@vxtls @roachdev-claude @pritesh-lahoti
jwtauth: check HTTP status code in getHttpResponse
2c21d3d 
Previously, getHttpResponse would silently accept non-2xx HTTP responses
when fetching JWKS or OpenID configuration, potentially leading to
confusing downstream parse errors. Now, non-2xx responses return an error
with the HTTP status and a truncated body snippet in the error details.

Release note (bug fix): JWT authentication now returns a clear error when
HTTP requests to fetch JWKS or OpenID configuration return non-2xx status
codes, instead of silently passing the response body to the JSON parser.

Co-Authored-By: roachdev-claude <roachdev-claude-bot@cockroachlabs.com>
pritesh-lahoti
pritesh-lahoti approved these changes Mar 13, 2026
View reviewed changes
Copy link
Contributor

@pritesh-lahoti pritesh-lahoti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Squashed the commits and added the release note.
Will merge once the CI passes now (seemed to be flaky earlier).

Thanks again, @vxtls !

@pritesh-lahoti
Copy link
Contributor

pritesh-lahoti commented Mar 13, 2026

/trunk merge

@trunk-io
Copy link
Contributor

trunk-io bot commented Mar 13, 2026 •
edited
Loading

😎 Merged directly without going through the merge queue, as the queue was empty and the PR was up to date with the target branch - details.

@trunk-io trunk-io bot merged commit 0c68d18 into cockroachdb:master Mar 13, 2026
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pritesh-lahoti pritesh-lahoti pritesh-lahoti approved these changes

Assignees

No one assigned

Labels

O-community Originated from the community target-release-26.2.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vxtls @cockroach-teamcity @pritesh-lahoti