Skip to content

Commit

Permalink
fix(authorization): mfa not detected in custom policies (#7116)
Browse files Browse the repository at this point in the history 
This fixes an issue where the MFA enabled state is not correctly detected if the only 'two_factor' policy configured was a custom OpenID Connect 1.0 policy. This ends up being a bad UX as admins have to configure a dummy 'two_factor' policy instead of it being automatically detected.

Fixes #7103.

Signed-off-by: James Elliott <james-d-elliott@users.noreply.github.com>
  • Loading branch information
@james-d-elliott
james-d-elliott committed Apr 10, 2024
1 parent 475c6a7 commit 2bd63fa
Show file tree
Hide file tree
Showing 4 changed files with 182 additions and 31 deletions.
12 changes: 1 addition & 11 deletions internal/authorization/authorizer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ type Authorizer struct {
defaultPolicy Level
rules []*AccessControlRule
mfa bool
config *schema.Configuration
log *logrus.Logger
}

Expand All @@ -21,7 +20,6 @@ func NewAuthorizer(config *schema.Configuration) (authorizer *Authorizer) {
authorizer = &Authorizer{
defaultPolicy: NewLevel(config.AccessControl.DefaultPolicy),
rules: NewAccessControlRules(config.AccessControl),
config: config,
log: logging.Logger(),
}

Expand All @@ -39,15 +37,7 @@ func NewAuthorizer(config *schema.Configuration) (authorizer *Authorizer) {
}
}

if authorizer.config.IdentityProviders.OIDC != nil {
for _, client := range authorizer.config.IdentityProviders.OIDC.Clients {
if client.AuthorizationPolicy == twoFactor {
authorizer.mfa = true

return authorizer
}
}
}
authorizer.mfa = isOpenIDConnectMFA(config)

return authorizer
}
Expand Down
20 changes: 0 additions & 20 deletions internal/authorization/authorizer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -416,8 +416,6 @@ func (s *AuthorizerSuite) TestShouldCheckDomainMatching() {

s.Require().Len(tester.rules[0].Domains, 1)

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)
s.Assert().Equal("public.example.com", ruleMatcher0.Name)
Expand All @@ -427,8 +425,6 @@ func (s *AuthorizerSuite) TestShouldCheckDomainMatching() {

s.Require().Len(tester.rules[1].Domains, 1)

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)
s.Assert().Equal("one-factor.example.com", ruleMatcher1.Name)
Expand All @@ -438,8 +434,6 @@ func (s *AuthorizerSuite) TestShouldCheckDomainMatching() {

s.Require().Len(tester.rules[2].Domains, 1)

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)
s.Assert().Equal("two-factor.example.com", ruleMatcher2.Name)
Expand All @@ -449,8 +443,6 @@ func (s *AuthorizerSuite) TestShouldCheckDomainMatching() {

s.Require().Len(tester.rules[3].Domains, 1)

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)
s.Assert().Equal(".example.com", ruleMatcher3.Name)
Expand All @@ -460,8 +452,6 @@ func (s *AuthorizerSuite) TestShouldCheckDomainMatching() {

s.Require().Len(tester.rules[4].Domains, 1)

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)
s.Assert().Equal(".example.com", ruleMatcher4.Name)
Expand Down Expand Up @@ -513,40 +503,30 @@ func (s *AuthorizerSuite) TestShouldCheckDomainRegexMatching() {

s.Require().Len(tester.rules[0].Domains, 1)

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)
s.Assert().Equal("^.*\\.example.com$", ruleMatcher0.String())

s.Require().Len(tester.rules[1].Domains, 1)

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)
s.Assert().Equal("^.*\\.example2.com$", ruleMatcher1.String())

s.Require().Len(tester.rules[2].Domains, 1)

