Skip to content

feat: additional NATS auth options#1315

Open
mfreeman451 wants to merge 4 commits intofalcosecurity:masterfrom
mfreeman451:feature/nats-enhanced-auth
Open

feat: additional NATS auth options#1315
mfreeman451 wants to merge 4 commits intofalcosecurity:masterfrom
mfreeman451:feature/nats-enhanced-auth

Conversation

@mfreeman451
Copy link

@mfreeman451 mfreeman451 commented Mar 3, 2026

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area build

/area config

/area outputs

/area tests

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #1316

Special notes for your reviewer:

@poiana
Copy link

poiana commented Mar 3, 2026

Welcome @mfreeman451! It looks like this is your first PR to falcosecurity/falcosidekick 🎉

@poiana poiana requested a review from fjogeleit March 3, 2026 05:20
@poiana
Copy link

poiana commented Mar 3, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mfreeman451
Once this PR has been reviewed and has the lgtm label, please assign issif for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@poiana poiana requested a review from leogr March 3, 2026 05:20
Copy link
Author

@mfreeman451 mfreeman451 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Signed-off-by: Michael Freeman <mfreeman451@gmail.com>
Signed-off-by: Michael Freeman <mfreeman451@gmail.com>
Signed-off-by: Michael Freeman <mfreeman451@gmail.com>
Signed-off-by: Michael Freeman <mfreeman451@gmail.com>
@mfreeman451
Copy link
Author

/kind feature

@poiana poiana added kind/feature New feature or request and removed needs-kind labels Mar 3, 2026
@mfreeman451
Copy link
Author

/area outputs

@mfreeman451 mfreeman451 marked this pull request as ready for review March 3, 2026 06:08
@poiana poiana requested a review from cpanato March 3, 2026 06:08

hostPort := strings.TrimSpace(config.Nats.HostPort)
tlsRequested := cfg.MutualTLS || !cfg.CheckCert || config.TLSClient.CaCertFile != "" ||
strings.HasPrefix(hostPort, "natss://") || strings.HasPrefix(hostPort, "tls://")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is natss:// a valid scheme? 🤔

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, will cleanup


hostPort := strings.TrimSpace(config.Nats.HostPort)
tlsRequested := cfg.MutualTLS || !cfg.CheckCert || config.TLSClient.CaCertFile != "" ||
strings.HasPrefix(hostPort, "natss://") || strings.HasPrefix(hostPort, "tls://")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
strings.HasPrefix(hostPort, "natss://") || strings.HasPrefix(hostPort, "tls://")
strings.HasPrefix(hostPort, "tls://")

natss:// is not a standard NATS URI scheme. The NATS Go client recognizes nats:// and tls://. While our NewClient regex happens to accept natss://, using it here as a TLS trigger could confuse users into thinking it's supported.

What was the intent behind natss://? If it's for symmetry with https://, could we just document tls:// as the way to enable TLS?

}

func validateNatsAuthFile(path, configKey string) error {
_, err := os.ReadFile(path)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
_, err := os.ReadFile(path)
_, err := os.Stat(path)

os.ReadFile loads the entire file into memory just to check if it's readable. os.Stat would be sufficient here and is the pattern used elsewhere in the codebase (e.g., outputs/logstash.go:45).

Comment on lines 331 to 333
}

}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: extra blank line before the closing brace.

Suggested change
}
}
}
}

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.

Missing NATS authentication options

3 participants