Re-introduce basic auth, generate password if not set#3492
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.
Summary by CodeRabbit
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)crates/api-web/src/auth.rsast-grep retry budget exhausted before isolating this batch crates/api-web/src/lib.rsast-grep retry budget exhausted before isolating this batch crates/api-web/src/redfish_actions.rsast-grep retry budget exhausted before isolating this batch
Comment |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3492.docs.buildwithfern.com/infra-controller |
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
🔍 Container Scan Summary
Per-CVE detail lives in the per-service |
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
|
|
||
| Retrieve the generated password (username: admin) with: | ||
|
|
||
| kubectl -n {{ include "nico-api.namespace" . }} get secret {{ include "nico-api.webAuth.basicSecretName" . }} -o jsonpath='{.data.{{ include "nico-api.webAuth.basicSecretKey" . }}}' | base64 -d |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the chart files and inspect the relevant template/value definitions.
git ls-files 'helm/charts/nico-api/**' | sed -n '1,200p'
printf '\n--- NOTES.txt ---\n'
cat -n helm/charts/nico-api/templates/NOTES.txt
printf '\n--- search for basicSecretKey/basicSecretName ---\n'
rg -n "basicSecretKey|basicSecretName|webAuth" helm/charts/nico-api -n
printf '\n--- values files mentioning secret key/name ---\n'
rg -n "secret|basic" helm/charts/nico-api/values*.yaml helm/charts/nico-api/**/values*.yaml 2>/dev/null || trueRepository: NVIDIA/infra-controller
Length of output: 7884
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the chart template and values definitions around the Secret key.
git ls-files 'helm/charts/nico-api/**' | sed -n '1,200p'
printf '\n--- NOTES.txt ---\n'
cat -n helm/charts/nico-api/templates/NOTES.txt
printf '\n--- related template helpers ---\n'
rg -n "basicSecretKey|basicSecretName|webAuth" helm/charts/nico-api
printf '\n--- values files mentioning secret/key ---\n'
rg -n "secret|basic" helm/charts/nico-api/values*.yaml helm/charts/nico-api/**/values*.yaml 2>/dev/null || trueRepository: NVIDIA/infra-controller
Length of output: 7862
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- web-basic-auth-secret.yaml ---\n'
cat -n helm/charts/nico-api/templates/web-basic-auth-secret.yaml
printf '\n--- values.yaml around webAuth.basic ---\n'
sed -n '155,170p' helm/charts/nico-api/values.yaml
printf '\n--- tests touching basic secret key ---\n'
sed -n '1,120p' helm/charts/nico-api/tests/web_auth_secret_test.yamlRepository: NVIDIA/infra-controller
Length of output: 3662
🌐 Web query:
kubectl jsonpath secret key with dot bracket notation documentation
💡 Result:
To access a key in a Kubernetes secret that contains dots (e.g., a filename used as a key), you cannot use standard dot notation (e.g., {.data.my.key}) because the parser interprets the dots as field separators [1][2]. Instead, you must use bracket notation and escape the dot with a backslash [3][4]. Syntax: kubectl get secret -o jsonpath="{.data['key.with.dots']}" Example: If you have a secret named my-secret with a key named config.json, use: kubectl get secret my-secret -o jsonpath="{.data['config.json']}" Key Technical Details: 1. Bracket Notation: Bracket notation, such as ['key'], allows you to access keys that contain special characters that would otherwise break standard dot-path resolution [3][5]. 2. Escaping: Because kubectl's JSONPath implementation treats the dot as a special character, you must precede it with a backslash () to treat it as a literal character within the bracketed string [3][4]. 3. Alternative (go-template): If you find JSONPath escaping difficult, you can use go-template, which often handles complex key names more intuitively [2][6]: kubectl get secret my-secret -o go-template='{{index.data "config.json"}}' Note on Shell Quoting: When running these commands in a terminal (like bash), ensure you wrap the entire jsonpath expression in single quotes ('...') or double quotes ("...") appropriately to prevent the shell from interpreting characters like brackets or backslashes [3][7]. If using double quotes, you may need to escape internal special characters depending on your shell [3][4].
Citations:
- 1: https://stackoverflow.com/questions/57379081/kubectl-get-specific-value-from-a-secret-in-plaintext
- 2: https://stackoverflow.com/questions/56909180/decoding-kubernetes-secret
- 3: https://v1-30.docs.kubernetes.io/docs/reference/kubectl/jsonpath
- 4: https://github.com/kubernetes/website/blob/master/content/en/docs/reference/kubectl/jsonpath.md
- 5: https://datatracker.ietf.org/doc/rfc9535/
- 6: https://stackoverflow.com/questions/54450943/how-do-i-get-a-kubernetes-secret-file
- 7: https://stackoverflow.com/questions/41527367/kubectl-jsonpath-expression-for-named-path
🌐 Web query:
kubectl jsonpath bracket notation secret key dots escape backslash official docs
💡 Result:
To access a key that contains dots (or other special characters) in a JSONPath expression when using kubectl, you should use bracket notation with the key name enclosed in quotes [1][2]. When using bracket notation, the key name is treated as a literal string, which avoids the issue of the JSONPath parser misinterpreting the dots as nested field separators [1][2]. Syntax Example: kubectl get secret my-secret -o jsonpath="{.data['my.key.name']}" Key details regarding this syntax: 1. Bracket Notation: Using ['key.name'] tells kubectl to treat the content within the brackets as a single, literal key rather than a path sequence [1][2]. 2. Escaping: In modern versions of Kubernetes, you generally do not need to escape the dots when using bracket notation if the key is properly quoted [2]. Previously, users were often forced to use an undocumented backslash escape syntax (e.g., ['my\.key\.name']), but this is typically no longer required for literal string indices in dicts [2]. 3. Quote Management: Be mindful of your shell's quoting requirements. When using double quotes for the entire jsonpath expression (e.g., in a bash command), you may need to use single quotes inside the brackets, or vice versa, to ensure the shell passes the brackets and inner quotes correctly to kubectl [3][4]. If you encounter issues where bracket notation does not behave as expected, consider using go-template, which provides a more robust way to handle map keys: kubectl get secret my-secret -o go-template='{{index.data "my.key.name"}}' [5][6][7]
Citations:
- 1: https://codemia.io/knowledge-hub/path/how_to_access_key_in_a_map_returned_by_kubectl
- 2: jsonpath: Correctly handle string key names in dicts kubernetes/kubernetes#81327
- 3: https://v1-30.docs.kubernetes.io/docs/reference/kubectl/jsonpath
- 4: https://github.com/kubernetes/website/blob/master/content/en/docs/reference/kubectl/jsonpath.md
- 5: Cannot select annotation key using jsonpath kubernetes/kubernetes#31984
- 6: https://stackoverflow.com/questions/57379081/kubectl-get-specific-value-from-a-secret-in-plaintext
- 7: https://stackoverflow.com/questions/56909180/decoding-kubernetes-secret
Use bracket notation for the Secret key in the NOTES command. The current .data.<key> path breaks for valid keys containing dots, such as admin.password. Render the key as jsonpath="{.data['{{ include "nico-api.webAuth.basicSecretKey" . }}']}" instead.
Proposed fix
- kubectl -n {{ include "nico-api.namespace" . }} get secret {{ include "nico-api.webAuth.basicSecretName" . }} -o jsonpath='{.data.{{ include "nico-api.webAuth.basicSecretKey" . }}}' | base64 -d
+ kubectl -n {{ include "nico-api.namespace" . }} get secret {{ include "nico-api.webAuth.basicSecretName" . }} -o jsonpath="{.data['{{ include "nico-api.webAuth.basicSecretKey" . }}']}" | base64 -d📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| kubectl -n {{ include "nico-api.namespace" . }} get secret {{ include "nico-api.webAuth.basicSecretName" . }} -o jsonpath='{.data.{{ include "nico-api.webAuth.basicSecretKey" . }}}' | base64 -d | |
| kubectl -n {{ include "nico-api.namespace" . }} get secret {{ include "nico-api.webAuth.basicSecretName" . }} -o jsonpath="{.data['{{ include "nico-api.webAuth.basicSecretKey" . }}']}" | base64 -d |
🤖 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/NOTES.txt` at 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.
Source: Path instructions
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