Skip to content

Conversation

@imogenkraak
Copy link
Contributor

@imogenkraak imogenkraak commented Nov 10, 2025

Description

This change improves the logging in the JWT Middleware, to make it clearer that the option to disable IDP client id mapping exists for external IDPs to improve performance.

Related Issue

TT-15354

Motivation and Context

This ticket described a bug that was fixed a few years ago: https://tyktech.atlassian.net/browse/TT-10566
For more information see this comment: https://tyktech.atlassian.net/browse/TT-15354?focusedCommentId=111729

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring or add test (improvements in base code or adds test coverage to functionality)

Checklist

  • I ensured that the documentation is up to date
  • I explained why this PR updates go.mod in detail with reasoning why it's required
  • I would like a code coverage CI quality gate exception and have explained why

Ticket Details

TT-15354
Status In Code Review
Summary Gateway fails to fetch keys from local Redis for JWT API

Generated at: 2025-11-12 12:32:46

@github-actions
Copy link
Contributor

github-actions bot commented Nov 10, 2025

🎯 Recommended Merge Targets

Based on JIRA ticket TT-15354: Gateway fails to fetch keys from local Redis for JWT API

Fix Version: Tyk 5.11.0

⚠️ Warning: Expected release branches not found in repository

Required:

  • master - No matching release branches found. Fix will be included in future releases.

Fix Version: Tyk 5.8.9

Required:

  • release-5.8 - Minor version branch for 5.8.x patches - required for creating Tyk 5.8.9
  • master - Main development branch - ensures fix is in all future releases

📋 Workflow

  1. Merge this PR to master first

  2. Cherry-pick to release branches by commenting on the merged PR:

    • /release to release-5.8
  3. Automated backport - The bot will automatically create backport PRs to the specified release branches

@probelabs
Copy link

probelabs bot commented Nov 10, 2025

🔍 Code Analysis Results

This PR improves the logging in the JWT Middleware to make it clearer that an option exists to disable IDP client ID mapping for external IDPs, which can improve performance. When the gateway fails to retrieve an OAuth client, it now logs a more informative warning message suggesting this configuration change.

Files Changed Analysis

  • File: gateway/mw_jwt.go
  • Additions: 5
  • Deletions: 2
  • Summary: The changes are highly localized to the processCentralisedJWT function. The modifications consist of elevating a log level from Debug to Warn, enriching the log message to be more actionable, adding a new Debug log for clarity, and updating code comments. No business logic has been altered.

Architecture & Impact Assessment

What this PR accomplishes

This PR enhances the observability of the JWT middleware. It makes a potentially obscure performance-tuning option (IDPClientIDMappingDisabled) more discoverable for operators by logging a clear warning when a potentially unnecessary OAuth client lookup fails. This is particularly relevant when using external Identity Providers (IDPs).

Key technical changes introduced

  1. Log Level Elevation: The log message for a failed OAuth client lookup has been changed from Debug to Warn, increasing its visibility.
  2. Actionable Log Message: The new warning message explicitly suggests disabling IDP client ID mapping to improve performance with external IDPs.
  3. New Debug Log: A Debug log has been added to show when the IDP client ID mapping feature is enabled and being used.
  4. Improved Comments: Code comments have been added to clarify the logic related to the IDPClientIDMappingDisabled flag.

Affected system components

The only component affected is the JWT Middleware within the Tyk Gateway. The change has no impact on the middleware's runtime behavior; it solely modifies the log output to provide better operational guidance.

The logical flow where the logging is improved can be visualized as follows:

graph TD
    A[Start processCentralisedJWT] --> B{IDPClientIDMappingDisabled == true?};
    B -- Yes --> C[Skip OAuth Client ID retrieval];
    B -- No --> D[Log "IDP client ID mapping enabled..." (New Debug Log)];
    D --> E[Attempt to get OAuth Client ID from claims];
    E --> F{OAuth client retrieved successfully?};
    F -- Yes --> G[Proceed with session object];
    F -- No --> H[Log Warn: "Failed to retrieve OAuth client... consider disabling IDP client ID mapping..." (Improved Warn Log)];
    C --> I[Continue processing];
    H --> I;
    G --> I;
Loading

Scope Discovery & Context Expansion

The change is confined to the processCentralisedJWT function in gateway/mw_jwt.go. This function is central to handling JWTs that are validated against policies managed by a Tyk Control Plane.

