Service-catalog: Access control for ExternalID

[ash2k]

What if a user specifies an ExternalID that is used by someone else's resource of the same class/plan when they create a ServiceInstance? A broker might be used by multiple distinct systems and might not be able to prevent "ownership extension". This is especially likely if the broker is a pass-though to some other api.

I'm proposing Service Catalog should check with the AuthZ API to see if the user is allowed to specify the id (only on initial object creation, after that it is immutable I believe).

There is a minor hiccup thought. Currently ExternalID is set by defaulting code and by the time the object reaches admission plugins built into the api server, the field would have been set to a value already. So the check has to be performed before the default is applied. We initially wanted to build an admission webhook to do that but the problem is the same - it gets an object with defaults applied. Too late.
This can be solved by performing ExternalID defaulting later in the processing cycle, after the check has been performed.

[nilebox]

I am not even sure if setting a random non-deterministic value (UUID) as part of applying defaults is the correct approach for Kubernetes.

@liggitt can you comment on that please?

For the context: the defaulting function for ServiceInstance resource generates a UUID if the ExternalID was not supplied by a user: https://github.com/kubernetes-incubator/service-catalog/blob/ba8d0508043a520be3b7b782d9cc27f9af7e34c2/pkg/apis/servicecatalog/v1beta1/defaults.go#L49-L53
Shouldn't defaulting functions only set constant values?

[liggitt]

defaults should be deterministic. what you are doing looks like a better fit to put in the PrepareForCreate method of the strategy or the Create method of the rest handler

[nilebox]

I'm proposing Service Catalog should check with the AuthZ API to see if the user is allowed to specify the id (only on initial object creation, after that it is immutable I believe).

@ash2k can you give a more detailed example of such RBAC policy? e.g. yamls

I don't think in normal Kubernetes RBAC you can control access to particular fields. So it sounds more like a combination of some arbitrary Kubernetes RBAC + Service Catalog specific admission controller that enforces the check for ExternalID?

[ash2k]

@nilebox https://kubernetes.io/docs/admin/authorization/#review-your-request-attributes

Verb would be "set" or "create";
Resource would be "servicebindings" or "serviceinstances" (or singular form, not sure);
Subresource would be "externalID";

Basically service catalog could ask the authz api whether the user is allowed to "set" a subresource called "externalID" for a resource "servicebindings" or "serviceinstances". This is the same as with separate permissions for "finalizers" and for "status" subresource to access corresponding fields in the object.
Also see
https://kubernetes.io/docs/admin/authorization/rbac/#referring-to-resources
https://kubernetes.io/docs/admin/admission-controllers/#ownerreferencespermissionenforcement

[ash2k]

Yes, it is going to be a built in admission plugin in Service Catalog that calls the SubjectAccessReview api with the suff I described in the previous comment above.

[pmorie]

Just want to note that this is something that will likely require attention if we try to ditch the API server

cc @n3wscott

[nilebox]

@pmorie we could use validating and mutating admission webhooks for CRD (if we switch from API server to CRD) as noted in https://github.com/kubernetes-incubator/service-catalog/issues/2035#issuecomment-389354794

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[mszostok]

/remove-lifecycle rotten
/lifecycle frozen