From 81e34d84de55b34e2734731b8aab6ad7dd188e57 Mon Sep 17 00:00:00 2001 From: Amir Zarrinkafsh Date: Sat, 16 Jan 2021 21:05:41 +1100 Subject: [PATCH] [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 | 13 -- internal/authorization/ip_matcher.go | 13 +- internal/authorization/ip_matcher_test.go | 34 +++-- internal/authorization/path_matcher.go | 8 +- internal/authorization/subject_matcher.go | 5 - .../configuration/schema/access_control.go | 28 ++++ .../configuration/validator/access_control.go | 35 +++-- .../validator/access_control_test.go | 139 ++++++++++++++++++ .../configuration/validator/configuration.go | 2 +- internal/suites/DuoPush/configuration.yml | 2 +- 10 files changed, 214 insertions(+), 65 deletions(-) create mode 100644 internal/configuration/validator/access_control_test.go diff --git a/internal/authorization/const.go b/internal/authorization/const.go index 597b9c94..99093bd8 100644 --- a/internal/authorization/const.go +++ b/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"}, - }, -} diff --git a/internal/authorization/ip_matcher.go b/internal/authorization/ip_matcher.go index e6da5e71..d76c7c2f 100644 --- a/internal/authorization/ip_matcher.go +++ b/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) - if err != nil { - logging.Logger().Errorf("Failed to parse network %s: %s", network, err) - } - - if ipNet.Contains(ip) { - return true - } - - return false + _, ipNet, _ := net.ParseCIDR(network) + return ipNet.Contains(ip) } // isIPMatching check whether user's IP is in one of the network ranges. diff --git a/internal/authorization/ip_matcher_test.go b/internal/authorization/ip_matcher_test.go index a2649734..ce6107e1 100644 --- a/internal/authorization/ip_matcher_test.go +++ b/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.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.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"}, schema.DefaultACLNetwork)) + 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.0/8"}, 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"}, 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{"192.168.0.0/24", "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"}, 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.False(t, isIPMatching(net.ParseIP("127.1"), []string{"localhost"}, testACLNetwork)) - assert.False(t, isIPMatching(net.ParseIP("not-an-ip"), []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"}, schema.DefaultACLNetwork)) + 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"}, 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"}, 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{"192.168.0.0/24", "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"}, schema.DefaultACLNetwork)) } diff --git a/internal/authorization/path_matcher.go b/internal/authorization/path_matcher.go index ea90c8e7..6eca5c27 100644 --- a/internal/authorization/path_matcher.go +++ b/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) - if err != nil { - // TODO(c.michaud): make sure this is safe in advance to - // avoid checking this case here. - continue - } - + match, _ := regexp.MatchString(pathRegexp, path) if match { return true } diff --git a/internal/authorization/subject_matcher.go b/internal/authorization/subject_matcher.go index 0e2d3b63..90f7aff9 100644 --- a/internal/authorization/subject_matcher.go +++ b/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 { diff --git a/internal/configuration/schema/access_control.go b/internal/configuration/schema/access_control.go index 68010ec3..9000fe1d 100644 --- a/internal/configuration/schema/access_control.go +++ b/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", + }, +} diff --git a/internal/configuration/validator/access_control.go b/internal/configuration/validator/access_control.go index e1b52866..28132244 100644 --- a/internal/configuration/validator/access_control.go +++ b/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)) - } - - 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)) - } - } + 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 _, 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)) } } } diff --git a/internal/configuration/validator/access_control_test.go b/internal/configuration/validator/access_control_test.go new file mode 100644 index 00000000..34bf057a --- /dev/null +++ b/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)) +} diff --git a/internal/configuration/validator/configuration.go b/internal/configuration/validator/configuration.go index dabd4232..90145d51 100644 --- a/internal/configuration/validator/configuration.go +++ b/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) diff --git a/internal/suites/DuoPush/configuration.yml b/internal/suites/DuoPush/configuration.yml index 6328189f..5dc17b62 100644 --- a/internal/suites/DuoPush/configuration.yml +++ b/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