Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix MSSQL compatibility with Azure SQL #11555

Closed

Conversation

g-psantos
Copy link

@g-psantos g-psantos commented May 7, 2021

Fixes #10806

Removes a query from the MSSQL plugin that checks that a Server Login exists before attempting to change its password. This behavior is incompatible with SQL Server instances that rely on contained users and that do not allow cross-database queries, as is the case with Azure SQL Databases.

Additionally, some other engines (such as MySQL) do not check that a user exists before attempting to change its password, which suggests that this behavior is not essential.

The deletion of this query does not materially impact the behavior, as attempting to change the password for a nonexistent login (on a regular SQL Server instance) will result in an error message:

# Create a static login, create a static role, then delete the static login before running this command to replicate
$ vault write -force database/rotate-role/test-static
Error writing data to database/rotate-role/test-static: Error making API request.

URL: PUT http://localhost:8200/v1/database/rotate-role/test-static
Code: 500. Errors:

* 1 error occurred:
	* error setting credentials: unable to update user: rpc error: code = Internal desc = unable to update user: failed to execute query: mssql: Cannot alter the login 'staticuser', because it does not exist or you do not have permission.

If the Vault user is a contained user, then the "root rotation statements" parameter can be modified to alter the password of a user rather than a login:

ALTER USER vault WITH PASSWORD = 'securePassword123!';

Removes a query from the MSSQL plugin that checks that a Server Login
exists before attempting to change its password. This behavior is
incompatible with SQL Server instances that rely on contained users
and that do not allow cross-database queries, as is the case with
Azure SQL Databases.

The deletion of this query does not materially impact the behavior,
as attempting to change the password for an inexistent login (on a
regular SQL Server instance) will result in an error message that
states that the user either does not exist (or the Vault user does
not have permission to change its password).

If the Vault user is a contained user, then the "root rotation
statements" parameter can be modified to alter the password of a
`user` rather than a `login` (`ALTER USER vault WITH PASSWORD ...`).

Fixes hashicorp#10806
@hashicorp-cla
Copy link

hashicorp-cla commented May 7, 2021

CLA assistant check
All committers have signed the CLA.

@vercel vercel bot temporarily deployed to Preview – vault May 7, 2021 03:56 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook May 7, 2021 03:56 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook May 7, 2021 04:01 Inactive
@vercel vercel bot temporarily deployed to Preview – vault May 7, 2021 04:01 Inactive
@g-psantos g-psantos marked this pull request as ready for review May 7, 2021 04:01
@g-psantos
Copy link
Author

g-psantos commented Sep 14, 2021

@austingebauer Any chance you could get this reviewed/merged?

(We're still on v1.4.7, which has a couple of vulnerabilities, because later versions can't currently handle rotation of MSSQL contained users' credentials. It corrects a change introduced with #9062)

@austingebauer
Copy link
Contributor

Hi, @g-psantos. Thanks for making me aware of this issue and opening a PR. I will be having a look at this.

@vinay-gopalan
Copy link
Contributor

Hi @g-psantos, Thanks for opening the issue and the PR! A PR to include support for contained DBs in root rotation and lease revocation was recently put up here: #12839

This PR includes support for contained DBs in the MSSQL plugin, and based on a contained_db boolean field that lets Vault know that the connection being configured is a contained DB, the root rotation and secret revocation flow has been refactored. I'm closing this PR since the changes here are already incorporated. Please feel free to re-open an issue if problems persist even after the fix is merged. Thanks once again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MSSQL: Credential rotation does not work with Azure SQL Databases
4 participants