Add Entra ID Authentication Support for Redis ( go-redis )
#1
Conversation
ndyakov
force-pushed
the
main
branch
2 times, most recently
from
443b1ef to
dc67db5
Compare
ndyakov
force-pushed
the
intro
branch
3 times, most recently
from
27ddb0a to
c48eb03
Compare
ndyakov
force-pushed
the
intro
branch
3 times, most recently
from
aa4f39c to
3c46773
Compare
There was a problem hiding this comment.
Copilot reviewed 42 out of 44 changed files in this pull request and generated 1 comment.
Files not reviewed (2)
- .githooks/pre-commit: Language not supported
- go.mod: Language not supported
Comments suppressed due to low confidence (1)
.testcoverage.yml:22
- The overall project coverage threshold (85%) is lower than the package threshold (90%). Verify that these values are intended and consistent with your quality goals.
total: 85
| } | ||
| // acquire token using the managed identity client | ||
| // the resource is the URL of the resource that the identity has access to | ||
| authResult, err := m.client.AcquireToken(context.TODO(), resource) |
There was a problem hiding this comment.
[nitpick] Consider using context.Background() instead of context.TODO() if cancellation support is not required for token acquisition. This may improve readability and intent clarity.
| authResult, err := m.client.AcquireToken(context.TODO(), resource) | |
| authResult, err := m.client.AcquireToken(context.Background(), resource) |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
I would prefer to keep it as TODO if we decide to pass the client context in the future.
There was a problem hiding this comment.
Is there a reason we're not passing the client context right now?
| // Use this when you want either a system assigned identity or a user assigned identity. | ||
| // The system assigned identity is automatically managed by Azure and does not require any additional configuration. | ||
| // The user assigned identity is a separate resource that can be managed independently. | ||
| func NewManagedIdentityCredentialsProvider(options ManagedIdentityCredentialsProviderOptions) (auth.StreamingCredentialsProvider, error) { |
There was a problem hiding this comment.
Would it make sense to have these as entraid.credentialsprovider.NewManagedIdentity, entraid.credentialsprovider.NewConfidential and so on...
| package internal | ||
|
|
||
| // IsClosed checks if a channel is closed. | ||
| func IsClosed(ch <-chan struct{}) bool { |
There was a problem hiding this comment.
I understand the utility of this helper function, but the name is misleading, cause it might return true even if the channel is not closed (when the channel is not empty).
There was a problem hiding this comment.
That's correct. Being in the internal package and since we only use the chanel for synchronisation I did not consider that. Any suggestions for a better name for it? I personally don't mind leaving it like this.
There was a problem hiding this comment.
I didn't look into how it's used, but it's usually a readiness signal, so maybe something like IsReady? Again, I don't have the whole context, so feel free to disconsider.
There was a problem hiding this comment.
After some consideration, I would prefer to keep it named as it is. Updated the comment to note that this function returns true no only for closed channels.
Removed unused functionality. Introduced getter for TTL. Improved tests to use defined time.Time.
manager/token_manager.go
Outdated
| // Start starts the token manager and returns a channel that will receive updates. | ||
| Start(listener TokenListener) (CloseFunc, error) | ||
| // Close closes the token manager and releases any resources. | ||
| Close() error |
There was a problem hiding this comment.
the TokenManager interface has both a Close() method and also returns a CloseFunc from Start(). This creates confusion about the proper way to close the token manager.
Having both approaches breaks variance and makes the API less intuitive to use. I recommend either:
- Remove the
Close()method from the interface and only rely on theCloseFuncreturned byStart(), or - Remove the
CloseFuncfrom the return values ofStart()and only use theClose()method.
The first option is preferable as it correctly ties the lifecycle of the token manager.
There was a problem hiding this comment.
nit: the negative of Start should be Stop ; Close should be the negative of Open
| // It is used to get the type of the authentication result, the authentication result itself (can be AuthResult or AccessToken), | ||
| type IdentityProviderResponse interface { | ||
| // Type returns the type of the auth result | ||
| Type() string |
There was a problem hiding this comment.
I reviewed the identity provider response implementation and have a suggestion about the type handling design:
The current IDPResp implementation works but has some potential issues with its type handling:
func (a *IDPResp) AuthResult() public.AuthResult {
if a.authResultVal == nil {
return public.AuthResult{} // Returns empty value, not an error
}
return *a.authResultVal
}This pattern requires callers to remember to check Type() or HasAuthResult() before using returned values. If they forget, they'll get empty structs rather than errors, which could lead to subtle bugs.
type TokenResponse interface {
ExpiresOn() time.Time
ReceivedAt() time.Time
RawToken() string
}
type AuthResultResponse interface {
TokenResponse
GetAuthResult() public.AuthResult
}
type AccessTokenResponse interface {
TokenResponse
GetAccessToken() azcore.AccessToken
}With this approach, the compiler would help enforce correct usage through type assertions:
// Example usage
func ParseResponse(resp TokenResponse) (*Token, error) {
if ar, ok := resp.(AuthResultResponse); ok {
result := ar.GetAuthResult()
// Use auth result...
} else if at, ok := resp.(AccessTokenResponse); ok {
token := at.GetAccessToken()
// Use access token...
} else {
return nil, errors.New("unsupported token type")
}
// ...
}There was a problem hiding this comment.
Thank you for sharing your insight, that is a great comment and suggestions.
-
I agree that the current interfaces could result in users not checking the
Typeand thus result in issues for the developers, since no clear err will be returned. -
Tried to keep anything
tokenrelated outside of theIdentityProviderResponsesince there is a parser later on to parse from IdentityProviderResponse to a Token and right now I am not completely sold on the idea to have getters for theReceivedAtandExpiresOn, but will consider it.
I prepared something that mimics the suggested approach in 22dc6ae , leaving only the Type() in the IdentityProviderInterface and after looking at it, I do think that a more explicit getters, that will return an error, can further improve the design.
There was a problem hiding this comment.
Added errors in f493bda and I think it looks better. Let me know if you agree.
Kindly contributed by @bobymicroby
- Change type names to make more sense (e.g. Start / Stop ) - Add context.Context to the IDP RequestToken - Add RequestTimeout to TokenManagerOptions which will be utilized by the context - Change the LowerRefreshBoundMs from int64 to time.Duration and use better name (dropping the Ms suffix) TODO: - Address changes in the documentation
A more idiomatic approach for Go would be to use time.Duration instead of int representation of Milliseconds.
There was a problem hiding this comment.
Pull Request Overview
This PR adds Entra ID authentication support for go-redis by introducing new token management components and multiple identity provider implementations (managed, confidential, and Azure default) along with extensive testing and CI/CD improvements.
- Adds new authentication flows and error handling for various Entra ID identity types.
- Introduces comprehensive tests with mocks, benchmarks, and updated configuration files for code quality and coverage.
Reviewed Changes
Copilot reviewed 43 out of 45 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| identity/providers.go | Added constants for identity and credential types. |
| identity/managed_identity_provider.go & tests | Implements managed identity provider and tests scenarios. |
| identity/confidential_identity_provider.go & tests | Implements confidential identity provider with various credential options and error handling. |
| identity/azure_default_identity_provider.go & tests | Implements default Azure identity provider with token request logic. |
| identity/authority_configuration.go & tests | Provides authority building logic and covers various authority types. |
| entraid and credentials_provider.go | Provides integration and subscription logic for token updates. |
| CI/CD & configuration files | Updated workflows, test coverage thresholds, and linter configurations for improved quality and performance. |
Files not reviewed (2)
- .githooks/pre-commit: Language not supported
- go.mod: Language not supported
Co-authored-by: Copilot <[email protected]>
IdentityProviderResponse getters will return error in the case where the type is incorrect or the response is not set.
Set default requestTimeout to 30 seconds
Remove Stop method, fix tests.
Simplify the default identity provider response parser by extracting the raw token from the response and then parsing it as jwt token.
Merging this branch changes the coverage (1 decrease, 5 increase)
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
Entra ID Authentication Support for
go-redisDisclaimer: AI generated PR Description
Overview
This PR introduces Entra ID authentication support for
go-redisusing the go-redis-entraid Go library. The implementation provides a robust and flexible authentication system with support for multiple Entra ID identity types, automatic token refresh, and thread-safe token management.Key Features
Implementation Details
Core Authentication Components
Token Management
TokenManagerinterface and implementationIdentity Providers
Credentials Provider
Testing Infrastructure
Development Tools and CI/CD
Documentation
Breaking Changes
None - This is a new feature addition.
Dependencies
Added new dependencies:
Testing
Documentation
Future Considerations
Reviewer Suggestions
Where to Start Reviewing
Core Authentication Components
entraid/credentials_provider.goto understand the main authentication flowmanager/token_manager.gofor token management implementationidentity/package for different identity provider implementationsTesting
entraid/credentials_provider_test.gofor core functionality testsmanager/token_manager_test.gofor token management testsidentity/*_test.gofiles for provider-specific testsConfiguration and Options
identity/providers.gofor available options and constantsmanager/options.gofor token manager configurationinternal/idp_response.gofor response handlingDevelopment Tools
.githooks/pre-commitfor pre-commit hooks.github/workflows/benchmark.ymlfor benchmarking setupgolangci.ymlfor linting configurationDocumentation
README.mdfor project overviewCONTRIBUTING.mdfor development guidelines