feat(infra): add keycloak as a single central OIDC #1879
Conversation
dosubot
bot
added
the
size:XL
Summary of ChangesHello @jezekra1, 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! This pull request fundamentally re-architects the authentication system of the Agent Stack by integrating Keycloak as the sole OpenID Connect provider. This change centralizes user identity and access management, moving user role definitions and management out of the application's internal database and into Keycloak. The deployment process is enhanced with automated Keycloak provisioning, and all client applications (server, UI, CLI) are updated to seamlessly interact with this new authentication backbone. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Keycloak as a central OIDC provider, significantly refactoring the authentication mechanism across the CLI, server, and UI components. The user management model has been streamlined by delegating user roles and authentication to Keycloak, leading to database schema changes and removal of related code. New Helm charts and provisioning jobs are included for Keycloak deployment, and testing infrastructure has been updated to reflect these changes. The introduction of a deep merge utility function is a positive addition for configuration management.
I am having trouble creating individual review comments. Click here to see my feedback.
agents/chat/src/chat/agent.py (290)
Hardcoding auth=("admin", "admin") directly in the self_registration_client_factory lambda is a security vulnerability. These credentials should be loaded from a secure configuration source (e.g., environment variables, Kubernetes secrets) rather than being hardcoded in the application code. This prevents exposure of sensitive information and allows for easier management and rotation of credentials.
self_registration_client_factory=lambda: PlatformClient(auth=(os.getenv("PLATFORM_ADMIN_USERNAME", "admin"), os.getenv("PLATFORM_ADMIN_PASSWORD", "admin"))),
helm/values.yaml (136)
The apiUrl is hardcoded to "http://localhost:8333". Similar to nextauthUrl, this is a local development URL and must be updated to the public URL of the API for production deployments. Failure to do so will result in connectivity issues for the UI and CLI.
apiUrl: "" # REQUIRED: Public URL of the API (e.g., "https://api.example.com")helm/values.yaml (134)
The nextauthUrl is hardcoded to "http://localhost:8334". This is a local development URL and must be changed for any non-local deployment. It's crucial to emphasize that this value needs to be updated to the public URL of the UI for proper OIDC redirects and functionality in production.
nextauthUrl: "" # REQUIRED: Public URL of the UI (e.g., "https://ui.example.com")helm/values.yaml (119)
The keycloakIssuerUrl is hardcoded to "http://keycloak:8336/realms/agentstack". As noted in the comment, this is an internal URL. For production deployments, this should be a publicly accessible HTTPS URL. It's critical to ensure this is explicitly configured by users deploying to production to avoid insecure communication or connectivity issues.
keycloakIssuerUrl: "" # REQUIRED: Publicly accessible HTTPS URL for Keycloak issuer (e.g., "https://keycloak.example.com/realms/agentstack")helm/templates/deployment.yaml (264-265)
The AUTH__OIDC__INSECURE_TRANSPORT is set based on whether keycloakIssuerUrl starts with http://keycloak. While this handles internal communication, explicitly setting AUTHLIB_INSECURE_TRANSPORT via an environment variable can have broader security implications. It's crucial to ensure that this is only enabled when absolutely necessary and that proper security measures (like mTLS or network policies) are in place for internal HTTP communication.
- name: AUTH__OIDC__INSECURE_TRANSPORT
value: {{ .Values.auth.oidcInsecureTransport | quote }}helm/templates/ui/providers.yaml (12)
The providers.json secret is being created using {{ .Values.auth.providers | toJson }}. However, the auth.providers field was removed from values.yaml in favor of a flattened OIDC configuration. This line will likely cause an error or produce an empty/incorrect secret. The UI should now consume the flattened OIDC environment variables directly, making this secret unnecessary.
apps/agentstack-server/tasks.toml (257-258)
Hardcoding AGENTSTACK__USERNAME=admin and AGENTSTACK__PASSWORD=admin directly in the tasks.toml for e2e tests is a security risk, even for tests. These credentials should be managed more securely, perhaps through environment variables that are set in the CI/CD pipeline or a dedicated test secrets management system, rather than being directly visible in the repository.
export AGENTSTACK__USERNAME=${E2E_TEST_USERNAME:-admin}
export AGENTSTACK__PASSWORD=${E2E_TEST_PASSWORD:-admin}
apps/agentstack-server/tasks.toml (287-292)
Hardcoding seedAgentstackUsers with username: admin and password: admin in the temporary config file for e2e tests is a security concern. While it's for testing, it's a pattern that could be misused. Consider generating random credentials for test users or loading them from a secure test-specific configuration.
seedAgentstackUsers:
- username: ${AGENTSTACK__USERNAME}
password: ${AGENTSTACK__PASSWORD}
firstName: Admin
lastName: User
email: admin@beeai.dev
roles: ["agentstack-admin"]
apps/agentstack-ui/src/modules/auth/components/SignInProviders.tsx (22)
The SignInProviders component now directly uses providers[0].id for auto-sign-in. This assumes there will always be at least one provider and that the first provider in the list is the desired one for auto-sign-in. This might not be flexible enough if multiple providers are configured or if a specific provider needs to be chosen. Consider adding a mechanism to select the default auto-sign-in provider or handle cases with no providers more gracefully.
return <AutoSignIn signIn={handleSignIn.bind(null, { providerId: providers[0]?.id, redirectTo })} />;
helm/templates/deployment.yaml (262-263)
The AUTH__OIDC__EXTERNAL_ISSUER is conditionally set. If keycloakIssuerUrl starts with http://keycloak, it defaults to http://localhost:8336/realms/agentstack. This might be problematic if localhost is not accessible from the UI or if the external issuer needs to be a different public URL. It's better to make external_issuer an explicit configurable value in values.yaml rather than inferring it, especially for production deployments.
- name: AUTH__OIDC__EXTERNAL_ISSUER
value: {{ .Values.auth.keycloakExternalIssuerUrl | default (.Values.auth.keycloakIssuerUrl | quote) }}apps/agentstack-server/src/agentstack_server/configuration.py (96-97)
The client_id and client_secret for the Keycloak server are hardcoded to agentstack-server and agentstack-server-secret. Similar to the issuer URLs, these should be configurable via environment variables or secrets for production deployments to enhance security and flexibility.
client_id: str = os.getenv("AGENTSTACK__AUTH__OIDC__CLIENT_ID", "agentstack-server")
client_secret: Secret[str] = Secret(os.getenv("AGENTSTACK__AUTH__OIDC__CLIENT_SECRET", "agentstack-server-secret"))
docs/development/deploy-agent-stack/deployment-guide.mdx (131)
The keycloakIssuerUrl is set to "http://keycloak:8336/realms/agentstack" in the example. The comment WARNING: if the keycloak URL is internal, the server is configured to allow insecure issuer (http) is important. It would be beneficial to explicitly state that for production deployments, this should be an https URL and ideally not localhost or an internal service name, unless proper mTLS or ingress is configured.
keycloakIssuerUrl: "https://your-keycloak-public-url/realms/agentstack" # Internal URL, change for production
docs/development/deploy-agent-stack/deployment-guide.mdx (134-135)
The nextauthUrl and apiUrl are hardcoded to http://localhost:8334 and http://localhost:8333 respectively in the example. For production deployments, these should be replaced with the actual public URLs of the UI and API. It would be helpful to explicitly mention this in the documentation to guide users on configuring these values correctly for non-local environments.
nextauthUrl: "https://your-ui-public-url" # Public URL of the UI
apiUrl: "https://your-api-public-url" # Public URL of the API
helm/templates/NOTES.txt (31)
The instruction AGENTSTACK__ADMIN_PASSWORD={{.Values.auth.adminPassword}} agentstack model setup still references adminPassword, which has been removed from the auth.basic configuration in values.yaml. This line should be updated to reflect the new authentication mechanism or removed if no longer applicable.
AGENTSTACK__USERNAME={{ .Values.keycloak.auth.adminUser }} AGENTSTACK__PASSWORD=$(kubectl get secret --namespace {{ .Release.Namespace }} keycloak-secret -o jsonpath="{.data.admin-password}" | base64 -d) agentstack model setup
helm/templates/_validators.tpl (48-54)
The validation for auth.basic.adminPassword has been removed. While the basic auth mechanism has changed, if basic auth is still intended to be used (even if only for specific scenarios), it's important to ensure that any required credentials are still validated or that the documentation clearly states how to manage them securely.
helm/templates/config/secret.yaml (48)
The adminPassword field has been removed from the agentstack-secret. This aligns with the shift to Keycloak for user management. Ensure that any components or scripts that previously relied on this secret are updated to use the new Keycloak-based authentication methods.
helm/templates/deployment.yaml (251-252)
The AUTH__BASIC__ENABLED environment variable is set using .Values.auth.basic.enabled. However, the auth.basic.adminPassword validation was removed from _validators.tpl and the field itself from values.yaml. If basic auth is still a supported feature, ensure that its configuration is complete and secure, or remove it entirely if it's no longer intended to be used.
- name: AUTH__BASIC__ENABLED
value: "false"apps/agentstack-ui/src/app/(auth)/token-endpoint.ts (31)
The fallback URL for the token endpoint is hardcoded to include /protocol/openid-connect/token. While this is a common pattern for OIDC, relying on a hardcoded fallback might not be robust enough for all OIDC providers, which can have variations in their endpoint discovery. It's generally better to rely on the discovered metadata or provide a configurable fallback if necessary.
const fallbackUrl = `${issuerUrl.replace(/\/$/, '')}/token`;
apps/agentstack-server/src/agentstack_server/configuration.py (94-95)
The default values for issuer and external_issuer are hardcoded to http://keycloak:8336/realms/agentstack and http://localhost:8336/realms/agentstack respectively. While this is suitable for local development, in a production environment, these URLs would likely need to be configurable via environment variables or other means. It's good practice to make such critical URLs easily adjustable for different deployment scenarios.
issuer: AnyUrl = HttpUrl(os.getenv("AGENTSTACK__AUTH__OIDC__ISSUER", "http://keycloak:8336/realms/agentstack"))
external_issuer: AnyUrl = HttpUrl(os.getenv("AGENTSTACK__AUTH__OIDC__EXTERNAL_ISSUER", "http://localhost:8336/realms/agentstack"))
helm/templates/ui/deployment.yaml (98-100)
Similar to the server deployment, the OIDC_PROVIDER_EXTERNAL_ISSUER is conditionally set based on keycloakIssuerUrl. This inference might not always be correct or desired. It's recommended to make OIDC_PROVIDER_EXTERNAL_ISSUER an explicit configurable value in values.yaml for clarity and flexibility, especially for UI deployments that might interact with Keycloak from different network contexts.
- name: OIDC_PROVIDER_EXTERNAL_ISSUER
value: {{ .Values.auth.keycloakExternalIssuerUrl | default (.Values.auth.keycloakIssuerUrl | quote) }}apps/agentstack-ui/.env.example (27-32)
Hardcoding OIDC_ENABLED=true, OIDC_PROVIDER_NAME=Keycloak, OIDC_PROVIDER_ID=keycloak, OIDC_PROVIDER_CLIENT_ID=agentstack-ui, OIDC_PROVIDER_CLIENT_SECRET=agentstack-ui-secret, and OIDC_PROVIDER_ISSUER=http://localhost:8336/realms/agentstack in .env.example sets specific defaults that might not be suitable for all environments. While it provides a quick start, it's important to ensure that these values are clearly understood as examples and that users are prompted to configure them for their specific deployments, especially OIDC_PROVIDER_CLIENT_SECRET which is sensitive.
OIDC_ENABLED=
OIDC_PROVIDER_NAME=
OIDC_PROVIDER_ID=
OIDC_PROVIDER_CLIENT_ID=
OIDC_PROVIDER_CLIENT_SECRET=
OIDC_PROVIDER_ISSUER=
helm/templates/ui/secret.yaml (9-10)
The authSecret is being generated randomly if empty. While this is a good fallback, it's crucial for NEXTAUTH_SECRET to be a strong, persistent secret in production environments. Generating a new one on every deployment can invalidate sessions. Consider adding a clear warning in values.yaml that this should be explicitly set for production, or implement a mechanism to persist this generated secret.
{{- if (empty $authSecret) }}
{{- fail "NEXTAUTH_SECRET is missing. Please provide a strong secret for production." }}
{{- end }}apps/agentstack-server/src/agentstack_server/api/dependencies.py (91)
Adding a warning log for token validation failures is a good practice for debugging and monitoring. However, the current log message logger.warning(f"Token validation failed: {e}") could benefit from including more context, such as the type of token or the specific provider being used, to make it more actionable.
logger.warning(f"Token validation failed for provider {configuration.auth.oidc.provider.name}: {e}")
helm/values.yaml (123)
The basic.enabled is set to true by default. Given the shift to Keycloak as the central OIDC, basic authentication might be considered less secure or unnecessary for most deployments. Consider defaulting this to false to encourage more secure OIDC-based authentication, or provide clear guidance on when basic auth is appropriate.
enabled: false # Disable basic auth for enhanced securityapps/agentstack-cli/src/agentstack_cli/commands/self.py (160)
The hardcoded URL http://localhost:8336 for opening the web browser should ideally be dynamic, perhaps derived from the active server configuration or a dedicated UI URL setting. This would make the CLI more flexible and adaptable to different deployment environments where the UI might not always be on localhost:8336.
webbrowser.open(configuration.auth_manager.active_server.replace(":8333", ":8336"))
apps/agentstack-cli/src/agentstack_cli/commands/platform/init.py (91)
The print(set_values_list) statement appears to be debug code. It should be removed before merging to avoid unnecessary output in production environments.
values_file_path = None
helm/values.yaml (675)
The keycloak.enabled field is set to true by default. While Keycloak is now an integral part, explicitly enabling it by default might not be desired in all deployment scenarios, especially if an external Keycloak instance is already in use or if authentication is managed differently. Consider if this should be false by default, requiring explicit opt-in.
enabled: false
jezekra1
force-pushed
the
add-central-keycloak-oidc
branch
from
7cf5c98 to
f82e7db
Compare
| issuer: config.external_issuer ?? config.issuer, | ||
| ...(config.external_issuer |
There was a problem hiding this comment.
Is the external issuer a backwards compatibility thing? Much more logic could be simplified if we just assumed Keycloak as the issuer.
There was a problem hiding this comment.
No, this is a hack for localhost:
- nextjs server can access keycloak at http://keycloak:8336 (internal issuer url)
- browser can access keycloak at http://localhost:8336 (external issuer url)
In prod these two URLs should ideally by the same pointing to some https keycloak deployment, I've added warnings everywhere :D
Signed-off-by: Radek Ježek <radek.jezek@ibm.com>
jezekra1
force-pushed
the
add-central-keycloak-oidc
branch
from
f82e7db to
33846f5
Compare
dosubot
bot
added
size:XXL
Summary
Adds keycloak as a central OIDC/auth provider:
Helm changes
Server changes
CLI changes
UI changes
Linked Issues
Documentation
If this PR adds new feature or changes existing. Make sure documentation is adjusted accordingly. If the docs is not needed, please explain why.