Allow registering custom CredentialProviders for per-conn passwords #1042

KJTsanaktsidis · 2019-12-15T21:53:38Z

Sorry - this is the second copy of this PR, because go modules is apparently very unhappy about the use of branches with slashes in them...

Description

When using a temporary credential system for MySQL, for example IAM database authenticaiton on AWS or the Database secret backend for Hashicorp Vault, it may not be the case that the same username and password be used for opening every connection in a sql.DB.

This PR adds funcionality whereby the caller can, instead of specifying cfg.User and cfg.Passwd (or in the DSN as user:pass@...), specify a credentialProvider= argument which refers to a callback registered with RegisterCredentialProvider.

When a new connection is to be opened, if the CredentialProvider callback is specified, that is called to obtain a username/password pair rather than using the values from the DSN.

We need this in order to use it with IAM database authentication for Aurora. With IAM database authentication, the "password" you use to connect is in fact a signed token generated with https://docs.aws.amazon.com/sdk-for-go/api/service/rds/rdsutils/#BuildAuthToken. However, the token is signed with an expiry of 15 minutes; so, if a sql.DB is constructed with such a token as the password, the database driver will fail to open any new connections after 15 minutes. By allowing a callback to be used to generate the password, we can re-sign it each time, so that it stays valid.

Although the AWS Aurora usecase only requires changing the password, I suspect some users of the database secrets engine in Hashicorp vault (https://www.vaultproject.io/docs/secrets/databases/mysql-maria.html) will find it useful to be able to change the username as well; Vault works by creating temporary users in the database, so when they expire, vault deletes the actual users out of MySQL and replaces it with one named differently. So this usecase would require being able to change the username in the callback as well as the password.

Anyway, keen to get some 👀 on this and see if people think this would be a useful addition to the library!

Checklist

Code compiles correctly
Created tests which fail without the change (if possible)
All tests passing
Extended the README / documentation, if necessary
Added myself / the copyright holder to the AUTHORS file

catkins · 2019-12-16T02:44:37Z

README.md

+Default:        ""
+```
+
+If set, this must refer to a credential provider name registerd with `RegisterCredentialProvider`. When this is set, the username and password in the DSN will be ignored; instead, each time a conneciton is to be opened, the named credential provider function will be called to obtain a username/password to connect with. This is useful when using, for example, IAM database auth in Amazon AWS, where "passwords" are actually temporary tokens that expire.


small typo: "registerd" -> "registered"

oops. Pushed a fix now.

catkins · 2019-12-16T02:46:46Z

auth.go

+// CredentialProviderFunc is a function which can be used to fetch a username/password
+// pair for use when opening a new MySQL connection. The first return value is the username
+// and the second the password.
+type CredentialProviderFunc func() (string, string, error)


For the purposes of documentation could it be useful to name the return values?

Suggested change

type CredentialProviderFunc func() (string, string, error)

type CredentialProviderFunc func() (user string, password string, err error)

Agreed, this makes the purpose of the return values clearer, I have updated it.

When using a temporary credential system for MySQL, for example IAM database authenticaiton on AWS or the Database secret backend for Hashicorp Vault, it may not be the case that the same username and password be used for opening every connection in a *sql.DB. This PR adds funcionality whereby the caller can, instead of specifying cfg.User and cfg.Passwd (in the DSN as user:pass@...), specify a CredentialProvider= arguemnt which refers to a callback registered with RegisterCredentialProvider. When a new connection is to be opened, if the CredentialProvider callback is specified, that is called to obtain a username/password pair rather than using the values from the DSN.

methane · 2019-12-16T09:01:51Z

I don't like adding DSN option + registry.
Since we have Connector interface now, complex option which can not be representable as simple string can be implemented in Config obj, instead of adding registry.

KJTsanaktsidis · 2019-12-16T09:30:53Z

Oh I totally didn't realise that the Connector interface existed! That's definitely way cleaner. Is the best approach to just put the CredentialProvider callback directly on the config object, and delete all the registry + DSN stuff?

Second question - If I added a commit to this PR (or open a new PR?) to do add a tls.Config to the config object so that we could configure that without the registry too, would that get merged too? (there's already a private tls field, we could just make it public?)

methane · 2019-12-16T11:09:25Z

Is the best approach to just put the CredentialProvider callback directly on the config object, and delete all the registry + DSN stuff?

I don't consider about the best API design carefully yet.
But I think it should be like DB.SetMaxIdleConns(): No public field in Config object. Just add Setter.
And I don't like to add more registries. Please remove it.

Second question - If I added a commit to this PR (or open a new PR?) to do add a tls.Config to the config object so that we could configure that without the registry too, would that get merged too? (there's already a private tls field, we could just make it public?)

It should be separated PR. And it might be (*Config).SetTLSConfig(conf *tls.Config).
But again, I have not decided about design of new APIs yet.
I don't expect we can merge any new feature in this year.

KJTsanaktsidis · 2019-12-16T22:25:05Z

@methane I pushed a commit to get rid of the registry and put the CredentialProvider callback on the Config object as a field.

I had a look at doing what you suggested with a setter instead. However, since all of the other config options are on the config struct as plain fields, and since the config struct is just passed into NewConnector and then not used by the client anymore, it seemed to make more sense to continue with the field thing rather than making a special kind of interface for just one option.

Thinking about this a bit more, a good API for making a Config object using the builder pattern or something like that might be a good idea? However, I think if we implement such an API, it should probably cover all options, not just new ones that get added - e.g. something like

connector, _ := mysql.NewConnector(mysql.NewConfig().
    WithAddress("localhost:3600").
    WithCredentials("username", "password"). // this for creds...
    WithCredentialFunc(callback). // or this (pick one)
    AllowPlainTextPasswords().
    .Config(),
)

What do you think? I'm happy to put together a builder like this for all the fields if that's something you're interested in.

KJTsanaktsidis · 2019-12-16T22:42:41Z

~~(I broke my test that I added to driver_test.go, I need to work out the collation thing)~~ figured it out and pushed a fix.

Instead, it can be used by passing the config object directly to a Connector.

methane · 2019-12-26T10:49:52Z

@KJTsanaktsidis I still feel this proposal is ugly. If we accept this, why not dynamic Hostname and Port? or SSL Cert?

Please look #1045 for my counter proposal. You can modify any config value dynamically.

shogo82148 · 2019-12-26T11:43:06Z

+1 to #1045
I've implemented IAM database authentication driver on AWS https://github.com/shogo82148/rdsmysql

If anything, I want to overwrite TLS configure of existing *mysql.Config.
I need to write mysql.ParseDSN(config.FormatDSN()) to do it now.
https://github.com/shogo82148/rdsmysql/blob/4de22cc423bcf88585a8f3f7f07618500bfae29b/connector.go#L53

KJTsanaktsidis · 2019-12-27T02:35:04Z

Oh, that makes... a ton of sense. I didn't realise it would be OK to use driver.Conn instances from multiple mysql.Connector instances.

I'm on holiday at the moment, but when I get back I'll swap our app over to use an approach like #1045 (unless @catkins beats me to the punch :) ).

Thanks a lot for your help! I'll go ahead and close this PR.

dolmen · 2020-06-01T09:40:09Z

@KJTsanaktsidis You also have the option of writing an alternate database/sql.Driver that wraps this one and does what you want on Connect.

See mine as a reference: it wraps the MySQL driver to load creadentials from the ~/.mylogin.cnf.
See my packages:

KJTsanaktsidis mentioned this pull request Dec 15, 2019

Allow registering custom CredentialProviders for per-conn passwords #1041

Closed

5 tasks

catkins reviewed Dec 16, 2019

View reviewed changes

KJTsanaktsidis force-pushed the ktsanaktsidis_credential_provider branch from 33d6e68 to 5042da9 Compare December 16, 2019 08:58

KJTsanaktsidis force-pushed the ktsanaktsidis_credential_provider branch from 8cfceb7 to e00b62e Compare December 16, 2019 22:42

KJTsanaktsidis force-pushed the ktsanaktsidis_credential_provider branch from e00b62e to 8dad233 Compare December 17, 2019 05:41

Remove registry for CredentialProvider

fc21a98

Instead, it can be used by passing the config object directly to a Connector.

KJTsanaktsidis force-pushed the ktsanaktsidis_credential_provider branch from 8dad233 to fc21a98 Compare December 17, 2019 05:58

methane mentioned this pull request Dec 26, 2019

Create a mysql.Connect API. #1045

Closed

5 tasks

KJTsanaktsidis closed this Dec 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Allow registering custom CredentialProviders for per-conn passwords #1042

Allow registering custom CredentialProviders for per-conn passwords #1042

Uh oh!

KJTsanaktsidis commented Dec 15, 2019

Uh oh!

catkins Dec 16, 2019

Uh oh!

KJTsanaktsidis Dec 16, 2019

Uh oh!

catkins Dec 16, 2019 •

edited

Loading

Uh oh!

KJTsanaktsidis Dec 16, 2019

Uh oh!

methane commented Dec 16, 2019

Uh oh!

KJTsanaktsidis commented Dec 16, 2019

Uh oh!

methane commented Dec 16, 2019

Uh oh!

KJTsanaktsidis commented Dec 16, 2019

Uh oh!

KJTsanaktsidis commented Dec 16, 2019 •

edited

Loading

Uh oh!

methane commented Dec 26, 2019

Uh oh!

shogo82148 commented Dec 26, 2019

Uh oh!

KJTsanaktsidis commented Dec 27, 2019

Uh oh!

dolmen commented Jun 1, 2020

Uh oh!

Uh oh!

	type CredentialProviderFunc func() (string, string, error)
	type CredentialProviderFunc func() (user string, password string, err error)

Allow registering custom CredentialProviders for per-conn passwords #1042

Allow registering custom CredentialProviders for per-conn passwords #1042

Uh oh!

Conversation

KJTsanaktsidis commented Dec 15, 2019

Description

Checklist

Uh oh!

catkins Dec 16, 2019

Choose a reason for hiding this comment

Uh oh!

KJTsanaktsidis Dec 16, 2019

Choose a reason for hiding this comment

Uh oh!

catkins Dec 16, 2019 • 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!

KJTsanaktsidis Dec 16, 2019

Choose a reason for hiding this comment

Uh oh!

methane commented Dec 16, 2019

Uh oh!

KJTsanaktsidis commented Dec 16, 2019

Uh oh!

methane commented Dec 16, 2019

Uh oh!

KJTsanaktsidis commented Dec 16, 2019

Uh oh!

KJTsanaktsidis commented Dec 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

methane commented Dec 26, 2019

Uh oh!

shogo82148 commented Dec 26, 2019

Uh oh!

KJTsanaktsidis commented Dec 27, 2019

Uh oh!

dolmen commented Jun 1, 2020

Uh oh!

Uh oh!

catkins Dec 16, 2019 •

edited

Loading

KJTsanaktsidis commented Dec 16, 2019 •

edited

Loading