From 1782f6a4e47dd271f01d55abd5b053c9f7a2b1c5 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Wed, 27 Oct 2021 20:32:07 -0400 Subject: [PATCH] Enhance worker connection security (#1641) This stores worker connection nonces within the database to ensure uniqueness of connections. Prior the connection required any attacker to be able to decrypt the incoming encrypted authentication message in order to replay (and if the attacker has access to the KMS you have bigger problems), but with this change even if an attacker does have access to the KMS key, they cannot reuse the same message. (If they're able to encrypt against the KMS key, they can still act as a worker with a new message, so this only really helps if they only have decryption capabilities.) --- CHANGELOG.md | 7 + globals/globals.go | 4 + go.mod | 1 - go.sum | 2 - .../oss/postgres/0/08_servers.up.sql | 1 + .../oss/postgres/18/01_nonce.up.sql | 57 ++++++ .../migrations/oss/postgres_16_05_test.go | 7 +- .../migrations/oss/postgres_18_01_test.go | 162 ++++++++++++++++++ internal/observability/event/eventer_test.go | 2 +- internal/observability/event/testing.go | 8 +- internal/servers/controller/auth/auth.go | 3 +- internal/servers/controller/controller.go | 15 +- .../servers/controller/handlers/verifiers.go | 6 +- .../controller/intercepting_listener.go | 2 +- internal/servers/controller/tickers.go | 8 +- .../servers/controller/worker_tls_config.go | 38 +++- internal/servers/repository.go | 49 ++++-- internal/servers/repository_test.go | 15 +- .../servers/worker/controller_connection.go | 17 +- internal/servers/worker/testing.go | 7 + internal/servers/worker/worker.go | 4 + internal/tests/cluster/worker_replay_test.go | 64 +++++++ .../concepts/security/connections-tls.mdx | 10 +- 23 files changed, 426 insertions(+), 63 deletions(-) create mode 100644 internal/db/schema/migrations/oss/postgres/18/01_nonce.up.sql create mode 100644 internal/db/schema/migrations/oss/postgres_18_01_test.go create mode 100644 internal/tests/cluster/worker_replay_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index f80167d01..28a28534d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,13 @@ Canonical reference for changes, improvements, and bugfixes for Boundary. * tls: Boundary's support for TLS 1.0/1.1 on the API listener was broken. Rather than fix this, we are simply not supporting TLS 1.0/1.1 as they are insecure. +### New and Improved + +* workers: The existing worker connection replay prevention logic has been + enhanced to be more robust against attackers that have decryption access to + the shared `worker-auth` KMS key + ([PR](https://github.com/hashicorp/boundary/pull/1641)) + ### Bug Fixes * tls: Support TLS 1.2 for more clients. This was broken for some clients due to diff --git a/globals/globals.go b/globals/globals.go index 087e5497c..b5b9acbe6 100644 --- a/globals/globals.go +++ b/globals/globals.go @@ -30,4 +30,8 @@ var ( // RecoveryTokenValidityPeriod is exported so we can modify it in tests if // we want RecoveryTokenValidityPeriod = 5 * time.Minute + + // WorkerAuthNonceValidityPeriod is exported so we can modify it in tests if + // we want + WorkerAuthNonceValidityPeriod = 2 * time.Minute ) diff --git a/go.mod b/go.mod index c4e5177af..d4889798c 100644 --- a/go.mod +++ b/go.mod @@ -64,7 +64,6 @@ require ( github.com/mr-tron/base58 v1.2.0 github.com/oligot/go-mod-upgrade v0.6.1 github.com/ory/dockertest/v3 v3.7.0 - github.com/patrickmn/go-cache v2.1.0+incompatible github.com/pires/go-proxyproto v0.6.1 github.com/pkg/errors v0.9.1 github.com/posener/complete v1.2.3 diff --git a/go.sum b/go.sum index 69c68d267..7b149e68c 100644 --- a/go.sum +++ b/go.sum @@ -847,8 +847,6 @@ github.com/pact-foundation/pact-go v1.0.4/go.mod h1:uExwJY4kCzNPcHRj+hCR/HBbOOIw github.com/pascaldekloe/goe v0.0.0-20180627143212-57f6aae5913c/go.mod h1:lzWF7FIEvWOWxwDKqyGYQf6ZUaNfKdP144TG7ZOy1lc= github.com/pascaldekloe/goe v0.1.0 h1:cBOtyMzM9HTpWjXfbbunk26uA6nG3a8n06Wieeh0MwY= github.com/pascaldekloe/goe v0.1.0/go.mod h1:lzWF7FIEvWOWxwDKqyGYQf6ZUaNfKdP144TG7ZOy1lc= -github.com/patrickmn/go-cache v2.1.0+incompatible h1:HRMgzkcYKYpi3C8ajMPV8OFXaaRUnok+kx1WdO15EQc= -github.com/patrickmn/go-cache v2.1.0+incompatible/go.mod h1:3Qf8kWWT7OJRJbdiICTKqZju1ZixQ/KpMGzzAfe6+WQ= github.com/pborman/uuid v1.2.0/go.mod h1:X/NO0urCmaxf9VXbdlT7C2Yzkj2IKimNn4k+gtPdI/k= github.com/pelletier/go-toml v1.2.0/go.mod h1:5z9KED0ma1S8pY6P1sdut58dfprrGBbd/94hg7ilaic= github.com/performancecopilot/speed v3.0.0+incompatible/go.mod h1:/CLtqpZ5gBg1M9iaPbIdPPGyKcA8hKdoy6hAWba7Yac= diff --git a/internal/db/schema/migrations/oss/postgres/0/08_servers.up.sql b/internal/db/schema/migrations/oss/postgres/0/08_servers.up.sql index a5fcd22c5..2e37ce69d 100644 --- a/internal/db/schema/migrations/oss/postgres/0/08_servers.up.sql +++ b/internal/db/schema/migrations/oss/postgres/0/08_servers.up.sql @@ -29,6 +29,7 @@ before insert on server for each row execute procedure default_create_time(); +-- UPDATED in 18/01_nonce create table recovery_nonces ( nonce text primary key, diff --git a/internal/db/schema/migrations/oss/postgres/18/01_nonce.up.sql b/internal/db/schema/migrations/oss/postgres/18/01_nonce.up.sql new file mode 100644 index 000000000..9b29a355d --- /dev/null +++ b/internal/db/schema/migrations/oss/postgres/18/01_nonce.up.sql @@ -0,0 +1,57 @@ +begin; + +-- This migration exists to generalize the nonce behavior since we need it for +-- more than recovery nonces. + +create table nonce_type_enm ( + name text primary key + constraint only_predefined_nonce_types_allowed + check ( + name in ( + 'recovery', + 'worker-auth' + ) + ) +); +comment on table nonce_type_enm is + 'nonce_type_enm is an enumeration table for nonce types. ' + 'It contains rows for representing nonces as either for recovery or worker authentication purposes.'; + +insert into nonce_type_enm (name) values + ('recovery'), + ('worker-auth'); + +-- Drop existing triggers +drop trigger default_create_time_column on recovery_nonces; +drop trigger immutable_columns on recovery_nonces; + +-- Update table name +alter table recovery_nonces rename to nonce; + +-- Add purpose field +alter table nonce + add column purpose text not null + default 'recovery' + constraint nonce_type_enm_fkey + references nonce_type_enm(name) + on update cascade + on delete restrict; + +alter table nonce + alter column purpose drop default; + + +-- Recreate triggers +create trigger + default_create_time_column +before +insert on nonce + for each row execute procedure default_create_time(); + +create trigger + immutable_columns +before +update on nonce + for each row execute procedure immutable_columns('nonce', 'create_time', 'purpose'); + +commit; \ No newline at end of file diff --git a/internal/db/schema/migrations/oss/postgres_16_05_test.go b/internal/db/schema/migrations/oss/postgres_16_05_test.go index 58cbda927..d2c279751 100644 --- a/internal/db/schema/migrations/oss/postgres_16_05_test.go +++ b/internal/db/schema/migrations/oss/postgres_16_05_test.go @@ -47,7 +47,7 @@ func TestMigrations_CredentialDimension(t *testing.T) { )) require.NoError(err) - assert.NoError(m.ApplyMigrations(ctx)) + require.NoError(m.ApplyMigrations(ctx)) state, err := m.CurrentState(ctx) require.NoError(err) want := &schema.State{ @@ -127,7 +127,7 @@ func TestMigrations_CredentialDimension(t *testing.T) { require.NoError(err) count, err := sessionRepo.TerminateCompletedSessions(ctx) - assert.NoError(err) + require.NoError(err) assert.Zero(count) for _, sess := range sessions { @@ -142,8 +142,9 @@ func TestMigrations_CredentialDimension(t *testing.T) { )) require.NoError(err) - assert.NoError(m.ApplyMigrations(ctx)) + require.NoError(m.ApplyMigrations(ctx)) state, err = m.CurrentState(ctx) + require.NoError(err) want = &schema.State{ Initialized: true, Editions: []schema.EditionState{ diff --git a/internal/db/schema/migrations/oss/postgres_18_01_test.go b/internal/db/schema/migrations/oss/postgres_18_01_test.go new file mode 100644 index 000000000..7b65ab84a --- /dev/null +++ b/internal/db/schema/migrations/oss/postgres_18_01_test.go @@ -0,0 +1,162 @@ +package oss_test + +import ( + "context" + "testing" + + "github.com/hashicorp/boundary/internal/db" + "github.com/hashicorp/boundary/internal/db/common" + "github.com/hashicorp/boundary/internal/db/schema" + "github.com/hashicorp/boundary/internal/kms" + "github.com/hashicorp/boundary/internal/servers" + "github.com/hashicorp/boundary/testing/dbtest" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestMigrations_Nonce(t *testing.T) { + const ( + priorMigration = 16005 + currentMigration = 18001 + ) + + t.Parallel() + assert, require := assert.New(t), require.New(t) + ctx := context.Background() + dialect := dbtest.Postgres + + c, u, _, err := dbtest.StartUsingTemplate(dialect, dbtest.WithTemplate(dbtest.Template1)) + require.NoError(err) + t.Cleanup(func() { + require.NoError(c()) + }) + d, err := common.SqlOpen(dialect, u) + require.NoError(err) + + // migration to the prior migration (before the one we want to test) + m, err := schema.NewManager(ctx, schema.Dialect(dialect), d, schema.WithEditions( + schema.TestCreatePartialEditions(schema.Dialect(dialect), schema.PartialEditions{"oss": priorMigration}), + )) + require.NoError(err) + + require.NoError(m.ApplyMigrations(ctx)) + state, err := m.CurrentState(ctx) + require.NoError(err) + want := &schema.State{ + Initialized: true, + Editions: []schema.EditionState{ + { + Name: "oss", + BinarySchemaVersion: priorMigration, + DatabaseSchemaVersion: priorMigration, + DatabaseSchemaState: schema.Equal, + }, + }, + } + require.Equal(want, state) + + // get a connection + dbType, err := db.StringToDbType(dialect) + require.NoError(err) + conn, err := db.Open(dbType, u) + require.NoError(err) + rw := db.New(conn) + + // okay, now we can seed the database with test data and validate existing + // functionality. At the end of this we will have two values in the table + // with no purpose. + { + // Create an old version of the struct + type RecoveryNonce struct { + Nonce string + } + + require.NoError(rw.CreateItems(ctx, []interface{}{ + &RecoveryNonce{Nonce: "abcd"}, + &RecoveryNonce{Nonce: "dcba"}, + })) + + var results []*RecoveryNonce + require.NoError(rw.SearchWhere(ctx, &results, "", nil)) + require.Len(results, 2) + } + + // now we're ready for the migration we want to test. + m, err = schema.NewManager(ctx, schema.Dialect(dialect), d, schema.WithEditions( + schema.TestCreatePartialEditions(schema.Dialect(dialect), schema.PartialEditions{"oss": currentMigration}), + )) + require.NoError(err) + + require.NoError(m.ApplyMigrations(ctx)) + state, err = m.CurrentState(ctx) + require.NoError(err) + want = &schema.State{ + Initialized: true, + Editions: []schema.EditionState{ + { + Name: "oss", + BinarySchemaVersion: currentMigration, + DatabaseSchemaVersion: currentMigration, + DatabaseSchemaState: schema.Equal, + }, + }, + } + require.Equal(want, state) + + // now we have migrated and we will validate functionality; we'll do this + // via the repo + { + wrapper := db.TestWrapper(t) + kmsCache := kms.TestKms(t, conn, wrapper) + rw := db.New(conn) + repo, err := servers.NewRepository(rw, rw, kmsCache) + require.NoError(err) + require.NotNil(repo) + + // First list and validate that the upgrade adjusted purpose correctly + results, err := repo.ListNonces(ctx, servers.NoncePurposeRecovery) + require.NoError(err) + assert.Len(results, 2) + for _, n := range results { + assert.Equal(servers.NoncePurposeRecovery, n.Purpose) + } + + // Test the constraint, first via the repo, then directly + const badPurpose = "foobar" + err = repo.AddNonce(ctx, "xyz", badPurpose) + require.Error(err) + assert.Contains(err.Error(), "unknown nonce purpose") + + // Check that null errors + require.Error(rw.CreateItems(ctx, []interface{}{ + &servers.Nonce{Nonce: "dcba"}, + })) + // Check that bad purpose errors + require.Error(rw.CreateItems(ctx, []interface{}{ + &servers.Nonce{Nonce: "dcba", Purpose: badPurpose}, + })) + + // Add some valid values, ensure that we can then list for both purposes + // and get different values + require.NoError(repo.AddNonce(ctx, "wxyz", servers.NoncePurposeWorkerAuth)) + require.NoError(repo.AddNonce(ctx, "zyxw", servers.NoncePurposeWorkerAuth)) + results, err = repo.ListNonces(ctx, servers.NoncePurposeRecovery) + require.NoError(err) + assert.Len(results, 2) + for _, n := range results { + assert.Equal(servers.NoncePurposeRecovery, n.Purpose) + } + results, err = repo.ListNonces(ctx, servers.NoncePurposeWorkerAuth) + require.NoError(err) + assert.Len(results, 2) + for _, n := range results { + assert.Equal(servers.NoncePurposeWorkerAuth, n.Purpose) + } + + // Last: add the same nonce, ensure we error + require.NoError(repo.AddNonce(ctx, "double", servers.NoncePurposeWorkerAuth)) + err = repo.AddNonce(ctx, "double", servers.NoncePurposeWorkerAuth) + require.Error(err) + require.Contains(err.Error(), "duplicate key value") + } +} diff --git a/internal/observability/event/eventer_test.go b/internal/observability/event/eventer_test.go index 0c1a4a0d4..8b0151f2d 100644 --- a/internal/observability/event/eventer_test.go +++ b/internal/observability/event/eventer_test.go @@ -353,7 +353,7 @@ func Test_NewEventer(t *testing.T) { t.Parallel() testSetup := TestEventerConfig(t, "Test_NewEventer") - testSetupWithOpts := TestEventerConfig(t, "Test_NewEventer", TestWithAuditSink(t), TestWithObservationSink(t), testWithSysSink(t)) + testSetupWithOpts := TestEventerConfig(t, "Test_NewEventer", TestWithAuditSink(t), TestWithObservationSink(t), TestWithSysSink(t)) testHclogSetup := TestEventerConfig(t, "Test_NewEventer", testWithSinkFormat(t, TextHclogSinkFormat)) diff --git a/internal/observability/event/testing.go b/internal/observability/event/testing.go index 0bc1c71e7..5736638b7 100644 --- a/internal/observability/event/testing.go +++ b/internal/observability/event/testing.go @@ -222,7 +222,7 @@ func TestWithBroker(t *testing.T, b broker) Option { } } -// TestWithObservationSink is an unexported and a test option +// TestWithObservationSink is a test option func TestWithObservationSink(t *testing.T) Option { t.Helper() return func(o *options) { @@ -230,7 +230,7 @@ func TestWithObservationSink(t *testing.T) Option { } } -// TestWithAuditSink is an unexported and a test option +// TestWithAuditSink is a test option func TestWithAuditSink(t *testing.T) Option { t.Helper() return func(o *options) { @@ -238,8 +238,8 @@ func TestWithAuditSink(t *testing.T) Option { } } -// testWithSysSink is an unexported and a test option -func testWithSysSink(t *testing.T) Option { +// TestWithSysSink is a test option +func TestWithSysSink(t *testing.T) Option { t.Helper() return func(o *options) { o.withSysSink = true diff --git a/internal/servers/controller/auth/auth.go b/internal/servers/controller/auth/auth.go index b72d4b7c5..f39a76505 100644 --- a/internal/servers/controller/auth/auth.go +++ b/internal/servers/controller/auth/auth.go @@ -18,6 +18,7 @@ import ( "github.com/hashicorp/boundary/internal/observability/event" "github.com/hashicorp/boundary/internal/perms" "github.com/hashicorp/boundary/internal/requests" + "github.com/hashicorp/boundary/internal/servers" "github.com/hashicorp/boundary/internal/servers/controller/common" "github.com/hashicorp/boundary/internal/servers/controller/handlers" "github.com/hashicorp/boundary/internal/types/action" @@ -391,7 +392,7 @@ func (v *verifier) decryptToken(ctx context.Context) { v.requestInfo.TokenFormat = uint32(AuthTokenTypeUnknown) return } - if err := repo.AddRecoveryNonce(v.ctx, info.Nonce); err != nil { + if err := repo.AddNonce(v.ctx, info.Nonce, servers.NoncePurposeRecovery); err != nil { event.WriteError(ctx, op, err, event.WithInfoMsg("decrypt recovery token: error adding nonce to database (possible replay attack)")) v.requestInfo.TokenFormat = uint32(AuthTokenTypeUnknown) return diff --git a/internal/servers/controller/controller.go b/internal/servers/controller/controller.go index 6041e93aa..064cd4264 100644 --- a/internal/servers/controller/controller.go +++ b/internal/servers/controller/controller.go @@ -26,7 +26,6 @@ import ( "github.com/hashicorp/boundary/internal/target" "github.com/hashicorp/go-hclog" "github.com/hashicorp/go-secure-stdlib/mlock" - "github.com/patrickmn/go-cache" ua "go.uber.org/atomic" "google.golang.org/grpc" ) @@ -39,10 +38,10 @@ type Controller struct { baseCancel context.CancelFunc started *ua.Bool - tickerWg sync.WaitGroup - schedulerWg sync.WaitGroup + tickerWg *sync.WaitGroup + schedulerWg *sync.WaitGroup - workerAuthCache *cache.Cache + workerAuthCache *sync.Map // Used for testing and tracking worker health workerStatusUpdateTimes *sync.Map @@ -74,6 +73,9 @@ func New(ctx context.Context, conf *Config) (*Controller, error) { conf: conf, logger: conf.Logger.Named("controller"), started: ua.NewBool(false), + tickerWg: new(sync.WaitGroup), + schedulerWg: new(sync.WaitGroup), + workerAuthCache: new(sync.Map), workerStatusUpdateTimes: new(sync.Map), } @@ -163,7 +165,6 @@ func New(ctx context.Context, conf *Config) (*Controller, error) { c.SessionRepoFn = func() (*session.Repository, error) { return session.NewRepository(dbase, dbase, c.kms) } - c.workerAuthCache = cache.New(0, 0) return c, nil } @@ -178,7 +179,7 @@ func (c *Controller) Start() error { if err := c.registerJobs(); err != nil { return fmt.Errorf("error registering jobs: %w", err) } - if err := c.scheduler.Start(c.baseContext, &c.schedulerWg); err != nil { + if err := c.scheduler.Start(c.baseContext, c.schedulerWg); err != nil { return fmt.Errorf("error starting scheduler: %w", err) } if err := c.startListeners(c.baseContext); err != nil { @@ -192,7 +193,7 @@ func (c *Controller) Start() error { }() go func() { defer c.tickerWg.Done() - c.startRecoveryNonceCleanupTicking(c.baseContext) + c.startNonceCleanupTicking(c.baseContext) }() go func() { defer c.tickerWg.Done() diff --git a/internal/servers/controller/handlers/verifiers.go b/internal/servers/controller/handlers/verifiers.go index 4d394ee07..fd9577667 100644 --- a/internal/servers/controller/handlers/verifiers.go +++ b/internal/servers/controller/handlers/verifiers.go @@ -195,9 +195,5 @@ func ValidNameDescription(in string) bool { return !unicode.IsPrint(c) }) - if idx != -1 { - return false - } - - return true + return idx == -1 } diff --git a/internal/servers/controller/intercepting_listener.go b/internal/servers/controller/intercepting_listener.go index a22eac92f..7cece060b 100644 --- a/internal/servers/controller/intercepting_listener.go +++ b/internal/servers/controller/intercepting_listener.go @@ -70,7 +70,7 @@ func (m *interceptingListener) Accept() (net.Conn, error) { } return nil, fmt.Errorf("error reading nonce from worker, expected %d bytes, got %d", 20, read) } - workerInfoRaw, found := m.c.workerAuthCache.Get(string(nonce)) + workerInfoRaw, found := m.c.workerAuthCache.Load(string(nonce)) if !found { if err := conn.Close(); err != nil { event.WriteError(ctx, op, err, event.WithInfoMsg("error closing worker connection")) diff --git a/internal/servers/controller/tickers.go b/internal/servers/controller/tickers.go index 84ca7cdc9..8b93b5e0d 100644 --- a/internal/servers/controller/tickers.go +++ b/internal/servers/controller/tickers.go @@ -17,7 +17,7 @@ const ( ) // This is exported so it can be tweaked in tests -var RecoveryNonceCleanupInterval = 2 * time.Minute +var NonceCleanupInterval = 2 * time.Minute func (c *Controller) startStatusTicking(cancelCtx context.Context) { const op = "controller.(Controller).startStatusTicking" @@ -51,8 +51,8 @@ func (c *Controller) startStatusTicking(cancelCtx context.Context) { } } -func (c *Controller) startRecoveryNonceCleanupTicking(cancelCtx context.Context) { - const op = "controller.(Controller).startRecoveryNonceCleanupTicking" +func (c *Controller) startNonceCleanupTicking(cancelCtx context.Context) { + const op = "controller.(Controller).startNonceCleanupTicking" timer := time.NewTimer(0) for { select { @@ -72,7 +72,7 @@ func (c *Controller) startRecoveryNonceCleanupTicking(cancelCtx context.Context) event.WriteSysEvent(cancelCtx, op, "recovery nonce cleanup successful", "nonces_cleaned", nonceCount) } } - timer.Reset(RecoveryNonceCleanupInterval) + timer.Reset(NonceCleanupInterval) } } } diff --git a/internal/servers/controller/worker_tls_config.go b/internal/servers/controller/worker_tls_config.go index 941fa898c..3901efef5 100644 --- a/internal/servers/controller/worker_tls_config.go +++ b/internal/servers/controller/worker_tls_config.go @@ -7,10 +7,12 @@ import ( "encoding/base64" "encoding/json" "errors" + "fmt" "net" "strings" "github.com/hashicorp/boundary/internal/cmd/base" + "github.com/hashicorp/boundary/internal/servers" wrapping "github.com/hashicorp/go-kms-wrapping" "google.golang.org/protobuf/proto" ) @@ -20,16 +22,30 @@ type workerAuthEntry struct { conn net.Conn } +// validateWorkerTls is called by the Go TLS stack with client info. It calls +// v1WorkerAuthConfig to validate the encryption and ensure the nonce has not +// been replayed, then stores connection information into the auth cache, which +// is used by the intercepting listener to log information. func (c Controller) validateWorkerTls(hello *tls.ClientHelloInfo) (*tls.Config, error) { for _, p := range hello.SupportedProtos { switch { case strings.HasPrefix(p, "v1workerauth-"): tlsConf, workerInfo, err := c.v1WorkerAuthConfig(hello.SupportedProtos) if err == nil { - // Set the info we need to prevent replays - c.workerAuthCache.Set(workerInfo.ConnectionNonce, &workerAuthEntry{ + // Store info that can be retrieved in the intercepting + // listener. + // + // NOTE: This is not a load-or-store because we need to ensure + // replays can't happen across controllers, hence storing in the + // database in v1WorkerAuthConfig. Although this could serve as + // a secondary check, a load-or-store here keeps the test from + // ensuring that it is rejected at the database level via the + // nonce unique constraint, which is by far the more important + // thing to validate (although there are DB-level tests for that + // too). + c.workerAuthCache.Store(workerInfo.ConnectionNonce, &workerAuthEntry{ WorkerAuthInfo: workerInfo, - }, 0) + }) } return tlsConf, err } @@ -37,6 +53,13 @@ func (c Controller) validateWorkerTls(hello *tls.ClientHelloInfo) (*tls.Config, return nil, nil } +// v1WorkerAuthConfig: +// +// * Reads the information from ALPN, combining across protos +// * Combines them to form the encrypted auth information +// * Validates it by decrypting against the configured KMS +// * Ensures that the nonce is unique to prevent replay attacks +// * Returns the shared TLS configuration that is used to establish the connection func (c Controller) v1WorkerAuthConfig(protos []string) (*tls.Config, *base.WorkerAuthInfo, error) { var firstMatchProto string var encString string @@ -69,6 +92,15 @@ func (c Controller) v1WorkerAuthConfig(protos []string) (*tls.Config, *base.Work return nil, nil, err } + // Check for replays + serversRepo, err := c.ServersRepoFn() + if err != nil { + return nil, nil, fmt.Errorf("unable to fetch servers repo: %w", err) + } + if err := serversRepo.AddNonce(c.baseContext, info.ConnectionNonce, servers.NoncePurposeWorkerAuth); err != nil { + return nil, nil, fmt.Errorf("unable to add connection nonce to database: %w", err) + } + rootCAs := x509.NewCertPool() if ok := rootCAs.AppendCertsFromPEM(info.CertPEM); !ok { return nil, info, errors.New("unable to add ca cert to cert pool") diff --git a/internal/servers/repository.go b/internal/servers/repository.go index 1a5a49639..4d71ae8b8 100644 --- a/internal/servers/repository.go +++ b/internal/servers/repository.go @@ -207,18 +207,38 @@ func (r *Repository) UpsertServer(ctx context.Context, server *Server, opt ...Op return controllers, int(rowsUpdated), nil } -type RecoveryNonce struct { - Nonce string +type Nonce struct { + Nonce string + Purpose string } -// AddRecoveryNonce adds a nonce -func (r *Repository) AddRecoveryNonce(ctx context.Context, nonce string, opt ...Option) error { - const op = "servers.AddRecoveryNonce" +// TableName returns the table name. +func (n *Nonce) TableName() string { + return "nonce" +} + +const ( + NoncePurposeRecovery = "recovery" + NoncePurposeWorkerAuth = "worker-auth" +) + +// AddNonce adds a nonce +func (r *Repository) AddNonce(ctx context.Context, nonce, purpose string, opt ...Option) error { + const op = "servers.AddNonce" if nonce == "" { return errors.New(ctx, errors.InvalidParameter, op, "empty nonce") } - rn := &RecoveryNonce{Nonce: nonce} - if err := r.writer.Create(ctx, rn); err != nil { + switch purpose { + case NoncePurposeRecovery, NoncePurposeWorkerAuth: + case "": + return errors.New(ctx, errors.InvalidParameter, op, "empty nonce purpose") + default: + return errors.New(ctx, errors.InvalidParameter, op, fmt.Sprintf("unknown nonce purpose %q", purpose)) + } + if err := r.writer.Create(ctx, &Nonce{ + Nonce: nonce, + Purpose: purpose, + }); err != nil { return errors.Wrap(ctx, err, op+":Insertion") } return nil @@ -226,10 +246,15 @@ func (r *Repository) AddRecoveryNonce(ctx context.Context, nonce string, opt ... // CleanupNonces removes nonces that no longer need to be stored func (r *Repository) CleanupNonces(ctx context.Context, opt ...Option) (int, error) { + // Use the largest validity period out of the various nonces we're looking at + maxDuration := globals.RecoveryTokenValidityPeriod + if globals.WorkerAuthNonceValidityPeriod > maxDuration { + maxDuration = globals.WorkerAuthNonceValidityPeriod + } // If something was inserted before 3x the actual validity period, clean it out - endTime := time.Now().Add(-3 * globals.RecoveryTokenValidityPeriod) + endTime := time.Now().Add(-3 * maxDuration) - rows, err := r.writer.Delete(ctx, &RecoveryNonce{}, db.WithWhere(deleteWhereCreateTimeSql, endTime)) + rows, err := r.writer.Delete(ctx, &Nonce{}, db.WithWhere(deleteWhereCreateTimeSql, endTime)) if err != nil { return db.NoRowsAffected, errors.Wrap(ctx, err, "servers.CleanupNonces") } @@ -237,9 +262,9 @@ func (r *Repository) CleanupNonces(ctx context.Context, opt ...Option) (int, err } // ListNonces lists nonces. Used only for tests at the moment. -func (r *Repository) ListNonces(ctx context.Context, opt ...Option) ([]*RecoveryNonce, error) { - var nonces []*RecoveryNonce - if err := r.reader.SearchWhere(ctx, &nonces, "", nil, db.WithLimit(-1)); err != nil { +func (r *Repository) ListNonces(ctx context.Context, purpose string, opt ...Option) ([]*Nonce, error) { + var nonces []*Nonce + if err := r.reader.SearchWhere(ctx, &nonces, "purpose = ?", []interface{}{purpose}, db.WithLimit(-1)); err != nil { return nil, errors.Wrap(ctx, err, "servers.ListNonces") } return nonces, nil diff --git a/internal/servers/repository_test.go b/internal/servers/repository_test.go index 30ed1c8ed..3ff57b655 100644 --- a/internal/servers/repository_test.go +++ b/internal/servers/repository_test.go @@ -22,7 +22,8 @@ func TestRecoveryNonces(t *testing.T) { // Set these low so that we can not have the test run forever globals.RecoveryTokenValidityPeriod = 10 * time.Second - controller.RecoveryNonceCleanupInterval = 20 * time.Second + globals.WorkerAuthNonceValidityPeriod = 10 * time.Second + controller.NonceCleanupInterval = 20 * time.Second wrapper := db.TestWrapper(t) tc := controller.NewTestController(t, &controller.TestControllerOpts{ @@ -46,14 +47,14 @@ func TestRecoveryNonces(t *testing.T) { roleClient := roles.NewClient(client) _, err = roleClient.Create(tc.Context(), scope.Global.String()) require.NoError(err) - nonces, err := repo.ListNonces(tc.Context()) + nonces, err := repo.ListNonces(tc.Context(), servers.NoncePurposeRecovery) require.NoError(err) assert.Len(nonces, 1) // Token 1, try 2 _, err = roleClient.Create(tc.Context(), scope.Global.String()) require.Error(err) - nonces, err = repo.ListNonces(tc.Context()) + nonces, err = repo.ListNonces(tc.Context(), servers.NoncePurposeRecovery) require.NoError(err) assert.Len(nonces, 1) @@ -61,13 +62,13 @@ func TestRecoveryNonces(t *testing.T) { roleClient.ApiClient().SetToken(token2) _, err = roleClient.Create(tc.Context(), scope.Global.String()) require.NoError(err) - nonces, err = repo.ListNonces(tc.Context()) + nonces, err = repo.ListNonces(tc.Context(), servers.NoncePurposeRecovery) require.NoError(err) assert.Len(nonces, 2) // Make sure they get cleaned up - time.Sleep(2 * controller.RecoveryNonceCleanupInterval) - nonces, err = repo.ListNonces(tc.Context()) + time.Sleep(2 * controller.NonceCleanupInterval) + nonces, err = repo.ListNonces(tc.Context(), servers.NoncePurposeRecovery) require.NoError(err) assert.Len(nonces, 0) @@ -76,7 +77,7 @@ func TestRecoveryNonces(t *testing.T) { roleClient.ApiClient().SetToken(token) _, err = roleClient.Create(tc.Context(), scope.Global.String()) require.Error(err) - nonces, err = repo.ListNonces(tc.Context()) + nonces, err = repo.ListNonces(tc.Context(), servers.NoncePurposeRecovery) require.NoError(err) assert.Len(nonces, 0) } diff --git a/internal/servers/worker/controller_connection.go b/internal/servers/worker/controller_connection.go index e2d8b5a81..5f77004ad 100644 --- a/internal/servers/worker/controller_connection.go +++ b/internal/servers/worker/controller_connection.go @@ -17,6 +17,7 @@ import ( "strings" "time" + "github.com/hashicorp/boundary/globals" "github.com/hashicorp/boundary/internal/cmd/base" pbs "github.com/hashicorp/boundary/internal/gen/controller/servers/services" "github.com/hashicorp/boundary/internal/observability/event" @@ -137,11 +138,17 @@ 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, + Name: w.conf.RawConfig.Worker.Name, + Description: w.conf.RawConfig.Worker.Description, + ConnectionNonce: w.testReusedAuthNonce, } - if info.ConnectionNonce, err = base62.Random(20); err != nil { - return nil, nil, err + if info.ConnectionNonce == "" { + if info.ConnectionNonce, err = base62.Random(20); err != nil { + return nil, nil, err + } + if w.testReuseAuthNonces { + w.testReusedAuthNonce = info.ConnectionNonce + } } pubKey, privKey, err := ed25519.GenerateKey(w.conf.SecureRandomReader) @@ -162,7 +169,7 @@ func (w *Worker) workerAuthTLSConfig() (*tls.Config, *base.WorkerAuthInfo, error KeyUsage: x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment | x509.KeyUsageKeyAgreement | x509.KeyUsageCertSign, SerialNumber: big.NewInt(mathrand.Int63()), NotBefore: time.Now().Add(-30 * time.Second), - NotAfter: time.Now().Add(2 * time.Minute), + NotAfter: time.Now().Add(globals.WorkerAuthNonceValidityPeriod), BasicConstraintsValid: true, IsCA: true, } diff --git a/internal/servers/worker/testing.go b/internal/servers/worker/testing.go index a9a06906a..e3e7d03dd 100644 --- a/internal/servers/worker/testing.go +++ b/internal/servers/worker/testing.go @@ -182,6 +182,9 @@ type TestWorkerOpts struct { // The amount of time to wait before marking connections as closed when a // connection cannot be made back to the controller StatusGracePeriodDuration time.Duration + + // Whether to set the replay value + EnableAuthReplay bool } func NewTestWorker(t *testing.T, opts *TestWorkerOpts) *TestWorker { @@ -277,6 +280,10 @@ func NewTestWorker(t *testing.T, opts *TestWorkerOpts) *TestWorker { t.Fatal(err) } + if opts.EnableAuthReplay { + tw.w.testReuseAuthNonces = true + } + if !opts.DisableAutoStart { if err := tw.w.Start(); err != nil { tw.Shutdown() diff --git a/internal/servers/worker/worker.go b/internal/servers/worker/worker.go index fe89bdd88..f7ca0163e 100644 --- a/internal/servers/worker/worker.go +++ b/internal/servers/worker/worker.go @@ -52,6 +52,10 @@ 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) { diff --git a/internal/tests/cluster/worker_replay_test.go b/internal/tests/cluster/worker_replay_test.go new file mode 100644 index 000000000..76be8b0cb --- /dev/null +++ b/internal/tests/cluster/worker_replay_test.go @@ -0,0 +1,64 @@ +package cluster + +import ( + "os" + "strings" + "sync" + "testing" + "time" + + "github.com/hashicorp/boundary/internal/cmd/config" + "github.com/hashicorp/boundary/internal/observability/event" + "github.com/hashicorp/boundary/internal/servers" + "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" +) + +func TestWorkerReplay(t *testing.T) { + ec := event.TestEventerConfig(t, "TestWorkerReplay", event.TestWithObservationSink(t), event.TestWithSysSink(t)) + testLock := &sync.Mutex{} + testLogger := hclog.New(&hclog.LoggerOptions{ + Mutex: testLock, + Name: "test", + }) + require.NoError(t, event.InitSysEventer(testLogger, testLock, "use-TestWorkerReplay", event.WithEventerConfig(&ec.EventerConfig))) + + conf, err := config.DevController() + conf.Eventing = &ec.EventerConfig + require.NoError(t, err) + c1 := controller.NewTestController(t, &controller.TestControllerOpts{ + Config: conf, + }) + defer c1.Shutdown() + + conf, err = config.DevWorker() + conf.Eventing = &ec.EventerConfig + require.NoError(t, err) + w1 := worker.NewTestWorker(t, &worker.TestWorkerOpts{ + Config: conf, + WorkerAuthKms: c1.Config().WorkerAuthKms, + InitialControllers: c1.ClusterAddrs(), + EnableAuthReplay: true, + }) + + // Give time for it to connect + time.Sleep(10 * time.Second) + require.NoError(t, w1.Worker().Shutdown(true)) + + // Now, start up again + require.NoError(t, w1.Worker().Start()) + 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()) + 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.NoError(t, err) + assert.Len(t, nonces, 1) +} diff --git a/website/content/docs/concepts/security/connections-tls.mdx b/website/content/docs/concepts/security/connections-tls.mdx index 6f772038d..95e77b871 100644 --- a/website/content/docs/concepts/security/connections-tls.mdx +++ b/website/content/docs/concepts/security/connections-tls.mdx @@ -57,8 +57,9 @@ TLS establishment is performed as follows: encrypted value. 5. The Controller decrypts this value via the shared KMS. If successful, the - Controller validates that the nonce is not known, to ensure that this is not a - replay. + Controller validates that the nonce is not known to ensure that this is not a + replay. The nonce value is checked against and stored into the database, + ensuring that nonce replay cannot occur across controllers. 6. The Controller uses the decrypted parameters to configures its TLS stack with the same certificate and key. @@ -67,11 +68,6 @@ TLS establishment is performed as follows: self-signed CA certificate that was securely transmitted is configured as a valid root CA for validation checking. -8. If successful, the nonce is stored in the database along with an expiration - time set several minutes past the actual expiration time of the certificate - itself. This ensures that any replay attempt that occurs is detected and - rejected until after the certificate is otherwise invalid. - ## Client-to-Worker TLS Workers do not require any configuration for their client-facing listeners to