Re-introduce basic auth, generate password if not set#3492
Merged
Conversation
A recent security finding pointed out that the default configuration of nico-core, with the helm chart at its defaults, is very insecure, since there is no authentication on the web frontend. We could easily say this behaves correctly, since "auth=none" should mean no auth, but I think there's a point to be made that the defaults should be secure. Defaulting to oauth2 is untenable, because it requires too many other parameters (client ID, client secret, endpoint, etc), so we can't just have a working "default" oauth2 setup. Instead, this follows prior art from projects like Grafana and ArgoCD, by defaulting to basic auth, and requiring a password to be set. If no password is set in the values.yaml of the helm chart, the chart will create an initial randomly-generated admin password via kubernetes secrets. If oauth2 is specified (like in other gitops deployments that aren't using the helm chart), nothing needs to be done: using oauth does not require an admin password to be set. If the helm chart is not used, and no auth is configured, nico-api will generate one at startup and use it for the lifetime of the process. The password is logged as a warning message, and the basic auth prompt explains to look in the logs for the generated password. This is not expected to be seen in production: the helm chart will generate a secret and configure it. Logging a password would only happen if somebody is running nico-api without the helm chart, without a password env var, and via a deployment that isn't already configured for auth=none or auth=oauth2. While we're at it, make auth a little easier to specify from values.yaml by creating a toplevel values.yaml config for `webAuth.mode` in nico-api, rather than having to use `extraEnv` and configuring it via environment variables.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughWebUI authentication now supports Basic, OAuth2, and none modes. The API adds typed mode handling and middleware, while Helm manages mode precedence, Basic-auth Secrets, deployment variables, tests, examples, and operational documentation. ChangesWebUI authentication
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Helm
participant KubernetesSecret
participant NicoApi
participant WebUIClient
Helm->>KubernetesSecret: Render or reference Basic-auth password
Helm->>NicoApi: Inject authentication mode and password
WebUIClient->>NicoApi: Request /admin
NicoApi->>NicoApi: Apply web_auth_middleware_fn
NicoApi-->>WebUIClient: Return protected response or authentication challenge
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3492.docs.buildwithfern.com/infra-controller |
rwthompsonii
approved these changes
Jul 14, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
helm/README.md (1)
171-195: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAlign the WebUI OAuth2 docs with the Entra/Graph flow.
oauth2mode is Entra-specific in the runtime: it relies on PKCE,User.Read, and MS Graph group lookup. Both docs should stop implying generic Keycloak/Okta support unless a provider-specific integration is added and exercised.
helm/README.md: replace the Okta/Azure AD wording with Entra-specific guidance.book/src/configuration/configurability.md: replace the Keycloak example with the Entra/WebUI configuration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@helm/README.md` around lines 171 - 195, Update the OAuth2 documentation to describe the Entra-specific WebUI flow, including PKCE, User.Read, and Microsoft Graph group lookup; in helm/README.md replace generic Azure AD/Okta wording and retain the Entra configuration example, and in book/src/configuration/configurability.md replace the Keycloak example with matching Entra/WebUI configuration. Do not imply generic provider support.Source: Path instructions
🧹 Nitpick comments (3)
crates/api-web/src/lib.rs (2)
484-488: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecord the authentication variable as a structured tracing field.
Proposed change
tracing::warn!( - "{}: admin web UI has no in-process authentication; restrict access with network policy, a private network, or an authenticating reverse proxy (for example OAuth2 Proxy)", - AUTH_TYPE_ENV + auth_type_env = AUTH_TYPE_ENV, + "admin web UI has no in-process authentication; restrict access with network policy, a private network, or an authenticating reverse proxy (for example OAuth2 Proxy)" );As per coding guidelines, common values must be structured fields rather than interpolated message content.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-web/src/lib.rs` around lines 484 - 488, Update the WebAuthMode::None tracing::warn call to record AUTH_TYPE_ENV as a structured tracing field instead of interpolating it into the message string. Keep the existing warning text and authentication guidance unchanged.Sources: Coding guidelines, Path instructions
908-923: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid cloning the complete authentication configuration on every request.
WebAuth::Basiccopies the password, whileWebAuth::OAuth2deep-clones its boxed layer and maps. Borrow the Basic variant and store the OAuth2 layer in anArc.Proposed direction
- OAuth2(Box<Oauth2Layer>), + OAuth2(Arc<Oauth2Layer>), - let oauth_extension_layer = match req.extensions().get::<WebAuth>().cloned() { + let oauth_extension_layer = match req.extensions().get::<WebAuth>() { ... - Some(WebAuth::OAuth2(layer)) => *layer, + Some(WebAuth::OAuth2(layer)) => Arc::clone(layer), };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/api-web/src/lib.rs` around lines 908 - 923, Update the authentication handling around WebAuth and the oauth_extension_layer match to avoid cloning the full configuration per request: borrow WebAuth::Basic fields when validating credentials, and change WebAuth::OAuth2 to retain its layer through an Arc rather than deep-cloning the boxed layer and maps. Preserve the existing None, Basic authorization, and OAuth2 request behavior while adjusting ownership and dereferencing as needed.helm/charts/nico-api/templates/web-basic-auth-secret.yaml (1)
3-6: 🩺 Stability & Availability | 🔵 TrivialPassword rotation risk if
lookupcan't read Secrets.Reusing the password via
lookupon upgrade only works if the Helm-installing ServiceAccount hasget/listRBAC on Secrets in the target namespace; otherwiselookupsilently returns empty and a brand-new password is generated on everyhelm upgrade, invalidating credentials already handed to operators without any warning. Please confirm the chart's RBAC (or the cluster-operator's role) grants Secret read access wherever this chart is installed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@helm/charts/nico-api/templates/web-basic-auth-secret.yaml` around lines 3 - 6, Ensure the chart’s installing ServiceAccount or cluster-operator role has get/list RBAC permissions for Secrets in the target namespace, so the lookup in the password initialization flow can reuse the existing password across upgrades. Update the chart’s RBAC configuration associated with the web-basic-auth secret without changing the existing password reuse logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/api-web/src/lib.rs`:
- Around line 914-921: Update the Basic-auth branch in the WebAuth middleware to
apply the existing CSRF protection check to unsafe methods before calling
next.run(req).await. Require an accepted Origin/Fetch Metadata signal or CSRF
token, while preserving the current credential validation and unauthorized
response behavior.
In `@dev/mac-local-dev/README.md`:
- Line 240: Update the README environment-variable example by removing the
inline “# local development only” text from the quoted echo value, so the
emitted line remains exactly CARBIDE_WEB_AUTH_TYPE=none. Move the explanatory
note outside the command, such as a Markdown comment above the code block, while
preserving the other echo commands.
In `@helm/charts/nico-api/templates/_helpers.tpl`:
- Around line 87-101: Defer evaluating configuredMode in
nico-api.webAuth.effectiveMode until the final branch that uses it, after
literal and non-literal override checks. Preserve validation and precedence for
literal CARBIDE_WEB_AUTH_TYPE overrides so a valid override does not trigger
configuredMode failure, while still returning configuredMode when no override
applies.
In `@helm/charts/nico-api/templates/deployment.yaml`:
- Around line 143-153: Update the password environment block in the deployment
template to use the same basic-auth Secret rendering condition as
web-basic-auth-secret.yaml, including the unknown mode with configuredMode set
to basic. Keep CARBIDE_WEB_BASIC_AUTH_PASSWORD sourced from
webAuth.basicSecretName and webAuth.basicSecretKey, and preserve the existing
effective basic-mode behavior.
In `@helm/charts/nico-api/templates/web-basic-auth-secret.yaml`:
- Around line 1-18: Replace the hardcoded secret name and password key in
web-basic-auth-secret.yaml with the nico-api.webAuth.basicSecretName and
nico-api.webAuth.basicSecretKey helpers, including the lookup and data access.
In helm/charts/nico-api/templates/NOTES.txt lines 1-7, replace the hardcoded
secret name in the kubectl command with the basicSecretName helper.
- Around line 1-18: Update the generated basic-auth Secret template to use
nico-api.webAuth.basicSecretName for the Secret name and
nico-api.webAuth.basicSecretKey for both lookup and data-key access. Replace the
hardcoded name and password key references while preserving the existing
generation and reuse behavior.
In `@helm/examples/values-full.yaml`:
- Around line 81-94: The extraEnv example is missing the required
CARBIDE_WEB_ALLOWED_ACCESS_GROUPS_ID_LIST variable. Add it alongside
CARBIDE_WEB_ALLOWED_ACCESS_GROUPS, using a realistic deployment-safe
comma-separated group ID value rather than a placeholder that could be deployed
unchanged.
---
Outside diff comments:
In `@helm/README.md`:
- Around line 171-195: Update the OAuth2 documentation to describe the
Entra-specific WebUI flow, including PKCE, User.Read, and Microsoft Graph group
lookup; in helm/README.md replace generic Azure AD/Okta wording and retain the
Entra configuration example, and in book/src/configuration/configurability.md
replace the Keycloak example with matching Entra/WebUI configuration. Do not
imply generic provider support.
---
Nitpick comments:
In `@crates/api-web/src/lib.rs`:
- Around line 484-488: Update the WebAuthMode::None tracing::warn call to record
AUTH_TYPE_ENV as a structured tracing field instead of interpolating it into the
message string. Keep the existing warning text and authentication guidance
unchanged.
- Around line 908-923: Update the authentication handling around WebAuth and the
oauth_extension_layer match to avoid cloning the full configuration per request:
borrow WebAuth::Basic fields when validating credentials, and change
WebAuth::OAuth2 to retain its layer through an Arc rather than deep-cloning the
boxed layer and maps. Preserve the existing None, Basic authorization, and
OAuth2 request behavior while adjusting ownership and dereferencing as needed.
In `@helm/charts/nico-api/templates/web-basic-auth-secret.yaml`:
- Around line 3-6: Ensure the chart’s installing ServiceAccount or
cluster-operator role has get/list RBAC permissions for Secrets in the target
namespace, so the lookup in the password initialization flow can reuse the
existing password across upgrades. Update the chart’s RBAC configuration
associated with the web-basic-auth secret without changing the existing password
reuse logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f0f5678b-7424-408b-87e5-bf9427ecd978
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
book/src/configuration/configurability.mdcrates/api-web/Cargo.tomlcrates/api-web/src/lib.rscrates/api-web/src/tests/mod.rsdeploy/nico-base/api/deployment.yamldev/deployment/devspace/values.base.yamldev/mac-local-dev/README.mddev/mac-local-dev/run-nico-api.shdev/webdev-env/run-env.shdocs/operations/debug_webui.mdhelm/README.mdhelm/charts/nico-api/templates/NOTES.txthelm/charts/nico-api/templates/_helpers.tplhelm/charts/nico-api/templates/deployment.yamlhelm/charts/nico-api/templates/web-basic-auth-secret.yamlhelm/charts/nico-api/tests/web_auth_deployment_test.yamlhelm/charts/nico-api/tests/web_auth_secret_test.yamlhelm/charts/nico-api/values.yamlhelm/examples/values-full.yaml
10 tasks
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
kensimon
enabled auto-merge (squash)
July 14, 2026 20:33
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
helm/charts/nico-api/tests/web_auth_deployment_test.yaml (1)
92-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the built-in mode is absent when the legacy override wins.
This test only checks that an
oauth2entry exists. It would still pass if the chart also rendered the configured/default mode as a duplicateCARBIDE_WEB_AUTH_TYPE. AddnotContainsassertions for the suppressed mode (at leastbasic, and the invalidsamlvalue) so precedence and duplicate suppression are actually verified.As per path instructions, Helm changes should be tested for rendered-template correctness and configuration compatibility.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@helm/charts/nico-api/tests/web_auth_deployment_test.yaml` around lines 92 - 107, The test for legacy CARBIDE_WEB_AUTH_TYPE precedence should also verify suppressed built-in values are not rendered. In the “literal legacy mode takes precedence…” case, add notContains assertions for CARBIDE_WEB_AUTH_TYPE entries with values basic and saml, while preserving the existing oauth2 presence and basic-auth-password absence checks.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@helm/charts/nico-api/templates/NOTES.txt`:
- Line 6: Update the kubectl jsonpath command in the NOTES template to access
the Secret data key using bracket notation, incorporating the value from
nico-api.webAuth.basicSecretKey so keys containing dots such as admin.password
resolve correctly.
---
Nitpick comments:
In `@helm/charts/nico-api/tests/web_auth_deployment_test.yaml`:
- Around line 92-107: The test for legacy CARBIDE_WEB_AUTH_TYPE precedence
should also verify suppressed built-in values are not rendered. In the “literal
legacy mode takes precedence…” case, add notContains assertions for
CARBIDE_WEB_AUTH_TYPE entries with values basic and saml, while preserving the
existing oauth2 presence and basic-auth-password absence checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9a28b1e2-f09a-477a-b0f3-49f2f775ec41
📒 Files selected for processing (11)
crates/api-web/src/auth.rscrates/api-web/src/lib.rscrates/api-web/src/redfish_actions.rscrates/api-web/src/redfish_browser.rshelm/charts/nico-api/templates/NOTES.txthelm/charts/nico-api/templates/_helpers.tplhelm/charts/nico-api/templates/deployment.yamlhelm/charts/nico-api/templates/web-basic-auth-secret.yamlhelm/charts/nico-api/tests/web_auth_deployment_test.yamlhelm/charts/nico-api/tests/web_auth_secret_test.yamlhelm/examples/values-full.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- helm/charts/nico-api/templates/web-basic-auth-secret.yaml
- helm/charts/nico-api/templates/deployment.yaml
- helm/charts/nico-api/tests/web_auth_secret_test.yaml
- helm/examples/values-full.yaml
- helm/charts/nico-api/templates/_helpers.tpl
- crates/api-web/src/lib.rs
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Ken Simon <ken@kensimon.io>
chet
approved these changes
Jul 15, 2026
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
2 tasks
polarweasel
approved these changes
Jul 20, 2026
bryan-aguilar
approved these changes
Jul 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A recent security finding pointed out that the default configuration of nico-core, with the helm chart at its defaults, is very insecure, since there is no authentication on the web frontend. We could easily say this behaves correctly, since "auth=none" should mean no auth, but I think there's a point to be made that the defaults should be secure.
Defaulting to oauth2 is untenable, because it requires too many other parameters (client ID, client secret, endpoint, etc), so we can't just have a working "default" oauth2 setup.
Instead, this follows prior art from projects like Grafana and ArgoCD, by defaulting to basic auth, and requiring a password to be set, with systems for generating a default one. The helm chart will generate an initial randomly-generated admin password via kubernetes secrets, or allow the user to configure a pre-existing secret. If oauth2 is specified (like in other gitops deployments that aren't using the helm chart), nothing needs to be done: using oauth does not require an admin password to be set.
If the helm chart is not used, and no auth is configured, nico-api will generate one at startup and use it for the lifetime of the process. The password is logged as a warning message, and the basic auth prompt explains to look in the logs for the generated password. This is not expected to be seen in production: the helm chart will generate a secret and configure it. Logging a password would only happen if somebody is running nico-api without the helm chart, without a password env var, and via a deployment that isn't already configured for auth=none or auth=oauth2.
The devspace deployment is configured to use webAuth.mode=none, which is an acceptable default for convenience for local development.
While we're at it, make auth a little easier to specify from values.yaml by creating a toplevel values.yaml config for
webAuth.modein nico-api, rather than having to useextraEnvand configuring it via environment variables.Related issues
#3485
Type of Change
Breaking Changes
It's debatable whether this is "breaking" or not... what breaks is the setup where you leave nothing configured and expect to use the web UI without a password. In such cases you will be guided on how to (a) find the password in the logs, if you didn't use the helm chart, or (b) get the secret from kubectl, if you used the helm chart. I'm going to call this "not breaking" but it's a judgement call.
Testing
Additional Notes