Add webhook checks for NamespacedRules

[JonCrowther]

NOTE: Currently uses my local rancher changes because it requires work currently in a PR

Issue: rancher/rancher#42216

Problem

Follow up work from: rancher/rancher#43018
There are 2 keys changes. First we add escalation checks for the new NamespacedRules field of GlobalRoles. Second we add a check to prevent modification/removal of the gr-owner and grb-owner labels.

Solution

Added two new validation webhooks that select based on the gr-owner and grb-owner labels.
Added a check in the the GlobalRole validation to ensure there is no escalation.

CheckList

• Test
• Docs

[JonCrowther]

noticed a typo while looking in this file

[eliyamlevy]

From what I know and have seen this looks pretty good to me. I am still not too familiar with the role/rolebindings/globalrole/globalrolebindings objects but if the validators and mutators are all accounted for I approve. Definitely would have to have someone more familiar with the resources sign off as well.

[MbolotSuse]

Overall comment:

• GRB Docs also need to be updated.

Nothing too big, just some various issues to consider.

[tomleb]

Seems good but I would like to see at least one unit tests handling multiple namespaces without the need for escalation (say nsA with rules A and nsB with rules B).

[MbolotSuse]

A couple of minor nits as well as some small issues:

• There are other criteria for invalid rules in addition to verbs. I'd recommend including them all.
• Sorry to go back on you, but I think that the check to see if the namespace exists might do more harm then good.

[MbolotSuse]

Couple of changes on the escalation checking logic, and some minor nits.

[MbolotSuse]

I appreciate what you are trying to do with this function, but I think it's unnecessary. The auth package provides an easily usable interface to check if a user has a given verb - I think this additional abstraction is adding complexity in an are where it can be dangerous (permission evaluation).

[tomleb]

The code becomes more complex if we want to

1. Return all possible validation error instead of just the first one encountered
2. Want to do an escalation check whenever we find an error
3. Only do an escalation once

Caching whether we've done the escalation check and its result makes it simpler. But I realize that right now this checker:

• isn't really caching the result, but just defaulting to false if it's already been called
• is named in a way that doesn't really describes it as a cached result

I think renaming the function (and changing its logic a bit) would make it clearer.

I think this additional abstraction is adding complexity in an are where it can be dangerous

I'd argue that no abstraction here was more complex. Do you think the proposed changes above would make it less complex? Or are you opposed in a caching behavior for this area of the code?

EDIT: Caching the result is ill-defined when an error can be returned with the SAR request. It's not clear if we should cache the returned error as well, or just return false if there was an error. Heh.

[MbolotSuse]

The code becomes more complex if we want to

The first requirement is more fungible than the others, but sure, there is definitely some extra complexity introduced.

Caching whether we've done the escalation check and its result makes it simpler

The issue here is that this code doesn't effectively handle that.

Do you think the proposed changes above would make it less complex? Or are you opposed in a caching behavior for this area of the code?

I don't think that the proposed changes reduce complexity, particularly with the issues that it introduces. I'm not opposed to making sure that the escalate verb is checked for at most one time, but I don't think this abstraction solves that problem in a secure fashion.

I think that maintaining the state in the Admit function with two bools (and possibly using a side function to do the debouncing/caching) is a better option. However, I'm open to discussion.

[tomleb]

using a side function to do the debouncing/caching

Can you expand on this? Sounds like something similar to above but more "local" to the code that needs it.

[MbolotSuse]

Sure, I'm thinking something like:

func (a *admitter) Admit(request *admission.Request) (*admissionv1.AdmissionResponse, error) {
// some other code before
var hasEscalate, escalateChecked bool
err = auth.ConfirmNoEscalation(request, clusterRules, "", a.grbResolver)
if err != nil {
	hasEscalate, escErr := a.hasEscalate(hasEscalate, escalateChecked, request, newGR.Name)
	if err != nil {
		return nil, escErr
	}
	escalateChecked = true
	if hasEscalate {
		return admission.ResponseAllowed(), nil
	}
	returnError = errors.Join(returnError, err)
}

// some other code after
}

func (a *admitter) hasEscalate(alreadyChecked bool, prevAnswer bool, request *admission.Request, grName string) (bool, error) {
	if alreadyChecked {
		return prevAnswer, nil
	}
	hasVerb, err := auth.RequestUserHasVerb(request, gvr, a.sar, escalateVerb, grName, "")
	if err != nil {
		return false, fmt.Errorf("unable to check if user has escalate: %w", err)
	}
	return hasVerb, nil
}

This has the following benefits over the current approach:

• The specific requirements of the GR/GRB validator (i.e. escalate is only checked once, but we only care about that one resource for any given request) are kept internal to that validator
• The state of what was called and when is kept inside the admit function, which is directing the flow and maintaining the state for this call (rather than in another call elsewhere

This is just my initial answer, if you see other ways to structure the side functions I'm also open to that.

[MbolotSuse]

Since GlobalRoles are cluster-scoped resources, you want to check if the user has "Escalate" at the cluster scope. Otherwise, users could take advantage of odd rbac configurations to give escalate in one namespace but not others, which we want to avoid.

[tomleb]

Could we add a test here to make the current implementation fail so that we don't introduce this mistake in the future?

[tomleb]

@MbolotSuse 's got a couple of good comments. Nothing to add on top.

[MbolotSuse]

Couple of comments on the new verb checker.

[MbolotSuse]

A couple of overall comments on this struct:

• From what I've heard, typically in go you want to avoid constructors and allow objects to be constructed directly. @KevinJoiner might feel more strongly than I do on that point, and may have an opinion on if this is permissible in this case.
• It's a bit odd to have an unexported type, which doesn't implement an interface, returned from an exported function. I think your CI is failing because of this behavior (and an associated revive check) so you will probably need to adjust this
• I'd recommend emphasizing in your comment the limitations of this struct. Namely:
  • Each admitter must request a new verbChecker for each request.
  • Verb checkers must not be shared across different admitters.
  • If the caller needs to change what it is checking (regardless of if it's a different verb, resource name, or resourceType), then they need to make a separate verb checker.
    The reason I'm emphasizing these limitations is that this struct is used for access control, so a bug here is likely to turn into a CVE. We need to be very clear here to avoid issues.

[MbolotSuse]

Nit:

Suggested change

	// HasVerb returns if the request have the overrideVerb. Defaults to false if it hasn't been checked.
	// HasVerb returns if the request has the overrideVerb. Defaults to false if it hasn't been checked.

[MbolotSuse]

Couple of small changes on the new CachedVerbChecker.

[MbolotSuse]

One comment on an unhandled error. Outside of that looks good.