🐛 fix: exec authz SAR error wrap + structured deny reason (#8140)#8141
🐛 fix: exec authz SAR error wrap + structured deny reason (#8140)#8141clubanderson merged 1 commit intomainfrom
Conversation
Addresses three Copilot review comments on merged PR #8139: 1. exec.go authorizePodExec: the SAR-call-failed branch formatted the underlying error with `%v`, so `errors.Is(err, underlyingSARError)` silently returned false despite surrounding comments and the test name implying the underlying error was recoverable. Switched to double-%w (Go 1.20+): `fmt.Errorf("%w: %w", errExecSARFailed, err)` so callers can recover BOTH the sentinel AND the underlying SAR error via errors.Is. 2. exec_test.go TestAuthorizePodExec_SARError: the comment claimed the underlying error was recoverable, but the test only asserted the sentinel. Added `require.ErrorIs(t, err, sentinel)` so the double-%w contract is locked in by a test, not just a comment. 3. exec.go HandleExec deny branch: pre-#8139 logged the SAR deny reason under a structured `"reason"` slog key; #8139 accidentally embedded it only in the error string, breaking operator log queries that filter by the `reason` attribute. To restore the structured field without re-parsing error strings, authorizePodExec now returns `(execAuthzResult, error)` where execAuthzResult.reason is populated ONLY on errExecRBACDenied (empty on every other branch — allow, nil authorizer, missing subject, SAR failure). HandleExec passes authzResult.reason to slog.String("reason", ...) on the deny branch, alongside the wrapped error. Added TestAuthorizePodExec_Deny assertion that res.reason is the exact apiserver reason string (non-empty, distinct from the error), and a new TestAuthorizePodExec_DenyNoReason covering the empty-reason subcase. Non-deny branches assert res.reason stays empty so callers never accidentally log a synthesized reason. No behavior change to the security decision: every fail-closed branch from #8134/#8137/#8139 still denies the exec before the executor is built. The `h.authorizer == nil` typed-nil guard from #8139 is untouched. build/vet/go test ./pkg/api/handlers/... -run Exec: all 13 tests pass. Fixes #8140 Signed-off-by: Andy Anderson <[email protected]>
kubestellar-prow
Bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
✅ Deploy Preview for kubestellarconsole canceled.
|
kubestellar-prow
Bot
added
the
size/L
|
👋 Hey @clubanderson — thanks for opening this PR!
This is an automated message. |
There was a problem hiding this comment.
Pull request overview
This PR is a follow-up fix to the pods/exec RBAC enforcement work, ensuring SubjectAccessReview (SAR) errors are properly double-wrapped for errors.Is recovery and restoring the structured "reason" log attribute on RBAC deny events for operator log querying.
Changes:
- Update
authorizePodExecto return(execAuthzResult, error)so deny reasons can be logged as a structured"reason"attribute. - Fix SAR error wrapping to use double
%wso botherrExecSARFailedand the underlying SAR error are recoverable viaerrors.Is. - Expand unit test coverage to assert the double-wrap contract and the structured deny-reason behavior (including the empty-reason deny case).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/api/handlers/exec.go | Adds execAuthzResult, double-wraps SAR errors, and restores structured "reason" logging for RBAC deny. |
| pkg/api/handlers/exec_test.go | Updates/extends tests to assert errors.Is behavior for SAR errors and validate deny reason propagation via result struct. |
|
Thank you for your contribution! Your PR has been merged. Check out what's new:
Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey |
|
Post-merge build verification passed ✅ Both Go and frontend builds compiled successfully against merge commit |
✅ Post-Merge Verification: passedCommit: |
Summary
Follow-up to merged PR #8139 addressing three Copilot review comments.
pkg/api/handlers/exec.goauthorizePodExecSAR-error branch: switchedfmt.Errorf("%w: %v", ...)to double-%w(Go 1.20+) so callers can recover BOTHerrExecSARFailedAND the underlying SAR error viaerrors.Is. The pre-fix%vform silently broke the recoverable-underlying-error contract the surrounding tests documented.pkg/api/handlers/exec_test.goTestAuthorizePodExec_SARError: the test comment claimed the underlying SAR error was recoverable, but only the sentinel was asserted. Addedrequire.ErrorIs(t, err, sentinel)so the double-%wcontract is locked in by a test assertion rather than a comment.pkg/api/handlers/exec.goHandleExecdeny branch: pre-🐛 fix: exec authz typed-nil guard + test coverage (#8137) #8139 logged the SAR deny reason under a structured"reason"slog key; 🐛 fix: exec authz typed-nil guard + test coverage (#8137) #8139 accidentally moved it into the error string only, breaking operator log queries that filter on thereasonattribute. To restore the structured field cleanly without parsing error strings,authorizePodExecnow returns(execAuthzResult, error)whereexecAuthzResult.reasonis populated ONLY onerrExecRBACDenied(empty on every other branch).HandleExecpassesauthzResult.reasontoslog.String("reason", ...)on deny, alongside the wrapped error.Tests
TestAuthorizePodExec_SARErrornow asserts BOTHerrors.Is(err, errExecSARFailed)ANDerrors.Is(err, sentinel)to lock in the double-wrap.TestAuthorizePodExec_Denynow assertsres.reason == denyReason(non-empty, distinct from the error) — the structured-log contract.TestAuthorizePodExec_DenyNoReasoncovers thereason == ""subcase (apiserver denied but gave no reason string).res.reasonstays empty so callers never accidentally log a synthesized reason.Security
No change to the fail-closed decision. Every branch from #8134/#8137/#8139 still denies the exec before the executor is built. The
h.authorizer == niltyped-nil guard from #8139 is untouched.Test plan
go build ./...passesgo vet ./...passesgo test ./pkg/api/handlers/... -run Exec -count=1— 13/13 passFixes #8140