From 9ca0e940da2e0eaa6f90290b344eeccc24c7806f Mon Sep 17 00:00:00 2001 From: Amir Zarrinkafsh Date: Mon, 4 Jan 2021 21:55:23 +1100 Subject: [PATCH] [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 | 11 +++ docs/configuration/access-control.md | 18 +++- internal/authorization/authorizer.go | 12 +-- internal/authorization/const.go | 13 +++ internal/authorization/ip_matcher.go | 71 +++++++++++---- internal/authorization/ip_matcher_test.go | 29 ++++-- .../configuration/schema/access_control.go | 75 +++------------- internal/configuration/schema/const.go | 2 - .../configuration/test_resources/config.yml | 2 +- .../test_resources/config_alt.yml | 2 +- .../test_resources/config_bad_keys.yml | 2 +- .../test_resources/config_with_secret.yml | 2 +- .../configuration/validator/access_control.go | 89 +++++++++++++++++++ .../configuration/validator/configuration.go | 6 ++ internal/configuration/validator/const.go | 3 + internal/suites/NetworkACL/configuration.yml | 8 +- 16 files changed, 239 insertions(+), 106 deletions(-) create mode 100644 internal/configuration/validator/access_control.go diff --git a/config.template.yml b/config.template.yml index 2de0e344..125653bd 100644 --- a/config.template.yml +++ b/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 diff --git a/docs/configuration/access-control.md b/docs/configuration/access-control.md index 8808357f..34db849a 100644 --- a/docs/configuration/access-control.md +++ b/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,6 +108,13 @@ 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 @@ -115,7 +122,10 @@ access_control: - 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 diff --git a/internal/authorization/authorizer.go b/internal/authorization/authorizer.go index cc81683a..4ae7ee87 100644 --- a/internal/authorization/authorizer.go +++ b/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, }) diff --git a/internal/authorization/const.go b/internal/authorization/const.go index 99093bd8..597b9c94 100644 --- a/internal/authorization/const.go +++ b/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"}, + }, +} diff --git a/internal/authorization/ip_matcher.go b/internal/authorization/ip_matcher.go index ec882241..e6da5e71 100644 --- a/internal/authorization/ip_matcher.go +++ b/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 } } diff --git a/internal/authorization/ip_matcher_test.go b/internal/authorization/ip_matcher_test.go index 747bdc66..a2649734 100644 --- a/internal/authorization/ip_matcher_test.go +++ b/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)) } diff --git a/internal/configuration/schema/access_control.go b/internal/configuration/schema/access_control.go index 09c91cc2..68010ec3 100644 --- a/internal/configuration/schema/access_control.go +++ b/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'")) - } -} diff --git a/internal/configuration/schema/const.go b/internal/configuration/schema/const.go index 013390c0..ea9b8822 100644 --- a/internal/configuration/schema/const.go +++ b/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. diff --git a/internal/configuration/test_resources/config.yml b/internal/configuration/test_resources/config.yml index bec86152..f34bf895 100644 --- a/internal/configuration/test_resources/config.yml +++ b/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 diff --git a/internal/configuration/test_resources/config_alt.yml b/internal/configuration/test_resources/config_alt.yml index 84cda028..b0bb95c1 100644 --- a/internal/configuration/test_resources/config_alt.yml +++ b/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 diff --git a/internal/configuration/test_resources/config_bad_keys.yml b/internal/configuration/test_resources/config_bad_keys.yml index ea9a303e..7c914fa6 100644 --- a/internal/configuration/test_resources/config_bad_keys.yml +++ b/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 diff --git a/internal/configuration/test_resources/config_with_secret.yml b/internal/configuration/test_resources/config_with_secret.yml index 26f99959..ddeee2d1 100644 --- a/internal/configuration/test_resources/config_with_secret.yml +++ b/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 diff --git a/internal/configuration/validator/access_control.go b/internal/configuration/validator/access_control.go new file mode 100644 index 00000000..e1b52866 --- /dev/null +++ b/internal/configuration/validator/access_control.go @@ -0,0 +1,89 @@ +package validator + +import ( + "fmt" + "net" + "strings" + + "github.com/authelia/authelia/internal/configuration/schema" + "github.com/authelia/authelia/internal/utils" +) + +// 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:") +} + +// IsNetworkGroupValid check if a network group is valid. +func IsNetworkGroupValid(configuration schema.AccessControlConfiguration, network string) bool { + for _, networks := range configuration.Networks { + if !utils.IsStringInSlice(network, networks.Name) { + continue + } else { + return true + } + } + + return false +} + +// IsNetworkValid check if a network is valid. +func IsNetworkValid(network string) bool { + if net.ParseIP(network) == nil { + _, _, err := net.ParseCIDR(network) + return err == nil + } + + return true +} + +// ValidateAccessControl validates access control configuration. +func ValidateAccessControl(configuration schema.AccessControlConfiguration, validator *schema.StructValidator) { + if !IsPolicyValid(configuration.DefaultPolicy) { + validator.Push(fmt.Errorf("'default_policy' must either be 'deny', 'two_factor', 'one_factor' or 'bypass'")) + } + + if configuration.Networks != nil { + 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)) + } + } + } + } +} + +// ValidateRules validates an ACL Rule configuration. +func ValidateRules(configuration schema.AccessControlConfiguration, validator *schema.StructValidator) { + for _, r := range configuration.Rules { + if len(r.Domains) == 0 { + validator.Push(fmt.Errorf("No access control rules have been defined")) + } + + 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)) + } + + 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)) + } + } + } + } +} diff --git a/internal/configuration/validator/configuration.go b/internal/configuration/validator/configuration.go index a881226b..dabd4232 100644 --- a/internal/configuration/validator/configuration.go +++ b/internal/configuration/validator/configuration.go @@ -64,6 +64,12 @@ func ValidateConfiguration(configuration *schema.Configuration, validator *schem configuration.AccessControl.DefaultPolicy = "deny" } + ValidateAccessControl(configuration.AccessControl, validator) + + if configuration.AccessControl.Rules != nil { + ValidateRules(configuration.AccessControl, validator) + } + ValidateSession(&configuration.Session, validator) if configuration.Regulation == nil { diff --git a/internal/configuration/validator/const.go b/internal/configuration/validator/const.go index af42914b..88a8c44b 100644 --- a/internal/configuration/validator/const.go +++ b/internal/configuration/validator/const.go @@ -26,6 +26,7 @@ var validKeys = []string{ // Access Control Keys. "access_control.rules", "access_control.default_policy", + "access_control.networks", // Session Keys. "session.name", @@ -167,6 +168,8 @@ var specificErrorKeys = map[string]string{ "authentication_backend.file.hashing.parallelism": "config key incorrect: authentication_backend.file.hashing should be authentication_backend.file.password", } +const denyPolicy = "deny" + const argon2id = "argon2id" const sha512 = "sha512" diff --git a/internal/suites/NetworkACL/configuration.yml b/internal/suites/NetworkACL/configuration.yml index 673160b8..71e142ce 100644 --- a/internal/suites/NetworkACL/configuration.yml +++ b/internal/suites/NetworkACL/configuration.yml @@ -32,6 +32,11 @@ storage: # resources. access_control: default_policy: deny + networks: + - name: Clients + networks: + - 192.168.240.202/32 + - 192.168.240.203/32 rules: - domain: secure.example.com policy: one_factor @@ -41,8 +46,7 @@ access_control: - domain: secure.example.com policy: bypass networks: - - 192.168.240.202/32 - - 192.168.240.203/32 + - Clients - domain: secure.example.com policy: two_factor