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

Add support for external user identity / authentication management #1916

Closed
wants to merge 3 commits into from

Conversation

jiantosca
Copy link
Contributor

@jiantosca jiantosca commented Nov 21, 2021

I gave a shot at #1774. Whatcha think?

Make sure that:

  • You have read the contribution guidelines.
  • You have created a feature request first to discuss your contribution intent. Please reference the feature request ticket number in the pull request.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.

@jiantosca jiantosca changed the title Updated authentication to support Supplier<Credentials> #1774 Updated authentication to support Supplier<Credentials> Nov 21, 2021
@mp911de mp911de self-assigned this Nov 22, 2021
@mp911de mp911de added the type: feature A new feature label Nov 22, 2021
@mp911de
Copy link
Collaborator

mp911de commented Nov 22, 2021

Thanks a lot. That looks pretty decent. I need to have a detailed look over the next couple of days.

@@ -113,36 +112,18 @@ protected void setUserNamePassword(List<char[]> args) {
}

if (args.size() > 1) {
setUsername(new String(args.get(0)));
setPassword(args.get(1));
this.credentialsSupplier = new DefaultCredentialsSupplier(new String(args.get(0)), args.get(1));

Choose a reason for hiding this comment

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

In the case which this.credentialsSupplier has a delegate which takes the passwd from another service: after sending an AUTH query to the server, we override the credentialsSupplier that has delegate with a new DefaultCredentialsSupplier which holds only the passwd at the time of the query.
In the case of a passwd rotation, the passwd at the time of the AUTH query will be expired.
I think there is supposed to be a check for existence credentialsSupplier and only if it does not exists, then to set it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's intended if the Redis connection is authenticated manually. In an arrangement where credentials are supplied externally, there's no need to call AUTH from a client application.

Choose a reason for hiding this comment

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

Hi @mp911de, we're looking into adding support for recurring authentication using ephemeral passwords.
This will mean that the client will need to repeatedly reauthenticate, and on each reauthentication attempt receive a fresh password from the credentials supplier.
Overwriting the credentials supplier will mean that this flow will break after the first reauthentication.
I can see 2 option to come over it.

The 1st:
To check if the this.credentialsSupplier is of type DefaultCredentialsSupplier, meaning there is no delegate by the customer.

if (!(credentialsSupplier instanceof DefaultCredentialsSupplier)) {
            return;
}

In order to be sure the customer didn't create a new CredentialsSupplier which inherits from DefaultCredentialsSupplier, we put DefaultCredentialsSupplier as final so no inheritance from it.

The 2nd:
To set this.credentialsSupplier using the DefaultCredentialsSupplier itself from the RedisUri , and then to check if it has delegate or not, if there is delegate, meaning the user/passwd is supplied externally and not supposed be to be overridden by any auth command.

Anyway, I think we should change this function name as it doesn't always change the username/passwd anymore and also not on all cases

Copy link
Collaborator

Choose a reason for hiding this comment

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

When you add or remove a password on the Redis ACL, does that affect connected clients?

Generally as I understand the scenario, you want to call AUTH yourself so there doesn't seem to be need to have a credentials provider at all.

@mp911de mp911de added this to the 6.2.0 milestone Mar 10, 2022
@mp911de mp911de changed the title Updated authentication to support Supplier<Credentials> Add support for external user identity / authentication management Mar 10, 2022
@mp911de
Copy link
Collaborator

mp911de commented Mar 10, 2022

Thanks for the PR. I'm in the process of looking into the PR and I extracted RedisCredentials and RedisCredentialsProvider interfaces. With the input of #1929, we will provide an asynchronous API to fetch credentials and identity providers can supply their own mechanisms to rotate/update credentials upon Redis connect/reconnect.

mp911de pushed a commit that referenced this pull request Mar 16, 2022
mp911de added a commit that referenced this pull request Mar 16, 2022
Refactor code to introduce RedisCredentialsProvider and RedisCredentials interfaces to provide a credentials provider abstraction.

Original pull request: #1916.
@mp911de
Copy link
Collaborator

mp911de commented Mar 16, 2022

Thank you for your contribution. That's merged and polished now.

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

Successfully merging this pull request may close these issues.

Password Rotation support - Add a CredentialProvider interface/class and set your own provider
3 participants