Fix misleading jwt rsa key error (#203)

* test more RSA public key variants * simplify error handling with RSA public key creation Co-authored-by: Marcel Ludwig <marcel.ludwig@avenga.com>
coupergateway · Apr 13, 2021 · beec8d0 · beec8d0
1 parent 705493e
commit beec8d0
Show file tree

Hide file tree

Showing 2 changed files with 96 additions and 23 deletions.
diff --git a/accesscontrol/jwt.go b/accesscontrol/jwt.go
@@ -4,7 +4,6 @@ import (
 	"context"
 	"crypto/rsa"
 	"crypto/x509"
-	"encoding/base64"
 	"encoding/pem"
 	"errors"
 	"fmt"
@@ -88,21 +87,10 @@ func NewJWT(algorithm, name string, claims map[string]interface{}, reqClaims []s
 	}
 
 	pubKey, err := parsePublicPEMKey(key)
-	if err != nil && (err != jwt.ErrKeyMustBePEMEncoded || err != jwt.ErrNotRSAPublicKey) {
-		cert, err := x509.ParseCertificate(key)
-		if err != nil {
-			decKey, err := base64.StdEncoding.DecodeString(string(key))
-			if err != nil {
-				return nil, ErrorNotSupported
-			}
-			cert, err = x509.ParseCertificate(decKey)
-			if err != nil {
-				return nil, err
-			}
-		}
-		rsaPubKey, _ := cert.PublicKey.(*rsa.PublicKey)
-		pubKey = &rsa.PublicKey{N: rsaPubKey.N, E: rsaPubKey.E}
+	if err != nil {
+		return nil, err
 	}
+
 	jwtObj.pubKey = pubKey
 	return jwtObj, err
 }
@@ -252,14 +240,7 @@ func newParser(algo Algorithm, claims map[string]interface{}) (*jwt.Parser, erro
 func parsePublicPEMKey(key []byte) (pub *rsa.PublicKey, err error) {
 	pemBlock, _ := pem.Decode(key)
 	if pemBlock == nil {
-		decKey, err := base64.StdEncoding.DecodeString(string(key))
-		if err != nil {
-			return nil, ErrorNotSupported
-		}
-		pemBlock, _ = pem.Decode(decKey)
-		if pemBlock == nil {
-			return nil, jwt.ErrKeyMustBePEMEncoded
-		}
+		return nil, jwt.ErrKeyMustBePEMEncoded
 	}
 	pubKey, pubErr := x509.ParsePKCS1PublicKey(pemBlock.Bytes)
 	if pubErr != nil {

diff --git a/accesscontrol/jwt_test.go b/accesscontrol/jwt_test.go
@@ -16,6 +16,98 @@ import (
 	"github.com/avenga/couper/config/request"
 )
 
+func Test_JWT_NewJWT_RSA(t *testing.T) {
+	type fields struct {
+		algorithm      string
+		claims         map[string]interface{}
+		claimsRequired []string
+		source         ac.Source
+		sourceKey      string
+		pubKey         []byte
+	}
+
+	privKey, err := rsa.GenerateKey(rand.Reader, 2048)
+	bytes, err := x509.MarshalPKIXPublicKey(&privKey.PublicKey)
+	if err != nil {
+		panic(err)
+	}
+	pubKeyBytesPKIX := pem.EncodeToMemory(&pem.Block{
+		Type:  "PUBLIC KEY",
+		Bytes: bytes,
+	})
+	pubKeyBytesPKCS1 := pem.EncodeToMemory(&pem.Block{
+		Type:  "RSA PUBLIC KEY",
+		Bytes: x509.MarshalPKCS1PublicKey(&privKey.PublicKey),
+	})
+	privKeyBytes := pem.EncodeToMemory(&pem.Block{
+		Type:  "RSA PRIVATE KEY",
+		Bytes: x509.MarshalPKCS1PrivateKey(privKey),
+	})
+	// created using
+	// openssl req -new -newkey rsa:1024 -days 100000 -nodes -x509
+	certBytes := []byte(`-----BEGIN CERTIFICATE-----
+MIICaDCCAdGgAwIBAgIUZe+V/eBcYEaoORX8mfsyR8LqY/kwDQYJKoZIhvcNAQEL
+BQAwRTELMAkGA1UEBhMCQVUxEzARBgNVBAgMClNvbWUtU3RhdGUxITAfBgNVBAoM
+GEludGVybmV0IFdpZGdpdHMgUHR5IEx0ZDAgFw0yMTA0MTIxMzI1MzRaGA8yMjk1
+MDEyNjEzMjUzNFowRTELMAkGA1UEBhMCQVUxEzARBgNVBAgMClNvbWUtU3RhdGUx
+ITAfBgNVBAoMGEludGVybmV0IFdpZGdpdHMgUHR5IEx0ZDCBnzANBgkqhkiG9w0B
+AQEFAAOBjQAwgYkCgYEA2m79uRP+f/L6YgCuQoAiY6Qs5pccKR4DNfb+vQOsO+xx
+ZxWrY3RLSLOYKCBybHClz0JLT61duq7yfOl+03lYE6wTdy5XN1PGoijITj3cA6g1
+Eah6/CirrDVqEVIng+5lsw/Qws1gOOkHaCdfkL85Trm4AWqppgFgIc/wafHZjekC
+AwEAAaNTMFEwHQYDVR0OBBYEFCAUN20ma8sVaz1KZttyofv6tDZdMB8GA1UdIwQY
+MBaAFCAUN20ma8sVaz1KZttyofv6tDZdMA8GA1UdEwEB/wQFMAMBAf8wDQYJKoZI
+hvcNAQELBQADgYEADyu05JNvWly50lvUksx85QwEMb7oZha6aov/9eslJnHD10Zu
+QolLGgj3tz4NbDEitq+zKMr0uTHvP1Vyu1mXAflcpYcJA4ZmuB3Oj39e0U0gnmr/
+1T2dX1uHaAWl3pCmkRH1Dmpsx2sHllN/yizHpve2rrVpM9ZMXEdPxnzNNFE=
+-----END CERTIFICATE-----`)
+
+	for _, signingMethod := range []jwt.SigningMethod{
+		jwt.SigningMethodRS256, jwt.SigningMethodRS384, jwt.SigningMethodRS512,
+	} {
+		alg := signingMethod.Alg()
+		tests := []struct {
+			name    string
+			fields  fields
+			wantErr string
+		}{
+			{"missing key", fields{}, "either key_file or key must be specified"},
+			{"PKIX", fields{
+				algorithm: alg,
+				pubKey:    pubKeyBytesPKIX,
+			}, ""},
+			{"PKCS1", fields{
+				algorithm: alg,
+				pubKey:    pubKeyBytesPKCS1,
+			}, ""},
+			{"Cert", fields{
+				algorithm: alg,
+				pubKey:    certBytes,
+			}, ""},
+			{"Priv", fields{
+				algorithm: alg,
+				pubKey:    privKeyBytes,
+			}, "key is not a valid RSA public key"},
+		}
+		for _, tt := range tests {
+			t.Run(fmt.Sprintf("%v / %s", signingMethod, tt.name), func(t *testing.T) {
+				j, err := ac.NewJWT(tt.fields.algorithm, "test_ac", tt.fields.claims, tt.fields.claimsRequired, tt.fields.source, tt.fields.sourceKey, tt.fields.pubKey)
+				if err != nil {
+					if tt.wantErr != err.Error() {
+						t.Errorf("error: %v, want: %v", err, tt.wantErr)
+					}
+				} else if tt.wantErr != "" {
+					t.Errorf("error expected: %v", tt.wantErr)
+				}
+				if tt.wantErr == "" {
+					if j == nil {
+						t.Errorf("JWT struct expected")
+					}
+				}
+			})
+		}
+	}
+}
+
 func Test_JWT_Validate(t *testing.T) {
 	type fields struct {
 		algorithm      ac.Algorithm