Remove serviceaccount for game server container #640
Conversation
|
Build Failed 😱 Build Id: 6567cf7f-5e7f-4260-998f-fc936f77d119 To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
markmandel
force-pushed
the
security/remove-gs-token
branch
from
5ed299f
to
dbe30ab
Compare
|
Build Succeeded 👏 Build Id: 4a4228a0-581a-4a03-bc43-0b668544bd6e The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
cmd/controller/main.go
Outdated
| apiServerBurstQPSFlag = "api-server-qps-burst" | ||
| logDirFlag = "log-dir" | ||
| logSizeLimitMBFlag = "log-size-limit-mb" | ||
| disableGameServerContainerServiceAccountFlag = "disable-gameserver-service-account" |
There was a problem hiding this comment.
can this be a flag on GameServerSpec (enableKubernetesAPIAccess or something, defaulting to false)?
There was a problem hiding this comment.
Yeah - it totally could be! Then it wouldn't have to be an all or nothing operation. That's a nice change.
(and I could move some code back into gameserver.go, rather than have it in the controller, which is nice)
So if I had my ideal, I'd probably want something like this, but it is a breaking change. What do you think?
apiVersion: "stable.agones.dev/v1alpha1"
kind: GameServer
metadata:
name: "gds-example" # set a fixed name
spec:
# game server container information
# this would be a breaking change
container:
name: example-server
mountServiceAccount: false
template:
# pod metadata. Name & Namespace is overwritten
metadata:
labels:
myspeciallabel: myspecialvalue
# Pod Specification
spec:
containers:
- name: example-server
image: gcr.io/agones/test-server:0.1
imagePullPolicy: Always
There was a problem hiding this comment.
Devil's advocate against this - I could see the high level ops team wanting to be able to say "globally, no game server containers get access to the the K8s API" rather than letting users specify it on a game server level?
Since this has security implications - this felt more like an all or nothing type operation.
There was a problem hiding this comment.
I think this type of policy enforcement should be orthogonal and perhaps left as an exercise to the reader (who can implement their own validating webhook to enforce it)?
There was a problem hiding this comment.
BTW. PodSpec has automountServiceAccountToken which can be set, can we just use that? We would need a way to pass API credentials to the sidecar somehow, regardless of this setting or any other choice of serviceAccountName
There was a problem hiding this comment.
I think this type of policy enforcement should be orthogonal and perhaps left as an exercise to the reader (who can implement their own validating webhook to enforce it)?
Yeah, that's not unreasonable. A webhook, or provide an abstraction of the top, etc. Okay, I'm in -- moving it to the GameServer config sounds good to me.
BTW. PodSpec has automountServiceAccountToken which can be set, can we just use that? We would need a way to pass API credentials to the sidecar somehow, regardless of this setting or any other choice of serviceAccountName
So I asked about that in sig-apimachinery, in an attempt to reverse this approach, and just set the secret on the containers that should have it, and the TL;DR was that it wasn't a good idea. Also, definitely seeing users building their own sidecars that want access to the K8s API.
Ideally, it would be lovely if K8s supported per-container service accounts, but that doesn't seem to be on the cards.
What do you think of the config option above?
There was a problem hiding this comment.
that looks unintuitive (effectively part of the "spec" that's defined outside of spec), but I can't think of anything that would be intuitive.
Ok, one option perhaps - how about the following rule:
If podspec defines serviceAccountName we will use it as-is (the presence of serviceAccountName states user's intention to define specific permissions for the pod, in which case the SA must have role binding that lets them to manipulate GameServers too)
If podspec does NOT include serviceAccountName - we assume the user does not want the game server to talk to Kubernetes API at all, in which case we use our own credentials and hide them from all non-sidecar containers?
There was a problem hiding this comment.
I went back and forth on this - but I think you are right, and that this is the best way forward (with appropriate documentation), and similar to what we do already.
Basically if you set the service account on the Pod, you move into "expert mode" and out of "opinionated mode", and then you can essentially do what you want.
I'll just want to add a documentation section as well that covers this 👍
There was a problem hiding this comment.
This has now been completed (including documentation), and is ready for review.
markmandel
force-pushed
the
security/remove-gs-token
branch
4 times, most recently
from
85d3273
to
f16e2e1
Compare
|
Build Succeeded 👏 Build Id: 7d1b4157-c82c-4204-9d6d-30a0a4d86e42 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
This mounts an emptydir over the service account token that is automatically mounted in the container that runs the game server binary. Since this is exposed to the outside world, removing the serviceaccount token removes authentication against the rest of the Kubernetes cluster if it ever gets compromised. Closes googleforgames#150
markmandel
force-pushed
the
security/remove-gs-token
branch
from
f16e2e1
to
fe0abc9
Compare
|
Build Succeeded 👏 Build Id: 75941a20-aeab-4627-91aa-58aeb63073c5 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
This mounts an emptydir over the service account token that is automatically mounted in the container that runs the game server binary.
Since this is exposed to the outside world, removing the serviceaccount token removes authentication against the rest of the Kubernetes cluster if it ever gets compromised.
Closes #150