feat(router): add TLS support for NATS event source#2608
Closed
vatsalpatel wants to merge 3 commits intowundergraph:mainfrom
Closed
feat(router): add TLS support for NATS event source#2608vatsalpatel wants to merge 3 commits intowundergraph:mainfrom
vatsalpatel wants to merge 3 commits intowundergraph:mainfrom
Conversation
Add a \`tls\` block to \`NatsEventSource\` supporting four modes: no TLS
(omit block), skip verify, custom CA (1-way TLS), and mTLS.
- New \`NatsTLSConfiguration\` struct with \`insecure_skip_verify\`,
\`ca_file\`, \`cert_file\`, and \`key_file\` fields
- TLS option building in \`buildNatsOptions\` via \`nats.Secure(tlsCfg)\`;
cert/key pair validated together, missing CA file surfaced at startup
- \`config.schema.json\` updated with \`tls\` object
(\`additionalProperties: false\`)
- Golden fixture updated
- Tests for all modes and error paths in both \`config_events_nats_test.go\`
and \`provider_builder_test.go\`
Closes wundergraph#2582
vatsalpatel
requested review from
Noroth,
StarpTech,
alepane21,
devsergiy,
dkorittki,
endigma and
jensneuse
as code owners
Contributor
WalkthroughAdds TLS support for NATS event sources: new NatsTLSConfiguration type and optional TLS field on NatsEventSource, JSON schema and fixture updates, TLS-aware option construction in the NATS provider builder (skip-verify, custom CA, mTLS), and unit tests for TLS behaviors and validation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Author
|
Link to the docs PR - wundergraph/cosmo-docs#261 |
![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: 1
🧹 Nitpick comments (1)
router/pkg/config/config.schema.json (1)
2584-2609: Consider addingdependenciesconstraint for cert_file/key_file mutual requirement.Other TLS configurations in this schema (e.g.,
tls.client.allat lines 476-479 andtls.client.subgraphsat lines 509-512) enforce thatcert_fileandkey_filemust be provided together using adependenciesconstraint. The new NATS TLS configuration is missing this, which could allow users to provide an incomplete mTLS configuration that would only be caught at runtime.📋 Suggested schema addition for consistency
"key_file": { "type": "string", "description": "Path to the client private key file (PEM) for mTLS.", "format": "file-path" } + }, + "dependencies": { + "cert_file": ["key_file"], + "key_file": ["cert_file"] } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/config/config.schema.json` around lines 2584 - 2609, The tls object for the NATS provider allows cert_file and key_file to be set independently, which can lead to incomplete mTLS configs; update the tls schema to enforce that cert_file and key_file are provided together by adding a dependencies constraint (or dependentRequired) on the tls object so that "cert_file" requires "key_file" and "key_file" requires "cert_file" (i.e., make cert_file and key_file mutually required under the tls properties).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@router/pkg/pubsub/nats/provider_builder.go`:
- Around line 141-149: The TLS setup currently allows providing
cert_file/key_file without ca_file and silently uses the host trust store;
update the logic around eventSource.TLS handling in provider_builder.go so that
if eventSource.TLS.CertFile or KeyFile is set (i.e., mTLS is being configured)
you validate that eventSource.TLS.CAFile is also non-empty and return an error
(similar format to the existing fmt.Errorf messages) if it is missing; keep the
existing certificate loading (tls.LoadX509KeyPair) and assignment to
tlsCfg.Certificates but add the explicit CA file presence check before
proceeding to ensure private CA verification fails fast rather than during
handshake.
---
Nitpick comments:
In `@router/pkg/config/config.schema.json`:
- Around line 2584-2609: The tls object for the NATS provider allows cert_file
and key_file to be set independently, which can lead to incomplete mTLS configs;
update the tls schema to enforce that cert_file and key_file are provided
together by adding a dependencies constraint (or dependentRequired) on the tls
object so that "cert_file" requires "key_file" and "key_file" requires
"cert_file" (i.e., make cert_file and key_file mutually required under the tls
properties).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: df8eafb0-cfac-43c1-8038-a6c7af8dce1e
📒 Files selected for processing (6)
router/pkg/config/config.gorouter/pkg/config/config.schema.jsonrouter/pkg/config/config_events_nats_test.gorouter/pkg/config/testdata/config_full.jsonrouter/pkg/pubsub/nats/provider_builder.gorouter/pkg/pubsub/nats/provider_builder_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
router/pkg/pubsub/nats/provider_builder_test.go (1)
70-142: Good coverage for TLS validation error paths.The tests effectively verify all failure modes: missing/unreadable CA file, mismatched cert/key, and mTLS without CA. This ensures configuration errors fail fast at startup.
Consider adding a positive test with real (or generated) test certificates to verify the happy path for CA loading and mTLS configuration. This would catch regressions in the certificate parsing logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/pubsub/nats/provider_builder_test.go` around lines 70 - 142, Add a positive integration test to TestBuildNatsOptionsWithTLS that generates temporary test certificates (CA, server/client cert+key), writes them to temp files, and calls buildNatsOptions with TLS configured to use the generated ca_file, cert_file and key_file; assert no error is returned and that returned opts include the TLS option (or that TLS handshake-related option is present) to ensure CA loading and mTLS parsing succeed; use helper functions inside the test to generate and cleanup the temp cert/key files and reference buildNatsOptions and TestBuildNatsOptionsWithTLS to locate where to add the test.router/pkg/pubsub/nats/provider_builder.go (1)
120-156: SetMinVersionon the TLS configuration.The
tls.ConfigomitsMinVersion, allowing negotiation down to TLS 1.0 (server) or 1.2 (client). For a production-grade router, explicitly setting a minimum version hardens the connection against downgrade attacks.Proposed fix
if eventSource.TLS != nil { tlsCfg := &tls.Config{ InsecureSkipVerify: eventSource.TLS.InsecureSkipVerify, + MinVersion: tls.VersionTLS12, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/pubsub/nats/provider_builder.go` around lines 120 - 156, The TLS config created for NATS (tlsCfg in the eventSource.TLS handling) lacks a MinVersion, so set tlsCfg.MinVersion = tls.VersionTLS12 (or tls.VersionTLS13 if you want to require TLS1.3) before passing it to nats.Secure; update the block that builds tlsCfg (the code creating tls.Config, adding RootCAs and Certificates) to explicitly assign MinVersion to enforce a minimum TLS version for the NATS provider.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@router/pkg/pubsub/nats/provider_builder_test.go`:
- Around line 70-142: Add a positive integration test to
TestBuildNatsOptionsWithTLS that generates temporary test certificates (CA,
server/client cert+key), writes them to temp files, and calls buildNatsOptions
with TLS configured to use the generated ca_file, cert_file and key_file; assert
no error is returned and that returned opts include the TLS option (or that TLS
handshake-related option is present) to ensure CA loading and mTLS parsing
succeed; use helper functions inside the test to generate and cleanup the temp
cert/key files and reference buildNatsOptions and TestBuildNatsOptionsWithTLS to
locate where to add the test.
In `@router/pkg/pubsub/nats/provider_builder.go`:
- Around line 120-156: The TLS config created for NATS (tlsCfg in the
eventSource.TLS handling) lacks a MinVersion, so set tlsCfg.MinVersion =
tls.VersionTLS12 (or tls.VersionTLS13 if you want to require TLS1.3) before
passing it to nats.Secure; update the block that builds tlsCfg (the code
creating tls.Config, adding RootCAs and Certificates) to explicitly assign
MinVersion to enforce a minimum TLS version for the NATS provider.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3107542d-24f7-4582-bc66-2a96cff032e4
📒 Files selected for processing (2)
router/pkg/pubsub/nats/provider_builder.gorouter/pkg/pubsub/nats/provider_builder_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
router/pkg/pubsub/nats/provider_builder_test.go (1)
92-92: Consider documenting or extracting the magic number7.The
len(opts) > 7assertion appears in multiple tests (here and in the existing tests at lines 53 and 75). This value represents the count of base NATS options, but ifbuildNatsOptionsadds or removes base options, these assertions become silently incorrect.A small comment documenting what the 7 base options are would improve maintainability.
Also applies to: 165-165, 183-183
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/pubsub/nats/provider_builder_test.go` at line 92, The test uses a magic number 7 when asserting len(opts) > 7; replace that literal with a named constant or computed expected count and document the base options so the assertion stays correct when buildNatsOptions changes. Concretely, introduce a constant like baseNatsOptionsCount (or a helper getBaseNatsOptionsCount()) and use require.Greater(t, len(opts), baseNatsOptionsCount) (or require.Greater(t, len(opts), baseNatsOptionsCount+1) for the TLS case), and add a brief comment listing the base option names to explain what the constant represents; update all occurrences that currently use the literal 7 and reference buildNatsOptions and the opts variable when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@router/pkg/pubsub/nats/provider_builder_test.go`:
- Line 92: The test uses a magic number 7 when asserting len(opts) > 7; replace
that literal with a named constant or computed expected count and document the
base options so the assertion stays correct when buildNatsOptions changes.
Concretely, introduce a constant like baseNatsOptionsCount (or a helper
getBaseNatsOptionsCount()) and use require.Greater(t, len(opts),
baseNatsOptionsCount) (or require.Greater(t, len(opts), baseNatsOptionsCount+1)
for the TLS case), and add a brief comment listing the base option names to
explain what the constant represents; update all occurrences that currently use
the literal 7 and reference buildNatsOptions and the opts variable when making
the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2dfd87a2-499a-4968-b82b-6c8e58991a44
📒 Files selected for processing (1)
router/pkg/pubsub/nats/provider_builder_test.go
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
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
1 participant
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.
Add a
tlsblock toNatsEventSourcesupporting four modes: no TLS (omit block), skip verify, custom CA (1-way TLS), and mTLS.NatsTLSConfigurationstruct withinsecure_skip_verify,ca_file,cert_file, andkey_filefieldsbuildNatsOptionsvianats.Secure(tlsCfg); cert/key pair validated together, missing CA file surfaced at startupconfig.schema.jsonupdated withtlsobject (additionalProperties: false)config_events_nats_test.goandprovider_builder_test.goCloses #2582
Summary by CodeRabbit
Checklist