Add more validations to trust-policy parsing

go.mod

@@ -3,6 +3,13 @@ module github.com/notaryproject/notation-go
 go 1.17
 
 require (
+	github.com/go-ldap/ldap/v3 v3.4.3
 	github.com/golang-jwt/jwt/v4 v4.4.1
 	github.com/opencontainers/go-digest v1.0.0
 )
+
+require (
+	github.com/Azure/go-ntlmssp v0.0.0-20211209120228-48547f28849e // indirect
+	github.com/go-asn1-ber/asn1-ber v1.5.4 // indirect
+	golang.org/x/crypto v0.0.0-20220331220935-ae2d96664a29 // indirect
+)

go.sum

@@ -1,4 +1,19 @@
+github.com/Azure/go-ntlmssp v0.0.0-20211209120228-48547f28849e h1:ZU22z/2YRFLyf/P4ZwUYSdNCWsMEI0VeyrFoI2rAhJQ=
+github.com/Azure/go-ntlmssp v0.0.0-20211209120228-48547f28849e/go.mod h1:chxPXzSsl7ZWRAuOIE23GDNzjWuZquvFlgA8xmpunjU=
+github.com/go-asn1-ber/asn1-ber v1.5.4 h1:vXT6d/FNDiELJnLb6hGNa309LMsrCoYFvpwHDF0+Y1A=
+github.com/go-asn1-ber/asn1-ber v1.5.4/go.mod h1:hEBeB/ic+5LoWskz+yKT7vGhhPYkProFKoKdwZRWMe0=
+github.com/go-ldap/ldap/v3 v3.4.3 h1:JCKUtJPIcyOuG7ctGabLKMgIlKnGumD/iGjuWeEruDI=
+github.com/go-ldap/ldap/v3 v3.4.3/go.mod h1:7LdHfVt6iIOESVEe3Bs4Jp2sHEKgDeduAhgM1/f9qmo=
 github.com/golang-jwt/jwt/v4 v4.4.1 h1:pC5DB52sCeK48Wlb9oPcdhnjkz1TKt1D/P7WKJ0kUcQ=
 github.com/golang-jwt/jwt/v4 v4.4.1/go.mod h1:m21LjoU+eqJr34lmDMbreY2eSTRJ1cv77w39/MY0Ch0=
 github.com/opencontainers/go-digest v1.0.0 h1:apOUWs51W5PlhuyGyz9FCeeBIOUDA/6nW8Oi/yOhh5U=
 github.com/opencontainers/go-digest v1.0.0/go.mod h1:0JzlMkj0TRzQZfJkVvzbP0HBR3IKzErnv2BNG4W4MAM=
+golang.org/x/crypto v0.0.0-20220331220935-ae2d96664a29 h1:tkVvjkPTB7pnW3jnid7kNyAMPVWllTNOf/qKDze4p9o=
+golang.org/x/crypto v0.0.0-20220331220935-ae2d96664a29/go.mod h1:IxCIyHEi3zRg3s0A5j5BB6A9Jmi73HwBIUl50j+osU4=
+golang.org/x/net v0.0.0-20211112202133-69e39bad7dc2/go.mod h1:9nx3DQGgdP8bBQD5qxJ1jj9UTztislL4KSBs9R2vV5Y=
+golang.org/x/sys v0.0.0-20201119102817-f84b799fce68/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
+golang.org/x/sys v0.0.0-20210423082822-04245dca01da/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
+golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
+golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo=
+golang.org/x/text v0.3.6/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ=
+golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=

verification/helpers.go

@@ -0,0 +1,97 @@
+package verification
+
+import (
+	"fmt"
+	"regexp"
+	"strings"
+
+	ldapv3 "github.com/go-ldap/ldap/v3"
+)
+
+// isPresent is a utility function to check if a string exists in an array
+func isPresent(val string, values []string) bool {
+	for _, v := range values {
+		if v == val {
+			return true
+		}
+	}
+	return false
+}
+
+// validateRegistryScopeFormat validates if a scope is following the format defined in distribution spec
+func validateRegistryScopeFormat(scope string) error {
+	// Domain and Repository regexes are adapted from distribution implementation
+	// https://github.com/distribution/distribution/blob/main/reference/regexp.go#L31
+	domainRegexp := regexp.MustCompile(`^(?:[a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9])(?:(?:\.(?:[a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9-]*[a-zA-Z0-9]))+)?(?::[0-9]+)?$`)
+	repositoryRegexp := regexp.MustCompile(`^[a-z0-9]+(?:(?:(?:[._]|__|[-]*)[a-z0-9]+)+)?(?:(?:/[a-z0-9]+(?:(?:(?:[._]|__|[-]*)[a-z0-9]+)+)?)+)?$`)
+	errorMessage := "registry scope %q is not valid, make sure it is the fully qualified registry URL without the scheme/protocol. e.g domain.com/my/repository"
+	firstSlash := strings.Index(scope, "/")
+	if firstSlash < 0 {
+		return fmt.Errorf(errorMessage, scope)
+	}
+	domain := scope[:firstSlash]
+	repository := scope[firstSlash+1:]
+
+	if domain == "" || repository == "" || !domainRegexp.MatchString(domain) || !repositoryRegexp.MatchString(repository) {
+		return fmt.Errorf(errorMessage, scope)
+	}
+
+	// No errors
+	return nil
+}
+
+// validateDistinguishedName validates if a DN name is parsable and follows Notary V2 rules
+func validateDistinguishedName(name string) (map[string]string, error) {
+	mandatoryFields := []string{"C", "ST", "O"}
+	attrKeyValue := make(map[string]string)
+	dn, err := ldapv3.ParseDN(name)
+
+	if err != nil {
+		return nil, fmt.Errorf("distinguished name (DN) %q is not valid, it must contain 'C', 'ST', and 'O' RDN attributes at a minimum, and follow RFC 4514 standard", name)
+	}
+
+	for _, rdn := range dn.RDNs {
+
+		// multi-valued RDNs are not supported (TODO: add spec reference here)
+		if len(rdn.Attributes) > 1 {
+			return nil, fmt.Errorf("distinguished name (DN) %q has multi-valued RDN attributes, remove multi-valued RDN attributes as they are not supported", name)
+		}
+		for _, attribute := range rdn.Attributes {
+			if attrKeyValue[attribute.Type] == "" {
+				attrKeyValue[attribute.Type] = attribute.Value
+			} else {
+				return nil, fmt.Errorf("distinguished name (DN) %q has duplicate RDN attribute for %q, DN can only have unique RDN attributes", name, attribute.Type)
+			}
+		}
+	}
+
+	// Verify mandatory fields are present
+	for _, field := range mandatoryFields {
+		if attrKeyValue[field] == "" {
+			return nil, fmt.Errorf("distinguished name (DN) %q has no mandatory RDN attribute for %q, it must contain 'C', 'ST', and 'O' RDN attributes at a minimum", name, field)
+		}
+	}
+	// No errors
+	return attrKeyValue, nil
+}
+
+func validateOverlappingDNs(policyName string, parsedDNs []parsedDN) error {
+	for i, dn1 := range parsedDNs {
+		for j, dn2 := range parsedDNs {
+			if i != j && isOverlappingDN(dn1.ParsedMap, dn2.ParsedMap) {
+				return fmt.Errorf("trust policy statement %q has overlapping x509 trustedIdentities, %q overlaps with %q", policyName, dn1.RawString, dn2.RawString)
+			}
+		}
+	}
+
+	return nil
+}
+
+func isOverlappingDN(dn1 map[string]string, dn2 map[string]string) bool {
+	for key := range dn1 {
+		if dn1[key] != dn2[key] {
+			return false
+		}
+	}
+	return true
+}

verification/policy.go

@@ -11,6 +11,11 @@ import (
 	"strings"
 )
 
