From a91bb3db8b593e2ae9c310bad3ddd9d537fbb6eb Mon Sep 17 00:00:00 2001 From: robmonte <17119716+robmonte@users.noreply.github.com> Date: Wed, 22 Dec 2021 19:01:43 -0600 Subject: [PATCH 1/3] Add parameter to disable escaping for DB secrets engine connections --- changelog/13414.txt | 3 + sdk/database/helper/connutil/sql.go | 21 +++++- sdk/database/helper/connutil/sql_test.go | 90 ++++++++++++++++++++++-- 3 files changed, 106 insertions(+), 8 deletions(-) create mode 100644 changelog/13414.txt diff --git a/changelog/13414.txt b/changelog/13414.txt new file mode 100644 index 000000000000..54ae53dcfaf2 --- /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 bd2693a332d8..52a6b3f0516c 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 c.DisableEscaping && + 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 ae9d3ecf84ef..8fee935dd2d5 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,80 @@ 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 _, tc := range testCases { + t.Logf("username %q password %q", tc.Username, tc.Password) + + 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": true, + } + _, err := sql.Init(ctx, conf, false) + 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;") + } + } +} From 945fa1eeaf847a019df047ac20a67ca3c4bd5910 Mon Sep 17 00:00:00 2001 From: robmonte <17119716+robmonte@users.noreply.github.com> Date: Fri, 7 Jan 2022 11:53:32 -0600 Subject: [PATCH 2/3] Expand DisallowTemplates test case and wrap OR conditions in parentheses --- sdk/database/helper/connutil/sql.go | 8 +++--- sdk/database/helper/connutil/sql_test.go | 36 +++++++++++++----------- 2 files changed, 24 insertions(+), 20 deletions(-) diff --git a/sdk/database/helper/connutil/sql.go b/sdk/database/helper/connutil/sql.go index 52a6b3f0516c..e6191d163182 100644 --- a/sdk/database/helper/connutil/sql.go +++ b/sdk/database/helper/connutil/sql.go @@ -59,10 +59,10 @@ func (c *SQLConnectionProducer) Init(ctx context.Context, conf map[string]interf // Do not allow the username or password template pattern to be used as // part of the user-supplied username or password if c.DisableEscaping && - strings.Contains(c.Username, "{{username}}") || - strings.Contains(c.Username, "{{password}}") || - strings.Contains(c.Password, "{{username}}") || - strings.Contains(c.Password, "{{password}}") { + (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") } diff --git a/sdk/database/helper/connutil/sql_test.go b/sdk/database/helper/connutil/sql_test.go index 8fee935dd2d5..2ca11b758986 100644 --- a/sdk/database/helper/connutil/sql_test.go +++ b/sdk/database/helper/connutil/sql_test.go @@ -111,24 +111,28 @@ func TestSQLDisallowTemplates(t *testing.T) { {"abc{{username}}xyz", "123{{password}}789"}, {"abc{{{username}}}xyz", "123{{{password}}}789"}, } - for _, tc := range testCases { - t.Logf("username %q password %q", tc.Username, tc.Password) + 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": true, - } - _, err := sql.Init(ctx, conf, false) - 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) + 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;") + } } - } else { - assert.Equal(t, sql.ConnectionURL, "server=localhost;port=1433;user id=abc{username}xyz;password=123{password}789;database=mydb;") } } } From 927d4600bba9e8ff2bdeb44586f1fed26d8b4c98 Mon Sep 17 00:00:00 2001 From: robmonte <17119716+robmonte@users.noreply.github.com> Date: Fri, 7 Jan 2022 20:57:00 -0600 Subject: [PATCH 3/3] Always disallow templates in username or password --- sdk/database/helper/connutil/sql.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/sdk/database/helper/connutil/sql.go b/sdk/database/helper/connutil/sql.go index e6191d163182..6256ff1a4cf0 100644 --- a/sdk/database/helper/connutil/sql.go +++ b/sdk/database/helper/connutil/sql.go @@ -58,11 +58,11 @@ func (c *SQLConnectionProducer) Init(ctx context.Context, conf map[string]interf // Do not allow the username or password template pattern to be used as // part of the user-supplied username or password - if c.DisableEscaping && - (strings.Contains(c.Username, "{{username}}") || - strings.Contains(c.Username, "{{password}}") || - strings.Contains(c.Password, "{{username}}") || - strings.Contains(c.Password, "{{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") }