Repository: kumahq/kuma
Pull request #1220 (2020-11-26 11:28:41)

fix(*) stricter token sanitization in kuma-dp
### Summary

Add even stricter token sanitization.

### Documentation

- Not needed



Patch:
diff --git a/app/kuma-dp/pkg/config/validate.go b/app/kuma-dp/pkg/config/validate.go
index 5b7e16a09..57513d95c 100644
--- a/app/kuma-dp/pkg/config/validate.go
+++ b/app/kuma-dp/pkg/config/validate.go
@@ -2,6 +2,7 @@ package config
 
 import (
 	"io/ioutil"
+	"unicode"
 
 	"github.com/dgrijalva/jwt-go"
 	"github.com/pkg/errors"
@@ -26,10 +27,32 @@ func ValidateTokenPath(path string) error {
 		return errors.Wrapf(err, "could not read the token in the file %s", path)
 	}
 
-	_, _, err = new(jwt.Parser).ParseUnverified(string(rawToken), &jwt.MapClaims{})
+	token, parts, err := new(jwt.Parser).ParseUnverified(string(rawToken), &jwt.MapClaims{})
 	if err != nil {
-		return errors.Wrapf(err, "token in the file %s is not valid JWT token. Double check blank characters in the file", path)
+		return errors.Wrap(err, "not valid JWT token. Can't parse it.")
+	}
+
+	if token.Method.Alg() == "" {
+		return errors.New("not valid JWT token. No Alg.")
+	}
+
+	if token.Header == nil {
+		return errors.New("not valid JWT token. No Header.")
+	}
+	for _, part := range parts {
+		if !isASCII(part) {
+			return errors.New("The file cannot have blank characters like empty lines. Example how to get rid of non-printable characters: sed -i '' '/^$/d' token.file")
+		}
 	}
 
 	return nil
 }
+
+func isASCII(s string) bool {
+	for i := 0; i < len(s); i++ {
+		if !unicode.IsPrint(rune(s[i])) {
+			return false
+		}
+	}
+	return true
+}
diff --git a/pkg/core/managers/apis/mesh/mesh_manager.go b/pkg/core/managers/apis/mesh/mesh_manager.go
index 886232c3f..a2120f710 100644
--- a/pkg/core/managers/apis/mesh/mesh_manager.go
+++ b/pkg/core/managers/apis/mesh/mesh_manager.go
@@ -103,7 +103,7 @@ func (m *meshManager) Delete(ctx context.Context, resource core_model.Resource,
 	}
 	opts := core_store.NewDeleteOptions(fs...)
 	// delete all secrets
