8000 Determine authentication host based on WWW-Authenticate header by evankanderson · Pull Request #5502 · mindersec/minder · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Determine authentication host based on WWW-Authenticate header #5502

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Mar 13, 2025
Merged

Determine authentication host based on WWW-Authenticate header #5502

merged 4 commits into from
Mar 13, 2025

Conversation

evankanderson
Copy link
Member

Summary

With these changes, I no longer need to set the client-side identity parameters, and I can call Minder as:

./bin/minder --grpc-host localhost --grpc-port 8090 --grpc-insecure auth login

And it will log in with the local keycloak instance. For other production instances, it should be possible to simply use --grpc-host to select them, and not need to explicitly know about their Keycloak configuration. (And, at some point, it could be non-keycloak...)

On the server side, this requires something like the following identity configuration:

identity:
  server:
    issuer_url: http://keycloak:8080 # Use http://localhost:8081 instead for running minder outside of docker-compose
    issuer_claim: http://localhost:8081/realms/stacklok # This needs to match keycloak's configured issuer, not the network path from Minder
    realm: stacklok
    client_id: minder-server
    client_secret: secret
    audience: minder
    scope: minder-audience

Change Type

Mark the type of change your PR introduces:

  • Bug fix (resolves an issue without affecting existing features)
  • Feature (adds new functionality without breaking changes)
  • Breaking change (may impact existing functionalities or require documentation updates)
  • Documentation (updates or additions to documentation)
  • Refactoring or test improvements (no bug fixes or new functionality)

Testing

Manual testing with a number of different configurations. Along the way, I discovered that #5478 actually broke minder auth login for Stacklok / the default configuration, because we weren't setting the mapstructure defaults on the client, only on the server. (There's a fix in here for that).

Review Checklist:

  • Reviewed my own code for quality and clarity.
  • Added comments to complex or tricky code sections.
  • Updated any affected documentation.
  • Included tests that validate the fix or feature.
  • Checked that related changes are merged.

@evankanderson evankanderson requested a review from a team as a code owner March 13, 2025 06:02
gemini-code-assist[bot]
gemini-code-assist bot reviewed Mar 13, 2025
View reviewed changes
Copy link
@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Hello @evankanderson, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

This pull request focuses on improving the authentication process for the Minder CLI by enabling the determination of the authentication host based on the WWW-Authenticate header. This eliminates the need to explicitly set the identity parameters on the client side. The changes include modifications to the CLI commands to leverage the gRPC configuration, fetching the realm URL dynamically, and adjusting the server-side interceptor to provide the WWW-Authenticate header. Additionally, a fix is included to address a regression introduced by a previous PR that broke minder auth login for the default configuration.

Highlights

  • Dynamic Authentication Host Determination: The CLI now determines the authentication host based on the WWW-Authenticate header, simplifying the configuration process.
  • gRPC Configuration Integration: The CLI commands now utilize the gRPC configuration for establishing connections and retrieving tokens.
  • Server-Side Interceptor Enhancement: The server-side interceptor now provides a WWW-Authenticate header hint for authentication, improving the client's ability to discover the authentication realm.
  • Bug Fix: Addresses a regression introduced by Remove hard-coded 'stacklok' realm from minder #5478 that broke minder auth login for the default configuration.

Changelog

Click here to see the changelog
  • cmd/cli/app/auth/auth_token.go
    • Modified the TokenCommand function to use the gRPC configuration for getting the gRPC address and transport credentials.
    • Replaced direct usage of issuerUrl, realm, and clientId with gRPC configuration-based retrieval.
  • cmd/cli/app/auth/offline_token/offline_use.go
    • Modified the offlineUseCommand function to fetch the realm URL dynamically using GetRealmUrl.
    • Updated the RefreshCredentials call to use the fetched realm URL instead of the issuer URL and realm.
  • cmd/cli/app/root.go
    • Added config.SetViperStructDefaults to set default values for the client configuration.
  • internal/controlplane/handlers_token.go
    • Modified TokenValidationInterceptor to be a method on the Server struct.
    • Added logic to send a WWW-Authenticate header in case of an unauthenticated request, providing the realm URL and scope.
    • The interceptor now sends the WWW-Authenticate header to help clients discover the authentication realm.
  • internal/controlplane/server.go
    • Updated the gRPC server to use s.TokenValidationInterceptor instead of the standalone TokenValidationInterceptor function.
  • internal/util/cli/credentials.go
    • Removed unused tls import.
    • Modified GetGrpcConnection to accept GRPCClientConfig instead of individual host, port, and insecure parameters.
    • Modified GetToken to accept the server address and gRPC dial options.
    • Added GetRealmUrl function to retrieve the realm URL from the server's WWW-Authenticate header or fall back to static configuration.
    • Modified RefreshCredentials to accept the realm URL instead of the issuer URL and realm.
    • Added extractWWWAuthenticateRealm function to parse the WWW-Authenticate header and extract the realm.
    • The GetToken function now uses GetRealmUrl to dynamically determine the realm URL.
  • internal/util/cli/credentials_test.go
    • Modified TestGetGrpcConnection to use GRPCClientConfig.
    • Modified TestRefreshCredentials to pass only the issuer URL instead of issuer URL and realm.
  • internal/util/cli/rpc_client.go
    • Modified GrpcForCommand to use GRPCClientConfig in GetGrpcConnection.
    • Modified EnsureCredentials to use GRPCClientConfig in GetToken.
    • Modified Login to use GRPCClientConfig and GetRealmUrl.
  • pkg/config/client/config.go
    • Moved GRPCClientConfig from pkg/config/common.go to pkg/config/client/config.go.
    • Added RegisterGRPCClientConfigFlags to register the gRPC client configuration flags.
    • Added GetGRPCAddress and TransportCredentialsOption methods to GRPCClientConfig.
    • The GRPCClientConfig struct and related functions have been moved and refactored.
  • pkg/config/common.go
    • Removed GRPCClientConfig and RegisterGRPCClientConfigFlags.
  • pkg/config/server/identity.go
    • Added Scope field to IdentityConfig.
    • Added GetRealmURL method to IdentityConfig.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

