Skip to content

sync with master#7533

Merged
Razeen-Abdal-Rahman merged 8 commits intofix/pipline-build-errors-TT-16060from
master
Nov 11, 2025
Merged

sync with master#7533
Razeen-Abdal-Rahman merged 8 commits intofix/pipline-build-errors-TT-16060from
master

Conversation

@Razeen-Abdal-Rahman
Copy link
Copy Markdown
Contributor

No description provided.

pvormste and others added 8 commits November 6, 2025 12:04
# Summary

Fixes mTLS authentication when certificate chains are used and improves
certificate matching to support hosts with port numbers.

# Problem

## Issue 1 - Certificate Chains Broken:
When a certificate chain (containing both leaf certificate and CA
certificate) was configured for mTLS, the system incorrectly added the
entire chain, including the leaf certificate, to the CA pool. This
caused TLS handshake failures because leaf certificates should be
verified against the CA pool, not added to it.

## Issue 2 - Port Handling:
Certificate matching failed when the target host included a port number
(e.g., api.example.com:8443). Configuratio entries like
"api.example.com": "cert-id" would not match requests to
api.example.com:8443.

# Solution

## 1. Certificate Chain Handling

Created a new helper function `AddCACertificatesFromChainToPool()` that
intelligently extracts only CA certificates from chains:
- For certificate chains (length > 1): Skips index 0 (leaf cert) and
adds only certificates where IsCA=true
- For single certificates (length = 1): Maintains backward compatibility
by adding the certificate regardless of IsCA flag
  (supports certificate pinning)

Updated two critical locations to use this function:
  - `certs/manager.go:601` - Certificate pool management
  - `gateway/cert.go:466` - Client CA configuration for mTLS APIs

## 2. Port Handling

Refactored certificate matching logic into a testable function
getCertificateIDForHost() that:
  - Uses Go's `net.SplitHostPort()` to properly handle hosts with ports
  - Implements priority-based matching (most specific match wins)
  - Supports IPv4, IPv6, and hostnames with or without ports

Matching Priority (low to high):
  1. `*` (wildcard)
  2. `*.example.com:8443` (wildcard with port)
  3. `*.example.com` (wildcard without port)
  4. `example.com` (exact without port)
  5. `example.com:8443` (exact with port - highest priority)

# Changes

- `internal/crypto/helpers.go` - New AddCACertificatesFromChainToPool()
function
  - `internal/crypto/helpers_test.go` - 8 unit tests for chain handling
- `gateway/cert.go` - Refactored certificate matching with port support
- `gateway/cert_test.go` - 20 new unit tests + end-to-end mTLS test with
chains
  - `certs/manager.go` - Updated to use new chain handling function

## Backward Compatibility

  ✅ Fully backward compatible:
- Existing certificate configurations (with or without ports) continue
to work
  - Single certificate configurations unchanged
  - Certificate chains now work correctly (previously broken)
  - No configuration migration required

# Reviewer Focus Areas

## Critical:
1. internal/crypto/helpers.go:156-212 - Core certificate chain
extraction logic
2. End-to-end test in gateway/cert_test.go:2203-2309 - Validates the fix
works

## Important:
3. gateway/cert.go:72-138 - Certificate matching with priority handling
  4. Port handling logic using net.SplitHostPort()

## Security Considerations

  - Follows TLS RFC 5246 (leaf certificate at index 0 in chains)
  - Only certificates with IsCA=true are added to CA pools
  - More specific certificate matches override general wildcards
  - Comprehensive negative testing ensures invalid clients are rejected





<!---TykTechnologies/jira-linter starts here-->

### Ticket Details

<details>
<summary>
<a href="https://tyktech.atlassian.net/browse/TT-15606" title="TT-15606"
target="_blank">TT-15606</a>
</summary>

|         |    |
|---------|----|
| Status  | In Dev |
| Summary | Tyk mTLS Advertises Leaf Subject DNs Instead of CA DNs |

Generated at: 2025-11-06 09:42:00

</details>

<!---TykTechnologies/jira-linter ends here-->
<!-- Provide a general summary of your changes in the Title above -->

## Description

<!-- Describe your changes in detail -->

## Related Issue

