[DNM] Do not allow CRTB and PRTB names to be longer than 63 characters

[maxsokolovsky]

Issue: rancher/rancher#33795

This PR adds validation to CRTB and PRTB names.
The webhook will ensure that a combined name (projectName_bindingName for PRTBs and clusterName_bindingName for CRTBs) will not exceed 63 characters.

[cmurphy]

We control the label value here https://github.com/rancher/rancher/blob/41b2cf3ab63fb8b3b90076a29cecb092bdc8d83d/pkg/controllers/managementuser/rbac/handler_base.go#L540

We have a widely used safe helper function for this here: https://github.com/rancher/wrangler/blob/6e3c8d7bc840506f33bd75aaa651f7c92d509d73/pkg/name/name.go#L52

Rancher is the thing causing the problem, I would prefer we address this in rancher instead.

[maxsokolovsky]

@cmurphy, thanks for the links! I agree with you, but what if something else starts to cause the problem - some other part of the code?
That way, the webhook will be the single main gatekeeper for everyone.

[cmurphy]

You could make the same hypothetical for some other resource. What if we're basing labels off of some configmap name? Some cluster name? Should we add a validator for every conceivable resource to make sure their names are 63-N characters?

[maxsokolovsky]

Actually... this does sound like a job of either K8s built-in validation or webhook's.

[KevinJoiner]

You could make the same hypothetical for some other resource. What if we're basing labels off of some configmap name? Some cluster name? Should we add a validator for every conceivable resource to make sure their names are 63-N characters?

I have 2 main thoughts that I keep coming back to.

1. I agree that validating the length of a resource's name because we are using it as a label is a slippery slope. Though I think the core problem is relying on the projectName as a label value. If we do not need the label value, I would instead be in favor of truncating the name when creating the label for our rolebinding.

2. Alternatively If Rancher relies on using the name of CRTB/PRTB or any other resource as a label value, then we need to have validation either in K8s via CRD definitions or rancher-webhook. I fall on this side because we can not always guarantee that Rancher will be the one creating new resources.

[maxsokolovsky]

@cmurphy, @KevinJoiner, what do you say - should I change the solution and add validation in Rancher instead of webhook?

[cmurphy]

If we do not need the label value, I would instead be in favor of truncating the name when creating the label for our rolebinding.

We do need the label value, it is used for determining whether to delete a rolebinding:

https://github.com/rancher/rancher/blob/aa7bea9baa40eb8eb1d7166cf37e0b15aaa81ed5/pkg/controllers/managementuser/rbac/namespace_handler.go#L204-L209
https://github.com/rancher/rancher/blob/aa7bea9baa40eb8eb1d7166cf37e0b15aaa81ed5/pkg/controllers/managementuser/rbac/namespace_handler.go#L289-L308
https://github.com/rancher/rancher/blob/aa7bea9baa40eb8eb1d7166cf37e0b15aaa81ed5/pkg/controllers/managementuser/rbac/handler_base.go#L463-L464

Actually... this does sound like a job of either K8s built-in validation or webhook's.

I disagree that it is reasonable to add webhook validation for every resource whose name might conceivably be used as part of a label. That could be hundreds of resources, everything in kubectl api-resources. We also can't predict how something might use it in a label. In this particular case, it's concatenated with the known length of a project or cluster ID, so the max length is 63-7 or 63-13, but some other label may have some other scheme.

We know that this label value is causing the problem. That value is generated by rancher. We can control the label value. We can safely convert any string to a unique 63-character string and use that as the unique label. The SafeConcatName helper was created for exactly this purpose because it's a common issue. I don't think it's fair to police every resource's name when we have a known safe way of handling these values.

As a side note, to do this we would also need to update the prtb indexer to convert to the safe string:

https://github.com/rancher/rancher/blob/aa7bea9baa40eb8eb1d7166cf37e0b15aaa81ed5/pkg/controllers/managementuser/rbac/handler_base.go#L586

[KevinJoiner]

@cmurphy I like that solution; I don't think we should use the SafeConcateName because it only uses the first 4 characters of the hash, giving us 20 bits of the SHA256, which has a high collision rate https://en.wikipedia.org/wiki/Birthday_attack#Mathematics. Is there anything wrong which just using the entire hash? a SHA256 hash base64 encoded should come to 44 bytes which is still valid.

[cmurphy]

It would be more user friendly not to use the entire hash so that someone looking at it can tell roughly where it came from. You're right 4 characters isn't much but if we're concerned about it here we should change it everywhere because avoiding collisions is largely the point of that function.

[KevinJoiner]

After talking about SafeConcateName @cmurphy, we concluded that even though the collision rate for the function is not ideal. Changing all the functions in Rancher currently using this implementation of SafeConcateName is preoptimization since we have not experienced any collisions.

[maxsokolovsky]

Closing this in favor of rancher/rancher#41543.