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

Introduce credentials provider #3224

Merged
merged 21 commits into from Feb 14, 2023
Merged

Introduce credentials provider #3224

merged 21 commits into from Feb 14, 2023

Conversation

@sazzad16 sazzad16 added this to the 4.4.0 milestone Dec 7, 2022
@codecov-commenter
Copy link

codecov-commenter commented Dec 7, 2022

Codecov Report

Base: 66.93% // Head: 66.97% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (a5e3349) compared to base (1d898f1).
Patch coverage: 71.64% of modified lines in pull request are covered.

❗ Current head a5e3349 differs from pull request most recent head 29b9a00. Consider uploading reports for the commit 29b9a00 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3224      +/-   ##
============================================
+ Coverage     66.93%   66.97%   +0.03%     
- Complexity     4641     4652      +11     
============================================
  Files           258      262       +4     
  Lines         15027    15072      +45     
  Branches        938      943       +5     
============================================
+ Hits          10059    10094      +35     
- Misses         4565     4570       +5     
- Partials        403      408       +5     
Impacted Files Coverage Δ
...in/java/redis/clients/jedis/ConnectionFactory.java 67.34% <ø> (ø)
...rc/main/java/redis/clients/jedis/JedisFactory.java 64.15% <ø> (ø)
src/main/java/redis/clients/jedis/Protocol.java 89.50% <0.00%> (-2.37%) ⬇️
...a/redis/clients/jedis/DefaultRedisCredentials.java 41.17% <41.17%> (ø)
...ain/java/redis/clients/jedis/RedisCredentials.java 50.00% <50.00%> (ø)
.../redis/clients/jedis/DefaultJedisClientConfig.java 95.29% <77.77%> (-3.43%) ⬇️
src/main/java/redis/clients/jedis/Connection.java 82.92% <81.25%> (-0.92%) ⬇️
...ain/java/redis/clients/jedis/CommandArguments.java 78.33% <100.00%> (ø)
...clients/jedis/DefaultRedisCredentialsProvider.java 100.00% <100.00%> (ø)
...in/java/redis/clients/jedis/JedisClientConfig.java 93.33% <100.00%> (+1.02%) ⬆️
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

dengliming
dengliming previously approved these changes Dec 14, 2022
@yangbodong22011
Copy link
Collaborator

@sazzad16 Great design, can you add background to this PR to give me a fuller understanding.

@sazzad16
Copy link
Collaborator Author

Copy link
Collaborator

@yangbodong22011 yangbodong22011 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the current code LGTM。
but it seems we could not new single Jedis with credentialsProvider. 🤔

@sazzad16
Copy link
Collaborator Author

@yangbodong22011 We can with JedisClientConfig.

public Jedis(final HostAndPort hostPort, final JedisClientConfig config) {

@yangbodong22011
Copy link
Collaborator

@yangbodong22011 We can with JedisClientConfig.

public Jedis(final HostAndPort hostPort, final JedisClientConfig config) {

ohh, I missed.

@chayim
Copy link
Contributor

chayim commented Jan 25, 2023

@sazzad16 this is a big feature - in line with the redis-py one. WDYT about providing a README or example, as opposed to just tests?

@sazzad16
Copy link
Collaborator Author

this is a big feature - in line with the redis-py one. WDYT about providing a README or example, as opposed to just tests?

@chayim Sure. At first let's make the implementation fixed. Otherwise the example would also play musical chairs along with the implementation.

@sazzad16
Copy link
Collaborator Author

this is a big feature - in line with the redis-py one. WDYT about providing a README or example, as opposed to just tests?

@chayim added an example

@sazzad16 sazzad16 requested review from chayim and a team February 14, 2023 06:14
vladvildanov
vladvildanov previously approved these changes Feb 14, 2023
@sazzad16 sazzad16 merged commit d4644da into redis:master Feb 14, 2023
@sazzad16 sazzad16 deleted the credentials branch February 14, 2023 13:37
kimmking added a commit to kimmking/jedis that referenced this pull request Feb 17, 2023
Introduce credentials provider (redis#3224)
@shiranyo
Copy link

@sazzad16
Thanks for that! Quick question about the behavior when the connection is disconnected.
We want to use it with AWS IAM for Elasticache with cluster mode enabled.

According to AWS documentation (https://docs.aws.amazon.com/AmazonElastiCache/latest/red-ug/auth-iam.html#auth-iam-limits), an IAM authenticated connection will automatically be disconnected after 12 hours ("...to extend the connection for another 12 hours, an AUTH or HELLO command with a new IAM authentication token must be sent").
How does the client handle the disconnection from the server after 12 hours connection using the dynamic credentials provider?
In the context of idle connections in the pool, is the expectation that these idle connections will be broken, and the client will fetch a new password from the credentials provider (credentials.getPassword())?
What is the anticipated behavior for idle connections in the pool in this scenario?
I want to verify that there is a connection renewal with new credentials

@sazzad16
Copy link
Collaborator Author

In the context of idle connections in the pool, is the expectation that these idle connections will be broken, and the client will fetch a new password from the credentials provider (credentials.getPassword())?

@shiranyo Yes.

Note: In such case, enablilng setTest..., i.e. setTestWhileIdle(true) or setTestOnBorrow(true), might be a good idea.

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

Successfully merging this pull request may close these issues.

Security Flaw In How Jedis Stores Server Password
8 participants