OPRUN-4601: use resource-based RBAC for lifecycle-server auth#1290
OPRUN-4601: use resource-based RBAC for lifecycle-server auth#1290perdasilva wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a new authentication and authorization HTTP middleware to the lifecycle server: introduces ChangesLifecycle Server Authentication Middleware
Sequence DiagramsequenceDiagram
participant Client
participant AuthMiddleware as Auth Middleware
participant Authenticator as TokenReview Authenticator
participant Authorizer as SubjectAccessReview Authorizer
participant Handler as Inner Handler
Client->>AuthMiddleware: HTTP Request
AuthMiddleware->>Authenticator: AuthenticateRequest()
alt Authentication Error
Authenticator-->>AuthMiddleware: Error
AuthMiddleware-->>Client: 500 Internal Server Error
else Unauthenticated
Authenticator-->>AuthMiddleware: No User
AuthMiddleware-->>Client: 401 Unauthorized
else Authenticated
Authenticator-->>AuthMiddleware: User Identity
AuthMiddleware->>Authorizer: Authorize(user, verb="get", apiGroup="lifecycle.olm.openshift.io", resource="lifecycles")
alt Authorization Error
Authorizer-->>AuthMiddleware: Error
AuthMiddleware-->>Client: 500 Internal Server Error
else Denied or NoOpinion
Authorizer-->>AuthMiddleware: Denied / NoOpinion
AuthMiddleware-->>Client: 403 Forbidden
else Allowed
Authorizer-->>AuthMiddleware: Allowed
AuthMiddleware->>Handler: Forward request
Handler-->>Client: Handler response
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 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 APPROVED This pull-request has been approved by: perdasilva The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
perdasilva
force-pushed
the
lifecycle-server-auth
branch
from
7fc0232 to
cd520b2
Compare
perdasilva
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@perdasilva: This pull request references OPRUN-4601 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/lifecycle-server/auth.go (1)
21-24: ⚡ Quick winKeep these constants unexported.
APIGroupandResourcelook internal to this middleware and the tests can still use them without exporting. Lowercasing them avoids accidentally growing theserverpackage API surface.🤖 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 `@pkg/lifecycle-server/auth.go` around lines 21 - 24, Rename the exported constants APIGroup and Resource to unexported identifiers (e.g., apiGroup and resource) in the auth.go file and update all internal references to those symbols (including unit tests in the same package) so the package API surface doesn't grow unintentionally; ensure no external package code relies on APIGroup/Resource and adjust tests or move them into the same package if necessary.
🤖 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 `@pkg/lifecycle-server/auth.go`:
- Around line 86-92: The SAR call is missing the resource Name so
resourceNames-scoped RBAC can't match; in the authz.Authorize invocation that
constructs authorizer.AttributesRecord (the block that sets User: res.User,
Verb: "get", APIGroup: APIGroup, Resource: Resource, ResourceRequest: true),
extract the package name from the request path (the {package} segment of
/api/{version}/lifecycles/{package} using req.URL.Path parsing or the router
vars) and set AttributesRecord.Name to that package string before calling
authz.Authorize so the authorization is evaluated against the specific lifecycle
resource.
---
Nitpick comments:
In `@pkg/lifecycle-server/auth.go`:
- Around line 21-24: Rename the exported constants APIGroup and Resource to
unexported identifiers (e.g., apiGroup and resource) in the auth.go file and
update all internal references to those symbols (including unit tests in the
same package) so the package API surface doesn't grow unintentionally; ensure no
external package code relies on APIGroup/Resource and adjust tests or move them
into the same package if necessary.
🪄 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: 269ceef6-45fa-480f-9ce0-030e8cb99009
📒 Files selected for processing (4)
cmd/lifecycle-server/start.gogo.modpkg/lifecycle-server/auth.gopkg/lifecycle-server/auth_test.go
|
PR description says:
But I don't see those in the changeset. iirc, the manifests directory is generated, and I had to trace where to put the new lifecycle-controller manifests to ensure the generation step puts them in the CVO manifests directory. |
sorry about that - I've updated the description. Those were either wrong or breadcrumbs for the client-side RBAC requirements |
|
@perdasilva: all tests passed! 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. |
Replace the controller-runtime metrics auth filter with a custom resource-based authorization middleware. The metrics filter performed nonResourceURL-based SubjectAccessReviews, which meant the default system:discovery ClusterRole (granting GET on /api/*) allowed any authenticated user to access the lifecycle-server API. The new middleware creates SubjectAccessReviews with ResourceAttributes (apiGroup: lifecycle.olm.openshift.io, resource: lifecycles, verb: get) so access requires an explicit ClusterRoleBinding to the new lifecycle-server-consumer ClusterRole. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Per G. da Silva <pegoncal@redhat.com>
perdasilva
force-pushed
the
lifecycle-server-auth
branch
from
cd520b2 to
11ba9c5
Compare
3 participants
Summary
filters.WithAuthenticationAndAuthorization) with a custom resource-based authorization middleware for the lifecycle-server APIsystem:discoveryClusterRole (granting GET on/api/*) allowed any authenticated user to bypass authorizationResourceAttributes(apiGroup: lifecycle.olm.openshift.io,resource: lifecycles,verb: get) so access requires an explicit ClusterRoleBinding to the newlifecycle-server-consumerClusterRoleChanges
pkg/lifecycle-server/auth.go— New auth middleware usingDelegatingAuthenticatorConfig+DelegatingAuthorizerConfigwith resource-basedAttributesRecordpkg/lifecycle-server/auth_test.go— 7 unit tests covering all auth code paths (401, 403, 500, 200, DecisionNoOpinion, attributes verification, reason non-leakage)cmd/lifecycle-server/start.go— Swap metrics filter for newNewAuthFilterTest plan
go build ./cmd/lifecycle-server/...compilesgo test ./pkg/lifecycle-server/...— all tests passgo vet ./pkg/lifecycle-server/... ./cmd/lifecycle-server/...— clean🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Tests
Chores