Token based authentication integration with core extension #4011

atakavci · 2024-11-06T22:27:58Z

This PR Provides the required changes to integrate the token based authentication using the core module from redis-authx.

AuthXManager is the main component which handles the id provider creation and management as well as glueing the renewal process to jedis clients internal connection management.

An instance of AuthXManager should be provided to jedis simply via JedisClientConfiguration.
The configuration and creation of AuthXManager can be easily achieved through identity provider-specific configuration builders.
MS EntraID identity provider is already provided within redis-authx.

A basic usage would look like;

    TokenAuthConfig tokenAuthConfig = EntraIDTokenAuthConfigBuilder.builder()
        .clientId("YOUR_CLIENT_ID").secret("YOUR_SECRET")
        .authority("YOUR_AUTHORITY").scopes("SCOPES").build();

    DefaultJedisClientConfig jedisConfig = DefaultJedisClientConfig.builder()
        .authXManager(new AuthXManager(tokenAuthConfig)).build();

    try (JedisPooled jedis = new JedisPooled(hnp, jedisConfig)) {
      ...
      ...
    }

- swith to long dates

-change exception message

- update dependency

sazzad16

It would easier to review if you could avoid formatting-only changes.

pom.xml

- introduce authenticationeventlisteners - clenaup in connectionpool - add entraidtestcontext - add redisintegrationtests - fix failing tokenbasedauthentication unit&integ tests

src/main/java/redis/clients/jedis/Connection.java

src/main/java/redis/clients/jedis/ConnectionFactory.java

src/main/java/redis/clients/jedis/ConnectionPool.java

src/main/java/redis/clients/jedis/authentication/TokenCredentials.java

src/test/java/redis/clients/jedis/authentication/TokenBasedAuthenticationUnitTests.java

src/test/java/redis/clients/jedis/authentication/RedisEntraIDIntegrationTests.java

- fix flaky test

- add cluster+tba tests

- fix flaky tests

- set audiences with scopes - managed identity tests

src/main/java/redis/clients/jedis/authentication/TokenCredentials.java

- use getuser instead oid from Token

src/main/java/redis/clients/jedis/JedisClusterInfoCache.java

src/main/java/redis/clients/jedis/JedisSafeAuthenticator.java

- fix failing unit tests

pom.xml

src/main/java/redis/clients/jedis/Connection.java

tishun · 2024-12-20T14:06:48Z

src/main/java/redis/clients/jedis/JedisPubSubBase.java

@@ -69,11 +76,23 @@ public final void punsubscribe(T... patterns) {
  }

  public final void ping() {
-    sendAndFlushCommand(Command.PING);
+    authenticator.commandSync.lock();


IMHO it would be good to mention why we need to lock here.

As far as I can tell this is because the AUTH command is async and we want to be sure we dont get a concurrency problem when the user and auth threads attempt to write in the same time?

thats correct , i will provide explanation in another iteration

tishun

LGTM

uglide

Endpoints in tests should be fixed.

src/test/java/redis/clients/jedis/authentication/TokenBasedAuthenticationUnitTests.java

uglide · 2024-12-20T14:50:44Z

...java/redis/clients/jedis/authentication/TokenBasedAuthenticationClusterIntegrationTests.java

+        AuthXManager manager = new AuthXManager(EntraIDTokenAuthConfigBuilder.builder()
+                .lowerRefreshBoundMillis(1000).identityProviderConfig(idpConfig).build());
+
+        HostAndPort hp = HostAndPorts.getClusterServers().get(0);


You should use here cluster-entraid-acl endpoint

as long as this endpoint answers to cluster commands it would be fine. Right now the tests can run without entraid enabled endpoints and make the necessary assertions on it. It would be better to keep them running on each commit.

In the integration test, we do not bring up the endpoints hardcoded in the cluster config, so we need to switch to the cluster-entraid-acl to be able to run the tests. Let me know if I'm missing something here.

yes , it doesnt run with entraid enabled env but runs within every integration test in jedis repo

uglide · 2024-12-20T14:51:05Z

...java/redis/clients/jedis/authentication/TokenBasedAuthenticationClusterIntegrationTests.java

+            return result;
+        }).when(authXManager).addConnection(any(Connection.class));
+
+        HostAndPort hp = HostAndPorts.getClusterServers().get(0);


Same as above.

same case with other one

src/test/java/redis/clients/jedis/authentication/TokenBasedAuthenticationIntegrationTests.java

