[DBPW 4/5] Update DB engine to support v4 and v5 interfaces with password policies by pcman312 · Pull Request #9878 · hashicorp/vault

pcman312 · 2020-09-02T17:22:10Z

Overview

This PR is part of a larger feature adding support for password policies into the combined database engine. This feature is being split into multiple PRs to make for smaller reviews & earlier feedback.

Updates the combined database secrets engine to serve both Database v5 and v4 interfaces. Most of the logic is located in versioning.go. This adds a new type: databaseVersionWrapper that is essentially a one-of type. It can either contain the v4 Database type or a v5 Database type. It should never contain both. This is then used everywhere the v4 Database was previously referenced in order to switch on the version of the interface. This was the simplest approach I could come up with since the two Database interfaces don't have useful overlap that would allow us to define a shared interface.

If using the v5 interface, this also supports using password policies for password generation. Setting password_policy=<string> when configuring the database will use the specified password policy for password generation. This is a change in current behavior. In the v4 Database, the database plugin is responsible for generating passwords for dynamic & static users, as well as the root user. In the v5 Database, this responsibility is handled by Vault such that passwords are generated in Vault & passed into the plugin's functions.

PR Size

This PR looks huge (20k+ lines!) but it's actually much smaller. Most of those lines are from vendoring:

$ git diff --stat dbpw-03-plugin dbpw-04-engine -- :/\!vendor\/\*
 builtin/logical/database/backend.go                 |  38 +++---
 builtin/logical/database/backend_test.go            |   6 +-
 builtin/logical/database/mocks_test.go              | 101 +++++++++++++++
 builtin/logical/database/path_config_connection.go  |  49 ++++---
 builtin/logical/database/path_creds_create.go       |  16 +--
 builtin/logical/database/path_rotate_credentials.go |  59 +++++----
 builtin/logical/database/rollback.go                |  26 +---
 builtin/logical/database/rollback_test.go           |  16 +--
 builtin/logical/database/rotation.go                |  19 +--
 builtin/logical/database/secret_creds.go            |  40 +++---
 builtin/logical/database/versioning.go              | 292 ++++++++++++++++++++++++++++++++++++++++++
 builtin/logical/database/versioning_large_test.go   | 310 +++++++++++++++++++++++++++++++++++++++++++++
 builtin/logical/database/versioning_test.go         | 666 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 go.mod                                              |   2 +-
 go.sum                                              |   4 +
 vault/plugin_catalog.go                             |  18 ++-
 16 files changed, 1520 insertions(+), 142 deletions(-)

Prerequisites

[DBPW 3/5] Add DBv5 plugin serving & management functions #9745 merged
Change base branch to master
Rebased against master

Related PRs

Original password policies PR
1/X - Database interface & gRPC
2/X - Middleware
3/X - Plugin management

This mirrors what DBv4 is doing, but with the updated interface

This continues to support backwards compatibility with V4

calvn · 2020-09-04T23:27:31Z

