Skip to content

fix(vault-human-auth-iac): let tenants browse to their folder in the UI - #1655

Merged
mashkovd merged 1 commit into
mainfrom
fix/vault-tenant-browse-list
Oct 4, 2026
Merged

mashkovd merged 1 commit into
mainfrom
fix/vault-tenant-browse-list

Conversation

@mashkovd

@mashkovd mashkovd commented Oct 4, 2026

Copy link
Copy Markdown
Member

Problem

In a live end-to-end test, a tenant user signed in to the Vault UI through mctl, opened the secret/ engine and saw an empty list. human-tenant-<t> grants list only under secret/metadata/teams/<t>/*, so the UI could not walk down to the tenant folder. The user had to know to type teams/<t>/ into "View secret" by hand.

Change

Every generated human-tenant-<t> policy gets two exact, list-only rules:

path "secret/metadata/" {
  capabilities = ["list"]
}

path "secret/metadata/teams/" {
  capabilities = ["list"]
}

The UI can then walk secret/ -> teams/ -> <tenant>/. The database deny rules and every other rule are unchanged. The runbook (docs/runbooks/vault-human-auth.md) documents the browse path and the proof.

Why this does not open other tenants (Vault semantics)

  • A path without * or + is an exact match, so list on secret/metadata/teams/ covers only the LIST request on that one path. secret/metadata/teams/<other>/ is a different path, matched by no rule, so it is denied. This holds at every depth.
  • list gives no read. Reading the metadata of secret/metadata/teams itself is still 403.
  • The rules sit under metadata/ only. Nothing under data/, delete/, undelete/ or destroy/ is widened.

Disclosure trade-off

A tenant user can now see two listings, and nothing else:

  1. The top-level key names of secret/. Today these are platform/ and teams/, both already named throughout this repository's docs.
  2. The folder names directly under secret/teams/. These are the tenant names, already public in platform-gitops/tenants/, plus any platform-owned folder kept under teams/.

Neither listing reveals a secret, a key inside another folder, or any metadata. The alternative was keeping the UI empty and having tenants type paths by hand, which is what triggered this.

If a folder name directly under teams/ ever has to stay private from tenants, it does not belong there. Move it under platform/.

Proof (local Vault 1.17.2, both ways)

The test used a token holding only the rendered human-tenant-erpact policy. A second tenant other and a platform/ tree were seeded. A check passes only on success or an explicit permission denied; any other error fails it.

Policy Result
This PR 20/20: list secret/metadata/, .../teams/ and .../teams/erpact/ allowed. List teams/other/, teams/other/<svc>/ and platform/ denied. Reading teams metadata and anything under other/ denied. Writes at teams/x or a top-level key denied. Deleting secret/metadata/teams denied. Own read/write allowed. database write, soft delete, destroy and own metadata delete denied.
Mutation 1: browse rules removed 18/20: the two listings are 403, which is the reported bug.
Mutation 2: secret/metadata/teams/* instead of the exact path 18/20: teams/other/ and teams/other/<svc>/ become listable.

This module has no test harness; a CI policy matrix is tracked in #1624. Its matrix should include these rows.

Rollout

Merging changes the vault_policy.tenant[*] resources in place (update, which allowedActions already allows). The next PostSync Job, or the hourly run, applies it, and the run after that reports no changes.

🤖 Generated with Claude Code

https://claude.ai/code/session_01D1kGUJMWeA8hyhQuxKuHd2

A tenant user signing in to the Vault UI saw an empty secret/ engine:
human-tenant-<t> granted list only under secret/metadata/teams/<t>/*, so
the UI could not walk down to the tenant folder, and the user had to know
to type teams/<t>/ into "View secret".

Grant list, and nothing else, on exactly secret/metadata/ and
secret/metadata/teams/, with no glob. Those two listings reveal the
top-level key names and the tenant folder names, which are public in
platform-gitops/tenants/. Another tenant's folder and everything under
platform/ stay 403; proven on a local Vault 1.17.2 both ways (without the
rules the listing is 403, with a glob another tenant's folder is listable).
Copilot AI balanced review requested due to automatic review settings October 4, 2026 19:31

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@claude

claude Bot commented Oct 4, 2026 •

Copy link
Copy Markdown

Claude finished @mashkovd's task in 2m 16s —— View job


Review complete ✅

  • Gather context (diff, CLAUDE.md, affected files)
  • Review groups.tf Vault policy change
  • Review runbook doc update
  • Post findings / verdict

Verdict: APPROVE (0 P1, 0 P2, 1 P3)

What changed: adds two static, non-glob ACL rules to every human-tenant-<tenant> policy — list only on exactly secret/metadata/ and secret/metadata/teams/ — so a tenant can browse secret/ → teams/ → <tenant>/ in the Vault UI instead of having to type the path by hand. Docs in docs/runbooks/vault-human-auth.md updated to match.

Checked:

  • Vault ACL semantics: an exact path (no */+) only matches that literal path, and list grants nothing beyond the LIST verb. secret/metadata/teams/<other-tenant>/ is a distinct literal path matched by no rule → still 403. No read/write/delete/destroy capability is added anywhere.
  • No conflict with the existing secret/metadata/teams/${each.key}/* rule (glob already covers the tenant's own listing).
  • Disclosure is limited to two already-public listings (top-level engine keys, tenant folder names under teams/) — matches the PR's stated trade-off.
  • Doc updates (capability table, browse-path bullet, proof table, CLI walkthrough) are consistent with the Terraform change.

1 P3 nit posted inline: the two browse rules are identical literals duplicated into every tenant's policy via for_each (they don't depend on each.key). Not a bug — just flagged as a possible future simplification (e.g. a shared policy attached to all tenant groups) to keep terraform plan diffs smaller.

@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown

Antigravity (agy) review — informational

Model: gemini-3.8-flash-medium · commit 070837c.

Findings

Finding 1 (P3)

  • Severity: P3
  • File: platform-gitops/helm-charts/vault-human-auth-iac/iac/groups.tf:84
  • Anchor: + path "secret/metadata/" {
  • Concrete failure scenario: The change adds browse navigation rules to the rendered Vault tenant policy so users can list keys at secret/metadata/ and secret/metadata/teams/. While manual verification against Vault 1.17.2 is documented in docs/runbooks/vault-human-auth.md, there is no test visible in this diff under tests/ verifying that rendered tenant policies grant list on these exact paths while denying listing or reading on other subpaths. Without policy tests in CI, future policy edits could inadvertently alter or drop these exact path rules.
  • Proposed fix: Add automated policy assertion tests under tests/ to verify that human-tenant-${each.key} tokens retain list capabilities on secret/metadata/ and secret/metadata/teams/ while continuing to deny access to other tenant and platform prefixes.


path "secret/metadata/teams/" {
capabilities = ["list"]
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: The two browse rules (secret/metadata/ and secret/metadata/teams/, list-only) are identical literal strings duplicated into every tenant's human-tenant-<tenant> policy via this for_each. Functionally fine (exact-match paths, well tested per the PR description), but since the rule doesn't depend on each.key, it could instead live on a single shared policy attached to every tenant group, avoiding N duplicate copies and keeping terraform plan diffs smaller when this rule ever changes. Not blocking.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferred (P3). Keeping the rules inside each tenant's own policy is deliberate: a tenant token carries exactly one human-tenant policy, so its access can be read off that policy alone, and a shared policy would be one more attachment to keep in sync per group. The duplication is two short exact-path rules per tenant.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No P1/P2 findings (1 P3). Vault ACL semantics check out: the two new rules are exact (non-glob) paths granting list-only on secret/metadata/ and secret/metadata/teams/, so they don't widen read/write/delete/destroy anywhere, and don't let a tenant list another tenant's folder (secret/metadata/teams// remains unmatched by any rule). Matches the PR's documented proof. Good to merge.

@mashkovd

mashkovd commented Oct 4, 2026

Copy link
Copy Markdown
Member Author

agy P3 (no CI policy assertions): deferred. The rendered policy was proven both ways against Vault 1.17.2 (see the PR body and runbook); a CI harness that boots Vault and asserts tenant policies is a separate piece of work. Copilot did not review (quota).

@mashkovd
mashkovd merged commit bbb17c0 into main Oct 4, 2026
27 checks passed
@mashkovd
mashkovd deleted the fix/vault-tenant-browse-list branch October 4, 2026 21:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants