[gmenher]

Summary

gateway add --local could replace an externally provisioned mTLS bundle with unrelated package-managed credentials. This PR keeps package-managed refresh as the default while adding an explicit --preserve-mtls-bundle path for externally managed credentials.

Related Issue

Closes #2560.

Compatibility

• Package-managed first setup and the default package refresh behavior are preserved.
• External bundles are preserved only when --preserve-mtls-bundle is supplied.
• Partial bundles, unreadable entries, dangling symlinks, malformed certificates, malformed private keys, and certificate/key material rejected by the Rustls client configuration fail before registration and are never replaced.
• The success output distinguishes imported package credentials from an existing bundle.
• The documented external-certificate examples name the gateway explicitly and use the preservation flag.

Changes

• Classify mTLS paths with symlink_metadata so existing dangling links cannot be mistaken for an absent bundle.
• Validate CA, client certificate, and private key through the same Rustls configuration path used by the CLI, not only by checking PEM framing.
• Add regressions for dangling symlinks and PEM-wrapped invalid private keys, including assertions that metadata is not stored and caller-owned paths are unchanged.
• Retain coverage for absent, partial, malformed, externally managed, and package-refresh flows.

Testing

• cargo test -p openshell-cli --lib commands::gateway — 40 tests
• cargo test -p openshell-cli --test mtls_integration — 6 tests
• cargo fmt --all -- --check
• cargo clippy -p openshell-cli --lib --bin openshell -- -D warnings
• mise run pre-commit
• Controlled mTLS validation with independent CA material
• Strix scan — Docker daemon unavailable in the validation environment

Checklist

• Follows Conventional Commits
• Commit is signed off (DCO)
• Documentation updated for opt-in preservation and default package refresh

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[Bobbins228]

Requesting changes for two reproducible cases:

1. crates/openshell-cli/src/commands/gateway.rs:1092-1098: mtls_certs_exist_for_gateway checks only whether all three paths are files. A zero-byte tls.key therefore suppresses the package import. With a valid package bundle and a local mTLS test server, the base commit imported the package bundle and connected; this PR kept the empty key, reported "Using existing mTLS certificates," and could not connect. Please treat unusable files as an incomplete bundle before choosing the fallback (or reject registration with a clear error rather than reporting success).

2. deploy/rpm/TROUBLESHOOTING.md:94-98: the externally-managed certificate example copies the bundle into gateways/openshell/mtls/ but registers https://<gateway-hostname>:17670 without --name openshell. For a non-loopback hostname, the CLI derives the hostname as the gateway name and looks in gateways/<gateway-hostname>/mtls/. I reproduced the PR importing package credentials for that derived name while leaving the explicit openshell bundle unused. Please add --name openshell to the example and cover that documented flow.

The primary preservation case works in a live before-and-after mTLS test, and all 33 gateway unit tests pass.

[gmenher]

@Bobbins228 both findings addressed in da2d21b:

• The bundle check now parses each file as its PEM kind (rustls-pemfile), so a present-but-unusable file (e.g. a zero-byte key from a partial copy) fails registration with an error naming the file, instead of falling back to package credentials or reporting success. The partial-bundle (missing files) fallback-import behavior is unchanged.
• The troubleshooting example now registers with --name openshell, with a note on why the explicit name matters (the CLI derives the name from the endpoint hostname otherwise), and the documented flow is covered by a test.
• Verified against a live mTLS gateway: pre-provisioned bundle preserved and connected (Authenticated (mTLS transport)), zero-byte key rejected while the gateway was reachable.

Thanks for the thorough review — the before/after repro caught exactly the flows we had not enumerated.

[TaylorMutch]

Requesting one change before merge:

[P1] Reject partially present gateway bundles instead of importing over them.

gateway_mtls_bundle_state() classifies any missing expected file as Incomplete, and gateway_add() then imports the package bundle, replacing any gateway-specific files already present. If external provisioning copies ca.crt and tls.crt but is interrupted before tls.key, registration silently replaces those authoritative files with unrelated package credentials and recreates the UnknownIssuer failure this PR addresses. The new gateway_add_local_imports_package_bundle_when_existing_bundle_is_incomplete test confirms this behavior.

