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

Datasource and IAM plugin #288

Closed
dron1t opened this issue Dec 19, 2022 · 7 comments
Closed

Datasource and IAM plugin #288

dron1t opened this issue Dec 19, 2022 · 7 comments
Assignees

Comments

@dron1t
Copy link

dron1t commented Dec 19, 2022

Hey I have a question about how to configure Datasource with IAM plugin. Tried using this following configuration as DataSource:

       ds.setJdbcProtocol("jdbc:postgresql:");
       ds.setDatabasePropertyName("databaseName");
       ds.setServerPropertyName("serverName");
       ds.setPortPropertyName("port");
       ds.setUser("app_user");
//        ds.setPassword("pass");


       // Specify the driver-specific data source:
       ds.setTargetDataSourceClassName("org.postgresql.ds.PGSimpleDataSource");


       // Configure the driver-specific data source:
       Properties targetDataSourceProps = new Properties();
       targetDataSourceProps.setProperty("serverName", "x");
       targetDataSourceProps.setProperty("databaseName", "database_name");
       targetDataSourceProps.setProperty("port", "5432");
       targetDataSourceProps.setProperty("currentSchema", "schema");
       targetDataSourceProps.setProperty("ssl", "true");
       targetDataSourceProps.setProperty("sslmode", "require");
       targetDataSourceProps.setProperty(PropertyDefinition.PLUGINS.name, "iam");
       ds.setTargetDataSourceProperties(targetDataSourceProps);

       return ds;

Unfortunately no joy. It generates a token that ends up in properties under password but in a later stage in DefaultConnectionProvider it ends up being removed and while obtaining a connection it tries to use a password instead of a token.

@karenc-bq
Copy link
Contributor

Hi @dron1t, thank you for raising this issue, we will look into it.

@dron1t
Copy link
Author

dron1t commented Dec 20, 2022

Hi,
I was writing this from home pc after hours, Now being on company laptop I can give you a bit more information:
I'm ending up inside software.amazon.jdbc.DataSourceConnectionProvider.java:102
Up until that point copy of properties has a token inside the password. With that PropertyDefinition.removeAll(copy) it gets removed - so in line 103 the dataSource doesn't get that password set.
As seen in the commented-out code, I've tried setting up the password to some dummy value in both data source and target data source properties.

@przemos
Copy link

przemos commented Dec 20, 2022

Indeed, I did observe the same behaviour.
Given javax.sql.Datasource exposes Connection getConnection(String username, String password) throws SQLException; method,
it makes sense to bind DataSourceConnectionProvider against it rather than Connection getConnection() throws SQLException;.
Whether it is Iam or SecretsManager plugin, DataSourceConnectionProvider will always have to initiate the connection on the fly with the current password so there is no alternative to the above.
So I would suggest changing

    PropertyDefinition.removeAll(copy);
    PropertyUtils.applyProperties(this.dataSource, copy);
    return this.dataSource.getConnection();

into (simplified)

    PropertyDefinition.removeAll(copy);
    PropertyUtils.applyProperties(this.dataSource, copy);
    return this.dataSource.getConnection(PropertyDefinition.USER.getString(props), PropertyDefinition.PASSWORD.getString(props));

or conditionally to the above if USER and PASSWORD are present in props

sergiyvamz pushed a commit that referenced this issue Dec 29, 2022
…ies on connect (#292)

### Summary

DataSourceConnectionProvider to not remove user/password properties on
connect
This is to address
#288

### Description

Changed PropertyDefinition removeAll() static method to
removeAllExceptCredentials().
Updated DataSourceConnectionProvider to switch from removeAll() to
removevAllExceptCredentials().
Added unit tests for PropertyDefinition
Added integration test to AuroraMysqlAwsIamIntegrationTest to test this
case.

### Additional Reviewers
@sergiyv-improving
@aaronchung-bitquill
Copy link
Contributor

Hello @dron1t
The fix has been merged. Could you test out our snapshot build (link to snapshot doc) and let us if the issue persists.
Thank you!
(and happy new years! 🎉 )

@dron1t
Copy link
Author

dron1t commented Jan 9, 2023

Hi @aaronchung-bitquill. Hey sorry for the delay, I just came back from the holiday break.
It was tested positively.

@przemos
Copy link

przemos commented Jan 9, 2023

Thank you, @aaronchung-bitquill, great stuff.

@aaronchung-bitquill
Copy link
Contributor

Thank you. We'll be closing this issue. Please feel free reach out again with any other issues you encounter. :)

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

No branches or pull requests

4 participants