tba draft

6689d65

atakavci marked this pull request as draft November 7, 2024 07:25

atakavci added 4 commits November 8, 2024 00:41

- stop authxmanager on pool close

a18e632

- swith to long dates

drop use of authxmanager and authenticatedconnection from core

392b3b0

-update submodule ref

f037440

-change exception message

- remove submodule

eb63520

- update dependency

atakavci force-pushed the ali/authx2 branch from 1111a2e to eb63520 Compare November 18, 2024 01:24

sazzad16 reviewed Nov 18, 2024

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

atakavci and others added 3 commits November 18, 2024 18:35

Merge branch 'master' into ali/authx2

cbb7781

back to current version

523fe42

- move autxhmanager creation to user space

4e71535

- introduce authenticationeventlisteners - clenaup in connectionpool - add entraidtestcontext - add redisintegrationtests - fix failing tokenbasedauthentication unit&integ tests

sazzad16 requested changes Dec 5, 2024

View reviewed changes

atakavci added 6 commits December 5, 2024 17:26

- prevent use of pubsub with TBA+RESP2 combination

08750e5

- fix flaky test

- support tba with clusters

b1ab1db

- add cluster+tba tests

- remove onerror from authxmanager

1ec5239

- fix flaky tests

- fix flaky test

7d3a0ae

fix renewalDuringOperationsTest

2176505

-reviews from @sazzad16

9ea510d

atakavci requested review from uglide, tishun, sazzad16 and ggivo December 10, 2024 09:16

Merge branch 'master' into ali/authx2

e077031

atakavci marked this pull request as ready for review December 10, 2024 09:16

- fix config for managedIdentity

2175c15

- set audiences with scopes - managed identity tests

ggivo reviewed Dec 11, 2024

View reviewed changes

src/main/java/redis/clients/jedis/authentication/TokenCredentials.java Outdated Show resolved Hide resolved

atakavci added 3 commits December 12, 2024 14:48

review from @ggivo

86cf6f6

- use getuser instead oid from Token

handle and propogate from unsuccessful AUTH response

9717c9a

adding reauth support for both pubsub and shardedpubsub

9185f44

atakavci force-pushed the ali/authx2 branch from cc036c2 to 9185f44 Compare December 14, 2024 23:42

atakavci and others added 2 commits December 15, 2024 04:24

fix ping issue with pubsub

5f8159d

Merge branch 'master' into ali/authx2

9da80c4

sazzad16 reviewed Dec 17, 2024

View reviewed changes

src/main/java/redis/clients/jedis/JedisClusterInfoCache.java Show resolved Hide resolved

sazzad16 reviewed Dec 17, 2024

View reviewed changes

src/main/java/redis/clients/jedis/JedisSafeAuthenticator.java Outdated Show resolved Hide resolved

- review from @sazzad16 : make JedisSafeAuthenticator protected

edf631a

- fix failing unit tests

tishun reviewed Dec 20, 2024

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

tishun reviewed Dec 20, 2024

View reviewed changes

src/main/java/redis/clients/jedis/Connection.java Show resolved Hide resolved

tishun reviewed Dec 20, 2024

View reviewed changes

tishun previously approved these changes Dec 20, 2024

View reviewed changes

update authx version

88a20c2

atakavci dismissed tishun’s stale review via 88a20c2 December 20, 2024 14:49

uglide requested changes Dec 20, 2024

View reviewed changes

- remove workaround for standalone endpoint

93f53a2

uglide approved these changes Dec 20, 2024

View reviewed changes

uglide merged commit d4a569c into redis:master Dec 20, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Token based authentication integration with core extension #4011

Token based authentication integration with core extension #4011

atakavci commented Nov 6, 2024 •

edited

Loading

sazzad16 left a comment

tishun Dec 20, 2024

atakavci Dec 20, 2024

tishun left a comment

uglide left a comment

uglide Dec 20, 2024

atakavci Dec 20, 2024

uglide Dec 20, 2024

atakavci Dec 20, 2024

uglide Dec 20, 2024

atakavci Dec 20, 2024

Token based authentication integration with core extension #4011

Token based authentication integration with core extension #4011

Conversation

atakavci commented Nov 6, 2024 • edited Loading

sazzad16 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tishun left a comment

Choose a reason for hiding this comment

uglide left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atakavci commented Nov 6, 2024 •

edited

Loading