Fix certificate selector parsing

Fix string-based certificate selector parsing

Add unit tests for certificate selector parsing

Author: 13ajay

cmd/credentials.go

@@ -38,10 +38,10 @@ var (
 	X509_ISSUER_KEY  = "x509Issuer"
 	X509_SERIAL_KEY  = "x509Serial"
 
-	validCertSelectorKeys = map[string]struct{}{
-		"x509Subject": {},
-		"x509Issuer":  {},
-		"x509Serial":  {},
+	validCertSelectorKeys = []string{
+		X509_SUBJECT_KEY,
+		X509_ISSUER_KEY,
+		X509_SERIAL_KEY,
 	}
 )
 
@@ -87,18 +87,24 @@ func getStringMap(s string) (map[string]string, error) {
 		}
 		key := strings.TrimSpace(strings.Join(keyTokens[1:], "="))
 
+		isValidKey := false
+		for _, validKey := range validCertSelectorKeys {
+			if validKey == key {
+				isValidKey = true
+				break
+			}
+		}
+		if !isValidKey {
+			return nil, errors.New("cert selector contained invalid key")
+		}
+
 		valueTokens := strings.Split(tokens[1], "=")
 		if valueTokens[0] != "Value" {
 			return nil, errors.New("invalid cert selector map value")
 		}
 		value := strings.TrimSpace(strings.Join(valueTokens[1:], "="))
 		m[key] = value
 	}
-	for key := range m {
-		if _, ok := m[key]; !ok {
-			return nil, errors.New("invalid cert selector map key")
-		}
-	}
 
 	return m, nil
 }
@@ -112,6 +118,16 @@ func getMapFromJsonEntries(jsonStr string) (map[string]string, error) {
 		return nil, errors.New("unable to parse JSON map entries")
 	}
 	for _, mapEntry := range mapEntries {
+		isValidKey := false
+		for _, validKey := range validCertSelectorKeys {
+			if validKey == mapEntry.Key {
+				isValidKey = true
+				break
+			}
+		}
+		if !isValidKey {
+			return nil, errors.New("cert selector contained invalid key")
+		}
 		m[mapEntry.Key] = mapEntry.Value
 	}
 	return m, nil
@@ -146,16 +162,9 @@ func PopulateCertIdentifierFromJsonStr(jsonStr string) (helper.CertIdentifier, e
 
 // Populates a CertIdentifier object using a cert selector string
 func PopulateCertIdentifierFromCertSelectorStr(certSelectorStr string) (helper.CertIdentifier, error) {
-	var certIdentifier helper.CertIdentifier
-
-	err := json.Unmarshal([]byte(certSelector), &certIdentifier)
-	certSelectorMap, err := getStringMap(certSelector)
+	certSelectorMap, err := getStringMap(certSelectorStr)
 	if err != nil {
-		certSelectorMap, err = getMapFromJsonEntries(certSelector)
-		if err != nil {
-			msg := "unable to parse cert selector"
-			return helper.CertIdentifier{}, errors.New(msg)
-		}
+		return helper.CertIdentifier{}, err
 	}
 
 	return createCertSelectorFromMap(certSelectorMap), nil
@@ -173,11 +182,14 @@ func PopulateCertIdentifier(certSelector string) (helper.CertIdentifier, error)
 				return helper.CertIdentifier{}, errors.New("unable to read cert selector file")
 			}
 			certIdentifier, err = PopulateCertIdentifierFromJsonStr(string(certSelectorFile[:]))
+			if err != nil {
+				return helper.CertIdentifier{}, errors.New("unable to parse JSON cert selector")
+			}
 		} else {
 			certIdentifier, err = PopulateCertIdentifierFromCertSelectorStr(certSelector)
-		}
-		if err != nil {
-			return helper.CertIdentifier{}, errors.New("unable to read cert selector")
+			if err != nil {
+				return helper.CertIdentifier{}, errors.New("unable to parse cert selector string")
+			}
 		}
 	}
 

cmd/credentials_test.go

@@ -0,0 +1,45 @@
+package cmd
+
+import (
+	"os"
+	"testing"
+)
+
+func TestMain(m *testing.M) {
+	code := m.Run()
+	os.Exit(code)
+}
+
+func TestValidSelectorParsing(t *testing.T) {
+	fixtures := []string{
+		"file://../tst/selectors/valid-all-attributes-selector.json",
+		"file://../tst/selectors/valid-some-attributes-selector.json",
+		"Key=x509Subject,Value=CN=Subject Key=x509Issuer,Value=CN=Issuer Key=x509Serial,Value=15D19632234BF759A32802C0DA88F9E8AFC8702D",
+		"Key=x509Issuer,Value=CN=Issuer",
+	}
+	for _, fixture := range fixtures {
+		_, err := PopulateCertIdentifier(fixture)
+		if err != nil {
+			t.Log("Unable to populate cert identifier from selector")
+			t.Fail()
+		}
+	}
+}
+
+func TestInvalidSelectorParsing(t *testing.T) {
+	fixtures := []string{
+		"file://../tst/selectors/invalid-selector.json",
+		"file://../tst/selectors/invalid-selector-2.json",
+		"file://../tst/selectors/invalid-selector-3.json",
+		"laksdjadf",
+		"Key=laksdjf,Valalsd",
+		"Key=aljsdf,Value=aljsdfadsf",
+	}
+	for _, fixture := range fixtures {
+		_, err := PopulateCertIdentifier(fixture)
+		if err == nil {
+			t.Log("Expected parsing failure, but received none")
+			t.Fail()
+		}
+	}
+}

tst/selectors/invalid-selector-2.json

@@ -0,0 +1,3 @@
+{
+    "lajsdf": "lajsdlfjadf"
+}

tst/selectors/invalid-selector-3.json

@@ -0,0 +1,4 @@
+{
+    "Key": "x509Serial", 
+    "Value": "alskjdfa"
+}

tst/selectors/invalid-selector.json

@@ -0,0 +1 @@
+asldjf;laj;laweijncaljda

tst/selectors/valid-all-attributes-selector.json

@@ -0,0 +1,14 @@
+[
+    {
+        "Key": "x509Subject",
+        "Value": "CN=Subject"
+    },
+    {
+        "Key": "x509Issuer",
+        "Value": "CN=Issuer"
+    },
+    {
+        "Key": "x509Serial",
+        "Value": "15D19632234BF759A32802C0DA88F9E8AFC8702D"
+    }
+]

tst/selectors/valid-some-attributes-selector.json

@@ -0,0 +1,10 @@
+[
+    {
+        "Key": "x509Subject",
+        "Value": "CN=Subject"
+    },
+    {
+        "Key": "x509Serial",
+        "Value": "15D19632234BF759A32802C0DA88F9E8AFC8702D"
+    }
+]