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

Allow manual set of service account #670

Closed
wants to merge 1 commit into from

Conversation

gsemet
Copy link
Contributor

@gsemet gsemet commented May 2, 2018

This allows to disable the automatic creation of the service account (that requires clusterbinding priviledge) and define the service account for jupyter (hub and proxy) to use

Signed-off-by: Gaetan Semet gaetan@xeberon.net

@@ -51,7 +51,7 @@ hub:

rbac:
enabled: true

serviceAccount: ''
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will actually have a kubernetes ServiceAccount object even though the value rbac.serviceAccount in values.yaml is left blank. I'm thinking that if we name the value rbac.customServiceAccount we might indicate the behavior of the setting better. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm fine with it

@gsemet gsemet force-pushed the manual_service_account branch 2 times, most recently from eb92692 to b28fe71 Compare May 2, 2018 14:06
Signed-off-by: Gaetan Semet <gaetan@xeberon.net>
@consideRatio
Copy link
Member

consideRatio commented May 2, 2018

@gsemet thank you for the PR! It exposes a piece of missing knowledge to me that I need to figure out, I love it!!

I'm currently trying to grasp "that requires clusterbinding priviledge" part and simply understand the situation better overall.

Thinking aloud...

A cluster administrator or someone who would be allowed to create kubernetes objects through helm / kubectl. Is that user allowed to do certain things, but not all, like create a pod, but not a ServiceAccount object perhaps? I'm thinking this may be my main knowledge gap - where is cluster access defined and what rights do such user have?

A ClusterRoleBinding couples a ServiceAccount with a ClusterRole as seen in image-puller's rbac.yaml for example.

How I can progress...

  1. Give your account super-user permissions, allowing you to perform all the actions needed to set up JupyterHub.
kubectl create clusterrolebinding cluster-admin-binding \
   --clusterrole=cluster-admin \
   --user=<YOUR-EMAIL-ADDRESS>

kubectl get clusterrolebinding cluster-admin-binding -o yaml

apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
  creationTimestamp: 2018-01-01T23:58:20Z
  name: cluster-admin-binding
  resourceVersion: "690"
  selfLink: /apis/rbac.authorization.k8s.io/v1/clusterrolebindings/cluster-admin-binding
  uid: asdfasdf-asdf-asdf-asdf-asdfasdfasdf
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: ClusterRole
  name: cluster-admin
subjects:
- apiGroup: rbac.authorization.k8s.io
  kind: User
  name: <MY-EMAIL-ADDRESS>

Update 1

So ClusterRoleBindings can tie together a kind: User or kind: ServiceAccount with a ClusterRole! That was a big missing piece of understanding for me.

@gsemet
Copy link
Contributor Author

gsemet commented May 2, 2018

Hi. I must admit my limited knowledge as well on this subjet. On our baremetal kubenertes, we cannot create ClusterRoleBinding at all in our namespace, which is understandable because it might allows easy escalation of priviledge out of our namespace.
In short, we have in our namespace a service acount linked to a role with just the right set of priviledges for helm to start/stop pods, secrets, configmaps, services, and so. We cannot create clusterrole, only namespace role. So I simply reuse the same service account for the hub. Works great.

The underneath reason might be that jupyterhub does not need a ClusterRoleBinding/ClusterRole.

@manics
Copy link
Member

manics commented May 2, 2018

My understanding is a ClusterRole has privileges across all namespaces, whereas a Role can only operate inside a single namespace. I don't know why the image-puller requires a ClusterRole instead of a Role.

@consideRatio
Copy link
Member

consideRatio commented May 2, 2018

@gsemet you helped me understand things better! Thank you!

@manics good point, hmmm, so it will speak with the kubernetes api server, asking it about a daemonset, but it does not need to find daemonsets outside its namespace... It previously required privilege to create deaemonsets but no longer...

Update 1

@manics I created issue #671 to address this!

Update 2

manics was right, it didn't require a ClusterRole, so I created #674 to solve it.

@consideRatio
Copy link
Member

@gsemet as I understand it. Your User's cluster privilege is declared somewhere, and that does not allow you as great privilege as the predefined ClusterRole cluster-admin. But I think it will allow you to create ServiceAccount objects and Role's, but you may not bind ClusterRole's to it with a ClusterRoleBinding. I think this because i think ServiceAccounts have no permissions unless they have been coupled with a Role or ClusterRole through a RoleBinding or ClusterRoleBinding.

I think @manics is correct in that the image-awaiter job utilizing the ServiceAccount / ClusterRole / ClusterRolebinding in image-puller's rbac.yaml.

If you fix #671, I think you solved your problem!

Copy link
Member

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gsemet I think by fixing #671, we don't need to do anything else. The #671 fix might be as simple as removing Cluster from ClusterRoleBinding and ClusterRole in the image-pullers rbac.yaml file.

