feat: add TLS certificate reloading on SIGHUP #26994
Conversation
Add TLS certificate reloading on SIGHUP. For httpd service certificates, the configuration is reloaded and certificate and key file locations are updated accordingly. For opentsdb service certificates, certificates and keys at the existing locations are reloaded. If reloading a certificate fails, the currently loaded certificate continues to be used. Also adds file permission checking for TLS certificates and private keys.
Rewrite some tests so that tricks are not used to get a hold of the TLS certificate in-use. Instead, an HTTP client is used to get the certificate presented by the http service.
| // VerifyLoad verifies that the certificate at certPath and keyPath will load without error. | ||
| // If the certificate can be loaded, a function that will apply the certificate reload is | ||
| // returned. Otherwise, an error is returned. | ||
| func (cl *TLSCertLoader) VerifyLoad(certPath, keyPath string) (func() error, error) { |
There was a problem hiding this comment.
I'm not convinced VerifyLoad is the best name for this method. The name does make sense, because it is used on config reload to verify that that the new configuration and/or certificate will load properly before it is applied. The name is seems off, though, when you consider that it also returns a closure that will apply the change. Maybe something like PrepareReload would be a better name?
| if out == nil { | ||
| out = new(tls.Config) | ||
| } | ||
| out = new(tls.Config) |
There was a problem hiding this comment.
Caught by a linter. Of course out is nil, this it the top of the method and out is a return variable.
| // VerifyReloadTLSCertificate verifies that the configured TLS certificate can be reloaded. | ||
| // If so, then a function that will apply the reloaded certificate is returned. If no reload | ||
| // action is necessary, then nil is returned for the reload function. | ||
| func (s *Service) VerifyReloadTLSCertificate() (func() error, error) { |
There was a problem hiding this comment.
Same thing as CertLoader.VerifyReload. Is this the best name for the method?
There was a problem hiding this comment.
MakeTlsCertLoaderFunc??? Just a suggestion.
davidby-influx
left a comment
davidby-influx
left a comment
There was a problem hiding this comment.
A few suggestions, questions, comments...
| config, err := cmd.ParseConfig(options.GetConfigPath()) | ||
| if err != nil { | ||
| return fmt.Errorf("parse config: %s", err) | ||
| return fail(fmt.Errorf("error parsing config file: %s", err)) |
There was a problem hiding this comment.
Add the config file path here so we can figure out bad environment variables, etc.
|
|
||
| cmd.Logger.Info("Signal received, initializing clean shutdown...") | ||
| if sig == syscall.SIGTERM && cmd.Server.LogQueriesOnTermination() { | ||
| if logQueries && cmd.Server.LogQueriesOnTermination() { |
There was a problem hiding this comment.
Do we need the logQueries variable? Is there a case where we get here with a signal on shutdownCh that is not a SIGTERM?
|
|
||
| // VerifyFilePermissivenessF checks if permissions on f are as restrictive | ||
| // or more restrictive than maxPerms. if not, then an error is returned. | ||
| // For security reasons, there is no VerifyFilePermissiveness functino |
|
|
||
| f, err := os.Open(tmpFile) | ||
| require.NoError(t, err) | ||
| defer f.Close() |
There was a problem hiding this comment.
Wrap in a require.NoError?
|
|
||
| f, err := os.Open(tmpFile) | ||
| require.NoError(t, err) | ||
| defer f.Close() |
| HTTPSEnabled: false, | ||
| } | ||
| applyFunc, err := s.VerifyReloadedConfig(newConfig) | ||
| require.Error(t, err) |
There was a problem hiding this comment.
This differs from usage elsewhere in this PR. Does require.ErrorContains imply require.Error? Or do you need to add require.Error to the other places?
| HTTPSPrivateKey: ss.KeyPath, | ||
| } | ||
| applyFunc, err := s.VerifyReloadedConfig(newConfig) | ||
| require.Error(t, err) |
|
|
||
| // Open service to initialize certLoader | ||
| require.NoError(t, s.Open()) | ||
| defer s.Close() |
There was a problem hiding this comment.
Any possible error?
|
|
||
| // Open service to initialize certLoader | ||
| require.NoError(t, s.Open()) | ||
| defer s.Close() |
There was a problem hiding this comment.
any possible error?
| // VerifyReloadTLSCertificate verifies that the configured TLS certificate can be reloaded. | ||
| // If so, then a function that will apply the reloaded certificate is returned. If no reload | ||
| // action is necessary, then nil is returned for the reload function. | ||
| func (s *Service) VerifyReloadTLSCertificate() (func() error, error) { |
There was a problem hiding this comment.
MakeTlsCertLoaderFunc??? Just a suggestion.
Add TLS certificate reloading on SIGHUP. For httpd service certificates, the configuration is reloaded and certificate and key file locations are updated accordingly. For opentsdb service certificates, certificates and keys at the existing locations are reloaded. If reloading a certificate fails, the currently loaded certificate continues to be used.
Also adds file permission checking for TLS certificates and private keys.