From 8cdf4a5624855fd137798954fd4de63bb151ccca Mon Sep 17 00:00:00 2001 From: James Elliott Date: Mon, 26 Sep 2022 14:33:08 +1000 Subject: [PATCH] fix(authorization): regex subj doesn't redirect anon user (#4037) This fixes an issue with the authorization policies where if the Domain Regex or Resources criteria would incorrectly return 403 Forbidden statuses instead of 302 Found statuses. Co-authored-by: Amir Zarrinkafsh --- .../authorization/access_control_domain.go | 6 ++-- .../authorization/access_control_resource.go | 6 ++-- internal/authorization/access_control_rule.go | 26 +++++++++----- internal/authorization/authorizer.go | 35 +++++++++--------- internal/authorization/authorizer_test.go | 36 +++++++++---------- internal/authorization/const.go | 2 +- internal/authorization/regexp.go | 8 +++++ internal/authorization/util.go | 32 +++++++++++------ internal/handlers/handler_verify_test.go | 31 ++++++++++++++++ 9 files changed, 122 insertions(+), 60 deletions(-) diff --git a/internal/authorization/access_control_domain.go b/internal/authorization/access_control_domain.go index dfe72264b..d19bdf3dd 100644 --- a/internal/authorization/access_control_domain.go +++ b/internal/authorization/access_control_domain.go @@ -32,7 +32,7 @@ func NewAccessControlDomain(domain string) AccessControlDomain { // NewAccessControlDomainRegex creates a new SubjectObjectMatcher that matches the domain either in a basic way or // dynamic User/Group subexpression group way. -func NewAccessControlDomainRegex(pattern regexp.Regexp) AccessControlDomain { +func NewAccessControlDomainRegex(pattern regexp.Regexp) (subjects bool, rule AccessControlDomain) { var iuser, igroup = -1, -1 for i, group := range pattern.SubexpNames() { @@ -45,10 +45,10 @@ func NewAccessControlDomainRegex(pattern regexp.Regexp) AccessControlDomain { } if iuser != -1 || igroup != -1 { - return AccessControlDomain{RegexpGroupStringSubjectMatcher{pattern, iuser, igroup}} + return true, AccessControlDomain{RegexpGroupStringSubjectMatcher{pattern, iuser, igroup}} } - return AccessControlDomain{RegexpStringSubjectMatcher{pattern}} + return false, AccessControlDomain{RegexpStringSubjectMatcher{pattern}} } // AccessControlDomainMatcher is the basic domain matcher. diff --git a/internal/authorization/access_control_resource.go b/internal/authorization/access_control_resource.go index c9141f863..2c4aa9d1c 100644 --- a/internal/authorization/access_control_resource.go +++ b/internal/authorization/access_control_resource.go @@ -5,7 +5,7 @@ import ( ) // NewAccessControlResource creates a AccessControlResource or AccessControlResourceGroup. -func NewAccessControlResource(pattern regexp.Regexp) AccessControlResource { +func NewAccessControlResource(pattern regexp.Regexp) (subjects bool, rule AccessControlResource) { var iuser, igroup = -1, -1 for i, group := range pattern.SubexpNames() { @@ -18,10 +18,10 @@ func NewAccessControlResource(pattern regexp.Regexp) AccessControlResource { } if iuser != -1 || igroup != -1 { - return AccessControlResource{RegexpGroupStringSubjectMatcher{pattern, iuser, igroup}} + return true, AccessControlResource{RegexpGroupStringSubjectMatcher{pattern, iuser, igroup}} } - return AccessControlResource{RegexpStringSubjectMatcher{pattern}} + return false, AccessControlResource{RegexpStringSubjectMatcher{pattern}} } // AccessControlResource represents an ACL resource that matches without named groups. diff --git a/internal/authorization/access_control_rule.go b/internal/authorization/access_control_rule.go index 077e8fe8d..7b2989e8a 100644 --- a/internal/authorization/access_control_rule.go +++ b/internal/authorization/access_control_rule.go @@ -20,19 +20,29 @@ func NewAccessControlRules(config schema.AccessControlConfiguration) (rules []*A // NewAccessControlRule parses a schema ACL and generates an internal ACL. 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, rule.DomainsRegex), - Resources: schemaResourcesToACL(rule.Resources), - Methods: schemaMethodsToACL(rule.Methods), - Networks: schemaNetworksToACL(rule.Networks, networksMap, networksCacheMap), - Subjects: schemaSubjectsToACL(rule.Subjects), - Policy: StringToLevel(rule.Policy), + r := &AccessControlRule{ + Position: pos, + Domains: schemaDomainsToACL(rule.Domains), + Methods: schemaMethodsToACL(rule.Methods), + Networks: schemaNetworksToACL(rule.Networks, networksMap, networksCacheMap), + Subjects: schemaSubjectsToACL(rule.Subjects), + Policy: StringToLevel(rule.Policy), } + + if len(r.Subjects) != 0 { + r.HasSubjects = true + } + + ruleAddDomainRegex(rule.DomainsRegex, r) + ruleAddResources(rule.Resources, r) + + return r } // AccessControlRule controls and represents an ACL internally. type AccessControlRule struct { + HasSubjects bool + Position int Domains []AccessControlDomain Resources []AccessControlResource diff --git a/internal/authorization/authorizer.go b/internal/authorization/authorizer.go index d08ec3a5b..6ce4dbbbc 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/v4/internal/configuration/schema" "github.com/authelia/authelia/v4/internal/logging" ) @@ -10,15 +12,17 @@ type Authorizer struct { defaultPolicy Level rules []*AccessControlRule mfa bool - configuration *schema.Configuration + config *schema.Configuration + log *logrus.Logger } -// NewAuthorizer create an instance of authorizer with a given access control configuration. -func NewAuthorizer(configuration *schema.Configuration) (authorizer *Authorizer) { +// NewAuthorizer create an instance of authorizer with a given access control config. +func NewAuthorizer(config *schema.Configuration) (authorizer *Authorizer) { authorizer = &Authorizer{ - defaultPolicy: StringToLevel(configuration.AccessControl.DefaultPolicy), - rules: NewAccessControlRules(configuration.AccessControl), - configuration: configuration, + defaultPolicy: StringToLevel(config.AccessControl.DefaultPolicy), + rules: NewAccessControlRules(config.AccessControl), + config: config, + log: logging.Logger(), } if authorizer.defaultPolicy == TwoFactor { @@ -35,8 +39,8 @@ func NewAuthorizer(configuration *schema.Configuration) (authorizer *Authorizer) } } - if authorizer.configuration.IdentityProviders.OIDC != nil { - for _, client := range authorizer.configuration.IdentityProviders.OIDC.Clients { + if authorizer.config.IdentityProviders.OIDC != nil { + for _, client := range authorizer.config.IdentityProviders.OIDC.Clients { if client.Policy == twoFactor { authorizer.mfa = true @@ -54,24 +58,21 @@ func (p Authorizer) IsSecondFactorEnabled() bool { } // GetRequiredLevel retrieve the required level of authorization to access the object. -func (p Authorizer) GetRequiredLevel(subject Subject, object Object) (bool, Level) { - logger := logging.Logger() - - logger.Debugf("Check authorization of subject %s and object %s (method %s).", +func (p Authorizer) GetRequiredLevel(subject Subject, object Object) (hasSubjects bool, level Level) { + p.log.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) + p.log.Tracef(traceFmtACLHitMiss, "HIT", rule.Position, subject, object, object.Method) - return len(rule.Subjects) > 0, rule.Policy + return rule.HasSubjects, rule.Policy } - logger.Tracef(traceFmtACLHitMiss, "MISS", rule.Position, subject.String(), object.String(), object.Method) + p.log.Tracef(traceFmtACLHitMiss, "MISS", rule.Position, subject, object, object.Method) } - logger.Debugf("No matching rule for subject %s and url %s... Applying default policy.", - subject.String(), object.String()) + p.log.Debugf("No matching rule for subject %s and url %s (method %s) applying default policy", subject, object, object.Method) return false, p.defaultPolicy } diff --git a/internal/authorization/authorizer_test.go b/internal/authorization/authorizer_test.go index 264d940c2..ff303f9d5 100644 --- a/internal/authorization/authorizer_test.go +++ b/internal/authorization/authorizer_test.go @@ -278,7 +278,7 @@ func (s *AuthorizerSuite) TestShouldCheckDomainMatching() { s.Require().Len(tester.rules[0].Domains, 1) - s.Assert().Equal("public.example.com", tester.configuration.AccessControl.Rules[0].Domains[0]) + s.Assert().Equal("public.example.com", tester.config.AccessControl.Rules[0].Domains[0]) ruleMatcher0, ok := tester.rules[0].Domains[0].Matcher.(*AccessControlDomainMatcher) s.Require().True(ok) @@ -289,7 +289,7 @@ func (s *AuthorizerSuite) TestShouldCheckDomainMatching() { s.Require().Len(tester.rules[1].Domains, 1) - s.Assert().Equal("one-factor.example.com", tester.configuration.AccessControl.Rules[1].Domains[0]) + s.Assert().Equal("one-factor.example.com", tester.config.AccessControl.Rules[1].Domains[0]) ruleMatcher1, ok := tester.rules[1].Domains[0].Matcher.(*AccessControlDomainMatcher) s.Require().True(ok) @@ -300,7 +300,7 @@ func (s *AuthorizerSuite) TestShouldCheckDomainMatching() { s.Require().Len(tester.rules[2].Domains, 1) - s.Assert().Equal("two-factor.example.com", tester.configuration.AccessControl.Rules[2].Domains[0]) + s.Assert().Equal("two-factor.example.com", tester.config.AccessControl.Rules[2].Domains[0]) ruleMatcher2, ok := tester.rules[2].Domains[0].Matcher.(*AccessControlDomainMatcher) s.Require().True(ok) @@ -311,7 +311,7 @@ func (s *AuthorizerSuite) TestShouldCheckDomainMatching() { s.Require().Len(tester.rules[3].Domains, 1) - s.Assert().Equal("*.example.com", tester.configuration.AccessControl.Rules[3].Domains[0]) + s.Assert().Equal("*.example.com", tester.config.AccessControl.Rules[3].Domains[0]) ruleMatcher3, ok := tester.rules[3].Domains[0].Matcher.(*AccessControlDomainMatcher) s.Require().True(ok) @@ -322,7 +322,7 @@ func (s *AuthorizerSuite) TestShouldCheckDomainMatching() { s.Require().Len(tester.rules[4].Domains, 1) - s.Assert().Equal("*.example.com", tester.configuration.AccessControl.Rules[4].Domains[0]) + s.Assert().Equal("*.example.com", tester.config.AccessControl.Rules[4].Domains[0]) ruleMatcher4, ok := tester.rules[4].Domains[0].Matcher.(*AccessControlDomainMatcher) s.Require().True(ok) @@ -375,7 +375,7 @@ func (s *AuthorizerSuite) TestShouldCheckDomainRegexMatching() { s.Require().Len(tester.rules[0].Domains, 1) - s.Assert().Equal("^.*\\.example.com$", tester.configuration.AccessControl.Rules[0].DomainsRegex[0].String()) + s.Assert().Equal("^.*\\.example.com$", tester.config.AccessControl.Rules[0].DomainsRegex[0].String()) ruleMatcher0, ok := tester.rules[0].Domains[0].Matcher.(RegexpStringSubjectMatcher) s.Require().True(ok) @@ -383,7 +383,7 @@ func (s *AuthorizerSuite) TestShouldCheckDomainRegexMatching() { s.Require().Len(tester.rules[1].Domains, 1) - s.Assert().Equal("^.*\\.example2.com$", tester.configuration.AccessControl.Rules[1].DomainsRegex[0].String()) + s.Assert().Equal("^.*\\.example2.com$", tester.config.AccessControl.Rules[1].DomainsRegex[0].String()) ruleMatcher1, ok := tester.rules[1].Domains[0].Matcher.(RegexpStringSubjectMatcher) s.Require().True(ok) @@ -391,7 +391,7 @@ func (s *AuthorizerSuite) TestShouldCheckDomainRegexMatching() { s.Require().Len(tester.rules[2].Domains, 1) - s.Assert().Equal("^(?P[a-zA-Z0-9]+)\\.regex.com$", tester.configuration.AccessControl.Rules[2].DomainsRegex[0].String()) + s.Assert().Equal("^(?P[a-zA-Z0-9]+)\\.regex.com$", tester.config.AccessControl.Rules[2].DomainsRegex[0].String()) ruleMatcher2, ok := tester.rules[2].Domains[0].Matcher.(RegexpGroupStringSubjectMatcher) s.Require().True(ok) @@ -399,7 +399,7 @@ func (s *AuthorizerSuite) TestShouldCheckDomainRegexMatching() { s.Require().Len(tester.rules[3].Domains, 1) - s.Assert().Equal("^group-(?P[a-zA-Z0-9]+)\\.regex.com$", tester.configuration.AccessControl.Rules[3].DomainsRegex[0].String()) + s.Assert().Equal("^group-(?P[a-zA-Z0-9]+)\\.regex.com$", tester.config.AccessControl.Rules[3].DomainsRegex[0].String()) ruleMatcher3, ok := tester.rules[3].Domains[0].Matcher.(RegexpGroupStringSubjectMatcher) s.Require().True(ok) @@ -407,7 +407,7 @@ func (s *AuthorizerSuite) TestShouldCheckDomainRegexMatching() { s.Require().Len(tester.rules[4].Domains, 1) - s.Assert().Equal("^.*\\.(one|two).com$", tester.configuration.AccessControl.Rules[4].DomainsRegex[0].String()) + s.Assert().Equal("^.*\\.(one|two).com$", tester.config.AccessControl.Rules[4].DomainsRegex[0].String()) ruleMatcher4, ok := tester.rules[4].Domains[0].Matcher.(RegexpStringSubjectMatcher) s.Require().True(ok) @@ -454,30 +454,30 @@ func (s *AuthorizerSuite) TestShouldCheckResourceSubjectMatching() { // Accessing an invalid users Personal page. tester.CheckAuthorizations(s.T(), John, "https://id.example.com/invaliduser/personal", "GET", Denied) tester.CheckAuthorizations(s.T(), Bob, "https://id.example.com/invaliduser/personal", "GET", Denied) - tester.CheckAuthorizations(s.T(), AnonymousUser, "https://id.example.com/invaliduser/personal", "GET", Denied) + tester.CheckAuthorizations(s.T(), AnonymousUser, "https://id.example.com/invaliduser/personal", "GET", OneFactor) // Accessing another users Personal page. tester.CheckAuthorizations(s.T(), John, "https://id.example.com/bob/personal", "GET", Denied) - tester.CheckAuthorizations(s.T(), AnonymousUser, "https://id.example.com/bob/personal", "GET", Denied) + tester.CheckAuthorizations(s.T(), AnonymousUser, "https://id.example.com/bob/personal", "GET", OneFactor) tester.CheckAuthorizations(s.T(), John, "https://id.example.com/Bob/personal", "GET", Denied) - tester.CheckAuthorizations(s.T(), AnonymousUser, "https://id.example.com/Bob/personal", "GET", Denied) + tester.CheckAuthorizations(s.T(), AnonymousUser, "https://id.example.com/Bob/personal", "GET", OneFactor) tester.CheckAuthorizations(s.T(), Bob, "https://id.example.com/john/personal", "GET", Denied) - tester.CheckAuthorizations(s.T(), AnonymousUser, "https://id.example.com/john/personal", "GET", Denied) + tester.CheckAuthorizations(s.T(), AnonymousUser, "https://id.example.com/john/personal", "GET", OneFactor) tester.CheckAuthorizations(s.T(), Bob, "https://id.example.com/John/personal", "GET", Denied) - tester.CheckAuthorizations(s.T(), AnonymousUser, "https://id.example.com/John/personal", "GET", Denied) + tester.CheckAuthorizations(s.T(), AnonymousUser, "https://id.example.com/John/personal", "GET", OneFactor) // Accessing a Group page. tester.CheckAuthorizations(s.T(), John, "https://id.example.com/dev/group", "GET", OneFactor) tester.CheckAuthorizations(s.T(), John, "https://id.example.com/admins/group", "GET", OneFactor) tester.CheckAuthorizations(s.T(), Bob, "https://id.example.com/dev/group", "GET", Denied) tester.CheckAuthorizations(s.T(), Bob, "https://id.example.com/admins/group", "GET", Denied) - tester.CheckAuthorizations(s.T(), AnonymousUser, "https://id.example.com/dev/group", "GET", Denied) - tester.CheckAuthorizations(s.T(), AnonymousUser, "https://id.example.com/admins/group", "GET", Denied) + tester.CheckAuthorizations(s.T(), AnonymousUser, "https://id.example.com/dev/group", "GET", OneFactor) + tester.CheckAuthorizations(s.T(), AnonymousUser, "https://id.example.com/admins/group", "GET", OneFactor) // Accessing an invalid group's Group page. tester.CheckAuthorizations(s.T(), John, "https://id.example.com/invalidgroup/group", "GET", Denied) tester.CheckAuthorizations(s.T(), Bob, "https://id.example.com/invalidgroup/group", "GET", Denied) - tester.CheckAuthorizations(s.T(), AnonymousUser, "https://id.example.com/invalidgroup/group", "GET", Denied) + tester.CheckAuthorizations(s.T(), AnonymousUser, "https://id.example.com/invalidgroup/group", "GET", OneFactor) s.Require().Len(tester.rules, 3) diff --git a/internal/authorization/const.go b/internal/authorization/const.go index c86440207..dcf991563 100644 --- a/internal/authorization/const.go +++ b/internal/authorization/const.go @@ -36,4 +36,4 @@ var ( IdentitySubexpNames = []string{subexpNameUser, subexpNameGroup} ) -const traceFmtACLHitMiss = "ACL %s Position %d for subject %s and object %s (Method %s)" +const traceFmtACLHitMiss = "ACL %s Position %d for subject %s and object %s (method %s)" diff --git a/internal/authorization/regexp.go b/internal/authorization/regexp.go index 7983d842f..f26c5fcbf 100644 --- a/internal/authorization/regexp.go +++ b/internal/authorization/regexp.go @@ -16,6 +16,14 @@ type RegexpGroupStringSubjectMatcher struct { // IsMatch returns true if the underlying pattern matches the input given the subject. func (r RegexpGroupStringSubjectMatcher) IsMatch(input string, subject Subject) (match bool) { + if !r.Pattern.MatchString(input) { + return false + } + + if subject.IsAnonymous() { + return true + } + matches := r.Pattern.FindAllStringSubmatch(input, -1) if matches == nil { return false diff --git a/internal/authorization/util.go b/internal/authorization/util.go index 877e46ca5..cfcd1a5f2 100644 --- a/internal/authorization/util.go +++ b/internal/authorization/util.go @@ -70,24 +70,36 @@ func schemaSubjectToACLSubject(subjectRule string) (subject SubjectMatcher) { return nil } -func schemaDomainsToACL(domainRules []string, domainRegexRules []regexp.Regexp) (domains []AccessControlDomain) { +func schemaDomainsToACL(domainRules []string) (domains []AccessControlDomain) { for _, domainRule := range domainRules { domains = append(domains, NewAccessControlDomain(domainRule)) } - for _, domainRegexRule := range domainRegexRules { - domains = append(domains, NewAccessControlDomainRegex(domainRegexRule)) - } - return domains } -func schemaResourcesToACL(resourceRules []regexp.Regexp) (resources []AccessControlResource) { - for _, resourceRule := range resourceRules { - resources = append(resources, NewAccessControlResource(resourceRule)) - } +func ruleAddDomainRegex(exps []regexp.Regexp, rule *AccessControlRule) { + for _, exp := range exps { + subjects, r := NewAccessControlDomainRegex(exp) - return resources + rule.Domains = append(rule.Domains, r) + + if !rule.HasSubjects && subjects { + rule.HasSubjects = true + } + } +} + +func ruleAddResources(exps []regexp.Regexp, rule *AccessControlRule) { + for _, exp := range exps { + subjects, r := NewAccessControlResource(exp) + + rule.Resources = append(rule.Resources, r) + + if !rule.HasSubjects && subjects { + rule.HasSubjects = true + } + } } func schemaMethodsToACL(methodRules []string) (methods []string) { diff --git a/internal/handlers/handler_verify_test.go b/internal/handlers/handler_verify_test.go index 6ae6e2932..42e66fee2 100644 --- a/internal/handlers/handler_verify_test.go +++ b/internal/handlers/handler_verify_test.go @@ -429,6 +429,37 @@ func TestShouldVerifyWrongCredentialsInBasicAuth(t *testing.T) { "https://test.example.com", actualStatus, expStatus) } +func TestShouldRedirectWithGroups(t *testing.T) { + mock := mocks.NewMockAutheliaCtx(t) + defer mock.Close() + + mock.Ctx.Providers.Authorizer = authorization.NewAuthorizer(&schema.Configuration{ + AccessControl: schema.AccessControlConfiguration{ + DefaultPolicy: "deny", + Rules: []schema.ACLRule{ + { + Domains: []string{"app.example.com"}, + Policy: "one_factor", + Resources: []regexp.Regexp{ + *regexp.MustCompile(`^/code-(?P\w+)([/?].*)?$`), + }, + }, + }, + }, + }) + + mock.Ctx.Request.Header.Set("Accept", "text/html; charset=utf-8") + mock.Ctx.Request.Header.Set(fasthttp.HeaderXForwardedProto, "https") + mock.Ctx.Request.Header.Set(fasthttp.HeaderXForwardedHost, "app.example.com") + mock.Ctx.Request.Header.Set("X-Forwarded-Uri", "/code-test/login") + + mock.Ctx.Request.SetRequestURI("/?rd=https://auth.mydomain.com") + + VerifyGET(verifyGetCfg)(mock.Ctx) + + assert.Equal(t, fasthttp.StatusFound, mock.Ctx.Response.StatusCode()) +} + func TestShouldVerifyFailingPasswordCheckingInBasicAuth(t *testing.T) { mock := mocks.NewMockAutheliaCtx(t) defer mock.Close()