Combined Database Backend with Plugins #2200

briankassouf · 2016-12-20T20:10:29Z

This PR introduces a new dynamic secrets backend that combines the functionality in the existing database backends and allows for more database types to be added through plugins. It uses hashicorp/go-plugin.

Plugins in this backend can be run in one of two ways: builtin vs external. Builtin plugins ship with the vault binary and are run as a subcommand of vault. External plugins can be placed in a configured plugin directory on the server.

External plugins must be registered to vault's plugin catalog before they can be run by a backend. The catalog requires a plugin to live in the configured plugin directory and a SHA-256 checksum matching the binary must be provided.

Once a plugin is registered to vault, or if it's built in, it can be configured through the database backend and used to create credentials for the database configuration.

======= Jeff's review list:

…ssion

…ate a new database instance with an updated config

…tings

…conns

chrishoffman · 2017-05-03T22:14:32Z

builtin/logical/database/backend.go

+	sync.RWMutex
+}
+
+// resetAllDBs closes all connections from all database types


I know we don't lint but s/resetAllDBs/closeAllDBs

chrishoffman · 2017-05-03T22:30:40Z

builtin/logical/database/backend.go

+}
+
+func (b *databaseBackend) Role(s logical.Storage, n string) (*roleEntry, error) {
+	entry, err := s.Get("role/" + n)


Can you update n to something like name or roleName? It looks like name is used to describe the database name so roleName may make more sense?!?

chrishoffman · 2017-05-03T23:48:56Z

builtin/logical/database/path_config_connection.go

+}
+
+// pathConnectionRead reads out the connection configuration
+func (b *databaseBackend) connectionReadHandler() framework.OperationFunc {


s/pathConnectionRead/connectionReadHandler/

chrishoffman · 2017-05-03T23:51:05Z

builtin/logical/database/path_config_connection.go

+for a full list of accepted connection details. 
+
+In addition to the database specific connection details, this endpoing also
+accepts:


s/endpoing/endpoint/

chrishoffman · 2017-05-04T00:09:10Z

builtin/logical/database/backend.go

+const backendHelp = `
+The database backend supports using many different databases
+as secret backends, including but not limited to:
+cassandra, msslq, mysql, postgres


s/msslq/mssql/

chrishoffman · 2017-05-04T00:14:12Z

helper/pluginutil/tls.go

+
+// generateSignedCert is used internally to create certificates for the plugin
+// client and server. These certs are signed by the given CA Cert and Key.
+func GenerateCert() ([]byte, *ecdsa.PrivateKey, error) {


I feel like many of the methods in this file should all be private.

chrishoffman · 2017-05-04T00:16:01Z

helper/pluginutil/tls.go

+			x509.ExtKeyUsageServerAuth,
+		},
+		KeyUsage:     x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment | x509.KeyUsageKeyAgreement,
+		SerialNumber: big.NewInt(mathrand.Int63()),


You can use certutil.GenerateSerialNumber() here. This is actually not random unless it is seeded. This is all internal but we should probably generate unique certificate serials.

Nice catch!

bk actually asked me about that a couple of days ago; since each plugin gets a unique cert and serial numbers must be unique for each CA (but only per CA) this isn't an issue, but that said, randomizing is totally fine too.

I think the uniqueness only matters if you have a CRL, but I could be wrong. I don't think this CA implementation is keeping a database so I'm not sure what duplicate serials would do. Anyway, I believe if you restart your vault instance you will start your serials over again in the same sequence.

Yes and no...in theory you should be able to identify any certificate uniquely via authority ID and serial, for whatever uses you may have. CRLs only take serials into account but that mostly means if you have duplicates you may block more than one rather than just one cert, so not a security hole really.

You are correct that if you restarted Vault you'd get the same sequence but the CA cert/key would be different, hence the authority ID would be different, so it doesn't really matter. No harm to randomize though!

chrishoffman · 2017-05-04T00:20:02Z

helper/pluginutil/tls.go

+		unwrapToken := os.Getenv(PluginUnwrapTokenEnv)
+
+		// Ensure unwrap token is a JWT
+		if strings.Count(unwrapToken, ".") != 2 {


This check as one of the first checks in jwt.ParseJWT. The error is a little different but it is a constant so you could switch on it.

chrishoffman · 2017-05-04T01:05:14Z

helper/strutil/strutil.go

@@ -31,6 +31,19 @@ func StrListSubset(super, sub []string) bool {

 // Parses a comma separated list of strings into a slice of strings.
 // The return slice will be sorted and will not contain duplicate or
+// empty items.
+func ParseDedupAndSortStrings(input string, sep string) []string {
+	input = strings.TrimSpace(input)


It looks like this came back with one of the commits. This was renamed to ParseDedupLowercaseAndSortStrings.

I actually added this function! I didn't want it to lowercase :)

My bad, I scanned them and they looked identical. I missed that bool flag.

chrishoffman · 2017-05-04T01:24:29Z

website/source/docs/secrets/databases/mssql.html.md

+The full list of configurable options can be seen in the [MSSQL database
+plugin API](/api/secret/databases/mssql.html) page.
+
+Or for more information on the Database secret backend's HTTP API please see the [Database secret


I think "Or" could probably be removed from this sentence in all these plugin docs.

calvn

A couple of small items.

calvn · 2017-05-04T00:05:10Z

builtin/logical/database/backend.go

+func (b *databaseBackend) DatabaseConfig(s logical.Storage, name string) (*DatabaseConfig, error) {
+	entry, err := s.Get(fmt.Sprintf("config/%s", name))
+	if err != nil {
+		return nil, fmt.Errorf("failed to read connection configuration with name: %s", name)


Should err be appended and returned back as well?

calvn · 2017-05-04T01:57:36Z

builtin/logical/database/path_roles.go

+			},
+			"rollback_statements": {
+				Type: framework.TypeString,
+				Description: `SQL statements to be executed to revoke a user. Must be a semicolon-separated


s/SQL statements/Statements/

calvn · 2017-05-04T02:01:02Z

logical/system_view.go

+	// name. Returns a PluginRunner or an error if a plugin can not be found.
+	LookupPlugin(string) (*pluginutil.PluginRunner, error)
+
+	// MlockEnabled returns the configuration setting for Enableing mlock on


s/Enableing/enabling/

calvn · 2017-05-04T02:06:53Z

plugins/helper/database/credsutil/cassandra.go

+	if err != nil {
+		return "", err
+	}
+	username := fmt.Sprintf("vault_%s_%s_%d", displayName, userUUID, time.Now().Unix())


Why is this username generation different from that of SQLCredentialsProducer?

username := fmt.Sprintf("v-%s-%s", displayName, userUUID)

To keep it consistent with how the other cassandra backend has been doing it!

cbandy · 2017-05-04T12:25:51Z

helper/builtinplugins/builtin.go

+	// different username settings passed by the constructor.
+	"mysql-database-plugin":        mysql.New(mysql.DisplayNameLen, mysql.UsernameLen),
+	"aurora-database-plugin":       mysql.New(mysql.LegacyDisplayNameLen, mysql.LegacyUsernameLen),
+	"rds-database-plugin":          mysql.New(mysql.LegacyDisplayNameLen, mysql.LegacyUsernameLen),


Both RDS and Aurora have PostgreSQL flavors. It may be prudent to include mysql in the names here.

Interesting. It looks like that got added in November 2016, long after the original RFC for this. That's a good catch, and maybe we should just add some variants for those up-front, even if (like here) they are the same to start out with. @briankassouf what do you think?

chrishoffman

Look good to me!

…epend on get

…atabase-refactor

jefferai

A thousand 🚢s and a million 🌈s, sir.

briankassouf force-pushed the database-refactor branch from 0ac09a4 to a80d67d Compare February 7, 2017 23:13

Brian Kassouf added 8 commits March 3, 2017 14:38

Begin work on database refactor

3d77a9a

More work on refactor and cassandra database

ad17d11

Add mysql database type

bfbb104

s/Statement/Statements/

cee3dc9

Add max connection lifetime param and set consistancy on cassandra se…

5e2cffc

…ssion

Add mysql into the factory

e442917

Fix mysql connections

fa8da4c

Make db instances immutable and add a reset path to tear down and cre…

4d33509

…ate a new database instance with an updated config

briankassouf force-pushed the database-refactor branch from 9457346 to 4d33509 Compare March 3, 2017 22:38

briankassouf and others added 16 commits March 3, 2017 15:07

rename mysql variable

354233f

Update locking functionaility

c823ad0

Remove double lock

1d23bbb

Remove unused sql object

01300e0

Pass statements object

78fdc2a

Add defaults to the cassandra databse type

73200db

Fix renew and revoke calls

cd68899

Update secrets fields

00359cd

Add plugin features

d4ea6c1

Add plugin file

3766ab1

Add special path to enforce root on plugin configuration

b63147b

Rename reset to close

72a878b

Add checksum attribute

a0d207e

Add a way to initalize plugins and builtin databases the same way.

c111b02

Add a metrics middleware

143166b

wrap plugin database type with metrics middleware

a6ae4bd

briankassouf mentioned this pull request Mar 14, 2017

Investigating an Oracle Secret Backend #2357

Closed

briankassouf added 3 commits March 15, 2017 17:14

Work on TLS communication over plugins

5b05f62

Break tls code into helper library

3890f19

Comment and slight refactor of the TLS plugin helper

2ef1cbf

Brian Kassouf added 7 commits May 3, 2017 13:01

Use log to output errors instead of fmt

f424a9a

Use ParseDurationSecond to parse the timeouts in connutil

c381b00

Add the other mysql plugin types with the correct username length set…

657826d

…tings

Fix mysql plugin tests

5b8ce92

Fix parsing the connection duration when it's nil

3ca266b

Fix the TLS functionality in cassandra plugin

3fcf1ad

Update databse backend tests to use the APIClientMeta for the plugin …

a3619c4

…conns

chrishoffman reviewed May 4, 2017

View reviewed changes

Feedback from PR

2af2b85

chrishoffman reviewed May 4, 2017

View reviewed changes

add new mysql plugin names and fix grammar

9e28b03

calvn reviewed May 4, 2017

View reviewed changes

cbandy reviewed May 4, 2017

View reviewed changes

chrishoffman approved these changes May 4, 2017

View reviewed changes

Brian Kassouf added 2 commits May 4, 2017 10:41

PR comments

c825362

Merge remote-tracking branch 'oss/master' into database-refactor

55f1f51

briankassouf mentioned this pull request May 4, 2017

Enhancement: MySQL backend - allow lease information to be tied to a role #2678

Closed

calvn approved these changes May 4, 2017

View reviewed changes

Brian Kassouf and others added 6 commits May 4, 2017 11:45

Update docs and return a better error message

886f873

Don't store the plugin directory prepended command in the barrier, pr…

17bea65

…epend on get

Merge remote-tracking branch 'oss/master' into database-refactor

fcd4f90

update docs

2e82e00

Update mssql docs

65b7bba

Merge branch 'database-refactor' of github.com:hashicorp/vault into d…

3f7ea0d

…atabase-refactor

jefferai approved these changes May 4, 2017

View reviewed changes

Few docs updates

c48b7fa

briankassouf merged commit f4ae1ff into master May 4, 2017

briankassouf deleted the database-refactor branch May 4, 2017 21:08

tomwilkie mentioned this pull request Jan 12, 2018

Plugin Catalog doesn't round trip, therefore doesn't work with terraform. #3776

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Combined Database Backend with Plugins #2200

Combined Database Backend with Plugins #2200

briankassouf commented Dec 20, 2016 •

edited by jefferai

Loading

chrishoffman May 3, 2017

chrishoffman May 3, 2017

chrishoffman May 3, 2017

chrishoffman May 3, 2017

chrishoffman May 4, 2017

chrishoffman May 4, 2017

chrishoffman May 4, 2017

briankassouf May 4, 2017

jefferai May 4, 2017

chrishoffman May 4, 2017

jefferai May 4, 2017

chrishoffman May 4, 2017

chrishoffman May 4, 2017

briankassouf May 4, 2017

chrishoffman May 4, 2017

chrishoffman May 4, 2017

calvn left a comment

calvn May 4, 2017 •

edited

Loading

calvn May 4, 2017

calvn May 4, 2017

calvn May 4, 2017

briankassouf May 4, 2017

cbandy May 4, 2017

jefferai May 4, 2017

chrishoffman left a comment

jefferai left a comment

Combined Database Backend with Plugins #2200

Combined Database Backend with Plugins #2200

Conversation

briankassouf commented Dec 20, 2016 • edited by jefferai Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calvn left a comment

Choose a reason for hiding this comment

calvn May 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrishoffman left a comment

Choose a reason for hiding this comment

jefferai left a comment

Choose a reason for hiding this comment

briankassouf commented Dec 20, 2016 •

edited by jefferai

Loading

calvn May 4, 2017 •

edited

Loading