Skip to content

fix(auth): match SPIFFE prefixes on path-segment boundaries#22

Merged
dmathur0 merged 3 commits into
mainfrom
port/spiffe-prefix-boundary-mr237
Jul 8, 2026
Merged

fix(auth): match SPIFFE prefixes on path-segment boundaries#22
dmathur0 merged 3 commits into
mainfrom
port/spiffe-prefix-boundary-mr237

Conversation

@dmathur0

@dmathur0 dmathur0 commented Jun 1, 2026

Copy link
Copy Markdown
Collaborator

A naive startswith() match let a sibling identity (e.g. spiffe://td/ns/nv-config-manager-admin) inherit the role mapped to a shorter prefix (spiffe://td/ns/nv-config-manager). Match now requires an exact prefix or a '/' boundary, so roles only apply along hierarchical SPIFFE path segments. Adds sibling/exact/layered regression tests.

Description

  • Enforce boundary-aware SPIFFE group-prefix matching (exact or /-delimited), preventing sibling identities from inheriting a shorter prefix's role.
  • Align the SpiffeConfig docstring and INI example with the implementation: removed the unimplemented spiffe:<workload_name> tracking-group claim, since identity_from_spiffe() only adds "all" plus configured mapped groups.

Validation

  • Standard CI passes.
  • Kind integration passes, or this PR explains why it was not run.

The kind integration test is manual due to taking ~30 min to complete. When the PR is ready for review,
run Actions -> Kind Integration -> Run workflow against the copy-pr-bot generated
pull-request/<PR_NUMBER> branch. Use the default test_path for the full suite,
or narrow it only while debugging.

Passing Kind Integration run:
https://github.com/NVIDIA/nv-config-manager/actions/runs/28549893053

Checklist

  • I am familiar with the contributing guidelines in CONTRIBUTING.md.
  • Commits are signed off for DCO compliance.
  • New or existing tests cover these changes, or the PR explains why tests are not needed.
  • Documentation is updated for user-facing behavior changes.
  • Generated artifacts are updated when applicable, such as OpenAPI specs,
    docs screenshots, or Helm/rendered outputs.

Summary by CodeRabbit

  • Bug Fixes

    • SPIFFE group-prefix matching now enforces boundary-aware semantics: a configured prefix grants roles only on exact match or when the ID continues with a path segment, preventing sibling identities from unintentionally receiving mapped roles.
  • Tests

    • Added regression tests covering boundary-aware prefix matching, exact-prefix grants, and cumulative behavior when multiple overlapping prefixes apply.

@coderabbitai

coderabbitai Bot commented Jun 1, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Replaces naive SPIFFE prefix matching with boundary-aware semantics (exact match or prefix + "/") in documentation and in identity_from_spiffe, and adds three tests verifying sibling rejection, exact-match acceptance, and layered-prefix cumulative groups.

Changes

SPIFFE Prefix Matching Boundary Fix

Layer / File(s) Summary
SPIFFE prefix matching semantics and implementation
src/nv_config_manager/common/auth.py
SpiffeConfig documentation expanded to require boundary-aware matching (exact prefix or prefix + "/"). identity_from_spiffe now adds groups only when spiffe_id == prefix or spiffe_id.startswith(prefix + "/"), replacing naive startswith(prefix) behaviour.
Test coverage for boundary-aware prefix matching
src/tests/common/test_auth.py
Three new tests in TestSpiffeGroupPrefixes assert (1) sibling namespace does not receive mapped role, (2) exact prefix match grants the mapped role, and (3) overlapping broader and narrower prefixes both grant their respective groups cumulatively.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A careful hop, a carved-out trail,

No sibling slips where boundaries fail.
Exact or followed by a slash so fine,
Roles land only where segments align. 🥕

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: fixing SPIFFE prefix matching to work on path-segment boundaries rather than naive substring matching.
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.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch port/spiffe-prefix-boundary-mr237

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

@dmathur0 dmathur0 force-pushed the port/spiffe-prefix-boundary-mr237 branch from 8af260e to 7df8982 Compare June 1, 2026 22:52

@coderabbitai coderabbitai 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.

Actionable comments posted: 2

🤖 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.

Inline comments:
In `@src/nv_config_manager/common/auth.py`:
- Around line 150-152: The docstring for SpiffeConfig claims that matched
callers get a tracking group "spiffe:<workload_name>" but the implementation in
identity_from_spiffe currently only adds "all" and the mapped groups from
group_prefixes; to fix, either update identity_from_spiffe to also append the
tracking group "spiffe:<workload_name>" (construct workload_name from the
caller's SPIFFE ID path segment and add f"spiffe:{workload_name}" to the
returned groups) or update the SpiffeConfig docstring to remove the mention of
the tracking group so it matches the current behavior; locate SpiffeConfig and
the identity_from_spiffe function to make the corresponding change so contract
and implementation align.

In `@src/tests/common/test_auth.py`:
- Around line 1200-1243: The test test_layered_prefixes_add_both_roles is only
exercising the exact-match branch for the narrower prefix; change the SVID
subject passed to _mock_spiffe_request (the sub= argument) to be a descendant
path (e.g. "spiffe://cluster.local/ns/nv-config-manager/render/worker") instead
of the exact "…/render" so the code path that matches via prefix + "/" is
executed and ensures layered roles accumulate; keep the rest of the call (aud,
token/mock_jwk handling, and assertions on data["groups"]) unchanged.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 4eb33851-624d-4f85-92b8-68fdc94fa2a3

📥 Commits

Reviewing files that changed from the base of the PR and between 6bec6d7 and 8af260e.

📒 Files selected for processing (2)
  • src/nv_config_manager/common/auth.py
  • src/tests/common/test_auth.py

Comment thread src/nv_config_manager/common/auth.py Outdated
Comment thread src/tests/common/test_auth.py
@dmathur0 dmathur0 force-pushed the port/spiffe-prefix-boundary-mr237 branch from 7df8982 to c10a6b0 Compare June 1, 2026 22:59
@daveshm

daveshm commented Jun 24, 2026

Copy link
Copy Markdown

/ok to test c10a6b0

1 similar comment
@dmathur0

Copy link
Copy Markdown
Collaborator Author

/ok to test c10a6b0

@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown

dmathur0 added 2 commits July 2, 2026 14:07
A naive startswith() match let a sibling identity (e.g.
spiffe://td/ns/nv-config-manager-admin) inherit the role mapped to a shorter
prefix (spiffe://td/ns/nv-config-manager). Match now requires an exact prefix
or a '/' boundary, so roles only apply along hierarchical SPIFFE path
segments. Adds sibling/exact/layered regression tests.

Signed-off-by: Davesh Mathur <daveshm@nvidia.com>
identity_from_spiffe() only adds "all" plus configured mapped groups; it
never emits a spiffe:<workload_name> tracking group. Drop that claim from
the SpiffeConfig docstring and INI example so the contract matches the
implementation and operators are not misled.

Signed-off-by: Davesh Mathur <daveshm@nvidia.com>
@dmathur0 dmathur0 force-pushed the port/spiffe-prefix-boundary-mr237 branch from de6b5d0 to 7fe3cf1 Compare July 2, 2026 19:08
The layered-prefix regression matched the narrow prefix exactly, so it
never proved that roles still accumulate when a narrow prefix is hit via
the ``prefix + "/"`` boundary. Push the SVID one segment deeper
(.../render/sa/api) so both mapped prefixes match through the descendant
branch. Addresses CodeRabbit review feedback on #22.

Signed-off-by: Davesh Mathur <daveshm@nvidia.com>
@dmathur0 dmathur0 merged commit 19f843f into main Jul 8, 2026
14 checks passed
@dmathur0 dmathur0 deleted the port/spiffe-prefix-boundary-mr237 branch July 8, 2026 20:06
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.

3 participants