-	if err := m.otherManagers.DeleteAll(ctx, &system.SecretResourceList{}, core_store.DeleteAllByMesh(opts.Mesh)); err != nil {
+	if err := m.otherManagers.DeleteAll(ctx, &system.SecretResourceList{}, core_store.DeleteAllByMesh(opts.Name)); err != nil {
 		return errors.Wrap(err, "could not delete associated secrets")
 	}
 	return notFoundErr
diff --git a/pkg/core/secrets/manager/validator.go b/pkg/core/secrets/manager/validator.go
index d5b5a1ae1..97a687040 100644
--- a/pkg/core/secrets/manager/validator.go
+++ b/pkg/core/secrets/manager/validator.go
@@ -43,6 +43,9 @@ func (s *secretValidator) ValidateDelete(ctx context.Context, name string, mesh
 	meshRes := &mesh_core.MeshResource{}
 	err := s.store.Get(ctx, meshRes, core_store.GetByKey(mesh, model.NoMesh))
 	if err != nil {
+		if core_store.IsResourceNotFound(err) {
+			return nil // when Mesh no longer exist we should be able to safely delete a secret because it's not referenced anywhere
+		}
 		return err
 	}
 
diff --git a/pkg/plugins/bootstrap/k8s/plugin.go b/pkg/plugins/bootstrap/k8s/plugin.go
index 3cd5bb75c..2a15b43e3 100644
--- a/pkg/plugins/bootstrap/k8s/plugin.go
+++ b/pkg/plugins/bootstrap/k8s/plugin.go
@@ -3,6 +3,7 @@ package k8s
 import (
 	kube_runtime "k8s.io/apimachinery/pkg/runtime"
 	kube_ctrl "sigs.k8s.io/controller-runtime"
+	kube_client "sigs.k8s.io/controller-runtime/pkg/client"
 	kube_manager "sigs.k8s.io/controller-runtime/pkg/manager"
 
 	kuma_kube_cache "github.com/kumahq/kuma/pkg/plugins/bootstrap/k8s/cache"
@@ -25,8 +26,9 @@ func init() {
 
 func (p *plugin) BeforeBootstrap(b *core_runtime.Builder, _ core_plugins.PluginConfig) error {
 	scheme := kube_runtime.NewScheme()
+	config := kube_ctrl.GetConfigOrDie()
 	mgr, err := kube_ctrl.NewManager(
-		kube_ctrl.GetConfigOrDie(),
+		config,
 		kube_ctrl.Options{
 			Scheme:   scheme,
 			NewCache: kuma_kube_cache.New,
@@ -42,8 +44,22 @@ func (p *plugin) BeforeBootstrap(b *core_runtime.Builder, _ core_plugins.PluginC
 	if err != nil {
 		return err
 	}
+
+	// We need non cached client for resources that we don't have (get/list/watch) RBAC for all namespaces / cluster scope. Right now the only such resource is Secret
+	// Kubernetes cache lists resources under the hood from all Namespace unless we specify the "Namespace" in Options.
+	// If we don't do this the result is the following error: E1126 10:42:52.097662       1 reflector.go:178] pkg/mod/k8s.io/client-go@v0.18.9/tools/cache/reflector.go:125: Failed to list *v1.Secret: secrets is forbidden: User "system:serviceaccount:kuma-system:kuma-control-plane" cannot list resource "secrets" in API group "" at the cluster scope
+	// We cannot specify this Namespace parameter because it affect all the resources, therefore we need separate client for Secrets.
+	nonCachedClient, err := kube_client.New(config, kube_client.Options{
+		Scheme: scheme,
+		Mapper: mgr.GetRESTMapper(),
+	})
+	if err != nil {
+		return err
+	}
+
 	b.WithComponentManager(&kubeComponentManager{mgr})
 	b.WithExtensions(k8s_extensions.NewManagerContext(b.Extensions(), mgr))
+	b.WithExtensions(k8s_extensions.NewNonCachedClientContext(b.Extensions(), nonCachedClient))
 	if expTime := b.Config().Runtime.Kubernetes.MarshalingCacheExpirationTime; expTime > 0 {
 		b.WithExtensions(k8s_extensions.NewResourceConverterContext(b.Extensions(), k8s.NewCachingConverter(expTime)))
 	} else {
diff --git a/pkg/plugins/extensions/k8s/context.go b/pkg/plugins/extensions/k8s/context.go
index 1e1f002c1..cb2335dd1 100644
--- a/pkg/plugins/extensions/k8s/context.go
+++ b/pkg/plugins/extensions/k8s/context.go
@@ -4,6 +4,7 @@ import (
 	"context"
 
 	kube_ctrl "sigs.k8s.io/controller-runtime"
+	kube_client "sigs.k8s.io/controller-runtime/pkg/client"
 
 	k8s_common "github.com/kumahq/kuma/pkg/plugins/common/k8s"
 )
@@ -33,3 +34,14 @@ func FromResourceConverterContext(ctx context.Context) (converter k8s_common.Con
 	converter, ok = ctx.Value(converterKey{}).(k8s_common.Converter)
 	return
 }
+
+type nonCachedClient struct{}
+
+func NewNonCachedClientContext(ctx context.Context, client kube_client.Client) context.Context {
+	return context.WithValue(ctx, nonCachedClient{}, client)
+}
+
+func FromNonCachedClientContext(ctx context.Context) (client kube_client.Client, ok bool) {
+	client, ok = ctx.Value(nonCachedClient{}).(kube_client.Client)
+	return
+}
diff --git a/pkg/plugins/runtime/k8s/plugin.go b/pkg/plugins/runtime/k8s/plugin.go
index 8c11d7526..3961d87da 100644
--- a/pkg/plugins/runtime/k8s/plugin.go
+++ b/pkg/plugins/runtime/k8s/plugin.go
@@ -222,8 +222,12 @@ func addValidators(mgr kube_ctrl.Manager, rt core_runtime.Runtime, converter k8s
 	mgr.GetWebhookServer().Register("/validate-v1-service", &kube_webhook.Admission{Handler: &k8s_webhooks.ServiceValidator{}})
 	log.Info("Registering a validation webhook for v1/Service", "path", "/validate-v1-service")
 
+	client, ok := k8s_extensions.FromNonCachedClientContext(rt.Extensions())
+	if !ok {
+		return errors.Errorf("non cached client hasn't been configured")
+	}
 	secretValidator := &k8s_webhooks.SecretValidator{
-		Client:    mgr.GetClient(),
+		Client:    client,
 		Validator: manager.NewSecretValidator(rt.CaManagers(), rt.ResourceStore()),
 	}
 	mgr.GetWebhookServer().Register("/validate-v1-secret", &kube_webhook.Admission{Handler: secretValidator})
diff --git a/pkg/plugins/secrets/k8s/plugin.go b/pkg/plugins/secrets/k8s/plugin.go
index 444efed31..14ffdcb1c 100644
--- a/pkg/plugins/secrets/k8s/plugin.go
+++ b/pkg/plugins/secrets/k8s/plugin.go
@@ -26,5 +26,9 @@ func (p *plugin) NewSecretStore(pc core_plugins.PluginContext, _ core_plugins.Pl
 	if err := kube_core.AddToScheme(mgr.GetScheme()); err != nil {
 		return nil, errors.Wrapf(err, "could not add %q to scheme", kube_core.SchemeGroupVersion)
 	}
-	return NewStore(mgr.GetAPIReader(), mgr.GetClient(), pc.Config().Store.Kubernetes.SystemNamespace)
+	client, ok := k8s_extensions.FromNonCachedClientContext(pc.Extensions())
+	if !ok {
+		return nil, errors.Errorf("non cached client hasn't been configured")
+	}
+	return NewStore(client, client, pc.Config().Store.Kubernetes.SystemNamespace)
 }


Test patch:
diff --git a/app/kuma-dp/pkg/config/validate_test.go b/app/kuma-dp/pkg/config/validate_test.go
index 904ab1d06..8d70d0e78 100644
--- a/app/kuma-dp/pkg/config/validate_test.go
+++ b/app/kuma-dp/pkg/config/validate_test.go
@@ -6,6 +6,7 @@ import (
 	"os"
 
 	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/ginkgo/extensions/table"
 	. "github.com/onsi/gomega"
 
 	"github.com/kumahq/kuma/app/kuma-dp/pkg/config"
@@ -57,18 +58,40 @@ var _ = Describe("ValidateTokenPath", func() {
 		Expect(err).To(MatchError(fmt.Sprintf("token under file %s is empty", tokenFile.Name())))
 	})
 
-	It("should fail for invalid token", func() {
-		// setup
-		invalidTokenFile, err := ioutil.TempFile("", "")
-		Expect(err).ToNot(HaveOccurred())
-
-		_, err = invalidTokenFile.Write([]byte("invalid.token.file"))
-		Expect(err).ToNot(HaveOccurred())
+	Context("should valicate token", func() {
+		type testCase struct {
+			token    string
+			expected string
+		}
+		DescribeTable("should fail with invalid token",
+			func(given testCase) {
+				// setup
+				invalidTokenFile, err := ioutil.TempFile("", "")
+				Expect(err).ToNot(HaveOccurred())
+
+				_, err = invalidTokenFile.Write([]byte(given.token))
+				Expect(err).ToNot(HaveOccurred())
+
+				// when
+				err = config.ValidateTokenPath(invalidTokenFile.Name())
+
+				// then
+				Expect(err).To(HaveOccurred())
+				Expect(err.Error()).To(Equal(given.expected))
+			},
+			Entry("can't parse token", testCase{
+				token:    "yJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJOYW1lIjoidGVzdCIsIk1lc2giOiJkZWZhdWx0IiwiVGFncyI6e30sIlR5cGUiOiIifQ.rdQ6l_6hzT93Kbk9kO-kZYY7BaexUH8QknvbdRy_f6s",
+				expected: "not valid JWT token. Can't parse it.: invalid character 'È' looking for beginning of value",
+			}),
+			Entry("need 3 segments", testCase{
+				token:    "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJOYW1lIjoidGVzdCIsIk1lc2giOiJkZWZhdWx0IiwiVGFncyI6e30sIlR5cGUiOiIifQ",
+				expected: "not valid JWT token. Can't parse it.: token contains an invalid number of segments",
+			}),
+			Entry("new line in the end ", testCase{
+				token:    "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJOYW1lIjoidGVzdCIsIk1lc2giOiJkZWZhdWx0IiwiVGFncyI6e30sIlR5cGUiOiIifQ.rdQ6l_6hzT93Kbk9kO-kZYY7BaexUH8QknvbdRy_f6s\n",
+				expected: "The file cannot have blank characters like empty lines. Example how to get rid of non-printable characters: sed -i '' '/^$/d' token.file",
+			}),
+		)
 
-		// when
-		err = config.ValidateTokenPath(invalidTokenFile.Name())
-
-		// then
-		Expect(err).To(HaveOccurred())
 	})
 })
diff --git a/pkg/core/managers/apis/mesh/mesh_manager_test.go b/pkg/core/managers/apis/mesh/mesh_manager_test.go
index 2f11c175d..42d91d8d5 100644
--- a/pkg/core/managers/apis/mesh/mesh_manager_test.go
+++ b/pkg/core/managers/apis/mesh/mesh_manager_test.go
@@ -212,6 +212,34 @@ var _ = Describe("Mesh Manager", func() {
 		})
 	})
 
+	Describe("Delete()", func() {
+		It("should delete secrets within one mesh", func() {
+			// given two meshes
+			err := resManager.Create(context.Background(), &core_mesh.MeshResource{}, store.CreateByKey("demo-1", model.NoMesh))
+			Expect(err).ToNot(HaveOccurred())
+			err = resManager.Create(context.Background(), &core_mesh.MeshResource{}, store.CreateByKey("demo-2", model.NoMesh))
+			Expect(err).ToNot(HaveOccurred())
+
+			// when demo-1 is deleted
+			err = resManager.Delete(context.Background(), &core_mesh.MeshResource{}, store.DeleteByKey("demo-1", model.NoMesh))
+
+			// then
+			Expect(err).ToNot(HaveOccurred())
+
+			// and all secrets are deleted
+			secrets := &system.SecretResourceList{}
+			err = secretManager.List(context.Background(), secrets, store.ListByMesh("demo-1"))
+			Expect(err).ToNot(HaveOccurred())
+			Expect(secrets.Items).To(BeEmpty())
+
+			// and all secrets from other mesh are preserved
+			secrets = &system.SecretResourceList{}
+			err = secretManager.List(context.Background(), secrets, store.ListByMesh("demo-2"))
+			Expect(err).ToNot(HaveOccurred())
+			Expect(secrets.Items).To(HaveLen(1)) // default signing key
+		})
+	})
+
 	Describe("Update()", func() {
 		It("should not allow to change CA when mTLS is enabled", func() {
 			// given
diff --git a/pkg/core/secrets/manager/validator_test.go b/pkg/core/secrets/manager/validator_test.go
index 2d3cfa1a0..e91202257 100644
--- a/pkg/core/secrets/manager/validator_test.go
+++ b/pkg/core/secrets/manager/validator_test.go
@@ -109,11 +109,19 @@ var _ = Describe("Secret Validator", func() {
 		}),
 	)
 
-	It("should validate that secret is not in use", func() {
+	It("should pass validation of secrets that are not in use", func() {
 		// when
 		err := validator.ValidateDelete(context.Background(), "some-not-used-secret", "default")
 
 		// then
 		Expect(err).ToNot(HaveOccurred())
 	})
+
+	It("should pass validation of secrets in mesh that non exist", func() {
+		// when
+		err := validator.ValidateDelete(context.Background(), "some-not-used-secret", "non-existing-mesh")
+
+		// then
+		Expect(err).ToNot(HaveOccurred())
+	})
 })