feat(kubernetes): add AllowedAPIGroups support to toolset API - #1308
feat(kubernetes): add AllowedAPIGroups support to toolset API#1308lyarwood wants to merge 1 commit into
Conversation
|
👋 Heads up — this pull request changes files owned by @aljesusg @josunect @ksimon1 @mjudeikis. You are listed as an owner of one or more of the changed areas in |
c6f6694 to
ee51c3e
Compare
ee51c3e to
b7f1a34
Compare
codingben
left a comment
There was a problem hiding this comment.
/lgtm
Can we get this PR merged to unblock others?
|
@lyarwood would you please fix the formatting so the tests are happy? |
b7f1a34 to
edfebb8
Compare
|
/lgtm |
2uasimojo
left a comment
There was a problem hiding this comment.
How does this interact with denied resources?
I don't understand how this will ever allow core resources to come through.
| } | ||
|
|
||
| func collectAllowedAPIGroups(toolsetNames []string) []string { | ||
| seen := make(map[string]struct{}) |
There was a problem hiding this comment.
Consider using Set instead of rolling your own.
| } | ||
| return nil, &api.ValidationError{ | ||
| Code: api.ErrorCodeResourceNotFound, | ||
| Message: fmt.Sprintf("Resource %s does not exist in the cluster", api.FormatResourceName(&gvr)), |
There was a problem hiding this comment.
I know there's a philosophy in security-land that distinguishing between, "You're not allowed to do that," and "Daaah, I have no idea what you're talking about," supposedly gives up some kind of information that could be exploited in theory. Is that really applicable here? I can see the phone ringing with, "WDYM doesn't exist in the cluster? oc get shows it right here!"
As for core API resources (pods, services, etc.) — their group is the empty string |
Right, this is my point: the default behavior appears to be to exclude (blacklist) core resources. Which might make sense for kubevirt, but is definitely Bad™ for e.g. the "core" toolset. I guess I would have expected a design like:
|
Add an AllowedAPIGroupsProvider interface that allows toolsets to explicitly declare API groups that should bypass REST mapper validation in the AccessControlRoundTripper. This enables toolsets to use virtual API groups (e.g. subresources.kubevirt.io) that are not present in standard API discovery without weakening the security model. The kubevirt toolset declares subresources.kubevirt.io as an allowed group, unblocking pause/unpause and guest agent subresource operations. Resolves: containers#1307 Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Lee Yarwood <lyarwood@redhat.com>
edfebb8 to
c9bbe2c
Compare
|
Closing in favour of #1361, which solves the same problem at the round tripper level via a discovery fallback — no toolset changes required. I've tested the pause/unpause use case from #1306 on top of #1361 and it works correctly on OCP. There is one issue with #1361 on clusters that use aggregated discovery (OCP 4.x) that I've commented on there. |
Summary
Add an
AllowedAPIGroupsProviderinterface that allows toolsets to explicitly declare API groups that should bypass REST mapper validation in theAccessControlRoundTripper. This addresses the reviewer feedback from @Cali0707 and @manusa that exceptions to the round tripper's security model should be explicit and declared through the toolset API, not a blanket pass-through.How it works
Toolsetinterface gainsGetAllowedAPIGroups() []stringAllowedAPIGroupsProviderinterface follows theDeniedResourcesProviderpattern exactlyStaticConfigAccessControlRoundTripperchecks allowed groups before rejecting requests to API groups not in the REST mapperdenied_resourcesstill takes precedence for groups that ARE in the REST mappercollectAllowedAPIGroupsandisAPIGroupAllowedto prevent accidental matches against core API resources.KubeVirt use cases
This PR adds the infrastructure. The kubevirt toolset currently returns
nilfromGetAllowedAPIGroups()— concrete use cases are in follow-up PRs:subresources.kubevirt.iofor guest agent queries (guestosinfo,filesystemlist,userlist,interfacelist) and pause/unpause endpointsvm_create_from_templatetool usingsubresources.template.kubevirt.io(should add its group via the same mechanism rather than bypassing the round tripper)Design notes
DeniedResourcesProviderfor consistency.AllowedAPIGroupshastoml:"-"so it cannot be set via config files — it is computed purely from toolset code.Testing
accesscontrol_round_tripper_test.go:collectAllowedAPIGroupscovering nil input, unknown toolsets, dedup, and empty-string filteringRelated