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

Modified "Make HikariConfig able to retrieve passwords dynamically from a password supplier" #1335

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

relaxkeren
Copy link

It's a modified PR from https://github.com/brettwooldridge/HikariCP/pull/1196/files. To avoid maintaining password and password supplier both together, this PR makes password setting more generic. Try to keep the simplicity while adding more extensibility for more db settings

@codecov
Copy link

codecov bot commented Mar 7, 2019

Codecov Report

Merging #1335 into dev will increase coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev    #1335      +/-   ##
============================================
+ Coverage     74.65%   74.72%   +0.07%     
- Complexity      545      549       +4     
============================================
  Files            23       23              
  Lines          1929     1935       +6     
  Branches        263      263              
============================================
+ Hits           1440     1446       +6     
  Misses          345      345              
  Partials        144      144              
Impacted Files Coverage Δ Complexity Δ
src/main/java/com/zaxxer/hikari/HikariConfig.java 79.04% <100.00%> (+0.38%) 124.00 <6.00> (+4.00)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d82b6f...d59e117. Read the comment docs.

@miltonhit
Copy link

Perfect! I made a fork with this change and works very well 💯

@simonkoennecke
Copy link

Thanks a lot @relaxkeren for your work! There are two remarks I like to share.

  • jdbc Url shouldn't contain any password. If so the PasswordSupplier hasn't any effect.
  • the username should be provided as well as a supplier function. Please see below for the reason.
package com.zaxxer.hikari.pool;
...
abstract class PoolBase
{
...
 private Connection newConnection() throws Exception
   {
      final long start = currentTime();

      Connection connection = null;
      try {
         String username = config.getUsername(); // returns null on the second call
         String password = config.getPassword();

         connection = (username == null) ? dataSource.getConnection() : dataSource.getConnection(username, password); // Therefore, the new password wouldn't be picked 

...

@brettwooldridge
Copy link
Owner

@simonkoennecke Going back to the comments on #1196, what is insufficient about using the MBean interface to alter the username/password as they user sees fit, on any schedule or by any algorithm they desire?

@uherberg
Copy link

uherberg commented Apr 6, 2020

This will be very useful for us. But why not also do the same for the username? As someone in the original PR mentioned, for cases when an alternate username is used (e.g., alongside AWS SecretsManager), I will also need to switch the username. A CredentialsSupplier for both username and password would be ideal.

@isen-ng
Copy link

isen-ng commented Apr 10, 2020

@brettwooldridge The main difference is that when using IAM Authentication for jdbc connections, the recommended approach is to simply generate a new 15-minute temporary password for every new physical connection. This is a simple and straight forward approach to using a more secure way to authenticate.

On the other hand, using the MXBean approach does not satisfy the simple use-case of using a new temporary password on every new physical connection. Instead, the user (or every user who is interested in this), will need to write plumbing code to have a background thread that wakes up periodically to update the temporary password. This will result in a lot of plumbing code required for all users who want to do this, as well as increase the complexity of their applications. I would argue that most junior developers who attempt to use background threads will trip and fall into the many pitfalls that comes with background threads (eg, they don't catch all exceptions in the background thread, and somehow an exception is thrown and the thread dies and the developer will be left clueless with a production p0 issue).

By allowing this feature, it will be beneficial to developers because all these complexity will no longer be need. Simplicity is the best.

Edit:
Another approach is to, maybe, let developers override some methods in the HikariConfig class. I attempted to circumvent this problem by overriding the getPassword method to generate and return a temporary password. However this does not work because all these overrides are lost when the constructor in HikariDataSource does configuration.copyStateTo(this);.

I reckon that you probably have a very good reason to perform this action, but it limits developers from further extending this project without coming here to post an issue.

@fransonsr
Copy link

At the very least the MXBean should be extended to provide a method to set both the username and password atomically. As mentioned, using the MXBean for dynamic configuration is ... awkward. A CredentialSupplier would be a simple and elegant addition to the API to address these needs.

@gopalgitscm
Copy link

I am exploring Hikari to use in one of my applications. I have a requirement to retrieve password dynamically from password supplier like cyber ark vault. Could you please help me to achieve this using Hikari. I am very new to using Hikari.

@beccagaspard
Copy link

Made some comments about this issue in the other pull request in case it helps anyone!

@lvanderbijl-kry
Copy link

Is this likely to be added any time soon? This appears to still be an unsolved issue 3 years on.

Or does anyone else have a better workaround?

@dalbani
Copy link

dalbani commented Apr 4, 2024

If the use case of the feature is AWS IAM authentication, then I recommend looking at this JDBC driver: https://github.com/awslabs/aws-advanced-jdbc-wrapper.
I haven't investigated though if/how to combine it with HikariCP.

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

Successfully merging this pull request may close these issues.