Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[backport v8] Return created date with new recovery codes (#8777) #8903

Merged
merged 3 commits into from
Nov 10, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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