From dd63028d00cea4d4867b9d937e1af4e83a8fff77 Mon Sep 17 00:00:00 2001 From: hc-github-team-secure-vault-core <82990506+hc-github-team-secure-vault-core@users.noreply.github.com> Date: Thu, 9 Mar 2023 10:41:40 -0500 Subject: [PATCH] backport of commit 9f8d831d9419e545b3e29af897abc13103797263 (#19492) Co-authored-by: Violet Hynes --- changelog/19483.txt | 3 ++ command/agent.go | 9 ++++-- command/agent_test.go | 69 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 78 insertions(+), 3 deletions(-) create mode 100644 changelog/19483.txt diff --git a/changelog/19483.txt b/changelog/19483.txt new file mode 100644 index 000000000000..c7ba6f66d97d --- /dev/null +++ b/changelog/19483.txt @@ -0,0 +1,3 @@ +```release-note:bug +agent: Fix panic when SIGHUP is issued to Agent while it has a non-TLS listener. +``` diff --git a/command/agent.go b/command/agent.go index fd1ea68105fe..6bc896de5109 100644 --- a/command/agent.go +++ b/command/agent.go @@ -1359,9 +1359,12 @@ func (c *AgentCommand) reloadCerts() error { defer c.tlsReloadFuncsLock.RUnlock() for _, reloadFunc := range c.tlsReloadFuncs { - err := reloadFunc() - if err != nil { - errors = multierror.Append(errors, err) + // Non-TLS listeners will have a nil reload func. + if reloadFunc != nil { + err := reloadFunc() + if err != nil { + errors = multierror.Append(errors, err) + } } } diff --git a/command/agent_test.go b/command/agent_test.go index 9cf4b4704852..0dbfaa0b2398 100644 --- a/command/agent_test.go +++ b/command/agent_test.go @@ -2428,6 +2428,75 @@ func TestAgent_Config_ReloadTls(t *testing.T) { wg.Wait() } +// TestAgent_NonTLSListener_SIGHUP tests giving a SIGHUP signal to a listener +// without a TLS configuration. Prior to fixing GitHub issue #19480, this +// would cause a panic. +func TestAgent_NonTLSListener_SIGHUP(t *testing.T) { + logger := logging.NewVaultLogger(hclog.Trace) + cluster := vault.NewTestCluster(t, nil, &vault.TestClusterOptions{ + HandlerFunc: vaulthttp.Handler, + }) + cluster.Start() + defer cluster.Cleanup() + + serverClient := cluster.Cores[0].Client + + // Unset the environment variable so that agent picks up the right test + // cluster address + defer os.Setenv(api.EnvVaultAddress, os.Getenv(api.EnvVaultAddress)) + os.Unsetenv(api.EnvVaultAddress) + + listenAddr := generateListenerAddress(t) + listenConfig := fmt.Sprintf(` +listener "tcp" { + address = "%s" + tls_disable = true +} +`, listenAddr) + + config := fmt.Sprintf(` +vault { + address = "%s" + tls_skip_verify = true +} +%s +`, serverClient.Address(), listenConfig) + configPath := makeTempFile(t, "config.hcl", config) + defer os.Remove(configPath) + + // Start the agent + ui, cmd := testAgentCommand(t, logger) + + cmd.startedCh = make(chan struct{}) + + wg := &sync.WaitGroup{} + wg.Add(1) + go func() { + if code := cmd.Run([]string{"-config", configPath}); code != 0 { + output := ui.ErrorWriter.String() + ui.OutputWriter.String() + t.Errorf("got a non-zero exit status: %s", output) + } + wg.Done() + }() + + select { + case <-cmd.startedCh: + case <-time.After(5 * time.Second): + t.Errorf("timeout") + } + + // Reload + cmd.SighupCh <- struct{}{} + select { + case <-cmd.reloadedCh: + case <-time.After(5 * time.Second): + t.Fatalf("timeout") + } + + close(cmd.ShutdownCh) + wg.Wait() +} + // Get a randomly assigned port and then free it again before returning it. // There is still a race when trying to use it, but should work better // than a static port.