Add credential passthrough in base-jdbc connector

[luohao]

I was working on adding credential passthrough in presto-base-jdbc connector and others extend it. Thanks to JdbcIdentity added by @kokosing this is straighforward.

To specify the credential for different jdbc connectors, we need to have a namespace based on catalog name, so I recreate the JdbcConnectorId deleted by @kokosing but name it JdbcCatalogName (as we are replacing connector id with catalog name).

Create this PR to collect feedback on this feature, as well as the implementation.

@electrum @findepi @kokosing

[electrum]

I think there is a simpler and more flexible way. When we designed extra credentials, we intentionally did not namespace them per connector to allow flexibility.

For example, you might have multiple MySQL catalogs for different servers, but they all share the same authentication, so you want to use the same property name.

We can put all the logic for using credentials inside of DriverConnectionFactory. The config could have properties with defaults:

• user-credential-name=${CATALOG}.user
• password-credential-name=${CATALOG}.password

These credential names would be stored with the catalog replaced when DriverConnectionFactory is created. This allows users to use a single name across multiple catalogs if desired. It also keeps the code cleaner as we don't need to modify the creation of JdbcIdentity.

For the config naming, something like credential-name.user would seem more natural, but is wrong from a grammar standpoint.

[kokosing]

I did another experiment (just for fun), where I used extra credentials as variables. That way it is very flexible. For example mysql.properties could look like:

...
connection-url=${mysql.user}
connection-password=${mysql.password}

Then all ${...} are replaced with values from extra credentials. Namespaces are handled here by user in a way they want. To establish connection all variables had to be resolved in my case, however I can imagine other strategies as well (like using default value for ${...} property when extra credential is missing - ${key:-default_value} from bash).

[kokosing]

Also I think, that at some point we need to have support extra credentials in access control. Similar to io.prestosql.security.AccessControl#checkCanSetSystemSessionProperty or io.prestosql.security.AccessControl#checkCanSetCatalogSessionProperty

[electrum]

@kokosing That’s an interesting idea. I like that it’s fledible, but I can’t think of any use cases that it enables over my proposal. Two cocncerns are that it makes error reporting more difficult and replacing part of the JDBC URL with use provided data seems dangerous. That said, please continue thinking along these lines.

[findepi]

replacing part of the JDBC URL with use provided data seems dangerous

This might require some escaping mechanism. However, most of the things (eg user, password) will be set via Properties

[luohao]

Thanks for the feedback! Setting the credential key in config files is brilliant, and indeed simplifies the implementation. I'll update the PR.

I think we should only allow user and password passed from extra credentials, not connection-url, is that right?

[electrum]

This looks good. However, I think we should use name rather than key. I think that name is more common (you normally see property name rather than property key), and key has a different meaning in a security context.

@kokosing and @findepi any other thoughts or concerns?

[kokosing]

It looks good to me.
I think it still needs end-to-end testing. To see that with extra credentials we are capable to connect as different user.

[luohao]

It looks good to me.
I think it still needs end-to-end testing. To see that with extra credentials we are capable to connect as different user.

@kokosing I was thinking about writing production tests for it but couldn't figure out an easy way to set up user/credential in mysql/postgresql container.

Or is there any other unit tests that can achieve the same goal?

[kokosing]

I was thinking about writing production tests

Let's have product tests a last resort option. I would go with "unit" tests. See that MySqlQueryRunner#createMySqlQueryRunner accepts additional connector properties and you sholud be able to set extra credentials in session in AbstractTestQueryFramework#assertQuery. I think that should be enough that you would need.

[luohao]

@electrum @kokosing I updated commits to address your comments. Please take a look.

Thanks!