I'm the person responsible for your trouble with this ;D Thank you for working on this!!

@gsemet
Copy link
Contributor Author

gsemet commented May 2, 2018

There are several places where the service account are created, at least: puller (that I don't use), culler (that I use) and the https proxy (that I don't use). For simplicity I have set all to use the same hardcoded service account (I actually use the service account used by Tiller). So I agree my solution is not clean, but it works.

I however do not have the time to work on it for the moment, but if one manage to restrict the roles to the namespace I think that would do the job perfectly!

@consideRatio
Copy link
Member

@gsemet oh, do you have automatic https enabled? Then I think the following clusterrolebinding being created is that which is overstepping your permissions, and fixing the image-puller overstepping won't help you, as if it is disabled you won't create any clusterrolebinding there no matter what.

# This is way too many permissions, but apparently the nginx-controller
# is written to sortof assume it is clusterwide ingress provider.
# So we keep this as is, for now.

@gsemet
Copy link
Contributor Author

gsemet commented May 2, 2018

we don't have https against our internal jupyter... but I think it will not have any impact, since we do'nt use nginx ingress. Traefik is really a good ingress. It already handles some https so it may not be difficult to turn https on against jupyter when the day comes :)

@consideRatio
Copy link
Member

@gsemet I just created #674, if that PR is implemented and you do not use the automatic letsencrypt funcionality, nothing will require a ClusterRoleBinding as far as I know, and you won't need to override the ServiceAccount's used to the very permissive Tiller account.

Oh but wait a second I confused myself! You had disabled the hook-image-puller, by setting prePuller.hook.enabled: false - which is true by default?. Well if so then #674 should not change anything...

:D Confusion! I'm not confident at all about the situation, are you not allowed to create a ServiceAccount? I understand them as placeholder accounts until they are tied to some Role or ClusterRole (predefined or manually created) through a RoleBinding or ClusterRoleBinding.

@gsemet
Copy link
Contributor Author

gsemet commented May 3, 2018

Yes service account can be easily created, it is the role that are restricted ! I have disabled the prepuller because it didn’t Worked (didn’t it need the docker socket at one point?). I can try it again but yes, I have disabled it on our prod

@consideRatio
Copy link
Member

@gsemet I'm still quite confused, but this is my current understanding of the situation.

  1. You can create ServiceAccounts, Roles and RoleBindings as found in /hub/rbac.yaml (Roles and ClusterRoles)
  2. You cannot create ClusterRoleBindings which are found within /image-puller/rbac.yaml and within /proxy/autohttps/rbac.yaml
  3. Everything works if you have prePuller.hook.enabled: false and proxy.autohttps.enabled: false even though you don't specify the usage of pre-existing ServiceAccounts.

@gsemet
Copy link
Contributor Author

gsemet commented May 4, 2018

Yes :) I will try #674 patch ASAP (probably not before 2 weeks) ! And I probably been over hacky when I disabled everything that wasn’t working instead of trying to make it work.
The thing is we have a pretty cool but not so common environment:

  • bare metal kubernetes
  • traefik ingress (very very good). I would like to give a try with envoy as well. At the end we will probably have several ingresses
  • namespace creation is restricted, and we only have the default role restricted with permission on our own namespace. I still need to investigate if we have enough role or not in our own namespace, but I really think this is the right pass. Nobody should be able to play with a clusterrole, it allows escalation too easily.
  • so we have one tiller per namespace

We are investigating on how we can extra level of security (TLS on helm, ...) so we are learning everyday ! Thanks for your patience and support :)

@consideRatio
Copy link
Member

@gsemet I'm excited about the setup you have! I figure it may be essential to be able to use z2jh-k8s on baremetal for many administrators in the end and you are leading the way to making things work! I appreciate the possibility to learn from your experiences! :D

@consideRatio
Copy link
Member

Hey again @gsemet, have you tried non-custom ServiceAccounts after using #674 ?

@gsemet
Copy link
Contributor Author

gsemet commented May 29, 2018

Hi, not tried. I will try it today

@consideRatio
Copy link
Member

consideRatio commented Jun 11, 2018

@gsemet my current understanding I've arrived to by learning through your experiences is that we should aim to avoid creating any ClusterRoleBindings as they require more permissions than some have over their cluster.

Currently, I think only the autohttps setup requires a ClusterRoleBinding, so if anything should be setup to use a custom predefined ServiceAccount with high allowed permissions it should be the autohttps deployment.

I'll close PR this for now. I'm very thankful that you made it, I got to learn a lot and now I know what we should aim for better.

Perhaps an upgrade from kube-lego to cert-manager, or what @minrk suggests in #710 might do the trick of getting rid of the last ClusterRoleBinding used for a ServiceAccount there as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants