Skip to content

Commit

Permalink
mfa: fix startup crash when SSO users with MFA expire
Browse files Browse the repository at this point in the history
The user loading code is kind of convoluted: it loads all the separate
backend items from the `/web/users/` prefix into a struct. That struct
is then converted to a full `types.User` object.

For each user there are 3 kinds of backend items:
- `/params` which is the main one, containing a marshalled `types.User`
  object
- `/pwd` which contains the hashed password for local users
- `/mfa/...` which contain registered MFA devices

When an SSO user expires, we delete the first two items but not
`/mfa/...`. This is intentional, to persist MFA devices across logins.
The user loading code would fail because the user was "found" (thanks to
MFA items), but didn't have the mandatory `/params` item.

This PR ignores any users that don't have `/params` instead of
hard-failing all `GetUsers` calls.
  • Loading branch information
Andrew Lytvynov committed May 7, 2021
1 parent 82185f2 commit c2379bb
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 3 deletions.
15 changes: 15 additions & 0 deletions lib/services/local/resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -557,6 +557,15 @@ func collectUserItems(items []backend.Item) (users map[string]userItems, rem []b
}
users[name] = collector
}
// Remove user entries that are incomplete.
//
// For example, an SSO user entry that expired but still has MFA devices
// persisted. These users should not be loaded until they login again.
for user, items := range users {
if !items.complete() {
delete(users, user)
}
}
return users, rem, nil
}

Expand Down Expand Up @@ -613,3 +622,9 @@ func (u *userItems) Len() int {
l += len(u.mfa)
return l
}

// complete checks whether userItems is complete enough to be parsed by
// userFromUserItems.
func (u *userItems) complete() bool {
return u.params != nil
}
15 changes: 12 additions & 3 deletions lib/services/local/resource_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,8 +104,10 @@ func (r *ResourceSuite) TestUserResourceWithSecrets(c *check.C) {
}

func (r *ResourceSuite) runUserResourceTest(c *check.C, withSecrets bool) {
alice := newUserTestCase(c, "alice", nil, withSecrets)
bob := newUserTestCase(c, "bob", nil, withSecrets)
expiry := r.bk.Clock().Now().Add(time.Minute)

alice := newUserTestCase(c, "alice", nil, withSecrets, expiry)
bob := newUserTestCase(c, "bob", nil, withSecrets, expiry)
// Check basic dynamic item creation
r.runCreationChecks(c, alice, bob)
// Check that dynamically created item is compatible with service
Expand All @@ -126,6 +128,12 @@ func (r *ResourceSuite) runUserResourceTest(c *check.C, withSecrets bool) {
c.Errorf("Unexpected user %q", user.GetName())
}
}

// Advance the clock to let the users to expire.
r.bk.Clock().(clockwork.FakeClock).Advance(2 * time.Minute)
allUsers, err = s.GetUsers(withSecrets)
c.Assert(err, check.IsNil)
c.Assert(len(allUsers), check.Equals, 0, check.Commentf("expected all users to expire"))
}

func (r *ResourceSuite) TestCertAuthorityResource(c *check.C) {
Expand Down Expand Up @@ -235,13 +243,14 @@ func localAuthSecretsTestCase(c *check.C) services.LocalAuthSecrets {
return auth
}

func newUserTestCase(c *check.C, name string, roles []string, withSecrets bool) services.User {
func newUserTestCase(c *check.C, name string, roles []string, withSecrets bool, expires time.Time) services.User {
user := services.UserV2{
Kind: services.KindUser,
Version: services.V2,
Metadata: services.Metadata{
Name: name,
Namespace: defaults.Namespace,
Expires: &expires,
},
Spec: services.UserSpecV2{
Roles: roles,
Expand Down

0 comments on commit c2379bb

Please sign in to comment.