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