Skip to content

Commit

Permalink
Add mfa_device_state to UserStatusV2
Browse files Browse the repository at this point in the history
This PR introduces the `mfa_device_state` value which is used to specify the weakest MFA device for the account.
When a user has no MFA device, it's set to `MFA_STATE_UNSET`.
When a user has at least one TOTP device, it's set to `MFA_STATE_TOTP`.
When a user ONLY has webauthn devices, it's set to `MFA_STATE_WebAuthn`.

Signed-off-by: Tiago Silva <tiago.silva@goteleport.com>
  • Loading branch information
tigrato committed Oct 2, 2024
1 parent 8a85342 commit e5642a4
Show file tree
Hide file tree
Showing 10 changed files with 2,078 additions and 1,775 deletions.
15 changes: 15 additions & 0 deletions api/proto/teleport/legacy/types/types.proto
Original file line number Diff line number Diff line change
Expand Up @@ -3523,6 +3523,9 @@ message UserStatusV2 {
// perform any password-related activity since then. See RFD 0159 for
// details. Do NOT use this value for authentication purposes!
PasswordState password_state = 1 [(gogoproto.jsontag) = "password_state,omitempty"];
// mfa_device_state reflects what the system knows about the user's MFA device.
// Note that this is a "best effort" property, in that it can be UNSPECIFIED.
MFAState mfa_device_state = 2 [(gogoproto.jsontag) = "mfa_device_state,omitempty"];
}

// PasswordState indicates what is known about existence of user's password.
Expand All @@ -3535,6 +3538,18 @@ enum PasswordState {
PASSWORD_STATE_SET = 2;
}

// MFAState indicates what is known about existence of user's MFA device.
enum MFAState {
// Unable to tell whether the MFA device has been configured.
MFA_STATE_UNSPECIFIED = 0;
// MFA device is known to be not configured.
MFA_STATE_UNSET = 1;
// MFA device is known to be configured using TOTP as the weakest form of MFA.
MFA_STATE_TOTP = 2;
// MFA device is known to be configured using WebAuthn as the weakest form of MFA.
MFA_STATE_WebAuthn = 3;
}

// UserSpecV2 is a specification for V2 user
message UserSpecV2 {
// OIDCIdentities lists associated OpenID Connect identities
Expand Down
3,607 changes: 1,839 additions & 1,768 deletions api/types/types.pb.go

Large diffs are not rendered by default.

12 changes: 12 additions & 0 deletions api/types/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,10 @@ type User interface {
// who were created before this property was introduced and didn't perform any
// password-related activity since then. See RFD 0159 for details.
SetPasswordState(PasswordState)
// SetMFAState sets the MFA state for the user.
SetMFAState(MFAState)
// GetMFAState gets the MFA state for the user.
GetMFAState() MFAState
}

// NewUser creates new empty user
Expand Down Expand Up @@ -557,6 +561,14 @@ func (u *UserV2) SetPasswordState(state PasswordState) {
u.Status.PasswordState = state
}

func (u *UserV2) SetMFAState(state MFAState) {
u.Status.MfaDeviceState = state
}

func (u *UserV2) GetMFAState() MFAState {
return u.Status.MfaDeviceState
}

// IsEmpty returns true if there's no info about who created this user
func (c CreatedBy) IsEmpty() bool {
return c.User.Name == ""
Expand Down
3 changes: 3 additions & 0 deletions lib/auth/machineid/machineidv1/machineidv1_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,7 @@ func TestCreateBot(t *testing.T) {
gotUser,
cmpopts.IgnoreFields(types.Metadata{}, "Revision"),
cmpopts.IgnoreFields(types.CreatedBy{}, "Time"),
cmpopts.IgnoreFields(types.UserStatusV2{}, "MfaDeviceState"),
),
)
}
Expand Down Expand Up @@ -821,6 +822,7 @@ func TestUpdateBot(t *testing.T) {
gotUser,
cmpopts.IgnoreFields(types.Metadata{}, "Revision"),
cmpopts.IgnoreFields(types.CreatedBy{}, "Time"),
cmpopts.IgnoreFields(types.UserStatusV2{}, "MfaDeviceState"),
),
)
}
Expand Down Expand Up @@ -1249,6 +1251,7 @@ func TestUpsertBot(t *testing.T) {
gotUser,
cmpopts.IgnoreFields(types.Metadata{}, "Revision"),
cmpopts.IgnoreFields(types.CreatedBy{}, "Time"),
cmpopts.IgnoreFields(types.UserStatusV2{}, "MfaDeviceState"),
),
)
}
Expand Down
2 changes: 1 addition & 1 deletion lib/auth/tls_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1360,7 +1360,7 @@ func TestGetCurrentUser(t *testing.T) {
Spec: types.UserSpecV2{
Roles: []string{"user:user1"},
},
}, currentUser, cmpopts.IgnoreFields(types.Metadata{}, "Revision")))
}, currentUser, cmpopts.IgnoreFields(types.Metadata{}, "Revision"), cmpopts.IgnoreFields(types.UserStatusV2{}, "MfaDeviceState")))
}

