Ensure that URL encoded passwords are properly redacted. (#14744)

The URL password redaction operation did not handle the case where the
database connection URL was provided as a percent-encoded string, and
its password component contained reserved characters. It attempted to
redact the password by replacing the unescaped password in the
percent-encoded URL. This resulted in the password being revealed when
reading the configuration from Vault.

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 {

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()
 			}
 		}
 

changelog/14744.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+secrets/database: Ensure that a `connection_url` password is redacted in all cases.
+```

helper/testhelpers/postgresql/postgresqlhelper.go

@@ -12,49 +12,63 @@ 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")
 	}
 
 	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)
 	}
 
 	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
 }