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(auth): add Entra ID identity provider integration for Redis client authentication #2877

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bobymicroby
Copy link
Member

Description

This PR adds Microsoft Entra ID (formerly Azure AD) authentication support to node-redis through:

  1. New @redis/entraid package providing:
  • Multiple auth flows (managed identity, client credentials, auth code with PKCE)
  • MSAL node library integration
  • Auth code PKCE flow sample
  • StreamingCredentialsProvider implementation for EntraID
  1. Core client authentication refactoring:
  • CredentialsProvider interface for async and streaming credentials
  • TokenManager and support classes for automated token lifecycle management
  • Pluggable authentication mechanism via providers

This enables Redis Enterprise customers to authenticate using Entra ID identities. The architecture also allows custom CredentialProvider implementations for third-party Identity Provider integration.

Checklist

[x] npm test passes (new tests added for token management / streaming credential provider )
[ ] New code is mostly tested, except for some pending test coverage in the EntraID package.
[ ] docs are in progress

- Introduce new credential providers: AsyncCredentialsProvider, StreamingCredentialsProvider
- Update client handshake process to use the new CredentialsProviders and to support async credentials fetch / credentials refresh
- Internal conversion of username/password to a CredentialsProvider
- Modify URL parsing to accommodate the new authentication structure
- Tests
packages/entraid/samples/auth-code-pkce/index.ts Dismissed Show dismissed Hide dismissed
packages/entraid/samples/auth-code-pkce/index.ts Dismissed Show dismissed Hide dismissed
@bobymicroby
Copy link
Member Author

Note: I'm considering adding a sync-credentials-provider variant as well:

export interface SyncCredentialsProvider {
  readonly type: 'sync-credentials-provider';
  credentials: () => BasicAuth
}

Would appreciate thoughts on whether this negligible performance optimization is worth the additional API surface area.

Copy link

@dhensby dhensby left a comment

Choose a reason for hiding this comment

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

Looks lovely, and much more thorough and thought-out than my minimal-effort approach in #2849

❤️

Comment on lines 1 to 2
import { TokenManager, TokenManagerConfig, TokenStreamListener, RetryPolicy, IDPError } from './token-manager';
export { TokenManager, TokenManagerConfig, TokenStreamListener, RetryPolicy, IDPError };
Copy link

Choose a reason for hiding this comment

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

Suggested change
import { TokenManager, TokenManagerConfig, TokenStreamListener, RetryPolicy, IDPError } from './token-manager';
export { TokenManager, TokenManagerConfig, TokenStreamListener, RetryPolicy, IDPError };
export { TokenManager, TokenManagerConfig, TokenStreamListener, RetryPolicy, IDPError } from './token-manager';

I believe these import/export calls can be unified - though this may be a style-guide/preference thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dhensby Sure, it makes total sense to unify them.

The idea behind the 'barrel' file initially was to create a proper export so users can import from @redis/client/authx :

"exports": {
    ".": {
      "types": "./dist/index.d.ts",
      "default": "./dist/index.js"
    }, 
    "./authx": {
      "types": "./dist/lib/client/authx/index.d.ts",
      "default": "./dist/lib/client/authx/index.js"
    },

But then I found out that we already have imports in peer packages using paths like @redis/client/lib/ and even @redis/client/dist/ and many others... This can be addressed in one of these ways:

  • Adding more explicit exports (not ideal):
"./index": {
      "types": "./dist/index.d.ts",
      "default": "./dist/index.js"
    },
    "./authx": {
      "types": "./dist/lib/client/authx/index.d.ts",
      "default": "./dist/lib/client/authx/index.js"
    },
    "./lib/commands/generic-transformers": {
      "types": "./dist/lib/commands/generic-transformers.d.ts",
      "default": "./dist/lib/commands/generic-transformers.js"
    },
    "./dist/*": {
      "types": "./dist/*.d.ts",
      "default": "./dist/*.js"
    },
    "./lib/errors": {
      "types": "./dist/lib/errors.d.ts",
      "default": "./dist/lib/errors.js"
    }
  • Not using exports at all, keeping it as is and creating a barrel file just for user convenience
  • Creating a separate submodule containing only the authx support classes

For now, I've decided to stick with not using exports, but I'm considering creating a separate submodule for the authx support classes.

Comment on lines 9 to 15
/**
* Helper function to delay execution for a given number of milliseconds.
* @param ms
*/
const delay = (ms: number) => {
return new Promise(resolve => setTimeout(resolve, ms));
}
Copy link

Choose a reason for hiding this comment

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

async setTimeout is now built into node, there used to be a utility in redis for this which I think was removed in preference of the built-in functionality which is being used extensively in other tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @dhensby! I'll switch to using the built-ins


describe('error scenarios', () => {
it('should not recover if retries are not enabled', async () => {
const clock = new FakeClock(0);
Copy link

Choose a reason for hiding this comment

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

node allows synthetic clocks, so it may be preferred to use them over a custom faked clock?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @dhensby! You're the second node developer to suggest using node's synthetic timers. While I have a slight preference for avoiding global method replacements like setTimeout/Date.now() , you're right - it would be better to align with common practices. I'll switch to using Node's synthetic timers instead of my custom Clock approach.

@@ -0,0 +1,103 @@
import {Disposable} from './types';
Copy link

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

@dhensby makes total sense, i will use the built-in Disposable, thanks for the tip!

@dhensby
Copy link

dhensby commented Dec 6, 2024

Note: I'm considering adding a sync-credentials-provider variant as well:

I don't think there is any need for a sync-credentials-provider. async functions that instantly resolve are treated essentially as sync in the node runtime and if await is used instead of .then() (which it is) non-asynchronous functions could still be provided if the consumer wanted to take that risk (which they shouldn't) and it'll "just work".

@bobymicroby
Copy link
Member Author

Note: I'm considering adding a sync-credentials-provider variant as well:

I don't think there is any need for a sync-credentials-provider. async functions that instantly resolve are treated essentially as sync in the node runtime and if await is used instead of .then() (which it is) non-asynchronous functions could still be provided if the consumer wanted to take that risk (which they shouldn't) and it'll "just work".

@dhensby I agree - I don't think adding a sync-credentials-provider would provide meaningful value. While Node.js optimizes immediately-resolving Promises, shaving off a few nanoseconds (possibly by avoiding the microtask queue) probably isn't worth the additional API surface area. Let's keep the API surface focused and minimal.

Introduces TokenManager and supporting classes to handle token acquisition, automatic
refresh, and updates via identity providers. This foundation enables consistent
authentication token management across different identity provider implementations.

Key additions:
- Add TokenManager to obtain and maintain auth tokens from identity providers
  with automated refresh scheduling based on TTL and configurable thresholds
- Add IdentityProvider interface for token acquisition from auth providers
- Implement Token class for managing token state and TTL tracking
- Include configurable retry mechanism with exponential backoff and jitter
- Add comprehensive test suite covering refresh cycles and error handling

This change establishes the core infrastructure needed for reliable token
lifecycle management across different authentication providers.
Introduces Entra ID (former Azure AD) authentication support with multiple authentication flows
and automated token lifecycle management.

Key additions:
- Add EntraIdCredentialsProvider for handling Entra ID authentication flows
- Implement MSALIdentityProvider to integrate with MSAL/EntraID authentication library
- Add support for multiple authentication methods:
  - Managed identities (system and user-assigned)
  - Client credentials with certificate
  - Client credentials with secret
  - Authorization Code flow with PKCE
- Add factory class with builder methods for each authentication flow
- Include sample Express server implementation for Authorization Code flow
- Add comprehensive configuration options for authority and token management
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.

2 participants