<!-- This project only accepts pull requests related to open issues. -->
<!-- If suggesting a new feature or change, please discuss it in an
issue first. -->
<!-- If fixing a bug, there should be an issue describing it with steps
to reproduce. -->
<!-- OSS: Please link to the issue here. Tyk: please create/link the
JIRA ticket. -->

## Motivation and Context

<!-- Why is this change required? What problem does it solve? -->

## How This Has Been Tested

<!-- Please describe in detail how you tested your changes -->
<!-- Include details of your testing environment, and the tests -->
<!-- you ran to see how your change affects other areas of the code,
etc. -->
<!-- This information is helpful for reviewers and QA. -->

## Screenshots (if appropriate)

## Types of changes

<!-- What types of changes does your code introduce? Put an `x` in all
the boxes that apply: -->

- [ ] 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

<!-- Go over all the following points, and put an `x` in all the boxes
that apply -->
<!-- If there are no documentation updates required, mark the item as
checked. -->
<!-- Raise up any additional concerns not covered by the 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

<!---TykTechnologies/jira-linter starts here-->

### Ticket Details

<details>
<summary>
<a href="https://tyktech.atlassian.net/browse/TT-15793" title="TT-15793"
target="_blank">TT-15793</a>
</summary>

|         |    |
|---------|----|
| Status  | In Dev |
| Summary | Implement Intelligent Branch Merge Recommendations |

Generated at: 2025-11-05 14:15:34

</details>

<!---TykTechnologies/jira-linter ends here-->

Co-authored-by: sredny buitrago <sredny@srednys-MacBook-Pro.local>
Implements TT-15473

The Tyk Gateway's OpenAPI specifications didn't accurately reflect how
to access versioned APIs. Server URLs in the OAS didn't show:
  - Correct paths for versioned child APIs
  - Versioning methods (URL path, query param, header)
  - How to actually invoke different API versions

This created inconsistency between the Gateway and Dashboard and
provided inaccurate API documentation to users.

### What This PR Does

This PR implements proper OpenAPI server URL generation in the Gateway,
ensuring that the OAS specifications accurately reflect how APIs can be
accessed. This brings the Gateway's behavior in line with the Dashboard
implementation.

### Key Features
  1. Accurate Server URLs for All Versioning Methods
    - URL path versioning: http://host/api/v2
    - Query parameter versioning: http://host/api?version=v2
    - Header versioning: http://host/api (version in header)
  2. Proper Handling of Internal vs External APIs
    - Internal child APIs: Only show base API path with version
    - External child APIs: Show both versioned and direct access paths
  3. Automatic Updates
- When a base API's versioning configuration changes, all child API
server URLs are updated automatically
    - Ensures consistency across all loaded API specifications
  4. Preserves User Customizations
    - User-provided server entries in OpenAPI specs are preserved
    - Only Tyk-generated URLs are updated/regenerated





<!---TykTechnologies/jira-linter starts here-->

### Ticket Details

<details>
<summary>
<a href="https://tyktech.atlassian.net/browse/TT-15473" title="TT-15473"
target="_blank">TT-15473</a>
</summary>

|         |    |
|---------|----|
| Status  | In Dev |
| Summary | [BE] Improve visibility of the API version identifier |

Generated at: 2025-11-05 18:50:32

</details>

<!---TykTechnologies/jira-linter ends here-->
)

<!-- Provide a general summary of your changes in the Title above -->

## Description
This PR adds validation to prevent creating or updating APIs with load
balancing enabled where all targets have weight 0. Previously, this
configuration would be accepted but cause runtime errors (index
out of range) when traffic was sent to the API. The new
RuleLoadBalancingTargets validation rule is added to the
DefaultValidationRuleSet and checks if load balancing is enabled with an
empty targets
list (which occurs when all weights are 0). The validation triggers on
both Classic and OAS API endpoints (POST/PUT) and returns a clear HTTP
400 error: "all load balancing targets have weight 0, at
least one target must have weight > 0". This catches the edge case at
API configuration time rather than failing at request time, improving
the developer experience and preventing invalid configurations
   from being saved.

<!-- Describe your changes in detail -->

## Related Issue