The configuration flag at the heart of this change, IDPClientIDMappingDisabled, is part of the API definition. Its purpose is to prevent the gateway from performing a potentially unnecessary lookup for an OAuth client ID in its local storage. This is useful when the JWT is issued by an external IDP, where claims like client_id might be used for different purposes and don't correspond to a client managed by Tyk. By logging a suggestion to use this flag, the PR helps operators diagnose and resolve performance issues in this specific scenario without requiring deep familiarity with Tyk's internal JWT handling. The change is self-contained and has no downstream effects on other components.

Metadata
  • Review Effort: 1 / 5
  • Primary Label: enhancement

Powered by Visor from Probelabs

Last updated: 2025-11-12T12:36:26.188Z | Triggered by: synchronize | Commit: 36b0e5b

💡 TIP: You can chat with Visor using /visor ask <your question>

@imogenkraak imogenkraak force-pushed the TT-15354-improve-jwt-logging branch 2 times, most recently from 7663215 to ef206e3 Compare November 10, 2025 07:56
@imogenkraak imogenkraak force-pushed the TT-15354-improve-jwt-logging branch from ef206e3 to 390ada8 Compare November 10, 2025 07:58
@TykTechnologies TykTechnologies deleted a comment from probelabs bot Nov 10, 2025
@imogenkraak imogenkraak marked this pull request as ready for review November 10, 2025 07:59
@probelabs
Copy link

probelabs bot commented Nov 10, 2025

🔍 Code Analysis Results

Security Issues (1)

Severity Location Issue
🟡 Warning gateway/mw_jwt.go:988-989
The log level for an error that could potentially contain sensitive data has been elevated from `Debug` to `Warn`. The error originates from a function that unmarshals an `OauthClient` object, which contains a `ClientSecret`. If the stored client data is malformed, the JSON unmarshal error could include fragments of sensitive data. Logging this at the `Warn` level increases the risk of exposing this data in production logs.
💡 SuggestionTo mitigate the risk of leaking sensitive data, log a generic warning message without the raw error object. The detailed error can remain at the `Debug` level for troubleshooting purposes, which is less likely to be enabled in production environments.

✅ Architecture Check Passed

No architecture issues found – changes LGTM.

✅ Performance Check Passed

No performance issues found – changes LGTM.

✅ Quality Check Passed

No quality issues found – changes LGTM.

✅ Dependency Check Passed

No dependency issues found – changes LGTM.

Connectivity Issues (1)

Severity Location Issue
🟡 Warning gateway/mw_jwt.go:986
The log level for a failed OAuth client lookup has been raised from `Debug` to `Warn`. This log is on the critical path for every authenticated API request. For common configurations using external Identity Providers, this failure is an expected event. Logging it as a warning for every request can create significant I/O overhead and log volume under high traffic, degrading gateway performance.
💡 SuggestionRevert the log level from `Warnf` back to `Debugf` to avoid performance degradation under high load. A warning for a frequent and expected event in a hot path is not ideal. Consider logging a single warning during API definition loading if a sub-optimal configuration is detected, rather than on every request.

Powered by Visor from Probelabs

Last updated: 2025-11-12T12:36:27.404Z | Triggered by: synchronize | Commit: 36b0e5b

💡 TIP: You can chat with Visor using /visor ask <your question>

@github-actions
Copy link
Contributor

API Changes

no api changes detected

@sonarqubecloud
Copy link

@imogenkraak imogenkraak merged commit 71eec19 into master Nov 12, 2025
67 of 79 checks passed
@imogenkraak imogenkraak deleted the TT-15354-improve-jwt-logging branch November 12, 2025 13:54
@imogenkraak
Copy link
Contributor Author

/release to release-5.8.9

probelabs bot pushed a commit that referenced this pull request Dec 9, 2025
Improves the logging in the JWT Middleware, to make it clearer that the option to disable IDP client id mapping exists for external IDPs.

(cherry picked from commit 71eec19)
@probelabs
Copy link

probelabs bot commented Dec 9, 2025

✅ Cherry-pick successful. A PR was created: #7612

@imogenkraak
Copy link
Contributor Author

/release to release-5.8

probelabs bot pushed a commit that referenced this pull request Dec 9, 2025
Improves the logging in the JWT Middleware, to make it clearer that the option to disable IDP client id mapping exists for external IDPs.

(cherry picked from commit 71eec19)
@probelabs
Copy link

probelabs bot commented Dec 9, 2025

✅ Cherry-pick successful. A PR was created: #7613

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants