fix local gateway CA trust#177
Conversation
Signed-off-by: Zoe Blevins <zblevins@nvidia.com>
📝 WalkthroughWalkthroughLocal deployments now provision a dedicated gateway CA, install it into system and browser trust stores, expose a PEM path for Node.js, and update local client documentation. CI retrieves the CA secret for HTTPS verification, while non-local deployments retain existing certificate behavior. ChangesLocal CA provisioning and Helm wiring
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Installer
participant Helm
participant CertManager
participant MakeInstallCert
participant TrustStores
Installer->>Helm: enable localCA for local TLS
Helm->>CertManager: create local CA and issuer
CertManager-->>Helm: issue gateway certificate
MakeInstallCert->>CertManager: read gateway CA secret
CertManager-->>MakeInstallCert: return CA certificate
MakeInstallCert->>TrustStores: install CA and save PEM copy
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
/ok to test 75b72fe |
|
/kind test |
|
Queued Kind integration for 75b72fe: https://github.com/NVIDIA/nv-config-manager/actions/workflows/kind-integration.yml |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Makefile (2)
1000-1011: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winmacOS trust install never removes a previously-trusted local CA, unlike the NSS branch.
The Linux NSS path explicitly deletes any existing "NVCM Local Gateway CA" entry (
certutil -D ... || true) before re-adding, but the Darwinsecurity add-trusted-certcall has no equivalent cleanup. Docs (docs/install/troubleshooting.mdx) explicitly tell users to re-runinstall-certafter recreating the Kind cluster, and each recreation generates a fresh CA keypair with the same CN — so on macOS this accumulates orphaned trusted root CAs in the System keychain indefinitely.🔒 Proposed fix to remove stale trust entries before installing
if [ "$$NVCM_OS" = "Darwin" ]; then \ + sudo security delete-certificate -c "$$NVCM_CERT_SUBJECT" /Library/Keychains/System.keychain >/dev/null 2>&1 || true; \ sudo security add-trusted-cert -d -r trustRoot \ -k /Library/Keychains/System.keychain "$$CERT_TMP"; \🤖 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 `@Makefile` around lines 1000 - 1011, Update the Darwin branch of the certificate installation flow to remove any existing “NVCM Local Gateway CA” trust entry from the System keychain before invoking security add-trusted-cert. Preserve the current certificate installation command and Linux behavior, and make stale-entry removal tolerant when no matching certificate exists.
937-1022: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTarget body length flagged by checkmake.
checkmake reports the
install-certrecipe (85 lines) far exceeds the recommended body length. Consider extracting the logic into a./scripts/install-certscript (as most other multi-step targets in this Makefile already do), keeping the target itself as a thin wrapper.🤖 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 `@Makefile` around lines 937 - 1022, The install-cert recipe is too long for Makefile maintainability and checkmake limits. Extract its shell logic into a dedicated scripts/install-cert executable, preserving all validations, certificate handling, trust-store installation, and user-facing messages; reduce the Makefile install-cert target to a thin wrapper that invokes the script with the required Makefile configuration values.Source: Linters/SAST tools
🤖 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.
Nitpick comments:
In `@Makefile`:
- Around line 1000-1011: Update the Darwin branch of the certificate
installation flow to remove any existing “NVCM Local Gateway CA” trust entry
from the System keychain before invoking security add-trusted-cert. Preserve the
current certificate installation command and Linux behavior, and make
stale-entry removal tolerant when no matching certificate exists.
- Around line 937-1022: The install-cert recipe is too long for Makefile
maintainability and checkmake limits. Extract its shell logic into a dedicated
scripts/install-cert executable, preserving all validations, certificate
handling, trust-store installation, and user-facing messages; reduce the
Makefile install-cert target to a thin wrapper that invokes the script with the
required Makefile configuration values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0c516325-ca14-416d-b4bb-966050995723
📒 Files selected for processing (12)
.github/workflows/kind-integration.ymlMakefileREADME.mddeploy/helm/templates/certificate.yamldeploy/helm/values-ci.yamldeploy/helm/values.yamldocs/getting-started/local-development-quick-start.mdxdocs/install/troubleshooting.mdxdocs/overview/mcp.mdxdocs/temporal/temporal-cli.mdxinstaller/src/nv_config_manager_installer/helm_values.pyinstaller/tests/test_helm_values.py
1 participant
Description
Validation
The kind integration test is manual due to taking ~30 min to complete. When the PR is ready for
review, approve the current commit and start the suite with these PR comments:
As a fallback, run Actions -> Kind Integration -> Run workflow against the copy-pr-bot generated
pull-request/<PR_NUMBER>branch. Use the defaulttest_pathfor the full suite, or narrow itonly while debugging.
The completed workflow updates this PR description with its conclusion and exact run URL.
Passing Kind Integration run, if not automatically reported:
Kind integration completed with success for
75b72fein this workflow run.Checklist
CONTRIBUTING.md.docs screenshots, or Helm/rendered outputs.
Summary by CodeRabbit
New Features
Documentation
Tests