[FEATURE] Validate ACLs and add network groups (#1568)

* adds validation to ACL's
* adds a new networks section that can be used as aliases in other sections (currently access_control)

config.template.yml

@@ -244,6 +244,14 @@ access_control:
   # to the user.
   default_policy: deny
 
+  networks:
+    - name: internal
+      networks:
+        - 10.10.0.0/16
+        - 192.168.2.0/24
+    - name: VPN
+      networks: 10.9.0.0/16
+
   rules:
     # Rules applied to everyone
     - domain: public.example.com
@@ -253,7 +261,10 @@ access_control:
       policy: one_factor
       # Network based rule, if not provided any network matches.
       networks:
+        - internal
+        - VPN
         - 192.168.1.0/24
+        - 10.0.0.1
 
     - domain:
       - secure.example.com

docs/configuration/access-control.md

@@ -29,7 +29,7 @@ The criteria are:
 * domain: domain targeted by the request.
 * resources: list of patterns that the path should match (one is sufficient).
 * subject: the user or group of users to define the policy for.
-* networks: the network range from where should comes the request.
+* networks: the network addresses, ranges (CIDR notation) or groups from where the request originates.
 
 A rule is matched when all criteria of the rule match.
 
@@ -88,8 +88,8 @@ username is `john` OR the group is `admins`.
 
 ## Networks
 
-A list of network ranges can be specified in a rule in order to apply different policies when
-requests come from different networks.
+A list of network addresses, ranges (CIDR notation) or groups can be specified in a rule in order to apply different
+policies when requests originate from different networks.
 
 The main use case is when, lets say a resource should be exposed both on the Internet and from an
 authenticated VPN for instance. Passing a second factor a first time to get access to the VPN and
@@ -108,14 +108,24 @@ Here is a complete example of complex access control list that can be defined in
 ```yaml
 access_control:
   default_policy: deny
+  networks:
+    - name: internal
+      networks:
+        - 10.10.0.0/16
+        - 192.168.2.0/24
+    - name: VPN
+      networks: 10.9.0.0/16
   rules:
     - domain: public.example.com
       policy: bypass
 
     - domain: secure.example.com
       policy: one_factor
       networks:
-      - 192.168.1.0/24
+        - internal
+        - VPN
+        - 192.168.1.0/24
+        - 10.0.0.1
 
     - domain:
       - secure.example.com

internal/authorization/authorizer.go

@@ -43,19 +43,19 @@ type Object struct {
 }
 
 // selectMatchingSubjectRules take a set of rules and select only the rules matching the subject constraints.
-func selectMatchingSubjectRules(rules []schema.ACLRule, subject Subject) []schema.ACLRule {
+func selectMatchingSubjectRules(rules []schema.ACLRule, networks []schema.ACLNetwork, subject Subject) []schema.ACLRule {
 	selectedRules := []schema.ACLRule{}
 
 	for _, rule := range rules {
 		switch {
 		case len(rule.Subjects) > 0:
 			for _, subjectRule := range rule.Subjects {
-				if isSubjectMatching(subject, subjectRule) && isIPMatching(subject.IP, rule.Networks) {
+				if isSubjectMatching(subject, subjectRule) && isIPMatching(subject.IP, rule.Networks, networks) {
 					selectedRules = append(selectedRules, rule)
 				}
 			}
 		default:
-			if isIPMatching(subject.IP, rule.Networks) {
+			if isIPMatching(subject.IP, rule.Networks, networks) {
 				selectedRules = append(selectedRules, rule)
 			}
 		}
@@ -76,8 +76,8 @@ func selectMatchingObjectRules(rules []schema.ACLRule, object Object) []schema.A
 	return selectedRules
 }
 
-func selectMatchingRules(rules []schema.ACLRule, subject Subject, object Object) []schema.ACLRule {
-	matchingRules := selectMatchingSubjectRules(rules, subject)
+func selectMatchingRules(rules []schema.ACLRule, networks []schema.ACLNetwork, subject Subject, object Object) []schema.ACLRule {
+	matchingRules := selectMatchingSubjectRules(rules, networks, subject)
 	return selectMatchingObjectRules(matchingRules, object)
 }
 
@@ -117,7 +117,7 @@ func (p *Authorizer) GetRequiredLevel(subject Subject, requestURL url.URL) Level
 	logging.Logger().Tracef("Check authorization of subject %s and url %s.",
 		subject.String(), requestURL.String())
 
-	matchingRules := selectMatchingRules(p.configuration.Rules, subject, Object{
+	matchingRules := selectMatchingRules(p.configuration.Rules, p.configuration.Networks, subject, Object{
 		Domain: requestURL.Hostname(),
 		Path:   requestURL.Path,
 	})

internal/authorization/const.go

@@ -1,5 +1,7 @@
 package authorization
 
+import "github.com/authelia/authelia/internal/configuration/schema"
+
 // Level is the type representing an authorization level.
 type Level int
 
@@ -13,3 +15,14 @@ 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

@@ -3,33 +3,70 @@ package authorization
 import (
 	"net"
 	"strings"
+
+	"github.com/authelia/authelia/internal/configuration/schema"
+	"github.com/authelia/authelia/internal/logging"
 )
 
+func selectMatchingNetworkGroups(networks []string, aclNetworks []schema.ACLNetwork) []schema.ACLNetwork {
+	selectedNetworkGroups := []schema.ACLNetwork{}
+
+	for _, network := range networks {
+		for _, n := range aclNetworks {
+			for _, ng := range n.Name {
+				if network == ng {
+					selectedNetworkGroups = append(selectedNetworkGroups, n)
+				}
+			}
+		}
+	}
+
+	return selectedNetworkGroups
+}
+
+func isIPAddressOrCIDR(ip net.IP, network string) bool {
+	switch {
+	case ip.String() == network:
+		return true
+	case strings.Contains(network, "/"):
+		return parseCIDR(ip, network)
+	}
+
+	return false
+}
+
+func parseCIDR(ip net.IP, network string) bool {
+	_, ipNet, err := net.ParseCIDR(network)
+	if err != nil {
+		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.
-func isIPMatching(ip net.IP, networks []string) bool {
+func isIPMatching(ip net.IP, networks []string, aclNetworks []schema.ACLNetwork) bool {
 	// If no network is provided in the rule, we match any network
 	if len(networks) == 0 {
 		return true
 	}
 
+	matchingNetworkGroups := selectMatchingNetworkGroups(networks, aclNetworks)
+
 	for _, network := range networks {
-		if !strings.Contains(network, "/") {
-			if ip.String() == network {
-				return true
+		if net.ParseIP(network) == nil && !strings.Contains(network, "/") {
+			for _, n := range matchingNetworkGroups {
+				for _, network := range n.Networks {
+					if isIPAddressOrCIDR(ip, network) {
+						return true
+					}
+				}
 			}
-
-			continue
-		}
-
-		_, ipNet, err := net.ParseCIDR(network)
-
-		if err != nil {
-			// TODO(c.michaud): make sure the rule is valid at startup to
-			// to such a case here.
-			continue
-		}
-
-		if ipNet.Contains(ip) {
+		} else if isIPAddressOrCIDR(ip, network) {
 			return true
 		}
 	}

internal/authorization/ip_matcher_test.go

@@ -9,15 +9,28 @@ import (
 
 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{}))
+	assert.True(t, isIPMatching(net.ParseIP("127.0.0.1"), []string{}, testACLNetwork))
 
-	assert.True(t, isIPMatching(net.ParseIP("127.0.0.1"), []string{"127.0.0.1"}))
-	assert.False(t, isIPMatching(net.ParseIP("127.1"), []string{"127.0.0.1"}))
-	assert.False(t, isIPMatching(net.ParseIP("not-an-ip"), []string{"127.0.0.1"}))
+	assert.True(t, isIPMatching(net.ParseIP("127.0.0.1"), []string{"127.0.0.1"}, testACLNetwork))
+	assert.False(t, isIPMatching(net.ParseIP("127.1"), []string{"127.0.0.1"}, testACLNetwork))
+	assert.False(t, isIPMatching(net.ParseIP("not-an-ip"), []string{"127.0.0.1"}, testACLNetwork))
 
-	assert.False(t, isIPMatching(net.ParseIP("127.0.0.1"), []string{"10.0.0.1"}))
-	assert.False(t, isIPMatching(net.ParseIP("127.0.0.1"), []string{"10.0.0.0/8"}))
+	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.0/8"}, testACLNetwork))
 
-	assert.True(t, isIPMatching(net.ParseIP("10.230.5.1"), []string{"10.0.0.0/8"}))
-	assert.True(t, isIPMatching(net.ParseIP("10.230.5.1"), []string{"192.168.0.0/24", "10.0.0.0/8"}))
+	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{"192.168.0.0/24", "10.0.0.0/8"}, testACLNetwork))
+
+	// 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{"localhost"}, testACLNetwork))
+	assert.False(t, isIPMatching(net.ParseIP("127.1"), []string{"localhost"}, testACLNetwork))
+	assert.False(t, isIPMatching(net.ParseIP("not-an-ip"), []string{"localhost"}, testACLNetwork))
+
+	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"}, testACLNetwork))
+
+	assert.True(t, isIPMatching(net.ParseIP("10.230.5.1"), []string{"internal"}, testACLNetwork))
+	assert.True(t, isIPMatching(net.ParseIP("10.230.5.1"), []string{"192.168.0.0/24", "internal"}, testACLNetwork))
 }

internal/configuration/schema/access_control.go

@@ -1,10 +1,17 @@
 package schema
 
-import (
-	"fmt"
-	"net"
-	"strings"
-)
+// AccessControlConfiguration represents the configuration related to ACLs.
+type AccessControlConfiguration struct {
+	DefaultPolicy string       `mapstructure:"default_policy"`
+	Networks      []ACLNetwork `mapstructure:"networks"`
+	Rules         []ACLRule    `mapstructure:"rules"`
+}
+
+// ACLNetwork represents one ACL network group entry; "weak" coerces a single value into slice.
+type ACLNetwork struct {
+	Name     []string `mapstructure:"name,weak"`
+	Networks []string `mapstructure:"networks"`
+}
 
 // ACLRule represents one ACL rule entry; "weak" coerces a single value into slice.
 type ACLRule struct {
@@ -14,61 +21,3 @@ type ACLRule struct {
 	Networks  []string   `mapstructure:"networks"`
 	Resources []string   `mapstructure:"resources"`
 }
-
-// IsPolicyValid check if policy is valid.
-func IsPolicyValid(policy string) bool {
-	return policy == denyPolicy || policy == "one_factor" || policy == "two_factor" || policy == "bypass"
-}
-
-// IsSubjectValid check if a subject is valid.
-func IsSubjectValid(subject string) bool {
-	return subject == "" || strings.HasPrefix(subject, "user:") || strings.HasPrefix(subject, "group:")
-}
-
-// IsNetworkValid check if a network is valid.
-func IsNetworkValid(network string) bool {
-	_, _, err := net.ParseCIDR(network)
-	return err == nil
-}
-
-// Validate validate an ACL Rule.
-func (r *ACLRule) Validate(validator *StructValidator) {
-	if len(r.Domains) == 0 {
-		validator.Push(fmt.Errorf("Domain must be provided"))
-	}
-
-	if !IsPolicyValid(r.Policy) {
-		validator.Push(fmt.Errorf("A policy must either be 'deny', 'two_factor', 'one_factor' or 'bypass'"))
-	}
-
-	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 i, network := range r.Networks {
-		if !IsNetworkValid(network) {
-			validator.Push(fmt.Errorf("Network %d must be a valid CIDR", i))
-		}
-	}
-}
-
-// AccessControlConfiguration represents the configuration related to ACLs.
-type AccessControlConfiguration struct {
-	DefaultPolicy string    `mapstructure:"default_policy"`
-	Rules         []ACLRule `mapstructure:"rules"`
-}
-
-// Validate validate the access control configuration.
-func (acc *AccessControlConfiguration) Validate(validator *StructValidator) {
-	if acc.DefaultPolicy == "" {
-		acc.DefaultPolicy = denyPolicy
-	}
-
-	if !IsPolicyValid(acc.DefaultPolicy) {
-		validator.Push(fmt.Errorf("'default_policy' must either be 'deny', 'two_factor', 'one_factor' or 'bypass'"))
-	}
-}

internal/configuration/schema/const.go

@@ -4,8 +4,6 @@ import (
 	"time"
 )
 
-const denyPolicy = "deny"
-
 const argon2id = "argon2id"
 
 // ProfileRefreshDisabled represents a value for refresh_interval that disables the check entirely.

internal/configuration/test_resources/config.yml

@@ -74,7 +74,7 @@ access_control:
       resources:
         - "^/deny-all.*$"
       subject: ["group:dev", "user:john"]
-      policy: denied
+      policy: deny
 
     # Rules applied to user 'harry'
     - domain: dev.example.com

internal/configuration/test_resources/config_alt.yml

@@ -74,7 +74,7 @@ access_control:
       resources:
         - "^/deny-all.*$"
       subject: ["group:dev", "user:john"]
-      policy: denied
+      policy: deny
 
     # Rules applied to user 'harry'
     - domain: dev.example.com

internal/configuration/test_resources/config_bad_keys.yml

@@ -75,7 +75,7 @@ access_control:
       resources:
         - "^/deny-all.*$"
       subject: ["group:dev", "user:john"]
-      policy: denied
+      policy: deny
 
     # Rules applied to user 'harry'
     - domain: dev.example.com

internal/configuration/test_resources/config_with_secret.yml

@@ -75,7 +75,7 @@ access_control:
       resources:
         - "^/deny-all.*$"
       subject: ["group:dev", "user:john"]
-      policy: denied
+      policy: deny
 
     # Rules applied to user 'harry'
     - domain: dev.example.com