From cec2f2b80403131c2d3f4fedd87ba5b09c558207 Mon Sep 17 00:00:00 2001 From: Pooneh Mortazavi Date: Wed, 13 May 2020 07:01:07 +0000 Subject: [PATCH] Use ServerCA instead of client secret ca.crt --- .../crds/gameserverallocationpolicy.yaml | 3 ++ install/yaml/install.yaml | 3 ++ .../v1/gameserverallocationpolicy.go | 2 + .../v1/gameserverallocationpolicy_test.go | 4 ++ .../multicluster/v1/zz_generated.deepcopy.go | 5 ++ pkg/gameserverallocations/allocator.go | 20 ++++---- pkg/gameserverallocations/controller_test.go | 48 ++++++++++++++++--- .../docs/Advanced/multi-cluster-allocation.md | 7 ++- .../Reference/agones_crd_api_reference.html | 11 +++++ test/e2e/allocator_test.go | 22 ++++----- 10 files changed, 98 insertions(+), 27 deletions(-) diff --git a/install/helm/agones/templates/crds/gameserverallocationpolicy.yaml b/install/helm/agones/templates/crds/gameserverallocationpolicy.yaml index ffeda5ab7c..0ae665b025 100644 --- a/install/helm/agones/templates/crds/gameserverallocationpolicy.yaml +++ b/install/helm/agones/templates/crds/gameserverallocationpolicy.yaml @@ -61,6 +61,9 @@ spec: properties: secretName: type: string + serverCa: + type: string + format: byte allocationEndpoints: items: type: string diff --git a/install/yaml/install.yaml b/install/yaml/install.yaml index 3dbebfe232..8d8821b260 100644 --- a/install/yaml/install.yaml +++ b/install/yaml/install.yaml @@ -768,6 +768,9 @@ spec: properties: secretName: type: string + serverCa: + type: string + format: byte allocationEndpoints: items: type: string diff --git a/pkg/apis/multicluster/v1/gameserverallocationpolicy.go b/pkg/apis/multicluster/v1/gameserverallocationpolicy.go index ca1d16aebc..670bef0cfa 100644 --- a/pkg/apis/multicluster/v1/gameserverallocationpolicy.go +++ b/pkg/apis/multicluster/v1/gameserverallocationpolicy.go @@ -42,6 +42,8 @@ type ClusterConnectionInfo struct { SecretName string `json:"secretName"` // The cluster namespace from which to allocate gameservers Namespace string `json:"namespace"` + // The PEM encoded server CA, used by the allocator client to authenticate the remote server. + ServerCA []byte `json:"serverCa,omitempty"` } // +genclient diff --git a/pkg/apis/multicluster/v1/gameserverallocationpolicy_test.go b/pkg/apis/multicluster/v1/gameserverallocationpolicy_test.go index a309a7954d..58904985e5 100644 --- a/pkg/apis/multicluster/v1/gameserverallocationpolicy_test.go +++ b/pkg/apis/multicluster/v1/gameserverallocationpolicy_test.go @@ -39,6 +39,7 @@ func TestConnectionInfoIterator(t *testing.T) { SecretName: "secret-name", AllocationEndpoints: []string{"allocation-endpoint"}, Namespace: "ns1", + ServerCA: []byte("c2VydmVyQ0E="), }, }, }, @@ -49,6 +50,7 @@ func TestConnectionInfoIterator(t *testing.T) { SecretName: "secret-name", AllocationEndpoints: []string{"allocation-endpoint"}, Namespace: "ns1", + ServerCA: []byte("c2VydmVyQ0E="), }, }, }, @@ -233,6 +235,7 @@ func TestConnectionInfoIterator(t *testing.T) { SecretName: "secret-name", AllocationEndpoints: []string{"alloc1", "alloc2"}, Namespace: "ns1", + ServerCA: []byte("c2VydmVyQ0E="), }, }, }, @@ -243,6 +246,7 @@ func TestConnectionInfoIterator(t *testing.T) { SecretName: "secret-name", AllocationEndpoints: []string{"alloc1", "alloc2"}, Namespace: "ns1", + ServerCA: []byte("c2VydmVyQ0E="), }, }, }, diff --git a/pkg/apis/multicluster/v1/zz_generated.deepcopy.go b/pkg/apis/multicluster/v1/zz_generated.deepcopy.go index 529528b1bd..85697c5ed4 100644 --- a/pkg/apis/multicluster/v1/zz_generated.deepcopy.go +++ b/pkg/apis/multicluster/v1/zz_generated.deepcopy.go @@ -32,6 +32,11 @@ func (in *ClusterConnectionInfo) DeepCopyInto(out *ClusterConnectionInfo) { *out = make([]string, len(*in)) copy(*out, *in) } + if in.ServerCA != nil { + in, out := &in.ServerCA, &out.ServerCA + *out = make([]byte, len(*in)) + copy(*out, *in) + } return } diff --git a/pkg/gameserverallocations/allocator.go b/pkg/gameserverallocations/allocator.go index 3fe9a9a0e2..47d224a6f3 100644 --- a/pkg/gameserverallocations/allocator.go +++ b/pkg/gameserverallocations/allocator.go @@ -67,7 +67,7 @@ var ( const ( secretClientCertName = "tls.crt" secretClientKeyName = "tls.key" - secretCaCertName = "ca.crt" + secretCACertName = "ca.crt" allocatorPort = "443" // Instead of selecting the top one, controller selects a random one @@ -324,7 +324,7 @@ func (c *Allocator) allocateFromRemoteCluster(gsa *allocationv1.GameServerAlloca var allocationResponse *pb.AllocationResponse // TODO: cache the client - dialOpts, err := c.createRemoteClusterDialOption(namespace, connectionInfo.SecretName) + dialOpts, err := c.createRemoteClusterDialOption(namespace, connectionInfo) if err != nil { return nil, err } @@ -363,13 +363,13 @@ func (c *Allocator) allocateFromRemoteCluster(gsa *allocationv1.GameServerAlloca } // createRemoteClusterDialOption creates a grpc client dial option with proper certs to make a remote call. -func (c *Allocator) createRemoteClusterDialOption(namespace, secretName string) (grpc.DialOption, error) { - clientCert, clientKey, caCert, err := c.getClientCertificates(namespace, secretName) +func (c *Allocator) createRemoteClusterDialOption(namespace string, connectionInfo *multiclusterv1.ClusterConnectionInfo) (grpc.DialOption, error) { + clientCert, clientKey, caCert, err := c.getClientCertificates(namespace, connectionInfo.SecretName) if err != nil { return nil, err } if clientCert == nil || clientKey == nil { - return nil, fmt.Errorf("missing client certificate key pair in secret %s", secretName) + return nil, fmt.Errorf("missing client certificate key pair in secret %s", connectionInfo.SecretName) } // Load client cert @@ -379,13 +379,17 @@ func (c *Allocator) createRemoteClusterDialOption(namespace, secretName string) } tlsConfig := &tls.Config{Certificates: []tls.Certificate{cert}} - if len(caCert) != 0 { + if len(connectionInfo.ServerCA) != 0 || len(caCert) != 0 { // Load CA cert, if provided and trust the server certificate. // This is required for self-signed certs. tlsConfig.RootCAs = x509.NewCertPool() - if !tlsConfig.RootCAs.AppendCertsFromPEM(caCert) { + if len(connectionInfo.ServerCA) != 0 && !tlsConfig.RootCAs.AppendCertsFromPEM(connectionInfo.ServerCA) { return nil, errors.New("only PEM format is accepted for server CA") } + // Add client CA cert, added for backward compatibility + if len(caCert) != 0 { + _ = tlsConfig.RootCAs.AppendCertsFromPEM(caCert) + } } return grpc.WithTransportCredentials(credentials.NewTLS(tlsConfig)), nil @@ -404,7 +408,7 @@ func (c *Allocator) getClientCertificates(namespace, secretName string) (clientC // Create http client using cert clientCert = secret.Data[secretClientCertName] clientKey = secret.Data[secretClientKeyName] - caCert = secret.Data[secretCaCertName] + caCert = secret.Data[secretCACertName] return clientCert, clientKey, caCert, nil } diff --git a/pkg/gameserverallocations/controller_test.go b/pkg/gameserverallocations/controller_test.go index ea38cba00d..0896892ee5 100644 --- a/pkg/gameserverallocations/controller_test.go +++ b/pkg/gameserverallocations/controller_test.go @@ -808,6 +808,7 @@ func TestMultiClusterAllocationFromLocal(t *testing.T) { ClusterName: "multicluster", SecretName: "localhostsecret", Namespace: defaultNs, + ServerCA: []byte("not-used"), }, }, ObjectMeta: metav1.ObjectMeta{ @@ -927,6 +928,7 @@ func TestMultiClusterAllocationFromRemote(t *testing.T) { ClusterName: clusterName, SecretName: secretName, Namespace: targetedNamespace, + ServerCA: clientCert, }, }, ObjectMeta: metav1.ObjectMeta{ @@ -939,7 +941,7 @@ func TestMultiClusterAllocationFromRemote(t *testing.T) { m.KubeClient.AddReactor("list", "secrets", func(action k8stesting.Action) (bool, k8sruntime.Object, error) { - return true, getTestSecret(secretName, clientCert), nil + return true, getTestSecret(secretName, nil), nil }) stop, cancel := agtesting.StartInformers(m, c.allocator.allocationPolicySynced, c.allocator.secretSynced, c.allocator.readyGameServerCache.gameServerSynced) @@ -1013,6 +1015,7 @@ func TestMultiClusterAllocationFromRemote(t *testing.T) { AllocationEndpoints: []string{endpoint}, ClusterName: clusterName, SecretName: secretName, + ServerCA: clientCert, }, }, ObjectMeta: metav1.ObjectMeta{ @@ -1028,6 +1031,7 @@ func TestMultiClusterAllocationFromRemote(t *testing.T) { AllocationEndpoints: []string{endpoint}, ClusterName: "remotecluster2", SecretName: secretName, + ServerCA: clientCert, }, }, ObjectMeta: metav1.ObjectMeta{ @@ -1108,6 +1112,7 @@ func TestMultiClusterAllocationFromRemote(t *testing.T) { AllocationEndpoints: []string{unhealthyEndpoint, healthyEndpoint}, ClusterName: clusterName, SecretName: secretName, + ServerCA: clientCert, }, }, ObjectMeta: metav1.ObjectMeta{ @@ -1158,7 +1163,11 @@ func TestCreateRestClientError(t *testing.T) { t.Parallel() t.Run("Missing secret", func(t *testing.T) { c, _ := newFakeController() - _, err := c.allocator.createRemoteClusterDialOption(defaultNs, "secret-name") + + connectionInfo := &multiclusterv1.ClusterConnectionInfo{ + SecretName: "secret-name", + } + _, err := c.allocator.createRemoteClusterDialOption(defaultNs, connectionInfo) assert.Error(t, err) assert.Contains(t, err.Error(), "secret-name") }) @@ -1182,7 +1191,10 @@ func TestCreateRestClientError(t *testing.T) { _, cancel := agtesting.StartInformers(m, c.allocator.secretSynced) defer cancel() - _, err := c.allocator.createRemoteClusterDialOption(defaultNs, "secret-name") + connectionInfo := &multiclusterv1.ClusterConnectionInfo{ + SecretName: "secret-name", + } + _, err := c.allocator.createRemoteClusterDialOption(defaultNs, connectionInfo) assert.Error(t, err) assert.Contains(t, err.Error(), "missing client certificate key pair in secret secret-name") }) @@ -1207,7 +1219,10 @@ func TestCreateRestClientError(t *testing.T) { _, cancel := agtesting.StartInformers(m, c.allocator.secretSynced) defer cancel() - _, err := c.allocator.createRemoteClusterDialOption(defaultNs, "secret-name") + connectionInfo := &multiclusterv1.ClusterConnectionInfo{ + SecretName: "secret-name", + } + _, err := c.allocator.createRemoteClusterDialOption(defaultNs, connectionInfo) assert.Error(t, err) assert.Contains(t, err.Error(), "failed to find any PEM data in certificate input") }) @@ -1216,16 +1231,37 @@ func TestCreateRestClientError(t *testing.T) { m.KubeClient.AddReactor("list", "secrets", func(action k8stesting.Action) (bool, k8sruntime.Object, error) { - return true, getTestSecret("secret-name", []byte("XXX")), nil + return true, getTestSecret("secret-name", clientCert), nil }) _, cancel := agtesting.StartInformers(m, c.allocator.secretSynced) defer cancel() - _, err := c.allocator.createRemoteClusterDialOption(defaultNs, "secret-name") + connectionInfo := &multiclusterv1.ClusterConnectionInfo{ + SecretName: "secret-name", + ServerCA: []byte("XXX"), + } + _, err := c.allocator.createRemoteClusterDialOption(defaultNs, connectionInfo) assert.Error(t, err) assert.Contains(t, err.Error(), "PEM format") }) + t.Run("Bad client CA cert", func(t *testing.T) { + c, m := newFakeController() + + m.KubeClient.AddReactor("list", "secrets", + func(action k8stesting.Action) (bool, k8sruntime.Object, error) { + return true, getTestSecret("secret-name", []byte("XXX")), nil + }) + + _, cancel := agtesting.StartInformers(m, c.allocator.secretSynced) + defer cancel() + + connectionInfo := &multiclusterv1.ClusterConnectionInfo{ + SecretName: "secret-name", + } + _, err := c.allocator.createRemoteClusterDialOption(defaultNs, connectionInfo) + assert.Nil(t, err) + }) } func executeAllocation(gsa *allocationv1.GameServerAllocation, c *Controller) (*allocationv1.GameServerAllocation, error) { diff --git a/site/content/en/docs/Advanced/multi-cluster-allocation.md b/site/content/en/docs/Advanced/multi-cluster-allocation.md index d01fe42cfa..f3c1f26717 100644 --- a/site/content/en/docs/Advanced/multi-cluster-allocation.md +++ b/site/content/en/docs/Advanced/multi-cluster-allocation.md @@ -35,6 +35,7 @@ spec: clusterName: "clusterB" namespace: cluster-B-ns secretName: allocator-client-to-cluster-B + sercerCA: c2VydmVyQ0E= priority: 1 weight: 100 EOF @@ -42,6 +43,10 @@ EOF To define the local cluster priority, similarly, an allocation rule should be defined, while leaving allocationEndpoints unset. If the local cluster priority is not defined, the allocation from the local cluster happens only if allocation from other clusters with the existing allocation rules is unsuccessful. +{{% feature publishVersion="1.6.0" %}} +`sercerCA` is the server TLS CA public certificate, set only if the remote server certificate is not signed by a public CA (e.g. self-signed). +{{% /feature %}} + ## Establish trust To accept allocation requests from other clusters, agones-allocator for cluster B should be configured to accept the client's certificate from cluster A and the cluster A's client should be configured to accept the server TLS certificate, if it is not signed by a public Certificate Authority (CA). @@ -67,7 +72,7 @@ EOF The certificates are base 64 string of the certificate file e.g. `cat ${CERT_FILE} | base64 -w 0` -`ca.crt` is the server TLS public certificate if it is self-signed. For simplicity, it is recommended to use one client secret per cluster and make `ca.crt` bundle of server certificates. +Agones recommends using [cert-manager.io](https://cert-manager.io/) solution for generating client certificates. 2.Add client CA to the list of authorized client certificates by agones-allocator in the targeted cluster. diff --git a/site/content/en/docs/Reference/agones_crd_api_reference.html b/site/content/en/docs/Reference/agones_crd_api_reference.html index bb173efc74..5a849b55d3 100644 --- a/site/content/en/docs/Reference/agones_crd_api_reference.html +++ b/site/content/en/docs/Reference/agones_crd_api_reference.html @@ -5247,6 +5247,17 @@

ClusterConnectionInfo

The cluster namespace from which to allocate gameservers

+ + +serverCa
+ +[]byte + + + +

The PEM encoded server CA, used by the allocator client to authenticate the remote server.

+ +

ConnectionInfoIterator diff --git a/test/e2e/allocator_test.go b/test/e2e/allocator_test.go index 61b1d246fd..672f9de0bc 100644 --- a/test/e2e/allocator_test.go +++ b/test/e2e/allocator_test.go @@ -51,7 +51,6 @@ const ( allocatorClientCAName = "allocator-client-ca" tlsCrtTag = "tls.crt" tlsKeyTag = "tls.key" - serverCATag = "ca.crt" allocatorReqURLFmt = "%s:%d" ) @@ -65,7 +64,7 @@ func TestAllocator(t *testing.T) { defer framework.DeleteNamespace(t, namespace) clientSecretName := fmt.Sprintf("allocator-client-%s", uuid.NewUUID()) - genClientSecret(t, tlsCA, namespace, clientSecretName) + genClientSecret(t, namespace, clientSecretName) flt, err := createFleet(namespace) if !assert.Nil(t, err) { @@ -83,7 +82,7 @@ func TestAllocator(t *testing.T) { // wait for the allocation system to come online err = wait.PollImmediate(2*time.Second, 5*time.Minute, func() (bool, error) { // create the grpc client each time, as we may end up looking at an old cert - dialOpts, err := createRemoteClusterDialOption(namespace, clientSecretName) + dialOpts, err := createRemoteClusterDialOption(namespace, clientSecretName, tlsCA) if err != nil { return false, err } @@ -127,7 +126,7 @@ func TestAllocatorCrossNamespace(t *testing.T) { // Create client secret A, B is receiver of the request and does not need client secret clientSecretNameA := fmt.Sprintf("allocator-client-%s", uuid.NewUUID()) - genClientSecret(t, tlsCA, namespaceA, clientSecretNameA) + genClientSecret(t, namespaceA, clientSecretNameA) policyName := fmt.Sprintf("a-to-b-%s", uuid.NewUUID()) p := &multiclusterv1.GameServerAllocationPolicy{ @@ -142,6 +141,7 @@ func TestAllocatorCrossNamespace(t *testing.T) { SecretName: clientSecretNameA, Namespace: namespaceB, AllocationEndpoints: []string{ip}, + ServerCA: tlsCA, }, }, } @@ -163,7 +163,7 @@ func TestAllocatorCrossNamespace(t *testing.T) { // wait for the allocation system to come online err = wait.PollImmediate(2*time.Second, 5*time.Minute, func() (bool, error) { // create the grpc client each time, as we may end up looking at an old cert - dialOpts, err := createRemoteClusterDialOption(namespaceA, clientSecretNameA) + dialOpts, err := createRemoteClusterDialOption(namespaceA, clientSecretNameA, tlsCA) if err != nil { return false, err } @@ -220,7 +220,7 @@ func getAllocatorEndpoint(t *testing.T) (string, int32) { } // createRemoteClusterDialOption creates a grpc client dial option with proper certs to make a remote call. -func createRemoteClusterDialOption(namespace, clientSecretName string) (grpc.DialOption, error) { +func createRemoteClusterDialOption(namespace, clientSecretName string, tlsCA []byte) (grpc.DialOption, error) { kubeCore := framework.KubeClient.CoreV1() clientSecret, err := kubeCore.Secrets(namespace).Get(clientSecretName, metav1.GetOptions{}) if err != nil { @@ -230,7 +230,6 @@ func createRemoteClusterDialOption(namespace, clientSecretName string) (grpc.Dia // Create http client using cert clientCert := clientSecret.Data[tlsCrtTag] clientKey := clientSecret.Data[tlsKeyTag] - tlsCA := clientSecret.Data[serverCATag] if clientCert == nil || clientKey == nil { return nil, errors.New("missing certificate") } @@ -279,7 +278,7 @@ func restartAllocator(t *testing.T) { } } -func genClientSecret(t *testing.T, serverCA []byte, namespace, secretName string) { +func genClientSecret(t *testing.T, namespace, secretName string) { t.Helper() pub, priv := generateTLSCertPair(t, "") @@ -291,9 +290,8 @@ func genClientSecret(t *testing.T, serverCA []byte, namespace, secretName string Namespace: namespace, }, Data: map[string][]byte{ - tlsCrtTag: pub, - tlsKeyTag: priv, - serverCATag: serverCA, + tlsCrtTag: pub, + tlsKeyTag: priv, }, } if _, err := kubeCore.Secrets(namespace).Create(s); err != nil { @@ -306,7 +304,7 @@ func genClientSecret(t *testing.T, serverCA []byte, namespace, secretName string if err != nil { t.Fatalf("getting secret %s/%s failed: %s", agonesSystemNamespace, allocatorClientCAName, err) } - s.Data["ca.crt"] = serverCA + s.Data["client-ca.crt"] = pub clientCASecret, err := kubeCore.Secrets(agonesSystemNamespace).Update(s) if err != nil {