func TestGetCurrentUserRoles(t *testing.T) {
Expand Down
6 changes: 6 additions & 0 deletions lib/services/local/resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -352,6 +352,12 @@ func userFromUserItems(name string, items userItems) (*types.UserV2, error) {
return nil, trace.Wrap(err)
}
user.SetLocalAuth(auth)

if auth != nil {
// when reading with secrets, we can populate the data automatically.
user.SetMFAState(getMFADeviceState(auth.MFA))
}

return user, nil
}

Expand Down
3 changes: 3 additions & 0 deletions lib/services/local/resource_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,10 @@ func runUserResourceTest(
expiry := tt.bk.Clock().Now().Add(time.Minute)

alice := newUserTestCase(t, "alice", nil, withSecrets, expiry)
alice.SetMFAState(types.MFAState_MFA_STATE_TOTP)

bob := newUserTestCase(t, "bob", nil, withSecrets, expiry)
bob.SetMFAState(types.MFAState_MFA_STATE_TOTP)

// Check basic dynamic item creation
err := CreateResources(ctx, tt.bk, alice, bob)
Expand Down
108 changes: 104 additions & 4 deletions lib/services/local/users.go
Original file line number Diff line number Diff line change
Expand Up @@ -338,10 +338,20 @@ func (s *IdentityService) CreateUser(ctx context.Context, user types.User) (type
// technically possible to create a user along with a password using a direct
// RPC call or `tctl create`, so we need to support this case, too.
auth := user.GetLocalAuth()
if auth != nil && len(auth.PasswordHash) > 0 {
user.SetPasswordState(types.PasswordState_PASSWORD_STATE_SET)
if auth != nil {
if len(auth.PasswordHash) > 0 {
user.SetPasswordState(types.PasswordState_PASSWORD_STATE_SET)
}
user.SetMFAState(getMFADeviceState(auth.MFA))
} else {
user.SetPasswordState(types.PasswordState_PASSWORD_STATE_UNSET)
// if the user has no local auth, we need to check if the user
// was previously created and enrolled an MFA device.
state, err := s.buildMFADeviceState(ctx, user.GetName())
if err != nil {
s.log.WithError(err).Warn("Failed to build MFA state for user")
}
user.SetMFAState(state)
}

value, err := services.MarshalUser(user.WithoutSecrets().(types.User))
Expand Down Expand Up @@ -382,6 +392,15 @@ func (s *IdentityService) LegacyUpdateUser(ctx context.Context, user types.User)
}

rev := user.GetRevision()

// if the user has no local auth, we need to check if the user
// was previously created and enrolled an MFA device.
state, err := s.buildMFADeviceState(ctx, user.GetName())
if err != nil {
s.log.WithError(err).Warn("Failed to build MFA state for user")
}
user.SetMFAState(state)

value, err := services.MarshalUser(user.WithoutSecrets().(types.User))
if err != nil {
return nil, trace.Wrap(err)
Expand Down Expand Up @@ -411,6 +430,14 @@ func (s *IdentityService) UpdateUser(ctx context.Context, user types.User) (type
return nil, trace.Wrap(err)
}

// if the user has no local auth, we need to check if the user
// was previously created and enrolled an MFA device.
state, err := s.buildMFADeviceState(ctx, user.GetName())
if err != nil {
s.log.WithError(err).Warn("Failed to build MFA state for user")
}
user.SetMFAState(state)

rev := user.GetRevision()
value, err := services.MarshalUser(user.WithoutSecrets().(types.User))
if err != nil {
Expand Down Expand Up @@ -475,6 +502,13 @@ func (s *IdentityService) UpsertUser(ctx context.Context, user types.User) (type
if err := services.ValidateUser(user); err != nil {
return nil, trace.Wrap(err)
}

state, err := s.buildMFADeviceState(ctx, user.GetName())
if err != nil {
s.log.WithError(err).Warn("Failed to build MFA state for user")
}
user.SetMFAState(state)

rev := user.GetRevision()
value, err := services.MarshalUser(user.WithoutSecrets().(types.User))
if err != nil {
Expand Down Expand Up @@ -544,6 +578,12 @@ func (s *IdentityService) CompareAndSwapUser(ctx context.Context, new, existing
return trace.CompareFailed("user %v did not match expected existing value", new.GetName())
}

state, err := s.buildMFADeviceState(ctx, currentWithoutSecrets.GetName())
if err != nil {
s.log.WithError(err).Warn("Failed to build MFA state for user")
}
newWithoutSecrets.SetMFAState(state)

if item.Value == nil {
v, err := services.MarshalUser(newWithoutSecrets)
if err != nil {
Expand Down Expand Up @@ -630,7 +670,7 @@ func (s *IdentityService) upsertLocalAuthSecrets(ctx context.Context, user strin
}
}
for _, d := range auth.MFA {
if err := s.UpsertMFADevice(ctx, user, d); err != nil {
if err := s.upsertMFADevice(ctx, user, d); err != nil {
return trace.Wrap(err)
}
}
Expand Down Expand Up @@ -1184,6 +1224,15 @@ func globalSessionDataKey(scope, id string) backend.Key {
}

func (s *IdentityService) UpsertMFADevice(ctx context.Context, user string, d *types.MFADevice) error {
if err := s.upsertMFADevice(ctx, user, d); err != nil {
return trace.Wrap(err)
}
if err := s.upsertUserStatusMFADevice(ctx, user); err != nil {
s.log.WithError(err).Warn("Unable to update user status after adding MFA device")
}
return nil
}
func (s *IdentityService) upsertMFADevice(ctx context.Context, user string, d *types.MFADevice) error {
if user == "" {
return trace.BadParameter("missing parameter user")
}
Expand Down Expand Up @@ -1239,6 +1288,51 @@ func (s *IdentityService) UpsertMFADevice(ctx context.Context, user string, d *t
return nil
}

// upsertUserStatusMFADevice updates the user's MFA state based on the devices
// they have.
// It's called after adding or removing an MFA device and ensures the user's
// MFA state is up-to-date and reflects the weakest MFA device they have.
func (s *IdentityService) upsertUserStatusMFADevice(ctx context.Context, user string) error {
devs, err := s.GetMFADevices(ctx, user, false)
if err != nil {
return trace.Wrap(err)
}
mfaState := getMFADeviceState(devs)

_, err = s.UpdateAndSwapUser(
ctx,
user,
false, /*withSecrets*/
func(u types.User) (bool, error) {
u.SetMFAState(mfaState)
return true, nil
})

return trace.Wrap(err)
}

func (s *IdentityService) buildMFADeviceState(ctx context.Context, user string) (types.MFAState, error) {
devs, err := s.GetMFADevices(ctx, user, false)
if err != nil {
return types.MFAState_MFA_STATE_UNSET, trace.Wrap(err)
}
return getMFADeviceState(devs), nil
}

func getMFADeviceState(devs []*types.MFADevice) types.MFAState {
mfaState := types.MFAState_MFA_STATE_UNSET
for _, d := range devs {
if d.GetWebauthn() != nil && mfaState == types.MFAState_MFA_STATE_UNSET {
mfaState = types.MFAState_MFA_STATE_WebAuthn
}
if d.GetTotp() != nil {
mfaState = types.MFAState_MFA_STATE_TOTP
break
}
}
return mfaState
}

func getCredentialID(d *types.MFADevice) ([]byte, bool) {
switch d := d.Device.(type) {
case *types.MFADevice_U2F:
Expand All @@ -1258,7 +1352,13 @@ func (s *IdentityService) DeleteMFADevice(ctx context.Context, user, id string)
}

err := s.Delete(ctx, backend.NewKey(webPrefix, usersPrefix, user, mfaDevicePrefix, id))
return trace.Wrap(err)
if err != nil {
return trace.Wrap(err)
}
if err := s.upsertUserStatusMFADevice(ctx, user); err != nil {
s.log.WithError(err).Warn("Unable to update user status after deleting MFA device")
}
return nil
}

func (s *IdentityService) GetMFADevices(ctx context.Context, user string, withSecrets bool) ([]*types.MFADevice, error) {
Expand Down
96 changes: 94 additions & 2 deletions lib/services/local/users_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1384,14 +1384,106 @@ func TestCompareAndSwapUser(t *testing.T) {
_, err = identity.Backend.Put(ctx, *item)
require.NoError(err)

currentBob, err = identity.GetUser(ctx, "bob", false)
currentBob, err = identity.GetUser(ctx, "bob", true)
require.NoError(err)
require.True(services.UsersEquals(currentBob, bob2))

bob2.SetMFAState(currentBob.GetMFAState())
err = identity.CompareAndSwapUser(ctx, bob1, bob2)
require.NoError(err)

currentBob, err = identity.GetUser(ctx, "bob", false)
require.NoError(err)
require.True(services.UsersEquals(currentBob, bob1))
}

func TestMFADeviceState(t *testing.T) {
t.Parallel()

ctx := context.Background()
clock := clockwork.NewFakeClock()
identity := newIdentityService(t, clock)

bob1, err := types.NewUser("bob")
require.NoError(t, err)
bob1.SetLogins([]string{"bob"})

got, err := identity.CreateUser(ctx, bob1)
require.NoError(t, err)
require.Equal(t, types.MFAState_MFA_STATE_UNSET, got.GetMFAState())

// Set the MFA state to MFA_STATE_TOTP
totpDevice := &types.MFADevice{
Metadata: types.Metadata{
Name: "totp",
},
Id: uuid.NewString(),
AddedAt: clock.Now(),
LastUsed: clock.Now(),
Device: &types.MFADevice_Totp{
Totp: &types.TOTPDevice{
Key: "supersecretkey",
},
},
}
err = identity.UpsertMFADevice(ctx, "bob", totpDevice)
require.NoError(t, err)

got, err = identity.GetUser(ctx, "bob", false)
require.NoError(t, err)
require.Equal(t, types.MFAState_MFA_STATE_TOTP, got.GetMFAState())

// Create webauthn device but state should still be MFA_STATE_TOTP
// because it shows the weakest state.
webauthnDevice := &types.MFADevice{
Metadata: types.Metadata{
Name: "webauthn",
},
Id: uuid.NewString(),
AddedAt: clock.Now(),
LastUsed: clock.Now(),
Device: &types.MFADevice_Webauthn{
Webauthn: &types.WebauthnDevice{
CredentialId: []byte("credential ID"),
PublicKeyCbor: []byte("public key"),
AttestationType: "none",
Aaguid: []byte{1, 2, 3, 4, 5},
SignatureCounter: 10,
},
},
}
err = identity.UpsertMFADevice(ctx, "bob", webauthnDevice)
require.NoError(t, err)

got, err = identity.GetUser(ctx, "bob", false)
require.NoError(t, err)
require.Equal(t, types.MFAState_MFA_STATE_TOTP, got.GetMFAState())

// Delete the TOTP device and the state should be MFA_STATE_WEBAUTHN
err = identity.DeleteMFADevice(ctx, "bob", totpDevice.Id)
require.NoError(t, err)

got, err = identity.GetUser(ctx, "bob", false)
require.NoError(t, err)
require.Equal(t, types.MFAState_MFA_STATE_WebAuthn, got.GetMFAState())

// Delete the Webauthn device and the state should be MFA_STATE_UNSET
err = identity.DeleteMFADevice(ctx, "bob", webauthnDevice.Id)
got, err = identity.GetUser(ctx, "bob", false)
require.NoError(t, err)
require.Equal(t, types.MFAState_MFA_STATE_UNSET, got.GetMFAState())

// Set the MFA state to MFA_STATE_WebAuthn
err = identity.UpsertMFADevice(ctx, "bob", webauthnDevice)
require.NoError(t, err)
got, err = identity.GetUser(ctx, "bob", false)
require.NoError(t, err)
require.Equal(t, types.MFAState_MFA_STATE_WebAuthn, got.GetMFAState())

// Create TOTP device and state should be MFA_STATE_TOTP
err = identity.UpsertMFADevice(ctx, "bob", totpDevice)
require.NoError(t, err)

got, err = identity.GetUser(ctx, "bob", false)
require.NoError(t, err)
require.Equal(t, types.MFAState_MFA_STATE_TOTP, got.GetMFAState())
}
Loading

0 comments on commit e5642a4

Please sign in to comment.