fix(helm-prereqs): default nico-cli-client PKI role OU to the role name, not "Invalid"

[shayan1995]

The nico-prereqs vault-pki-config job created the nicoca nico-cli-client PKI role with ou="Invalid" by default, stamping OU=Invalid into every operator CLI client cert. On affected clusters every carbide-admin-cli call returned a bare HTTP 403.

Root cause analysis

The ou: "Invalid" default was copied from the ForgeAdminCLI principal in internal_rbac_rules.rs on the belief that the cert OU had to match that hardcoded group. It does not: Principal::is_proper_subset_of matches any ExternalUser against any ExternalUser without comparing the group (crates/authn/src/middleware.rs), so that constant is documentation-only. What actually gates admin-CLI authorization is the cert mapping to an ExternalUser at all — i.e. the issuing CA's CN being listed in nico-api auth.additional_issuer_cns (which helm-prereqs/values/nico-core.yaml already sets to ["site-root"]).

Changes

• helm-prereqs/values.yaml: nicoCliClientRole.ou now defaults to "", which falls back to the role name (nico-cli-client) via the template's existing | default chain. The comment now documents the real authorization contract instead of the incorrect "must match the hardcoded group" claim.
• helm-prereqs/values/nico-core.yaml: drop the stale pair with ou: "Invalid" cross-reference.
• helm-prereqs/README.md: document the previously-undocumented nicoCliClientRole.ou knob; correct the enabled default cell (the chart ships enabled: true).
• crates/api-core/src/auth/internal_rbac_rules.rs: rename the never-compared placeholder group from "Invalid" to "nico-cli-client" with a comment explaining it is not load-bearing — so it cannot leak into site configs as a "required" value again. No behavior change (the group is not compared by the matcher).

Related issues

Fixes #3662

Type of Change

• Fix - Bug fixes

Breaking Changes

• This PR contains breaking changes

Existing clusters whose PKI role was already created with ou=Invalid keep working (the OU is not compared); re-running the vault-config job rewrites the role to ou=nico-cli-client and newly minted certs pick it up. Explicit vault.nicoCliClientRole.ou overrides are honored unchanged.

Testing

• Manual testing performed

helm template on the chart renders the role with ou="nico-cli-client" with default values, and honors an explicit ou override. cargo check -p carbide-api-core passes; no test references the old placeholder group value.

[coderabbitai]

Summary by CodeRabbit

• Configuration

  • Enabled issuance of short-lived NICo CLI client certificates by default.
  • Updated certificate defaults to use the nico-cli-client role consistently instead of an invalid placeholder.
  • Clarified how certificate organizational units and issuer names affect admin CLI authorization.

• Documentation

  • Expanded setup guidance for Vault PKI roles and certificate values.
  • Added clearer instructions for configuring CLI client certificates and avoiding invalid placeholder settings.

Walkthrough

The changes align the nico-cli-client PKI role defaults, certificate authorization documentation, and ForgeAdminCLI RBAC principal mapping. The PKI role now enables by default and uses an empty OU fallback, while authorization comments distinguish issuer-CN authorization from OU group naming.

Changes

CLI client authorization alignment

Layer / File(s)	Summary
PKI role defaults and authorization documentation helm-prereqs/values.yaml, helm-prereqs/README.md, helm-prereqs/values/nico-core.yaml	The CLI client role defaults to enabled, uses an empty OU that falls back to its name, and documents issuer-CN authorization separately from OU group naming.
RBAC principal mapping crates/api-core/src/auth/internal_rbac_rules.rs	The ForgeAdminCLI external-user mapping changes from "Invalid" to "nico-cli-client" and documents the certificate issuer-CN gate.
Estimated code review effort: 2 (Simple)	~10 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and accurately summarizes the primary change to the nico-cli-client PKI role OU default.
Description check	✅ Passed	The description is directly related to the change set and explains the bug, fix, and impact clearly.
Linked Issues check	✅ Passed	The changes satisfy #3662 by defaulting the role OU to the role name and removing the invalid placeholder behavior.
Out of Scope Changes check	✅ Passed	The documentation and comment updates are supporting changes and remain within the scope of the linked issue.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[coderabbitai]

🧹 Nitpick comments (1)

crates/api-core/src/auth/internal_rbac_rules.rs (1)

963-975: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Add a regression test for the ForgeAdminCLI authorization contract.

This change relies on the group value being documentation-only. Add table-driven cases covering the default OU, an explicit OU override, and an issuer CN absent from auth.additional_issuer_cns; verify that OU changes do not alter authorization while an unmapped issuer is rejected. Also assert that RuleInfo::new emits the expected nico-cli-client principal.

As per coding guidelines, Rust input-to-output authorization mappings should use table-driven tests.

🤖 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/auth/internal_rbac_rules.rs` around lines 963 - 975, The
ForgeAdminCLI authorization contract lacks regression coverage for
documentation-only OU matching. Add a table-driven test covering the default OU,
an explicit OU override, and an issuer CN missing from
auth.additional_issuer_cns; verify both mapped issuers authorize regardless of
OU and unmapped issuers are rejected, and assert RuleInfo::new produces the
nico-cli-client ExternalUser principal.

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.

Nitpick comments:
In `@crates/api-core/src/auth/internal_rbac_rules.rs`:
- Around line 963-975: The ForgeAdminCLI authorization contract lacks regression
coverage for documentation-only OU matching. Add a table-driven test covering
the default OU, an explicit OU override, and an issuer CN missing from
auth.additional_issuer_cns; verify both mapped issuers authorize regardless of
OU and unmapped issuers are rejected, and assert RuleInfo::new produces the
nico-cli-client ExternalUser principal.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: f1a6e09f-b783-479f-aa26-a4db7a0bee7a

📥 Commits

Reviewing files that changed from the base of the PR and between cfe6bd9 and c3ae98d.

📒 Files selected for processing (4)

• crates/api-core/src/auth/internal_rbac_rules.rs
• helm-prereqs/README.md
• helm-prereqs/values.yaml
• helm-prereqs/values/nico-core.yaml