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

[VAULT-3379] Add support for contained DBs in MSSQL root rotation and lease revocation #12839

Merged
merged 10 commits into from
Oct 19, 2021

Conversation

vinay-gopalan
Copy link
Contributor

@vinay-gopalan vinay-gopalan commented Oct 14, 2021

This PR adds a containedDB field to the Vault plugin's MSSQL implementation in order to support contained DBs like AzureSQL that do not allow cross DB queries and don't have server logins. This boolean field can be set to skip server login checks during root rotation and also execute a default user deletion query for lease revocation if no revocation statements are provided:

"DROP USER IF EXISTS [{{name}}]"

Fixes #10806
Closes #12830

This PR also updates the dependency github.com/denisenkom/go-mssqldb to v0.11.0 and resolves panics in closing SQL connections. A bug in the tests where users were not being deleted after each test has also been fixed.

TODO: Update docs to state the use of containedDB and include the default user deletion query.

Output from acceptance tests (need to follow setup steps listed here):

make test TEST=./plugins/database/mssql                                    
==> Checking that build is using go version >= 1.16.7...
==> Using go version 1.17...
ok      github.com/hashicorp/vault/plugins/database/mssql       (cached)


make test TESTARGS="-count=1" TEST=./plugins/database/mssql
==> Checking that build is using go version >= 1.16.7...
==> Using go version 1.17...
ok      github.com/hashicorp/vault/plugins/database/mssql       0.978s

@vinay-gopalan vinay-gopalan requested a review from a team October 14, 2021 19:24
@vercel vercel bot temporarily deployed to Preview – vault October 14, 2021 19:27 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook October 14, 2021 19:27 Inactive
@vercel vercel bot temporarily deployed to Preview – vault October 14, 2021 19:28 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook October 14, 2021 19:28 Inactive
@@ -201,6 +216,19 @@ func (m *MSSQL) revokeUserDefault(ctx context.Context, username string) error {
return err
}

// Check if DB is contained
if m.containedDB {
revokeStmt, err := db.PrepareContext(ctx, fmt.Sprintf("DROP USER IF EXISTS [%s]", username))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default user deletion query for contained DBs. Since there are no server logins or database logins, we can simply drop the user. User still needs permissions and this is only in the case that no revocation statements are provided

@vercel vercel bot temporarily deployed to Preview – vault-storybook October 15, 2021 21:28 Inactive
@vercel vercel bot temporarily deployed to Preview – vault October 15, 2021 21:28 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook October 15, 2021 21:48 Inactive
@vercel vercel bot temporarily deployed to Preview – vault October 15, 2021 21:48 Inactive
Copy link
Contributor

@calvn calvn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updateUserPass on L346 can set a query that calls alterLoginSQL which contains ALTER LOGIN ... if changePass.Statements.Commands is empty. This may not work with contained DBs though.

@vercel vercel bot temporarily deployed to Preview – vault October 19, 2021 17:57 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook October 19, 2021 17:57 Inactive
@vinay-gopalan
Copy link
Contributor Author

updateUserPass on L346 can set a query that calls alterLoginSQL which contains ALTER LOGIN ... if changePass.Statements.Commands is empty. This may not work with contained DBs though.

@calvn good call! Updated to check if DB is contained on L346

@vercel vercel bot temporarily deployed to Preview – vault October 19, 2021 18:11 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook October 19, 2021 18:11 Inactive
Copy link
Contributor

@calvn calvn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but let's force an acceptance test re-run by passing TESTARGS="-count=1" as well.

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