Skip to content

Commit

Permalink
Return created date with new recovery codes (#8777) (#8903)
Browse files Browse the repository at this point in the history
  • Loading branch information
kimlisa authored Nov 10, 2021
1 parent aabae26 commit e5a6744
Show file tree
Hide file tree
Showing 13 changed files with 802 additions and 695 deletions.
4 changes: 2 additions & 2 deletions api/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -2153,7 +2153,7 @@ func (c *Client) CompleteAccountRecovery(ctx context.Context, req *proto.Complet
}

// CreateAccountRecoveryCodes creates new set of recovery codes for a user, replacing and invalidating any previously owned codes.
func (c *Client) CreateAccountRecoveryCodes(ctx context.Context, req *proto.CreateAccountRecoveryCodesRequest) (*proto.CreateAccountRecoveryCodesResponse, error) {
func (c *Client) CreateAccountRecoveryCodes(ctx context.Context, req *proto.CreateAccountRecoveryCodesRequest) (*proto.RecoveryCodes, error) {
res, err := c.grpc.CreateAccountRecoveryCodes(ctx, req, c.callOpts...)
return res, trail.FromGRPC(err)
}
Expand All @@ -2166,7 +2166,7 @@ func (c *Client) GetAccountRecoveryToken(ctx context.Context, req *proto.GetAcco
}

// GetAccountRecoveryCodes returns the user in context their recovery codes resource without any secrets.
func (c *Client) GetAccountRecoveryCodes(ctx context.Context, req *proto.GetAccountRecoveryCodesRequest) (*types.RecoveryCodesV1, error) {
func (c *Client) GetAccountRecoveryCodes(ctx context.Context, req *proto.GetAccountRecoveryCodesRequest) (*proto.RecoveryCodes, error) {
res, err := c.grpc.GetAccountRecoveryCodes(ctx, req, c.callOpts...)
return res, trail.FromGRPC(err)
}
Expand Down
1,341 changes: 700 additions & 641 deletions api/client/proto/authservice.pb.go

Large diffs are not rendered by default.

30 changes: 18 additions & 12 deletions api/client/proto/authservice.proto
Original file line number Diff line number Diff line change
Expand Up @@ -1209,12 +1209,12 @@ message ChangeUserAuthenticationRequest {
message ChangeUserAuthenticationResponse {
// WebSession is a user's web sesssion created from successful changing of password.
types.WebSessionV2 WebSession = 1 [ (gogoproto.jsontag) = "web_session" ];
// RecoveryCodes are user's new recovery codes. Previous recovery codes become invalid.
// Recovery holds user's new recovery related fields. Previous recovery codes become invalid.
// This field can be empty if a user does not meet the following
// requirements to receive recovery codes:
// - cloud feature is enabled
// - username is in valid email format
repeated string RecoveryCodes = 2 [ (gogoproto.jsontag) = "recovery_codes,omitempty" ];
RecoveryCodes Recovery = 2 [ (gogoproto.jsontag) = "recovery,omitempty" ];
}

// StartAccountRecoveryRequest defines a request to create a recovery start token for a user who is
Expand Down Expand Up @@ -1274,6 +1274,20 @@ message CompleteAccountRecoveryRequest {
}
}

// RecoveryCodes describes account recovery fields. Used as a RPC
// response or as part of a RPC response that requires any of these fields.
message RecoveryCodes {
// Codes holds the list of recovery phrase words.
// Field is only used when new recovery codes are generated and returned to user.
repeated string Codes = 1 [ (gogoproto.jsontag) = "codes,omitempty" ];
// Created is the date the recovery codes were created.
google.protobuf.Timestamp Created = 2 [
(gogoproto.stdtime) = true,
(gogoproto.nullable) = false,
(gogoproto.jsontag) = "created"
];
}

// CreateAccountRecoveryCodesRequest is a request to create new set of recovery codes for a user,
// replacing and invalidating any previously existing codes. Recovery codes can only be given to
// users who meet the following requirements:
Expand All @@ -1287,13 +1301,6 @@ message CreateAccountRecoveryCodesRequest {
string TokenID = 1 [ (gogoproto.jsontag) = "token_id" ];
}

// CreateAccountRecoveryCodesResponse is the response to CreateAccountRecoveryCodesRequest.
message CreateAccountRecoveryCodesResponse {
// RecoveryCodes are user's new account recovery codes. Any previous codes are invalidated by
// the replacement of new codes.
repeated string RecoveryCodes = 1 [ (gogoproto.jsontag) = "recovery_codes" ];
}

// GetAccountRecoveryTokenRequest is a request to return a user token resource after verifying that
// the token in the request is not expired and is of the recovery kind.
message GetAccountRecoveryTokenRequest {
Expand Down Expand Up @@ -1784,8 +1791,7 @@ service AuthService {
// CreateAccountRecoveryCodes (exclusive to cloud users) creates new set of recovery codes for a
// user, replacing and invalidating any previously owned codes. Users can only get recovery
// codes if their username is in a valid email format.
rpc CreateAccountRecoveryCodes(CreateAccountRecoveryCodesRequest)
returns (CreateAccountRecoveryCodesResponse);
rpc CreateAccountRecoveryCodes(CreateAccountRecoveryCodesRequest) returns (RecoveryCodes);
// GetAccountRecoveryToken (exclusive to cloud users) returns a user token resource after
// verifying that the token requested has not expired and is of the correct recovery kind.
// Besides checking for validity of a token ID, it is also used to get basic information from
Expand All @@ -1795,7 +1801,7 @@ service AuthService {
// GetAccountRecoveryCodes (exclusive to cloud users) is a request to return the user in context
// their recovery codes. This request will not return any secrets (the values of recovery
// codes), but instead returns non-sensitive data eg. when the recovery codes were created.
rpc GetAccountRecoveryCodes(GetAccountRecoveryCodesRequest) returns (types.RecoveryCodesV1);
rpc GetAccountRecoveryCodes(GetAccountRecoveryCodesRequest) returns (RecoveryCodes);

// CreatePrivilegeToken returns a new privilege token after a logged in user successfully
// re-authenticates with their second factor device. Privilege token lasts PrivilegeTokenTTL and
Expand Down
2 changes: 1 addition & 1 deletion e
Submodule e updated from 6f3574 to 6b755f
21 changes: 12 additions & 9 deletions lib/auth/accountrecovery.go
Original file line number Diff line number Diff line change
Expand Up @@ -452,7 +452,7 @@ func (s *Server) CompleteAccountRecovery(ctx context.Context, req *proto.Complet
}

// CreateAccountRecoveryCodes implements AuthService.CreateAccountRecoveryCodes.
func (s *Server) CreateAccountRecoveryCodes(ctx context.Context, req *proto.CreateAccountRecoveryCodesRequest) (*proto.CreateAccountRecoveryCodesResponse, error) {
func (s *Server) CreateAccountRecoveryCodes(ctx context.Context, req *proto.CreateAccountRecoveryCodesRequest) (*proto.RecoveryCodes, error) {
const unableToCreateCodesMsg = "unable to create new recovery codes, please contact your system administrator"

if err := s.isAccountRecoveryAllowed(ctx); err != nil {
Expand All @@ -474,7 +474,7 @@ func (s *Server) CreateAccountRecoveryCodes(ctx context.Context, req *proto.Crea
return nil, trace.Wrap(err)
}

codes, err := s.generateAndUpsertRecoveryCodes(ctx, token.GetUser())
newRecovery, err := s.generateAndUpsertRecoveryCodes(ctx, token.GetUser())
if err != nil {
log.Error(trace.DebugReport(err))
return nil, trace.AccessDenied(unableToCreateCodesMsg)
Expand All @@ -487,9 +487,7 @@ func (s *Server) CreateAccountRecoveryCodes(ctx context.Context, req *proto.Crea
}
}

return &proto.CreateAccountRecoveryCodesResponse{
RecoveryCodes: codes,
}, nil
return newRecovery, nil
}

// GetAccountRecoveryToken implements AuthService.GetAccountRecoveryToken.
Expand All @@ -508,7 +506,7 @@ func (s *Server) GetAccountRecoveryToken(ctx context.Context, req *proto.GetAcco
}

// GetAccountRecoveryCodes implements AuthService.GetAccountRecoveryCodes.
func (s *Server) GetAccountRecoveryCodes(ctx context.Context, req *proto.GetAccountRecoveryCodesRequest) (*types.RecoveryCodesV1, error) {
func (s *Server) GetAccountRecoveryCodes(ctx context.Context, req *proto.GetAccountRecoveryCodesRequest) (*proto.RecoveryCodes, error) {
username, err := GetClientUsername(ctx)
if err != nil {
return nil, trace.Wrap(err)
Expand All @@ -519,10 +517,12 @@ func (s *Server) GetAccountRecoveryCodes(ctx context.Context, req *proto.GetAcco
return nil, trace.Wrap(err)
}

return rc, nil
return &proto.RecoveryCodes{
Created: rc.Spec.Created,
}, nil
}

func (s *Server) generateAndUpsertRecoveryCodes(ctx context.Context, username string) ([]string, error) {
func (s *Server) generateAndUpsertRecoveryCodes(ctx context.Context, username string) (*proto.RecoveryCodes, error) {
codes, err := generateRecoveryCodes()
if err != nil {
return nil, trace.Wrap(err)
Expand Down Expand Up @@ -559,7 +559,10 @@ func (s *Server) generateAndUpsertRecoveryCodes(ctx context.Context, username st
log.WithError(err).WithFields(logrus.Fields{"user": username}).Warn("Failed to emit recovery tokens generate event.")
}

return codes, nil
return &proto.RecoveryCodes{
Codes: codes,
Created: rc.Spec.Created,
}, nil
}

// isAccountRecoveryAllowed gets cluster auth configuration and check if cloud, local auth
Expand Down
24 changes: 13 additions & 11 deletions lib/auth/accountrecovery_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,8 @@ func TestGenerateAndUpsertRecoveryCodes(t *testing.T) {
user := "[email protected]"
rc, err := srv.Auth().generateAndUpsertRecoveryCodes(ctx, user)
require.NoError(t, err)
require.Len(t, rc, numOfRecoveryCodes)
require.Len(t, rc.Codes, numOfRecoveryCodes)
require.NotEmpty(t, rc.Created)

// Test codes are not marked used.
recovery, err := srv.Auth().GetRecoveryCodes(ctx, user, true /* withSecrets */)
Expand All @@ -74,7 +75,7 @@ func TestGenerateAndUpsertRecoveryCodes(t *testing.T) {
}

// Test each codes are of correct format and used.
for _, code := range rc {
for _, code := range rc.Codes {
s := strings.Split(code, "-")

// 9 b/c 1 for prefix, 8 for words.
Expand All @@ -94,15 +95,15 @@ func TestGenerateAndUpsertRecoveryCodes(t *testing.T) {
}

// Test with a used code returns error.
err = srv.Auth().verifyRecoveryCode(ctx, user, []byte(rc[0]))
err = srv.Auth().verifyRecoveryCode(ctx, user, []byte(rc.Codes[0]))
require.True(t, trace.IsAccessDenied(err))

// Test with invalid recovery code returns error.
err = srv.Auth().verifyRecoveryCode(ctx, user, []byte("invalidcode"))
require.True(t, trace.IsAccessDenied(err))

// Test with non-existing user returns error.
err = srv.Auth().verifyRecoveryCode(ctx, "doesnotexist", []byte(rc[0]))
err = srv.Auth().verifyRecoveryCode(ctx, "doesnotexist", []byte(rc.Codes[0]))
require.True(t, trace.IsAccessDenied(err))
}

Expand All @@ -116,21 +117,21 @@ func TestRecoveryCodeEventsEmitted(t *testing.T) {
user := "[email protected]"

// Test generated recovery codes event.
tc, err := srv.Auth().generateAndUpsertRecoveryCodes(ctx, user)
rc, err := srv.Auth().generateAndUpsertRecoveryCodes(ctx, user)
require.NoError(t, err)
event := mockEmitter.LastEvent()
require.Equal(t, events.RecoveryCodeGeneratedEvent, event.GetType())
require.Equal(t, events.RecoveryCodesGenerateCode, event.GetCode())

// Test used recovery code event.
err = srv.Auth().verifyRecoveryCode(ctx, user, []byte(tc[0]))
err = srv.Auth().verifyRecoveryCode(ctx, user, []byte(rc.Codes[0]))
require.NoError(t, err)
event = mockEmitter.LastEvent()
require.Equal(t, events.RecoveryCodeUsedEvent, event.GetType())
require.Equal(t, events.RecoveryCodeUseSuccessCode, event.GetCode())

// Re-using the same token emits failed event.
err = srv.Auth().verifyRecoveryCode(ctx, user, []byte(tc[0]))
err = srv.Auth().verifyRecoveryCode(ctx, user, []byte(rc.Codes[0]))
require.Error(t, err)
event = mockEmitter.LastEvent()
require.Equal(t, events.RecoveryCodeUsedEvent, event.GetType())
Expand Down Expand Up @@ -1275,7 +1276,8 @@ func TestCreateAccountRecoveryCodes(t *testing.T) {

default:
require.NoError(t, err)
require.Len(t, res.GetRecoveryCodes(), numOfRecoveryCodes)
require.Len(t, res.GetCodes(), numOfRecoveryCodes)
require.NotEmpty(t, res.GetCreated())

// Check token is deleted after success.
_, err = srv.Auth().Identity.GetUserToken(ctx, req.TokenID)
Expand Down Expand Up @@ -1318,8 +1320,8 @@ func TestGetAccountRecoveryCodes(t *testing.T) {

rc, err := clt.GetAccountRecoveryCodes(ctx, &proto.GetAccountRecoveryCodesRequest{})
require.NoError(t, err)
require.Empty(t, rc.Spec.Codes)
require.NotEmpty(t, rc.Spec.Created)
require.Empty(t, rc.Codes)
require.NotEmpty(t, rc.Created)
}

func triggerLoginLock(t *testing.T, srv *Server, username string) {
Expand Down Expand Up @@ -1416,7 +1418,7 @@ func createUserWithSecondFactors(srv *TestTLSServer) (*userAuthCreds, error) {
return &userAuthCreds{
username: username,
password: password,
recoveryCodes: res.GetRecoveryCodes(),
recoveryCodes: res.GetRecovery().GetCodes(),
totpDev: totpDev,
webDev: webDev,
}, nil
Expand Down
4 changes: 2 additions & 2 deletions lib/auth/auth_with_roles.go
Original file line number Diff line number Diff line change
Expand Up @@ -3605,7 +3605,7 @@ func (a *ServerWithRoles) CompleteAccountRecovery(ctx context.Context, req *prot
}

// CreateAccountRecoveryCodes is implemented by AuthService.CreateAccountRecoveryCodes.
func (a *ServerWithRoles) CreateAccountRecoveryCodes(ctx context.Context, req *proto.CreateAccountRecoveryCodesRequest) (*proto.CreateAccountRecoveryCodesResponse, error) {
func (a *ServerWithRoles) CreateAccountRecoveryCodes(ctx context.Context, req *proto.CreateAccountRecoveryCodesRequest) (*proto.RecoveryCodes, error) {
return a.authServer.CreateAccountRecoveryCodes(ctx, req)
}

Expand Down Expand Up @@ -3635,7 +3635,7 @@ func (a *ServerWithRoles) CreateRegisterChallenge(ctx context.Context, req *prot
}

// GetAccountRecoveryCodes is implemented by AuthService.GetAccountRecoveryCodes.
func (a *ServerWithRoles) GetAccountRecoveryCodes(ctx context.Context, req *proto.GetAccountRecoveryCodesRequest) (*types.RecoveryCodesV1, error) {
func (a *ServerWithRoles) GetAccountRecoveryCodes(ctx context.Context, req *proto.GetAccountRecoveryCodesRequest) (*proto.RecoveryCodes, error) {
// User in context can retrieve their own recovery codes.
return a.authServer.GetAccountRecoveryCodes(ctx, req)
}
Expand Down
4 changes: 2 additions & 2 deletions lib/auth/clt.go
Original file line number Diff line number Diff line change
Expand Up @@ -1802,12 +1802,12 @@ type IdentityService interface {
CompleteAccountRecovery(ctx context.Context, req *proto.CompleteAccountRecoveryRequest) error

// CreateAccountRecoveryCodes creates new set of recovery codes for a user, replacing and invalidating any previously owned codes.
CreateAccountRecoveryCodes(ctx context.Context, req *proto.CreateAccountRecoveryCodesRequest) (*proto.CreateAccountRecoveryCodesResponse, error)
CreateAccountRecoveryCodes(ctx context.Context, req *proto.CreateAccountRecoveryCodesRequest) (*proto.RecoveryCodes, error)
// GetAccountRecoveryToken returns a user token resource after verifying the token in
// request is not expired and is of the correct recovery type.
GetAccountRecoveryToken(ctx context.Context, req *proto.GetAccountRecoveryTokenRequest) (types.UserToken, error)
// GetAccountRecoveryCodes returns the user in context their recovery codes resource without any secrets.
GetAccountRecoveryCodes(ctx context.Context, req *proto.GetAccountRecoveryCodesRequest) (*types.RecoveryCodesV1, error)
GetAccountRecoveryCodes(ctx context.Context, req *proto.GetAccountRecoveryCodesRequest) (*proto.RecoveryCodes, error)

// CreatePrivilegeToken creates a privilege token for the logged in user who has successfully re-authenticated with their second factor.
// A privilege token allows users to perform privileged action eg: add/delete their MFA device.
Expand Down
4 changes: 2 additions & 2 deletions lib/auth/grpcserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -3353,7 +3353,7 @@ func (g *GRPCServer) CompleteAccountRecovery(ctx context.Context, req *proto.Com
}

// CreateAccountRecoveryCodes is implemented by AuthService.CreateAccountRecoveryCodes.
func (g *GRPCServer) CreateAccountRecoveryCodes(ctx context.Context, req *proto.CreateAccountRecoveryCodesRequest) (*proto.CreateAccountRecoveryCodesResponse, error) {
func (g *GRPCServer) CreateAccountRecoveryCodes(ctx context.Context, req *proto.CreateAccountRecoveryCodesRequest) (*proto.RecoveryCodes, error) {
auth, err := g.authenticate(ctx)
if err != nil {
return nil, trace.Wrap(err)
Expand Down Expand Up @@ -3384,7 +3384,7 @@ func (g *GRPCServer) GetAccountRecoveryToken(ctx context.Context, req *proto.Get
}

// GetAccountRecoveryCodes is implemented by AuthService.GetAccountRecoveryCodes.
func (g *GRPCServer) GetAccountRecoveryCodes(ctx context.Context, req *proto.GetAccountRecoveryCodesRequest) (*types.RecoveryCodesV1, error) {
func (g *GRPCServer) GetAccountRecoveryCodes(ctx context.Context, req *proto.GetAccountRecoveryCodesRequest) (*proto.RecoveryCodes, error) {
auth, err := g.authenticate(ctx)
if err != nil {
return nil, trace.Wrap(err)
Expand Down
8 changes: 4 additions & 4 deletions lib/auth/password.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,9 @@ func (s *Server) ChangeUserAuthentication(ctx context.Context, req *proto.Change
recoveryAllowed := s.isAccountRecoveryAllowed(ctx) == nil
createRecoveryCodes := hasEmail && hasMFA && recoveryAllowed

var recoveryCodes []string
var newRecovery *proto.RecoveryCodes
if createRecoveryCodes {
recoveryCodes, err = s.generateAndUpsertRecoveryCodes(ctx, user.GetName())
newRecovery, err = s.generateAndUpsertRecoveryCodes(ctx, user.GetName())
if err != nil {
return nil, trace.Wrap(err)
}
Expand All @@ -85,8 +85,8 @@ func (s *Server) ChangeUserAuthentication(ctx context.Context, req *proto.Change
}

return &proto.ChangeUserAuthenticationResponse{
WebSession: sess,
RecoveryCodes: recoveryCodes,
WebSession: sess,
Recovery: newRecovery,
}, nil
}

Expand Down
9 changes: 8 additions & 1 deletion lib/web/apiserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -1540,7 +1540,14 @@ func (h *Handler) changeUserAuthentication(w http.ResponseWriter, r *http.Reques
return nil, trace.Wrap(err)
}

return res.RecoveryCodes, nil
if res.GetRecovery() == nil {
return &ui.RecoveryCodes{}, nil
}

return &ui.RecoveryCodes{
Codes: res.Recovery.Codes,
Created: &res.Recovery.Created,
}, nil
}

// createResetPasswordToken allows a UI user to reset a user's password.
Expand Down
19 changes: 11 additions & 8 deletions lib/web/apiserver_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1535,9 +1535,10 @@ func (s *WebSuite) TestChangePasswordAndAddTOTPDeviceWithToken(c *C) {
c.Assert(err, IsNil)

// Test that no recovery codes are returned b/c cloud feature isn't enabled.
var recoveryCodes []string
c.Assert(json.Unmarshal(re.Bytes(), &recoveryCodes), IsNil)
c.Assert(recoveryCodes, HasLen, 0)
var response ui.RecoveryCodes
c.Assert(json.Unmarshal(re.Bytes(), &response), IsNil)
c.Assert(response.Codes, IsNil)
c.Assert(response.Created, IsNil)
}

func (s *WebSuite) TestChangePasswordAndAddU2FDeviceWithToken(c *C) {
Expand Down Expand Up @@ -1592,9 +1593,10 @@ func (s *WebSuite) TestChangePasswordAndAddU2FDeviceWithToken(c *C) {
c.Assert(err, IsNil)

// Test that no recovery codes are returned b/c cloud is not turned on.
var recoveryCodes []string
c.Assert(json.Unmarshal(re.Bytes(), &recoveryCodes), IsNil)
c.Assert(recoveryCodes, HasLen, 0)
var response ui.RecoveryCodes
c.Assert(json.Unmarshal(re.Bytes(), &response), IsNil)
c.Assert(response.Codes, IsNil)
c.Assert(response.Created, IsNil)
}

// TestEmptyMotD ensures that responses returned by both /webapi/ping and
Expand Down Expand Up @@ -2802,7 +2804,7 @@ func TestChangeUserAuthentication_recoveryCodesReturnedForCloud(t *testing.T) {
}},
})
require.NoError(t, err)
require.Empty(t, re.RecoveryCodes)
require.Nil(t, re.Recovery)

// Create a user that is valid for recovery.
teleUser, err = types.NewUser("[email protected]")
Expand Down Expand Up @@ -2831,7 +2833,8 @@ func TestChangeUserAuthentication_recoveryCodesReturnedForCloud(t *testing.T) {
}},
})
require.NoError(t, err)
require.Len(t, re.RecoveryCodes, 3)
require.Len(t, re.Recovery.Codes, 3)
require.NotEmpty(t, re.Recovery.Created)
}

type authProviderMock struct {
Expand Down
Loading

0 comments on commit e5a6744

Please sign in to comment.