From 9b39345c7f1676871a6dde9c70e775ebd3f19094 Mon Sep 17 00:00:00 2001 From: Louis Ruch Date: Wed, 15 Dec 2021 15:52:28 -0800 Subject: [PATCH] bug(credential): Fix panic in credential Issue Vault KV backend returns a nil secret and no error if the secret does not exist. This caused a panic in during Issue: panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x20 pc=0x1388279] goroutine 2639 [running]: github.com/hashicorp/boundary/internal/credential/vault.(*Repository).Issue(0xc001222200, {0x3b932d0, 0xc001203350}, {0xc000dc6e90, 0xc}, {0xc0010e5e40, 0x0, 0x0}) /home/louis/boundary/internal/credential/vault/repository_credentials.go:67 +0x2d9 github.com/hashicorp/boundary/internal/servers/controller/handlers/targets.Service.AuthorizeSession({{}, 0xc000cd4a38, 0xc000cd4840, 0xc000cd48d0, 0xc000cd4a50, 0xc000cd4870, 0xc000cd4858, 0xc000cd48b8, 0xc0001a8880}, {0x3b932d0, ...}, ...) /home/louis/boundary/internal/servers/controller/handlers/targets/target_service.go:1059 +0x1525 github.com/hashicorp/boundary/internal/gen/controller/api/services._TargetService_AuthorizeSession_Handler.func1({0x3b932d0, 0xc001203350}, {0x1a0dd40, 0xc000dc8880}) /home/louis/boundary/internal/gen/controller/api/services/target_service_grpc.pb.go:631 +0x78 github.com/hashicorp/boundary/internal/servers/controller.auditResponseInterceptor.func1({0x3b932d0, 0xc001203350}, {0x1a0dd40, 0xc000dc8880}, 0x461279, 0x40d234) /home/louis/boundary/internal/servers/controller/interceptor.go:262 +0x4e github.com/grpc-ecosystem/go-grpc-middleware.ChainUnaryServer.func1.1.1({0x3b932d0, 0xc001203350}, {0x1a0dd40, 0xc000dc8880}) /home/louis/go/pkg/mod/github.com/grpc-ecosystem/go-grpc-middleware@v1.3.0/chain.go:25 +0x3a github.com/hashicorp/boundary/internal/servers/controller.statusCodeInterceptor.func1({0x3b932d0, 0xc001203350}, {0x1a0dd40, 0xc000dc8880}, 0x203000, 0x203000) /home/louis/boundary/internal/servers/controller/interceptor.go:207 +0x34 github.com/grpc-ecosystem/go-grpc-middleware.ChainUnaryServer.func1.1.1({0x3b932d0, 0xc001203350}, {0x1a0dd40, 0xc000dc8880}) /home/louis/go/pkg/mod/github.com/grpc-ecosystem/go-grpc-middleware@v1.3.0/chain.go:25 +0x3a github.com/hashicorp/boundary/internal/servers/controller.errorInterceptor.func1({0x3b932d0, 0xc001203350}, {0x1a0dd40, 0xc000dc8880}, 0x18e2b40, 0x0) /home/louis/boundary/internal/servers/controller/interceptor.go:156 +0x4e github.com/grpc-ecosystem/go-grpc-middleware.ChainUnaryServer.func1.1.1({0x3b932d0, 0xc001203350}, {0x1a0dd40, 0xc000dc8880}) /home/louis/go/pkg/mod/github.com/grpc-ecosystem/go-grpc-middleware@v1.3.0/chain.go:25 +0x3a github.com/hashicorp/boundary/internal/servers/controller.auditRequestInterceptor.func1({0x3b932d0, 0xc001203350}, {0x1a0dd40, 0xc000dc8880}, 0x1a1c860, 0xc0010e5280) /home/louis/boundary/internal/servers/controller/interceptor.go:248 +0x198 github.com/grpc-ecosystem/go-grpc-middleware.ChainUnaryServer.func1.1.1({0x3b932d0, 0xc001203350}, {0x1a0dd40, 0xc000dc8880}) /home/louis/go/pkg/mod/github.com/grpc-ecosystem/go-grpc-middleware@v1.3.0/chain.go:25 +0x3a github.com/hashicorp/boundary/internal/servers/controller.requestCtxInterceptor.func1({0x3b932d0, 0xc001203260}, {0x1a0dd40, 0xc000dc8880}, 0x18, 0xc0010e52a0) /home/louis/boundary/internal/servers/controller/interceptor.go:140 +0x5de github.com/grpc-ecosystem/go-grpc-middleware.ChainUnaryServer.func1.1.1({0x3b932d0, 0xc001203260}, {0x1a0dd40, 0xc000dc8880}) /home/louis/go/pkg/mod/github.com/grpc-ecosystem/go-grpc-middleware@v1.3.0/chain.go:25 +0x3a github.com/grpc-ecosystem/go-grpc-middleware.ChainUnaryServer.func1({0x3b932d0, 0xc001203260}, {0x1a0dd40, 0xc000dc8880}, 0xc00331abb8, 0x18675e0) /home/louis/go/pkg/mod/github.com/grpc-ecosystem/go-grpc-middleware@v1.3.0/chain.go:34 +0xbf github.com/hashicorp/boundary/internal/gen/controller/api/services._TargetService_AuthorizeSession_Handler({0x1ada420, 0xc00093a180}, {0x3b932d0, 0xc001203260}, 0xc001200840, 0xc000465260) /home/louis/boundary/internal/gen/controller/api/services/target_service_grpc.pb.go:633 +0x138 google.golang.org/grpc.(*Server).processUnaryRPC(0xc000cca540, {0x3bb4c58, 0xc0005a9c80}, 0xc001208480, 0xc0004a7440, 0x499e2b8, 0x0) /home/louis/go/pkg/mod/google.golang.org/grpc@v1.40.0/server.go:1297 +0xccf google.golang.org/grpc.(*Server).handleStream(0xc000cca540, {0x3bb4c58, 0xc0005a9c80}, 0xc001208480, 0x0) /home/louis/go/pkg/mod/google.golang.org/grpc@v1.40.0/server.go:1626 +0xa2a google.golang.org/grpc.(*Server).serveStreams.func1.2() /home/louis/go/pkg/mod/google.golang.org/grpc@v1.40.0/server.go:941 +0x98 created by google.golang.org/grpc.(*Server).serveStreams.func1 /home/louis/go/pkg/mod/google.golang.org/grpc@v1.40.0/server.go:939 +0x294 --- .../vault/repository_credentials.go | 3 + .../vault/repository_credentials_test.go | 49 +++++++++++++- internal/credential/vault/testing.go | 33 +++++++++ internal/credential/vault/testing_test.go | 67 +++++++++++++++++++ internal/errors/code.go | 1 + internal/errors/code_test.go | 5 ++ internal/errors/info.go | 4 ++ 7 files changed, 161 insertions(+), 1 deletion(-) diff --git a/internal/credential/vault/repository_credentials.go b/internal/credential/vault/repository_credentials.go index c06cc385f4..af19a2e72a 100644 --- a/internal/credential/vault/repository_credentials.go +++ b/internal/credential/vault/repository_credentials.go @@ -63,6 +63,9 @@ func (r *Repository) Issue(ctx context.Context, sessionId string, requests []cre // expired or invalid token return nil, errors.Wrap(ctx, err, op) } + if secret == nil { + return nil, errors.E(ctx, errors.WithCode(errors.VaultEmptySecret), errors.WithOp(op)) + } leaseDuration := time.Duration(secret.LeaseDuration) * time.Second if minLease > leaseDuration { diff --git a/internal/credential/vault/repository_credentials_test.go b/internal/credential/vault/repository_credentials_test.go index 14038f00fa..3ddc2b1db8 100644 --- a/internal/credential/vault/repository_credentials_test.go +++ b/internal/credential/vault/repository_credentials_test.go @@ -28,6 +28,7 @@ func TestRepository_IssueCredentials(t *testing.T) { v := vault.NewTestVaultServer(t, vault.WithDockerNetwork(true), vault.WithTestVaultTLS(vault.TestClientTLS)) v.MountDatabase(t) v.MountPKI(t) + v.AddKVPolicy(t) conn, _ := db.TestSetup(t, "postgres") rw := db.New(conn) @@ -44,7 +45,10 @@ func TestRepository_IssueCredentials(t *testing.T) { err = vault.RegisterJobs(ctx, sche, rw, rw, kms) require.NoError(err) - _, token := v.CreateToken(t, vault.WithPolicies([]string{"default", "boundary-controller", "database", "pki"})) + _, token := v.CreateToken(t, vault.WithPolicies([]string{"default", "boundary-controller", "database", "pki", "secret"})) + + // Create valid KV secret + v.CreateKVSecret(t, "my-secret", []byte(`{"data":{"username":"user","password":"pass"}}`)) var opts []vault.Option opts = append(opts, vault.WithCACert(v.CaCert)) @@ -64,6 +68,8 @@ func TestRepository_IssueCredentials(t *testing.T) { libDB libT = iota libPKI libErrPKI + libKV + libErrKV ) libs := make(map[libT]string) @@ -98,6 +104,26 @@ func TestRepository_IssueCredentials(t *testing.T) { require.NotNil(lib) libs[libErrPKI] = lib.GetPublicId() } + { + libPath := path.Join("secret", "data", "my-secret") + libIn, err := vault.NewCredentialLibrary(origStore.GetPublicId(), libPath, opts...) + assert.NoError(err) + require.NotNil(libIn) + lib, err := repo.CreateCredentialLibrary(ctx, prj.GetPublicId(), libIn) + assert.NoError(err) + require.NotNil(lib) + libs[libKV] = lib.GetPublicId() + } + { + libPath := path.Join("secret", "data", "fake-secret") + libIn, err := vault.NewCredentialLibrary(origStore.GetPublicId(), libPath, opts...) + assert.NoError(err) + require.NotNil(libIn) + lib, err := repo.CreateCredentialLibrary(ctx, prj.GetPublicId(), libIn) + assert.NoError(err) + require.NotNil(lib) + libs[libErrKV] = lib.GetPublicId() + } at := authtoken.TestAuthToken(t, conn, kms, org.GetPublicId()) uId := at.GetIamUserId() @@ -178,6 +204,27 @@ func TestRepository_IssueCredentials(t *testing.T) { }, wantErr: errors.InvalidDynamicCredential, }, + { + name: "valid-kv-secret", + convertFn: rc2dc, + requests: []credential.Request{ + { + SourceId: libs[libKV], + Purpose: credential.IngressPurpose, + }, + }, + }, + { + name: "invalid-kv-does-not-exist", + convertFn: rc2dc, + requests: []credential.Request{ + { + SourceId: libs[libErrKV], + Purpose: credential.ApplicationPurpose, + }, + }, + wantErr: errors.VaultEmptySecret, + }, } for _, tt := range tests { tt := tt diff --git a/internal/credential/vault/testing.go b/internal/credential/vault/testing.go index 53042d6268..637757caea 100644 --- a/internal/credential/vault/testing.go +++ b/internal/credential/vault/testing.go @@ -789,6 +789,39 @@ func (v *TestVaultServer) MountPKI(t *testing.T, opt ...TestOption) *vault.Secre return s } +// AddKVPolicy adds a Vault policy named 'secret' to v and adds it to the +// standard set of polices attached to tokens created with v.CreateToken. +// The policy is defined as: +// +// path "secret/*" { +// capabilities = ["create", "read", "update", "delete", "list"] +// } +// +// All options are ignored. +func (v *TestVaultServer) AddKVPolicy(t *testing.T, _ ...TestOption) { + t.Helper() + + pc := pathCapabilities{ + "secret/data/*": createCapability | readCapability | updateCapability | deleteCapability | listCapability, + } + v.addPolicy(t, "secret", pc) +} + +// CreateKVSecret calls the /secret/data/:p endpoint with the provided +// data. Please note for KV-v2 the provided data needs to be in JSON format similar to: +// `{"data": {"key": "value", "key2": "value2"}}` +// See +// https://www.vaultproject.io/api-docs/secret/kv/kv-v2#create-update-secret +func (v *TestVaultServer) CreateKVSecret(t *testing.T, p string, data []byte) *vault.Secret { + t.Helper() + require := require.New(t) + + vc := v.client(t) + cred, err := vc.post(path.Join("secret", "data", p), data) + require.NoError(err) + return cred +} + // TestVaultServer is a vault server running in a docker container suitable // for testing. type TestVaultServer struct { diff --git a/internal/credential/vault/testing_test.go b/internal/credential/vault/testing_test.go index 637d537f0e..ed244434cb 100644 --- a/internal/credential/vault/testing_test.go +++ b/internal/credential/vault/testing_test.go @@ -7,11 +7,13 @@ import ( "crypto/x509" "encoding/json" "encoding/pem" + "net/http" "path" "testing" "time" "github.com/hashicorp/boundary/internal/db" + "github.com/hashicorp/boundary/internal/errors" "github.com/hashicorp/boundary/internal/iam" "github.com/hashicorp/go-secure-stdlib/parseutil" vault "github.com/hashicorp/vault/api" @@ -540,3 +542,68 @@ func Test_testClientCert(t *testing.T) { assert.NotEqual(cert1.Cert.Cert, cert2.Cert.Cert) assert.Equal(cert1.Cert.Key, cert2.Cert.Key) } + +func TestTestVaultServer_AddKVPolicy(t *testing.T) { + t.Run("defaults", func(t *testing.T) { + t.Parallel() + assert, require := assert.New(t), require.New(t) + v := NewTestVaultServer(t) + + sec := v.CreateKVSecret(t, "my-secret", []byte(`{"data" : {"foo":"bar"}}`)) + require.NotNil(sec) + + _, token := v.CreateToken(t, WithPolicies([]string{"default", "secret"})) + require.NotNil(token) + client := v.clientUsingToken(t, token) + + _, err := client.get("/secret/data/my-secret") + assert.Error(err) + + // An attempt to get my-secret should now fail with a 403 + var respErr *vault.ResponseError + ok := errors.As(err, &respErr) + require.True(ok) + assert.Equal(http.StatusForbidden, respErr.StatusCode) + + // Add KV policy and get should work + v.AddKVPolicy(t) + _, token = v.CreateToken(t, WithPolicies([]string{"default", "secret"})) + require.NotNil(token) + client = v.clientUsingToken(t, token) + + _, err = client.get("/secret/data/my-secret") + assert.NoError(err) + }) +} + +func TestTestVaultServer_CreateKVSecret(t *testing.T) { + t.Run("defaults", func(t *testing.T) { + t.Parallel() + assert, require := assert.New(t), require.New(t) + v := NewTestVaultServer(t) + vc := v.client(t) + + // Attempt to read my-secret should return a nil secret + got, err := vc.cl.Logical().Read("/secret/data/my-secret") + require.NoError(err) + require.Nil(got) + + secret := v.CreateKVSecret(t, "my-secret", []byte(`{"data" : {"foo":"bar"}}`)) + require.NotNil(secret) + + // Now that secret exists try read again + got, err = vc.cl.Logical().Read("/secret/data/my-secret") + require.NoError(err) + require.NotNil(got) + require.NotNil(got.Data) + require.NotNil(got.Data["data"]) + + gotData, ok := got.Data["data"].(map[string]interface{}) + require.True(ok) + require.NotNil(gotData["foo"]) + + gotFoo, ok := gotData["foo"].(string) + require.True(ok) + assert.Equal("bar", gotFoo) + }) +} diff --git a/internal/errors/code.go b/internal/errors/code.go index 1c4a75846b..a138528a57 100644 --- a/internal/errors/code.go +++ b/internal/errors/code.go @@ -107,6 +107,7 @@ const ( VaultTokenNotRenewable Code = 3012 // VaultTokenNotRenewable represents an error for a Vault token that is not renewable VaultTokenMissingCapabilities Code = 3013 // VaultTokenMissingCapabilities represents an error for a Vault token that is missing capabilities VaultCredentialRequest Code = 3014 // VaultCredentialRequest represents an error returned from Vault when retrieving a credential + VaultEmptySecret Code = 3015 // VaultEmptySecret represents a empty secret was returned from Vault without error // OIDC authentication provided errors OidcProviderCallbackError Code = 4000 // OidcProviderCallbackError represents an error that is passed by the OIDC provider to the callback endpoint diff --git a/internal/errors/code_test.go b/internal/errors/code_test.go index 78326f4de3..129a4970fa 100644 --- a/internal/errors/code_test.go +++ b/internal/errors/code_test.go @@ -282,6 +282,11 @@ func TestCode_Both_String_Info(t *testing.T) { c: VaultCredentialRequest, want: VaultCredentialRequest, }, + { + name: "VaultEmptySecret", + c: VaultEmptySecret, + want: VaultEmptySecret, + }, { name: "OidcProviderCallbackError", c: OidcProviderCallbackError, diff --git a/internal/errors/info.go b/internal/errors/info.go index 40004249d5..e1f3177553 100644 --- a/internal/errors/info.go +++ b/internal/errors/info.go @@ -228,6 +228,10 @@ var errorCodeInfo = map[Code]Info{ Message: "request for a new credential from vault failed", Kind: External, }, + VaultEmptySecret: { + Message: "vault secret is empty", + Kind: Integrity, + }, OidcProviderCallbackError: { Message: "oidc provider callback error", Kind: External,