review remarks

Signed-off-by: Jakub Dyszkiewicz <[email protected]>
kumahq · Nov 30, 2021 · 03fa106 · 03fa106
1 parent 6146402
commit 03fa106
Show file tree

Hide file tree

Showing 9 changed files with 37 additions and 31 deletions.
diff --git a/pkg/core/tokens/meshed_signing_key_accessor.go b/pkg/core/tokens/meshed_signing_key_accessor.go
@@ -21,7 +21,7 @@ var _ SigningKeyAccessor = &meshedSigningKeyAccessor{}
 
 // NewMeshedSigningKeyAccessor builds SigningKeyAccessor that is bound to a Mesh.
 // Some tokens like Dataplane Token are bound to a mesh.
-// In this case, singing key is also stored as a Secret in the Mesh, not as GlobalSecret.
+// In this case, the singing key is also stored as a Secret in the Mesh, not as GlobalSecret.
 func NewMeshedSigningKeyAccessor(resManager manager.ResourceManager, signingKeyPrefix string, mesh string) SigningKeyAccessor {
 	return &meshedSigningKeyAccessor{
 		resManager:       resManager,

diff --git a/pkg/core/tokens/meshed_signing_key_manager.go b/pkg/core/tokens/meshed_signing_key_manager.go
@@ -42,12 +42,13 @@ func (s *meshedSigningKeyManager) GetLatestSigningKey() (*rsa.PrivateKey, int, e
 	var signingKey *system.SecretResource
 	highestSerialNumber := -1
 	for _, resource := range resources.Items {
-		if strings.HasPrefix(resource.Meta.GetName(), s.signingKeyPrefix) {
-			serialNumber, _ := signingKeySerialNumber(resource.Meta.GetName(), s.signingKeyPrefix)
-			if serialNumber > highestSerialNumber {
-				signingKey = resource
-				highestSerialNumber = serialNumber
-			}
+		if !strings.HasPrefix(resource.Meta.GetName(), s.signingKeyPrefix) {
+			continue
+		}
+		serialNumber, _ := signingKeySerialNumber(resource.Meta.GetName(), s.signingKeyPrefix)
+		if serialNumber > highestSerialNumber {
+			signingKey = resource
+			highestSerialNumber = serialNumber
 		}
 	}
 
@@ -73,7 +74,7 @@ func (s *meshedSigningKeyManager) CreateDefaultSigningKey() error {
 func (s *meshedSigningKeyManager) CreateSigningKey(serialNumber int) error {
 	key, err := NewSigningKey()
 	if err != nil {
-		return errors.Wrap(err, "could not construct signing key")
+		return err
 	}
 
 	secret := system.NewSecretResource()
@@ -82,8 +83,5 @@ func (s *meshedSigningKeyManager) CreateSigningKey(serialNumber int) error {
 			Value: key,
 		},
 	}
-	if err := s.manager.Create(context.Background(), secret, store.CreateBy(SigningKeyResourceKey(s.signingKeyPrefix, serialNumber, s.mesh))); err != nil {
-		return errors.Wrap(err, "could not create signing key")
-	}
-	return nil
+	return s.manager.Create(context.Background(), secret, store.CreateBy(SigningKeyResourceKey(s.signingKeyPrefix, serialNumber, s.mesh)))
 }
diff --git a/pkg/core/tokens/signing_key.go b/pkg/core/tokens/signing_key.go
@@ -48,12 +48,12 @@ func (s *SigningKeyNotFound) Error() string {
 }
 
 func IsSigningKeyNotFound(err error) bool {
-	_, ok := err.(*SigningKeyNotFound)
-	return ok
+	target := &SigningKeyNotFound{}
+	return errors.As(err, &target)
 }
 
 func keyBytesToRsaKey(keyBytes []byte) (*rsa.PrivateKey, error) {
-	if util_rsa.IsRsaPEMBytes(keyBytes) {
+	if util_rsa.IsPEMBytes(keyBytes) {
 		key, err := util_rsa.FromPEMBytes(keyBytes)
 		if err != nil {
 			return nil, err

diff --git a/pkg/core/tokens/signing_key_manager.go b/pkg/core/tokens/signing_key_manager.go
@@ -53,12 +53,13 @@ func (s *signingKeyManager) GetLatestSigningKey() (*rsa.PrivateKey, int, error)
 	var signingKey *system.GlobalSecretResource
 	highestSerialNumber := -1
 	for _, resource := range resources.Items {
-		if strings.HasPrefix(resource.Meta.GetName(), s.signingKeyPrefix) {
-			serialNumber, _ := signingKeySerialNumber(resource.Meta.GetName(), s.signingKeyPrefix)
-			if serialNumber > highestSerialNumber {
-				signingKey = resource
-				highestSerialNumber = serialNumber
-			}
+		if !strings.HasPrefix(resource.Meta.GetName(), s.signingKeyPrefix) {
+			continue
+		}
+		serialNumber, _ := signingKeySerialNumber(resource.Meta.GetName(), s.signingKeyPrefix)
+		if serialNumber > highestSerialNumber {
+			signingKey = resource
+			highestSerialNumber = serialNumber
 		}
 	}
 
@@ -80,7 +81,7 @@ func (s *signingKeyManager) CreateDefaultSigningKey() error {
 func (s *signingKeyManager) CreateSigningKey(serialNumber int) error {
 	key, err := NewSigningKey()
 	if err != nil {
-		return errors.Wrap(err, "could not construct signing key")
+		return err
 	}
 
 	secret := system.NewGlobalSecretResource()
@@ -89,8 +90,5 @@ func (s *signingKeyManager) CreateSigningKey(serialNumber int) error {
 			Value: key,
 		},
 	}
-	if err := s.manager.Create(context.Background(), secret, store.CreateBy(SigningKeyResourceKey(s.signingKeyPrefix, serialNumber, model.NoMesh))); err != nil {
-		return errors.Wrap(err, "could not create signing key")
-	}
-	return nil
+	return s.manager.Create(context.Background(), secret, store.CreateBy(SigningKeyResourceKey(s.signingKeyPrefix, serialNumber, model.NoMesh)))
 }
diff --git a/pkg/core/tokens/validator.go b/pkg/core/tokens/validator.go
@@ -32,10 +32,14 @@ func (j *jwtTokenValidator) ParseWithValidation(rawToken Token, claims Claims) e
 		if err != nil {
 			return nil, err
 		}
-		if token.Method.Alg() == jwt.SigningMethodHS256.Name {
+		switch token.Method.Alg() {
+		case jwt.SigningMethodHS256.Name:
 			return j.keyAccessor.GetLegacyKey(serialNumber)
+		case jwt.SigningMethodRS256.Name:
+			return j.keyAccessor.GetPublicKey(serialNumber)
+		default:
+			return nil, errors.Errorf("unknown token alg. Allowed algs are %s and %s", jwt.SigningMethodHS256.Name, jwt.SigningMethodRS256.Name)
 		}
-		return j.keyAccessor.GetPublicKey(serialNumber)
 	})
 	if err != nil {
 		if verr, ok := err.(*jwt.ValidationError); ok { // jwt.ValidationError does not implement Unwrap() to just use errors.As

diff --git a/pkg/defaults/mesh/mesh.go b/pkg/defaults/mesh/mesh.go
@@ -83,7 +83,7 @@ func EnsureDefaultMeshResources(resManager manager.ResourceManager, meshName str
 		resKey := tokens.SigningKeyResourceKey(issuer.DataplaneTokenSigningKeyPrefix(meshName), tokens.DefaultSerialNumber, meshName)
 		log.Info("default Dataplane Token Signing Key created", "mesh", meshName, "name", resKey.Name)
 	} else {
-		log.Info("Dataplane Token Signing Key already exist", "mesh", meshName)
+		log.Info("Dataplane Token Signing Key already exists", "mesh", meshName)
 	}
 	return nil
 }
diff --git a/pkg/tokens/builtin/server/webservice.go b/pkg/tokens/builtin/server/webservice.go
@@ -66,6 +66,12 @@ func (d *tokenWebService) handleIdentityRequest(request *restful.Request, respon
 	validForErr, validFor := validateValidFor(idReq.ValidFor)
 	verr.Add(validForErr)
 
+	if idReq.Type != "" {
+		if err := mesh_proto.ProxyType(idReq.Type).IsValid(); err != nil {
+			verr.AddViolation("type", err.Error())
+		}
+	}
+
 	if verr.HasViolations() {
 		errors.HandleError(response, verr.OrNil(), "Invalid request")
 		return

diff --git a/pkg/util/rsa/pem.go b/pkg/util/rsa/pem.go
@@ -31,7 +31,7 @@ func FromPEMBytes(b []byte) (*rsa.PrivateKey, error) {
 	return x509.ParsePKCS1PrivateKey(block.Bytes)
 }
 
-func IsRsaPEMBytes(b []byte) bool {
+func IsPEMBytes(b []byte) bool {
 	block, _ := pem.Decode(b)
 	return block != nil && block.Type == rsaBlockType
 }
diff --git a/pkg/xds/auth/universal/auth_test.go b/pkg/xds/auth/universal/auth_test.go
@@ -261,6 +261,6 @@ var _ = Describe("Authentication flow", func() {
 		}, 24*time.Hour)
 
 		// then
-		Expect(err.Error()).To(Equal(`there is no signing key with serial number 1. Secret of name "dataplane-token-signing-key-demo-2-1" in mesh "demo-2" is not found. If signing key was rotated, regenerate the token`))
+		Expect(err.Error()).To(ContainSubstring(`there is no signing key`))
 	})
 })