Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nsqd: use --tls-root-ca-file in nsqauth request #1473

Merged

Conversation

intellitrend-team
Copy link
Contributor

This change allows the parameter --tls-root-ca-file to set the root CA file for auth requests as well.

tlsConfig.RootCAs = tlsCertPool
}

return tlsConfig, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Shouldn't tlsConfig be nil if opts.TLSRootCAFile is unspecified?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our intention was to respect the existing TLSMinVersion flag for clients as well, which is why we're passing a tls.Config with MinVersion set to the flag. This behavior was inspired by the very similar buildTLSConfig function for assembling the tls.Config for the server:

MinVersion: opts.TLSMinVersion,

The documentation for tls.Config in Go 1.17 the current go version specified in go.mod specifies TLS 1.0 as the default minimum versions (which has been deprecated by RFC 8996, however the latest Go version (1.21.3) sets TLS 1.2 as the minimum for TLS clients per default (https://pkg.go.dev/crypto/tls@go1.21.3#Config). Because of this, we would want to respect the existing config option in Go 1.17. Alternatively, the project could be upgraded to Go 1.21 to ensure that a secure TLS version is used.

Because (according to the documentation) the zero value of tls.Config should work in the same way its nil value does, this should not affect any other TLS options.

@mreiferson
Copy link
Member

LGTM, thanks!

@mreiferson mreiferson merged commit c3941e1 into nsqio:master Dec 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants