fix(auth): match SPIFFE prefixes on path-segment boundaries#22
Open
dmathur0 wants to merge 1 commit into
Open
Conversation
dmathur0
requested review from
jojung1,
polarweasel,
ryanheffernan,
spidercensus,
susanhooks and
zsblevins
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughReplaces naive SPIFFE prefix matching with boundary-aware semantics (exact match or prefix + "/") in documentation and in ChangesSPIFFE Prefix Matching Boundary Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
dmathur0
force-pushed
the
port/spiffe-prefix-boundary-mr237
branch
from
8af260e to
7df8982
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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
📒 Files selected for processing (2)
src/nv_config_manager/common/auth.pysrc/tests/common/test_auth.py
Comment on lines
+150
to
+152
| ``group_prefixes`` maps SPIFFE ID *path-segment* prefixes to group names. | ||
| When a caller's SPIFFE ID matches a prefix the mapped group is added to | ||
| its identity (in addition to ``spiffe:<workload_name>``). |
There was a problem hiding this comment.
Keep the SpiffeConfig contract aligned with the implementation.
This docstring says matched callers also receive spiffe:<workload_name>, but identity_from_spiffe() currently only adds "all" plus configured mapped groups. Either add that tracking group in code or remove the claim here so operators are not misled.
🤖 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 `@src/nv_config_manager/common/auth.py` around lines 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.
Comment on lines
+1200
to
+1243
| def test_layered_prefixes_add_both_roles(self, rsa_keypair, make_jwt): | ||
| """Per-service narrow prefix layers on top of a coarser one. | ||
|
|
||
| A SPIFFE ID under ``…/nv-config-manager/render`` must pick up BOTH the | ||
| broad ``nv-config-manager`` role (from the ``…/nv-config-manager`` | ||
| prefix, matched via path-segment boundary) and the narrow | ||
| ``nv-config-manager-render`` role (from the exact | ||
| ``…/nv-config-manager/render`` prefix). This is what enables | ||
| progressive scoping: keep coarse roles working while introducing | ||
| tighter per-service roles to gate sensitive endpoints. | ||
| """ | ||
| cp = _make_config( | ||
| **{ | ||
| "auth.spiffe": { | ||
| "jwks_uri": "https://spire-server:8443/keys", | ||
| "audiences": "spiffe://cluster.local", | ||
| }, | ||
| "auth.spiffe.groups": { | ||
| "spiffe://cluster.local/ns/nv-config-manager": "nv-config-manager", | ||
| "spiffe://cluster.local/ns/nv-config-manager/render": "nv-config-manager-render", | ||
| }, | ||
| } | ||
| ) | ||
| auth_mod._auth_config = load_auth_config(cp) | ||
|
|
||
| token, mock_jwk = self._mock_spiffe_request( | ||
| rsa_keypair, | ||
| make_jwt, | ||
| sub="spiffe://cluster.local/ns/nv-config-manager/render", | ||
| aud="spiffe://cluster.local", | ||
| ) | ||
|
|
||
| app = self._make_app() | ||
| client = TestClient(app) | ||
| with patch("nv_config_manager.common.auth._get_jwks_client") as mock_get_client: | ||
| mock_client = MagicMock() | ||
| mock_client.get_signing_key_from_jwt.return_value = mock_jwk | ||
| mock_get_client.return_value = mock_client | ||
| resp = client.get("/whoami", headers={"Authorization": f"Bearer {token}"}) | ||
|
|
||
| data = resp.json() | ||
| assert "nv-config-manager" in data["groups"] | ||
| assert "nv-config-manager-render" in data["groups"] | ||
|
|
There was a problem hiding this comment.
Exercise the descendant branch in the layered-prefix regression.
This test currently matches the narrower prefix exactly, so it never proves that layered roles still accumulate when the narrower mapping is hit via prefix + "/". Switching the SVID to something below that prefix would cover the second branch and make this regression harder to bypass.
Suggested tweak
- sub="spiffe://cluster.local/ns/nv-config-manager/render",
+ sub="spiffe://cluster.local/ns/nv-config-manager/render/sa/api",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_layered_prefixes_add_both_roles(self, rsa_keypair, make_jwt): | |
| """Per-service narrow prefix layers on top of a coarser one. | |
| A SPIFFE ID under ``…/nv-config-manager/render`` must pick up BOTH the | |
| broad ``nv-config-manager`` role (from the ``…/nv-config-manager`` | |
| prefix, matched via path-segment boundary) and the narrow | |
| ``nv-config-manager-render`` role (from the exact | |
| ``…/nv-config-manager/render`` prefix). This is what enables | |
| progressive scoping: keep coarse roles working while introducing | |
| tighter per-service roles to gate sensitive endpoints. | |
| """ | |
| cp = _make_config( | |
| **{ | |
| "auth.spiffe": { | |
| "jwks_uri": "https://spire-server:8443/keys", | |
| "audiences": "spiffe://cluster.local", | |
| }, | |
| "auth.spiffe.groups": { | |
| "spiffe://cluster.local/ns/nv-config-manager": "nv-config-manager", | |
| "spiffe://cluster.local/ns/nv-config-manager/render": "nv-config-manager-render", | |
| }, | |
| } | |
| ) | |
| auth_mod._auth_config = load_auth_config(cp) | |
| token, mock_jwk = self._mock_spiffe_request( | |
| rsa_keypair, | |
| make_jwt, | |
| sub="spiffe://cluster.local/ns/nv-config-manager/render", | |
| aud="spiffe://cluster.local", | |
| ) | |
| app = self._make_app() | |
| client = TestClient(app) | |
| with patch("nv_config_manager.common.auth._get_jwks_client") as mock_get_client: | |
| mock_client = MagicMock() | |
| mock_client.get_signing_key_from_jwt.return_value = mock_jwk | |
| mock_get_client.return_value = mock_client | |
| resp = client.get("/whoami", headers={"Authorization": f"Bearer {token}"}) | |
| data = resp.json() | |
| assert "nv-config-manager" in data["groups"] | |
| assert "nv-config-manager-render" in data["groups"] | |
| def test_layered_prefixes_add_both_roles(self, rsa_keypair, make_jwt): | |
| """Per-service narrow prefix layers on top of a coarser one. | |
| A SPIFFE ID under ``…/nv-config-manager/render`` must pick up BOTH the | |
| broad ``nv-config-manager`` role (from the ``…/nv-config-manager`` | |
| prefix, matched via path-segment boundary) and the narrow | |
| ``nv-config-manager-render`` role (from the exact | |
| ``…/nv-config-manager/render`` prefix). This is what enables | |
| progressive scoping: keep coarse roles working while introducing | |
| tighter per-service roles to gate sensitive endpoints. | |
| """ | |
| cp = _make_config( | |
| **{ | |
| "auth.spiffe": { | |
| "jwks_uri": "https://spire-server:8443/keys", | |
| "audiences": "spiffe://cluster.local", | |
| }, | |
| "auth.spiffe.groups": { | |
| "spiffe://cluster.local/ns/nv-config-manager": "nv-config-manager", | |
| "spiffe://cluster.local/ns/nv-config-manager/render": "nv-config-manager-render", | |
| }, | |
| } | |
| ) | |
| auth_mod._auth_config = load_auth_config(cp) | |
| token, mock_jwk = self._mock_spiffe_request( | |
| rsa_keypair, | |
| make_jwt, | |
| sub="spiffe://cluster.local/ns/nv-config-manager/render/sa/api", | |
| aud="spiffe://cluster.local", | |
| ) | |
| app = self._make_app() | |
| client = TestClient(app) | |
| with patch("nv_config_manager.common.auth._get_jwks_client") as mock_get_client: | |
| mock_client = MagicMock() | |
| mock_client.get_signing_key_from_jwt.return_value = mock_jwk | |
| mock_get_client.return_value = mock_client | |
| resp = client.get("/whoami", headers={"Authorization": f"Bearer {token}"}) | |
| data = resp.json() | |
| assert "nv-config-manager" in data["groups"] | |
| assert "nv-config-manager-render" in data["groups"] |
🤖 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 `@src/tests/common/test_auth.py` around lines 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.
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>
dmathur0
force-pushed
the
port/spiffe-prefix-boundary-mr237
branch
from
7df8982 to
c10a6b0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
Validation
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 defaulttest_pathfor the full suite,or narrow it only while debugging.
Passing Kind Integration run:
Checklist
CONTRIBUTING.md.docs screenshots, or Helm/rendered outputs.
Summary by CodeRabbit
Bug Fixes
Tests