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

feat: add credentialProvider option when creating clients #2849

Closed
wants to merge 1 commit into from

Conversation

dhensby
Copy link

@dhensby dhensby commented Oct 7, 2024

Description

In some instances, credentials for the redis client will be short-lived and need to be fetched on-demand when connecting to redis. This is the case when connecting in AWS using IAM authentication or Entra ID in Azure.

This feature allows for a credentialProvider to be provided which is a callable function returning a Promise that resolves to a username/password object.

Closes #821

A potential enhancement to this feature could be to also return a TTL with the credentials and then automatically attempt to re-auth the client before the credentials expire; however I've assumed that logic will be the responsibility of the consumer rather than the library. Worst case scenario is that the socket disconnects due to authentication error and then re-authenticates on a future reconnection retry attempt.

Something else that is missing is an explicit decision on how errors from the credentialProvider should be surfaced.


Checklist

  • Does npm test pass with this change (including linting)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

@dhensby dhensby force-pushed the pulls/credential-providers branch 2 times, most recently from 398642a to 8fcb0d2 Compare October 17, 2024 07:53
@dhensby dhensby force-pushed the pulls/credential-providers branch from 8fcb0d2 to 19bf48c Compare December 4, 2024 21:29
}, {
...GLOBAL.SERVERS.PASSWORD,
clientOptions: {
// simulate a slight pause to fetch the credentials
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does "simulating a pause" make the test better in any way? can't we just Promise.resolve directly?

Copy link
Author

Choose a reason for hiding this comment

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

I found that the order of the resolution of promises could cause a problem if it was just an instantly resolving promise, that edge case wouldn't be covered.

Copy link
Collaborator

@leibale leibale left a comment

Choose a reason for hiding this comment

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

Overall looks good, we might have a different name for it

Thank you very much!

cc @bobymicroby

In some instances, credentials for the redis client will be short-lived and need to be
fetched on-demand when connecting to redis. This is the case when connecting in AWS using
IAM authentication or Entra ID in Azure.

This feature allows for a credentialProvider to be provided which is a callable function
returning a Promise that resolves to a username/password object.
@dhensby dhensby force-pushed the pulls/credential-providers branch from 19bf48c to 1b0ac32 Compare December 5, 2024 20:03
@bobymicroby
Copy link
Member

Hi @dhensby, thank you very much for the PR! We (Redis) are currently working on enabling not only async credential providers, but credential providers that also support rolling credentials/credential invalidation. We need them to enable EntraID (former Azure Active Directory) and more 3rd party identity providers in the future. We have a PR in the works that already covers async credential providers (check the first commit). It is similar to what you already have. We will be very happy to get your feedback.

@dhensby
Copy link
Author

dhensby commented Dec 6, 2024

@bobymicroby great, that's exactly what I'm after (EntraID support), so glad to see that's in the works.

I'll close this as superseded by #2877

@dhensby dhensby closed this Dec 6, 2024
@dhensby dhensby deleted the pulls/credential-providers branch December 6, 2024 09:34
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.

Handle enabling/changing redis password without application restart
4 participants