From 5ed299fd70cda345eaffca72c294ddb9a5e9b464 Mon Sep 17 00:00:00 2001 From: Mark Mandel Date: Fri, 22 Feb 2019 18:11:35 +1100 Subject: [PATCH] Remove serviceaccount for game server container 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 --- cmd/controller/main.go | 113 ++++++++-------- install/helm/agones/templates/controller.yaml | 2 + install/helm/agones/values.yaml | 3 +- pkg/apis/stable/v1alpha1/gameserver.go | 13 ++ pkg/apis/stable/v1alpha1/gameserver_test.go | 34 ++++- pkg/gameservers/controller.go | 126 ++++++++++-------- pkg/gameservers/controller_test.go | 38 +++++- site/content/en/docs/Installation/helm.md | 1 + 8 files changed, 222 insertions(+), 108 deletions(-) diff --git a/cmd/controller/main.go b/cmd/controller/main.go index e1bc58a1fd..d8e06d4952 100644 --- a/cmd/controller/main.go +++ b/cmd/controller/main.go @@ -52,24 +52,25 @@ import ( ) const ( - enableStackdriverMetricsFlag = "stackdriver-exporter" - enablePrometheusMetricsFlag = "prometheus-exporter" - projectIDFlag = "gcp-project-id" - sidecarImageFlag = "sidecar-image" - sidecarCPURequestFlag = "sidecar-cpu-request" - sidecarCPULimitFlag = "sidecar-cpu-limit" - pullSidecarFlag = "always-pull-sidecar" - minPortFlag = "min-port" - maxPortFlag = "max-port" - certFileFlag = "cert-file" - keyFileFlag = "key-file" - numWorkersFlag = "num-workers" - apiServerSustainedQPSFlag = "api-server-qps" - apiServerBurstQPSFlag = "api-server-qps-burst" - logDirFlag = "log-dir" - logSizeLimitMBFlag = "log-size-limit-mb" - kubeconfigFlag = "kubeconfig" - defaultResync = 30 * time.Second + enableStackdriverMetricsFlag = "stackdriver-exporter" + enablePrometheusMetricsFlag = "prometheus-exporter" + projectIDFlag = "gcp-project-id" + sidecarImageFlag = "sidecar-image" + sidecarCPURequestFlag = "sidecar-cpu-request" + sidecarCPULimitFlag = "sidecar-cpu-limit" + pullSidecarFlag = "always-pull-sidecar" + minPortFlag = "min-port" + maxPortFlag = "max-port" + certFileFlag = "cert-file" + keyFileFlag = "key-file" + numWorkersFlag = "num-workers" + apiServerSustainedQPSFlag = "api-server-qps" + apiServerBurstQPSFlag = "api-server-qps-burst" + logDirFlag = "log-dir" + logSizeLimitMBFlag = "log-size-limit-mb" + disableGameServerContainerServiceAccountFlag = "disable-gameserver-service-account" + kubeconfigFlag = "kubeconfig" + defaultResync = 30 * time.Second // topNGSForAllocation is used by the GameServerAllocation controller // to reduce the contention while allocating gameservers. topNGSForAllocation = 100 @@ -183,7 +184,7 @@ func main() { gsController := gameservers.NewController(wh, health, ctlConf.MinPort, ctlConf.MaxPort, ctlConf.SidecarImage, ctlConf.AlwaysPullSidecar, - ctlConf.SidecarCPURequest, ctlConf.SidecarCPULimit, + ctlConf.SidecarCPURequest, ctlConf.SidecarCPULimit, ctlConf.DisableGameServerContainerServiceAccount, kubeClient, kubeInformerFactory, extClient, agonesClient, agonesInformerFactory) gsSetController := gameserversets.NewController(wh, health, kubeClient, extClient, agonesClient, agonesInformerFactory) @@ -236,6 +237,7 @@ func parseEnvFlags() config { viper.SetDefault(apiServerBurstQPSFlag, 200) viper.SetDefault(logDirFlag, "") viper.SetDefault(logSizeLimitMBFlag, 10000) // 10 GB, will be split into 100 MB chunks + viper.SetDefault(disableGameServerContainerServiceAccountFlag, true) pflag.String(sidecarImageFlag, viper.GetString(sidecarImageFlag), "Flag to overwrite the GameServer sidecar image that is used. Can also use SIDECAR env variable") pflag.String(sidecarCPULimitFlag, viper.GetString(sidecarCPULimitFlag), "Flag to overwrite the GameServer sidecar container's cpu limit. Can also use SIDECAR_CPU_LIMIT env variable") @@ -254,6 +256,8 @@ func parseEnvFlags() config { pflag.Int32(apiServerBurstQPSFlag, 200, "Maximum burst queries per second to send to the API server") pflag.String(logDirFlag, viper.GetString(logDirFlag), "If set, store logs in a given directory.") pflag.Int32(logSizeLimitMBFlag, 1000, "Log file size limit in MB") + pflag.Bool(disableGameServerContainerServiceAccountFlag, viper.GetBool(disableGameServerContainerServiceAccountFlag), "When enabled, mounts an `emptyDir` over the service account token in the GameServer container, to stop game server processes from accessing Kubernetes. Can also use env DISABLE_GAMESERVER_SERVICE_ACCOUNT") + pflag.Parse() viper.SetEnvKeyReplacer(strings.NewReplacer("-", "_")) @@ -275,6 +279,7 @@ func parseEnvFlags() config { runtime.Must(viper.BindEnv(apiServerBurstQPSFlag)) runtime.Must(viper.BindEnv(logDirFlag)) runtime.Must(viper.BindEnv(logSizeLimitMBFlag)) + runtime.Must(viper.BindEnv(disableGameServerContainerServiceAccountFlag)) request, err := resource.ParseQuantity(viper.GetString(sidecarCPURequestFlag)) if err != nil { @@ -287,45 +292,47 @@ func parseEnvFlags() config { } return config{ - MinPort: int32(viper.GetInt64(minPortFlag)), - MaxPort: int32(viper.GetInt64(maxPortFlag)), - SidecarImage: viper.GetString(sidecarImageFlag), - SidecarCPURequest: request, - SidecarCPULimit: limit, - AlwaysPullSidecar: viper.GetBool(pullSidecarFlag), - KeyFile: viper.GetString(keyFileFlag), - CertFile: viper.GetString(certFileFlag), - KubeConfig: viper.GetString(kubeconfigFlag), - PrometheusMetrics: viper.GetBool(enablePrometheusMetricsFlag), - Stackdriver: viper.GetBool(enableStackdriverMetricsFlag), - GCPProjectID: viper.GetString(projectIDFlag), - NumWorkers: int(viper.GetInt32(numWorkersFlag)), - APIServerSustainedQPS: int(viper.GetInt32(apiServerSustainedQPSFlag)), - APIServerBurstQPS: int(viper.GetInt32(apiServerBurstQPSFlag)), - LogDir: viper.GetString(logDirFlag), - LogSizeLimitMB: int(viper.GetInt32(logSizeLimitMBFlag)), + MinPort: int32(viper.GetInt64(minPortFlag)), + MaxPort: int32(viper.GetInt64(maxPortFlag)), + SidecarImage: viper.GetString(sidecarImageFlag), + SidecarCPURequest: request, + SidecarCPULimit: limit, + AlwaysPullSidecar: viper.GetBool(pullSidecarFlag), + KeyFile: viper.GetString(keyFileFlag), + CertFile: viper.GetString(certFileFlag), + KubeConfig: viper.GetString(kubeconfigFlag), + PrometheusMetrics: viper.GetBool(enablePrometheusMetricsFlag), + Stackdriver: viper.GetBool(enableStackdriverMetricsFlag), + GCPProjectID: viper.GetString(projectIDFlag), + NumWorkers: int(viper.GetInt32(numWorkersFlag)), + APIServerSustainedQPS: int(viper.GetInt32(apiServerSustainedQPSFlag)), + APIServerBurstQPS: int(viper.GetInt32(apiServerBurstQPSFlag)), + LogDir: viper.GetString(logDirFlag), + LogSizeLimitMB: int(viper.GetInt32(logSizeLimitMBFlag)), + DisableGameServerContainerServiceAccount: viper.GetBool(disableGameServerContainerServiceAccountFlag), } } // config stores all required configuration to create a game server controller. type config struct { - MinPort int32 - MaxPort int32 - SidecarImage string - SidecarCPURequest resource.Quantity - SidecarCPULimit resource.Quantity - AlwaysPullSidecar bool - PrometheusMetrics bool - Stackdriver bool - KeyFile string - CertFile string - KubeConfig string - GCPProjectID string - NumWorkers int - APIServerSustainedQPS int - APIServerBurstQPS int - LogDir string - LogSizeLimitMB int + MinPort int32 + MaxPort int32 + SidecarImage string + SidecarCPURequest resource.Quantity + SidecarCPULimit resource.Quantity + AlwaysPullSidecar bool + PrometheusMetrics bool + Stackdriver bool + DisableGameServerContainerServiceAccount bool + KeyFile string + CertFile string + KubeConfig string + GCPProjectID string + NumWorkers int + APIServerSustainedQPS int + APIServerBurstQPS int + LogDir string + LogSizeLimitMB int } // validate ensures the ctlConfig data is valid. diff --git a/install/helm/agones/templates/controller.yaml b/install/helm/agones/templates/controller.yaml index 971f212b7a..c52d019c3d 100644 --- a/install/helm/agones/templates/controller.yaml +++ b/install/helm/agones/templates/controller.yaml @@ -98,6 +98,8 @@ spec: value: {{ .Values.agones.controller.apiServerQPS | quote }} - name: API_SERVER_QPS_BURST value: {{ .Values.agones.controller.apiServerQPSBurst | quote }} + - name: DISABLE_GAMESERVER_SERVICE_ACCOUNT + value: {{ .Values.gameservers.disableGameServerContainerServiceAccount | quote }} {{- if .Values.agones.controller.persistentLogs }} - name: LOG_DIR value: "/home/agones/logs" diff --git a/install/helm/agones/values.yaml b/install/helm/agones/values.yaml index 3046eb3fa1..5b45211137 100644 --- a/install/helm/agones/values.yaml +++ b/install/helm/agones/values.yaml @@ -111,7 +111,8 @@ agones: gameservers: namespaces: - - default + - default minPort: 7000 maxPort: 8000 + disableGameServerContainerServiceAccount: true diff --git a/pkg/apis/stable/v1alpha1/gameserver.go b/pkg/apis/stable/v1alpha1/gameserver.go index d3042f88f8..477daa346f 100644 --- a/pkg/apis/stable/v1alpha1/gameserver.go +++ b/pkg/apis/stable/v1alpha1/gameserver.go @@ -354,6 +354,19 @@ func (gs *GameServer) FindGameServerContainer() (int, corev1.Container, error) { return -1, corev1.Container{}, errors.Errorf("Could not find a container named %s", gs.Spec.Container) } +// ApplyToPodGameServerContainer applies func(v1.Container) to the pod's gameserver container +func (gs *GameServer) ApplyToPodGameServerContainer(pod *corev1.Pod, f func(corev1.Container) corev1.Container) *corev1.Pod { + for i, c := range pod.Spec.Containers { + if c.Name == gs.Spec.Container { + c = f(c) + pod.Spec.Containers[i] = c + break + } + } + + return pod +} + // Pod creates a new Pod from the PodTemplateSpec // attached to the GameServer resource func (gs *GameServer) Pod(sidecars ...corev1.Container) (*corev1.Pod, error) { diff --git a/pkg/apis/stable/v1alpha1/gameserver_test.go b/pkg/apis/stable/v1alpha1/gameserver_test.go index fd5ba98407..b187a42d01 100644 --- a/pkg/apis/stable/v1alpha1/gameserver_test.go +++ b/pkg/apis/stable/v1alpha1/gameserver_test.go @@ -425,7 +425,7 @@ func TestGameServerPatch(t *testing.T) { assert.Contains(t, string(patch), `{"op":"replace","path":"/spec/container","value":"bear"}`) } -func TestGetDevAddress(t *testing.T) { +func TestGameServerGetDevAddress(t *testing.T) { devGs := &GameServer{ ObjectMeta: metav1.ObjectMeta{ Name: "dev-game", @@ -451,3 +451,35 @@ func TestGetDevAddress(t *testing.T) { assert.False(t, isDev, "dev-game should NOT have a dev-address") assert.Equal(t, "", devAddress, "dev-address IP address should be 127.1.1.1") } + +func TestGameServerApplyToPodGameServerContainer(t *testing.T) { + t.Parallel() + + name := "mycontainer" + gs := &GameServer{ + Spec: GameServerSpec{ + Container: name, + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + {Name: name, Image: "foo/mycontainer"}, + {Name: "notmycontainer", Image: "foo/notmycontainer"}, + }, + }, + }, + }, + } + + p1 := &corev1.Pod{Spec: *gs.Spec.Template.Spec.DeepCopy()} + + p2 := gs.ApplyToPodGameServerContainer(p1, func(c corev1.Container) corev1.Container { + // easy thing to change and test for + c.TTY = true + + return c + }) + + assert.Len(t, p2.Spec.Containers, 2) + assert.True(t, p2.Spec.Containers[0].TTY) + assert.False(t, p2.Spec.Containers[1].TTY) +} diff --git a/pkg/gameservers/controller.go b/pkg/gameservers/controller.go index d1f4594ee4..113e32fd28 100644 --- a/pkg/gameservers/controller.go +++ b/pkg/gameservers/controller.go @@ -60,25 +60,26 @@ var ( // Controller is a the main GameServer crd controller type Controller struct { - baseLogger *logrus.Entry - sidecarImage string - alwaysPullSidecarImage bool - sidecarCPURequest resource.Quantity - sidecarCPULimit resource.Quantity - crdGetter v1beta1.CustomResourceDefinitionInterface - podGetter typedcorev1.PodsGetter - podLister corelisterv1.PodLister - podSynced cache.InformerSynced - gameServerGetter getterv1alpha1.GameServersGetter - gameServerLister listerv1alpha1.GameServerLister - gameServerSynced cache.InformerSynced - nodeLister corelisterv1.NodeLister - portAllocator *PortAllocator - healthController *HealthController - workerqueue *workerqueue.WorkerQueue - creationWorkerQueue *workerqueue.WorkerQueue // handles creation only - deletionWorkerQueue *workerqueue.WorkerQueue // handles deletion only - stop <-chan struct { + baseLogger *logrus.Entry + sidecarImage string + alwaysPullSidecarImage bool + disableGameServerContainerServiceAccount bool + sidecarCPURequest resource.Quantity + sidecarCPULimit resource.Quantity + crdGetter v1beta1.CustomResourceDefinitionInterface + podGetter typedcorev1.PodsGetter + podLister corelisterv1.PodLister + podSynced cache.InformerSynced + gameServerGetter getterv1alpha1.GameServersGetter + gameServerLister listerv1alpha1.GameServerLister + gameServerSynced cache.InformerSynced + nodeLister corelisterv1.NodeLister + portAllocator *PortAllocator + healthController *HealthController + workerqueue *workerqueue.WorkerQueue + creationWorkerQueue *workerqueue.WorkerQueue // handles creation only + deletionWorkerQueue *workerqueue.WorkerQueue // handles deletion only + stop <-chan struct { } recorder record.EventRecorder } @@ -92,6 +93,7 @@ func NewController( alwaysPullSidecarImage bool, sidecarCPURequest resource.Quantity, sidecarCPULimit resource.Quantity, + disableGameServerContainerServiceAccount bool, kubeClient kubernetes.Interface, kubeInformerFactory informers.SharedInformerFactory, extClient extclientset.Interface, @@ -103,20 +105,21 @@ func NewController( gsInformer := gameServers.Informer() c := &Controller{ - sidecarImage: sidecarImage, - sidecarCPULimit: sidecarCPULimit, - sidecarCPURequest: sidecarCPURequest, - alwaysPullSidecarImage: alwaysPullSidecarImage, - crdGetter: extClient.ApiextensionsV1beta1().CustomResourceDefinitions(), - podGetter: kubeClient.CoreV1(), - podLister: pods.Lister(), - podSynced: pods.Informer().HasSynced, - gameServerGetter: agonesClient.StableV1alpha1(), - gameServerLister: gameServers.Lister(), - gameServerSynced: gsInformer.HasSynced, - nodeLister: kubeInformerFactory.Core().V1().Nodes().Lister(), - portAllocator: NewPortAllocator(minPort, maxPort, kubeInformerFactory, agonesInformerFactory), - healthController: NewHealthController(kubeClient, agonesClient, kubeInformerFactory, agonesInformerFactory), + sidecarImage: sidecarImage, + sidecarCPULimit: sidecarCPULimit, + sidecarCPURequest: sidecarCPURequest, + disableGameServerContainerServiceAccount: disableGameServerContainerServiceAccount, + alwaysPullSidecarImage: alwaysPullSidecarImage, + crdGetter: extClient.ApiextensionsV1beta1().CustomResourceDefinitions(), + podGetter: kubeClient.CoreV1(), + podLister: pods.Lister(), + podSynced: pods.Informer().HasSynced, + gameServerGetter: agonesClient.StableV1alpha1(), + gameServerLister: gameServers.Lister(), + gameServerSynced: gsInformer.HasSynced, + nodeLister: kubeInformerFactory.Core().V1().Nodes().Lister(), + portAllocator: NewPortAllocator(minPort, maxPort, kubeInformerFactory, agonesInformerFactory), + healthController: NewHealthController(kubeClient, agonesClient, kubeInformerFactory, agonesInformerFactory), } c.baseLogger = runtime.NewLoggerWithType(c) @@ -525,6 +528,7 @@ func (c *Controller) createGameServerPod(gs *v1alpha1.GameServer) (*v1alpha1.Gam return gs, err } + c.disableServiceAccount(gs, pod) c.addGameServerHealthCheck(gs, pod) c.loggerForGameServer(gs).WithField("pod", pod).Info("creating Pod for GameServer") @@ -593,29 +597,47 @@ func (c *Controller) sidecar(gs *v1alpha1.GameServer) corev1.Container { return sidecar } +// disableServiceAccount disables the service account for the gameserver container +func (c *Controller) disableServiceAccount(gs *v1alpha1.GameServer, pod *corev1.Pod) { + if !c.disableGameServerContainerServiceAccount { + return + } + + // gameservers don't get access to the k8s api. + emptyVol := corev1.Volume{Name: "empty", VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{}}} + pod.Spec.Volumes = append(pod.Spec.Volumes, emptyVol) + mount := corev1.VolumeMount{MountPath: "/var/run/secrets/kubernetes.io/serviceaccount", Name: emptyVol.Name, ReadOnly: true} + + gs.ApplyToPodGameServerContainer(pod, func(c corev1.Container) corev1.Container { + c.VolumeMounts = append(c.VolumeMounts, mount) + + return c + }) +} + // addGameServerHealthCheck adds the http health check to the GameServer container func (c *Controller) addGameServerHealthCheck(gs *v1alpha1.GameServer, pod *corev1.Pod) { - if !gs.Spec.Health.Disabled { - for i, c := range pod.Spec.Containers { - if c.Name == gs.Spec.Container { - if c.LivenessProbe == nil { - c.LivenessProbe = &corev1.Probe{ - Handler: corev1.Handler{ - HTTPGet: &corev1.HTTPGetAction{ - Path: "/gshealthz", - Port: intstr.FromInt(8080), - }, - }, - InitialDelaySeconds: gs.Spec.Health.InitialDelaySeconds, - PeriodSeconds: gs.Spec.Health.PeriodSeconds, - FailureThreshold: gs.Spec.Health.FailureThreshold, - } - pod.Spec.Containers[i] = c - } - break + if gs.Spec.Health.Disabled { + return + } + + gs.ApplyToPodGameServerContainer(pod, func(c corev1.Container) corev1.Container { + if c.LivenessProbe == nil { + c.LivenessProbe = &corev1.Probe{ + Handler: corev1.Handler{ + HTTPGet: &corev1.HTTPGetAction{ + Path: "/gshealthz", + Port: intstr.FromInt(8080), + }, + }, + InitialDelaySeconds: gs.Spec.Health.InitialDelaySeconds, + PeriodSeconds: gs.Spec.Health.PeriodSeconds, + FailureThreshold: gs.Spec.Health.FailureThreshold, } } - } + + return c + }) } // syncGameServerStartingState looks for a pod that has been scheduled for this GameServer diff --git a/pkg/gameservers/controller_test.go b/pkg/gameservers/controller_test.go index d70adc668b..274fda51e4 100644 --- a/pkg/gameservers/controller_test.go +++ b/pkg/gameservers/controller_test.go @@ -1143,6 +1143,41 @@ func TestIsGameServerPod(t *testing.T) { } +func TestControllerDisableServiceAccount(t *testing.T) { + t.Parallel() + + t.Run("disabled", func(t *testing.T) { + c, _ := newFakeController() + c.disableGameServerContainerServiceAccount = true + + gs := &v1alpha1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "gameserver", UID: "1234"}, Spec: newSingleContainerSpec()} + gs.ApplyDefaults() + pod, err := gs.Pod() + assert.NoError(t, err) + assert.Len(t, pod.Spec.Containers, 1) + assert.Empty(t, pod.Spec.Containers[0].VolumeMounts) + + c.disableServiceAccount(gs, pod) + assert.Len(t, pod.Spec.Containers, 1) + assert.Len(t, pod.Spec.Containers[0].VolumeMounts, 1) + assert.Equal(t, "/var/run/secrets/kubernetes.io/serviceaccount", pod.Spec.Containers[0].VolumeMounts[0].MountPath) + }) + + t.Run("enabled", func(t *testing.T) { + c, _ := newFakeController() + c.disableGameServerContainerServiceAccount = false + + gs := &v1alpha1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "gameserver", UID: "1234"}, Spec: newSingleContainerSpec()} + gs.ApplyDefaults() + pod, err := gs.Pod() + assert.NoError(t, err) + + expected := pod.DeepCopy() + c.disableServiceAccount(gs, pod) + assert.Equal(t, expected, pod) + }) +} + // testNoChange runs a test with a state that doesn't exist, to ensure a handler // doesn't do process anything beyond the state it is meant to handle. func testNoChange(t *testing.T, state v1alpha1.GameServerState, f func(*Controller, *v1alpha1.GameServer) (*v1alpha1.GameServer, error)) { @@ -1188,7 +1223,8 @@ func newFakeController() (*Controller, agtesting.Mocks) { wh := webhooks.NewWebHook("", "") c := NewController(wh, healthcheck.NewHandler(), 10, 20, "sidecar:dev", false, - resource.MustParse("0.05"), resource.MustParse("0.1"), m.KubeClient, m.KubeInformerFactory, m.ExtClient, m.AgonesClient, m.AgonesInformerFactory) + resource.MustParse("0.05"), resource.MustParse("0.1"), true, + m.KubeClient, m.KubeInformerFactory, m.ExtClient, m.AgonesClient, m.AgonesInformerFactory) c.recorder = m.FakeRecorder return c, m } diff --git a/site/content/en/docs/Installation/helm.md b/site/content/en/docs/Installation/helm.md index c75579375f..43b9a123a6 100644 --- a/site/content/en/docs/Installation/helm.md +++ b/site/content/en/docs/Installation/helm.md @@ -166,6 +166,7 @@ The following tables lists the configurable parameters of the Agones chart and t | --------------------------------------------------- | ----------------------------------------------------------------------------------------------- | ---------------------- | | `agones.controller.persistentLogs` | Store Agones controller logs in a temporary volume attached to a container for debugging | `true` | | `agones.controller.persistentLogsSizeLimitMB` | Maximum total size of all Agones container logs in MB | `10000` | +| `gameservers.disableGameServerContainerServiceAccount` | When enabled, mounts an `emptyDir` over the service account token in the GameServer container, to stop game server processes from accessing Kubernetes | `true` | {{% /feature %}}