e2e: verify spire-agent runs as root with namespace GID range#142
e2e: verify spire-agent runs as root with namespace GID range#142sayak-redhat wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughA new e2e test checks SPIRE Agent pod identity in OpenShift by reading process status inside the container, asserting UID 0, and verifying a supplemental GID falls within the namespace range. Shared helpers and a container-name constant support the test. ChangesSPIRE Agent OpenShift Security Test
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sayak-redhat The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![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 `@test/e2e/utils/utils.go`:
- Around line 283-293: The strconv.Atoi function accepts negative numbers, but
supplemental group ranges with negative values like -1/100 are invalid and can
cause incorrect GID namespace handling. After parsing the start value from
parts[0] and the size value from parts[1] in the current error handling blocks,
add validation checks to ensure both start and size are non-negative (greater
than or equal to 0), and return an error with a descriptive message if either
value is negative.
- Around line 231-267: The parsing logic in this function has a vulnerability
where the uid variable defaults to 0, so if the id command output is malformed
and lacks a uid= token, the function will still return uid as 0, potentially
causing incorrect assertions in tests. Add boolean flags to track whether the
uid and groups fields were actually found during parsing of the output. After
the loop completes, check these flags and return an error if the uid field was
not found in the parsed output, ensuring that malformed id command output is
properly rejected rather than defaulting to values that might satisfy root
assertions.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 21fce4db-fc5d-45be-b13f-7f6d01881c16
📒 Files selected for processing (3)
test/e2e/e2e_test.gotest/e2e/utils/constants.gotest/e2e/utils/utils.go
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/e2e_test.go (1)
321-322: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the stale assertion message.
The failure text still says
"id" output, but this test now parses/proc/self/status. Keeping the old wording will send the next failure in the wrong direction.🤖 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 `@test/e2e/e2e_test.go` around lines 321 - 322, The assertion message in the e2e test is stale and still references "id" output even though the logic now reads from /proc/self/status. Update the failure text in the relevant Expect(inRange).To(BeTrue()) call so it matches the current source of the supplemental GID data, using the same test block in e2e_test.go to keep the message aligned with the parsing logic.
🤖 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 `@test/e2e/e2e_test.go`:
- Around line 283-287: The test is inspecting the exec helper process instead of
the SPIRE agent process, so update the status read in the e2e test to target the
agent’s actual PID rather than /proc/self/status. In the block that uses
utils.ExecInPod and utils.ParseProcStatusUIDGIDs, change the command to read the
running agent’s status (for example /proc/1/status if the agent is PID 1 in the
container, or resolve the agent PID first) so the assertion reflects the real
agent credentials.
---
Outside diff comments:
In `@test/e2e/e2e_test.go`:
- Around line 321-322: The assertion message in the e2e test is stale and still
references "id" output even though the logic now reads from /proc/self/status.
Update the failure text in the relevant Expect(inRange).To(BeTrue()) call so it
matches the current source of the supplemental GID data, using the same test
block in e2e_test.go to keep the message aligned with the parsing logic.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5216c478-a415-4796-9c20-b628ad0af586
📒 Files selected for processing (2)
test/e2e/e2e_test.gotest/e2e/utils/utils.go
sayak-redhat
force-pushed
the
test-verified
branch
from
11c5714 to
f5302ee
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
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 `@test/e2e/e2e_test.go`:
- Around line 283-318: The `/proc/1/status` read in the command list under the
agent PID inspection block uses a shell-specific `$(<...)` form that is not
POSIX-portable. Update the commands used by `utils.ExecInPod` to rely on POSIX
`sh` syntax for reading the file so it works in BusyBox/dash-based images, while
keeping the existing fallback loop, error aggregation, and
`ParseProcStatusUIDGIDs` flow unchanged.
In `@test/e2e/utils/utils.go`:
- Around line 297-313: The supplemental range parsing in the annotation parser
currently validates start and size separately, but still allows an overflow when
the end is derived later. Update the range parsing helper that returns start and
size to also validate the combined end using the parsed values before returning,
rejecting cases where start + size would overflow int or wrap negative. Keep the
existing start/size checks, and make the error path explicit in the parser used
by the e2e supplemental group membership check.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 77301dd2-34b4-43c5-b573-f358d36a78d7
📒 Files selected for processing (3)
test/e2e/e2e_test.gotest/e2e/utils/constants.gotest/e2e/utils/utils.go
✅ Files skipped from review due to trivial changes (1)
- test/e2e/utils/constants.go
sayak-redhat
force-pushed
the
test-verified
branch
from
b455462 to
3d0a8db
Compare
Replace the buggy pod-spec-based supplemental GID test with a runtime verification test that execs into the spire-agent container and reads /proc/self/status to verify: - Process runs as root (UID 0) - Process has root group (GID 0) in Groups - Process has supplemental GID within OpenShift namespace range The test is placed after all operand installations to ensure stable verification when the system is fully deployed. Changes: - Add GetReadySpireAgentPod() utility to find a ready spire-agent pod - Add GetProcStatusFromPod() utility to exec and parse /proc/self/status - Replace buggy test with runtime verification test - Move test after ZeroTrustWorkloadIdentityManager aggregation check
sayak-redhat
force-pushed
the
test-verified
branch
from
3d0a8db to
02fb21d
Compare
|
@sayak-redhat: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
1 participant
Summary by CodeRabbit
0and that at least one supplemental GID falls within the OpenShift SCCsupplemental-groupsrange for the operator namespace./proc/*/statuscontent and parsestart/sizesupplemental group annotation values.