Support for StreamingCredentials #3068

ggivo · 2024-12-02T16:46:35Z

Description
This PR is preparation step for adding support for Microsoft Entra ID (formerly Azure AD) authentication support

It introduces support for streaming credentials provider allowing connection re-authentication when new credentials are available.
Posible use cases is token based authentication (e.g. expiring tokens) or credential rotation.

PR introduces :

StreamingCredentialsProvider extending existing CredentialsProvider with option for providing a stream of credentials
If enabled connection will subscribe for credential updates and perform re-authentication

An example of credential provider supporting streaming : MyStreamingRedisCredentialsProvider
A basic usage would look like;

        MyStreamingRedisCredentialsProvider credentialsProvider = new MyStreamingRedisCredentialsProvider();
        
        RedisURI uri = RedisURI.builder()
            .withHost("REDIS_HOST")
            .withPort("REDIS_PORT")
            // use credential provider supporting credential stream
            .withAuthentication(credentialsProvider).build();  

        RedisClient client = RedisClient.create(uri);
        client.setOptions(ClientOptions.builder()
                 //enable re-authentication on new credentials
                .reauthenticateBehavior(ClientOptions.ReauthenticateBehavior.ON_NEW_CREDENTIALS)  
                .build());

        connection = client.connect()  
        ....

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 applied code formatting rules using the mvn formatter:format target. Don’t submit any formatting related changes.
You submit test cases (unit or integration tests) that back your changes.

This enables use cases like credential rotation and token based auth without client disconnect. Especially with Pub/Sub clients will reduce the chnance of missing events.

DEFAULT_REAUTHENTICATE_BEHAVIOUR

- Moved Authentication handler to DefaultEndpoint - updated since 6.6.0

# Conflicts: # src/test/java/io/lettuce/core/AuthenticationIntegrationTests.java

Defer the re-auth operation in case there is on-going multi Tx in lettuce need to be externally synchronised when used in multithreaded env. Since re-auth happens from different thread we need to make sure it does not happen while there is ongoing transaction.

…provider with static one provider Add unit tests for setCredentials

atakavci

looks clean from my POV, i got a few questions though.

atakavci · 2024-12-16T07:47:35Z

src/main/java/io/lettuce/core/RedisAuthenticationHandler.java

+        }
+
+        if (credentialsProvider instanceof StreamingCredentialsProvider) {
+            if (!isSupportedConnection()) {


are we good with the behavior when its configured with StreamingCredentials but doesnt work that way. It looks like it happens silently and nobody knows about it.

I missed that one. WIll add a warning to the log.

On second thought, this is expected. We can have a streaming-enabled credentials provider and disabled the reauthentication

src/main/java/io/lettuce/core/RedisAuthenticationHandler.java

atakavci · 2024-12-16T11:51:39Z

src/main/java/io/lettuce/core/StatefulRedisConnectionImpl.java

            multi = (multi == null ? new MultiOutput<>(codec) : multi);

            if (command instanceof CompleteableCommand) {
                ((CompleteableCommand<?>) command).onComplete((ignored, e) -> {
                    if (e != null) {
                        multi = null;
+                        inTransaction.set(false);


what are the cases that we know of it is not a CompleteableCommand ?

Looking at the code you can provide directly Command to dispath.
What is the actual use? I am not sure but if it is not CompletableCommand it will make most of preProcessCommand() method irrelevant.
Not sure if @tishun can add here.

🤷

I thought it was that all sync commands are not completable commands.
But why are they exempt from these checks I am not sure.

Does MULTI even work with sync commands?

src/main/java/io/lettuce/core/StatefulRedisConnectionImpl.java

src/main/java/io/lettuce/core/cluster/RedisClusterClient.java

ggivo

@tishun
Thanks for the review and help with clean up! It looks neat and tidy!

I think there a few changes that lead to change in behavior
Putting some comments and will try to address them with follow up commit.

src/main/java/io/lettuce/core/StatefulRedisConnectionImpl.java

src/main/java/io/lettuce/core/RedisAuthenticationHandler.java

move credentials() method to RedisCredentialsProvider. Resolve issue with unsafe cast after extending RedisCredentialsProvider with supportsStreaming() method

tishun

LGTM

Support for StreamingCredentials

91871b6

This enables use cases like credential rotation and token based auth without client disconnect. Especially with Pub/Sub clients will reduce the chnance of missing events.

ggivo force-pushed the streaming-auth branch from 54f089a to 91871b6 Compare December 2, 2024 18:04

Tests & publish ReauthEvent

e9d4d63

ggivo force-pushed the streaming-auth branch from 8bff958 to e9d4d63 Compare December 3, 2024 23:31

Clean up & Format & Add ReauthenticateEvent test

820ffff

ggivo self-assigned this Dec 4, 2024

ggivo requested a review from tishun December 4, 2024 09:52

ggivo marked this pull request as ready for review December 4, 2024 09:53

ggivo added a commit to ggivo/lettuce that referenced this pull request Dec 4, 2024

Rebase on PR redis#3068

6531424

ggivo added 2 commits December 8, 2024 14:09

Conditionally enable connection reauthentication based on client setting

5858286

DEFAULT_REAUTHENTICATE_BEHAVIOUR

Client setting for enabling reauthentication

779edca

- Moved Authentication handler to DefaultEndpoint - updated since 6.6.0

ggivo force-pushed the streaming-auth branch from 797c75a to f0c35ed Compare December 8, 2024 12:17

formating

21bf696

ggivo force-pushed the streaming-auth branch from f0c35ed to 21bf696 Compare December 8, 2024 12:45

ggivo added a commit to ggivo/lettuce that referenced this pull request Dec 8, 2024

Rebase on PR redis#3068

78fb436

# Conflicts: # src/test/java/io/lettuce/core/AuthenticationIntegrationTests.java

ggivo added 2 commits December 8, 2024 14:56

Merge branch 'main' into streaming-auth

c8ca829

resolve conflict with main

f3aef04

ggivo added a commit to ggivo/lettuce that referenced this pull request Dec 8, 2024

Rebase on PR redis#3068

98e72fb

# Conflicts: # src/test/java/io/lettuce/core/AuthenticationIntegrationTests.java

format

631d420

ggivo added a commit to ggivo/lettuce that referenced this pull request Dec 8, 2024

Rebase on PR redis#3068

b7efe6c

# Conflicts: # src/test/java/io/lettuce/core/AuthenticationIntegrationTests.java

dispath using connection handler

6f46022

ggivo marked this pull request as draft December 11, 2024 12:02

ggivo force-pushed the streaming-auth branch 2 times, most recently from d483898 to 7185e22 Compare December 11, 2024 17:32

ggivo force-pushed the streaming-auth branch from 7185e22 to b32f84c Compare December 11, 2024 17:51

ggivo added 4 commits December 12, 2024 11:10

Fix EndpointId missing in events

7eaaf6b

format

086ccf3

Add unit tests for setCredenatials

61158f2

Skip preProcessing of auth command to avoid replacing the credential …

9a0e513

…provider with static one provider Add unit tests for setCredentials

clean up - remove dead code

6ec2846

ggivo marked this pull request as ready for review December 13, 2024 11:51

ggivo requested a review from atakavci December 13, 2024 12:17

atakavci reviewed Dec 16, 2024

View reviewed changes

Moved almost all code inside the new handler

110eb1a

ggivo commented Dec 17, 2024

View reviewed changes

src/main/java/io/lettuce/core/StatefulRedisConnectionImpl.java Outdated Show resolved Hide resolved

src/main/java/io/lettuce/core/StatefulRedisConnectionImpl.java Outdated Show resolved Hide resolved

src/main/java/io/lettuce/core/RedisAuthenticationHandler.java Show resolved Hide resolved

ggivo added 4 commits December 17, 2024 17:16

fix inTransaction lock with dispatch command batch

8e9ab48

Remove StreamingCredentialsProvider interface.

746dd82

move credentials() method to RedisCredentialsProvider. Resolve issue with unsafe cast after extending RedisCredentialsProvider with supportsStreaming() method

Add authentication handler to ClusterPubSub connections

31341f1

Merge branch 'main' into streaming-auth

6891281

tishun approved these changes Dec 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for StreamingCredentials #3068

Support for StreamingCredentials #3068

ggivo commented Dec 2, 2024 •

edited

Loading

atakavci left a comment

atakavci Dec 16, 2024

ggivo Dec 16, 2024

ggivo Dec 18, 2024

atakavci Dec 16, 2024

ggivo Dec 16, 2024

tishun Dec 18, 2024

ggivo left a comment •

edited

Loading

tishun left a comment

Support for StreamingCredentials #3068

Are you sure you want to change the base?

Support for StreamingCredentials #3068

Conversation

ggivo commented Dec 2, 2024 • edited Loading

atakavci left a comment

Choose a reason for hiding this comment

atakavci Dec 16, 2024

Choose a reason for hiding this comment

ggivo Dec 16, 2024

Choose a reason for hiding this comment

ggivo Dec 18, 2024

Choose a reason for hiding this comment

atakavci Dec 16, 2024

Choose a reason for hiding this comment

ggivo Dec 16, 2024

Choose a reason for hiding this comment

tishun Dec 18, 2024

Choose a reason for hiding this comment

ggivo left a comment • edited Loading

Choose a reason for hiding this comment

tishun left a comment

Choose a reason for hiding this comment

ggivo commented Dec 2, 2024 •

edited

Loading

ggivo left a comment •

edited

Loading