[MISC] Validate all sections of ACLs on startup (#1595)

* [MISC] Validate all sections of ACLs on startup

This change ensure that all sections of the `access_control` key are validated on startup.

* Change error format to clearly identify values

internal/authorization/const.go

@@ -1,7 +1,5 @@
 package authorization
 
-import "github.com/authelia/authelia/internal/configuration/schema"
-
 // Level is the type representing an authorization level.
 type Level int
 
@@ -15,14 +13,3 @@ const (
 	// Denied denied level.
 	Denied Level = iota
 )
-
-var testACLNetwork = []schema.ACLNetwork{
-	{
-		Name:     []string{"localhost"},
-		Networks: []string{"127.0.0.1"},
-	},
-	{
-		Name:     []string{"internal"},
-		Networks: []string{"10.0.0.0/8"},
-	},
-}

internal/authorization/ip_matcher.go

@@ -5,7 +5,6 @@ import (
 	"strings"
 
 	"github.com/authelia/authelia/internal/configuration/schema"
-	"github.com/authelia/authelia/internal/logging"
 )
 
 func selectMatchingNetworkGroups(networks []string, aclNetworks []schema.ACLNetwork) []schema.ACLNetwork {
@@ -36,16 +35,8 @@ func isIPAddressOrCIDR(ip net.IP, network string) bool {
 }
 
 func parseCIDR(ip net.IP, network string) bool {
-	_, ipNet, err := net.ParseCIDR(network)
+	_, ipNet, _ := net.ParseCIDR(network)
-	if err != nil {
+	return ipNet.Contains(ip)
-		logging.Logger().Errorf("Failed to parse network %s: %s", network, err)
-	}
-
-	if ipNet.Contains(ip) {
-		return true
-	}
-
-	return false
 }
 
 // isIPMatching check whether user's IP is in one of the network ranges.

internal/authorization/ip_matcher_test.go

@@ -5,32 +5,34 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+
+	"github.com/authelia/authelia/internal/configuration/schema"
 )
 
 func TestIPMatcher(t *testing.T) {
 	// Default policy is 'allow all ips' if no IP is defined
-	assert.True(t, isIPMatching(net.ParseIP("127.0.0.1"), []string{}, testACLNetwork))
+	assert.True(t, isIPMatching(net.ParseIP("127.0.0.1"), []string{}, schema.DefaultACLNetwork))
 
-	assert.True(t, isIPMatching(net.ParseIP("127.0.0.1"), []string{"127.0.0.1"}, testACLNetwork))
+	assert.True(t, isIPMatching(net.ParseIP("127.0.0.1"), []string{"127.0.0.1"}, schema.DefaultACLNetwork))
-	assert.False(t, isIPMatching(net.ParseIP("127.1"), []string{"127.0.0.1"}, testACLNetwork))
+	assert.False(t, isIPMatching(net.ParseIP("127.1"), []string{"127.0.0.1"}, schema.DefaultACLNetwork))
-	assert.False(t, isIPMatching(net.ParseIP("not-an-ip"), []string{"127.0.0.1"}, testACLNetwork))
+	assert.False(t, isIPMatching(net.ParseIP("not-an-ip"), []string{"127.0.0.1"}, schema.DefaultACLNetwork))
 
-	assert.False(t, isIPMatching(net.ParseIP("127.0.0.1"), []string{"10.0.0.1"}, testACLNetwork))
+	assert.False(t, isIPMatching(net.ParseIP("127.0.0.1"), []string{"10.0.0.1"}, schema.DefaultACLNetwork))
-	assert.False(t, isIPMatching(net.ParseIP("127.0.0.1"), []string{"10.0.0.0/8"}, testACLNetwork))
+	assert.False(t, isIPMatching(net.ParseIP("127.0.0.1"), []string{"10.0.0.0/8"}, schema.DefaultACLNetwork))
 
-	assert.True(t, isIPMatching(net.ParseIP("10.230.5.1"), []string{"10.0.0.0/8"}, testACLNetwork))
+	assert.True(t, isIPMatching(net.ParseIP("10.230.5.1"), []string{"10.0.0.0/8"}, schema.DefaultACLNetwork))
-	assert.True(t, isIPMatching(net.ParseIP("10.230.5.1"), []string{"192.168.0.0/24", "10.0.0.0/8"}, testACLNetwork))
+	assert.True(t, isIPMatching(net.ParseIP("10.230.5.1"), []string{"192.168.0.0/24", "10.0.0.0/8"}, schema.DefaultACLNetwork))
 
 	// Test network groups
-	assert.True(t, isIPMatching(net.ParseIP("127.0.0.1"), []string{}, testACLNetwork))
+	assert.True(t, isIPMatching(net.ParseIP("127.0.0.1"), []string{}, schema.DefaultACLNetwork))
 
-	assert.True(t, isIPMatching(net.ParseIP("127.0.0.1"), []string{"localhost"}, testACLNetwork))
+	assert.True(t, isIPMatching(net.ParseIP("127.0.0.1"), []string{"localhost"}, schema.DefaultACLNetwork))
-	assert.False(t, isIPMatching(net.ParseIP("127.1"), []string{"localhost"}, testACLNetwork))
+	assert.False(t, isIPMatching(net.ParseIP("127.1"), []string{"localhost"}, schema.DefaultACLNetwork))
-	assert.False(t, isIPMatching(net.ParseIP("not-an-ip"), []string{"localhost"}, testACLNetwork))
+	assert.False(t, isIPMatching(net.ParseIP("not-an-ip"), []string{"localhost"}, schema.DefaultACLNetwork))
 
-	assert.False(t, isIPMatching(net.ParseIP("127.0.0.1"), []string{"internal"}, testACLNetwork))
+	assert.False(t, isIPMatching(net.ParseIP("127.0.0.1"), []string{"internal"}, schema.DefaultACLNetwork))
-	assert.False(t, isIPMatching(net.ParseIP("127.0.0.1"), []string{"internal"}, testACLNetwork))
+	assert.False(t, isIPMatching(net.ParseIP("127.0.0.1"), []string{"internal"}, schema.DefaultACLNetwork))
 
-	assert.True(t, isIPMatching(net.ParseIP("10.230.5.1"), []string{"internal"}, testACLNetwork))
+	assert.True(t, isIPMatching(net.ParseIP("10.230.5.1"), []string{"internal"}, schema.DefaultACLNetwork))
-	assert.True(t, isIPMatching(net.ParseIP("10.230.5.1"), []string{"192.168.0.0/24", "internal"}, testACLNetwork))
+	assert.True(t, isIPMatching(net.ParseIP("10.230.5.1"), []string{"192.168.0.0/24", "internal"}, schema.DefaultACLNetwork))
 }

internal/authorization/path_matcher.go

@@ -9,13 +9,7 @@ func isPathMatching(path string, pathRegexps []string) bool {
 	}
 
 	for _, pathRegexp := range pathRegexps {
-		match, err := regexp.MatchString(pathRegexp, path)
+		match, _ := regexp.MatchString(pathRegexp, path)
-		if err != nil {
-			// TODO(c.michaud): make sure this is safe in advance to
-			// avoid checking this case here.
-			continue
-		}
-
 		if match {
 			return true
 		}

internal/authorization/subject_matcher.go

@@ -8,11 +8,6 @@ import (
 
 func isSubjectMatching(subject Subject, subjectRule []string) bool {
 	for _, ruleSubject := range subjectRule {
-		// If no subject is provided in the rule, we match any user.
-		if ruleSubject == "" {
-			continue
-		}
-
 		if strings.HasPrefix(ruleSubject, userPrefix) {
 			user := strings.Trim(ruleSubject[len(userPrefix):], " ")
 			if user == subject.Username {

internal/configuration/schema/access_control.go

@@ -21,3 +21,31 @@ type ACLRule struct {
 	Networks  []string   `mapstructure:"networks"`
 	Resources []string   `mapstructure:"resources"`
 }
+
+// DefaultACLNetwork represents the default configuration related to access control network group configuration.
+var DefaultACLNetwork = []ACLNetwork{
+	{
+		Name:     []string{"localhost"},
+		Networks: []string{"127.0.0.1"},
+	},
+	{
+		Name:     []string{"internal"},
+		Networks: []string{"10.0.0.0/8"},
+	},
+}
+
+// DefaultACLRule represents the default configuration related to access control rule configuration.
+var DefaultACLRule = []ACLRule{
+	{
+		Domains: []string{"public.example.com"},
+		Policy:  "bypass",
+	},
+	{
+		Domains: []string{"singlefactor.example.com"},
+		Policy:  "one_factor",
+	},
+	{
+		Domains: []string{"secure.example.com"},
+		Policy:  "two_factor",
+	},
+}

internal/configuration/validator/access_control.go

@@ -3,6 +3,7 @@ package validator
 import (
 	"fmt"
 	"net"
+	"regexp"
 	"strings"
 
 	"github.com/authelia/authelia/internal/configuration/schema"
@@ -14,6 +15,12 @@ func IsPolicyValid(policy string) bool {
 	return policy == denyPolicy || policy == "one_factor" || policy == "two_factor" || policy == "bypass"
 }
 
+// IsResourceValid check if a resource is valid.
+func IsResourceValid(resource string) error {
+	_, err := regexp.Compile(resource)
+	return err
+}
+
 // IsSubjectValid check if a subject is valid.
 func IsSubjectValid(subject string) bool {
 	return subject == "" || strings.HasPrefix(subject, "user:") || strings.HasPrefix(subject, "group:")
@@ -52,7 +59,7 @@ func ValidateAccessControl(configuration schema.AccessControlConfiguration, vali
 		for _, n := range configuration.Networks {
 			for _, networks := range n.Networks {
 				if !IsNetworkValid(networks) {
-					validator.Push(fmt.Errorf("Network %s from group %s must be a valid IP or CIDR", networks, n.Name))
+					validator.Push(fmt.Errorf("Network %s from network group: %s must be a valid IP or CIDR", n.Networks, n.Name))
 				}
 			}
 		}
@@ -67,21 +74,27 @@ func ValidateRules(configuration schema.AccessControlConfiguration, validator *s
 		}
 
 		if !IsPolicyValid(r.Policy) {
-			validator.Push(fmt.Errorf("Policy for domain: %s is invalid, a policy must either be 'deny', 'two_factor', 'one_factor' or 'bypass'", r.Domains))
+			validator.Push(fmt.Errorf("Policy [%s] for domain: %s is invalid, a policy must either be 'deny', 'two_factor', 'one_factor' or 'bypass'", r.Policy, r.Domains))
-		}
-
-		for i, subjectRule := range r.Subjects {
-			for j, subject := range subjectRule {
-				if !IsSubjectValid(subject) {
-					validator.Push(fmt.Errorf("Subject %d-%d must start with 'user:' or 'group:'", i, j))
-				}
-			}
 		}
 
 		for _, network := range r.Networks {
 			if !IsNetworkValid(network) {
 				if !IsNetworkGroupValid(configuration, network) {
-					validator.Push(fmt.Errorf("Network %s is not a valid network or network group", network))
+					validator.Push(fmt.Errorf("Network %s for domain: %s is not a valid network or network group", r.Networks, r.Domains))
+				}
+			}
+		}
+
+		for _, resource := range r.Resources {
+			if err := IsResourceValid(resource); err != nil {
+				validator.Push(fmt.Errorf("Resource %s for domain: %s is invalid, %s", r.Resources, r.Domains, err))
+			}
+		}
+
+		for _, subjectRule := range r.Subjects {
+			for _, subject := range subjectRule {
+				if !IsSubjectValid(subject) {
+					validator.Push(fmt.Errorf("Subject %s for domain: %s must start with 'user:' or 'group:'", subjectRule, r.Domains))
 				}
 			}
 		}

internal/configuration/validator/access_control_test.go

@@ -0,0 +1,139 @@
+package validator
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/suite"
+
+	"github.com/authelia/authelia/internal/configuration/schema"
+)
+
+type AccessControl struct {
+	suite.Suite
+	configuration schema.AccessControlConfiguration
+	validator     *schema.StructValidator
+}
+
+func (suite *AccessControl) SetupTest() {
+	suite.validator = schema.NewStructValidator()
+	suite.configuration.DefaultPolicy = denyPolicy
+	suite.configuration.Networks = schema.DefaultACLNetwork
+	suite.configuration.Rules = schema.DefaultACLRule
+}
+
+func (suite *AccessControl) TestShouldValidateCompleteConfiguration() {
+	ValidateAccessControl(suite.configuration, suite.validator)
+
+	suite.Assert().False(suite.validator.HasWarnings())
+	suite.Assert().False(suite.validator.HasErrors())
+}
+
+func (suite *AccessControl) TestShouldRaiseErrorInvalidDefaultPolicy() {
+	suite.configuration.DefaultPolicy = "invalid"
+
+	ValidateAccessControl(suite.configuration, suite.validator)
+
+	suite.Assert().False(suite.validator.HasWarnings())
+	suite.Require().Len(suite.validator.Errors(), 1)
+
+	suite.Assert().EqualError(suite.validator.Errors()[0], "'default_policy' must either be 'deny', 'two_factor', 'one_factor' or 'bypass'")
+}
+
+func (suite *AccessControl) TestShouldRaiseErrorInvalidNetworkGroupNetwork() {
+	suite.configuration.Networks = []schema.ACLNetwork{
+		{
+			Name:     []string{"internal"},
+			Networks: []string{"abc.def.ghi.jkl"},
+		},
+	}
+
+	ValidateAccessControl(suite.configuration, suite.validator)
+
+	suite.Assert().False(suite.validator.HasWarnings())
+	suite.Require().Len(suite.validator.Errors(), 1)
+
+	suite.Assert().EqualError(suite.validator.Errors()[0], "Network [abc.def.ghi.jkl] from network group: [internal] must be a valid IP or CIDR")
+}
+
+func (suite *AccessControl) TestShouldRaiseErrorNoRulesDefined() {
+	suite.configuration.Rules = []schema.ACLRule{{}}
+
+	ValidateRules(suite.configuration, suite.validator)
+
+	suite.Assert().False(suite.validator.HasWarnings())
+	suite.Require().Len(suite.validator.Errors(), 2)
+
+	suite.Assert().EqualError(suite.validator.Errors()[0], "No access control rules have been defined")
+	suite.Assert().EqualError(suite.validator.Errors()[1], "Policy [] for domain: [] is invalid, a policy must either be 'deny', 'two_factor', 'one_factor' or 'bypass'")
+}
+
+func (suite *AccessControl) TestShouldRaiseErrorInvalidPolicy() {
+	suite.configuration.Rules = []schema.ACLRule{
+		{
+			Domains: []string{"public.example.com"},
+			Policy:  "invalid",
+		},
+	}
+
+	ValidateRules(suite.configuration, suite.validator)
+
+	suite.Assert().False(suite.validator.HasWarnings())
+	suite.Require().Len(suite.validator.Errors(), 1)
+
+	suite.Assert().EqualError(suite.validator.Errors()[0], "Policy [invalid] for domain: [public.example.com] is invalid, a policy must either be 'deny', 'two_factor', 'one_factor' or 'bypass'")
+}
+
+func (suite *AccessControl) TestShouldRaiseErrorInvalidNetwork() {
+	suite.configuration.Rules = []schema.ACLRule{
+		{
+			Domains:  []string{"public.example.com"},
+			Policy:   "bypass",
+			Networks: []string{"abc.def.ghi.jkl/32"},
+		},
+	}
+
+	ValidateRules(suite.configuration, suite.validator)
+
+	suite.Assert().False(suite.validator.HasWarnings())
+	suite.Require().Len(suite.validator.Errors(), 1)
+
+	suite.Assert().EqualError(suite.validator.Errors()[0], "Network [abc.def.ghi.jkl/32] for domain: [public.example.com] is not a valid network or network group")
+}
+
+func (suite *AccessControl) TestShouldRaiseErrorInvalidResource() {
+	suite.configuration.Rules = []schema.ACLRule{
+		{
+			Domains:   []string{"public.example.com"},
+			Policy:    "bypass",
+			Resources: []string{"^/(api.*"},
+		},
+	}
+
+	ValidateRules(suite.configuration, suite.validator)
+
+	suite.Assert().False(suite.validator.HasWarnings())
+	suite.Require().Len(suite.validator.Errors(), 1)
+
+	suite.Assert().EqualError(suite.validator.Errors()[0], "Resource [^/(api.*] for domain: [public.example.com] is invalid, error parsing regexp: missing closing ): `^/(api.*`")
+}
+
+func (suite *AccessControl) TestShouldRaiseErrorInvalidSubject() {
+	suite.configuration.Rules = []schema.ACLRule{
+		{
+			Domains:  []string{"public.example.com"},
+			Policy:   "bypass",
+			Subjects: [][]string{{"invalid"}},
+		},
+	}
+
+	ValidateRules(suite.configuration, suite.validator)
+
+	suite.Assert().False(suite.validator.HasWarnings())
+	suite.Require().Len(suite.validator.Errors(), 1)
+
+	suite.Assert().EqualError(suite.validator.Errors()[0], "Subject [invalid] for domain: [public.example.com] must start with 'user:' or 'group:'")
+}
+
+func TestAccessControl(t *testing.T) {
+	suite.Run(t, new(AccessControl))
+}

internal/configuration/validator/configuration.go

@@ -61,7 +61,7 @@ func ValidateConfiguration(configuration *schema.Configuration, validator *schem
 	ValidateAuthenticationBackend(&configuration.AuthenticationBackend, validator)
 
 	if configuration.AccessControl.DefaultPolicy == "" {
-		configuration.AccessControl.DefaultPolicy = "deny"
+		configuration.AccessControl.DefaultPolicy = denyPolicy
 	}
 
 	ValidateAccessControl(configuration.AccessControl, validator)

internal/suites/DuoPush/configuration.yml

@@ -47,7 +47,7 @@ duo_api:
 # resources.
 access_control:
   # Default policy can either be `bypass`, `one_factor`, `two_factor` or `deny`.
-  default_policy: deny
+  default_policy: two_factor
 
   rules:
     - domain: singlefactor.example.com