Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds per-subgraph TLS/mTLS support for outbound connections, including configuration types and schema, TLS assembly logic, router/transport plumbing to apply TLS configs, test-server TLS support, unit tests for TLS builders, and integration tests validating subgraph mTLS behaviors. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
SkArchon
force-pushed
the
milinda/mTLS
branch
from
06ffa7c to
dd8b8ef
Compare
SkArchon
requested review from
Noroth,
StarpTech,
devsergiy,
endigma and
jensneuse
as code owners
Router-nonroot image scan passed✅ No security vulnerabilities found in image: |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2514 +/- ##
==========================================
+ Coverage 62.21% 62.77% +0.56%
==========================================
Files 241 242 +1
Lines 25499 25560 +61
==========================================
+ Hits 15864 16046 +182
+ Misses 8298 8178 -120
+ Partials 1337 1336 -1
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@router/core/router.go`:
- Around line 204-206: The tls.Config created in router/core/router.go
(tlsConfig) lacks a MinVersion, allowing downgrades; set tlsConfig.MinVersion =
tls.VersionTLS13 (or tls.VersionTLS12 if older subgraphs require it) when
constructing tlsConfig (the block that uses
clientCfg.InsecureSkipCaVerification) so outbound subgraph client connections
enforce a minimum TLS version.
In `@router/pkg/config/config.schema.json`:
- Around line 455-469: The schema's global TLS client cert block uses
"certificate_authority" but runtime expects "ca_file"; update the TLS global
object (tls.subgraph.all / the TLSClientCertConfiguration schema) to replace the
"certificate_authority" property name with "ca_file" (keeping type "string",
format "file-path" and the description about verifying subgraph server
certificates and falling back to system root CAs) so the schema field matches
the runtime config name and the global CA will be applied correctly.
🧹 Nitpick comments (1)
router/core/router_test.go (1)
424-455: Close test cert files and surface Close errors.The helper writes PEM files but ignores Close errors. If a Close fails, tests may read a partial file or leak descriptors. Consider deferring Close and asserting errors.
💡 Proposed fix
certPath = filepath.Join(dir, prefix+".crt") certFile, err := os.Create(certPath) require.NoError(t, err) + defer func() { assert.NoError(t, certFile.Close()) }() require.NoError(t, pem.Encode(certFile, &pem.Block{Type: "CERTIFICATE", Bytes: certDER})) - certFile.Close() keyPath = filepath.Join(dir, prefix+".key") keyFile, err := os.Create(keyPath) require.NoError(t, err) + defer func() { assert.NoError(t, keyFile.Close()) }() keyDER, err := x509.MarshalECPrivateKey(key) require.NoError(t, err) require.NoError(t, pem.Encode(keyFile, &pem.Block{Type: "EC PRIVATE KEY", Bytes: keyDER})) - keyFile.Close()
Contributor
There was a problem hiding this comment.
🤖 Fix all issues with AI agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@router/core/tls_test.go`:
- Around line 73-83: Add a new unit test in tls_test.go that exercises
buildTLSClientConfig with a partial TLSClientCertConfiguration: call
generateTestCert(t, "client") to get certPath and keyPath, then invoke
buildTLSClientConfig twice—once with only CertificateChain set (no Key) and once
with only Key set (no CertificateChain)—and assert that it returns an error or a
nil/empty tls.Config.Certificates as the implementation should handle incomplete
configs; reference the existing test "loads client cert and key", function
buildTLSClientConfig, and type config.TLSClientCertConfiguration when adding
these assertions.
In `@router/core/tls.go`:
- Around line 18-25: The code silently skips loading client cert when
clientCfg.CertificateChain and clientCfg.Key are not both set; update the block
that currently calls tls.LoadX509KeyPair to check for the asymmetric case (one
is set but the other is empty) and emit a clear warning (or return an error if
preferred) referencing clientCfg.CertificateChain and clientCfg.Key so users
know the cert/key pair is incomplete; keep the existing tls.LoadX509KeyPair and
setting of tlsConfig.Certificates when both are present and only add the
conditional warning path before attempting to load.
🧹 Nitpick comments (2)
🤖 Fix all nitpicks with AI agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@router/core/tls_test.go`: - Around line 73-83: Add a new unit test in tls_test.go that exercises buildTLSClientConfig with a partial TLSClientCertConfiguration: call generateTestCert(t, "client") to get certPath and keyPath, then invoke buildTLSClientConfig twice—once with only CertificateChain set (no Key) and once with only Key set (no CertificateChain)—and assert that it returns an error or a nil/empty tls.Config.Certificates as the implementation should handle incomplete configs; reference the existing test "loads client cert and key", function buildTLSClientConfig, and type config.TLSClientCertConfiguration when adding these assertions. In `@router/core/tls.go`: - Around line 18-25: The code silently skips loading client cert when clientCfg.CertificateChain and clientCfg.Key are not both set; update the block that currently calls tls.LoadX509KeyPair to check for the asymmetric case (one is set but the other is empty) and emit a clear warning (or return an error if preferred) referencing clientCfg.CertificateChain and clientCfg.Key so users know the cert/key pair is incomplete; keep the existing tls.LoadX509KeyPair and setting of tlsConfig.Certificates when both are present and only add the conditional warning path before attempting to load.router/core/tls_test.go (1)
73-83: Consider adding a test for partial certificate configuration.The current tests don't cover the case where only
CertificateChainis provided withoutKey(or vice versa). This would help ensure the implementation handles incomplete configurations gracefully.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@router/core/tls_test.go` around lines 73 - 83, Add a new unit test in tls_test.go that exercises buildTLSClientConfig with a partial TLSClientCertConfiguration: call generateTestCert(t, "client") to get certPath and keyPath, then invoke buildTLSClientConfig twice—once with only CertificateChain set (no Key) and once with only Key set (no CertificateChain)—and assert that it returns an error or a nil/empty tls.Config.Certificates as the implementation should handle incomplete configs; reference the existing test "loads client cert and key", function buildTLSClientConfig, and type config.TLSClientCertConfiguration when adding these assertions.router/core/tls.go (1)
18-25: Consider warning when certificate configuration is incomplete.If a user provides
CertificateChainwithoutKey(or vice versa), the configuration is silently ignored. This could lead to confusion during troubleshooting.🛡️ Suggested enhancement
// Load client certificate and key if provided - if clientCfg.CertificateChain != "" && clientCfg.Key != "" { + hasCert := clientCfg.CertificateChain != "" + hasKey := clientCfg.Key != "" + if hasCert != hasKey { + return nil, fmt.Errorf("both certificate_chain and key must be provided together") + } + if hasCert && hasKey { cert, err := tls.LoadX509KeyPair(clientCfg.CertificateChain, clientCfg.Key) if err != nil { return nil, fmt.Errorf("failed to load client TLS cert and key: %w", err) } tlsConfig.Certificates = []tls.Certificate{cert} }🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@router/core/tls.go` around lines 18 - 25, The code silently skips loading client cert when clientCfg.CertificateChain and clientCfg.Key are not both set; update the block that currently calls tls.LoadX509KeyPair to check for the asymmetric case (one is set but the other is empty) and emit a clear warning (or return an error if preferred) referencing clientCfg.CertificateChain and clientCfg.Key so users know the cert/key pair is incomplete; keep the existing tls.LoadX509KeyPair and setting of tlsConfig.Certificates when both are present and only add the conditional warning path before attempting to load.
Contributor
There was a problem hiding this comment.
🤖 Fix all issues with AI agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@router-tests/subgraph_mtls_test.go`:
- Around line 31-40: The test TLS config created in subgraphMTLSServerConfig
does not set MinVersion; update the function to set cfg.MinVersion =
tls.VersionTLS12 (or tls.VersionTLS13) to enforce a modern TLS minimum for the
httptest server, keeping existing behavior for client cert handling and still
using loadSubgraphMTLSCACertPool when requireClientCert is true.
🧹 Nitpick comments (1)
🤖 Fix all nitpicks with AI agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@router-tests/subgraph_mtls_test.go`: - Around line 31-40: The test TLS config created in subgraphMTLSServerConfig does not set MinVersion; update the function to set cfg.MinVersion = tls.VersionTLS12 (or tls.VersionTLS13) to enforce a modern TLS minimum for the httptest server, keeping existing behavior for client cert handling and still using loadSubgraphMTLSCACertPool when requireClientCert is true.router-tests/subgraph_mtls_test.go (1)
31-40: Consider settingMinVersionon test TLS config.Static analysis flags that
MinVersionis not set on thetls.Config. While this is test code with low security risk (local httptest server), settingMinVersion: tls.VersionTLS12ortls.VersionTLS13is a good practice for consistency and to serve as example code.🔧 Proposed fix
func subgraphMTLSServerConfig(t *testing.T, requireClientCert bool) *tls.Config { t.Helper() - cfg := &tls.Config{} + cfg := &tls.Config{ + MinVersion: tls.VersionTLS12, + } if requireClientCert { caPool := loadSubgraphMTLSCACertPool(t, "testdata/tls/cert.pem") cfg.ClientCAs = caPool cfg.ClientAuth = tls.RequireAndVerifyClientCert } return cfg }🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@router-tests/subgraph_mtls_test.go` around lines 31 - 40, The test TLS config created in subgraphMTLSServerConfig does not set MinVersion; update the function to set cfg.MinVersion = tls.VersionTLS12 (or tls.VersionTLS13) to enforce a modern TLS minimum for the httptest server, keeping existing behavior for client cert handling and still using loadSubgraphMTLSCACertPool when requireClientCert is true.
6 tasks
StarpTech
requested changes
Feb 18, 2026
Contributor
There was a problem hiding this comment.
It should also be possible to use a shared certificate for all subgraphs, with the option to override it for individual subgraphs.
tls:
client:
all:
cert_file: "/Users/milindadias/Work/cosmo/router/certs/employee.crt"
key_file: "/Users/milindadias/Work/cosmo/router/certs/employee.key"
subgraphs:
employee:
cert_file: "/Users/milindadias/Work/cosmo/router/certs/employee.crt"
key_file: "/Users/milindadias/Work/cosmo/router/certs/employee.key"
StarpTech
requested changes
Feb 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR adds the following capabilities for communication between the router and subgraph.
The below config is an example configuration
Note: There already exists
tls.serverwhere the router is the "server", thus I went with thetls.clientnaming as in this case the router is the "client" when communicating with subgraphs.Summary by CodeRabbit
New Features
Configuration
Tests
Checklist