Add parameter to disable escaping the username and password for database connections

[robmonte]

This feature adds the ability to disable Vault escaping any special characters in the username and password fields when connecting to a database. When writing the database config, supplying disable_escaping="true" will activate this feature. It is off by default/not specified.

[hashicorp-cla]

All committers have signed the CLA.

[hsimon-hashicorp]

Thanks for this submission! Can you please add a changelog entry? I'll get a reviewer for you as soon as I can. Thanks!

[austingebauer]

Nice job, @robmonte! Have a few questions/suggestions around where the configuration parameter should live. +1 @hsimon-hashicorp on changelog entry needed. Would also be nice to add some level of test coverage for this.

I was able to successfully test this using an ADO style connection string and MSSQL. One observation about the ADO style connection string is that we no longer warn for a password in connection_url or redact when reading the config. This is only true is the username/password is supplied directly in the connection_url, but I thought it'd be worth mentioning.

[robmonte]

Still working on updating sql_test.go

[calvn]

One small note, otherwise looks good!

[austingebauer]

LGTM!

[maxb]

What problem is this code trying to solve?

	// 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")
	}

I just came across it in the code, and it is unclear to me why this logic was added.

[robmonte]

What problem is this code trying to solve?

	// 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")
	}

I just came across it in the code, and it is unclear to me why this logic was added.

Hi Max, when working on this feature I kept getting infrequent test failures in between successful runs, like a race condition. At the time, I had narrowed it down to the ConnectionURL sometimes being a different value despite no changes.

When building the ConnectionURL, it calls the function QueryHelper which then calls strings.ReplaceAll. It was at this point that it seemed to randomly fail replacing the the old value with a new value containing the old value itself.

At the time I couldn't figure out why this was happening. I started going down this rabbit hole again because of your question here, and I think I figured out the issue this time.

To quickly test this, I first commented out the block of code restricting the use of the template values. Next, I used the TestSQLDisableEscaping test case in sql_test.go. I commented out all of the table tests and used {"{{password}}mssql{0}", "password{0}", true} as the only test value. Here is the git patch file if you'd like to run the same test:
gh.patch

I was overlooking the fact that this QueryHelper function was calling strings.ReplaceAll in a loop from a map. Since map order isn't guaranteed, it changes the result.

Replacing the username template first (the common case):

1. Starting value: server=localhost;port=1433;user id={{username}};password={{password}};database=mydb;
2. Replace username: server=localhost;port=1433;user id={{password}}mssql{0};password={{password}};database=mydb;
3. Replace password: server=localhost;port=1433;user id=password{0}mssql{0};password=password{0};database=mydb;

...but sometimes, it does the password template first:

1. Starting value: server=localhost;port=1433;user id={{username}};password={{password}};database=mydb;
2. Replace password: server=localhost;port=1433;user id={{username}};password=password{0};database=mydb;
3. Replace username: server=localhost;port=1433;user id={{password}}mssql{0};password=password{0};database=mydb;

The opposite case would also happen from using {{username}} in the password value.

At the time, the quickest solution was to simply disallow the use of the template variables as part of the username or password themselves. I also don't see any good coming from allowing the use of them. It just seems like it's asking for trouble.

[maxb]

Aha! Thanks for the elucidation! I had observed that all template expressions were being replaced in one call to dbutil.QueryHelper, but hadn't looked inside it to see that it is performing a separate iteration over the string for each one, rendering it liable to the kinds of effects you saw.

Disallowing the specific template expressions as replacements is a reasonable workaroud - I dislike it because of the potential of surprising someone at some point - it wouldn't be that crazy for {{password}} to appear in a test password - but I admit it is pretty unlikely to happen in practice.

[fairclothjm]

@robmonte Yes, thanks for the info! I wonder if we should add a comment with a brief reason for this behavior? Specifically might be worth mentioning that it is only required to prevent race conditions in the tests.

[maxb]

I wonder if we should add a comment with a brief reason for this behavior?

I propose instead we fix the underlying templating routine so the workaround is no longer required: #22224

Specifically might be worth mentioning that it is only required to prevent race conditions in the tests.

It would be possible to run into this problem in production too if usernames or passwords contained any sequences like {{name}}, {{username}}, {{password}}, {{expiration}}, as whilst this workaround guarded one particular use of dbutil.QueryHelper, there are others.