From 15bd5fd6b6bde395ec7a456acefa1c30dacc916d Mon Sep 17 00:00:00 2001 From: Robert <17119716+robmonte@users.noreply.github.com> Date: Mon, 10 Jan 2022 12:05:17 -0600 Subject: [PATCH] secrets/database: Add parameter to disable escaping username and password chars for DB connections (#13414) * Add a parameter that disables escaping characters in the username or password fields for secrets engines database connections * Always disallow template variables inside the username or password --- changelog/13414.txt | 3 + sdk/database/helper/connutil/sql.go | 21 +++++- sdk/database/helper/connutil/sql_test.go | 94 ++++++++++++++++++++++-- 3 files changed, 110 insertions(+), 8 deletions(-) create mode 100644 changelog/13414.txt diff --git a/changelog/13414.txt b/changelog/13414.txt new file mode 100644 index 000000000..54ae53dcf --- /dev/null +++ b/changelog/13414.txt @@ -0,0 +1,3 @@ +```release-note:improvement +secrets/database: Add database configuration parameter 'disable_escaping' for username and password when connecting to a database. +``` diff --git a/sdk/database/helper/connutil/sql.go b/sdk/database/helper/connutil/sql.go index bd2693a33..6256ff1a4 100644 --- a/sdk/database/helper/connutil/sql.go +++ b/sdk/database/helper/connutil/sql.go @@ -26,6 +26,7 @@ type SQLConnectionProducer struct { MaxConnectionLifetimeRaw interface{} `json:"max_connection_lifetime" mapstructure:"max_connection_lifetime" structs:"max_connection_lifetime"` Username string `json:"username" mapstructure:"username" structs:"username"` Password string `json:"password" mapstructure:"password" structs:"password"` + DisableEscaping bool `json:"disable_escaping" mapstructure:"disable_escaping" structs:"disable_escaping"` Type string RawConfig map[string]interface{} @@ -55,16 +56,32 @@ 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 + username := c.Username password := c.Password - if c.Type != "mysql" { + 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. c.ConnectionURL = dbutil.QueryHelper(c.ConnectionURL, map[string]string{ - "username": url.PathEscape(c.Username), + "username": username, "password": password, }) diff --git a/sdk/database/helper/connutil/sql_test.go b/sdk/database/helper/connutil/sql_test.go index ae9d3ecf8..2ca11b758 100644 --- a/sdk/database/helper/connutil/sql_test.go +++ b/sdk/database/helper/connutil/sql_test.go @@ -3,7 +3,10 @@ package connutil import ( "context" "net/url" + "strings" "testing" + + "github.com/stretchr/testify/assert" ) func TestSQLPasswordChars(t *testing.T) { @@ -16,9 +19,6 @@ 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) @@ -26,9 +26,10 @@ func TestSQLPasswordChars(t *testing.T) { sql := &SQLConnectionProducer{} ctx := context.Background() conf := map[string]interface{}{ - "connection_url": "postgres://{{username}}:{{password}}@localhost:5432/mydb", - "username": tc.Username, - "password": tc.Password, + "connection_url": "postgres://{{username}}:{{password}}@localhost:5432/mydb", + "username": tc.Username, + "password": tc.Password, + "disable_escaping": false, } _, err := sql.Init(ctx, conf, false) if err != nil { @@ -54,3 +55,84 @@ func TestSQLPasswordChars(t *testing.T) { } } } + +func TestSQLDisableEscaping(t *testing.T) { + testCases := []struct { + Username string + Password string + DisableEscaping bool + }{ + {"mssql{0}", "password{0}", true}, + {"mssql{0}", "password{0}", false}, + {"ms\"sql\"", "pass\"word\"", true}, + {"ms\"sql\"", "pass\"word\"", false}, + {"ms'sq;l", "pass'wor;d", true}, + {"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) + + 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": tc.DisableEscaping, + } + _, err := sql.Init(ctx, conf, false) + if err != nil { + t.Errorf("Init error on %q %q: %+v", tc.Username, tc.Password, err) + } else { + if tc.DisableEscaping { + if !strings.Contains(sql.ConnectionURL, tc.Username) || !strings.Contains(sql.ConnectionURL, tc.Password) { + t.Errorf("Raw username and/or password missing from ConnectionURL") + } + } else { + if strings.Contains(sql.ConnectionURL, tc.Username) || strings.Contains(sql.ConnectionURL, tc.Password) { + t.Errorf("Raw username and/or password was present in ConnectionURL") + } + } + } + } +} + +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;") + } + } + } + } +}