diff --git a/builtin/logical/database/backend_test.go b/builtin/logical/database/backend_test.go index 447ab7d66..9a0846082 100644 --- a/builtin/logical/database/backend_test.go +++ b/builtin/logical/database/backend_test.go @@ -3,7 +3,9 @@ package database import ( "context" "database/sql" + "fmt" "log" + "net/url" "os" "reflect" "strings" @@ -12,6 +14,9 @@ import ( "github.com/go-test/deep" mongodbatlas "github.com/hashicorp/vault-plugin-database-mongodbatlas" + "github.com/lib/pq" + "github.com/mitchellh/mapstructure" + "github.com/hashicorp/vault/helper/namespace" postgreshelper "github.com/hashicorp/vault/helper/testhelpers/postgresql" vaulthttp "github.com/hashicorp/vault/http" @@ -25,8 +30,6 @@ import ( "github.com/hashicorp/vault/sdk/helper/pluginutil" "github.com/hashicorp/vault/sdk/logical" "github.com/hashicorp/vault/vault" - "github.com/lib/pq" - "github.com/mitchellh/mapstructure" ) func getCluster(t *testing.T) (*vault.TestCluster, logical.SystemView) { @@ -1349,6 +1352,116 @@ func TestBackend_RotateRootCredentials(t *testing.T) { } } +func TestBackend_ConnectionURL_redacted(t *testing.T) { + cluster, sys := getCluster(t) + t.Cleanup(cluster.Cleanup) + + config := logical.TestBackendConfig() + config.StorageView = &logical.InmemStorage{} + config.System = sys + + b, err := Factory(context.Background(), config) + if err != nil { + t.Fatal(err) + } + defer b.Cleanup(context.Background()) + + tests := []struct { + name string + password string + }{ + { + name: "basic", + password: "secret", + }, + { + name: "encoded", + password: "yourStrong(!)Password", + }, + } + + respCheck := func(req *logical.Request) *logical.Response { + t.Helper() + resp, err := b.HandleRequest(namespace.RootContext(nil), req) + if err != nil { + t.Fatalf("err: %v", err) + } + if resp == nil { + t.Fatalf("expected a response, resp: %#v", resp) + } + + if resp.Error() != nil { + t.Fatalf("unexpected error in response, err: %#v", resp.Error()) + } + + return resp + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cleanup, u := postgreshelper.PrepareTestContainerWithPassword(t, "13.4-buster", tt.password) + t.Cleanup(cleanup) + + p, err := url.Parse(u) + if err != nil { + t.Fatal(err) + } + + actualPassword, _ := p.User.Password() + if tt.password != actualPassword { + t.Fatalf("expected computed URL password %#v, actual %#v", tt.password, actualPassword) + } + + // Configure a connection + data := map[string]interface{}{ + "connection_url": u, + "plugin_name": "postgresql-database-plugin", + "allowed_roles": []string{"plugin-role-test"}, + } + req := &logical.Request{ + Operation: logical.UpdateOperation, + Path: fmt.Sprintf("config/%s", tt.name), + Storage: config.StorageView, + Data: data, + } + respCheck(req) + + // read config + readReq := &logical.Request{ + Operation: logical.ReadOperation, + Path: req.Path, + Storage: config.StorageView, + } + resp := respCheck(readReq) + + var connDetails map[string]interface{} + if v, ok := resp.Data["connection_details"]; ok { + connDetails = v.(map[string]interface{}) + } + + if connDetails == nil { + t.Fatalf("response data missing connection_details, resp: %#v", resp) + } + + actual := connDetails["connection_url"].(string) + expected := p.Redacted() + if expected != actual { + t.Fatalf("expected redacted URL %q, actual %q", expected, actual) + } + + if tt.password != "" { + // extra test to ensure that URL.Redacted() is working as expected. + p, err = url.Parse(actual) + if err != nil { + t.Fatal(err) + } + if pp, _ := p.User.Password(); pp == tt.password { + t.Fatalf("password was not redacted by URL.Redacted()") + } + } + }) + } +} + func testCredsExist(t *testing.T, resp *logical.Response, connURL string) bool { t.Helper() var d struct { diff --git a/builtin/logical/database/path_config_connection.go b/builtin/logical/database/path_config_connection.go index 554d83fe3..2c2f7b4b0 100644 --- a/builtin/logical/database/path_config_connection.go +++ b/builtin/logical/database/path_config_connection.go @@ -5,10 +5,10 @@ import ( "errors" "fmt" "net/url" - "strings" "github.com/fatih/structs" - uuid "github.com/hashicorp/go-uuid" + "github.com/hashicorp/go-uuid" + v5 "github.com/hashicorp/vault/sdk/database/dbplugin/v5" "github.com/hashicorp/vault/sdk/framework" "github.com/hashicorp/vault/sdk/logical" @@ -195,13 +195,10 @@ func (b *databaseBackend) connectionReadHandler() framework.OperationFunc { return nil, err } - // Mask the password if it is in the url + // Ensure that we only ever include a redacted valid URL in the response. if connURLRaw, ok := config.ConnectionDetails["connection_url"]; ok { - connURL := connURLRaw.(string) - if conn, err := url.Parse(connURL); err == nil { - if password, ok := conn.User.Password(); ok { - config.ConnectionDetails["connection_url"] = strings.Replace(connURL, password, "*****", -1) - } + if p, err := url.Parse(connURLRaw.(string)); err == nil { + config.ConnectionDetails["connection_url"] = p.Redacted() } } diff --git a/changelog/14744.txt b/changelog/14744.txt new file mode 100644 index 000000000..e5226f991 --- /dev/null +++ b/changelog/14744.txt @@ -0,0 +1,3 @@ +```release-note:bug +secrets/database: Ensure that a `connection_url` password is redacted in all cases. +``` diff --git a/helper/testhelpers/postgresql/postgresqlhelper.go b/helper/testhelpers/postgresql/postgresqlhelper.go index d15249a85..f22f207a4 100644 --- a/helper/testhelpers/postgresql/postgresqlhelper.go +++ b/helper/testhelpers/postgresql/postgresqlhelper.go @@ -12,6 +12,14 @@ import ( ) func PrepareTestContainer(t *testing.T, version string) (func(), string) { + return prepareTestContainer(t, version, "secret", "database") +} + +func PrepareTestContainerWithPassword(t *testing.T, version, password string) (func(), string) { + return prepareTestContainer(t, version, password, "database") +} + +func prepareTestContainer(t *testing.T, version, password, db string) (func(), string) { if os.Getenv("PG_URL") != "" { return func() {}, os.Getenv("PG_URL") } @@ -19,17 +27,21 @@ func PrepareTestContainer(t *testing.T, version string) (func(), string) { if version == "" { version = "11" } + runner, err := docker.NewServiceRunner(docker.RunOptions{ ImageRepo: "postgres", ImageTag: version, - Env: []string{"POSTGRES_PASSWORD=secret", "POSTGRES_DB=database"}, - Ports: []string{"5432/tcp"}, + Env: []string{ + "POSTGRES_PASSWORD=" + password, + "POSTGRES_DB=" + db, + }, + Ports: []string{"5432/tcp"}, }) if err != nil { t.Fatalf("Could not start docker Postgres: %s", err) } - svc, err := runner.StartService(context.Background(), connectPostgres) + svc, err := runner.StartService(context.Background(), connectPostgres(password)) if err != nil { t.Fatalf("Could not start docker Postgres: %s", err) } @@ -37,24 +49,26 @@ func PrepareTestContainer(t *testing.T, version string) (func(), string) { return svc.Cleanup, svc.Config.URL().String() } -func connectPostgres(ctx context.Context, host string, port int) (docker.ServiceConfig, error) { - u := url.URL{ - Scheme: "postgres", - User: url.UserPassword("postgres", "secret"), - Host: fmt.Sprintf("%s:%d", host, port), - Path: "postgres", - RawQuery: "sslmode=disable", - } +func connectPostgres(password string) docker.ServiceAdapter { + return func(ctx context.Context, host string, port int) (docker.ServiceConfig, error) { + u := url.URL{ + Scheme: "postgres", + User: url.UserPassword("postgres", password), + Host: fmt.Sprintf("%s:%d", host, port), + Path: "postgres", + RawQuery: "sslmode=disable", + } - db, err := sql.Open("postgres", u.String()) - if err != nil { - return nil, err - } - defer db.Close() + db, err := sql.Open("postgres", u.String()) + if err != nil { + return nil, err + } + defer db.Close() - err = db.Ping() - if err != nil { - return nil, err + err = db.Ping() + if err != nil { + return nil, err + } + return docker.NewServiceURL(u), nil } - return docker.NewServiceURL(u), nil }