Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set the default sandbox role APBs run with under Automation Broker to 'admin' instead of 'edit' to allow creation of RoleBindings in target namespace #9231

Closed
jwmatthews opened this issue Jul 17, 2018 · 17 comments
Labels
lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.

Comments

@jwmatthews
Copy link
Member

Description

The Eclipse CHE team requires the ability to create a Role Binding from within their APB

The default configuration of Automation Broker sets the sandbox role to 'edit' while 'admin' is required for this operation to succeed.

Current recommendation is for a cluster administrator to tweak this setting and restart the Automation Broker.

Eclipce CHE team is requesting that the default sandbox role be changed from 'edit' to 'admin'.
This means that the APB would run with 'admin' privileges in the target namespace opposed to 'edit'

This issue is to track that discussion, asking for guidance from @openshift/sig-security

@enj @simo5 thoughts, do you have concerns if we change the sandbox role to 'admin' opposed to 'edit'?

cc @openshift/sig-ansible-service-broker

@enj
Copy link
Contributor

enj commented Jul 17, 2018

The Eclipse CHE team requires the ability to create a Role Binding from within their APB

Why?

They are trying to assign the admin cluster role to a SA in the namespace. This means that they have destroyed the distinction between admin and edit in that namespace. Anyone with the edit rights in the namespace will be able to read the SA's secret and use that to escalate to admin. Thus their approach is broken in general.

The default configuration of Automation Broker sets the sandbox role to 'edit' while 'admin' is required for this operation to succeed.

Correct. This is to limit the escalation paths available to APBs. It also avoids the issue I mentioned above.

Current recommendation is for a cluster administrator to tweak this setting and restart the Automation Broker.

Seems fine to let the cluster admin opt-in if they desire to. A per APB role configuration would be best.

Eclipce CHE team is requesting that the default sandbox role be changed from 'edit' to 'admin'.
...
you have concerns if we change the sandbox role to 'admin' opposed to 'edit'?

No, we should not reduce the default security for all APBs to support their use case. I realize that some APBs will need higher access rights. This should be handled on a per APB basis and any APB needing these extra rights should require explict opt-in from the cluster admin.

@enj
Copy link
Contributor

enj commented Jul 17, 2018

Long term I am hoping to have more robust token delegation flows that makes it so that the ASB gets a token for the user and simply uses it to perform all actions. If the user has the correct access rights, everything works as expected and no specific access is required on the ASB side.

@liggitt
Copy link
Contributor

liggitt commented Jul 17, 2018

explict opt-in from the cluster admin.

or from a namespace admin in that namespace, who can also choose to grant the admin role to that service account

@enj
Copy link
Contributor

enj commented Jul 17, 2018

or from a namespace admin in that namespace, who can also choose to grant the admin role to that service account

IIRC the resistance to this flow is that it must be manually done out of band by the user (since the actual provisioning is done by the ASB SA which only has edit rights).


From ansibleplaybookbundle/eclipse-che-apb#17 @l0rd

why we need a RoleBinding with role_ref_kind: ClusterRole instead of role_ref_kind: Role and

You still need admin rights to create roles in the namespace.

if sandbox_role set to edit would be enough to create a RoleBindings with role_ref_kind: Role

It does not. All RBAC related actions require admin.

@l0rd
Copy link

l0rd commented Jul 17, 2018

if sandbox_role set to edit would be enough to create a RoleBindings with role_ref_kind: Role

It does not. All RBAC related actions require admin.

@enj thanks that is clear now. We need a ServiceAccount with edit rights in the provisioned namespace:

  1. You are saying that this approach is broken. Why? I don't get the argument about having access to SA secrets: nobody else should have edit rights in the namespace except the SA itself.
  2. What should be the right approach to let Che create pods, services, pvcs and routes?

@enj
Copy link
Contributor

enj commented Jul 17, 2018

You are saying that this approach is broken. Why?

The linked YAML file grants admin to the SA. That is incorrect. It should have edit at most.

I don't get the argument about having access to SA secrets

  1. User A has edit rights in namespace B
  2. SA C has admin rights in namespace B
  3. User A uses SA C's secret to become the SA, and thus has indirect admin rights - i.e. they trivially escalate themselves

nobody else should have edit rights in the namespace except the SA itself

You do not control that.

What should be the right approach to let Che create pods, services, pvcs and routes?

The safe way we have today is to have the SA go through an OAuth flow with the user to get a token for the user. This allows safe delegation of privileges. This is how Jenkins works today.

Otherwise you can ask the user to manually grant the Che SA edit rights to the namespace. IIRC this is the approach OLM will take for the foreseeable future.

@l0rd
Copy link

l0rd commented Jul 17, 2018

The linked YAML file grants admin to the SA. That is incorrect. It should have edit at most.

Agreed. Che is designed to require edit rights only. We will fix the yaml.

The safe way we have today is to have the SA go through an OAuth flow with the user to get a token for the user. This allows safe delegation of privileges. This is how Jenkins works today.

This is what we are looking for! I had a look at Jenkins APB and it looks like it creates a RoleBinding as we do. Do you know if that flow is possible using an APB?

Otherwise you can ask the user to manually grant the Che SA edit rights to the namespace.

The point of using an APB for us was to avoid manual steps :-/

@enj
Copy link
Contributor

enj commented Jul 17, 2018

This is what we are looking for! I had a look at Jenkins APB and it looks like it creates a RoleBinding as we do. Do you know if that flow is possible using an APB?

I am not familiar with the Jenkins APB. The standard Jenkins template code is run by the user, so it has the correct access needed to set up Jenkins.

I do not see how any service broker could do an OAuth flow with the user (needs browser and user interaction).

The point of using an APB for us was to avoid manual steps :-/

The same is true for OLM, but they will have to ask the user to do the RBAC steps manually.

@l0rd
Copy link

l0rd commented Jul 18, 2018

@jwmatthews what I understand is that applications that require edit role cannot be fully provisioned with an APB (or OLM) but will always require a manual step. If that's correct we should probably mention it in the documentation (e.g. as a note here?).

@shawn-hurley
Copy link
Contributor

what I understand is that applications that require edit role cannot be fully provisioned with an APB (or OLM) but will always require a manual step.

This is not true, if the sandbox role is set to admin for the broker then the ASB could deploy this APB without a manual step.

@jwmatthews
Copy link
Member Author

@l0rd

FYI @shawn-hurley and @dymurray chatted with @enj today, we may have an option that will allow the APB to express the roles it needs along with configuration options to allow a cluster administrator to allow/disallow this behavior.

A writeup is in progress to document the concept, we'll update this issue when we have the thoughts captured.

@l0rd
Copy link

l0rd commented Jul 19, 2018

@shawn-hurley what I mean is that users that deploy Che (or another application that requires edit role at namespace scope) using an APB need to perform a manual step (e.g. setting the sandbox role to admin). Of course if the sandbox role has already been changed there is no manual step to do.

Anyway I prefer asking Che users to manually create the rolebinding rather than setting the ASB sandbox role to admin. Because it can be done even after Che provisioning and because it doesn't have any side effect on ASB security.

@jwmatthews thank you. I am keen to learn more details about this new option.

@dymurray
Copy link
Member

@l0rd I need to clarify something here. You keep saying Che only requires edit role. This is not true. Che requires admin role because you are creating RoleBindings in your deployment.

The point that has been made from our team is that you really do not need to be creating this RoleBinding to deploy Pods, Services, Routes, etc.

Is there an explicit need for you to be creating this RoleBinding from the APB? If there is then a solution can be found but I'm not convinced you need to be creating a RoleBinding if you say that you only need the edit role.

There should be no manual steps needed from the user if edit is sufficient.

@openshift-bot
Copy link

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

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

/lifecycle stale

@openshift-ci-robot openshift-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 26, 2020
@openshift-bot
Copy link

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

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

/lifecycle rotten
/remove-lifecycle stale

@openshift-ci-robot openshift-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jun 25, 2020
@openshift-bot
Copy link

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

@openshift-ci-robot
Copy link

@openshift-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed.
Projects
None yet
Development

No branches or pull requests

8 participants