Read NVSwitch credentials from Core instead of Vault - #4374
Open
wminckler wants to merge 3 commits into
Open
Conversation
Contributor
Summary by CodeRabbit
WalkthroughThe change adds selector-based switch credential RPCs, moves NV-Switch Manager persistent credential retrieval from Vault to NICo Core, limits in-memory writes to local mode, and removes NSM’s required Vault token while retaining PSM token provisioning. ChangesSwitch credential sourcing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Health as ApiCredentialProvider
participant Manager as CoreCredentialManager
participant Forge as Forge API
participant Store as Credential storage
Health->>Manager: GetNVOS(BMC MAC)
Manager->>Forge: GetSwitchNvosCredentials(BMC MAC)
Forge->>Store: Read SwitchNvosAdmin credential
Store-->>Forge: Username/password
Forge-->>Manager: GetBmcCredentialsResponse
Manager-->>Health: Validated credential
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-07-30 14:09:32 UTC | Commit: 7b0dd83 |
![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: 3
🧹 Nitpick comments (6)
rest-api/nvswitch-manager/pkg/nvswitchmanager/nvswitchmanager_test.go (1)
29-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct lifecycle coverage for the new registration semantics.
These cases seed the backend manually and bypass
NVSwitchManager.Register, so they do not verify in-memory credential seeding or the new delete-retention behavior. Add focused cases that callnm.Registerand then load/delete the tray.Also applies to: 144-148
🤖 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 `@rest-api/nvswitch-manager/pkg/nvswitchmanager/nvswitchmanager_test.go` around lines 29 - 38, Add focused lifecycle tests that call NVSwitchManager.Register directly rather than the seeder helper, then load and delete the registered tray. Verify registration seeds credentials in the in-memory manager and deletion preserves the expected credential retention behavior; keep existing manually seeded tests for other coverage.rest-api/nvswitch-manager/pkg/credentials/credential_manager_test.go (1)
29-35: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftRetain a successful Core manager-selection test.
This case covers only nil-config validation. After removing the non-nil backend assertion, the test no longer proves that
NewdispatchesDatastoreTypeCoretoCoreCredentialManager; that regression could pass unnoticed. Add a success-path case using an injected factory/client or test TLS setup.As per coding guidelines, changed critical paths should retain appropriate test coverage.
🤖 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 `@rest-api/nvswitch-manager/pkg/credentials/credential_manager_test.go` around lines 29 - 35, Add a successful Core datastore case alongside the nil-config validation case in the New manager-selection tests, using the existing injectable factory/client or test TLS setup to construct a valid configuration and avoid external dependencies. Assert that New returns a CoreCredentialManager, preserving explicit coverage of DatastoreTypeCore dispatch.Source: Coding guidelines
crates/api-core/src/handlers/credential.rs (1)
544-562: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument why the MAC branch here needs no inventory gate.
The asymmetry with
get_switch_bmc_credentialsis defensible —SwitchNvosAdminis a namespace exclusive to switch NVOS credentials, so an attacker-supplied MAC can only ever surface a switch NVOS credential — but that reasoning currently lives only in the doc comment of the other handler. A reader auditing this function sees an unvalidated caller-supplied MAC feeding a credential lookup and has no local evidence that it is safe. One line here prevents both a false alarm and a future refactor that generalizes the key.📝 Suggested clarifying comment
// `subject` is the caller's own spelling of the switch, used verbatim in // NotFound errors so the operator sees back what they asked for. + // + // Unlike `get_switch_bmc_credentials`, no switch-inventory gate is needed: + // `SwitchNvosAdmin` is a namespace exclusive to switch NVOS credentials, so + // an arbitrary MAC can only ever resolve to a switch's own credential. let (bmc_mac_address, subject) = match selector {🤖 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-core/src/handlers/credential.rs` around lines 544 - 562, Add a concise comment in the BmcMacAddr branch of the selector handling before the credential lookup, documenting that SwitchNvosAdmin is an NVOS-exclusive credential namespace and therefore does not require an inventory gate for caller-supplied MAC addresses. Keep the existing resolution and lookup behavior unchanged.rest-api/nvswitch-manager/pkg/credentials/credential_manager.go (1)
46-49: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReturning
NewManager()directly yields a typed-nil interface on failure.
NewManagerreturns(*CoreCredentialManager, error). Returning that pair straight through a(CredentialManager, error)signature wraps the nil pointer in a non-nil interface value, so on the cert-missing path a caller testingmgr != nilobserves a live-looking manager and dereferences nil on first use. The neighbouring test already assertsassert.Nil(t, mgr)for error cases, so this path would also read as a confusing test failure the moment it is covered. Assign and branch explicitly.🛡️ Proposed fix
case DatastoreTypeCore: log.Printf("Initializing CredentialManager with NICo Core datastore (config: %s)", config.CoreConfig) - return config.CoreConfig.NewManager() + mgr, err := config.CoreConfig.NewManager() + if err != nil { + return nil, err + } + return mgr, nil🤖 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 `@rest-api/nvswitch-manager/pkg/credentials/credential_manager.go` around lines 46 - 49, Update the DatastoreTypeCore branch in the credential manager factory to capture both results from config.CoreConfig.NewManager(), check the returned error, and return nil with the error when creation fails; only return the manager as the CredentialManager interface after confirming it is non-nil and error-free.crates/api-core/src/tests/credential.rs (1)
327-365: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTable these homogeneous selector cases.
All three invocations differ only in the selector and the expected
Code, which is precisely the shape the repository's table-driven helpers exist for. Extracting avalue_scenarios!/check_casestable (or, at minimum, a local[(selector, expected_code, msg)]slice) removes the copy-paste and makes adding a fourth malformed-input case a one-line change. The neighbouringtest_get_switch_bmc_credentialslegitimately stays standalone, since it interleaves credential seeding between assertions.As per coding guidelines: "Use tables whenever multiple tests call the same operation with different inputs, but keep genuinely distinct tests as standalone tests" and "Use the
carbide-test-supporttable-driven testing helpers for functions mapping inputs to outputs or errors".♻️ Sketch of the tabled form
for (selector, expected, why) in [ (None, Code::InvalidArgument, "missing selector must be rejected"), ( Some(NvosSelector::BmcMacAddr("not-a-mac".to_string())), Code::InvalidArgument, "unparseable MAC must be rejected", ), ( Some(NvosSelector::BmcMacAddr("00:11:22:33:44:55".to_string())), Code::NotFound, "unknown MAC must be NotFound", ), ] { let status = env .api .get_switch_nvos_credentials(tonic::Request::new( rpc::forge::GetSwitchNvosCredentialsRequest { selector }, )) .await .expect_err(why); assert_eq!(status.code(), expected, "{why}"); }🤖 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-core/src/tests/credential.rs` around lines 327 - 365, Refactor test_get_switch_nvos_credentials_rejects_bad_selector to use a local table of selector, expected Code, and failure message values, then iterate through it while invoking the same API and assertions. Preserve the existing InvalidArgument and NotFound expectations and keep the test environment setup outside the loop.Source: Coding guidelines
rest-api/nvswitch-manager/pkg/credentials/core.go (1)
108-127: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a retry policy for these idempotent Core reads.
GetBMCandGetNVOSare single-shot lookups on the hot path, so a transientUNAVAILABLEfrom Core still fails the entire switch load. Add a gRPC service-config retry policy inNewManager;m.timeoutcan remain the overall deadline across attempts.🤖 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 `@rest-api/nvswitch-manager/pkg/credentials/core.go` around lines 108 - 127, Configure a gRPC service-config retry policy in NewManager for the idempotent Core read RPCs used by GetBMC and GetNVOS, retrying transient UNAVAILABLE failures while keeping m.timeout as the overall context deadline across attempts. Leave the request methods’ lookup behavior unchanged.
🤖 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-core/src/auth/internal_rbac_rules.rs`:
- Around line 269-272: Add explicit Flow assertions for GetSwitchNvosCredentials
and GetSwitchBmcCredentials in the existing Flow assertion block, ensuring both
RPCs remain authorized for nico-flow while preserving their current permission
declarations.
In `@helm-prereqs/templates/flow-vault-tokens-job.yaml`:
- Around line 215-223: Update the obsolete-secret cleanup logic around the
nsm-vault-token reference check to fail closed: capture the kubectl query result
and require the query to succeed before allowing deletion, preserving the secret
when discovery fails. Expand reference scanning beyond Deployment
regular-container env secretKeyRefs to include init containers, envFrom, Secret
volumes, and all relevant workload kinds before declaring nsm-vault-token
unreferenced.
In `@rest-api/nvswitch-manager/README.md`:
- Around line 84-86: Add the bash language identifier to the Markdown code
fences around the nvswitch-manager commands, including the additional command
block referenced at lines 109–136, while preserving the command contents.
---
Nitpick comments:
In `@crates/api-core/src/handlers/credential.rs`:
- Around line 544-562: Add a concise comment in the BmcMacAddr branch of the
selector handling before the credential lookup, documenting that SwitchNvosAdmin
is an NVOS-exclusive credential namespace and therefore does not require an
inventory gate for caller-supplied MAC addresses. Keep the existing resolution
and lookup behavior unchanged.
In `@crates/api-core/src/tests/credential.rs`:
- Around line 327-365: Refactor
test_get_switch_nvos_credentials_rejects_bad_selector to use a local table of
selector, expected Code, and failure message values, then iterate through it
while invoking the same API and assertions. Preserve the existing
InvalidArgument and NotFound expectations and keep the test environment setup
outside the loop.
In `@rest-api/nvswitch-manager/pkg/credentials/core.go`:
- Around line 108-127: Configure a gRPC service-config retry policy in
NewManager for the idempotent Core read RPCs used by GetBMC and GetNVOS,
retrying transient UNAVAILABLE failures while keeping m.timeout as the overall
context deadline across attempts. Leave the request methods’ lookup behavior
unchanged.
In `@rest-api/nvswitch-manager/pkg/credentials/credential_manager_test.go`:
- Around line 29-35: Add a successful Core datastore case alongside the
nil-config validation case in the New manager-selection tests, using the
existing injectable factory/client or test TLS setup to construct a valid
configuration and avoid external dependencies. Assert that New returns a
CoreCredentialManager, preserving explicit coverage of DatastoreTypeCore
dispatch.
In `@rest-api/nvswitch-manager/pkg/credentials/credential_manager.go`:
- Around line 46-49: Update the DatastoreTypeCore branch in the credential
manager factory to capture both results from config.CoreConfig.NewManager(),
check the returned error, and return nil with the error when creation fails;
only return the manager as the CredentialManager interface after confirming it
is non-nil and error-free.
In `@rest-api/nvswitch-manager/pkg/nvswitchmanager/nvswitchmanager_test.go`:
- Around line 29-38: Add focused lifecycle tests that call
NVSwitchManager.Register directly rather than the seeder helper, then load and
delete the registered tray. Verify registration seeds credentials in the
in-memory manager and deletion preserves the expected credential retention
behavior; keep existing manually seeded tests for other coverage.
🪄 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: b4981855-290a-4d25-a521-e9c67483f6c3
⛔ Files ignored due to path filters (2)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.gorest-api/proto/core/gen/v1/nico_nico_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go,!rest-api/**/*_grpc.pb.go
📒 Files selected for processing (31)
crates/api-core/src/api.rscrates/api-core/src/auth/internal_rbac_rules.rscrates/api-core/src/handlers/credential.rscrates/api-core/src/tests/credential.rscrates/health/src/api_client.rscrates/rpc/build.rscrates/rpc/proto/forge.protohelm-prereqs/health-check.shhelm-prereqs/setup.shhelm-prereqs/templates/flow-vault-tokens-job.yamlhelm/charts/nico-flow/templates/deployment.yamlhelm/charts/nico-flow/values.yamlrest-api/nvswitch-manager/README.mdrest-api/nvswitch-manager/cmd/serve.gorest-api/nvswitch-manager/docker-compose.ymlrest-api/nvswitch-manager/internal/service/config.gorest-api/nvswitch-manager/internal/service/server_impl.gorest-api/nvswitch-manager/local-dev.shrest-api/nvswitch-manager/pkg/credentials/config.gorest-api/nvswitch-manager/pkg/credentials/config_test.gorest-api/nvswitch-manager/pkg/credentials/core.gorest-api/nvswitch-manager/pkg/credentials/core_test.gorest-api/nvswitch-manager/pkg/credentials/credential_manager.gorest-api/nvswitch-manager/pkg/credentials/credential_manager_test.gorest-api/nvswitch-manager/pkg/credentials/inmemory.gorest-api/nvswitch-manager/pkg/credentials/inmemory_test.gorest-api/nvswitch-manager/pkg/credentials/vault.gorest-api/nvswitch-manager/pkg/credentials/vault_test.gorest-api/nvswitch-manager/pkg/nvswitchmanager/nvswitchmanager.gorest-api/nvswitch-manager/pkg/nvswitchmanager/nvswitchmanager_test.gorest-api/proto/core/src/v1/nico_nico.proto
💤 Files with no reviewable changes (5)
- rest-api/nvswitch-manager/local-dev.sh
- rest-api/nvswitch-manager/docker-compose.yml
- rest-api/nvswitch-manager/pkg/credentials/vault_test.go
- rest-api/nvswitch-manager/pkg/credentials/inmemory_test.go
- rest-api/nvswitch-manager/pkg/credentials/vault.go
Comment on lines
+215
to
+223
| if kubectl get deployments -n {{ .Values.flow.namespace }} \ | ||
| -o jsonpath='{.items[*].spec.template.spec.containers[*].env[*].valueFrom.secretKeyRef.name}' 2>/dev/null \ | ||
| | tr ' ' '\n' | grep -qx nsm-vault-token; then | ||
| echo "nsm-vault-token still referenced by a Deployment in {{ .Values.flow.namespace }}; leaving it in place" | ||
| else | ||
| kubectl delete secret nsm-vault-token \ | ||
| -n {{ .Values.flow.namespace }} \ | ||
| --from-literal=token="$value" \ | ||
| --dry-run=client -o yaml | kubectl apply -f - | ||
| done | ||
| echo "psm-vault-token and nsm-vault-token written to {{ .Values.flow.namespace }} namespace" | ||
| --ignore-not-found | ||
| echo "retired nsm-vault-token removed from {{ .Values.flow.namespace }} namespace (if present)" |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make obsolete-Secret cleanup fail closed and scan all references.
If kubectl get deployments fails, its error is suppressed and grep returns no match, so this deletes a potentially live token. Capture and require a successful query before deletion; also include init containers, envFrom, Secret volumes, and other workload kinds before declaring the Secret unreferenced.
🤖 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-prereqs/templates/flow-vault-tokens-job.yaml` around lines 215 - 223,
Update the obsolete-secret cleanup logic around the nsm-vault-token reference
check to fail closed: capture the kubectl query result and require the query to
succeed before allowing deletion, preserving the secret when discovery fails.
Expand reference scanning beyond Deployment regular-container env secretKeyRefs
to include init containers, envFrom, Secret volumes, and all relevant workload
kinds before declaring nsm-vault-token unreferenced.
Comment on lines
+84
to
86
| ``` | ||
| ./nvswitch-manager serve -d InMemory | ||
| ``` |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add bash language identifiers to the command fences.
This resolves MD040 and enables shell-aware rendering.
Also applies to: 109-136
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 84-84: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@rest-api/nvswitch-manager/README.md` around lines 84 - 86, Add the bash
language identifier to the Markdown code fences around the nvswitch-manager
commands, including the additional command block referenced at lines 109–136,
while preserving the command contents.
Source: Linters/SAST tools
thossain-nv
added
the
rest-api
ChatGPT Codex Connector
NVSwitch OS credentials lived in Vault KV, written and read by NSM's own
Vault client. Core already stores per-component credentials envelope-
encrypted in Postgres under the same key namespace
(CredentialKey::SwitchNvosAdmin -> switch_nvos/{MAC}/admin), and already
rotates them per switch from the switch controller. The gap was only that
NSM talked to Vault directly.
NSM now reads both NVOS and switch-BMC credentials from Core over mTLS
gRPC and never writes them: Core seeds them from the expected-switch
record and owns rotation, so an NSM write would race it. Registration
records identity and routing only, and deregistration leaves credentials
alone since a switch removed from NSM may still be live in Core. No
caching, so a rotated password is picked up on the next lookup.
Core side:
- GetSwitchNvosCredentialsRequest gains a `oneof selector` so callers
holding only a BMC MAC (NSM has no SwitchId) can read; field 1 is
unchanged, so existing callers are wire-compatible.
- New GetSwitchBmcCredentials. Not routed through GetBmcCredentials:
that path resolves a BMC IP from machine_interface and mints a
Redfish session token, neither of which applies to switch BMCs.
It gates on the switch inventory first -- BmcRoot is one namespace
shared with compute trays and power shelves, keyed only by MAC, so
without the gate a caller authorized for switches could read any
machine's root password.
- Malformed MACs report InvalidArgument rather than falling through to
the catch-all Internal mapping.
Deployment:
- The nsm container drops VAULT_ADDR/VAULT_TOKEN for NICO_CORE_API_URL,
and nsm-vault-token is retired from the prereqs job, setup.sh and
health-check.sh. The obsolete Secret is deleted on upgrade; its
policy is left in place so an unordered prereqs upgrade cannot strip
capabilities from a token still in use.
- NSM_DATASTORE=Persistent is now set explicitly. The image entrypoint
is `nsm serve` with no arguments and --datastore was the one flag
without an env fallback, so the deployed NSM had been running
in-memory with its Postgres registry and Vault token both unused.
No credential migration is needed: these paths are under
CredentialPrefix::all(), so Core's existing Vault->Postgres import
already covers them, and NSM's paths were byte-identical including the
uppercase-MAC convention.
Fixes NVIDIA#1852
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… entry "Read-only" applies to the NICo Core backend; the in-memory store is still seeded at registration for local and test use, so say which is which rather than leaving the two modes described by one word. Also stop suggesting `export DB_PASSWORD=...`, which lands the password in shell history -- prompt for it with `read -rs` instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…build Two CI failures on the credential migration. `buf breaking` rejects moving switch_id into a oneof (FIELD_SAME_ONEOF) even though the wire encoding is unchanged, so GetSwitchNvosCredentialsRequest goes back to two fields. The exclusivity the oneof gave for free is now enforced in the handler, which rejects both-set as well as neither-set rather than picking a winner and silently ignoring half the request; both cases are tested. The nico-nsm image build could not see the generated Core gRPC client, because the Dockerfile copies only common/ and nvswitch-manager/ into the build context. `go build ./...` misses this since it compiles the whole module tree. The local variant was missing common/ as well, so it could not have built even before this change. Also tightens the in-memory credential path, which is dev and test only but was asymmetric: Register seeded before the registry accepted the switch, leaving credentials behind on a rejected registration, and Delete never dropped what Register had seeded, so a re-registered MAC would inherit stale credentials. Register now seeds only after the registry accepts, and Delete forgets what it seeded. Core-owned credentials are still never touched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
wminckler
force-pushed
the
nvos-creds-postgres-1852
branch
from
July 30, 2026 15:08
7b0dd83 to
1667c54
Compare
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4374.docs.buildwithfern.com/infra-controller |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/api-core/src/tests/credential.rs (1)
328-386: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConvert to a
scenarios!table instead of four sequential assertions.Each case calls the same RPC with a differently-shaped request and checks only the resulting
tonic::Code— a canonicalcarbide-test-support::scenarios!shape (mapping inputs to aResult/error outcome).As per coding guidelines: "Use the carbide-test-support table-driven testing helpers for functions mapping inputs to outputs or errors; use scenarios! for Result, value_scenarios! for total operations, and direct check helpers when macros obscure intent."
🤖 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-core/src/tests/credential.rs` around lines 328 - 386, Convert test_get_switch_nvos_credentials_rejects_bad_selector into a carbide-test-support scenarios! table covering the four request shapes: missing selectors, both selectors, malformed MAC, and unknown valid MAC. Have each scenario invoke get_switch_nvos_credentials and assert only the resulting tonic::Code, preserving InvalidArgument for the first three cases and NotFound for the final case.Source: Coding guidelines
🤖 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-prereqs/templates/flow-vault-tokens-job.yaml`:
- Around line 50-53: Move the secrets deletion rule for resourceNames
“nsm-vault-token” out of the cluster-scoped RBAC and into a namespaced Role
targeting {{ .Values.flow.namespace }}. Bind the hook ServiceAccount to that
Role in the same namespace, while retaining only namespace-operation permissions
in the ClusterRole and its ClusterRoleBinding.
In `@rest-api/nvswitch-manager/README.md`:
- Around line 109-114: Update the credential input commands in the README so the
DB_USER and DB_PASSWORD reads display explicit prompts, and restore the terminal
newline after the silent password read while preserving the existing exports.
---
Nitpick comments:
In `@crates/api-core/src/tests/credential.rs`:
- Around line 328-386: Convert
test_get_switch_nvos_credentials_rejects_bad_selector into a
carbide-test-support scenarios! table covering the four request shapes: missing
selectors, both selectors, malformed MAC, and unknown valid MAC. Have each
scenario invoke get_switch_nvos_credentials and assert only the resulting
tonic::Code, preserving InvalidArgument for the first three cases and NotFound
for the final case.
🪄 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: 7940e89f-064d-4a3a-a541-fdc52e889e39
⛔ Files ignored due to path filters (2)
rest-api/proto/core/gen/v1/nico_nico.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.gorest-api/proto/core/gen/v1/nico_nico_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**,!rest-api/**/*.pb.go,!rest-api/**/*_grpc.pb.go
📒 Files selected for processing (33)
crates/api-core/src/api.rscrates/api-core/src/auth/internal_rbac_rules.rscrates/api-core/src/handlers/credential.rscrates/api-core/src/tests/credential.rscrates/health/src/api_client.rscrates/rpc/build.rscrates/rpc/proto/forge.protohelm-prereqs/health-check.shhelm-prereqs/setup.shhelm-prereqs/templates/flow-vault-tokens-job.yamlhelm/charts/nico-flow/templates/deployment.yamlhelm/charts/nico-flow/values.yamlrest-api/docker/local/Dockerfile.nico-nsmrest-api/docker/production/Dockerfile.nico-nsmrest-api/nvswitch-manager/README.mdrest-api/nvswitch-manager/cmd/serve.gorest-api/nvswitch-manager/docker-compose.ymlrest-api/nvswitch-manager/internal/service/config.gorest-api/nvswitch-manager/internal/service/server_impl.gorest-api/nvswitch-manager/local-dev.shrest-api/nvswitch-manager/pkg/credentials/config.gorest-api/nvswitch-manager/pkg/credentials/config_test.gorest-api/nvswitch-manager/pkg/credentials/core.gorest-api/nvswitch-manager/pkg/credentials/core_test.gorest-api/nvswitch-manager/pkg/credentials/credential_manager.gorest-api/nvswitch-manager/pkg/credentials/credential_manager_test.gorest-api/nvswitch-manager/pkg/credentials/inmemory.gorest-api/nvswitch-manager/pkg/credentials/inmemory_test.gorest-api/nvswitch-manager/pkg/credentials/vault.gorest-api/nvswitch-manager/pkg/credentials/vault_test.gorest-api/nvswitch-manager/pkg/nvswitchmanager/nvswitchmanager.gorest-api/nvswitch-manager/pkg/nvswitchmanager/nvswitchmanager_test.gorest-api/proto/core/src/v1/nico_nico.proto
💤 Files with no reviewable changes (5)
- rest-api/nvswitch-manager/local-dev.sh
- rest-api/nvswitch-manager/pkg/credentials/inmemory_test.go
- rest-api/nvswitch-manager/docker-compose.yml
- rest-api/nvswitch-manager/pkg/credentials/vault_test.go
- rest-api/nvswitch-manager/pkg/credentials/vault.go
Comment on lines
+50
to
+53
| - apiGroups: [""] | ||
| resources: ["secrets"] | ||
| resourceNames: ["nsm-vault-token"] | ||
| verbs: ["delete"] |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Scope deletion permission to the Flow namespace.
A ClusterRoleBinding permits deletion of nsm-vault-token in any namespace. Move this Secret permission into a Role in {{ .Values.flow.namespace }} and bind the hook ServiceAccount there; retain cluster-scoped RBAC only for namespace operations.
🤖 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-prereqs/templates/flow-vault-tokens-job.yaml` around lines 50 - 53, Move
the secrets deletion rule for resourceNames “nsm-vault-token” out of the
cluster-scoped RBAC and into a namespaced Role targeting {{
.Values.flow.namespace }}. Bind the hook ServiceAccount to that Role in the same
namespace, while retaining only namespace-operation permissions in the
ClusterRole and its ClusterRoleBinding.
Source: Path instructions
Comment on lines
+109
to
+114
| ``` | ||
| # Prompt for the password rather than typing it as a command argument or an | ||
| # `export` that lands in shell history. | ||
| read -r DB_USER | ||
| read -rs DB_PASSWORD | ||
| export DB_USER DB_PASSWORD |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the credential input commands actually prompt the user.
read currently waits for input without identifying which value is requested, and the silent password read does not restore the terminal newline. Add explicit prompts and print a newline afterward.
Proposed fix
-read -r DB_USER
-read -rs DB_PASSWORD
+read -r -p "DB user: " DB_USER
+read -r -s -p "DB password: " DB_PASSWORD
+printf '\n'
export DB_USER DB_PASSWORD📝 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.
Suggested change
| ``` | |
| # Prompt for the password rather than typing it as a command argument or an | |
| # `export` that lands in shell history. | |
| read -r DB_USER | |
| read -rs DB_PASSWORD | |
| export DB_USER DB_PASSWORD | |
| # Prompt for the password rather than typing it as a command argument or an | |
| # `export` that lands in shell history. | |
| read -r -p "DB user: " DB_USER | |
| read -r -s -p "DB password: " DB_PASSWORD | |
| printf '\n' | |
| export DB_USER DB_PASSWORD |
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 109-109: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@rest-api/nvswitch-manager/README.md` around lines 109 - 114, Update the
credential input commands in the README so the DB_USER and DB_PASSWORD reads
display explicit prompts, and restore the terminal newline after the silent
password read while preserving the existing exports.
Source: Path instructions
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
2 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.
NVSwitch OS (NVOS) credentials were stored in Vault KV, written and read by the
NVSwitch Manager's own Vault client. That was the last piece of NSM's direct
Vault dependency, and it sat outside the credential infrastructure the rest of
the system already uses.
Core already solves this problem. It stores per-component credentials
envelope-encrypted in Postgres (per-write DEK, KEK wrap, path-bound ciphertext,
KEK routing, re-wrap, and a Vault import path), under the same key namespace NSM
was using in Vault:
CredentialKey::SwitchNvosAdminmaps toswitch_nvos/{MAC}/admin. It also already rotates those credentials per switchfrom the switch controller. The only real gap was that NSM talked to Vault
directly instead of asking Core.
So rather than build a second credential store, NSM now reads its switch
credentials from Core over mTLS gRPC and never writes them. Core seeds them from
the expected-switch record and owns rotation, so an NSM write would race that
rotation. Registration records identity and routing only, and deregistration
leaves credentials alone, since a switch removed from NSM may still be a live,
credentialed switch in Core. NSM does not cache, so a rotated password is picked
up on the next lookup rather than needing invalidation.
Two things surfaced while doing this and are fixed here.
The deployed NSM had never actually been running in persistent mode. Its image
entrypoint is
nsm servewith no arguments, the chart passed no arguments, and--datastorewas the one flag without an environment fallback, so it defaultedto in-memory. Its configured Postgres registry and its Vault token were both
unused. The flag now has an
NSM_DATASTOREfallback and the chart sets itexplicitly.
Reading a switch BMC credential needed its own RPC rather than reusing
GetBmcCredentials, which resolves a BMC IP frommachine_interfaceand mints aRedfish session token, neither of which applies to a switch BMC. Because
BmcRootis a single namespace shared by switch BMCs, compute tray BMCs, andpower shelf PMCs and keyed only by MAC, the new RPC checks the switch inventory
before reading, so a caller authorized for switch credentials cannot name an
arbitrary machine's BMC MAC and retrieve that machine's root password.
No credential migration is required: these paths fall under
CredentialPrefix::all(), so Core's existing Vault-to-Postgres import alreadycovers them, and NSM's Vault paths were byte-identical to Core's, including the
uppercase-MAC convention.
Related issues
Closes #1852
Part of #195
Type of Change
Breaking Changes
Upgrade nico-api before nico-flow. NSM depends on two Core RPCs that older
releases do not have:
GetSwitchBmcCredentials, andGetSwitchNvosCredentialswith the new
bmc_mac_addrfield. There is no fallback path, because NSM keyscredentials by BMC MAC and holds no Carbide
SwitchId, so against an older Coreevery credential lookup fails. It fails loudly rather than proceeding with a
stale credential.
NSM switches to its persistent datastore on upgrade. The chart now sets
NSM_DATASTORE=Persistent, activating the Postgres registry and Core credentialreads that were previously configured but inert.
Credentials supplied to RegisterNVSwitch are ignored in persistent mode.
Core owns switch credential storage. The proto fields remain for compatibility,
and NSM logs a warning when a caller still sets them rather than dropping them
silently.
The
nsm-vault-tokenSecret and its provisioning are retired. Removal isguarded: the prereqs job skips deletion while any Deployment in the flow
namespace still carries a
secretKeyRefto it, because a not-yet-upgraded NSMDeployment references it non-optionally and kubelet resolves that at container
start regardless of whether NSM reads the value. The Secret is removed on the
first prereqs upgrade after nico-flow rolls out.
Testing
Rust: new handler tests cover NVOS lookup by both selectors, rejection of a
request setting neither or both of them, rejection of a malformed or unknown
MAC, and
GetSwitchBmcCredentials. The authorization gate has a dedicated testthat seeds a
BmcRootcredential under a non-switch MAC and asserts it is notreadable, which is the assertion that actually proves the gate rather than
merely showing an unknown MAC returns nothing.
cargo test -p carbide-api-core --lib tests::credentialpasses, as do the RBAC table tests.Go: new
core_test.goruns a real gRPC server over bufconn and covers thefound, not-found, transport-error, permission-denied, session-token-instead-of-
password, and empty-username paths, and asserts the client selects by BMC MAC.
Further tests cover the in-memory seeding lifecycle: that a rejected
registration seeds nothing, and that deletion forgets what registration seeded.
go build ./...andgo test ./nvswitch-manager/...pass.Manual:
buf breakingagainst the merge-base passes, and the productionnico-nsmimage builds locally. Those are the two checks that failed on thefirst push of this PR. Also
helm lintandhelm templateon both affectedcharts with the rendered output parsed as YAML; confirmed the nsm container has
no
VAULT_*env remaining and carriesNSM_DATASTOREandNICO_CORE_API_URL.The Secret-cleanup guard's shell logic was exercised across four cases,
including that it does not false-match a similarly named Secret.
lint-policepasses all checks.
Not done: no end-to-end validation against a live cluster or real switch. The
rotation path in particular, rotating in Core and confirming NSM's next lookup
returns the new password, has not been exercised against real hardware.
Additional Notes
GetSwitchNvosCredentialsRequestcarriesswitch_idandbmc_mac_addras twoseparate fields rather than a
oneof. A oneof would express the exclusivity inthe type system, but moving an existing field into one trips
buf breaking(FIELD_SAME_ONEOF) even though the wire encoding is identical. Exclusivity is
therefore enforced in the handler: setting both is rejected rather than silently
preferring one, and both that case and the empty case are tested.
Reviewers may want to look closely at the switch-inventory gate in
get_switch_bmc_credentials, and at the RBAC change, which addsFlowto thetwo credential-read methods.
deleteon Secrets is pinned withresourceNames: ["nsm-vault-token"]rather than granted over Secrets generally.Follow-ups, deliberately out of scope: powershelf-manager still has a
near-identical Vault client, and the "every switch gets its own unique set of
credentials" half of #1852 is a Core-side change to
nvos_password_rotation.rs,since rotation today is driven by a site-wide target version that each switch
converges to individually.