feat(config): deny resources by using RESTMapper as an interceptor #149

manusa · 2025-06-23T13:48:50Z

Fixes #132

This approach ensures that resources in the deny list are always processed regardless of the implementation.

The RESTMapper takes care of verifying that the requested Group Version Kind complies with the deny list while checking for the REST endpoint.

/cc @ardaguclu

ardaguclu

This change looks nice. Dropped a few comments.

ardaguclu · 2025-06-30T07:00:01Z

pkg/kubernetes/accesscontrol_restmapper.go

+
+type AccessControlRESTMapper struct {
+	delegate     *restmapper.DeferredDiscoveryRESTMapper
+	staticConfig *config.StaticConfig // TODO: maybe just store the denied resource slice


In my opinion, we can pass staticConfig in here instead of denied resource slice. Maybe in the future, we want to extend this based on allowed resources similar to denied resources.

I think, I'm leaning towards your TODO comment. We can pass only denyList instead of an entire staticConfig (as it includes different type of resources)

That's why I left a TODO comment, I think that we can leave it and reconsider it once we have more pieces of the puzzle.

ardaguclu · 2025-06-30T07:03:07Z

pkg/kubernetes/accesscontrol_restmapper.go

+// isAllowed checks the resource is in denied list or not.
+// If it is in denied list, this function returns false.
+func (a AccessControlRESTMapper) isAllowed(gvk *schema.GroupVersionKind) bool {
+	if a.staticConfig == nil {


This logic is true. But if staticConfig is accidentally passed as nil, this would mean we allow all resources. Just brainstorming, would it be too disruptive if we return false, if staticConfig is nil? or can we return error in NewAccessControlRESTMapper if staticConfig is nil?. WDYT?

Note: the method is the same as the original method, I just moved it here.

It's very likely that this method will be deeply refactored in the future.

Currently, staticConfig can be nil (in fact will be nil unless a config is explicitly loaded.

If we go with #151 (comment) or similar, then probably staticConfig will always have a value and we can probably change this.

pkg/kubernetes/kubernetes.go

ardaguclu · 2025-06-30T12:54:42Z

/lgtm

This approach ensures that resources in the deny list are **always** processed regardless of the implementation. The RESTMapper takes care of verifying that the requested Group Version Kind complies with the deny list while checking for the REST endpoint.

ardaguclu · 2025-07-01T05:52:36Z

pkg/kubernetes/accesscontrol.go

+}
+
+func isNotAllowedError(gvk *schema.GroupVersionKind) error {
+	return fmt.Errorf("resource not allowed: %s", gvk.String())


(not a blocker for this PR):
We have to return 403: Forbidden HTTP Status. But I'm not really sure that mcp library we use support this. If it doesn't support returning http code we have, we'll have major issue in oauth side as well.

It just simply sets http 200 statically https://github.com/mark3labs/mcp-go/blob/1eddde7bd69b760f745a1b4064969cffcf97e935/server/streamable_http.go#L347 :(

Yes, I don't think the library (or even the protocol) is prepared for fine-grained operation status codes yet.
As I understand it, the protocol negotiates authentication and authorization first, and then enables

ardaguclu · 2025-07-01T05:56:23Z

pkg/kubernetes/accesscontrol_clientset.go

+	return a.delegate.CoreV1().Pods(namespace), nil
+}
+
+func (a *AccessControlClientset) PodsExec(namespace, name string) (*rest.Request, error) {


What about moving createExecutor also in here and this function returns remotecommand.Executor?

This should be done now, Schema and ParameterCodec are now reused from client-go globals

ardaguclu

I really like the way of how this PR handles the problem. Dropped a few non-blocker comments. Other than that this looks good to me.

ardaguclu · 2025-07-01T12:22:24Z

pkg/kubernetes/resources.go

-	response, err := k.manager.clientSet.AuthorizationV1().SelfSubjectAccessReviews().Create(ctx, &authv1.SelfSubjectAccessReview{
+	accessReviews, err := k.manager.accessControlClientSet.SelfSubjectAccessReviews()
+	if err != nil {
+		return false


ardaguclu · 2025-07-01T12:23:07Z

pkg/kubernetes/resources.go

 	url = append(url, gvr.Resource)
 	var table metav1.Table
-	err := k.manager.clientSet.CoreV1().RESTClient().
+	err := k.manager.discoveryClient.RESTClient().


I think, now we support all the resources

Not sure what you mean with this.
AFAIU, before all resources were supported too, this was/is just a lazy way to pass share the RESTClient.

ardaguclu · 2025-07-01T12:25:42Z

pkg/kubernetes/resources.go

 			"application/json",
 		}, ",")).
 		AbsPath(url...).
-		SpecificallyVersionedParams(&options.ListOptions, k.manager.parameterCodec, schema.GroupVersion{Version: "v1"}).


I don't want to block this PR for this. So we can handle it in a subsequent PR. Can we now safely remove parameterCodec from manager? or we still require it?

parameterCodec should no longer be in Manager.

ardaguclu · 2025-07-01T12:27:27Z

pkg/kubernetes/kubernetes.go

+	CloseWatchKubeConfig CloseWatchKubeConfig
 }

+var Scheme = scheme.Scheme


(not a blocker for this PR)
It is unfortunate that we are tied to scheme.Scheme (as cluster is a dynamic environment, we'll miss some new resources that is not defined in our static scheme). But I understand the reason of this. I'd recommend moving this to its own scheme.go file.

Let's merge this in first and reiterate on that.
In this case, I just needed a quick way to access the Scheme and ParameterCodec. I recalled you mentioned about using a global Scheme at some other PR so went on this direction.

manusa · 2025-07-01T12:43:16Z

I really like the way of how this PR handles the problem

I think it's a good start, but I'm still not fully convinced about the solution.
I like the RESTMapper because it ensures the access control is verified for absolutely each request.
However, this is not possible for the ClientSets. The only way I envisioned to do this was by using a RoundTripper, but it added a lot of complexity and didn't cover the problem completely.

For now, I think we need to keep adding Denied Tests for each new feature we implement.

ardaguclu · 2025-07-01T12:43:39Z

/lgtm

manusa force-pushed the feat/access-control-rest-mapper branch from bcf0d30 to d6289c0 Compare June 23, 2025 13:53

manusa added this to the 0.1.0 milestone Jun 25, 2025 — with automated-tasks

ardaguclu reviewed Jun 30, 2025

View reviewed changes

manusa added 4 commits July 1, 2025 05:53

feat(config): provide a limited clientset which check access

78d92f2

review: addressed PR comments

8a0681a

feat(config): provide a limited metrics clientset to check access

9a6ebe8

manusa force-pushed the feat/access-control-rest-mapper branch from 1711402 to 9a6ebe8 Compare July 1, 2025 05:27

manusa marked this pull request as ready for review July 1, 2025 05:27

manusa requested a review from ardaguclu July 1, 2025 05:27

ardaguclu reviewed Jul 1, 2025

View reviewed changes

This was referenced Jul 1, 2025

Serve sse and streamable from a single port #153

Merged

Introduce enabled/disabled tool list in configuration file #155

Merged

review: addressed PR comments regarding pods_exec

8b39eea

manusa requested a review from ardaguclu July 1, 2025 12:14

ardaguclu reviewed Jul 1, 2025

View reviewed changes

manusa merged commit af2a8cd into main Jul 1, 2025
5 checks passed

manusa deleted the feat/access-control-rest-mapper branch July 1, 2025 12:44

feat(config): deny resources by using RESTMapper as an interceptor #149

feat(config): deny resources by using RESTMapper as an interceptor #149

Uh oh!

Conversation

manusa commented Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ardaguclu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ardaguclu commented Jun 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ardaguclu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

manusa commented Jul 1, 2025

Uh oh!

ardaguclu commented Jul 1, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

manusa commented Jun 23, 2025 •

edited

Loading