From aae944be37145d7ac661978510710acb3ff0ffc9 Mon Sep 17 00:00:00 2001 From: Hugo Vieira Date: Thu, 10 Mar 2022 14:59:26 +0000 Subject: [PATCH] test(worker): Remove test-only hooks from actual code This change introduces a `nonceFn` field that can be changed by a test but at runtime is transparent to the code that calls it. The current nonce-generating logic defaults to a test-only field which opens us up to this value inadvertently being set. With this change, the default behavior becomes using `base62` to generate a random nonce, and any tests have to explicitly change that. --- .../servers/worker/controller_connection.go | 16 +++----- internal/servers/worker/testing.go | 9 +++-- internal/servers/worker/worker.go | 11 +++-- internal/servers/worker/worker_test.go | 16 +++++++- internal/tests/cluster/worker_replay_test.go | 40 +++++++++++++++---- 5 files changed, 65 insertions(+), 27 deletions(-) diff --git a/internal/servers/worker/controller_connection.go b/internal/servers/worker/controller_connection.go index 023a2e8964..1b1402d952 100644 --- a/internal/servers/worker/controller_connection.go +++ b/internal/servers/worker/controller_connection.go @@ -138,17 +138,13 @@ func (w *Worker) createClientConn(addr string) error { func (w *Worker) workerAuthTLSConfig() (*tls.Config, *base.WorkerAuthInfo, error) { var err error info := &base.WorkerAuthInfo{ - Name: w.conf.RawConfig.Worker.Name, - Description: w.conf.RawConfig.Worker.Description, - ConnectionNonce: w.testReusedAuthNonce, + Name: w.conf.RawConfig.Worker.Name, + Description: w.conf.RawConfig.Worker.Description, } - if info.ConnectionNonce == "" { - if info.ConnectionNonce, err = base62.Random(20); err != nil { - return nil, nil, err - } - if w.testReuseAuthNonces { - w.testReusedAuthNonce = info.ConnectionNonce - } + + info.ConnectionNonce, err = w.nonceFn(20) + if err != nil { + return nil, nil, err } pubKey, privKey, err := ed25519.GenerateKey(w.conf.SecureRandomReader) diff --git a/internal/servers/worker/testing.go b/internal/servers/worker/testing.go index 6282a5cacc..e13d3aacc6 100644 --- a/internal/servers/worker/testing.go +++ b/internal/servers/worker/testing.go @@ -183,8 +183,9 @@ type TestWorkerOpts struct { // connection cannot be made back to the controller StatusGracePeriodDuration time.Duration - // Whether to set the replay value - EnableAuthReplay bool + // Overrides worker's nonceFn, for cases where we want to have control + // over the nonce we send to the Controller + NonceFn randFn } func NewTestWorker(t *testing.T, opts *TestWorkerOpts) *TestWorker { @@ -280,8 +281,8 @@ func NewTestWorker(t *testing.T, opts *TestWorkerOpts) *TestWorker { t.Fatal(err) } - if opts.EnableAuthReplay { - tw.w.testReuseAuthNonces = true + if opts.NonceFn != nil { + tw.w.nonceFn = opts.NonceFn } if !opts.DisableAutoStart { diff --git a/internal/servers/worker/worker.go b/internal/servers/worker/worker.go index b345775b2b..c9659918a1 100644 --- a/internal/servers/worker/worker.go +++ b/internal/servers/worker/worker.go @@ -21,12 +21,15 @@ import ( "github.com/hashicorp/boundary/internal/servers" "github.com/hashicorp/boundary/internal/servers/worker/session" "github.com/hashicorp/go-hclog" + "github.com/hashicorp/go-secure-stdlib/base62" "github.com/hashicorp/go-secure-stdlib/mlock" ua "go.uber.org/atomic" "google.golang.org/grpc/resolver" "google.golang.org/grpc/resolver/manual" ) +type randFn func(length int) (string, error) + type Worker struct { conf *Config logger hclog.Logger @@ -48,6 +51,9 @@ type Worker struct { listeners []*base.ServerListener + // Used to generate a random nonce for Controller connections + nonceFn randFn + // We store the current set in an atomic value so that we can add // reload-on-sighup behavior later tags *atomic.Value @@ -55,10 +61,6 @@ type Worker struct { // request. It can be set via startup in New below, or (eventually) via // SIGHUP. updateTags ua.Bool - - // Test-related values - testReuseAuthNonces bool - testReusedAuthNonce string } func New(conf *Config) (*Worker, error) { @@ -72,6 +74,7 @@ func New(conf *Config) (*Worker, error) { controllerSessionConn: new(atomic.Value), sessionInfoMap: new(sync.Map), tags: new(atomic.Value), + nonceFn: base62.Random, } w.lastStatusSuccess.Store((*LastStatusInformation)(nil)) diff --git a/internal/servers/worker/worker_test.go b/internal/servers/worker/worker_test.go index 62136f53c0..8ae62a97db 100644 --- a/internal/servers/worker/worker_test.go +++ b/internal/servers/worker/worker_test.go @@ -11,7 +11,7 @@ import ( "github.com/stretchr/testify/require" ) -func TestWorkerNewListenerConfig(t *testing.T) { +func TestWorkerNew(t *testing.T) { tests := []struct { name string in *Config @@ -76,6 +76,20 @@ func TestWorkerNewListenerConfig(t *testing.T) { require.Len(t, w.listeners, 2) }, }, + { + name: "worker nonce func is set", + in: &Config{ + Server: &base.Server{ + Listeners: []*base.ServerListener{ + {Config: &listenerutil.ListenerConfig{Purpose: []string{"proxy"}}}, + }, + }, + }, + expErr: false, + assertions: func(t *testing.T, w *Worker) { + require.NotNil(t, w.nonceFn) + }, + }, } for _, tt := range tests { diff --git a/internal/tests/cluster/worker_replay_test.go b/internal/tests/cluster/worker_replay_test.go index ae64ecf605..3182ee6d32 100644 --- a/internal/tests/cluster/worker_replay_test.go +++ b/internal/tests/cluster/worker_replay_test.go @@ -13,7 +13,6 @@ import ( "github.com/hashicorp/boundary/internal/servers/controller" "github.com/hashicorp/boundary/internal/servers/worker" "github.com/hashicorp/go-hclog" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -41,24 +40,49 @@ func TestWorkerReplay(t *testing.T) { Config: conf, WorkerAuthKms: c1.Config().WorkerAuthKms, InitialControllers: c1.ClusterAddrs(), - EnableAuthReplay: true, + NonceFn: func(length int) (string, error) { + return "test_noncetest_nonce", nil + }, }) // Give time for it to connect time.Sleep(10 * time.Second) + + // Assert that the worker has connected + logBuf, err := os.ReadFile(ec.AllEvents.Name()) + require.NoError(t, err) + require.Equal(t, 1, strings.Count(string(logBuf), "worker successfully authed")) + + // Assert we have the expected nonce in the DB + nonces, err := c1.ServersRepo().ListNonces(c1.Context(), servers.NoncePurposeWorkerAuth) + require.NoError(t, err) + require.Len(t, nonces, 1) + require.Equal(t, &servers.Nonce{Nonce: "test_noncetest_nonce", Purpose: servers.NoncePurposeWorkerAuth}, nonces[0]) + require.NoError(t, w1.Worker().Shutdown()) - // Now, start up again - require.NoError(t, w1.Worker().Start()) + // Now, start up again with the same nonce + w1 = worker.NewTestWorker(t, &worker.TestWorkerOpts{ + Config: conf, + WorkerAuthKms: c1.Config().WorkerAuthKms, + InitialControllers: c1.ClusterAddrs(), + NonceFn: func(length int) (string, error) { + return "test_noncetest_nonce", nil + }, + }) + + // Give time for it to connect time.Sleep(10 * time.Second) // We should find only one nonce, and one successful worker authentication, // both in the output and in the database ec.AllEvents.Close() - logBuf, err := os.ReadFile(ec.AllEvents.Name()) + logBuf, err = os.ReadFile(ec.AllEvents.Name()) require.NoError(t, err) - assert.Equal(t, 1, strings.Count(string(logBuf), "worker successfully authed")) - nonces, err := c1.ServersRepo().ListNonces(c1.Context(), servers.NoncePurposeWorkerAuth) + require.Equal(t, 1, strings.Count(string(logBuf), "worker successfully authed")) + + nonces, err = c1.ServersRepo().ListNonces(c1.Context(), servers.NoncePurposeWorkerAuth) require.NoError(t, err) - assert.Len(t, nonces, 1) + require.Len(t, nonces, 1) + require.Equal(t, &servers.Nonce{Nonce: "test_noncetest_nonce", Purpose: servers.NoncePurposeWorkerAuth}, nonces[0]) }