From 1e30b00f7e830573ef4be94aa0fee07be07f68cf Mon Sep 17 00:00:00 2001 From: James Elliott Date: Wed, 14 Apr 2021 20:53:23 +1000 Subject: [PATCH] fix(validator): misleading warning for empty acl domains (#1898) This fixes misleading errors for ACL rules with an empty list of domains. This also enables admins to have a default policy with zero ACL rules as long as the default policy is not deny or bypass. It also adds a rule number to all ACL rule related log messages which is the position in the YAML list plus 1. Lastly it adds comprehensive per rule HIT/MISS logging when Authelia trace logging is enabled. This trace logging includes the rule number. --- internal/authorization/access_control_rule.go | 8 ++- internal/authorization/authorizer.go | 22 ++++++- internal/authorization/const.go | 2 + .../configuration/validator/access_control.go | 62 ++++++++++++------- .../validator/access_control_test.go | 61 ++++++++++++++---- .../configuration/validator/configuration.go | 4 +- .../validator/configuration_test.go | 18 +++++- internal/configuration/validator/const.go | 8 ++- 8 files changed, 136 insertions(+), 49 deletions(-) diff --git a/internal/authorization/access_control_rule.go b/internal/authorization/access_control_rule.go index 4b4e968e3..01fbbf708 100644 --- a/internal/authorization/access_control_rule.go +++ b/internal/authorization/access_control_rule.go @@ -11,16 +11,17 @@ import ( func NewAccessControlRules(config schema.AccessControlConfiguration) (rules []*AccessControlRule) { networksMap, networksCacheMap := parseSchemaNetworks(config.Networks) - for _, schemaRule := range config.Rules { - rules = append(rules, NewAccessControlRule(schemaRule, networksMap, networksCacheMap)) + for i, schemaRule := range config.Rules { + rules = append(rules, NewAccessControlRule(i+1, schemaRule, networksMap, networksCacheMap)) } return rules } // NewAccessControlRule parses a schema ACL and generates an internal ACL. -func NewAccessControlRule(rule schema.ACLRule, networksMap map[string][]*net.IPNet, networksCacheMap map[string]*net.IPNet) *AccessControlRule { +func NewAccessControlRule(pos int, rule schema.ACLRule, networksMap map[string][]*net.IPNet, networksCacheMap map[string]*net.IPNet) *AccessControlRule { return &AccessControlRule{ + Position: pos, Domains: schemaDomainsToACL(rule.Domains), Resources: schemaResourcesToACL(rule.Resources), Methods: schemaMethodsToACL(rule.Methods), @@ -32,6 +33,7 @@ func NewAccessControlRule(rule schema.ACLRule, networksMap map[string][]*net.IPN // AccessControlRule controls and represents an ACL internally. type AccessControlRule struct { + Position int Domains []AccessControlDomain Resources []AccessControlResource Methods []string diff --git a/internal/authorization/authorizer.go b/internal/authorization/authorizer.go index 1e0d654f1..4b5537325 100644 --- a/internal/authorization/authorizer.go +++ b/internal/authorization/authorizer.go @@ -1,6 +1,8 @@ package authorization import ( + "github.com/sirupsen/logrus" + "github.com/authelia/authelia/internal/configuration/schema" "github.com/authelia/authelia/internal/logging" ) @@ -13,6 +15,13 @@ type Authorizer struct { // NewAuthorizer create an instance of authorizer with a given access control configuration. func NewAuthorizer(configuration schema.AccessControlConfiguration) *Authorizer { + if logging.Logger().IsLevelEnabled(logrus.TraceLevel) { + return &Authorizer{ + defaultPolicy: PolicyToLevel(configuration.DefaultPolicy), + rules: NewAccessControlRules(configuration), + } + } + return &Authorizer{ defaultPolicy: PolicyToLevel(configuration.DefaultPolicy), rules: NewAccessControlRules(configuration), @@ -35,17 +44,24 @@ func (p *Authorizer) IsSecondFactorEnabled() bool { } // GetRequiredLevel retrieve the required level of authorization to access the object. -func (p *Authorizer) GetRequiredLevel(subject Subject, object Object) Level { +func (p Authorizer) GetRequiredLevel(subject Subject, object Object) Level { logger := logging.Logger() - logger.Tracef("Check authorization of subject %s and url %s.", subject.String(), object.String()) + + logger.Debugf("Check authorization of subject %s and object %s (method %s).", + subject.String(), object.String(), object.Method) for _, rule := range p.rules { if rule.IsMatch(subject, object) { + logger.Tracef(traceFmtACLHitMiss, "HIT", rule.Position, subject.String(), object.String(), object.Method) + return rule.Policy } + + logger.Tracef(traceFmtACLHitMiss, "MISS", rule.Position, subject.String(), object.String(), object.Method) } - logger.Tracef("No matching rule for subject %s and url %s... Applying default policy.", subject.String(), object.String()) + logger.Debugf("No matching rule for subject %s and url %s... Applying default policy.", + subject.String(), object.String()) return p.defaultPolicy } diff --git a/internal/authorization/const.go b/internal/authorization/const.go index 7e15ab568..8b00cf414 100644 --- a/internal/authorization/const.go +++ b/internal/authorization/const.go @@ -16,3 +16,5 @@ const ( const userPrefix = "user:" const groupPrefix = "group:" + +const traceFmtACLHitMiss = "ACL %s Position %d for subject %s and object %s (Method %s)" diff --git a/internal/configuration/validator/access_control.go b/internal/configuration/validator/access_control.go index 8c770f3da..880a57bfe 100644 --- a/internal/configuration/validator/access_control.go +++ b/internal/configuration/validator/access_control.go @@ -12,7 +12,7 @@ import ( // IsPolicyValid check if policy is valid. func IsPolicyValid(policy string) (isValid bool) { - return policy == denyPolicy || policy == "one_factor" || policy == "two_factor" || policy == bypassPolicy + return policy == denyPolicy || policy == oneFactorPolicy || policy == twoFactorPolicy || policy == bypassPolicy } // IsResourceValid check if a resource is valid. @@ -68,61 +68,75 @@ func ValidateAccessControl(configuration schema.AccessControlConfiguration, vali // 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 configuration.Rules == nil || len(configuration.Rules) == 0 { + if configuration.DefaultPolicy != oneFactorPolicy && configuration.DefaultPolicy != twoFactorPolicy { + validator.Push(fmt.Errorf("Default Policy [%s] is invalid, access control rules must be provided or a policy must either be 'one_factor' or 'two_factor'", configuration.DefaultPolicy)) + + return } - if !IsPolicyValid(r.Policy) { - 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)) + validator.PushWarning(fmt.Errorf("No access control rules have been defined so the default policy %s will be applied to all requests", configuration.DefaultPolicy)) + + return + } + + for i, rule := range configuration.Rules { + rulePosition := i + 1 + + if len(rule.Domains) == 0 { + validator.Push(fmt.Errorf("Rule #%d is invalid, a policy must have one or more domains", rulePosition)) } - validateNetworks(r, configuration, validator) + if !IsPolicyValid(rule.Policy) { + validator.Push(fmt.Errorf("Policy [%s] for rule #%d domain: %s is invalid, a policy must either be 'deny', 'two_factor', 'one_factor' or 'bypass'", rule.Policy, rulePosition, rule.Domains)) + } - validateResources(r, validator) + validateNetworks(rulePosition, rule, configuration, validator) - validateSubjects(r, validator) + validateResources(rulePosition, rule, validator) - validateMethods(r, validator) + validateSubjects(rulePosition, rule, validator) - if r.Policy == bypassPolicy && len(r.Subjects) != 0 { - validator.Push(fmt.Errorf(errAccessControlInvalidPolicyWithSubjects, r.Domains, r.Subjects)) + validateMethods(rulePosition, rule, validator) + + if rule.Policy == bypassPolicy && len(rule.Subjects) != 0 { + validator.Push(fmt.Errorf(errAccessControlInvalidPolicyWithSubjects, rulePosition, rule.Domains, rule.Subjects)) } } } -func validateNetworks(r schema.ACLRule, configuration schema.AccessControlConfiguration, validator *schema.StructValidator) { - for _, network := range r.Networks { +func validateNetworks(rulePosition int, rule schema.ACLRule, configuration schema.AccessControlConfiguration, validator *schema.StructValidator) { + for _, network := range rule.Networks { if !IsNetworkValid(network) { if !IsNetworkGroupValid(configuration, network) { - validator.Push(fmt.Errorf("Network %s for domain: %s is not a valid network or network group", r.Networks, r.Domains)) + validator.Push(fmt.Errorf("Network %s for rule #%d domain: %s is not a valid network or network group", rule.Networks, rulePosition, rule.Domains)) } } } } -func validateResources(r schema.ACLRule, validator *schema.StructValidator) { - for _, resource := range r.Resources { +func validateResources(rulePosition int, rule schema.ACLRule, validator *schema.StructValidator) { + for _, resource := range rule.Resources { if err := IsResourceValid(resource); err != nil { - validator.Push(fmt.Errorf("Resource %s for domain: %s is invalid, %s", r.Resources, r.Domains, err)) + validator.Push(fmt.Errorf("Resource %s for rule #%d domain: %s is invalid, %s", rule.Resources, rulePosition, rule.Domains, err)) } } } -func validateSubjects(r schema.ACLRule, validator *schema.StructValidator) { - for _, subjectRule := range r.Subjects { +func validateSubjects(rulePosition int, rule schema.ACLRule, validator *schema.StructValidator) { + for _, subjectRule := range rule.Subjects { for _, subject := range subjectRule { if !IsSubjectValid(subject) { - validator.Push(fmt.Errorf("Subject %s for domain: %s is invalid, must start with 'user:' or 'group:'", subjectRule, r.Domains)) + validator.Push(fmt.Errorf("Subject %s for rule #%d domain: %s is invalid, must start with 'user:' or 'group:'", subjectRule, rulePosition, rule.Domains)) } } } } -func validateMethods(r schema.ACLRule, validator *schema.StructValidator) { - for _, method := range r.Methods { +func validateMethods(rulePosition int, rule schema.ACLRule, validator *schema.StructValidator) { + for _, method := range rule.Methods { if !utils.IsStringInSliceFold(method, validRequestMethods) { - validator.Push(fmt.Errorf("Method %s for domain: %s is invalid, must be one of the following methods: %s", method, r.Domains, strings.Join(validRequestMethods, ", "))) + validator.Push(fmt.Errorf("Method %s for rule #%d domain: %s is invalid, must be one of the following methods: %s", method, rulePosition, rule.Domains, strings.Join(validRequestMethods, ", "))) } } } diff --git a/internal/configuration/validator/access_control_test.go b/internal/configuration/validator/access_control_test.go index 7105680bd..ed810edaf 100644 --- a/internal/configuration/validator/access_control_test.go +++ b/internal/configuration/validator/access_control_test.go @@ -4,6 +4,7 @@ import ( "fmt" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/suite" "github.com/authelia/authelia/internal/configuration/schema" @@ -56,16 +57,42 @@ func (suite *AccessControl) TestShouldRaiseErrorInvalidNetworkGroupNetwork() { 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{{}} +func (suite *AccessControl) TestShouldRaiseErrorWithNoRulesDefined() { + suite.configuration.Rules = []schema.ACLRule{} ValidateRules(suite.configuration, suite.validator) suite.Assert().False(suite.validator.HasWarnings()) - suite.Require().Len(suite.validator.Errors(), 2) + suite.Require().Len(suite.validator.Errors(), 1) - 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'") + suite.Assert().EqualError(suite.validator.Errors()[0], "Default Policy [deny] is invalid, access control rules must be provided or a policy must either be 'one_factor' or 'two_factor'") +} + +func (suite *AccessControl) TestShouldRaiseWarningWithNoRulesDefined() { + suite.configuration.Rules = []schema.ACLRule{} + + suite.configuration.DefaultPolicy = twoFactorPolicy + + ValidateRules(suite.configuration, suite.validator) + + suite.Assert().False(suite.validator.HasErrors()) + suite.Require().Len(suite.validator.Warnings(), 1) + + suite.Assert().EqualError(suite.validator.Warnings()[0], "No access control rules have been defined so the default policy two_factor will be applied to all requests") +} + +func (suite *AccessControl) TestShouldRaiseErrorsWithEmptyRules() { + suite.configuration.Rules = []schema.ACLRule{{}, {}} + + ValidateRules(suite.configuration, suite.validator) + + suite.Assert().False(suite.validator.HasWarnings()) + suite.Require().Len(suite.validator.Errors(), 4) + + suite.Assert().EqualError(suite.validator.Errors()[0], "Rule #1 is invalid, a policy must have one or more domains") + suite.Assert().EqualError(suite.validator.Errors()[1], "Policy [] for rule #1 domain: [] is invalid, a policy must either be 'deny', 'two_factor', 'one_factor' or 'bypass'") + suite.Assert().EqualError(suite.validator.Errors()[2], "Rule #2 is invalid, a policy must have one or more domains") + suite.Assert().EqualError(suite.validator.Errors()[3], "Policy [] for rule #2 domain: [] is invalid, a policy must either be 'deny', 'two_factor', 'one_factor' or 'bypass'") } func (suite *AccessControl) TestShouldRaiseErrorInvalidPolicy() { @@ -81,7 +108,7 @@ func (suite *AccessControl) TestShouldRaiseErrorInvalidPolicy() { 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'") + suite.Assert().EqualError(suite.validator.Errors()[0], "Policy [invalid] for rule #1 domain: [public.example.com] is invalid, a policy must either be 'deny', 'two_factor', 'one_factor' or 'bypass'") } func (suite *AccessControl) TestShouldRaiseErrorInvalidNetwork() { @@ -98,7 +125,7 @@ func (suite *AccessControl) TestShouldRaiseErrorInvalidNetwork() { 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") + suite.Assert().EqualError(suite.validator.Errors()[0], "Network [abc.def.ghi.jkl/32] for rule #1 domain: [public.example.com] is not a valid network or network group") } func (suite *AccessControl) TestShouldRaiseErrorInvalidMethod() { @@ -115,7 +142,7 @@ func (suite *AccessControl) TestShouldRaiseErrorInvalidMethod() { suite.Assert().False(suite.validator.HasWarnings()) suite.Require().Len(suite.validator.Errors(), 1) - suite.Assert().EqualError(suite.validator.Errors()[0], "Method HOP for domain: [public.example.com] is invalid, must be one of the following methods: GET, HEAD, POST, PUT, PATCH, DELETE, TRACE, CONNECT, OPTIONS") + suite.Assert().EqualError(suite.validator.Errors()[0], "Method HOP for rule #1 domain: [public.example.com] is invalid, must be one of the following methods: GET, HEAD, POST, PUT, PATCH, DELETE, TRACE, CONNECT, OPTIONS") } func (suite *AccessControl) TestShouldRaiseErrorInvalidResource() { @@ -132,7 +159,7 @@ func (suite *AccessControl) TestShouldRaiseErrorInvalidResource() { 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.*`") + suite.Assert().EqualError(suite.validator.Errors()[0], "Resource [^/(api.*] for rule #1 domain: [public.example.com] is invalid, error parsing regexp: missing closing ): `^/(api.*`") } func (suite *AccessControl) TestShouldRaiseErrorInvalidSubject() { @@ -151,10 +178,22 @@ func (suite *AccessControl) TestShouldRaiseErrorInvalidSubject() { suite.Require().Len(suite.validator.Warnings(), 0) suite.Require().Len(suite.validator.Errors(), 2) - suite.Assert().EqualError(suite.validator.Errors()[0], "Subject [invalid] for domain: [public.example.com] is invalid, must start with 'user:' or 'group:'") - suite.Assert().EqualError(suite.validator.Errors()[1], fmt.Sprintf(errAccessControlInvalidPolicyWithSubjects, domains, subjects)) + suite.Assert().EqualError(suite.validator.Errors()[0], "Subject [invalid] for rule #1 domain: [public.example.com] is invalid, must start with 'user:' or 'group:'") + suite.Assert().EqualError(suite.validator.Errors()[1], fmt.Sprintf(errAccessControlInvalidPolicyWithSubjects, 1, domains, subjects)) } func TestAccessControl(t *testing.T) { suite.Run(t, new(AccessControl)) } + +func TestShouldReturnCorrectResultsForValidNetworkGroups(t *testing.T) { + config := schema.AccessControlConfiguration{ + Networks: schema.DefaultACLNetwork, + } + + validNetwork := IsNetworkGroupValid(config, "internal") + invalidNetwork := IsNetworkGroupValid(config, "127.0.0.1") + + assert.True(t, validNetwork) + assert.False(t, invalidNetwork) +} diff --git a/internal/configuration/validator/configuration.go b/internal/configuration/validator/configuration.go index e589c2177..3f6c63355 100644 --- a/internal/configuration/validator/configuration.go +++ b/internal/configuration/validator/configuration.go @@ -72,9 +72,7 @@ func ValidateConfiguration(configuration *schema.Configuration, validator *schem ValidateAccessControl(configuration.AccessControl, validator) - if configuration.AccessControl.Rules != nil { - ValidateRules(configuration.AccessControl, validator) - } + ValidateRules(configuration.AccessControl, validator) ValidateSession(&configuration.Session, validator) diff --git a/internal/configuration/validator/configuration_test.go b/internal/configuration/validator/configuration_test.go index 3024da0f6..751f859dd 100644 --- a/internal/configuration/validator/configuration_test.go +++ b/internal/configuration/validator/configuration_test.go @@ -17,8 +17,12 @@ func newDefaultConfig() schema.Configuration { config.LogLevel = "info" config.LogFormat = "text" config.JWTSecret = testJWTSecret - config.AuthenticationBackend.File = new(schema.FileAuthenticationBackendConfiguration) - config.AuthenticationBackend.File.Path = "/a/path" + config.AuthenticationBackend.File = &schema.FileAuthenticationBackendConfiguration{ + Path: "/a/path", + } + config.AccessControl = schema.AccessControlConfiguration{ + DefaultPolicy: "two_factor", + } config.Session = schema.SessionConfiguration{ Domain: "example.com", Name: "authelia_session", @@ -98,6 +102,16 @@ func TestShouldAddDefaultAccessControl(t *testing.T) { validator := schema.NewStructValidator() config := newDefaultConfig() + config.AccessControl.DefaultPolicy = "" + config.AccessControl.Rules = []schema.ACLRule{ + { + Policy: "bypass", + Domains: []string{ + "public.example.com", + }, + }, + } + ValidateConfiguration(&config, validator) require.Len(t, validator.Errors(), 0) assert.NotNil(t, config.AccessControl) diff --git a/internal/configuration/validator/const.go b/internal/configuration/validator/const.go index c06484c8d..1b125bd11 100644 --- a/internal/configuration/validator/const.go +++ b/internal/configuration/validator/const.go @@ -10,8 +10,10 @@ const ( errFilePHashing = "config key incorrect: authentication_backend.file.password_hashing should be authentication_backend.file.password" errFilePOptions = "config key incorrect: authentication_backend.file.password_options should be authentication_backend.file.password" - denyPolicy = "deny" - bypassPolicy = "bypass" + bypassPolicy = "bypass" + oneFactorPolicy = "one_factor" + twoFactorPolicy = "two_factor" + denyPolicy = "deny" argon2id = "argon2id" sha512 = "sha512" @@ -30,7 +32,7 @@ const ( testTLSCert = "/tmp/cert.pem" testTLSKey = "/tmp/key.pem" - errAccessControlInvalidPolicyWithSubjects = "Policy [bypass] for domain %s with subjects %s is invalid. It is " + + errAccessControlInvalidPolicyWithSubjects = "Policy [bypass] for rule #%d domain %s with subjects %s is invalid. It is " + "not supported to configure both policy bypass and subjects. For more information see: " + "https://www.authelia.com/docs/configuration/access-control.html#combining-subjects-and-the-bypass-policy" )