feat(router): add TLS support for NATS event source#2749
feat(router): add TLS support for NATS event source#2749vatsalpatel wants to merge 8 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
# Conflicts: # router/pkg/config/config.go # router/pkg/config/config.schema.json # router/pkg/config/testdata/config_full.json
vatsalpatel
requested review from
Noroth,
SkArchon,
StarpTech,
alepane21,
devsergiy,
dkorittki,
endigma and
jensneuse
as code owners
WalkthroughAdds TLS support for NATS event sources: new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 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 |
|
Docs PR: wundergraph/cosmo-docs#261 |
Hi @vatsalpatel, |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
router/pkg/config/config.schema.json (1)
2553-2579: Add schema-level dependency rules for mTLS fields.Consider enforcing
cert_file/key_filepairing andca_filerequirement directly in schema so invalid TLS combos fail at config validation time (not only at startup runtime).🧩 Proposed schema refinement
"tls": { "type": "object", "description": "TLS configuration for the NATS provider.", "additionalProperties": false, "properties": { "insecure_skip_verify": { "type": "boolean", "default": false, "description": "Skip server certificate verification. Not recommended for production use." }, "ca_file": { "type": "string", "description": "Path to a custom CA certificate file (PEM) used to verify the server certificate.", "format": "file-path" }, "cert_file": { "type": "string", "description": "Path to the client certificate file (PEM) for mTLS.", "format": "file-path" }, "key_file": { "type": "string", "description": "Path to the client private key file (PEM) for mTLS.", "format": "file-path" } - } + }, + "dependentRequired": { + "cert_file": ["key_file"], + "key_file": ["cert_file"] + }, + "allOf": [ + { + "if": { + "anyOf": [ + { "required": ["cert_file"] }, + { "required": ["key_file"] } + ] + }, + "then": { + "required": ["ca_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 2553 - 2579, The TLS object currently allows cert_file/key_file/ca_file independently; add schema-level rules under the "tls" object to enforce mTLS constraints: add an allOf (or anyOf) clause that requires both "cert_file" and "key_file" to be present together (e.g., an anyOf with {not having either} OR {having both}), and add a rule that if either "cert_file" or "key_file" is present then "ca_file" is required (use an if/then with "if": {"anyOf":[{"required":["cert_file"]},{"required":["key_file"]}]} and "then": {"required":["ca_file"]}). Reference the "tls" object and the property names "cert_file", "key_file", and "ca_file" when applying these changes so invalid mTLS combos fail validation.
🤖 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/config/config.schema.json`:
- Around line 2553-2579: The TLS object currently allows
cert_file/key_file/ca_file independently; add schema-level rules under the "tls"
object to enforce mTLS constraints: add an allOf (or anyOf) clause that requires
both "cert_file" and "key_file" to be present together (e.g., an anyOf with {not
having either} OR {having both}), and add a rule that if either "cert_file" or
"key_file" is present then "ca_file" is required (use an if/then with "if":
{"anyOf":[{"required":["cert_file"]},{"required":["key_file"]}]} and "then":
{"required":["ca_file"]}). Reference the "tls" object and the property names
"cert_file", "key_file", and "ca_file" when applying these changes so invalid
mTLS combos fail validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: df69fa22-8ef3-4bec-91ea-391409508520
📒 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
|
Updated docs-website @alepane21 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs-website/router/configuration.mdx`:
- Around line 1753-1769: Update the TLS field descriptions so they explicitly
state runtime validation rules: in the table entries for tls.cert_file and
tls.key_file mention that cert_file and key_file must be provided together (both
required for mTLS) and that tls.ca_file is also required whenever either
cert_file or key_file is set; also update tls.ca_file description to note it is
required when using cert_file/key_file for mutual TLS. Ensure references to
tls.cert_file, tls.key_file and tls.ca_file are clear so readers know the valid
combinations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22027364-1964-485f-8a2c-5def3c5d3284
📒 Files selected for processing (1)
docs-website/router/configuration.mdx
Don't worry, I did it! Thanks again. |
|
Hey @vatsalpatel, thanks for your contribution! Just wanted to let you know I am looking into your pull request and will provide feedback very soon. |
dkorittki
left a comment
dkorittki
left a comment
There was a problem hiding this comment.
Thanks again for contribution, really appreciate it! The implementation looks good and works as expected. I left some comments on areas which need some touches.
In general what it lacks are some integration tests, which verify tls works against a real nats server instance. The right place to do this would be the router-tests module but I think it's asked too much for, since it would become rather complex. I will brainstorm a few ideas on how to solve this but meantime I want to provide some feedback to you already
There was a problem hiding this comment.
🧹 Nitpick comments (1)
router/pkg/pubsub/nats/provider_builder.go (1)
121-123: Add explicit TLS minimum version for deterministic security posture.The
tls.Configat line 121 relies on Go's defaultMinVersion(TLS 1.2), which may change across Go runtime versions. For production NATS connections, explicitly setMinVersion: tls.VersionTLS13to enforce consistent security guarantees.Suggested change
tlsCfg := &tls.Config{ + MinVersion: tls.VersionTLS13, InsecureSkipVerify: eventSource.TLS.InsecureSkipVerify, }🤖 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 121 - 123, The tls.Config construction in provider_builder.go currently sets only InsecureSkipVerify via tlsCfg and relies on Go's default MinVersion; update the tlsCfg in the build path that creates tls.Config (the tlsCfg variable used for NATS connections) to explicitly set MinVersion: tls.VersionTLS13 alongside InsecureSkipVerify (using eventSource.TLS.InsecureSkipVerify) so the NATS client enforces TLS 1.3 minimum.
🤖 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.go`:
- Around line 121-123: The tls.Config construction in provider_builder.go
currently sets only InsecureSkipVerify via tlsCfg and relies on Go's default
MinVersion; update the tlsCfg in the build path that creates tls.Config (the
tlsCfg variable used for NATS connections) to explicitly set MinVersion:
tls.VersionTLS13 alongside InsecureSkipVerify (using
eventSource.TLS.InsecureSkipVerify) so the NATS client enforces TLS 1.3 minimum.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7fb3e057-98c3-44a8-8085-ecd6a43bfbdd
📒 Files selected for processing (5)
docs-website/router/configuration.mdxrouter/pkg/config/config.schema.jsonrouter/pkg/config/config_events_nats_test.gorouter/pkg/pubsub/nats/provider_builder.gorouter/pkg/pubsub/nats/provider_builder_test.go
✅ Files skipped from review due to trivial changes (2)
- docs-website/router/configuration.mdx
- router/pkg/pubsub/nats/provider_builder_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- router/pkg/config/config.schema.json
- router/pkg/config/config_events_nats_test.go
|
Hey @dkorittki
For the integration test, I did some research and there are 2 options we can go for
opts := &server.Options{
Host: "127.0.0.1",
Port: -1,
NoLog: true,
}
s := natstest.RunServer(opts)
t.Cleanup(s.Shutdown)
s.ReadyForConnections(5 * time.Second)
I think option 1 seems better, however we will have to add Also FYI, looks like the services:
nats:
image: ghcr.io/wundergraph/cosmo/nats:2.11.0-alpine |
3 participants
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
New Features
Tests
Documentation
Checklist