<!-- This project only accepts pull requests related to open issues. -->
<!-- If suggesting a new feature or change, please discuss it in an
issue first. -->
<!-- If fixing a bug, there should be an issue describing it with steps
to reproduce. -->
<!-- OSS: Please link to the issue here. Tyk: please create/link the
JIRA ticket. -->

## Motivation and Context

<!-- Why is this change required? What problem does it solve? -->

## How This Has Been Tested

<!-- Please describe in detail how you tested your changes -->
<!-- Include details of your testing environment, and the tests -->
<!-- you ran to see how your change affects other areas of the code,
etc. -->
<!-- This information is helpful for reviewers and QA. -->

## Screenshots (if appropriate)

## Types of changes

<!-- What types of changes does your code introduce? Put an `x` in all
the boxes that apply: -->

- [ ] 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

<!-- Go over all the following points, and put an `x` in all the boxes
that apply -->
<!-- If there are no documentation updates required, mark the item as
checked. -->
<!-- Raise up any additional concerns not covered by the 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


<!---TykTechnologies/jira-linter starts here-->

### Ticket Details

<details>
<summary>
<a href="https://tyktech.atlassian.net/browse/TT-15595" title="TT-15595"
target="_blank">TT-15595</a>
</summary>

|         |    |
|---------|----|
| Status  | In Dev |
| Summary | Temporarily remove upstream target from load balancing |

Generated at: 2025-11-07 15:07:19

</details>

<!---TykTechnologies/jira-linter ends here-->
<!-- Provide a general summary of your changes in the Title above -->

## Description
This PR fixes TT-14359 by adding support for nested JWT claims in the
JWTIdentityBaseField and JWTPolicyFieldName configuration fields,
allowing users to specify claim paths like user.id or
authorization.policy to extract values from nested JSON structures in
JWT tokens. The implementation maintains full backward compatibility by
first checking for literal claim keys (including keys that
contain actual dots in their names) before attempting nested lookup, and
includes proper fallback behavior to the sub claim when nested identity
fields are not found. The changes extend the existing
nestedMapLookup function (previously only used for scopes) to also
support identity base field and policy field resolution, with
comprehensive test coverage including single-level nesting, multi-level
nesting, fallback scenarios, empty string handling, non-string value
handling, multiple policy claim priority, backward compatibility
verification, and the specific customer scenario from the ticket
  where claims are nested under a test object.

<!-- Describe your changes in detail -->

## Related Issue

<!-- This project only accepts pull requests related to open issues. -->
<!-- If suggesting a new feature or change, please discuss it in an
issue first. -->
<!-- If fixing a bug, there should be an issue describing it with steps
to reproduce. -->
<!-- OSS: Please link to the issue here. Tyk: please create/link the
JIRA ticket. -->

## Motivation and Context

<!-- Why is this change required? What problem does it solve? -->

## How This Has Been Tested

<!-- Please describe in detail how you tested your changes -->
<!-- Include details of your testing environment, and the tests -->
<!-- you ran to see how your change affects other areas of the code,
etc. -->
<!-- This information is helpful for reviewers and QA. -->

## Screenshots (if appropriate)

## Types of changes

<!-- What types of changes does your code introduce? Put an `x` in all
the boxes that apply: -->

- [ ] 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

<!-- Go over all the following points, and put an `x` in all the boxes
that apply -->
<!-- If there are no documentation updates required, mark the item as
checked. -->
<!-- Raise up any additional concerns not covered by the 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


<!---TykTechnologies/jira-linter starts here-->

### Ticket Details

<details>
<summary>
<a href="https://tyktech.atlassian.net/browse/TT-14359" title="TT-14359"
target="_blank">TT-14359</a>
</summary>

|         |    |
|---------|----|
| Status  | In Refinement |
| Summary | Support nested locations for JWT policy and subject claims |

Generated at: 2025-11-07 11:26:52

</details>

