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

Options.CredentialsProvider should support Context and returning an error #2681

Closed
maerlyn5 opened this issue Aug 25, 2023 · 5 comments
Closed

Comments

@maerlyn5
Copy link

Expected Behavior

The interface for Options.CredentialsProvider should be func(ctx context.Context) (username string, password string, err error)

baseClient.initConn should return any error from Options.CredentialsProvider

Current Behavior

Options.CredentialsProvider doesn't support context and doesn't pass errors down the call chain.

Context (Environment)

I want to use Options.CredentialsProvider to implement dynamic password fetching from cloud providers. e.g. #2343

Detailed Description

Looking at the original commit #2097, the intent of CredentialsProvider seems to be to memory scrub plaintext passwords, but now people want to use it to dynamically fetch passwords from cloud providers. As the library generally supports Context, i'll skip the explanation for why its idiomatic and appropriate to pass context into code that is intended to make network requests. This would be a breaking change and require a version increment.

Possible Implementation

CredentialsProvider func(ctx context.Context) (username string, password string, err error)

func (c *baseClient) initConn(ctx context.Context, cn *pool.Conn) error {
	if cn.Inited {
		return nil
	}
	cn.Inited = true
	username, password := c.opt.Username, c.opt.Password
	if c.opt.CredentialsProvider != nil {
		var credentialsProviderErr error
		username, password, credentialsProviderErr = c.opt.CredentialsProvider(ctx)
		if credentialsProviderErr != nil {
			return credentialsProviderErr
		}
	}
..
}
@AzureMarker
Copy link

AzureMarker commented May 23, 2024

This has been open for a while, is there any plan to implement this or provide further support for dynamic credentials?
Azure Cache for Redis supports Entra ID auth now, but it's tricky to use this library (the token is the password, and it has an expiration time).

@monkey92t Any inputs?

Related to #2710 and #2343

@monkey92t
Copy link
Collaborator

This has been open for a while, is there any plan to implement this or provide further support for dynamic credentials? Azure Cache for Redis supports Entra ID auth now, but it's tricky to use this library (the token is the password, and it has an expiration time).

@monkey92t Any inputs?

Related to #2710 and #2343

I apologize for not noticing this issue earlier. I believe we can easily resolve this problem by adding a CredentialsProviderContext, which seems to be a simpler and more compatible solution.

CredentialsProvider func() (username string, password string)
CredentialsProviderContext func(ctx context.Context) (username string, password string, err error)

It may seem somewhat redundant, but in other projects, it appears to be a common practice, such as with Dial() and DialContext().

@monkey92t
Copy link
Collaborator

Directly modifying the CredentialsProvider function seems more straightforward, but we must maintain compatibility with go-mod. I can accept CredentialsProvider+CredentialsProviderContext. We can mark their relationship in comments and address this issue in the future.

monkey92t added a commit that referenced this issue May 24, 2024
Signed-off-by: monkey92t <golang@88.com>
@AzureMarker
Copy link

Thanks @monkey92t! Any idea when it will be released?

@AzureMarker
Copy link

AzureMarker commented May 31, 2024

This was released in v9.5.2

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

3 participants