Remove an incomplete workaround for database connection_url formatting

plugins/database/mysql/connection_producer.go

@@ -69,7 +69,7 @@ func (c *mySQLConnectionProducer) Init(ctx context.Context, conf map[string]inte
 	password := c.Password
 
 	// QueryHelper doesn't do any SQL escaping, but if it starts to do so
-	// then maybe we won't be able to use it to do URL substitution any more.
+	// then maybe we won't be able to use it to do URL substitution anymore.
 	c.ConnectionURL = dbutil.QueryHelper(c.ConnectionURL, map[string]string{
 		"username": url.PathEscape(c.Username),
 		"password": password,

sdk/database/helper/connutil/sql.go

@@ -59,30 +59,19 @@ func (c *SQLConnectionProducer) Init(ctx context.Context, conf map[string]interf
 		return nil, fmt.Errorf("connection_url cannot be empty")
 	}
 
-	// Do not allow the username or password template pattern to be used as
-	// part of the user-supplied username or password
-	if strings.Contains(c.Username, "{{username}}") ||
-		strings.Contains(c.Username, "{{password}}") ||
-		strings.Contains(c.Password, "{{username}}") ||
-		strings.Contains(c.Password, "{{password}}") {
-
-		return nil, fmt.Errorf("username and/or password cannot contain the template variables")
-	}
-
-	// Don't escape special characters for MySQL password
-	// Also don't escape special characters for the username and password if
-	// the disable_escaping parameter is set to true
+	// Don't escape special characters for the username and password if the
+	// disable_escaping parameter is set to true. A user does this if the
+	// connection_url is actually not a URL, but database-specific other
+	// format, typically space or semicolon separated key=value pairs.
 	username := c.Username
 	password := c.Password
 	if !c.DisableEscaping {
 		username = url.PathEscape(c.Username)
-	}
-	if (c.Type != "mysql") && !c.DisableEscaping {
 		password = url.PathEscape(c.Password)
 	}
 
 	// QueryHelper doesn't do any SQL escaping, but if it starts to do so
-	// then maybe we won't be able to use it to do URL substitution any more.
+	// then maybe we won't be able to use it to do URL substitution anymore.
 	c.ConnectionURL = dbutil.QueryHelper(c.ConnectionURL, map[string]string{
 		"username": username,
 		"password": password,

sdk/database/helper/connutil/sql_test.go

@@ -8,8 +8,6 @@ import (
 	"net/url"
 	"strings"
 	"testing"
-
-	"github.com/stretchr/testify/assert"
 )
 
 func TestSQLPasswordChars(t *testing.T) {
@@ -22,6 +20,9 @@ func TestSQLPasswordChars(t *testing.T) {
 		{"postgres", "pass/word"},
 		{"postgres", "p@ssword"},
 		{"postgres", "pass\"word\""},
+		// Much to my surprise, CREATE USER "{{password}}" PASSWORD 'foo' worked.
+		{"{{password}}", "foo"},
+		{"user", "{{username}}"},
 	}
 	for _, tc := range testCases {
 		t.Logf("username %q password %q", tc.Username, tc.Password)
@@ -73,7 +74,7 @@ func TestSQLDisableEscaping(t *testing.T) {
 		{"ms'sq;l", "pass'wor;d", false},
 	}
 	for _, tc := range testCases {
-		t.Logf("username %q password %q disable_escaling %t", tc.Username, tc.Password, tc.DisableEscaping)
+		t.Logf("username %q password %q disable_escaping %t", tc.Username, tc.Password, tc.DisableEscaping)
 
 		sql := &SQLConnectionProducer{}
 		ctx := context.Background()
@@ -99,43 +100,3 @@ func TestSQLDisableEscaping(t *testing.T) {
 		}
 	}
 }
-
-func TestSQLDisallowTemplates(t *testing.T) {
-	testCases := []struct {
-		Username string
-		Password string
-	}{
-		{"{{username}}", "pass"},
-		{"{{password}}", "pass"},
-		{"user", "{{username}}"},
-		{"user", "{{password}}"},
-		{"{{username}}", "{{password}}"},
-		{"abc{username}xyz", "123{password}789"},
-		{"abc{{username}}xyz", "123{{password}}789"},
-		{"abc{{{username}}}xyz", "123{{{password}}}789"},
-	}
-	for _, disableEscaping := range []bool{true, false} {
-		for _, tc := range testCases {
-			t.Logf("username %q password %q disable_escaping %t", tc.Username, tc.Password, disableEscaping)
-
-			sql := &SQLConnectionProducer{}
-			ctx := context.Background()
-			conf := map[string]interface{}{
-				"connection_url":   "server=localhost;port=1433;user id={{username}};password={{password}};database=mydb;",
-				"username":         tc.Username,
-				"password":         tc.Password,
-				"disable_escaping": disableEscaping,
-			}
-			_, err := sql.Init(ctx, conf, false)
-			if disableEscaping {
-				if err != nil {
-					if !assert.EqualError(t, err, "username and/or password cannot contain the template variables") {
-						t.Errorf("Init error on %q %q: %+v", tc.Username, tc.Password, err)
-					}
-				} else {
-					assert.Equal(t, sql.ConnectionURL, "server=localhost;port=1433;user id=abc{username}xyz;password=123{password}789;database=mydb;")
-				}
-			}
-		}
-	}
-}

sdk/database/helper/dbutil/dbutil.go

@@ -5,7 +5,7 @@ package dbutil
 
 import (
 	"errors"
-	"fmt"
+	"regexp"
 	"strings"
 
 	"github.com/hashicorp/vault/sdk/database/dbplugin"
@@ -18,13 +18,24 @@ var (
 	ErrEmptyRotationStatement = errors.New("empty rotation statements")
 )
 
-// Query templates a query for us.
-func QueryHelper(tpl string, data map[string]string) string {
-	for k, v := range data {
-		tpl = strings.ReplaceAll(tpl, fmt.Sprintf("{{%s}}", k), v)
-	}
+var queryHelperRegex = regexp.MustCompile(`{{[^{}]+}}`)
 
-	return tpl
+// QueryHelper evaluates a simple string template syntax by replacing {{value}}
+// placeholders with the values from the supplied data map. Despite the name,
+// it is NOT only used to template queries - it is also used for connection
+// URIs or DSNs. Since it has no idea of the specific syntax into which it is
+// templating, it does not perform any escaping. Unbalanced opening and closing
+// brace sequences are passed through as is, as are any placeholders for which
+// there is no key found in the map.
+func QueryHelper(tpl string, data map[string]string) string {
+	return queryHelperRegex.ReplaceAllStringFunc(tpl, func(s string) string {
+		replacement, ok := data[s[2:len(s)-2]]
+		if ok {
+			return replacement
+		} else {
+			return s
+		}
+	})
 }
 
 // StatementCompatibilityHelper will populate the statements fields to support

sdk/database/helper/dbutil/dbutil_test.go

@@ -8,6 +8,7 @@ import (
 	"testing"
 
 	"github.com/hashicorp/vault/sdk/database/dbplugin"
+	"github.com/stretchr/testify/assert"
 )
 
 func TestStatementCompatibilityHelper(t *testing.T) {
@@ -62,3 +63,45 @@ func TestStatementCompatibilityHelper(t *testing.T) {
 		t.Fatalf("mismatch: %#v, %#v", expectedStatements3, statements3)
 	}
 }
+
+func TestQueryHelper(t *testing.T) {
+	data := map[string]string{
+		// These are typical keys you find in a data map used with QueryHelper
+		"username":   "hello",
+		"name":       "hello",
+		"password":   "world",
+		"expiration": "24h",
+	}
+	for _, tc := range []struct {
+		tpl      string
+		expected string
+	}{
+		{"", ""},
+		{"somedb://{{username}}:{{password}}@something", "somedb://hello:world@something"},
+		// Unknown placeholders pass through as is
+		{"user={{name}} other={{unknown}}", "user=hello other={{unknown}}"},
+		// Various incorrect delimiters pass through as is
+		{"{{{{{{{{", "{{{{{{{{"},
+		{"{{username}} {{incomplete", "hello {{incomplete"},
+		{"VALID UNTIL '{{expiration}}'; {{", "VALID UNTIL '24h'; {{"},
+		// This case tests whether `{{!{{password}}` successfully looks past the earlier unmatched {{
+		{"}}backwards{{!{{password}}!", "}}backwards{{!world!"},
+	} {
+		assert.Equal(t, tc.expected, QueryHelper(tc.tpl, data),
+			"template processing produced unexpected result")
+	}
+}
+
+// TestQueryHelper_Recursion confirms QueryHelper does not replace placeholders that were themselves added as part of
+// a replacement value.
+func TestQueryHelper_Recursion(t *testing.T) {
+	data := map[string]string{
+		"a": "A{{a}}{{b}}{{c}}{{d}}",
+		"b": "B{{a}}{{b}}{{c}}{{d}}",
+		"c": "C{{a}}{{b}}{{c}}{{d}}",
+		"d": "D{{a}}{{b}}{{c}}{{d}}",
+	}
+	assert.Equal(t, "A{{a}}{{b}}{{c}}{{d}}B{{a}}{{b}}{{c}}{{d}}C{{a}}{{b}}{{c}}{{d}}D{{a}}{{b}}{{c}}{{d}}",
+		QueryHelper("{{a}}{{b}}{{c}}{{d}}", data),
+		"template processing produced unexpected result")
+}