Please distinguish between no expected files, where package fallback is appropriate, and some-but-not-all expected files, which should fail while naming the missing files. The incomplete-bundle test should assert that registration fails and the existing files remain untouched; a separate no-files case can retain coverage for package fallback.

The complete-bundle preservation, malformed-PEM rejection, explicit --name openshell documentation, and associated tests otherwise look good. I ran all 36 gateway unit tests locally at da2d21bb; they passed, and git diff --check is clean.

[gmenher]

@TaylorMutch , Adressed in my last push, I splitted the Incomplete state into Absent (package fallback is appropriate) and Partial (the failure names the missing files, and the already present files remain untouched). I also replaced the old bundle test with two new tetss: one asserting the partial bundle is rejected with existing files preserved, and other one retaining coverage for the no-files package fallback.

Thanks a ton for the review!!

[TaylorMutch]

/ok to test ce741db

[github-actions]

Compatibility review (advisory)

New or updated breaking changes; see the full report for all findings.

1. cli — No explicit risk acceptance found

openshell gateway add <endpoint> --local --name <name> previously refreshed the package-managed mTLS bundle, but now preserves a complete existing bundle and rejects partial or unusable files.

• Impact: Package-managed users refreshing rotated credentials must now delete ~/.config/openshell/gateways/<name>/mtls/, while existing automation can retain stale credentials or fail.
• Action: Restore package-managed refresh behavior for v0.1.6; the candidate’s migration instructions do not address a Stable-interface break on a patch train.
• PR: fix(cli): preserve pre-provisioned mTLS bundle during gateway add --local #4709

Review details

• Execution: complete
• Candidate: PR #4709 (dabc76b2feea8785704bf631658be571e18871b5)
• Baseline: 39d95b42cb403d883e2f545a3861131ccd3d2ebf (39d95b42cb403d883e2f545a3861131ccd3d2ebf)
• Reviewed: rust-sdk, python-sdk, go-sdk, typescript-sdk, api-behavior, cli, configuration, policy, helm, state

Advisory review; publication gates are unchanged.

Branch check run

[TaylorMutch]

The previous partial-bundle finding is resolved at ce741db9: Absent and Partial are now distinct, partial bundles fail with the missing paths, existing files remain untouched, and the no-files package fallback has separate coverage. I ran all 37 gateway unit tests locally; they pass, git diff --check is clean, and the full GitHub branch checks are green.

One newly surfaced compatibility issue still blocks approval:

[P1] Preserve the package-managed refresh behavior on the 0.1.x patch train.

The Stable v0.1.5 documentation explicitly says that openshell gateway add <endpoint> --local --name openshell refreshes the CLI bundle from the package-managed TLS directory. After gateway remove (which intentionally leaves mtls/ behind), the old add path imports the current package credentials over that complete stored bundle. This PR now classifies that bundle as Usable and skips the import, so existing package automation and users following the documented remove/add refresh flow retain stale credentials instead.

This is a breaking change to a Stable CLI behavior in the active 0.1.6 patch train. Please retain the existing refresh behavior by default and make preservation of externally managed credentials explicit through an additive flag or durable provenance marker, then update the external-certificate example and tests to select that path. Alternatively, this default behavior change needs a minor release with notice and migration guidance.

Evidence:

• Stable v0.1.5 contract: https://github.com/NVIDIA/OpenShell/blob/v0.1.5/docs/how-it-works/gateways/authentication.mdx#L51-L52
• New skip-import branch:

  OpenShell/crates/openshell-cli/src/commands/gateway.rs

  Lines 1198 to 1202 in ce741db

  	let has_mtls_certs = matches!(bundle_state, GatewayMtlsBundle::Usable);
  	let imported_mtls_dir = if local && !has_mtls_certs {
  	import_local_package_mtls_bundle(name)?
  	} else {
  	None

• Release policy: https://github.com/NVIDIA/OpenShell/blob/main/docs/about/support-matrix.mdx#L33-L36

The preservation logic itself otherwise looks correct.

[gmenher]

Addressed at 94b7036: gateway add --local now refreshes the package-managed bundle over a complete stored one by default, restoring the documented v0.1.5 remove/add flow. Preserving an externally managed bundle is now explicit through the additive --preserve-mtls-bundle flag — I went with the flag over a provenance marker because bundles written before the marker existed are indistinguishable from externally provisioned ones, so a marker cannot classify them without breaking the refresh for upgraded installs.

The external-certificate examples (TROUBLESHOOTING.md Option 2 and the compose mTLS flow in container-deployment.mdx) and their tests now select the flag, and a new test pins the default refresh contract (gateway_add_local_refreshes_package_credentials_by_default). Partial and unusable bundles still fail registration naming the affected paths, unchanged from the previous round.

Happy to address anything else.

[Bobbins228]

Reviewed and tested 94b7036d0be6b82796f98cc270eb224060a90375.

The specific reproductions from the previous reviews are addressed: empty/non-PEM files are rejected, ordinary partial bundles remain untouched, the external-certificate example names the gateway explicitly, and package-managed refresh is restored by default with preservation selected through --preserve-mtls-bundle.

Requesting changes for two narrower remaining cases, detailed in the inline comments:

1. With all three expected paths represented by dangling symlinks, --preserve-mtls-bundle still replaces those entries with package credentials.
2. A PEM-wrapped invalid private key is classified as usable, so registration succeeds and saves metadata even though the TLS client rejects the material.

Both were reproduced locally with additional tests at this commit. The tests fail their safety assertions because the links are replaced and the invalid key is registered, respectively.

Validation passed:

• All 38 gateway unit tests.
• All six existing mTLS integration tests.
• An additional live mTLS test with two independent CAs: the preserve flag retains the supplied bundle and connects while a competing package bundle is available.
• cargo fmt --all -- --check.
• Clippy for the CLI library and binary with warnings denied.
• Documentation navigation validation and Fern check (zero errors; three warnings outside the changed pages).

Please also update the PR description to match the current implementation: preservation is opt-in, default package refresh is retained, and partial bundles are rejected. GitHub branch CI for this head is still waiting for authorization.

[Bobbins228]

Keep dangling certificate links distinct from an absent bundle. Before this change, local registration could replace externally provisioned credentials. The new --preserve-mtls-bundle path still does this when all three expected paths are dangling symlinks: is_file() follows the links and returns false, so the bundle becomes Absent and the package import replaces the entire directory, including the caller's links.

I reproduced this at 94b7036 with ca.crt, tls.crt, and tls.key pointing into a temporarily unavailable external directory, a valid package bundle, and registration with --local --preserve-mtls-bundle. Registration succeeded and all three symlinks were replaced by regular package credential files. This is a remaining edge case of the earlier preservation finding; ordinary partially present files are now handled correctly.

Please distinguish genuinely missing directory entries from existing links or unreadable entries, reject the latter without importing, and add a regression test asserting that the links remain intact.

[Bobbins228]

Validate the decoded key before classifying the bundle as usable. The earlier empty-key/non-PEM reproductions are fixed, but the new validation still lets a caller register with credentials that the TLS client cannot use. rustls_pemfile::private_key() decodes the PEM envelope; finding a block does not establish that its contents are a valid private key or match the client certificate.

I reproduced this at 94b7036 with a valid CA/client certificate, a valid competing package bundle, and the following stored tls.key:

-----BEGIN PRIVATE KEY-----
AA==
-----END PRIVATE KEY-----

With --local --preserve-mtls-bundle, registration succeeds, saves metadata, and prints Using existing mTLS certificates, although build_rustls_config() rejects those same materials. This is a remaining case of the original unusable-bundle concern: the failure is reduced to a generic reachability warning instead of rejecting the broken bundle before registration.

Please validate the decoded certificate/key material with the client TLS configuration builder or a shared equivalent before returning Usable, and cover a PEM-wrapped invalid key in the regression tests.