s.Assert().Equal("^(?P<User>[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)
s.Assert().Equal("^(?P<User>[a-zA-Z0-9]+)\\.regex.com$", ruleMatcher2.String())

s.Require().Len(tester.rules[3].Domains, 1)

s.Assert().Equal("^group-(?P<Group>[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)
s.Assert().Equal("^group-(?P<Group>[a-zA-Z0-9]+)\\.regex.com$", ruleMatcher3.String())

s.Require().Len(tester.rules[4].Domains, 1)

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)
s.Assert().Equal("^.*\\.(one|two).com$", ruleMatcher4.String())
Expand Down
32 changes: 32 additions & 0 deletions internal/authorization/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -232,3 +232,35 @@ func IsAuthLevelSufficient(authenticationLevel authentication.Level, authorizati

return true
}

func isOpenIDConnectMFA(config *schema.Configuration) (mfa bool) {
if config == nil || config.IdentityProviders.OIDC == nil {
return false
}

for _, client := range config.IdentityProviders.OIDC.Clients {
switch client.AuthorizationPolicy {
case oneFactor:
continue
case twoFactor:
return true
default:
policy, ok := config.IdentityProviders.OIDC.AuthorizationPolicies[client.AuthorizationPolicy]
if !ok {
continue
}

if policy.DefaultPolicy == twoFactor {
return true
}

for _, rule := range policy.Rules {
if rule.Policy == twoFactor {
return true
}
}
}
}

return false
}
149 changes: 149 additions & 0 deletions internal/authorization/util_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,155 @@ func TestSchemaNetworksToACL(t *testing.T) {
}
}

func TestIsOpenIDConnectMFA(t *testing.T) {
testCases := []struct {
name string
have *schema.Configuration
expected bool
}{
{
"ShouldReturnFalseNilConfig",
nil,
false,
},
{
"ShouldReturnFalseNilOIDC",
&schema.Configuration{
IdentityProviders: schema.IdentityProviders{
OIDC: nil,
},
},
false,
},
{
"ShouldReturnFalseNoClients",
&schema.Configuration{
IdentityProviders: schema.IdentityProviders{
OIDC: &schema.IdentityProvidersOpenIDConnect{
Clients: nil,
},
},
},
false,
},
{
"ShouldReturnFalseNoClients2FA",
&schema.Configuration{
IdentityProviders: schema.IdentityProviders{
OIDC: &schema.IdentityProvidersOpenIDConnect{
Clients: []schema.IdentityProvidersOpenIDConnectClient{
{
ID: "one",
AuthorizationPolicy: "one_factor",
},
},
},
},
},
false,
},
{
"ShouldReturnTrueClientsDirect2FA",
&schema.Configuration{
IdentityProviders: schema.IdentityProviders{
OIDC: &schema.IdentityProvidersOpenIDConnect{
Clients: []schema.IdentityProvidersOpenIDConnectClient{
{
ID: "one",
AuthorizationPolicy: "two_factor",
},
},
},
},
},
true,
},
{
"ShouldReturnTrueClientsIndirect2FADefault",
&schema.Configuration{
IdentityProviders: schema.IdentityProviders{
OIDC: &schema.IdentityProvidersOpenIDConnect{
AuthorizationPolicies: map[string]schema.IdentityProvidersOpenIDConnectPolicy{
"example": {
DefaultPolicy: "two_factor",
},
},
Clients: []schema.IdentityProvidersOpenIDConnectClient{
{
ID: "one",
AuthorizationPolicy: "example",
},
},
},
},
},
true,
},
{
"ShouldReturnTrueClientsIndirect2FADefault",
&schema.Configuration{
IdentityProviders: schema.IdentityProviders{
OIDC: &schema.IdentityProvidersOpenIDConnect{
AuthorizationPolicies: map[string]schema.IdentityProvidersOpenIDConnectPolicy{
"example": {
DefaultPolicy: "deny",
Rules: []schema.IdentityProvidersOpenIDConnectPolicyRule{
{
Policy: "two_factor",
},
},
},
},
Clients: []schema.IdentityProvidersOpenIDConnectClient{
{
ID: "one",
AuthorizationPolicy: "example",
},
},
},
},
},
true,
},
{
"ShouldReturnTrueClientsIndirect2FADefault",
&schema.Configuration{
IdentityProviders: schema.IdentityProviders{
OIDC: &schema.IdentityProvidersOpenIDConnect{
AuthorizationPolicies: map[string]schema.IdentityProvidersOpenIDConnectPolicy{
"example": {
DefaultPolicy: "deny",
Rules: []schema.IdentityProvidersOpenIDConnectPolicyRule{
{
Policy: "two_factor",
},
},
},
},
Clients: []schema.IdentityProvidersOpenIDConnectClient{
{
ID: "skip",
AuthorizationPolicy: "skip",
},
{
ID: "one",
AuthorizationPolicy: "example",
},
},
},
},
},
true,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
assert.Equal(t, tc.expected, isOpenIDConnectMFA(tc.have))
})
}
}

func MustParseCIDR(input string) *net.IPNet {
_, out, err := net.ParseCIDR(input)
if err != nil {
Expand Down

0 comments on commit 2bd63fa

Please sign in to comment.