In realms of code, where tokens roam,
Authentication's the key to home.
A header whispers, a realm's address,
No more hardcoding, less of a mess.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist[bot]
gemini-code-assist bot suggested changes Mar 13, 2025
View reviewed changes
Copy link
@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a significant improvement by dynamically determining the authentication host based on the WWW-Authenticate header. The changes aim to simplify client-side configuration and enhance the overall user experience. The code is generally well-structured, but there are a few areas that could benefit from further attention.

Summary of Findings

  • Missing error check: In internal/util/cli/credentials.go, the conn.Close() call in GetRealmUrl is deferred but the error is not checked. It's good practice to handle potential errors when closing resources.
  • Potential nil pointer dereference: In internal/util/cli/rpc_client.go, the code reads clientConfig.GRPCClientConfig.Host and clientConfig.GRPCClientConfig.Port before checking if clientConfig is nil. This could lead to a nil pointer dereference if the config fails to load.
  • Mixed error handling: In internal/util/cli/rpc_client.go, the Login function returns a loginError type in some cases and a generic error in others. This could make error handling on the client side more complex.
  • TODO comment: In internal/util/cli/rpc_client.go, there's a TODO comment about getting scopes from WWW-Authenticate rather than hardcoding them. This is a good area for future improvement.

Merge Readiness

While the changes in this pull request offer valuable enhancements, there are a few issues that should be addressed before merging. Specifically, the missing error check and potential nil pointer dereference should be resolved to ensure code stability. The mixed error handling in the Login function could also be improved for better client-side error management. I am unable to approve this pull request, and recommend that it not be merged until those issues are addressed (at a minimum), and that others review and approve this code before merging.

@evankanderson
Copy link
Member Author

Let me know if you'd like me to break out the config changes from the other changes... I sort of ended up refactoring a bunch of stuff that had felt awkward, but I can see that it makes a large total set of things to keep in your head.

@evankanderson
Copy link
Member Author

/gemini review

gemini-code-assist[bot]
gemini-code-assist bot previously requested changes Mar 13, 2025
View reviewed changes
Copy link
@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a significant feature by enabling the determination of the authentication host based on the WWW-Authenticate header. This eliminates the need for client-side identity parameters and simplifies the configuration process. The changes are well-structured and include manual testing and a fix for a previously introduced bug. However, there are some areas where the code could be improved, particularly in error handling and code clarity.

Summary of Findings

  • Error handling in GetRealmUrl: The GetRealmUrl function attempts to fetch the realm URL from the server headers but falls back to static configuration if that fails. The error handling in the initial connection attempt could be improved to provide more informative error messages or logging.
  • TODO comment in GetToken: The GetToken function contains a TODO comment indicating that rp.NewRelyingPartyOIDC from zitadel should be used rather than making its own URL. Implementing this would improve the code's maintainability and potentially its security.
  • Inconsistent error handling: There are inconsistencies in how errors are handled and propagated throughout the codebase. Some functions return errors directly, while others use cli.MessageAndError, which can make it harder to track the original error.

Merge Readiness

The pull request is not yet ready for merging. While the core functionality appears to be working correctly, the identified issues related to error handling and code clarity should be addressed to ensure the code's long-term maintainability and robustness. I am unable to approve this pull request, and recommend that the author address the issues surfaced in the review comments before merging. It's also recommended that other reviewers take a look at this code before merging.

@evankanderson evankanderson dismissed gemini-code-assist[bot]’s stale review March 13, 2025 06:22

  • Error handling in GetRealmUrl: we do not want to alarm users when operating with a normal fallback, so additional errors or logging is not appropriate.

  • TODO comment in GetToken: I left the TODO to make it findable in the future, but this PR is big enough as-is.

  • Inconsistent error handling: this is probably a fair complaint, but I don't think I've made things substantially worse, and using cli.MessageAndError is probably appropriate in many of the places it is used.

JAORMX
JAORMX previously approved these changes Mar 13, 2025
View reviewed changes
@coveralls
Copy link

Coverage Status

coverage: 56.561% (-0.7%) from 57.253%
when pulling f812546 on evankanderson:auth-header
into 0964147 on mindersec:main.

@evankanderson evankanderson merged commit f763d73 into mindersec:main Mar 13, 2025
26 checks passed
@evankanderson evankanderson mentioned this pull request Mar 17, 2025
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JAORMX JAORMX JAORMX approved these changes

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@evankanderson @coveralls @JAORMX
0