+const (
+	wildcard    = "*"
+	x509Subject = "x509.subject"
+)
+
 // PolicyDocument represents a trustPolicy.json document
 type PolicyDocument struct {
 	// Version of the policy document
@@ -33,23 +38,107 @@ type TrustPolicy struct {
 	TrustedIdentities []string `json:"trustedIdentities,omitempty"`
 }
 
-func isPresent(val string, values []string) bool {
-	for _, v := range values {
-		if v == val {
-			return true
+// Internal type to hold raw and parsed Distinguished Names
+type parsedDN struct {
+	RawString string
+	ParsedMap map[string]string
+}
+
+// validateRegistryScopes validates if the policy document is following the Notary V2 spec rules for registry scopes
+func validateRegistryScopes(policyDoc *PolicyDocument) error {
+	registryScopeCount := make(map[string]int)
+
+	for _, statement := range policyDoc.TrustPolicies {
+		// Verify registry scopes are valid
+		if len(statement.RegistryScopes) == 0 {
+			return fmt.Errorf("trust policy statement %q has zero registry scopes, it must specify registry scopes with at least one value", statement.Name)
+		}
+		if len(statement.RegistryScopes) > 1 && isPresent(wildcard, statement.RegistryScopes) {
+			return fmt.Errorf("trust policy statement %q uses wildcard registry scope '*', a wildcard scope cannot be used in conjunction with other scope values", statement.Name)
+		}
+		for _, scope := range statement.RegistryScopes {
+			if scope != wildcard {
+				if err := validateRegistryScopeFormat(scope); err != nil {
+					return err
+				}
+			}
+			registryScopeCount[scope]++
+		}
+	}
+
+	// Verify one policy statement per registry scope
+	for key := range registryScopeCount {
+		if registryScopeCount[key] > 1 {
+			return fmt.Errorf("registry scope %q is present in multiple trust policy statements, one registry scope value can only be associated with one statement", key)
+		}
+	}
+
+	// No error
+	return nil
+}
+
+// validateRegistryScopes validates if the policy statement is following the Notary V2 spec rules for trusted identities
+func validateTrustedIdentities(statement TrustPolicy) error {
+
+	// If there is a wildcard in trusted identies, there shouldn't be any other identities
+	if len(statement.TrustedIdentities) > 1 && isPresent(wildcard, statement.TrustedIdentities) {
+		return fmt.Errorf("trust policy statement %q uses a wildcard trusted identity '*', a wildcard identity cannot be used in conjunction with other values", statement.Name)
+	}
+
+	var parsedDNs []parsedDN
+	// If there are trusted identities, verify they are valid
+	for _, identity := range statement.TrustedIdentities {
+		if identity == "" {
+			return fmt.Errorf("trust policy statement %q has an empty trusted identity", statement.Name)
+		}
+
+		if identity != wildcard {
+			i := strings.Index(identity, ":")
+			if i < 0 {
+				return fmt.Errorf("trust policy statement %q has trusted identity %q without an identity prefix", statement.Name, identity)
+			}
+
+			identityPrefix := identity[:i]
+			identityValue := identity[i+1:]
+
+			// notation natively supports x509.subject identities only
+			if identityPrefix == x509Subject {
+				validatedDN, err := validateDistinguishedName(identityValue)
+				if err != nil {
+					return err
+				}
+				parsedDNs = append(parsedDNs, parsedDN{RawString: identity, ParsedMap: validatedDN})
+			}
 		}
 	}
-	return false
+
+	// Verify there are no overlapping DNs
+	if err := validateOverlappingDNs(statement.Name, parsedDNs); err != nil {
+		return err
+	}
+
+	// No error
+	return nil
+}
+
+// validateTrustStore validates if the policy statement is following the Notary V2 spec rules for truststores
+func validateTrustStore(statement TrustPolicy) error {
+	supportedTrustStorePrefixes := []string{"ca"}
+
+	i := strings.Index(statement.TrustStore, ":")
+	if i < 0 || !isPresent(statement.TrustStore[:i], supportedTrustStorePrefixes) {
+		return fmt.Errorf("trust policy statement %q uses an unsupported trust store type %q in trust store value %q", statement.Name, statement.TrustStore[:i], statement.TrustStore)
+	}
+
+	return nil
 }
 
 // ValidatePolicyDocument validates a policy document according to it's version's rule set.
 // if any rule is violated, returns an error
 func ValidatePolicyDocument(policyDoc *PolicyDocument) error {
 	// Constants
-	wildcard := "*"
 	supportedPolicyVersions := []string{"1.0"}
 	supportedVerificationPresets := []string{"strict", "permissive", "audit", "skip"}
-	supportedTrustStorePrefixes := []string{"ca"}
 
 	// Validate Version
 	if !isPresent(policyDoc.Version, supportedPolicyVersions) {
@@ -60,8 +149,9 @@ func ValidatePolicyDocument(policyDoc *PolicyDocument) error {
 	if len(policyDoc.TrustPolicies) == 0 {
 		return errors.New("trust policy document can not have zero trust policy statements")
 	}
+
 	policyStatementNameCount := make(map[string]int)
-	registryScopeCount := make(map[string]int)
+
 	for _, statement := range policyDoc.TrustPolicies {
 
 		// Verify statement name is valid
@@ -70,45 +160,37 @@ func ValidatePolicyDocument(policyDoc *PolicyDocument) error {
 		}
 		policyStatementNameCount[statement.Name]++
 
-		// Verify registry scopes are valid
-		if len(statement.RegistryScopes) == 0 {
-			return fmt.Errorf("trust policy statement %q has zero registry scopes, it must specify registry scopes with at least one value", statement.Name)
-		}
-		if len(statement.RegistryScopes) > 1 && isPresent(wildcard, statement.RegistryScopes) {
-			return fmt.Errorf("trust policy statement %q uses wildcard registry scope '*', a wildcard scope cannot be used in conjunction with other scope values", statement.Name)
-		}
-		for _, scope := range statement.RegistryScopes {
-			registryScopeCount[scope]++
-		}
-
 		// Verify signature verification preset is valid
 		if !isPresent(statement.SignatureVerification, supportedVerificationPresets) {
 			return fmt.Errorf("trust policy statement %q uses unsupported signatureVerification value %q", statement.Name, statement.SignatureVerification)
 		}
 
-		// Any signature verification other than "skip" needs a trust store
-		if statement.SignatureVerification != "skip" && (statement.TrustStore == "" || len(statement.TrustedIdentities) == 0) {
-			return fmt.Errorf("trust policy statement %q is either missing a trust store or trusted identities, both must be specified", statement.Name)
-		}
+		// Any signature verification other than "skip" needs a trust store and trusted identities
+		if statement.SignatureVerification == "skip" {
+			if statement.TrustStore != "" || len(statement.TrustedIdentities) > 0 {
+				return fmt.Errorf("trust policy statement %q is set to skip signature verification but configured with a trust store or trusted identities, remove them if signature verification needs to be skipped", statement.Name)
+			}
+		} else {
+			if statement.TrustStore == "" || len(statement.TrustedIdentities) == 0 {
+				return fmt.Errorf("trust policy statement %q is either missing a trust store or trusted identities, both must be specified", statement.Name)
+			}
 
-		// Verify trust store type is valid if it is present (trust store is optional for "skip" signature verification)
-		if statement.TrustStore != "" {
-			i := strings.Index(statement.TrustStore, ":")
-			if i < 0 || !isPresent(statement.TrustStore[:i], supportedTrustStorePrefixes) {
-				return fmt.Errorf("trust policy statement %q uses an unsupported trust store type %q in trust store value %q", statement.Name, statement.TrustStore[:i], statement.TrustStore)
+			// Verify Trust Store is valid
+			if err := validateTrustStore(statement); err != nil {
+				return err
 			}
-		}
 
-		// If there are trusted identities, verify they are not empty
-		for _, identity := range statement.TrustedIdentities {
-			if identity == "" {
-				return fmt.Errorf("trust policy statement %q has an empty trusted identity", statement.Name)
+			// Verify Trusted Identities are valid
+			if err := validateTrustedIdentities(statement); err != nil {
+				return err
 			}
 		}
-		// If there is a wildcard in trusted identies, there shouldn't be any other identities
-		if len(statement.TrustedIdentities) > 1 && isPresent(wildcard, statement.TrustedIdentities) {
-			return fmt.Errorf("trust policy statement %q uses a wildcard trusted identity '*', a wildcard identity cannot be used in conjunction with other values", statement.Name)
-		}
+
+	}
+
+	// Verify registry scopes are valid
+	if err := validateRegistryScopes(policyDoc); err != nil {
+		return err
 	}
 
 	// Verify unique policy statement names across the policy document
@@ -118,13 +200,6 @@ func ValidatePolicyDocument(policyDoc *PolicyDocument) error {
 		}
 	}
 
-	// Verify one policy statement per registry scope
-	for key := range registryScopeCount {
-		if registryScopeCount[key] > 1 {
-			return fmt.Errorf("registry scope %q is present in multiple trust policy statements, one registry scope value can only be associated with one statement", key)
-		}
-	}
-
 	// No errors
 	return nil
 }