From 157bea08b9b156004d1445d2157ba6b30c7be13b Mon Sep 17 00:00:00 2001 From: Louis Ruch Date: Fri, 22 Oct 2021 15:53:18 -0700 Subject: [PATCH 1/7] test(vault): Register vault jobs for tests to reduce error spam in logs (#1630) --- internal/credential/vault/jobs_test.go | 73 +++++++++++++++++-- .../credential/vault/private_library_test.go | 4 +- .../credential/vault/private_store_test.go | 5 +- .../vault/repository_credential_store_test.go | 46 +++++++++--- .../vault/repository_credentials_test.go | 2 + .../credentialstore_service_test.go | 9 +++ .../handlers/targets/target_service_test.go | 6 ++ 7 files changed, 123 insertions(+), 22 deletions(-) diff --git a/internal/credential/vault/jobs_test.go b/internal/credential/vault/jobs_test.go index b6f16e73e..e739d3dfb 100644 --- a/internal/credential/vault/jobs_test.go +++ b/internal/credential/vault/jobs_test.go @@ -238,6 +238,7 @@ func TestTokenRenewalJob_RunLimits(t *testing.T) { kmsCache := kms.TestKms(t, conn, wrapper) _, prj := iam.TestScopes(t, iam.TestRepo(t, conn, wrapper)) v := NewTestVaultServer(t) + sche := scheduler.TestScheduler(t, conn, wrapper) count := 5 tests := []struct { @@ -273,9 +274,10 @@ func TestTokenRenewalJob_RunLimits(t *testing.T) { _, token := v.CreateToken(t) in, err := NewCredentialStore(prj.GetPublicId(), v.Addr, []byte(token)) require.NoError(err) - sche := scheduler.TestScheduler(t, conn, wrapper) repo, err := NewRepository(rw, rw, kmsCache, sche) require.NoError(err) + err = RegisterJobs(context.Background(), sche, rw, rw, kmsCache) + require.NoError(err) cs, err := repo.CreateCredentialStore(context.Background(), in) require.NoError(err) @@ -330,17 +332,17 @@ func TestTokenRenewalJob_Run(t *testing.T) { in, err := NewCredentialStore(prj.GetPublicId(), v.Addr, []byte(token)) require.NoError(err) sche := scheduler.TestScheduler(t, conn, wrapper) - repo, err := NewRepository(rw, rw, kmsCache, sche) - require.NoError(err) - cs, err := repo.CreateCredentialStore(context.Background(), in) - require.NoError(err) r, err := newTokenRenewalJob(rw, rw, kmsCache) require.NoError(err) - err = sche.RegisterJob(context.Background(), r) require.NoError(err) + repo, err := NewRepository(rw, rw, kmsCache, sche) + require.NoError(err) + cs, err := repo.CreateCredentialStore(context.Background(), in) + require.NoError(err) + err = r.Run(context.Background()) require.NoError(err) // No tokens should have been renewed since token expiration is 24 hours by default @@ -474,6 +476,7 @@ func TestTokenRenewalJob_NextRunIn(t *testing.T) { wrapper := db.TestWrapper(t) kmsCache := kms.TestKms(t, conn, wrapper) _, prj := iam.TestScopes(t, iam.TestRepo(t, conn, wrapper)) + sche := scheduler.TestScheduler(t, conn, wrapper) v := NewTestVaultServer(t) @@ -562,9 +565,10 @@ func TestTokenRenewalJob_NextRunIn(t *testing.T) { _, token := v.CreateToken(t) in, err := NewCredentialStore(prj.GetPublicId(), v.Addr, []byte(token)) require.NoError(err) - sche := scheduler.TestScheduler(t, conn, wrapper) repo, err := NewRepository(rw, rw, kmsCache, sche) require.NoError(err) + err = RegisterJobs(context.Background(), sche, rw, rw, kmsCache) + require.NoError(err) cs, err := repo.CreateCredentialStore(context.Background(), in) require.NoError(err) @@ -682,6 +686,7 @@ func TestTokenRevocationJob_RunLimits(t *testing.T) { wrapper := db.TestWrapper(t) kmsCache := kms.TestKms(t, conn, wrapper) _, prj := iam.TestScopes(t, iam.TestRepo(t, conn, wrapper)) + sche := scheduler.TestScheduler(t, conn, wrapper) v := NewTestVaultServer(t) @@ -719,9 +724,11 @@ func TestTokenRevocationJob_RunLimits(t *testing.T) { _, token := v.CreateToken(t) in, err := NewCredentialStore(prj.GetPublicId(), v.Addr, []byte(token)) require.NoError(err) - sche := scheduler.TestScheduler(t, conn, wrapper) repo, err := NewRepository(rw, rw, kmsCache, sche) require.NoError(err) + err = RegisterJobs(context.Background(), sche, rw, rw, kmsCache) + require.NoError(err) + cs, err := repo.CreateCredentialStore(context.Background(), in) require.NoError(err) @@ -777,6 +784,12 @@ func TestTokenRevocationJob_Run(t *testing.T) { sche := scheduler.TestScheduler(t, conn, wrapper) repo, err := NewRepository(rw, rw, kmsCache, sche) require.NoError(err) + + j, err := newTokenRenewalJob(rw, rw, kmsCache) + require.NoError(err) + err = sche.RegisterJob(context.Background(), j) + require.NoError(err) + cs, err := repo.CreateCredentialStore(context.Background(), in) require.NoError(err) @@ -992,6 +1005,12 @@ func TestCredentialRenewalJob_RunLimits(t *testing.T) { sche := scheduler.TestScheduler(t, conn, wrapper) repo, err := NewRepository(rw, rw, kmsCache, sche) require.NoError(t, err) + + j, err := newTokenRenewalJob(rw, rw, kmsCache) + require.NoError(t, err) + err = sche.RegisterJob(context.Background(), j) + require.NoError(t, err) + cs, err := repo.CreateCredentialStore(context.Background(), in) require.NoError(t, err) @@ -1103,6 +1122,10 @@ func TestCredentialRenewalJob_Run(t *testing.T) { _, token := v.CreateToken(t, WithPolicies([]string{"default", "boundary-controller", "database"})) credStoreIn, err := NewCredentialStore(prj.GetPublicId(), v.Addr, []byte(token)) require.NoError(err) + j, err := newTokenRenewalJob(rw, rw, kmsCache) + require.NoError(err) + err = sche.RegisterJob(context.Background(), j) + require.NoError(err) cs, err := repo.CreateCredentialStore(context.Background(), credStoreIn) require.NoError(err) @@ -1209,6 +1232,10 @@ func TestCredentialRenewalJob_RunExpired(t *testing.T) { _, token := v.CreateToken(t, WithPolicies([]string{"default", "boundary-controller", "database"})) credStoreIn, err := NewCredentialStore(prj.GetPublicId(), v.Addr, []byte(token)) require.NoError(err) + j, err := newTokenRenewalJob(rw, rw, kmsCache) + require.NoError(err) + err = sche.RegisterJob(context.Background(), j) + require.NoError(err) cs, err := repo.CreateCredentialStore(context.Background(), credStoreIn) require.NoError(err) @@ -1282,6 +1309,10 @@ func TestCredentialRenewalJob_NextRunIn(t *testing.T) { sche := scheduler.TestScheduler(t, conn, wrapper) repo, err := NewRepository(rw, rw, kmsCache, sche) require.NoError(t, err) + j, err := newTokenRenewalJob(rw, rw, kmsCache) + require.NoError(t, err) + err = sche.RegisterJob(context.Background(), j) + require.NoError(t, err) cs, err := repo.CreateCredentialStore(context.Background(), in) require.NoError(t, err) @@ -1519,6 +1550,10 @@ func TestCredentialRevocationJob_RunLimits(t *testing.T) { sche := scheduler.TestScheduler(t, conn, wrapper) repo, err := NewRepository(rw, rw, kmsCache, sche) require.NoError(t, err) + j, err := newTokenRenewalJob(rw, rw, kmsCache) + require.NoError(t, err) + err = sche.RegisterJob(context.Background(), j) + require.NoError(t, err) cs, err := repo.CreateCredentialStore(context.Background(), in) require.NoError(t, err) @@ -1632,6 +1667,10 @@ func TestCredentialRevocationJob_Run(t *testing.T) { _, token := v.CreateToken(t, WithPolicies([]string{"default", "boundary-controller", "database"})) credStoreIn, err := NewCredentialStore(prj.GetPublicId(), v.Addr, []byte(token)) require.NoError(err) + j, err := newTokenRenewalJob(rw, rw, kmsCache) + require.NoError(err) + err = sche.RegisterJob(context.Background(), j) + require.NoError(err) cs, err := repo.CreateCredentialStore(context.Background(), credStoreIn) require.NoError(err) @@ -1722,6 +1761,10 @@ func TestCredentialRevocationJob_RunDeleted(t *testing.T) { _, token := v.CreateToken(t, WithPolicies([]string{"default", "boundary-controller", "database"})) credStoreIn, err := NewCredentialStore(prj.GetPublicId(), v.Addr, []byte(token)) require.NoError(err) + j, err := newTokenRenewalJob(rw, rw, kmsCache) + require.NoError(err) + err = sche.RegisterJob(context.Background(), j) + require.NoError(err) cs, err := repo.CreateCredentialStore(context.Background(), credStoreIn) require.NoError(err) @@ -1907,6 +1950,10 @@ func TestCredentialStoreCleanupJob_Run(t *testing.T) { in, err := NewCredentialStore(prj.GetPublicId(), v.Addr, []byte(ct)) require.NoError(err) sche := scheduler.TestScheduler(t, conn, wrapper) + j, err := newTokenRenewalJob(rw, rw, kmsCache) + require.NoError(err) + err = sche.RegisterJob(context.Background(), j) + require.NoError(err) repo, err := NewRepository(rw, rw, kmsCache, sche) require.NoError(err) cs1, err := repo.CreateCredentialStore(context.Background(), in) @@ -1941,6 +1988,12 @@ func TestCredentialStoreCleanupJob_Run(t *testing.T) { require.NoError(err) assert.Equal(0, r.numStores) + // Register token revocation job needed for delete + j1, err := newTokenRevocationJob(rw, rw, kmsCache) + require.NoError(err) + err = sche.RegisterJob(context.Background(), j1) + require.NoError(err) + // Soft delete both credential stores count, err := repo.DeleteCredentialStore(context.Background(), cs1.PublicId) require.NoError(err) @@ -2100,6 +2153,10 @@ func TestCredentialCleanupJob_Run(t *testing.T) { _, token := v.CreateToken(t, WithPolicies([]string{"default", "boundary-controller", "database"})) credStoreIn, err := NewCredentialStore(prj.GetPublicId(), v.Addr, []byte(token)) require.NoError(err) + j, err := newTokenRenewalJob(rw, rw, kmsCache) + require.NoError(err) + err = sche.RegisterJob(context.Background(), j) + require.NoError(err) cs, err := repo.CreateCredentialStore(context.Background(), credStoreIn) require.NoError(err) diff --git a/internal/credential/vault/private_library_test.go b/internal/credential/vault/private_library_test.go index 7fd2b787c..b2f925b08 100644 --- a/internal/credential/vault/private_library_test.go +++ b/internal/credential/vault/private_library_test.go @@ -18,6 +18,7 @@ func TestRepository_getPrivateLibraries(t *testing.T) { conn, _ := db.TestSetup(t, "postgres") rw := db.New(conn) wrapper := db.TestWrapper(t) + sche := scheduler.TestScheduler(t, conn, wrapper) tests := []struct { name string @@ -44,10 +45,11 @@ func TestRepository_getPrivateLibraries(t *testing.T) { _, prj := iam.TestScopes(t, iam.TestRepo(t, conn, wrapper)) kms := kms.TestKms(t, conn, wrapper) - sche := scheduler.TestScheduler(t, conn, wrapper) repo, err := NewRepository(rw, rw, kms, sche) require.NoError(err) require.NotNil(repo) + err = RegisterJobs(ctx, sche, rw, rw, kms) + require.NoError(err) var opts []Option if tt.tls == TestServerTLS { diff --git a/internal/credential/vault/private_store_test.go b/internal/credential/vault/private_store_test.go index 6f9908060..c6e8a9523 100644 --- a/internal/credential/vault/private_store_test.go +++ b/internal/credential/vault/private_store_test.go @@ -17,6 +17,7 @@ func TestRepository_lookupPrivateStore(t *testing.T) { conn, _ := db.TestSetup(t, "postgres") rw := db.New(conn) wrapper := db.TestWrapper(t) + sche := scheduler.TestScheduler(t, conn, wrapper) tests := []struct { name string @@ -40,10 +41,12 @@ func TestRepository_lookupPrivateStore(t *testing.T) { assert, require := assert.New(t), require.New(t) ctx := context.Background() kms := kms.TestKms(t, conn, wrapper) - sche := scheduler.TestScheduler(t, conn, wrapper) repo, err := NewRepository(rw, rw, kms, sche) require.NoError(err) require.NotNil(repo) + err = RegisterJobs(ctx, sche, rw, rw, kms) + require.NoError(err) + _, prj := iam.TestScopes(t, iam.TestRepo(t, conn, wrapper)) v := NewTestVaultServer(t, WithTestVaultTLS(tt.tls)) diff --git a/internal/credential/vault/repository_credential_store_test.go b/internal/credential/vault/repository_credential_store_test.go index 2b7f72484..657975b6f 100644 --- a/internal/credential/vault/repository_credential_store_test.go +++ b/internal/credential/vault/repository_credential_store_test.go @@ -30,16 +30,18 @@ func TestRepository_CreateCredentialStoreResource(t *testing.T) { conn, _ := db.TestSetup(t, "postgres") rw := db.New(conn) wrapper := db.TestWrapper(t) + sche := scheduler.TestScheduler(t, conn, wrapper) t.Run("invalid-duplicate-names", func(t *testing.T) { assert, require := assert.New(t), require.New(t) ctx := context.Background() kms := kms.TestKms(t, conn, wrapper) - sche := scheduler.TestScheduler(t, conn, wrapper) repo, err := NewRepository(rw, rw, kms, sche) require.NoError(err) require.NotNil(repo) _, prj := iam.TestScopes(t, iam.TestRepo(t, conn, wrapper)) + err = RegisterJobs(ctx, sche, rw, rw, kms) + require.NoError(err) v := NewTestVaultServer(t) _, token := v.CreateToken(t) @@ -68,11 +70,12 @@ func TestRepository_CreateCredentialStoreResource(t *testing.T) { assert, require := assert.New(t), require.New(t) ctx := context.Background() kms := kms.TestKms(t, conn, wrapper) - sche := scheduler.TestScheduler(t, conn, wrapper) repo, err := NewRepository(rw, rw, kms, sche) require.NoError(err) require.NotNil(repo) org, prj := iam.TestScopes(t, iam.TestRepo(t, conn, wrapper)) + err = RegisterJobs(ctx, sche, rw, rw, kms) + require.NoError(err) v := NewTestVaultServer(t) @@ -123,6 +126,7 @@ func TestRepository_CreateCredentialStoreNonResource(t *testing.T) { conn, _ := db.TestSetup(t, "postgres") rw := db.New(conn) wrapper := db.TestWrapper(t) + sche := scheduler.TestScheduler(t, conn, wrapper) tests := []struct { name string @@ -168,11 +172,12 @@ func TestRepository_CreateCredentialStoreNonResource(t *testing.T) { assert, require := assert.New(t), require.New(t) ctx := context.Background() kms := kms.TestKms(t, conn, wrapper) - sche := scheduler.TestScheduler(t, conn, wrapper) repo, err := NewRepository(rw, rw, kms, sche) require.NoError(err) require.NotNil(repo) _, prj := iam.TestScopes(t, iam.TestRepo(t, conn, wrapper)) + err = RegisterJobs(ctx, sche, rw, rw, kms) + require.NoError(err) v := NewTestVaultServer(t, WithTestVaultTLS(tt.tls)) _, token := v.CreateToken(t, tt.tokenOpts...) @@ -226,6 +231,7 @@ func TestRepository_LookupCredentialStore(t *testing.T) { conn, _ := db.TestSetup(t, "postgres") rw := db.New(conn) wrapper := db.TestWrapper(t) + sche := scheduler.TestScheduler(t, conn, wrapper) _, prj := iam.TestScopes(t, iam.TestRepo(t, conn, wrapper)) stores := TestCredentialStores(t, conn, wrapper, prj.PublicId, 2) @@ -278,10 +284,11 @@ func TestRepository_LookupCredentialStore(t *testing.T) { assert, require := assert.New(t), require.New(t) ctx := context.Background() kms := kms.TestKms(t, conn, wrapper) - sche := scheduler.TestScheduler(t, conn, wrapper) repo, err := NewRepository(rw, rw, kms, sche) assert.NoError(err) require.NotNil(repo) + err = RegisterJobs(ctx, sche, rw, rw, kms) + require.NoError(err) got, err := repo.LookupCredentialStore(ctx, tt.id) if tt.wantErr != 0 { @@ -314,6 +321,7 @@ func TestRepository_UpdateCredentialStore_Attributes(t *testing.T) { conn, _ := db.TestSetup(t, "postgres") rw := db.New(conn) wrapper := db.TestWrapper(t) + sche := scheduler.TestScheduler(t, conn, wrapper) changeNamespace := func(n string) func(*CredentialStore) *CredentialStore { return func(cs *CredentialStore) *CredentialStore { @@ -716,10 +724,11 @@ func TestRepository_UpdateCredentialStore_Attributes(t *testing.T) { assert, require := assert.New(t), require.New(t) ctx := context.Background() kms := kms.TestKms(t, conn, wrapper) - sche := scheduler.TestScheduler(t, conn, wrapper) repo, err := NewRepository(rw, rw, kms, sche) assert.NoError(err) require.NotNil(repo) + err = RegisterJobs(ctx, sche, rw, rw, kms) + require.NoError(err) _, prj := iam.TestScopes(t, iam.TestRepo(t, conn, wrapper)) tt.orig.ScopeId = prj.GetPublicId() @@ -796,10 +805,11 @@ func TestRepository_UpdateCredentialStore_Attributes(t *testing.T) { assert, require := assert.New(t), require.New(t) ctx := context.Background() kms := kms.TestKms(t, conn, wrapper) - sche := scheduler.TestScheduler(t, conn, wrapper) repo, err := NewRepository(rw, rw, kms, sche) assert.NoError(err) require.NotNil(repo) + err = RegisterJobs(ctx, sche, rw, rw, kms) + require.NoError(err) vs := NewTestVaultServer(t) _, token := vs.CreateToken(t) @@ -847,10 +857,11 @@ func TestRepository_UpdateCredentialStore_Attributes(t *testing.T) { assert, require := assert.New(t), require.New(t) ctx := context.Background() kms := kms.TestKms(t, conn, wrapper) - sche := scheduler.TestScheduler(t, conn, wrapper) repo, err := NewRepository(rw, rw, kms, sche) assert.NoError(err) require.NotNil(repo) + err = RegisterJobs(ctx, sche, rw, rw, kms) + require.NoError(err) vs := NewTestVaultServer(t, WithTestVaultTLS(TestServerTLS)) @@ -896,10 +907,11 @@ func TestRepository_UpdateCredentialStore_Attributes(t *testing.T) { assert, require := assert.New(t), require.New(t) ctx := context.Background() kms := kms.TestKms(t, conn, wrapper) - sche := scheduler.TestScheduler(t, conn, wrapper) repo, err := NewRepository(rw, rw, kms, sche) assert.NoError(err) require.NotNil(repo) + err = RegisterJobs(ctx, sche, rw, rw, kms) + require.NoError(err) name := "test-dup-name" _, prj := iam.TestScopes(t, iam.TestRepo(t, conn, wrapper)) @@ -926,10 +938,11 @@ func TestRepository_UpdateCredentialStore_Attributes(t *testing.T) { assert, require := assert.New(t), require.New(t) ctx := context.Background() kms := kms.TestKms(t, conn, wrapper) - sche := scheduler.TestScheduler(t, conn, wrapper) repo, err := NewRepository(rw, rw, kms, sche) assert.NoError(err) require.NotNil(repo) + err = RegisterJobs(ctx, sche, rw, rw, kms) + require.NoError(err) vs := NewTestVaultServer(t) @@ -975,10 +988,11 @@ func TestRepository_UpdateCredentialStore_Attributes(t *testing.T) { assert, require := assert.New(t), require.New(t) ctx := context.Background() kms := kms.TestKms(t, conn, wrapper) - sche := scheduler.TestScheduler(t, conn, wrapper) repo, err := NewRepository(rw, rw, kms, sche) assert.NoError(err) require.NotNil(repo) + err = RegisterJobs(ctx, sche, rw, rw, kms) + require.NoError(err) iamRepo := iam.TestRepo(t, conn, wrapper) _, prj1 := iam.TestScopes(t, iamRepo) @@ -1004,6 +1018,7 @@ func TestRepository_UpdateCredentialStore_VaultToken(t *testing.T) { conn, _ := db.TestSetup(t, "postgres") rw := db.New(conn) wrapper := db.TestWrapper(t) + sche := scheduler.TestScheduler(t, conn, wrapper) tests := []struct { name string @@ -1044,11 +1059,12 @@ func TestRepository_UpdateCredentialStore_VaultToken(t *testing.T) { assert, require := assert.New(t), require.New(t) ctx := context.Background() kms := kms.TestKms(t, conn, wrapper) - sche := scheduler.TestScheduler(t, conn, wrapper) repo, err := NewRepository(rw, rw, kms, sche) require.NoError(err) require.NotNil(repo) _, prj := iam.TestScopes(t, iam.TestRepo(t, conn, wrapper)) + err = RegisterJobs(ctx, sche, rw, rw, kms) + require.NoError(err) v := NewTestVaultServer(t) _, origToken := v.CreateToken(t) @@ -1100,6 +1116,7 @@ func TestRepository_UpdateCredentialStore_ClientCert(t *testing.T) { conn, _ := db.TestSetup(t, "postgres") rw := db.New(conn) wrapper := db.TestWrapper(t) + sche := scheduler.TestScheduler(t, conn, wrapper) existingClientCert := func(t *testing.T, v *TestVaultServer) *ClientCertificate { clientCert, err := NewClientCertificate(v.ClientCert, v.ClientKey) @@ -1176,11 +1193,12 @@ func TestRepository_UpdateCredentialStore_ClientCert(t *testing.T) { assert, require := assert.New(t), require.New(t) ctx := context.Background() kms := kms.TestKms(t, conn, wrapper) - sche := scheduler.TestScheduler(t, conn, wrapper) repo, err := NewRepository(rw, rw, kms, sche) require.NoError(err) require.NotNil(repo) _, prj := iam.TestScopes(t, iam.TestRepo(t, conn, wrapper)) + err = RegisterJobs(ctx, sche, rw, rw, kms) + require.NoError(err) v := NewTestVaultServer(t, WithTestVaultTLS(tt.tls)) @@ -1244,6 +1262,8 @@ func TestRepository_ListCredentialStores_Multiple_Scopes(t *testing.T) { repo, err := NewRepository(rw, rw, kms, sche) assert.NoError(err) require.NotNil(repo) + err = RegisterJobs(context.Background(), sche, rw, rw, kms) + require.NoError(err) const numPerScope = 10 var prjs []string @@ -1279,6 +1299,8 @@ func TestRepository_DeleteCredentialStore(t *testing.T) { repo, err := NewRepository(rw, rw, kms, sche) require.NoError(t, err) require.NotNil(t, repo) + err = RegisterJobs(context.Background(), sche, rw, rw, kms) + require.NoError(t, err) _, prj := iam.TestScopes(t, iam.TestRepo(t, conn, wrapper)) scopeId = prj.GetPublicId() diff --git a/internal/credential/vault/repository_credentials_test.go b/internal/credential/vault/repository_credentials_test.go index 20f9d1cc5..91f8245bb 100644 --- a/internal/credential/vault/repository_credentials_test.go +++ b/internal/credential/vault/repository_credentials_test.go @@ -41,6 +41,8 @@ func TestRepository_IssueCredentials(t *testing.T) { repo, err := vault.NewRepository(rw, rw, kms, sche) require.NoError(err) require.NotNil(repo) + err = vault.RegisterJobs(ctx, sche, rw, rw, kms) + require.NoError(err) _, token := v.CreateToken(t, vault.WithPolicies([]string{"default", "boundary-controller", "database", "pki"})) diff --git a/internal/servers/controller/handlers/credentialstores/credentialstore_service_test.go b/internal/servers/controller/handlers/credentialstores/credentialstore_service_test.go index bfa43c54b..bd6508304 100644 --- a/internal/servers/controller/handlers/credentialstores/credentialstore_service_test.go +++ b/internal/servers/controller/handlers/credentialstores/credentialstore_service_test.go @@ -1,6 +1,7 @@ package credentialstores import ( + "context" "crypto/ecdsa" "crypto/elliptic" "crypto/rand" @@ -52,6 +53,8 @@ func TestList(t *testing.T) { kms := kms.TestKms(t, conn, wrapper) sche := scheduler.TestScheduler(t, conn, wrapper) rw := db.New(conn) + err := vault.RegisterJobs(context.Background(), sche, rw, rw, kms) + require.NoError(t, err) iamRepo := iam.TestRepo(t, conn, wrapper) iamRepoFn := func() (*iam.Repository, error) { @@ -156,6 +159,8 @@ func TestCreate(t *testing.T) { kms := kms.TestKms(t, conn, wrapper) sche := scheduler.TestScheduler(t, conn, wrapper) rw := db.New(conn) + err := vault.RegisterJobs(context.Background(), sche, rw, rw, kms) + require.NoError(t, err) iamRepo := iam.TestRepo(t, conn, wrapper) iamRepoFn := func() (*iam.Repository, error) { @@ -655,6 +660,8 @@ func TestDelete(t *testing.T) { kms := kms.TestKms(t, conn, wrapper) sche := scheduler.TestScheduler(t, conn, wrapper) rw := db.New(conn) + err := vault.RegisterJobs(context.Background(), sche, rw, rw, kms) + require.NoError(t, err) iamRepo := iam.TestRepo(t, conn, wrapper) iamRepoFn := func() (*iam.Repository, error) { @@ -714,6 +721,8 @@ func TestUpdate(t *testing.T) { kms := kms.TestKms(t, conn, wrapper) sche := scheduler.TestScheduler(t, conn, wrapper) rw := db.New(conn) + err := vault.RegisterJobs(context.Background(), sche, rw, rw, kms) + require.NoError(t, err) iamRepo := iam.TestRepo(t, conn, wrapper) iamRepoFn := func() (*iam.Repository, error) { diff --git a/internal/servers/controller/handlers/targets/target_service_test.go b/internal/servers/controller/handlers/targets/target_service_test.go index 8b1e20558..37b7329d2 100644 --- a/internal/servers/controller/handlers/targets/target_service_test.go +++ b/internal/servers/controller/handlers/targets/target_service_test.go @@ -2471,6 +2471,9 @@ func TestAuthorizeSession(t *testing.T) { kms := kms.TestKms(t, conn, wrapper) sche := scheduler.TestScheduler(t, conn, wrapper) + err := vault.RegisterJobs(context.Background(), sche, rw, rw, kms) + require.NoError(t, err) + repoFn := func() (*target.Repository, error) { return target.NewRepository(rw, rw, kms) } @@ -2641,6 +2644,9 @@ func TestAuthorizeSession_Errors(t *testing.T) { kms := kms.TestKms(t, conn, wrapper) sche := scheduler.TestScheduler(t, conn, wrapper) + err := vault.RegisterJobs(context.Background(), sche, rw, rw, kms) + require.NoError(t, err) + repoFn := func() (*target.Repository, error) { return target.NewRepository(rw, rw, kms) } From 99d6da412127cf47839328d226930e8c714c5fb7 Mon Sep 17 00:00:00 2001 From: Jim Date: Sat, 23 Oct 2021 21:23:49 -0400 Subject: [PATCH 2/7] feature (events): Add audit request interceptors (#1620) --- internal/gen/controller/auth/auth.pb.go | 35 ++++++++--- .../proto/local/controller/auth/v1/auth.proto | 6 ++ internal/servers/controller/gateway.go | 22 ++++++- internal/servers/controller/handler.go | 12 ++++ .../handlers/outgoing_response_filter.go | 7 --- internal/servers/controller/interceptor.go | 63 +++++++++++++++++++ .../servers/controller/interceptor_test.go | 45 ++++++++++++- internal/servers/controller/listeners.go | 2 +- 8 files changed, 171 insertions(+), 21 deletions(-) diff --git a/internal/gen/controller/auth/auth.pb.go b/internal/gen/controller/auth/auth.pb.go index 103a2360a..90649f069 100644 --- a/internal/gen/controller/auth/auth.pb.go +++ b/internal/gen/controller/auth/auth.pb.go @@ -49,6 +49,10 @@ type RequestInfo struct { // ticket is a unique id that allows the grpc-gateway to verify that the info // came from its companion http proxy Ticket string `protobuf:"bytes,110,opt,name=ticket,proto3" json:"ticket,omitempty"` + // trace_id is the request's trace_id + TraceId string `protobuf:"bytes,120,opt,name=trace_id,json=traceId,proto3" json:"trace_id,omitempty"` + // event_id is the request's event id + EventId string `protobuf:"bytes,130,opt,name=event_id,json=eventId,proto3" json:"event_id,omitempty"` } func (x *RequestInfo) Reset() { @@ -160,13 +164,27 @@ func (x *RequestInfo) GetTicket() string { return "" } +func (x *RequestInfo) GetTraceId() string { + if x != nil { + return x.TraceId + } + return "" +} + +func (x *RequestInfo) GetEventId() string { + if x != nil { + return x.EventId + } + return "" +} + var File_controller_auth_v1_auth_proto protoreflect.FileDescriptor var file_controller_auth_v1_auth_proto_rawDesc = []byte{ 0x0a, 0x1d, 0x63, 0x6f, 0x6e, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x65, 0x72, 0x2f, 0x61, 0x75, 0x74, 0x68, 0x2f, 0x76, 0x31, 0x2f, 0x61, 0x75, 0x74, 0x68, 0x2e, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x12, 0x12, 0x63, 0x6f, 0x6e, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x65, 0x72, 0x2e, 0x61, 0x75, 0x74, 0x68, - 0x2e, 0x76, 0x31, 0x22, 0x90, 0x03, 0x0a, 0x0b, 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x49, + 0x2e, 0x76, 0x31, 0x22, 0xc7, 0x03, 0x0a, 0x0b, 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x49, 0x6e, 0x66, 0x6f, 0x12, 0x12, 0x0a, 0x04, 0x70, 0x61, 0x74, 0x68, 0x18, 0x0a, 0x20, 0x01, 0x28, 0x09, 0x52, 0x04, 0x70, 0x61, 0x74, 0x68, 0x12, 0x16, 0x0a, 0x06, 0x6d, 0x65, 0x74, 0x68, 0x6f, 0x64, 0x18, 0x14, 0x20, 0x01, 0x28, 0x09, 0x52, 0x06, 0x6d, 0x65, 0x74, 0x68, 0x6f, 0x64, 0x12, @@ -191,12 +209,15 @@ var file_controller_auth_v1_auth_proto_rawDesc = []byte{ 0x65, 0x6c, 0x79, 0x18, 0x64, 0x20, 0x01, 0x28, 0x08, 0x52, 0x13, 0x64, 0x69, 0x73, 0x61, 0x62, 0x6c, 0x65, 0x41, 0x75, 0x74, 0x68, 0x45, 0x6e, 0x74, 0x69, 0x72, 0x65, 0x6c, 0x79, 0x12, 0x16, 0x0a, 0x06, 0x74, 0x69, 0x63, 0x6b, 0x65, 0x74, 0x18, 0x6e, 0x20, 0x01, 0x28, 0x09, 0x52, 0x06, - 0x74, 0x69, 0x63, 0x6b, 0x65, 0x74, 0x42, 0x41, 0x5a, 0x3f, 0x67, 0x69, 0x74, 0x68, 0x75, 0x62, - 0x2e, 0x63, 0x6f, 0x6d, 0x2f, 0x68, 0x61, 0x73, 0x68, 0x69, 0x63, 0x6f, 0x72, 0x70, 0x2f, 0x62, - 0x6f, 0x75, 0x6e, 0x64, 0x61, 0x72, 0x79, 0x2f, 0x69, 0x6e, 0x74, 0x65, 0x72, 0x6e, 0x61, 0x6c, - 0x2f, 0x67, 0x65, 0x6e, 0x2f, 0x63, 0x6f, 0x6e, 0x74, 0x72, 0x6f, 0x6c, 0x6c, 0x65, 0x72, 0x2f, - 0x61, 0x75, 0x74, 0x68, 0x3b, 0x61, 0x75, 0x74, 0x68, 0x62, 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, - 0x33, + 0x74, 0x69, 0x63, 0x6b, 0x65, 0x74, 0x12, 0x19, 0x0a, 0x08, 0x74, 0x72, 0x61, 0x63, 0x65, 0x5f, + 0x69, 0x64, 0x18, 0x78, 0x20, 0x01, 0x28, 0x09, 0x52, 0x07, 0x74, 0x72, 0x61, 0x63, 0x65, 0x49, + 0x64, 0x12, 0x1a, 0x0a, 0x08, 0x65, 0x76, 0x65, 0x6e, 0x74, 0x5f, 0x69, 0x64, 0x18, 0x82, 0x01, + 0x20, 0x01, 0x28, 0x09, 0x52, 0x07, 0x65, 0x76, 0x65, 0x6e, 0x74, 0x49, 0x64, 0x42, 0x41, 0x5a, + 0x3f, 0x67, 0x69, 0x74, 0x68, 0x75, 0x62, 0x2e, 0x63, 0x6f, 0x6d, 0x2f, 0x68, 0x61, 0x73, 0x68, + 0x69, 0x63, 0x6f, 0x72, 0x70, 0x2f, 0x62, 0x6f, 0x75, 0x6e, 0x64, 0x61, 0x72, 0x79, 0x2f, 0x69, + 0x6e, 0x74, 0x65, 0x72, 0x6e, 0x61, 0x6c, 0x2f, 0x67, 0x65, 0x6e, 0x2f, 0x63, 0x6f, 0x6e, 0x74, + 0x72, 0x6f, 0x6c, 0x6c, 0x65, 0x72, 0x2f, 0x61, 0x75, 0x74, 0x68, 0x3b, 0x61, 0x75, 0x74, 0x68, + 0x62, 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x33, } var ( diff --git a/internal/proto/local/controller/auth/v1/auth.proto b/internal/proto/local/controller/auth/v1/auth.proto index 840ffa12b..d4395722c 100644 --- a/internal/proto/local/controller/auth/v1/auth.proto +++ b/internal/proto/local/controller/auth/v1/auth.proto @@ -39,4 +39,10 @@ message RequestInfo { // ticket is a unique id that allows the grpc-gateway to verify that the info // came from its companion http proxy string ticket = 110; + + // trace_id is the request's trace_id + string trace_id = 120; + + // event_id is the request's event id + string event_id = 130; } diff --git a/internal/servers/controller/gateway.go b/internal/servers/controller/gateway.go index 41f6d76a9..40142890e 100644 --- a/internal/servers/controller/gateway.go +++ b/internal/servers/controller/gateway.go @@ -11,6 +11,7 @@ import ( "github.com/hashicorp/boundary/internal/db" "github.com/hashicorp/boundary/internal/errors" "github.com/hashicorp/boundary/internal/kms" + "github.com/hashicorp/boundary/internal/observability/event" "github.com/hashicorp/boundary/internal/servers/controller/common" "github.com/hashicorp/boundary/internal/servers/controller/handlers" "google.golang.org/grpc" @@ -49,19 +50,34 @@ func newGatewayMux() *runtime.ServeMux { ) } -func newGatewayServer(ctx context.Context, iamRepoFn common.IamRepoFactory, authTokenRepoFn common.AuthTokenRepoFactory, serversRepoFn common.ServersRepoFactory, kms *kms.Kms) (*grpc.Server, string, error) { +func newGatewayServer( + ctx context.Context, + iamRepoFn common.IamRepoFactory, + authTokenRepoFn common.AuthTokenRepoFactory, + serversRepoFn common.ServersRepoFactory, + kms *kms.Kms, + eventer *event.Eventer, +) (*grpc.Server, string, error) { const op = "controller.newGatewayServer" ticket, err := db.NewPrivateId("gwticket") if err != nil { return nil, "", errors.Wrap(ctx, err, op, errors.WithMsg("unable to generate gateway ticket")) } - requestCtxInterceptor, err := requestCtxInterceptor(ctx, iamRepoFn, authTokenRepoFn, serversRepoFn, kms, ticket) + requestCtxInterceptor, err := requestCtxInterceptor(ctx, iamRepoFn, authTokenRepoFn, serversRepoFn, kms, ticket, eventer) if err != nil { return nil, "", err } return grpc.NewServer( grpc.MaxRecvMsgSize(math.MaxInt32), grpc.MaxSendMsgSize(math.MaxInt32), - grpc.UnaryInterceptor(grpc_middleware.ChainUnaryServer(requestCtxInterceptor, errorInterceptor(ctx), statusCodeInterceptor(ctx))), + grpc.UnaryInterceptor( + grpc_middleware.ChainUnaryServer( + requestCtxInterceptor, // populated requestInfo from headers into the request ctx + auditRequestInterceptor(ctx), // before we get started, audit the request + errorInterceptor(ctx), // convert domain and api errors into headers for the http proxy + statusCodeInterceptor(ctx), // convert grpc codes into http status codes for the http proxy (can modify the resp) + auditResponseInterceptor(ctx), // as we finish, audit the response + ), + ), ), ticket, nil } diff --git a/internal/servers/controller/handler.go b/internal/servers/controller/handler.go index da5f6d5ec..1f92de586 100644 --- a/internal/servers/controller/handler.go +++ b/internal/servers/controller/handler.go @@ -287,6 +287,16 @@ func wrapHandlerWithCommonFuncs(h http.Handler, c *Controller, props HandlerProp requestInfo.PublicId, requestInfo.EncryptedToken, requestInfo.TokenFormat = auth.GetTokenFromRequest(ctx, c.kms, r) + if info, ok := event.RequestInfoFromContext(ctx); ok { + // piggyback some eventing fields with the auth info proto message + requestInfo.EventId = info.EventId + requestInfo.TraceId = info.Id + } else { + w.WriteHeader(http.StatusInternalServerError) + event.WriteError(ctx, op, errors.New("unable to read event request info from context")) + return + } + // Serialize the request info to send it across the wire to the // grpc-gateway via an http header requestInfo.Ticket = c.gatewayTicket // allows the grpc-gateway to verify the request info came from it's in-memory companion http proxy @@ -301,6 +311,8 @@ func wrapHandlerWithCommonFuncs(h http.Handler, c *Controller, props HandlerProp // See: https://pkg.go.dev/github.com/grpc-ecosystem/grpc-gateway/runtime#DefaultHeaderMatcher r.Header.Set("Grpc-Metadata-"+requestInfoMdKey, base58.FastBase58Encoding(marshalledRequestInfo)) + // Set the context back on the request + r = r.WithContext(ctx) h.ServeHTTP(w, r) }) } diff --git a/internal/servers/controller/handlers/outgoing_response_filter.go b/internal/servers/controller/handlers/outgoing_response_filter.go index 304409775..08150f783 100644 --- a/internal/servers/controller/handlers/outgoing_response_filter.go +++ b/internal/servers/controller/handlers/outgoing_response_filter.go @@ -11,7 +11,6 @@ import ( "github.com/grpc-ecosystem/grpc-gateway/v2/runtime" pbs "github.com/hashicorp/boundary/internal/gen/controller/api/services" - "github.com/hashicorp/boundary/internal/observability/event" authtokenpb "github.com/hashicorp/boundary/sdk/pbs/controller/api/resources/authtokens" "google.golang.org/grpc" @@ -65,12 +64,6 @@ func OutgoingResponseFilter(ctx context.Context, w http.ResponseWriter, m proto. } } - // add the outgoing resp proto. See: common.flushGatedEvents is where the - // event.Response.StatusCode is added. - if err := event.WriteAudit(ctx, op, event.WithResponse(&event.Response{Details: m})); err != nil { - return fmt.Errorf("%s: unable to write audit event: %w", op, err) - } - switch m := m.(type) { case *pbs.AuthenticateResponse: if m.GetAttributes() == nil || m.GetAttributes().GetFields() == nil { diff --git a/internal/servers/controller/interceptor.go b/internal/servers/controller/interceptor.go index 4dfaeab98..c7887ce76 100644 --- a/internal/servers/controller/interceptor.go +++ b/internal/servers/controller/interceptor.go @@ -11,6 +11,7 @@ import ( authpb "github.com/hashicorp/boundary/internal/gen/controller/auth" pberrors "github.com/hashicorp/boundary/internal/gen/errors" "github.com/hashicorp/boundary/internal/kms" + "github.com/hashicorp/boundary/internal/observability/event" "github.com/hashicorp/boundary/internal/requests" "github.com/hashicorp/boundary/internal/servers/controller/auth" "github.com/hashicorp/boundary/internal/servers/controller/common" @@ -48,6 +49,7 @@ func requestCtxInterceptor( serversRepoFn common.ServersRepoFactory, kms *kms.Kms, ticket string, + eventer *event.Eventer, ) (grpc.UnaryServerInterceptor, error) { const op = "controller.requestCtxInterceptor" if iamRepoFn == nil { @@ -65,6 +67,9 @@ func requestCtxInterceptor( if ticket == "" { return nil, errors.New(ctx, errors.InvalidParameter, op, "missing ticket") } + if eventer == nil { + return nil, errors.New(ctx, errors.InvalidParameter, op, "missing eventer") + } // Authorization unary interceptor function to handle authorize per RPC call return func(interceptorCtx context.Context, req interface{}, @@ -112,6 +117,23 @@ func requestCtxInterceptor( Method: requestInfo.Method, }) + // This event request info is required by downstream handlers + info := &event.RequestInfo{ + EventId: requestInfo.EventId, + Id: requestInfo.TraceId, + PublicId: requestInfo.PublicId, + Method: requestInfo.Method, + Path: requestInfo.Path, + } + interceptorCtx, err = event.NewRequestInfoContext(interceptorCtx, info) + if err != nil { + return nil, errors.Wrap(interceptorCtx, err, op, errors.WithCode(errors.Internal), errors.WithMsg("unable to create context with request info")) + } + interceptorCtx, err = event.NewEventerContext(interceptorCtx, eventer) + if err != nil { + return nil, errors.Wrap(interceptorCtx, err, op, errors.WithCode(errors.Internal), errors.WithMsg("unable to create context with eventer")) + } + // Calls the handler h, err := handler(interceptorCtx, req) @@ -205,3 +227,44 @@ func isNil(i interface{}) bool { } return false } + +func auditRequestInterceptor( + _ context.Context, +) grpc.UnaryServerInterceptor { + const op = "controller.auditRequestInterceptor" + return func(interceptorCtx context.Context, + req interface{}, + _ *grpc.UnaryServerInfo, + handler grpc.UnaryHandler) (interface{}, error) { + + if msg, ok := req.(proto.Message); ok { + if err := event.WriteAudit(interceptorCtx, op, event.WithRequest(&event.Request{Details: msg})); err != nil { + return req, status.Errorf(codes.Internal, "unable to write request msg audit: %s", err) + } + } + + return handler(interceptorCtx, req) + } +} + +func auditResponseInterceptor( + _ context.Context, +) grpc.UnaryServerInterceptor { + const op = "controller.auditRequestInterceptor" + return func(interceptorCtx context.Context, + req interface{}, + _ *grpc.UnaryServerInfo, + handler grpc.UnaryHandler) (interface{}, error) { + + // call the handler... + resp, err := handler(interceptorCtx, req) + + if msg, ok := resp.(proto.Message); ok { + if err := event.WriteAudit(interceptorCtx, op, event.WithResponse(&event.Response{Details: msg})); err != nil { + return req, status.Errorf(codes.Internal, "unable to write response msg audit: %s", err) + } + } + + return resp, err + } +} diff --git a/internal/servers/controller/interceptor_test.go b/internal/servers/controller/interceptor_test.go index f970022c0..d1b0d5740 100644 --- a/internal/servers/controller/interceptor_test.go +++ b/internal/servers/controller/interceptor_test.go @@ -6,6 +6,7 @@ import ( "net/http" "net/http/httptest" "strconv" + "sync" "testing" "github.com/hashicorp/boundary/internal/authtoken" @@ -17,10 +18,12 @@ import ( "github.com/hashicorp/boundary/internal/gen/testing/interceptor" "github.com/hashicorp/boundary/internal/iam" "github.com/hashicorp/boundary/internal/kms" + "github.com/hashicorp/boundary/internal/observability/event" "github.com/hashicorp/boundary/internal/servers" "github.com/hashicorp/boundary/internal/servers/controller/auth" "github.com/hashicorp/boundary/internal/servers/controller/common" "github.com/hashicorp/boundary/internal/servers/controller/handlers" + "github.com/hashicorp/go-hclog" "github.com/mr-tron/base58" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -59,8 +62,10 @@ func Test_requestCtxInterceptor(t *testing.T) { req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", tokValue)) // Add values for authn/authz checking requestInfo := authpb.RequestInfo{ - Path: req.URL.Path, - Method: req.Method, + Path: req.URL.Path, + Method: req.Method, + EventId: "test-event-id", + TraceId: "test-trace-id", } requestInfo.PublicId, requestInfo.EncryptedToken, requestInfo.TokenFormat = auth.GetTokenFromRequest(context.TODO(), kmsCache, req) requestInfo.Ticket = gwTicket // allows the grpc-gateway to verify the request info came from it's in-memory companion http proxy @@ -82,6 +87,15 @@ func Test_requestCtxInterceptor(t *testing.T) { return ctx, nil } + c := event.TestEventerConfig(t, "Test_requestCtxInterceptor", event.TestWithAuditSink(t), event.TestWithObservationSink(t)) + testLock := &sync.Mutex{} + testLogger := hclog.New(&hclog.LoggerOptions{ + Mutex: testLock, + Name: "test", + }) + testEventer, err := event.NewEventer(testLogger, testLock, "Test_requestCtxInterceptor", c.EventerConfig) + require.NoError(t, err) + tests := []struct { name string requestCtx context.Context @@ -89,6 +103,7 @@ func Test_requestCtxInterceptor(t *testing.T) { authTokenRepoFn common.AuthTokenRepoFactory serversRepoFn common.ServersRepoFactory kms *kms.Kms + eventer *event.Eventer ticket string wantFactoryErr bool wantFactoryErrMatch *errors.Template @@ -103,6 +118,7 @@ func Test_requestCtxInterceptor(t *testing.T) { authTokenRepoFn: atRepoFn, serversRepoFn: serversRepoFn, kms: kmsCache, + eventer: testEventer, wantFactoryErr: true, wantFactoryErrMatch: errors.T(errors.InvalidParameter), wantFactoryErrContains: "missing iam repo", @@ -113,6 +129,7 @@ func Test_requestCtxInterceptor(t *testing.T) { iamRepoFn: iamRepoFn, serversRepoFn: serversRepoFn, kms: kmsCache, + eventer: testEventer, wantFactoryErr: true, wantFactoryErrMatch: errors.T(errors.InvalidParameter), wantFactoryErrContains: "missing auth token repo", @@ -123,6 +140,7 @@ func Test_requestCtxInterceptor(t *testing.T) { iamRepoFn: iamRepoFn, authTokenRepoFn: atRepoFn, kms: kmsCache, + eventer: testEventer, wantFactoryErr: true, wantFactoryErrMatch: errors.T(errors.InvalidParameter), wantFactoryErrContains: "missing servers repo function", @@ -133,6 +151,17 @@ func Test_requestCtxInterceptor(t *testing.T) { iamRepoFn: iamRepoFn, authTokenRepoFn: atRepoFn, serversRepoFn: serversRepoFn, + eventer: testEventer, + wantFactoryErr: true, + wantFactoryErrMatch: errors.T(errors.InvalidParameter), + wantFactoryErrContains: "missing kms", + }, + { + name: "missing-eventer", + requestCtx: newReqCtx(validGatewayTicket), + iamRepoFn: iamRepoFn, + authTokenRepoFn: atRepoFn, + serversRepoFn: serversRepoFn, wantFactoryErr: true, wantFactoryErrMatch: errors.T(errors.InvalidParameter), wantFactoryErrContains: "missing kms", @@ -144,6 +173,7 @@ func Test_requestCtxInterceptor(t *testing.T) { authTokenRepoFn: atRepoFn, serversRepoFn: serversRepoFn, kms: kmsCache, + eventer: testEventer, wantFactoryErr: true, wantFactoryErrMatch: errors.T(errors.InvalidParameter), wantFactoryErrContains: "missing ticket", @@ -155,6 +185,7 @@ func Test_requestCtxInterceptor(t *testing.T) { authTokenRepoFn: atRepoFn, serversRepoFn: serversRepoFn, kms: kmsCache, + eventer: testEventer, ticket: validGatewayTicket, wantRequestErr: true, wantRequestErrMatch: errors.T(errors.Internal), @@ -170,6 +201,7 @@ func Test_requestCtxInterceptor(t *testing.T) { authTokenRepoFn: atRepoFn, serversRepoFn: serversRepoFn, kms: kmsCache, + eventer: testEventer, ticket: validGatewayTicket, wantRequestErr: true, wantRequestErrMatch: errors.T(errors.Internal), @@ -185,6 +217,7 @@ func Test_requestCtxInterceptor(t *testing.T) { authTokenRepoFn: atRepoFn, serversRepoFn: serversRepoFn, kms: kmsCache, + eventer: testEventer, ticket: validGatewayTicket, wantRequestErr: true, wantRequestErrMatch: errors.T(errors.Internal), @@ -200,6 +233,7 @@ func Test_requestCtxInterceptor(t *testing.T) { authTokenRepoFn: atRepoFn, serversRepoFn: serversRepoFn, kms: kmsCache, + eventer: testEventer, ticket: validGatewayTicket, wantRequestErr: true, wantRequestErrMatch: errors.T(errors.Internal), @@ -215,6 +249,7 @@ func Test_requestCtxInterceptor(t *testing.T) { authTokenRepoFn: atRepoFn, serversRepoFn: serversRepoFn, kms: kmsCache, + eventer: testEventer, ticket: validGatewayTicket, wantRequestErr: true, wantRequestErrMatch: errors.T(errors.Internal), @@ -230,6 +265,7 @@ func Test_requestCtxInterceptor(t *testing.T) { authTokenRepoFn: atRepoFn, serversRepoFn: serversRepoFn, kms: kmsCache, + eventer: testEventer, ticket: validGatewayTicket, wantRequestErr: true, wantRequestErrMatch: errors.T(errors.Internal), @@ -242,6 +278,7 @@ func Test_requestCtxInterceptor(t *testing.T) { authTokenRepoFn: atRepoFn, serversRepoFn: serversRepoFn, kms: kmsCache, + eventer: testEventer, ticket: "validGatewayTicket", wantRequestErr: true, wantRequestErrMatch: errors.T(errors.Internal), @@ -254,6 +291,7 @@ func Test_requestCtxInterceptor(t *testing.T) { authTokenRepoFn: atRepoFn, serversRepoFn: serversRepoFn, kms: kmsCache, + eventer: testEventer, ticket: "validGatewayTicket", wantRequestErr: true, wantRequestErrMatch: errors.T(errors.Internal), @@ -266,13 +304,14 @@ func Test_requestCtxInterceptor(t *testing.T) { authTokenRepoFn: atRepoFn, serversRepoFn: serversRepoFn, kms: kmsCache, + eventer: testEventer, ticket: validGatewayTicket, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { assert, require := assert.New(t), require.New(t) - interceptor, err := requestCtxInterceptor(factoryCtx, tt.iamRepoFn, tt.authTokenRepoFn, tt.serversRepoFn, tt.kms, tt.ticket) + interceptor, err := requestCtxInterceptor(factoryCtx, tt.iamRepoFn, tt.authTokenRepoFn, tt.serversRepoFn, tt.kms, tt.ticket, tt.eventer) if tt.wantFactoryErr { require.Error(err) assert.Nil(interceptor) diff --git a/internal/servers/controller/listeners.go b/internal/servers/controller/listeners.go index ad62e2822..d5c175ff4 100644 --- a/internal/servers/controller/listeners.go +++ b/internal/servers/controller/listeners.go @@ -26,7 +26,7 @@ func (c *Controller) startListeners(ctx context.Context) error { configureForAPI := func(ln *base.ServerListener) error { var err error - if c.gatewayServer, c.gatewayTicket, err = newGatewayServer(ctx, c.IamRepoFn, c.AuthTokenRepoFn, c.ServersRepoFn, c.kms); err != nil { + if c.gatewayServer, c.gatewayTicket, err = newGatewayServer(ctx, c.IamRepoFn, c.AuthTokenRepoFn, c.ServersRepoFn, c.kms, c.conf.Eventer); err != nil { return err } c.gatewayMux = newGatewayMux() From c361e6ec22bacf409c1c8747df623e9203cbdcbf Mon Sep 17 00:00:00 2001 From: Michael Gaffney Date: Tue, 26 Oct 2021 05:20:34 -0400 Subject: [PATCH 3/7] fix(warehouse): Add primary key to credential group membership table (#1636) --- .../migrations/oss/postgres/17/02_wh_primary_key.up.sql | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 internal/db/schema/migrations/oss/postgres/17/02_wh_primary_key.up.sql diff --git a/internal/db/schema/migrations/oss/postgres/17/02_wh_primary_key.up.sql b/internal/db/schema/migrations/oss/postgres/17/02_wh_primary_key.up.sql new file mode 100644 index 000000000..ea8e1f396 --- /dev/null +++ b/internal/db/schema/migrations/oss/postgres/17/02_wh_primary_key.up.sql @@ -0,0 +1,6 @@ +begin; + + alter table wh_credential_group_membership + add primary key (credential_group_key, credential_key); + +commit; From 24c6c329fe926c07c7d3e00053c42f83c7137ae1 Mon Sep 17 00:00:00 2001 From: Timothy Messier Date: Fri, 22 Oct 2021 08:51:46 -0400 Subject: [PATCH 4/7] chore(target): Remove unused option support from repo Create For the target Repository's Create method, the options of WithHostSources and WithCredentialSources where never used by the targets.Service. As such the repository does not need to support these options. See: 51812ff77937a3e5e6873995b07ae5400965ac7c --- internal/cmd/base/initial_resources.go | 5 +- .../handlers/targets/target_service_test.go | 6 ++- internal/target/repository.go | 40 +------------- .../target/tcp/repository_tcp_target_test.go | 54 ------------------- 4 files changed, 9 insertions(+), 96 deletions(-) diff --git a/internal/cmd/base/initial_resources.go b/internal/cmd/base/initial_resources.go index a0115f361..edb221e4b 100644 --- a/internal/cmd/base/initial_resources.go +++ b/internal/cmd/base/initial_resources.go @@ -508,7 +508,6 @@ func (b *Server) CreateInitialTarget(ctx context.Context) (target.Target, error) target.WithName("Generated target"), target.WithDescription("Provides an initial target in Boundary"), target.WithDefaultPort(uint32(b.DevTargetDefaultPort)), - target.WithHostSources([]string{b.DevHostSetId}), target.WithSessionMaxSeconds(uint32(b.DevTargetSessionMaxSeconds)), target.WithSessionConnectionLimit(int32(b.DevTargetSessionConnectionLimit)), target.WithPublicId(b.DevTargetId), @@ -521,6 +520,10 @@ func (b *Server) CreateInitialTarget(ctx context.Context) (target.Target, error) if err != nil { return nil, fmt.Errorf("error saving target to the db: %w", err) } + tt, _, _, err = targetRepo.AddTargetHostSources(ctx, tt.GetPublicId(), tt.GetVersion(), []string{b.DevHostSetId}) + if err != nil { + return nil, fmt.Errorf("error saving target to the db: %w", err) + } b.InfoKeys = append(b.InfoKeys, "generated target id") b.Info["generated target id"] = b.DevTargetId diff --git a/internal/servers/controller/handlers/targets/target_service_test.go b/internal/servers/controller/handlers/targets/target_service_test.go index 37b7329d2..700ad493b 100644 --- a/internal/servers/controller/handlers/targets/target_service_test.go +++ b/internal/servers/controller/handlers/targets/target_service_test.go @@ -575,11 +575,13 @@ func TestUpdate(t *testing.T) { tar, err := tcp.New(proj.GetPublicId(), target.WithName("default"), target.WithDescription("default")) tar.DefaultPort = 2 require.NoError(t, err) - gtar, _, _, err := repo.CreateTarget(context.Background(), tar, target.WithHostSources([]string{hs[0].GetPublicId(), hs[1].GetPublicId()})) + gtar, _, _, err := repo.CreateTarget(context.Background(), tar) + require.NoError(t, err) + gtar, _, _, err = repo.AddTargetHostSources(context.Background(), gtar.GetPublicId(), gtar.GetVersion(), []string{hs[0].GetPublicId(), hs[1].GetPublicId()}) require.NoError(t, err) tar = gtar.(*tcp.Target) - var version uint32 = 1 + var version uint32 = gtar.GetVersion() resetTarget := func() { version++ diff --git a/internal/target/repository.go b/internal/target/repository.go index 04843199c..fb965b68c 100644 --- a/internal/target/repository.go +++ b/internal/target/repository.go @@ -323,7 +323,7 @@ func (r *Repository) update(ctx context.Context, target Target, version uint32, // CreateTarget inserts into the repository and returns the new Target with // its list of host sets and credential libraries. -// WithHostSources and WithCredentialSources are the only supported option. +// WithPublicId is the only supported option. func (r *Repository) CreateTarget(ctx context.Context, target Target, opt ...Option) (Target, []HostSource, []CredentialSource, error) { const op = "target.(Repository).CreateTarget" opts := GetOpts(opt...) @@ -371,24 +371,6 @@ func (r *Repository) CreateTarget(ctx context.Context, target Target, opt ...Opt return nil, nil, nil, errors.Wrap(ctx, err, op, errors.WithMsg("unable to get oplog wrapper")) } - newHostSets := make([]interface{}, 0, len(opts.WithHostSources)) - for _, hsId := range opts.WithHostSources { - hostSet, err := NewTargetHostSet(t.GetPublicId(), hsId) - if err != nil { - return nil, nil, nil, errors.Wrap(ctx, err, op, errors.WithMsg("unable to create in memory target host set")) - } - newHostSets = append(newHostSets, hostSet) - } - - newCredLibs := make([]interface{}, 0, len(opts.WithCredentialSources)) - for _, clId := range opts.WithCredentialSources { - credLib, err := NewCredentialLibrary(t.GetPublicId(), clId) - if err != nil { - return nil, nil, nil, errors.Wrap(ctx, err, op, errors.WithMsg("unable to create in memory target credential library")) - } - newCredLibs = append(newCredLibs, credLib) - } - metadata := t.Oplog(oplog.OpType_OP_TYPE_CREATE) var returnedTarget interface{} var returnedHostSources []HostSource @@ -409,26 +391,6 @@ func (r *Repository) CreateTarget(ctx context.Context, target Target, opt ...Opt return errors.Wrap(ctx, err, op, errors.WithMsg("unable to create target")) } msgs = append(msgs, &targetOplogMsg) - if len(newHostSets) > 0 { - hostSetOplogMsgs := make([]*oplog.Message, 0, len(newHostSets)) - if err := w.CreateItems(ctx, newHostSets, db.NewOplogMsgs(&hostSetOplogMsgs)); err != nil { - return errors.Wrap(ctx, err, op, errors.WithMsg("unable to add host sets")) - } - if returnedHostSources, err = fetchHostSources(ctx, read, t.GetPublicId()); err != nil { - return errors.Wrap(ctx, err, op, errors.WithMsg("unable to read host sources")) - } - msgs = append(msgs, hostSetOplogMsgs...) - } - if len(newCredLibs) > 0 { - credLibOplogMsgs := make([]*oplog.Message, 0, len(newCredLibs)) - if err := w.CreateItems(ctx, newCredLibs, db.NewOplogMsgs(&credLibOplogMsgs)); err != nil { - return errors.Wrap(ctx, err, op, errors.WithMsg("unable to add credential sources")) - } - if returnedCredSources, err = fetchCredentialSources(ctx, read, t.GetPublicId()); err != nil { - return errors.Wrap(ctx, err, op, errors.WithMsg("unable to read credential sources")) - } - msgs = append(msgs, credLibOplogMsgs...) - } if err := w.WriteOplogEntryWith(ctx, oplogWrapper, targetTicket, metadata, msgs); err != nil { return errors.Wrap(ctx, err, op, errors.WithMsg("unable to write oplog")) } diff --git a/internal/target/tcp/repository_tcp_target_test.go b/internal/target/tcp/repository_tcp_target_test.go index 21b5806ed..6ae271b5c 100644 --- a/internal/target/tcp/repository_tcp_target_test.go +++ b/internal/target/tcp/repository_tcp_target_test.go @@ -72,60 +72,6 @@ func TestRepository_CreateTarget(t *testing.T) { wantCredLibs: []string{}, wantHostSources: []string{}, }, - { - name: "valid-org-with-host-sets", - args: args{ - target: func() *tcp.Target { - target, err := tcp.New(proj.PublicId, - target.WithName("valid-org-with-host-sets"), - target.WithDescription("valid-org"), - target.WithDefaultPort(uint32(22))) - require.NoError(t, err) - return target - }(), - opt: []target.Option{target.WithHostSources(sets)}, - }, - wantErr: false, - wantHostSources: sets, - wantCredLibs: []string{}, - }, - { - name: "valid-org-with-cred-libs", - args: args{ - target: func() *tcp.Target { - target, err := tcp.New(proj.PublicId, - target.WithName("valid-org-with-cred-libs"), - target.WithDescription("valid-org"), - target.WithDefaultPort(uint32(22))) - require.NoError(t, err) - return target - }(), - opt: []target.Option{target.WithCredentialSources(clIds)}, - }, - wantErr: false, - wantCredLibs: clIds, - wantHostSources: []string{}, - }, - { - name: "valid-org-with-cred-libs-and-host-sets", - args: args{ - target: func() *tcp.Target { - target, err := tcp.New(proj.PublicId, - target.WithName("valid-org-with-cred-libs-and-host-sets"), - target.WithDescription("valid-org"), - target.WithDefaultPort(uint32(22))) - require.NoError(t, err) - return target - }(), - opt: []target.Option{ - target.WithHostSources(sets), - target.WithCredentialSources(clIds), - }, - }, - wantErr: false, - wantCredLibs: clIds, - wantHostSources: sets, - }, { name: "nil-target", args: args{ From 3de16b6e2144ca99ba345041d4991e3d9b14798a Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Tue, 26 Oct 2021 16:30:02 -0400 Subject: [PATCH 5/7] Fix TLS 1.2 support (#1637) --- CHANGELOG.md | 12 ++++++++++++ internal/cmd/base/listener.go | 13 ++++++++++++- internal/cmd/base/servers.go | 4 +++- 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d6b8de7b0..f80167d01 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,18 @@ Canonical reference for changes, improvements, and bugfixes for Boundary. ## Next +### Deprecations/Changes + +* 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. + +### Bug Fixes + +* tls: Support TLS 1.2 for more clients. This was broken for some clients due to + a missing mandated cipher suite of the HTTP/2 (`h2`) specification that could + result in no shared cipher suites between the Boundary API listener and those + clients. ([PR](https://github.com/hashicorp/boundary/pull/1637)) + ## 0.6.2 (2021/09/27) ### Deprecations/Changes diff --git a/internal/cmd/base/listener.go b/internal/cmd/base/listener.go index cde813fe4..97bfbec0a 100644 --- a/internal/cmd/base/listener.go +++ b/internal/cmd/base/listener.go @@ -54,7 +54,7 @@ func NewListener(l *listenerutil.ListenerConfig, ui cli.Ui) (*alpnmux.ALPNMux, m } if len(l.Purpose) != 1 { - return nil, nil, nil, fmt.Errorf("Expected single listener purpose, found %d", len(l.Purpose)) + return nil, nil, nil, fmt.Errorf("expected single listener purpose, found %d", len(l.Purpose)) } purpose := l.Purpose[0] @@ -65,6 +65,17 @@ func NewListener(l *listenerutil.ListenerConfig, ui cli.Ui) (*alpnmux.ALPNMux, m // TODO: Eventually we'll support bringing your own cert, and we'd only // want to disable if you aren't actually bringing your own l.TLSDisable = true + default: + switch l.TLSMinVersion { + case "", "tls12", "tls13": + default: + return nil, nil, nil, fmt.Errorf("unsupported minimum tls version %q", l.TLSMinVersion) + } + switch l.TLSMaxVersion { + case "", "tls12", "tls13": + default: + return nil, nil, nil, fmt.Errorf("unsupported maximum tls version %q", l.TLSMaxVersion) + } } finalAddr, ln, err := f(purpose, l, ui) diff --git a/internal/cmd/base/servers.go b/internal/cmd/base/servers.go index 90d7671b3..965d49716 100644 --- a/internal/cmd/base/servers.go +++ b/internal/cmd/base/servers.go @@ -390,7 +390,9 @@ func (b *Server) SetupListeners(ui cli.Ui, config *configutil.SharedConfig, allo // 1.2 tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, tls.TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384, - tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305, + tls.TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256, + tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384, } } From a679300b506fb843ffcbf70917e4cff1b350b54b Mon Sep 17 00:00:00 2001 From: Jim Date: Wed, 27 Oct 2021 09:37:14 -0400 Subject: [PATCH 6/7] feature (events): Classify auth method request/resp messages for audit events. (#1640) * refactor (oidc): Stop emitting error for not found token id Stop emitting errors when authtoken repo.IssueAuthToken is called and there's if no pending token. It's not an "normal" state and not an error condition. * feature (audit tagging): Add tags for auth method requests/responses Including testing functions for asserting audit events created when a service is called --- CONTRIBUTING.md | 1 + go.mod | 4 +- go.sum | 6 +- internal/adding-a-new-field-readme.md | 15 ++- internal/authtoken/repository.go | 2 +- .../api/services/auth_method_service.pb.go | 10 +- .../services/auth_method_service_taggable.go | 67 ++++++++++++ .../auth_method_service_taggable_test.go | 72 +++++++++++-- internal/observability/event/eventer.go | 21 +++- .../authmethods/v1/auth_method.proto | 20 ++-- .../api/services/v1/auth_method_service.proto | 10 +- .../api/authmethods/authenticate_test.go | 50 ++++++++- .../tests/api/authmethods/authmethod_test.go | 101 ++++++++++++++++-- internal/tests/api/testing.go | 80 ++++++++++++++ sdk/go.mod | 2 +- sdk/go.sum | 4 +- .../resources/authmethods/auth_method.pb.go | 20 ++-- .../authmethods/auth_method_taggable.go | 4 + .../authmethods/auth_method_taggable_test.go | 14 ++- sdk/pbs/controller/api/testing.go | 23 ++++ 20 files changed, 452 insertions(+), 74 deletions(-) create mode 100644 internal/tests/api/testing.go create mode 100644 sdk/pbs/controller/api/testing.go diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6cd3dfcd6..9167495cc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -183,3 +183,4 @@ Note that if `max_connections` is set too low, it may result in sporadic test failures if a connection cannot be established. In this case, reduce the number of concurrent tests via `GOMAXPROCS` or selectively run tests. +## [Adding additional field to an existing API (or new API)](internal/adding-a-new-field-readme.md) diff --git a/go.mod b/go.mod index 29869f0d4..c4e5177af 100644 --- a/go.mod +++ b/go.mod @@ -18,7 +18,7 @@ require ( github.com/golang-sql/civil v0.0.0-20190719163853-cb61b32ac6fe github.com/golang/protobuf v1.5.2 github.com/google/go-cmp v0.5.6 - github.com/grpc-ecosystem/go-grpc-middleware v1.3.0 // indirect + github.com/grpc-ecosystem/go-grpc-middleware v1.3.0 github.com/grpc-ecosystem/grpc-gateway/v2 v2.6.0 github.com/hashicorp/boundary/api v0.0.19 github.com/hashicorp/boundary/sdk v0.0.11 @@ -26,7 +26,7 @@ require ( github.com/hashicorp/dawdle v0.4.0 github.com/hashicorp/dbassert v0.0.0-20210708202608-ecf920cf1ed8 github.com/hashicorp/eventlogger v0.1.0 - github.com/hashicorp/eventlogger/filters/encrypt v0.1.4-0.20210928205053-80364fba97eb + github.com/hashicorp/eventlogger/filters/encrypt v0.1.5 github.com/hashicorp/go-bexpr v0.1.10 github.com/hashicorp/go-cleanhttp v0.5.2 github.com/hashicorp/go-hclog v0.16.2 diff --git a/go.sum b/go.sum index c36a7696e..69c68d267 100644 --- a/go.sum +++ b/go.sum @@ -458,9 +458,9 @@ github.com/hashicorp/errwrap v1.1.0 h1:OxrOeh75EUXMY8TBjag2fzXGZ40LB6IKw45YeGUDY github.com/hashicorp/errwrap v1.1.0/go.mod h1:YH+1FKiLXxHSkmPseP+kNlulaMuP3n2brvKWEqk/Jc4= github.com/hashicorp/eventlogger v0.1.0 h1:S6xc4gZVzewuDUP4R4Ngko419h/CGDuV/b4ADL3XLik= github.com/hashicorp/eventlogger v0.1.0/go.mod h1:a3IXf1aEJfpCPzseTOrwKj4fVW/Qn3oEmpQeaIznzH0= -github.com/hashicorp/eventlogger/filters/encrypt v0.1.3/go.mod h1:8rcez7Kw1zanB0/074qnOuGu7zxmNh9Xr2ZI+K4xVIA= -github.com/hashicorp/eventlogger/filters/encrypt v0.1.4-0.20210928205053-80364fba97eb h1:AhprXfDoXGI8hP5fUCyCB29jpfd01Ck996ErqG0nVGk= -github.com/hashicorp/eventlogger/filters/encrypt v0.1.4-0.20210928205053-80364fba97eb/go.mod h1:8rcez7Kw1zanB0/074qnOuGu7zxmNh9Xr2ZI+K4xVIA= +github.com/hashicorp/eventlogger/filters/encrypt v0.1.5-0.20211025115820-78e1ded4aea1/go.mod h1:8rcez7Kw1zanB0/074qnOuGu7zxmNh9Xr2ZI+K4xVIA= +github.com/hashicorp/eventlogger/filters/encrypt v0.1.5 h1:kNkH4G6zzWlZSoI1I+B/ud4chVKTPL516C6jB7dRdlE= +github.com/hashicorp/eventlogger/filters/encrypt v0.1.5/go.mod h1:8rcez7Kw1zanB0/074qnOuGu7zxmNh9Xr2ZI+K4xVIA= github.com/hashicorp/go-bexpr v0.1.10 h1:9kuI5PFotCboP3dkDYFr/wi0gg0QVbSNz5oFRpxn4uE= github.com/hashicorp/go-bexpr v0.1.10/go.mod h1:oxlubA2vC/gFVfX1A6JGp7ls7uCDlfJn732ehYYg+g0= github.com/hashicorp/go-cleanhttp v0.5.0/go.mod h1:JpRdi6/HCYpAwUzNwuwqhbovhLtngrth3wmdIIUrZ80= diff --git a/internal/adding-a-new-field-readme.md b/internal/adding-a-new-field-readme.md index 753309df8..96146074a 100644 --- a/internal/adding-a-new-field-readme.md +++ b/internal/adding-a-new-field-readme.md @@ -7,13 +7,16 @@ Once you've figured out that you need an additional field in Boundary's domain m * Make schema changes: * Define the new column and provide for the migration of existing data - * Create a new migration under `internal/db/schema/migrations/postgres` - * Run `make migrations` after modifying DDL in SQL files. + + * Create a new migration under `internal/db/schema/migrations/oss/postgres` * Add the new field to the storage protobuf * storage protobufs are under: `internal/proto/local/controller/storage` - * Define a gorm tag for the new field via `@inject_tag` + + * Define a gorm tag for the new field via `@gotags` (`@inject_tag` has been deprecated) + * Define a `custom_options.v1.mask_mapping` for the field which maps the storage `this` field to the API `that` field (yes, it's the opposite of how it's defined for the API protobuf) + * Run `make proto` after modifying storage protobuf * Extend the existing repository function for Updating the resource to incorporate the new field. This could/may entail defining new options for the Update function. @@ -29,6 +32,12 @@ Now that the repository supports the new field, you can move on to adding this n * Define a `custom_options.v1.generate_sdk_option` tag for the SDK +* Define a data classification/filter tag for the field via `@gotags` or the + `encrypt.Taggable` interface which specifies how sensitive/secret/public data + will be handled for the API's audit events. Please write unit tests to verify + the audit event is properly "redacted" (see the unit tests of + `TestAuthenticate_Tags` and `TestAuthenticate` for examples). + * Run `make proto` and `make api` after modifying the API/SDK protobufs ## Update the API handler service diff --git a/internal/authtoken/repository.go b/internal/authtoken/repository.go index 3853d1901..6f59dd416 100644 --- a/internal/authtoken/repository.go +++ b/internal/authtoken/repository.go @@ -383,7 +383,7 @@ func (r *Repository) IssueAuthToken(ctx context.Context, tokenRequestId string) // trigger to set ApproximateLastAccessTime to the commit timestamp. rowsUpdated, err := w.Update(ctx, at, []string{"Status"}, []string{"ApproximateLastAccessTime"}, db.WithWhere("status = ?", PendingStatus)) if err != nil { - return errors.Wrap(ctx, err, op) + return errors.Wrap(ctx, err, op, errors.WithoutEvent()) } if rowsUpdated == 0 { return errors.New(ctx, errors.RecordNotFound, op, "pending auth token not found") diff --git a/internal/gen/controller/api/services/auth_method_service.pb.go b/internal/gen/controller/api/services/auth_method_service.pb.go index a22b7cb26..ce3b85cd3 100644 --- a/internal/gen/controller/api/services/auth_method_service.pb.go +++ b/internal/gen/controller/api/services/auth_method_service.pb.go @@ -30,7 +30,7 @@ type GetAuthMethodRequest struct { sizeCache protoimpl.SizeCache unknownFields protoimpl.UnknownFields - Id string `protobuf:"bytes,1,opt,name=id,proto3" json:"id,omitempty"` + Id string `protobuf:"bytes,1,opt,name=id,proto3" json:"id,omitempty" class:"public"` // @gotags: `class:"public"` } func (x *GetAuthMethodRequest) Reset() { @@ -281,7 +281,7 @@ type CreateAuthMethodResponse struct { sizeCache protoimpl.SizeCache unknownFields protoimpl.UnknownFields - Uri string `protobuf:"bytes,1,opt,name=uri,proto3" json:"uri,omitempty"` + Uri string `protobuf:"bytes,1,opt,name=uri,proto3" json:"uri,omitempty" class:"public"` // @gotags: `class:"public"` Item *authmethods.AuthMethod `protobuf:"bytes,2,opt,name=item,proto3" json:"item,omitempty"` } @@ -336,7 +336,7 @@ type UpdateAuthMethodRequest struct { sizeCache protoimpl.SizeCache unknownFields protoimpl.UnknownFields - Id string `protobuf:"bytes,1,opt,name=id,proto3" json:"id,omitempty"` + Id string `protobuf:"bytes,1,opt,name=id,proto3" json:"id,omitempty" class:"public"` // @gotags: `class:"public"` Item *authmethods.AuthMethod `protobuf:"bytes,2,opt,name=item,proto3" json:"item,omitempty"` UpdateMask *fieldmaskpb.FieldMask `protobuf:"bytes,3,opt,name=update_mask,proto3" json:"update_mask,omitempty"` } @@ -591,7 +591,7 @@ type ChangeStateRequest struct { sizeCache protoimpl.SizeCache unknownFields protoimpl.UnknownFields - Id string `protobuf:"bytes,1,opt,name=id,proto3" json:"id,omitempty"` + Id string `protobuf:"bytes,1,opt,name=id,proto3" json:"id,omitempty" class:"public"` // @gotags: `class:"public"` // Version is used to ensure this resource has not changed. // The mutation will fail if the version does not match the latest known good version. Version uint32 `protobuf:"varint,2,opt,name=version,proto3" json:"version,omitempty"` @@ -876,7 +876,7 @@ type AuthenticateRequest struct { AuthMethodId string `protobuf:"bytes,1,opt,name=auth_method_id,proto3" json:"auth_method_id,omitempty" class:"public"` // @gotags: `class:"public"` // This can be "cookie" or "token". If not provided, "token" will be used. "cookie" activates a split-cookie method where the token is split partially between http-only and regular cookies in order // to keep it safe from rogue JS in the browser. - TokenType string `protobuf:"bytes,2,opt,name=token_type,proto3" json:"token_type,omitempty"` + TokenType string `protobuf:"bytes,2,opt,name=token_type,proto3" json:"token_type,omitempty" class:"public"` // @gotags: `class:"public"` // Attributes are passed to the Auth Method; the valid keys and values depend on the type of Auth Method as well as the command. Attributes *structpb.Struct `protobuf:"bytes,4,opt,name=attributes,proto3" json:"attributes,omitempty"` // The command to perform. diff --git a/internal/gen/controller/api/services/auth_method_service_taggable.go b/internal/gen/controller/api/services/auth_method_service_taggable.go index 7359eaf3d..c359a0bed 100644 --- a/internal/gen/controller/api/services/auth_method_service_taggable.go +++ b/internal/gen/controller/api/services/auth_method_service_taggable.go @@ -54,6 +54,22 @@ func (req *AuthenticateResponse) Tags() ([]encrypt.PointerTag, error) { Pointer: "/Attributes/Fields/user_id", Classification: encrypt.PublicClassification, }, + { + Pointer: "/Attributes/Fields/status", + Classification: encrypt.PublicClassification, + }, + { + Pointer: "/Attributes/Fields/auth_url", + Classification: encrypt.PublicClassification, + }, + { + Pointer: "/Attributes/Fields/token_id", + Classification: encrypt.PublicClassification, + }, + { + Pointer: "/Attributes/Fields/final_redirect_url", + Classification: encrypt.PublicClassification, + }, // secret fields { Pointer: "/Attributes/Fields/token", @@ -61,3 +77,54 @@ func (req *AuthenticateResponse) Tags() ([]encrypt.PointerTag, error) { }, }, nil } + +// Tags implements the encrypt.Taggable interface which allows +// AuthenticateRequest Attributes to be classified for the encrypt filter. +func (req *AuthenticateRequest) Tags() ([]encrypt.PointerTag, error) { + if req.Attributes == nil { + return nil, nil + } + return []encrypt.PointerTag{ + // public fields + { + Pointer: "/Attributes/Fields/login_name", + Classification: encrypt.PublicClassification, + }, + { + Pointer: "/Attributes/Fields/auth_url", + Classification: encrypt.PublicClassification, + }, + { + Pointer: "/Attributes/Fields/token_id", + Classification: encrypt.PublicClassification, + }, + { + Pointer: "/Attributes/Fields/state", + Classification: encrypt.PublicClassification, + }, + // secret fields + { + Pointer: "/Attributes/Fields/password", + Classification: encrypt.SecretClassification, + }, + { + Pointer: "/Attributes/Fields/code", + Classification: encrypt.SecretClassification, + }, + }, nil +} + +// Tags implements the encrypt.Taggable interface which allows +// ChangeStateRequest Attributes to be classified for the encrypt filter. +func (req *ChangeStateRequest) Tags() ([]encrypt.PointerTag, error) { + if req.Attributes == nil { + return nil, nil + } + return []encrypt.PointerTag{ + // public fields + { + Pointer: "/Attributes/Fields/state", + Classification: encrypt.PublicClassification, + }, + }, nil +} diff --git a/internal/gen/controller/api/services/auth_method_service_taggable_test.go b/internal/gen/controller/api/services/auth_method_service_taggable_test.go index aee0e8602..0dc6fb6b6 100644 --- a/internal/gen/controller/api/services/auth_method_service_taggable_test.go +++ b/internal/gen/controller/api/services/auth_method_service_taggable_test.go @@ -1,4 +1,4 @@ -package services +package services_test import ( "context" @@ -6,23 +6,24 @@ import ( "testing" "time" + "github.com/hashicorp/boundary/internal/gen/controller/api/services" + "github.com/hashicorp/boundary/sdk/pbs/controller/api" "github.com/hashicorp/boundary/sdk/wrapper" "github.com/hashicorp/eventlogger" - "github.com/hashicorp/eventlogger/filters/encrypt" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" structpb "google.golang.org/protobuf/types/known/structpb" ) -func TestScope_Tags(t *testing.T) { +// TestAuthenticate_Tags will test that the response filtering aligns with the +// AuthenticateResponse and AuthenticateResponse tags. See: +// internal/tests/api/authmethods/authenticate_test.go TestAuthenticate where +// the audit events produced using these tags is unit tested. +func TestAuthenticate_Tags(t *testing.T) { ctx := context.Background() now := time.Now() wrapper := wrapper.TestWrapper(t) - testEncryptingFilter := &encrypt.Filter{ - Wrapper: wrapper, - HmacSalt: []byte("salt"), - HmacInfo: []byte("info"), - } + testEncryptingFilter := api.NewEncryptFilter(t, wrapper) tests := []struct { name string @@ -30,11 +31,11 @@ func TestScope_Tags(t *testing.T) { wantEvent *eventlogger.Event }{ { - name: "validate-filtering", + name: "validate-response-filtering", testEvent: &eventlogger.Event{ Type: "test", CreatedAt: now, - Payload: &AuthenticateResponse{ + Payload: &services.AuthenticateResponse{ Command: "public-command", Attributes: &structpb.Struct{ Fields: map[string]*structpb.Value{ @@ -49,6 +50,10 @@ func TestScope_Tags(t *testing.T) { "token_type": structpb.NewStringValue("public-token_type"), "updated_time": structpb.NewStringValue("public-updated_time"), "user_id": structpb.NewStringValue("public-user_id"), + "status": structpb.NewStringValue("public-status"), + "auth_url": structpb.NewStringValue("public-auth_url"), + "token_id": structpb.NewStringValue("public-token_id"), + "final_redirect_url": structpb.NewStringValue("public-final_redirect_url"), "token": structpb.NewStringValue("secret-token"), }, }, @@ -57,7 +62,7 @@ func TestScope_Tags(t *testing.T) { wantEvent: &eventlogger.Event{ Type: "test", CreatedAt: now, - Payload: &AuthenticateResponse{ + Payload: &services.AuthenticateResponse{ Command: "public-command", Attributes: &structpb.Struct{ Fields: map[string]*structpb.Value{ @@ -72,12 +77,57 @@ func TestScope_Tags(t *testing.T) { "token_type": structpb.NewStringValue("public-token_type"), "updated_time": structpb.NewStringValue("public-updated_time"), "user_id": structpb.NewStringValue("public-user_id"), + "status": structpb.NewStringValue("public-status"), + "auth_url": structpb.NewStringValue("public-auth_url"), + "token_id": structpb.NewStringValue("public-token_id"), + "final_redirect_url": structpb.NewStringValue("public-final_redirect_url"), "token": structpb.NewStringValue(""), }, }, }, }, }, + { + name: "validate-request-filtering", + testEvent: &eventlogger.Event{ + Type: "test", + CreatedAt: now, + Payload: &services.AuthenticateRequest{ + AuthMethodId: "public-auth-method-id", + TokenType: "public-token-type", + Command: "public-command", + Attributes: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + "login_name": structpb.NewStringValue("public-login_name"), + "auth_url": structpb.NewStringValue("public-auth_url"), + "token_id": structpb.NewStringValue("public-token_id"), + "state": structpb.NewStringValue("public-state"), + "password": structpb.NewStringValue("secret-password"), + "code": structpb.NewStringValue("secret-code"), + }, + }, + }, + }, + wantEvent: &eventlogger.Event{ + Type: "test", + CreatedAt: now, + Payload: &services.AuthenticateRequest{ + AuthMethodId: "public-auth-method-id", + TokenType: "public-token-type", + Command: "public-command", + Attributes: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + "login_name": structpb.NewStringValue("public-login_name"), + "auth_url": structpb.NewStringValue("public-auth_url"), + "token_id": structpb.NewStringValue("public-token_id"), + "state": structpb.NewStringValue("public-state"), + "password": structpb.NewStringValue(""), + "code": structpb.NewStringValue(""), + }, + }, + }, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/internal/observability/event/eventer.go b/internal/observability/event/eventer.go index 2943e0bc6..4bf82eab4 100644 --- a/internal/observability/event/eventer.go +++ b/internal/observability/event/eventer.go @@ -9,6 +9,7 @@ import ( "log" "net/url" "os" + "reflect" "runtime" "strings" "sync" @@ -19,8 +20,8 @@ import ( "github.com/hashicorp/eventlogger/filters/gated" "github.com/hashicorp/eventlogger/formatter_filters/cloudevents" "github.com/hashicorp/eventlogger/sinks/writer" - "github.com/hashicorp/go-hclog" + "google.golang.org/protobuf/types/known/fieldmaskpb" ) const ( @@ -137,6 +138,19 @@ func SysEventer() *Eventer { return sysEventer } +// NewAuditEncryptFilter returns a new encrypt filter which is initialized for +// audit events. +func NewAuditEncryptFilter(opt ...Option) (*encrypt.Filter, error) { + opts := getOpts(opt...) + + return &encrypt.Filter{ + Wrapper: opts.withAuditWrapper, + IgnoreTypes: []reflect.Type{ + reflect.TypeOf(&fieldmaskpb.FieldMask{}), + }, + }, nil +} + // NewEventer creates a new Eventer using the config. Supports options: // WithNow, WithSerializationLock, WithBroker, WithAuditWrapper func NewEventer(log hclog.Logger, serializationLock *sync.Mutex, serverName string, c EventerConfig, opt ...Option) (*Eventer, error) { @@ -269,8 +283,9 @@ func NewEventer(log hclog.Logger, serializationLock *sync.Mutex, serverName stri if err != nil { return nil, fmt.Errorf("%s: %w", op, err) } - encryptFilter := &encrypt.Filter{ - Wrapper: opts.withAuditWrapper, + encryptFilter, err := NewAuditEncryptFilter(opt...) + if err != nil { + return nil, fmt.Errorf("%s: %w", op, err) } if len(s.AuditConfig.FilterOverrides) > 0 { overrides := encrypt.DefaultFilterOperations() diff --git a/internal/proto/local/controller/api/resources/authmethods/v1/auth_method.proto b/internal/proto/local/controller/api/resources/authmethods/v1/auth_method.proto index a34480a4e..ba4a16bfc 100644 --- a/internal/proto/local/controller/api/resources/authmethods/v1/auth_method.proto +++ b/internal/proto/local/controller/api/resources/authmethods/v1/auth_method.proto @@ -153,36 +153,36 @@ message OidcAuthMethodAttributes { // The structure of the OIDC authenticate start response, in the JSON object message OidcAuthMethodAuthenticateStartResponse { // The returned authentication URL - string auth_url = 10 [json_name = "auth_url"]; + string auth_url = 10 [json_name = "auth_url"]; // @gotags: `class:"public"` // The returned token ID - string token_id = 30 [json_name = "token_id"]; + string token_id = 30 [json_name = "token_id"]; // @gotags: `class:"public"` } // The structure of OIDC callback request parameters message OidcAuthMethodAuthenticateCallbackRequest { // The returned code - string code = 10 [json_name = "code"]; + string code = 10 [json_name = "code"]; // @gotags: `class:"secret"` // The returned state - string state = 20 [json_name = "state"]; + string state = 20 [json_name = "state"]; // @gotags: `class:"public"` // Error parameters, if they are returned - string error = 30 [json_name = "error"]; - string error_description = 40 [json_name = "error_description"]; - string error_uri = 50 [json_name = "error_uri"]; + string error = 30 [json_name = "error"]; // @gotags: `class:"public"` + string error_description = 40 [json_name = "error_description"]; // @gotags: `class:"public"` + string error_uri = 50 [json_name = "error_uri"]; // @gotags: `class:"public"` } // The structure of OIDC callback response parameters message OidcAuthMethodAuthenticateCallbackResponse { // The final redirection URL - string final_redirect_url = 10 [json_name = "final_redirect_url"]; + string final_redirect_url = 10 [json_name = "final_redirect_url"]; // @gotags: `class:"public"` } // The structure of OIDC token request parameters message OidcAuthMethodAuthenticateTokenRequest { // The ID of the pending token - string token_id = 10 [json_name = "token_id"]; + string token_id = 10 [json_name = "token_id"]; // @gotags: `class:"private"` } // Internal only: the structure of a token response if it _does not_ contain a @@ -190,5 +190,5 @@ message OidcAuthMethodAuthenticateTokenRequest { message OidcAuthMethodAuthenticateTokenResponse { // The status. This will always be "unknown". It will never be forwarded to // the consumer. - string status = 10; + string status = 10; // @gotags: `class:"public"` } \ No newline at end of file diff --git a/internal/proto/local/controller/api/services/v1/auth_method_service.proto b/internal/proto/local/controller/api/services/v1/auth_method_service.proto index c66e0ec96..9e2324f03 100644 --- a/internal/proto/local/controller/api/services/v1/auth_method_service.proto +++ b/internal/proto/local/controller/api/services/v1/auth_method_service.proto @@ -107,7 +107,7 @@ service AuthMethodService { } message GetAuthMethodRequest { - string id = 1; + string id = 1; // @gotags: `class:"public"` } message GetAuthMethodResponse { @@ -129,12 +129,12 @@ message CreateAuthMethodRequest { } message CreateAuthMethodResponse { - string uri = 1; + string uri = 1; // @gotags: `class:"public"` resources.authmethods.v1.AuthMethod item = 2; } message UpdateAuthMethodRequest { - string id = 1; + string id = 1; // @gotags: `class:"public"` resources.authmethods.v1.AuthMethod item = 2; google.protobuf.FieldMask update_mask = 3 [json_name = "update_mask"]; } @@ -160,7 +160,7 @@ message OidcChangeStateAttributes { } message ChangeStateRequest { - string id = 1; + string id = 1; // @gotags: `class:"public"` // Version is used to ensure this resource has not changed. // The mutation will fail if the version does not match the latest known good version. uint32 version = 2; @@ -203,7 +203,7 @@ message AuthenticateRequest { string auth_method_id = 1 [json_name = "auth_method_id"]; // @gotags: `class:"public"` // This can be "cookie" or "token". If not provided, "token" will be used. "cookie" activates a split-cookie method where the token is split partially between http-only and regular cookies in order // to keep it safe from rogue JS in the browser. - string token_type = 2 [json_name = "token_type"]; + string token_type = 2 [json_name = "token_type"]; // @gotags: `class:"public"` // Attributes are passed to the Auth Method; the valid keys and values depend on the type of Auth Method as well as the command. google.protobuf.Struct attributes = 4 [json_name = "attributes"]; // The command to perform. diff --git a/internal/tests/api/authmethods/authenticate_test.go b/internal/tests/api/authmethods/authenticate_test.go index 55ee44ed3..0cc5a1eee 100644 --- a/internal/tests/api/authmethods/authenticate_test.go +++ b/internal/tests/api/authmethods/authenticate_test.go @@ -4,19 +4,41 @@ import ( "encoding/json" "fmt" "net/http" + "os" + "sync" "testing" "github.com/hashicorp/boundary/api" "github.com/hashicorp/boundary/api/authmethods" "github.com/hashicorp/boundary/api/authtokens" + "github.com/hashicorp/boundary/internal/cmd/config" + "github.com/hashicorp/boundary/internal/observability/event" "github.com/hashicorp/boundary/internal/servers/controller" + tests_api "github.com/hashicorp/boundary/internal/tests/api" + "github.com/hashicorp/go-hclog" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +// TestAuthenticate tests the api calls and the audit events it should produce func TestAuthenticate(t *testing.T) { + // this cannot run in parallel because it relies on envvar + // globals.BOUNDARY_DEVELOPER_ENABLE_EVENTS + event.TestEnableEventing(t, true) + assert, require := assert.New(t), require.New(t) - tc := controller.NewTestController(t, nil) + eventConfig := event.TestEventerConfig(t, "TestAuthenticateAuditEntry", event.TestWithAuditSink(t)) + testLock := &sync.Mutex{} + testLogger := hclog.New(&hclog.LoggerOptions{ + Mutex: testLock, + Name: "test", + }) + require.NoError(event.InitSysEventer(testLogger, testLock, "TestAuthenticateAuditEntry", event.WithEventerConfig(&eventConfig.EventerConfig))) + tcConfig, err := config.DevController() + require.NoError(err) + tcConfig.Eventing = &eventConfig.EventerConfig + + tc := controller.NewTestController(t, &controller.TestControllerOpts{Config: tcConfig}) defer tc.Shutdown() client := tc.Client() @@ -49,4 +71,30 @@ func TestAuthenticate(t *testing.T) { token := new(authtokens.AuthToken) require.NoError(json.Unmarshal(result.GetRawAttributes(), token)) require.NotEmpty(token.Token) + + require.NotNil(eventConfig.AuditEvents) + _ = os.WriteFile(eventConfig.AuditEvents.Name(), nil, 0o666) // clean out audit events from previous calls + + tok, err = methods.Authenticate(tc.Context(), tc.Server().DevPasswordAuthMethodId, "login", map[string]interface{}{"login_name": "user", "password": "passpass"}) + require.NoError(err) + assert.NotNil(tok) + got := tests_api.CloudEventFromFile(t, eventConfig.AuditEvents.Name()) + + reqDetails := tests_api.GetEventDetails(t, got, "request") + tests_api.AssertRedactedValues(t, reqDetails) + tests_api.AssertRedactedValues(t, reqDetails["attributes"], "password") + + respDetails := tests_api.GetEventDetails(t, got, "response") + tests_api.AssertRedactedValues(t, respDetails) + tests_api.AssertRedactedValues(t, respDetails["attributes"], "token") + + _ = os.WriteFile(eventConfig.AuditEvents.Name(), nil, 0o666) // clean out audit events from previous calls + tok, err = methods.Authenticate(tc.Context(), tc.Server().DevPasswordAuthMethodId, "login", map[string]interface{}{"login_name": "user", "password": "bad-pass"}) + require.Error(err) + assert.Nil(tok) + got = tests_api.CloudEventFromFile(t, eventConfig.AuditEvents.Name()) + + reqDetails = tests_api.GetEventDetails(t, got, "request") + tests_api.AssertRedactedValues(t, reqDetails) + tests_api.AssertRedactedValues(t, reqDetails["attributes"], "password") } diff --git a/internal/tests/api/authmethods/authmethod_test.go b/internal/tests/api/authmethods/authmethod_test.go index 6e54f7ddc..4936e49c8 100644 --- a/internal/tests/api/authmethods/authmethod_test.go +++ b/internal/tests/api/authmethods/authmethod_test.go @@ -2,13 +2,19 @@ package authmethods_test import ( "net/http" + "os" + "sync" "testing" "github.com/hashicorp/boundary/api" "github.com/hashicorp/boundary/api/authmethods" "github.com/hashicorp/boundary/internal/auth/password" + "github.com/hashicorp/boundary/internal/cmd/config" + "github.com/hashicorp/boundary/internal/observability/event" "github.com/hashicorp/boundary/internal/servers/controller" + tests_api "github.com/hashicorp/boundary/internal/tests/api" capoidc "github.com/hashicorp/cap/oidc" + "github.com/hashicorp/go-hclog" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -16,8 +22,23 @@ import ( const global = "global" func TestCrud(t *testing.T) { + // this cannot run in parallel because it relies on envvar + // globals.BOUNDARY_DEVELOPER_ENABLE_EVENTS + event.TestEnableEventing(t, true) + assert, require := assert.New(t), require.New(t) - tc := controller.NewTestController(t, nil) + eventConfig := event.TestEventerConfig(t, "TestCrud", event.TestWithAuditSink(t)) + testLock := &sync.Mutex{} + testLogger := hclog.New(&hclog.LoggerOptions{ + Mutex: testLock, + Name: "test", + }) + require.NoError(event.InitSysEventer(testLogger, testLock, "TestCrud", event.WithEventerConfig(&eventConfig.EventerConfig))) + tcConfig, err := config.DevController() + require.NoError(err) + tcConfig.Eventing = &eventConfig.EventerConfig + + tc := controller.NewTestController(t, &controller.TestControllerOpts{Config: tcConfig}) defer tc.Shutdown() client := tc.Client() @@ -35,18 +56,44 @@ func TestCrud(t *testing.T) { assert.EqualValues(wantedVersion, u.Version) } + require.NotNil(eventConfig.AuditEvents) + _ = os.WriteFile(eventConfig.AuditEvents.Name(), nil, 0o666) // clean out audit events from previous calls u, err := amClient.Create(tc.Context(), "password", global, authmethods.WithName("bar")) require.NoError(err) checkAuthMethod("create", u.Item, "bar", 1) + got := tests_api.CloudEventFromFile(t, eventConfig.AuditEvents.Name()) + reqItem := tests_api.GetEventDetails(t, got, "request")["item"].(map[string]interface{}) + tests_api.AssertRedactedValues(t, reqItem) + + respItem := tests_api.GetEventDetails(t, got, "response")["item"].(map[string]interface{}) + tests_api.AssertRedactedValues(t, respItem) + tests_api.AssertRedactedValues(t, respItem["attributes"]) + + _ = os.WriteFile(eventConfig.AuditEvents.Name(), nil, 0o666) // clean out audit events from previous calls u, err = amClient.Read(tc.Context(), u.Item.Id) require.NoError(err) checkAuthMethod("read", u.Item, "bar", 1) + got = tests_api.CloudEventFromFile(t, eventConfig.AuditEvents.Name()) + tests_api.AssertRedactedValues(t, tests_api.GetEventDetails(t, got, "request")) + + respItem = tests_api.GetEventDetails(t, got, "response")["item"].(map[string]interface{}) + tests_api.AssertRedactedValues(t, respItem) + tests_api.AssertRedactedValues(t, respItem["attributes"]) + + _ = os.WriteFile(eventConfig.AuditEvents.Name(), nil, 0o666) // clean out audit events from previous calls u, err = amClient.Update(tc.Context(), u.Item.Id, u.Item.Version, authmethods.WithName("buz")) require.NoError(err) checkAuthMethod("update", u.Item, "buz", 2) + got = tests_api.CloudEventFromFile(t, eventConfig.AuditEvents.Name()) + + tests_api.AssertRedactedValues(t, tests_api.GetEventDetails(t, got, "request")["item"].(map[string]interface{})) + + respItem = tests_api.GetEventDetails(t, got, "response")["item"].(map[string]interface{}) + tests_api.AssertRedactedValues(t, respItem) + tests_api.AssertRedactedValues(t, respItem["attributes"]) u, err = amClient.Update(tc.Context(), u.Item.Id, u.Item.Version, authmethods.DefaultName()) require.NoError(err) @@ -55,6 +102,7 @@ func TestCrud(t *testing.T) { _, err = amClient.Delete(tc.Context(), u.Item.Id) require.NoError(err) + _ = os.WriteFile(eventConfig.AuditEvents.Name(), nil, 0o666) // clean out audit events from previous calls // OIDC auth methods u, err = amClient.Create(tc.Context(), "oidc", global, authmethods.WithName("foo"), @@ -64,6 +112,15 @@ func TestCrud(t *testing.T) { authmethods.WithOidcAuthMethodClientId("client-id")) require.NoError(err) checkAuthMethod("create", u.Item, "foo", 1) + got = tests_api.CloudEventFromFile(t, eventConfig.AuditEvents.Name()) + + reqItem = tests_api.GetEventDetails(t, got, "request")["item"].(map[string]interface{}) + tests_api.AssertRedactedValues(t, reqItem) + tests_api.AssertRedactedValues(t, reqItem["attributes"], "client_secret") + + respItem = tests_api.GetEventDetails(t, got, "response")["item"].(map[string]interface{}) + tests_api.AssertRedactedValues(t, respItem) + tests_api.AssertRedactedValues(t, respItem["attributes"]) u, err = amClient.Read(tc.Context(), u.Item.Id) require.NoError(err) @@ -82,7 +139,23 @@ func TestCrud(t *testing.T) { } func TestCustomMethods(t *testing.T) { - tc := controller.NewTestController(t, nil) + // this cannot run in parallel because it relies on envvar + // globals.BOUNDARY_DEVELOPER_ENABLE_EVENTS + event.TestEnableEventing(t, true) + + assert, require := assert.New(t), require.New(t) + eventConfig := event.TestEventerConfig(t, "TestCrud", event.TestWithAuditSink(t)) + testLock := &sync.Mutex{} + testLogger := hclog.New(&hclog.LoggerOptions{ + Mutex: testLock, + Name: "test", + }) + require.NoError(event.InitSysEventer(testLogger, testLock, "TestCrud", event.WithEventerConfig(&eventConfig.EventerConfig))) + tcConfig, err := config.DevController() + require.NoError(err) + tcConfig.Eventing = &eventConfig.EventerConfig + + tc := controller.NewTestController(t, &controller.TestControllerOpts{Config: tcConfig}) defer tc.Shutdown() client := tc.Client() @@ -104,20 +177,30 @@ func TestCustomMethods(t *testing.T) { authmethods.WithOidcAuthMethodClientId("client-id"), authmethods.WithOidcAuthMethodSigningAlgorithms([]string{string("EdDSA")}), authmethods.WithOidcAuthMethodIdpCaCerts([]string{tp.CACert()})) - require.NoError(t, err) + require.NoError(err) const newState = "active-private" nilU, err := amClient.ChangeState(tc.Context(), u.Item.Id, u.Item.Version, newState) - require.Error(t, err) - assert.Nil(t, nilU) + require.Error(err) + assert.Nil(nilU) + _ = os.WriteFile(eventConfig.AuditEvents.Name(), nil, 0o666) // clean out audit events from previous calls u, err = amClient.ChangeState(tc.Context(), u.Item.Id, u.Item.Version, newState, authmethods.WithOidcAuthMethodDisableDiscoveredConfigValidation(true)) - require.NoError(t, err) - assert.NotNil(t, u) - assert.Equal(t, newState, u.Item.Attributes["state"]) + require.NoError(err) + assert.NotNil(u) + assert.Equal(newState, u.Item.Attributes["state"]) + got := tests_api.CloudEventFromFile(t, eventConfig.AuditEvents.Name()) + + reqDetails := tests_api.GetEventDetails(t, got, "request") + tests_api.AssertRedactedValues(t, reqDetails) + tests_api.AssertRedactedValues(t, reqDetails["attributes"]) + + respItem := tests_api.GetEventDetails(t, got, "response")["item"].(map[string]interface{}) + tests_api.AssertRedactedValues(t, respItem) + tests_api.AssertRedactedValues(t, respItem["attributes"]) _, err = amClient.ChangeState(tc.Context(), u.Item.Id, u.Item.Version, "", authmethods.WithOidcAuthMethodDisableDiscoveredConfigValidation(true)) - assert.Error(t, err) + assert.Error(err) } func TestErrors(t *testing.T) { diff --git a/internal/tests/api/testing.go b/internal/tests/api/testing.go new file mode 100644 index 000000000..0a561f471 --- /dev/null +++ b/internal/tests/api/testing.go @@ -0,0 +1,80 @@ +package api + +import ( + "encoding/json" + "io/ioutil" + "testing" + + "github.com/hashicorp/eventlogger/filters/encrypt" + "github.com/hashicorp/eventlogger/formatter_filters/cloudevents" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// CloudEventFromFile will marshal a single cloud event from the provided file +// name +func CloudEventFromFile(t *testing.T, fileName string) *cloudevents.Event { + t.Helper() + b, err := ioutil.ReadFile(fileName) + assert.NoError(t, err) + got := &cloudevents.Event{} + err = json.Unmarshal(b, got) + require.NoErrorf(t, err, "json: %s", string(b)) + return got +} + +// GetEventDetails is a testing helper will return the details from the event +// payload for a given messageType (request or response) +func GetEventDetails(t *testing.T, e *cloudevents.Event, messageType string) map[string]interface{} { + t.Helper() + require := require.New(t) + require.NotNil(e) + require.NotEmpty(messageType) + data, ok := e.Data.(map[string]interface{}) + if !ok { + return nil + } + msgType, ok := data[messageType].(map[string]interface{}) + if !ok { + return nil + } + + details, ok := msgType["details"].(map[string]interface{}) + if !ok { + return nil + } + return details +} + +// AssertRedactedValues will assert that the values for the given keys within +// the data have been redacted +func AssertRedactedValues(t *testing.T, data interface{}, keys ...string) { + t.Helper() + assert, require := assert.New(t), require.New(t) + require.NotNil(data) + dataMap, ok := data.(map[string]interface{}) + require.Truef(ok, "data must be a map[string]interface{}") + + rMap := make(map[string]bool, len(keys)) + for _, s := range keys { + rMap[s] = true + } + for k, v := range dataMap { + switch typ := v.(type) { + case []interface{}: + for _, s := range typ { + if _, ok := rMap[k]; ok { + assert.Equalf(encrypt.RedactedData, s, "expected %s to be redacted and it was set to: %s", k, v) + } else { + assert.NotEqualf(encrypt.RedactedData, s, "did not expect %s to be redacted", k) + } + } + default: + if _, ok := rMap[k]; ok { + assert.Equalf(encrypt.RedactedData, v, "expected %s to be redacted and it was set to: %s", k, v) + } else { + assert.NotEqualf(encrypt.RedactedData, v, "did not expect %s to be redacted", k) + } + } + } +} diff --git a/sdk/go.mod b/sdk/go.mod index 30ef008c5..e4ec45cf9 100644 --- a/sdk/go.mod +++ b/sdk/go.mod @@ -7,7 +7,7 @@ require ( github.com/golang/protobuf v1.5.2 // indirect github.com/google/go-cmp v0.5.6 // indirect github.com/hashicorp/eventlogger v0.1.0 - github.com/hashicorp/eventlogger/filters/encrypt v0.1.3 + github.com/hashicorp/eventlogger/filters/encrypt v0.1.5 github.com/hashicorp/go-cleanhttp v0.5.2 // indirect github.com/hashicorp/go-kms-wrapping v0.6.6 github.com/hashicorp/go-retryablehttp v0.7.0 // indirect diff --git a/sdk/go.sum b/sdk/go.sum index 08db5d8e0..ce2eba262 100644 --- a/sdk/go.sum +++ b/sdk/go.sum @@ -237,8 +237,8 @@ github.com/hashicorp/errwrap v1.1.0 h1:OxrOeh75EUXMY8TBjag2fzXGZ40LB6IKw45YeGUDY github.com/hashicorp/errwrap v1.1.0/go.mod h1:YH+1FKiLXxHSkmPseP+kNlulaMuP3n2brvKWEqk/Jc4= github.com/hashicorp/eventlogger v0.1.0 h1:S6xc4gZVzewuDUP4R4Ngko419h/CGDuV/b4ADL3XLik= github.com/hashicorp/eventlogger v0.1.0/go.mod h1:a3IXf1aEJfpCPzseTOrwKj4fVW/Qn3oEmpQeaIznzH0= -github.com/hashicorp/eventlogger/filters/encrypt v0.1.3 h1:RKYUGplnpDoE/Cj2vupQqUJS/wWBVvmpxEYeEpGL88s= -github.com/hashicorp/eventlogger/filters/encrypt v0.1.3/go.mod h1:8rcez7Kw1zanB0/074qnOuGu7zxmNh9Xr2ZI+K4xVIA= +github.com/hashicorp/eventlogger/filters/encrypt v0.1.5 h1:kNkH4G6zzWlZSoI1I+B/ud4chVKTPL516C6jB7dRdlE= +github.com/hashicorp/eventlogger/filters/encrypt v0.1.5/go.mod h1:8rcez7Kw1zanB0/074qnOuGu7zxmNh9Xr2ZI+K4xVIA= github.com/hashicorp/go-cleanhttp v0.5.0/go.mod h1:JpRdi6/HCYpAwUzNwuwqhbovhLtngrth3wmdIIUrZ80= github.com/hashicorp/go-cleanhttp v0.5.1/go.mod h1:JpRdi6/HCYpAwUzNwuwqhbovhLtngrth3wmdIIUrZ80= github.com/hashicorp/go-cleanhttp v0.5.2 h1:035FKYIWjmULyFRBKPs8TBQoi0x6d9G4xc9neXJWAZQ= diff --git a/sdk/pbs/controller/api/resources/authmethods/auth_method.pb.go b/sdk/pbs/controller/api/resources/authmethods/auth_method.pb.go index 24176d980..ea7612a1a 100644 --- a/sdk/pbs/controller/api/resources/authmethods/auth_method.pb.go +++ b/sdk/pbs/controller/api/resources/authmethods/auth_method.pb.go @@ -452,9 +452,9 @@ type OidcAuthMethodAuthenticateStartResponse struct { unknownFields protoimpl.UnknownFields // The returned authentication URL - AuthUrl string `protobuf:"bytes,10,opt,name=auth_url,proto3" json:"auth_url,omitempty"` + AuthUrl string `protobuf:"bytes,10,opt,name=auth_url,proto3" json:"auth_url,omitempty" class:"public"` // @gotags: `class:"public"` // The returned token ID - TokenId string `protobuf:"bytes,30,opt,name=token_id,proto3" json:"token_id,omitempty"` + TokenId string `protobuf:"bytes,30,opt,name=token_id,proto3" json:"token_id,omitempty" class:"public"` // @gotags: `class:"public"` } func (x *OidcAuthMethodAuthenticateStartResponse) Reset() { @@ -510,13 +510,13 @@ type OidcAuthMethodAuthenticateCallbackRequest struct { unknownFields protoimpl.UnknownFields // The returned code - Code string `protobuf:"bytes,10,opt,name=code,proto3" json:"code,omitempty"` + Code string `protobuf:"bytes,10,opt,name=code,proto3" json:"code,omitempty" class:"secret"` // @gotags: `class:"secret"` // The returned state - State string `protobuf:"bytes,20,opt,name=state,proto3" json:"state,omitempty"` + State string `protobuf:"bytes,20,opt,name=state,proto3" json:"state,omitempty" class:"public"` // @gotags: `class:"public"` // Error parameters, if they are returned - Error string `protobuf:"bytes,30,opt,name=error,proto3" json:"error,omitempty"` - ErrorDescription string `protobuf:"bytes,40,opt,name=error_description,proto3" json:"error_description,omitempty"` - ErrorUri string `protobuf:"bytes,50,opt,name=error_uri,proto3" json:"error_uri,omitempty"` + Error string `protobuf:"bytes,30,opt,name=error,proto3" json:"error,omitempty" class:"public"` // @gotags: `class:"public"` + ErrorDescription string `protobuf:"bytes,40,opt,name=error_description,proto3" json:"error_description,omitempty" class:"public"` // @gotags: `class:"public"` + ErrorUri string `protobuf:"bytes,50,opt,name=error_uri,proto3" json:"error_uri,omitempty" class:"public"` // @gotags: `class:"public"` } func (x *OidcAuthMethodAuthenticateCallbackRequest) Reset() { @@ -593,7 +593,7 @@ type OidcAuthMethodAuthenticateCallbackResponse struct { unknownFields protoimpl.UnknownFields // The final redirection URL - FinalRedirectUrl string `protobuf:"bytes,10,opt,name=final_redirect_url,proto3" json:"final_redirect_url,omitempty"` + FinalRedirectUrl string `protobuf:"bytes,10,opt,name=final_redirect_url,proto3" json:"final_redirect_url,omitempty" class:"public"` // @gotags: `class:"public"` } func (x *OidcAuthMethodAuthenticateCallbackResponse) Reset() { @@ -642,7 +642,7 @@ type OidcAuthMethodAuthenticateTokenRequest struct { unknownFields protoimpl.UnknownFields // The ID of the pending token - TokenId string `protobuf:"bytes,10,opt,name=token_id,proto3" json:"token_id,omitempty"` + TokenId string `protobuf:"bytes,10,opt,name=token_id,proto3" json:"token_id,omitempty" class:"private"` // @gotags: `class:"private"` } func (x *OidcAuthMethodAuthenticateTokenRequest) Reset() { @@ -693,7 +693,7 @@ type OidcAuthMethodAuthenticateTokenResponse struct { // The status. This will always be "unknown". It will never be forwarded to // the consumer. - Status string `protobuf:"bytes,10,opt,name=status,proto3" json:"status,omitempty"` + Status string `protobuf:"bytes,10,opt,name=status,proto3" json:"status,omitempty" class:"public"` // @gotags: `class:"public"` } func (x *OidcAuthMethodAuthenticateTokenResponse) Reset() { diff --git a/sdk/pbs/controller/api/resources/authmethods/auth_method_taggable.go b/sdk/pbs/controller/api/resources/authmethods/auth_method_taggable.go index 26c729eac..97ec05b15 100644 --- a/sdk/pbs/controller/api/resources/authmethods/auth_method_taggable.go +++ b/sdk/pbs/controller/api/resources/authmethods/auth_method_taggable.go @@ -45,6 +45,10 @@ func (req *AuthMethod) Tags() ([]encrypt.PointerTag, error) { Pointer: "/Attributes/Fields/signing_algorithms", Classification: encrypt.PublicClassification, }, + { + Pointer: "/Attributes/Fields/idp_ca_certs", + Classification: encrypt.PublicClassification, + }, { Pointer: "/Attributes/Fields/api_url_prefix", Classification: encrypt.PublicClassification, diff --git a/sdk/pbs/controller/api/resources/authmethods/auth_method_taggable_test.go b/sdk/pbs/controller/api/resources/authmethods/auth_method_taggable_test.go index 865deab88..a4018792a 100644 --- a/sdk/pbs/controller/api/resources/authmethods/auth_method_taggable_test.go +++ b/sdk/pbs/controller/api/resources/authmethods/auth_method_taggable_test.go @@ -6,9 +6,9 @@ import ( "testing" "time" + "github.com/hashicorp/boundary/sdk/pbs/controller/api" "github.com/hashicorp/boundary/sdk/wrapper" "github.com/hashicorp/eventlogger" - "github.com/hashicorp/eventlogger/filters/encrypt" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "google.golang.org/protobuf/types/known/structpb" @@ -19,11 +19,7 @@ func TestAuthMethod_Tags(t *testing.T) { ctx := context.Background() now := time.Now() wrapper := wrapper.TestWrapper(t) - testEncryptingFilter := &encrypt.Filter{ - Wrapper: wrapper, - HmacSalt: []byte("salt"), - HmacInfo: []byte("info"), - } + testEncryptingFilter := api.NewEncryptFilter(t, wrapper) tests := []struct { name string @@ -49,6 +45,7 @@ func TestAuthMethod_Tags(t *testing.T) { "client_secret_hmac": structpb.NewStringValue("public-client_secret_hmac"), "max_age": structpb.NewStringValue("public-max_age"), "signing_algorithms": structpb.NewStringValue("public-signing_algorithms"), + "idp_ca_certs": structpb.NewStringValue("public-signing_algorithms"), "api_url_prefix": structpb.NewStringValue("public-api_url_prefix"), "callback_url": structpb.NewStringValue("public-callback_url"), "allowed_audiences": structpb.NewStringValue("public-allowed_audiences"), @@ -87,6 +84,7 @@ func TestAuthMethod_Tags(t *testing.T) { "client_secret_hmac": structpb.NewStringValue("public-client_secret_hmac"), "max_age": structpb.NewStringValue("public-max_age"), "signing_algorithms": structpb.NewStringValue("public-signing_algorithms"), + "idp_ca_certs": structpb.NewStringValue("public-signing_algorithms"), "api_url_prefix": structpb.NewStringValue("public-api_url_prefix"), "callback_url": structpb.NewStringValue("public-callback_url"), "allowed_audiences": structpb.NewStringValue("public-allowed_audiences"), @@ -119,7 +117,7 @@ func TestAuthMethod_Tags(t *testing.T) { ScopeId: "scope-id", Name: &wrapperspb.StringValue{Value: "name"}, Description: &wrapperspb.StringValue{Value: "description"}, - Type: "oidc", + Type: "password", Attributes: &structpb.Struct{ Fields: map[string]*structpb.Value{ "min_login_name_length": structpb.NewStringValue("public-min_login_name_length"), @@ -145,7 +143,7 @@ func TestAuthMethod_Tags(t *testing.T) { ScopeId: "scope-id", Name: &wrapperspb.StringValue{Value: "name"}, Description: &wrapperspb.StringValue{Value: "description"}, - Type: "oidc", + Type: "password", Attributes: &structpb.Struct{ Fields: map[string]*structpb.Value{ "min_login_name_length": structpb.NewStringValue("public-min_login_name_length"), diff --git a/sdk/pbs/controller/api/testing.go b/sdk/pbs/controller/api/testing.go new file mode 100644 index 000000000..0e771e924 --- /dev/null +++ b/sdk/pbs/controller/api/testing.go @@ -0,0 +1,23 @@ +package api + +import ( + "reflect" + "testing" + + "github.com/hashicorp/eventlogger/filters/encrypt" + wrapping "github.com/hashicorp/go-kms-wrapping" + "google.golang.org/protobuf/types/known/fieldmaskpb" +) + +// NewEncryptFilter is a copy of event.NewEncryptFilter since importing it would +// case circular deps. The primary reason for this test func is to make sure +// the proper IgnoreTypes are included for testing. +func NewEncryptFilter(t *testing.T, w wrapping.Wrapper) *encrypt.Filter { + t.Helper() + return &encrypt.Filter{ + Wrapper: w, + IgnoreTypes: []reflect.Type{ + reflect.TypeOf(&fieldmaskpb.FieldMask{}), + }, + } +} From 1782f6a4e47dd271f01d55abd5b053c9f7a2b1c5 Mon Sep 17 00:00:00 2001 From: Jeff Mitchell Date: Wed, 27 Oct 2021 20:32:07 -0400 Subject: [PATCH 7/7] 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