<!---TykTechnologies/jira-linter ends here-->
…PI (#7532)

## Description

Exposese `GenerateTykServers` so that Dashboard can use it when
generating the tyk OAS urls and return them via
/api/apis/oas/{:apiId}/urls implemented here
TykTechnologies/tyk-analytics#5030

## Related Issue

<!-- This project only accepts pull requests related to open issues. -->
<!-- If suggesting a new feature or change, please discuss it in an
issue first. -->
<!-- If fixing a bug, there should be an issue describing it with steps
to reproduce. -->
<!-- OSS: Please link to the issue here. Tyk: please create/link the
JIRA ticket. -->

## Motivation and Context

<!-- Why is this change required? What problem does it solve? -->

## How This Has Been Tested

<!-- Please describe in detail how you tested your changes -->
<!-- Include details of your testing environment, and the tests -->
<!-- you ran to see how your change affects other areas of the code,
etc. -->
<!-- This information is helpful for reviewers and QA. -->

## Screenshots (if appropriate)

## Types of changes

<!-- What types of changes does your code introduce? Put an `x` in all
the boxes that apply: -->

- [ ] 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

<!-- Go over all the following points, and put an `x` in all the boxes
that apply -->
<!-- If there are no documentation updates required, mark the item as
checked. -->
<!-- Raise up any additional concerns not covered by the 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


<!---TykTechnologies/jira-linter starts here-->

### Ticket Details

<details>
<summary>
<a href="https://tyktech.atlassian.net/browse/TT-16109" title="TT-16109"
target="_blank">TT-16109</a>
</summary>

|         |    |
|---------|----|
| Status  | In Dev |
| Summary | Add a new Dashboard API endpoint to retrieve Tyk-generated
URLs from Tyk OAS API definitions |

Generated at: 2025-11-11 12:05:08

</details>

<!---TykTechnologies/jira-linter ends here-->
…e-to-policy mapping is used (#7504)

<!-- Provide a general summary of your changes in the Title above -->

## Description
- Added tests to validate JWT authentication behavior with only
scope-to-policy mapping and no default policies.
- Included scenarios for valid and invalid scopes, as well as cases with
multiple scopes.
- Enhanced existing JWT processing logic to ensure proper handling of
cases with no default policies configured.
- Ensured backward compatibility with existing behavior when default
policies are present.

This update improves test coverage and ensures the system behaves as
expected under various JWT configurations.


<!-- Describe your changes in detail -->

## Related Issue

<!-- This project only accepts pull requests related to open issues. -->
<!-- If suggesting a new feature or change, please discuss it in an
issue first. -->
<!-- If fixing a bug, there should be an issue describing it with steps
to reproduce. -->
<!-- OSS: Please link to the issue here. Tyk: please create/link the
JIRA ticket. -->
https://tyktech.atlassian.net/browse/TT-6613
## Motivation and Context
https://tyktech.atlassian.net/browse/TT-6613
<!-- Why is this change required? What problem does it solve? -->

## How This Has Been Tested

<!-- Please describe in detail how you tested your changes -->
<!-- Include details of your testing environment, and the tests -->
<!-- you ran to see how your change affects other areas of the code,
etc. -->
<!-- This information is helpful for reviewers and QA. -->
Manual testing and integration tests
## Screenshots (if appropriate)

## Types of changes

<!-- What types of changes does your code introduce? Put an `x` in all
the boxes that apply: -->

- [ ] 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

<!-- Go over all the following points, and put an `x` in all the boxes
that apply -->
<!-- If there are no documentation updates required, mark the item as
checked. -->
<!-- Raise up any additional concerns not covered by the 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





<!---TykTechnologies/jira-linter starts here-->

### Ticket Details

<details>
<summary>
<a href="https://tyktech.atlassian.net/browse/TT-6613" title="TT-6613"
target="_blank">TT-6613</a>
</summary>

|         |    |
|---------|----|
| Status  | Ready for Testing |
| Summary | JWT authentication should not require a base policy if
scope-to-policy mapping is used |

Generated at: 2025-11-11 11:58:13

</details>

<!---TykTechnologies/jira-linter ends here-->
@Razeen-Abdal-Rahman Razeen-Abdal-Rahman merged commit 56859de into fix/pipline-build-errors-TT-16060 Nov 11, 2025
44 of 46 checks passed
@github-actions
Copy link
Copy Markdown
Contributor

🚨 Jira Linter Failed

Commit: 6cc0c35
Failed at: 2025-11-11 15:33:26 UTC

The Jira linter failed to validate your PR. Please check the error details below:

🔍 Click to view error details
failed to validate branch and PR title rules: branch name 'master' must contain a valid Jira ticket ID (e.g., ABC-123)

Next Steps

  • Ensure your branch name contains a valid Jira ticket ID (e.g., ABC-123)
  • Verify your PR title matches the branch's Jira ticket ID
  • Check that the Jira ticket exists and is accessible

This comment will be automatically deleted once the linter passes.

@github-actions
Copy link
Copy Markdown
Contributor

API Changes

no api changes detected

@probelabs
Copy link
Copy Markdown
Contributor

probelabs Bot commented Nov 11, 2025

🔍 Code Analysis Results

This pull request, despite its title, is not a simple synchronization but a major feature and enhancement release. It introduces significant improvements to OAS API management, JWT authentication, and mTLS certificate handling.

Files Changed Analysis

The changes are substantial, with 6 new files added and 16 files modified. The vast majority of the 6266 new lines are dedicated to new functionality and comprehensive test suites, particularly for OAS server regeneration and JWT nested claims. The changes are concentrated in core gateway components: apidef (API definitions), gateway (core logic, middleware), and internal/crypto (security helpers).

  • New Functionality: The addition of files like apidef/oas/servers_regeneration.go, gateway/api_oas_servers.go, and gateway/mw_jwt_nested_claims_test.go signals the introduction of entirely new, complex features.
  • High Impact: Modifications to critical files like gateway/api.go, gateway/mw_jwt.go, and gateway/cert.go indicate that this PR has a wide-ranging impact on the gateway's behavior.

Architecture & Impact Assessment

What this PR accomplishes

This PR delivers several key enhancements:

  1. Robust OAS Server Management: Implements a new engine for intelligently regenerating the servers block in OAS API definitions, preserving user-defined URLs while correctly updating Tyk-generated ones based on API configuration (including versioning).
  2. Flexible JWT Claim Handling: Adds support for nested claims (using dot notation) for identity (JWTIdentityBaseField) and policy (JWTPolicyFieldName), allowing integration with more complex JWT structures from identity providers.
  3. Improved JWT Scope-Based Authorization: Removes the requirement for a JWTDefaultPolicies when using scope-to-policy mapping, enabling purely scope-based authorization. Includes a security fix to prevent privilege escalation on existing sessions.
  4. Enhanced mTLS Certificate Matching: Improves the logic for selecting upstream mTLS certificates, correctly handling hosts with and without ports for both exact and wildcard matches.
  5. Correct mTLS Certificate Chain Handling: Fixes an issue where leaf certificates from a client certificate chain were incorrectly added to the CA pool, ensuring proper validation of intermediate CAs.

Key Technical Changes

  • OAS Server Regeneration Engine: A new module (apidef/oas/servers_regeneration.go) provides a stateful mechanism to calculate the expected Tyk-managed server URLs based on the old and new API definitions. This logic is integrated into the API create/update/patch endpoints in gateway/api.go and gateway/api_oas_servers.go.

    graph TD
        subgraph OAS API Update/Create
            A[API Request] --> B{Is OAS API?};
            B -- Yes --> C[Regenerate Servers Logic];
        end
    
        subgraph C [Server Regeneration]
            C1[1. Compute Old Tyk URLs from previous spec]
            C2[2. Remove old URLs, preserving user-added URLs]
            C3[3. Generate new Tyk URLs based on current spec]
            C4[4. Merge new Tyk URLs and user URLs]
            C1 --> C2 --> C3 --> C4 --> C5[Updated OAS Spec]
        end
    
        subgraph C3
            D{API Type?}
            D -- Versioned --> E[generateVersionedServers]
            D -- Standard --> F[generateStandardServers]
        end
    
        C --> G{Base API Versioning Changed?}
        G -- Yes --> H[Cascade update to child APIs]
    
    Loading
  • Nested JWT Claim Lookup: The JWT middleware (gateway/mw_jwt.go) now uses a getClaimValue helper that first attempts a literal key match (for backward compatibility with keys containing dots) and then falls back to a nested map lookup if the key contains dot notation.

    graph TD
        A[Get Claim Field Name e.g., "user.id"] --> B{Lookup literal key "user.id"};
        B -- Found --> C[Return Value];
        B -- Not Found --> D{Perform Nested Lookup for "user", then "id"};
        D -- Found --> C;
        D -- Not Found --> E[Not Found / Fallback];
    
    Loading

Affected System Components

  • API Management: The entire lifecycle of OAS-based APIs is affected, from creation and updates to how they are exposed via the gateway.
  • JWT Authentication Middleware: Core authentication logic is enhanced to be more flexible and secure.
  • mTLS & Upstream Security: The reliability of mutual TLS authentication is improved, both for client-to-gateway and gateway-to-upstream connections.

Scope Discovery & Context Expansion

The scope of this PR is extensive and touches multiple core functionalities:

  • OAS API Versioning: The server regeneration logic is deeply integrated with API versioning. Changes to a base API's versioning configuration (e.g., changing from URL path to query param versioning) will now automatically trigger updates to the server URLs of all its child APIs.
  • JWT Identity Providers: The support for nested claims significantly broadens the range of identity providers and token formats that can be used with Tyk out-of-the-box, reducing the need for custom middleware to flatten claims.
  • Policy Management: The changes to JWT scope mapping provide a more intuitive way to manage authorization. Users can now rely solely on scopes to grant access without needing a placeholder default policy, simplifying configuration.
  • Certificate Management: The fixes to mTLS chain handling (internal/crypto/helpers.go) and upstream certificate matching (gateway/cert.go) will resolve subtle but critical bugs for users employing advanced mTLS configurations.
Metadata
  • Review Effort: 5 / 5
  • Primary Label: enhancement

Powered by Visor from Probelabs

Last updated: 2025-11-11T15:38:10.939Z | Triggered by: opened | Commit: 6cc0c35

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

@probelabs
Copy link
Copy Markdown
Contributor

probelabs Bot commented Nov 11, 2025

🔍 Code Analysis Results

Security Issues (1)

Severity Location Issue
🟡 Warning internal/crypto/helpers.go:185-193
The function `AddCACertificatesFromChainToPool` adds single certificates to a certificate pool without verifying if they are CA certificates (i.e., the `IsCA` flag is not checked). While the comment mentions this is for backward compatibility and certificate pinning, it poses a security risk when this function is used to build a CA pool for mutual TLS (mTLS). If a user misconfigures a non-CA end-entity certificate for mTLS client validation, the gateway will incorrectly trust it as a Certificate Authority. This could allow an attacker who possesses the private key for that non-CA certificate to sign new certificates and bypass mTLS authentication.
💡 SuggestionWhen handling a single certificate, consider checking if `certToAdd.IsCA` is true before adding it to the pool, especially when the pool is intended for client certificate validation. If non-CA certificates must be supported for pinning, consider separating the logic for building pinning pools from mTLS CA pools, or at a minimum, log a prominent warning when a non-CA certificate is added to a CA trust pool.

Architecture Issues (1)

Severity Location Issue
🟡 Warning gateway/mw_jwt.go:388-424
The log messages at lines 388 and 424 have been changed to use a question mark `?` instead of an em dash `—` (e.g., "... URL %s ? ignoring"). This makes the log message's intent unclear and appears to be an unintentional typo.
💡 SuggestionRevert the question marks `?` back to em dashes `—` or rephrase the log messages for clarity. For example, use `... — ignoring` or `..., ignoring.`.

Performance Issues (1)

Severity Location Issue
🟡 Warning gateway/api_oas_servers.go:108-138
When a base API with a large number of versioned child APIs is updated, the `updateChildAPIsServersGW` function sequentially updates each child API. This could lead to a significant delay in the API update operation, as it involves loading and regenerating the OAS spec for every child. For a base API with hundreds of versions, this synchronous loop could become a performance bottleneck for configuration changes.
💡 SuggestionConsider parallelizing the updates for child APIs. A pool of worker goroutines or simply spawning a goroutine for each child update (with a `sync.WaitGroup` to wait for completion) could significantly reduce the total time taken for the base API update operation. Care must be taken to ensure that any shared resources accessed during the update are handled in a thread-safe manner.

Quality Issues (3)

Severity Location Issue
🟢 Info gateway/mw_jwt.go:387
Log message contains a stray question mark, which appears to be a typo or merge conflict artifact.
💡 SuggestionRemove the stray '?' from the log message for clarity.
🟢 Info gateway/mw_jwt.go:424
Log message contains a stray question mark, which appears to be a typo or merge conflict artifact.
💡 SuggestionRemove the stray '?' from the log message for clarity.
🟡 Warning internal/crypto/helpers.go:178-194
The function `AddCACertificatesFromChainToPool` has inconsistent behavior for single certificates versus certificate chains. For a single certificate, it is added to the pool regardless of its `IsCA` flag, which is intended for backward compatibility and pinning. However, this could lead to non-CA certificates being added to a CA pool, potentially weakening trust verification if used incorrectly. The debug log message also doesn't clearly distinguish this special handling.
💡 SuggestionConsider making the behavior more explicit. Either strictly enforce the `IsCA` flag for all cases, or improve the logging to clearly indicate when a non-CA certificate is being added to the pool for a specific reason like pinning or backward compatibility. For example: `logrus.Debug("Added single certificate to pool (pinning/compatibility)")`.

Dependency Issues (7)

Severity Location Issue
🔴 Critical apidef/oas/servers_regeneration.go:1
The introduction of a new, complex server regeneration logic for OAS APIs is a significant behavioral change that impacts downstream tools like tyk-operator and the Tyk Portal. The gateway now actively manages the `servers` array, preserving user-defined entries while generating its own, especially for versioned APIs. Downstream tools that read or modify the `servers` block must be updated to align with this new logic to prevent conflicts, data loss, or displaying incorrect API URLs.
🟠 Error gateway/mw_jwt.go:724
The validation logic for JWT-enabled APIs has changed. A default policy (`JWTDefaultPolicies`) is no longer required if a scope-to-policy mapping is configured. The `tyk-operator`'s validation webhook must be updated to reflect this relaxed validation, otherwise it will incorrectly reject valid `ApiDefinition` custom resources.
🟠 Error apidef/validator.go:253
A new validation rule has been added that rejects API definitions where load balancing is enabled but all targets have a weight of 0 (resulting in an empty target list). This is a breaking validation change that affects `tyk-operator` and `tyk-sink`. The operator's validation webhook and sink's pre-sync validation should be updated to incorporate this rule.
🟡 Warning gateway/mw_jwt.go:635
Support for nested JWT claims (using dot-notation for `JWTIdentityBaseField` and `JWTPolicyFieldName`) has been added. Downstream tools like `tyk-operator` need to be updated to support and document this feature in their respective schemas (e.g., the `ApiDefinition` CRD) to ensure feature parity.
🟡 Warning internal/crypto/helpers.go:156
The logic for handling client certificate chains for mTLS (`AddCACertificatesFromChainToPool`) has been corrected to only add actual CA certificates to the trust pool, skipping the leaf certificate. This is a behavioral change that fixes a bug but could impact users who were relying on the previous incorrect behavior. This change should be clearly communicated in release notes for all dependent projects.
🟡 Warning gateway/mw_jwt.go:388-424
The log messages on lines 388 and 424 contain a '?' character which appears to be a typo and should likely be a dash '—' or removed for clarity. For example: '...URL %s ? ignoring'.
💡 SuggestionReplace the '?' with '—' to match other log message styles or rephrase to avoid ambiguity.
🟡 Warning internal/crypto/helpers.go:156
The function `AddCACertificatesFromChainToPool` is misleading. While it correctly adds only CA certificates from a chain (length > 1), for a single provided certificate (length == 1), it adds the certificate to the pool regardless of whether it is a CA or not. The comment mentions this is for backward compatibility, but the function name implies only CAs are ever added.
💡 SuggestionConsider renaming the function to better reflect its behavior (e.g., `AddTrustAnchorsFromCertificate`) or update the function's documentation to be very explicit about the exceptional behavior for single, non-CA certificates and the risks of misconfiguration.

✅ Connectivity Check Passed

No connectivity issues found – changes LGTM.


Powered by Visor from Probelabs

Last updated: 2025-11-11T15:38:12.010Z | Triggered by: opened | Commit: 6cc0c35

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

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.

7 participants