+			client.Close()
+			return consts.PluginTypeDatabase, nil
+		} else {
+			logger.Warn(fmt.Sprintf("received %s attempting as db plugin, attempting as auth/secret plugin", err))


This statement is not entirely accurate (and might print twice if it's not a db plugin by both this line and L88). We should skip logging this warning, or move the V4 check into this else block.

👍 Moved Database type checking into its own function

calvn · 2020-09-05T00:19:58Z

+
+type databaseVersionWrapper struct {
+	database       newdbplugin.Database
+	legacyDatabase dbplugin.Database


Even though there's the intent to leave the v4 nomenclature out out of code references, I think that terms like legacy, old, new, or similar have its own set of problems too. If we were to introduce the next iteration of database plugins (which is unlikely but not impossible), this terminology might make things confusing as it would be come the "legacy" database, and the legacyDatabase is really a oldLegacyDatabase, which would be just as ambiguous if not more so.

Unfortunately I don't have a good way of differentiating between versions other than referring to the gRPC version number. I've switched this over to using that nomenclature within the version wrapping struct since it has more forwards compatibility.

calvn · 2020-09-05T00:27:03Z

+// Change new user
+// /////////////////////////////////////////////////////////////////////////////////
+
+func createUser(ctx context.Context, dbw databaseVersionWrapper, pg passwordGenerator, statements dbplugin.Statements, displayName, roleName string, expiration time.Time, passwordPolicy string) (username, password string, err error) {


Can we make these funcs be part of dbPluginInstance's methods?

This has been put into a separate wrapper struct in version_wrapper.go rather than on the dbPluginInstance.

calvn · 2020-09-11T21:46:51Z

 	}
 }

+func (b *databaseBackend) deleteWal(ctx context.Context, storage logical.Storage, walID string) {


I don't see this being re-used elsewhere, can we move it back to the func above?

Done. I think at one point I had this doing more but it's not useful as a separate function any more.

calvn · 2020-09-11T21:49:41Z

 	// Attempt to get a connection with the WAL entry new password.
 	config.ConnectionDetails["password"] = entry.NewPassword
-	dbc, err := b.GetConnectionWithConfig(ctx, entry.ConnectionName, config)
+	db, err := b.GetConnectionWithConfig(ctx, entry.ConnectionName, config)


Nit: can we name the var dbi to have it be consistent with similar calls?

Done. I also updated the other places where GetConnection is being called to use dbi.

calvn · 2020-09-11T22:07:28Z

+		v5Client.Close()
+		return nil
+	}
+	merr = multierror.Append(merr, err)


Does the error from there indicate which database version was attempted? If not, might be useful to wrap the error message with this bit of info.

It does not but that's because the error will only be returned if both v5 and v4 fail.

Holding off on doing anything with this until we figure out what to do with the log message that this error is used in.

Wrapped the error to indicate which error failed on which database version. This will bubble up to the log message referenced in the next comment thread.

calvn · 2020-09-11T22:13:49Z

+	if err == nil {
+		return consts.PluginTypeDatabase, nil
 	}
+	logger.Warn(fmt.Sprintf("received %s attempting as db plugin, attempting as auth/secret plugin", err))


The error might look a bit off in here now since it can be a multierror, so consider appending error to the end of the string. https://play.golang.org/p/rOofMYebB2d

I ended up keeping this warning since it was there in the previous version, however I don't think this message makes sense as a warning. Every plugin goes through this path, and if the plugin isn't a database plugin you're going to get a warning saying the plugin isn't a database plugin. What do you think of changing this to a debug (or trace?) log? I'm also tempted to not include the error message at all as I'm skeptical that it's useful in this context. Thoughts?

getPluginTypeFromUnknown shouldn't be called that often -- once when setting up the plugin catalog if a plugin has not been upgraded yet, and when a plugin with unknown type is added to the catalog (which shouldn't happen since we're now enforcing the type to be provided via the API). The latter is more of a backward compat behavior, so new plugin registration operation shouldn't be hitting this path.

For posterity:
@calvn & I talked offline and agreed that we should keep this as a warning, but move it to the end of the function if we are unable to determine the type of the plugin. I also changed it so the message is a bit more descriptive differentiating between an unknown type vs one that isn't recognized by this particular function. That will help guard against adding new types to the logical.BackendType enum.

calvn · 2020-09-15T23:10:53Z

 			},
 			"allowed_roles":                      []string{"*"},
 			"root_credentials_rotate_statements": []string{},
+			"password_policy":                    "",


Is this required to be provided?

No, password policies are optional.

Is there a reason why we're explicitly setting this to "" on every case? Not sure if I missed this test case, but we could test various permutation of how this value is passed in (missing, empty, valid, invalid).

Nvm, this is the expected value returned! Additional tests on password_policy would be nice though since this is a new param on the config, on both v4 and v5 test cases to ensure that defaults are used (if empty and on v5) or ignored (if on v4).

The key needs to exist because the DatabaseConfig doesn't have omitempty on this field.

calvn · 2020-09-16T00:02:01Z

+// does not have a way of returning the password so this function signature needs to be different.
+// The password returned here should be considered the source of truth, not the provided password.
+// Errors if the wrapper does not contain an underlying database.
+func (d databaseVersionWrapper) NewUser(ctx context.Context, req newdbplugin.NewUserRequest) (resp newdbplugin.NewUserResponse, password string, err error) {


This probably okay as-is, but I also wanted to bring up a suggestion that was tossed around, which is the idea of having the password be part of newdbplugin.NewUserResponse (and its proto equivalent), but marking that field as being there solely for backward compatibility purposes. This should address the issue of NewUser being unable to return the password string if we had a wrapper around v4. We could probably do something in the same vein (e.g. an UpdateType or similar on the request payload) for UpdateUser so that the v4 wrapper knows what method to call.

I don't think this is a good idea for three reasons:

It conflates the two interfaces. We'd be stuck with the new interface returning a password even if we removed the old interface.

It confuses plugin authors. NewUser is receiving a password and returning one?

We're explicitly moving password generation out of plugins and into Vault. By having the response return a password, we revert that decision.

Unfortunately, we cannot make the version selection code simpler because we are dramatically changing what parts of the system are responsible for what (in particular password generation).

That's right, I'm satisfied with the current approach since there are quite a few downsides for little gain going with the other suggestion. Documentation around fields should hopefully help alleviate with the confusion on the return value. However, leaking some of the old behavior that will be unused in v5 and having dangling fields when v4 is removed is certainly non-ideal.

calvn · 2020-09-16T22:59:52Z

+
+			ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
+			defer cancel()
+			password, err := dbw.GeneratePassword(ctx, passGen, "test_policy")


Looks like this is always testing for a password to be provided at the wrapper layer. Can we also test for "" so that we make sure the call to defaultPasswordGenerator.Generate(ctx, rand.Reader) is exercised?

Added this test. It was being exercised in another test, but it wasn't explicit.

briankassouf · 2020-09-17T03:32:23Z

+// newDatabaseWrapper figures out which version of the database the pluginName is referring to and returns a wrapper object
+// that can be used to make operations on the underlying database plugin.
+func newDatabaseWrapper(ctx context.Context, pluginName string, sys pluginutil.LookRunnerUtil, logger log.Logger) (dbw databaseVersionWrapper, err error) {
+	newDB, err := newdbplugin.PluginFactory(ctx, pluginName, sys, logger)


@pcman312 @calvn Since we are switching to a completely new plugin client this would be a perfect time to switch from the TLSProvider to AutoMTLS config on go plugin!

Will attempt! This will end up being in a separate PR though since it will be a part of the SDK code rather than the engine.

pcman312 added 3 commits September 1, 2020 10:47

Add DBv5 plugin serving & management functions

dcd0677

This mirrors what DBv4 is doing, but with the updated interface

WIP - Make database engine support database V5

bab86fb

This continues to support backwards compatibility with V4

Update DB engine to support either v4 or v5 interfaces

a46ba73

This was referenced Sep 2, 2020

[DBPW 3/5] Add DBv5 plugin serving & management functions #9745

Merged

[DBPW 1/5] Add Database v5 interface with gRPC client & server #9641

Merged

[DBPW 2/5] Create middleware for Database v5 #9642

Merged

pcman312 added 5 commits September 2, 2020 11:36

Remove debugging

44ffb36

Fix GeneratePassword tests with legacy generation

e95901f

Add black box tests to ensure backwards compatiblity

fd0cf11

Updated vendoring

bcbfb7e

Add mock v4 & v5 DBs for testing

4e63344

calvn reviewed Sep 5, 2020

View reviewed changes

pcman312 added 4 commits September 10, 2020 17:01

Change version selection to a struct

8808d02

Increase timeouts to decrease flakiness potential

b3111d6

Don't always try rotating root creds

eadbf80

Simplify database plugin selection

9de404e

calvn reviewed Sep 11, 2020

View reviewed changes

pcman312 added 5 commits September 14, 2020 11:43

Remove unnecessary deleteWal function

90fc9b1

Use consistent variable names

6e62d1f

Minor formatting fixes

0331cde

Return the right error

7cd0caa

Fix typo

e14010d

This was referenced Sep 14, 2020

DBPW - Add test helpers for DB v5 interface #9948

Merged

DBPW - Update MongoDB to adhere to v5 Database interface #9949

Closed

Base automatically changed from dbpw-03-plugin to master September 14, 2020 22:04

pcman312 added 2 commits September 14, 2020 17:58

Log a warning if unable to determine type rather than at each step

65e4bc8

More explicit erroring on database lookup

903f0c3

calvn reviewed Sep 16, 2020

View reviewed changes

Merge branch 'master' into dbpw-04-engine

8b7f2e2

calvn approved these changes Sep 16, 2020

View reviewed changes

calvn reviewed Sep 16, 2020

View reviewed changes

Comment thread builtin/logical/database/version_wrapper.go Outdated

Fix typos; function renaming; added error wrapping

a7a39b4

calvn reviewed Sep 16, 2020

View reviewed changes

briankassouf reviewed Sep 17, 2020

View reviewed changes

pcman312 added 2 commits September 18, 2020 10:48

Add no-password policy test

8d148fb

Update logging to structured logging

a0d0414

Jamesedwand0147 approved these changes Sep 18, 2020

View reviewed changes

pcman312 merged commit 1cd0c05 into master Sep 18, 2020

pcman312 deleted the dbpw-04-engine branch September 18, 2020 21:12

pcman312 mentioned this pull request Sep 21, 2020

[DBPW 5/5] Use AutoMTLS with DB plugins #10008

Merged

tvoran mentioned this pull request Sep 26, 2020

Migrate elasticsearch plugin to v5 database plugin interface hashicorp/vault-plugin-database-elasticsearch#19

Merged

3 tasks

pcman312 changed the title ~~[DBPW 4/X] Update DB engine to support v4 and v5 interfaces with password policies~~ [DBPW 4/5] Update DB engine to support v4 and v5 interfaces with password policies Oct 7, 2020

pcman312 added the secret/database label Oct 19, 2020

Conversation

pcman312 commented Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

PR Size

Prerequisites

Related PRs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pcman312 Sep 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calvn Sep 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

pcman312 commented Sep 2, 2020 •

edited

Loading

pcman312 Sep 14, 2020 •

edited

Loading

calvn Sep 16, 2020 •

edited

Loading