[#1867] fix: convert vault client trust_domain and namespace configurable

[prbinu-nvidia]

Summary

Vault-issued machine certificates previously hardcoded SPIFFE URI SANs as spiffe://forge.local/forge-system/machine/<id>. Sites with non-default trust domains (e.g. nico.local) received certs that did not match auth/SPIFFE expectations.
This PR wires machine PKI URI SANs to the same trust settings used elsewhere:

• Adds optional spiffe_trust_domain and spiffe_machine_base_path on VaultConfig, with defaults (forge.local, /forge-system/machine/) and env overrides (VAULT_SPIFFE_TRUST_DOMAIN, VAULT_SPIFFE_MACHINE_BASE_PATH)
• Introduces machine_spiffe_uri() to build URIs consistently from trust domain, base path, and machine ID
• At carbide-api startup, vault_config_for_site() copies [auth.trust] into the Vault client config when site auth config is present, so issued certs align with site SPIFFE layout without a separate namespace field
• Behavior: unchanged for deployments using defaults; sites with custom [auth.trust] now get matching URI SANs on Vault-issued machine certs.

Files changed

Area	Files
Vault SPIFFE URI construction	crates/secrets/src/forge_vault.rs
API startup wiring	crates/api/src/run.rs
Test fixtures	crates/api-test-helper/src/utils.rs, crates/api-integration-tests/tests/vault_catalogue.rs

Description

Type of Change

• Add - New feature or capability
• Change - Changes in existing functionality
• Fix - Bug fixes
• Remove - Removed features or deprecated functionality
• Internal - Internal changes (refactoring, tests, docs, etc.)

Related Issues (Optional)

#1867

Breaking Changes

• This PR contains breaking changes

Testing

• Unit tests added/updated
• Integration tests added/updated
• Manual testing performed
• No testing required (docs, internal refactor, etc.)

Additional Notes

Summary by CodeRabbit

• New Features

  • Machine certificates from Vault can now use configurable SPIFFE trust domain and base path settings.
  • Site-specific Vault configuration is supported through environment variables and initialization.

• Improvements

  • Configuration initialization now properly sets all fields with default values.

[copy-pr-bot]

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

[ianderson-nvidia]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Walkthrough

This PR parameterizes SPIFFE certificate configuration by extending VaultConfig with optional trust domain and machine base path fields, introducing a configurable URI builder, and wiring the configuration through certificate generation. It also adds site-specific vault configuration support in the API layer for trust-aware certificate requests.

Changes

SPIFFE Configuration and Certificate Generation

Layer / File(s)	Summary
VaultConfig Schema and Resolution crates/secrets/src/forge_vault.rs	VaultConfig adds spiffe_trust_domain and spiffe_machine_base_path optional fields and public accessor methods that resolve values from struct fields, environment variables (SPIFFE_TRUST_DOMAIN, SPIFFE_MACHINE_BASE_PATH), then hardcoded defaults ("forge.local", ""). Configuration constants are defined at module level.
SPIFFE URI Construction Helper crates/secrets/src/forge_vault.rs	New machine_spiffe_uri(trust_domain, machine_base_path, machine_id) helper normalizes the machine base path by trimming whitespace and slashes, then constructs a SPIFFE URI with optional path inclusion. Unit test validates URI construction across trust-domain and base-path combinations.
ForgeVaultClientConfig and GetCertificateHelper Wiring crates/secrets/src/forge_vault.rs	ForgeVaultClientConfig gains spiffe_trust_domain and spiffe_machine_base_path fields. create_vault_client populates these from VaultConfig accessors. GetCertificateHelper stores the same fields and wires them from ForgeVaultClientConfig during certificate requests.
Certificate SPIFFE Generation crates/secrets/src/forge_vault.rs	Certificate SPIFFE ID construction now calls machine_spiffe_uri(...) with configured trust domain and machine base path instead of using hardcoded "forge.local" and fixed namespace layout.

Site-Specific Vault Configuration

Layer / File(s)	Summary
vault_config_for_site Helper and API Integration crates/api/src/run.rs	New vault_config_for_site helper clones the base vault config and, if carbide_config.auth.trust exists, overrides spiffe_trust_domain and spiffe_machine_base_path to match trust settings. During run, the derived vault_config is used to create the certificate provider and initialize the vault credential store instead of always using credential_config.vault.

Test Fixture Completeness

Layer / File(s)	Summary
VaultConfig Struct Update Completeness crates/api-integration-tests/tests/vault_catalogue.rs, crates/api-test-helper/src/utils.rs	VaultConfig struct initializers in test setup functions now include ..Default::default() to ensure all fields not explicitly set are populated from the type's default values.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Poem

🐰 Whiskers twitch with delight as SPIFFE paths unfold,
No hardcoded forests, but trust domains controlled,
Config flows through helpers, defaults resolve with care,
Each certificate blooms with its configured flair! ✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the primary change: making Vault's trust domain and namespace (base path) configurable instead of hardcoded, which directly addresses issue #1867.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

crates/secrets/src/forge_vault.rs (1)

1047-1061: ⚡ Quick win

Add test coverage for empty/whitespace-only base path.

The machine_spiffe_uri function has an if base.is_empty() branch (lines 145-146) that is not exercised by these tests. Consider adding a case to verify that behavior.

Proposed addition

         assert_eq!(
             machine_spiffe_uri("forge.local", "forge-system/machine", "abc-123"),
             "spiffe://forge.local/forge-system/machine/abc-123"
         );
+        // Empty or whitespace-only base path omits the path segment
+        assert_eq!(
+            machine_spiffe_uri("forge.local", "", "abc-123"),
+            "spiffe://forge.local/abc-123"
+        );
+        assert_eq!(
+            machine_spiffe_uri("forge.local", "   ", "abc-123"),
+            "spiffe://forge.local/abc-123"
+        );
     }

🤖 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/secrets/src/forge_vault.rs` around lines 1047 - 1061, Add test
assertions to exercise the machine_spiffe_uri branch where base.is_empty(): in
the existing test (or a new test) call machine_spiffe_uri with an empty string
base ("") and with a whitespace-only base ("   ") for at least one trust domain
and id, and assert the resulting URI matches the expected
spiffe://<trust-domain>/machine/<id> (i.e., no extra slashes or base path
inserted); reference the machine_spiffe_uri function to locate and extend the
test cases.

🤖 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 `@crates/secrets/src/forge_vault.rs`:
- Around line 1047-1061: Add test assertions to exercise the machine_spiffe_uri
branch where base.is_empty(): in the existing test (or a new test) call
machine_spiffe_uri with an empty string base ("") and with a whitespace-only
base ("   ") for at least one trust domain and id, and assert the resulting URI
matches the expected spiffe://<trust-domain>/machine/<id> (i.e., no extra
slashes or base path inserted); reference the machine_spiffe_uri function to
locate and extend the test cases.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: 927fca33-d7a9-4941-914a-0371b60a73e1

📥 Commits

Reviewing files that changed from the base of the PR and between 09345b2 and 1c4df6c.

📒 Files selected for processing (4)

• crates/api-integration-tests/tests/vault_catalogue.rs
• crates/api-test-helper/src/utils.rs
• crates/api/src/run.rs
• crates/secrets/src/forge_vault.rs

[prbinu-nvidia]

@ianderson-nvidia, please review.

[dathan]

I would recommend that nico.local is the default, and any location that has the name forge in it, is replaced with nico, to be less confusing for someone who reads this in two years time.

[ianderson-nvidia]

@prbinu-nvidia

This should be a follow-up issue IMO:

By changing to nico.local the only way to deploy this build without breaking existing deployments is to coordinate the deployment with changes to the ConfigMap of the api config file for all pre-existing sites. For each pre-existing site, we'd need to ensure spiffe_trust_domain = forge.local is set.

[prbinu-nvidia]

agree, and to add, the name change ideally should be a repo wide task broadly considering the side effects and also being consistant.

[prbinu-nvidia]

ref: #2060