From 7fefd5e2feba382577c3336fa5ab1aaa2b02d6ea Mon Sep 17 00:00:00 2001 From: Michael Gaffney Date: Thu, 6 Aug 2020 13:03:42 -0400 Subject: [PATCH] Change password (#237) * Implement ChangePassword * Refactor: extract authenticate method * Update tests to verify oplog entries * More refactoring * Remove extraneous clones * More refactoring --- internal/auth/password/argon2.go | 1 + internal/auth/password/errors.go | 4 + internal/auth/password/query.go | 1 + internal/auth/password/repository_password.go | 183 ++++++++++++------ .../auth/password/repository_password_test.go | 180 +++++++++++++++++ internal/db/migrations/postgres.gen.go | 19 ++ .../postgres/12_auth_password.up.sql | 14 ++ .../postgres/13_auth_password_argon.up.sql | 5 + 8 files changed, 350 insertions(+), 57 deletions(-) diff --git a/internal/auth/password/argon2.go b/internal/auth/password/argon2.go index c53a6d4be4..6539de0656 100644 --- a/internal/auth/password/argon2.go +++ b/internal/auth/password/argon2.go @@ -136,6 +136,7 @@ func newArgon2Credential(accountId string, password string, conf *Argon2Configur PrivateId: id, PasswordAccountId: accountId, PasswordConfId: conf.PrivateId, + PasswordMethodId: conf.PasswordMethodId, }, } diff --git a/internal/auth/password/errors.go b/internal/auth/password/errors.go index 00b0dbfb06..fae2738681 100644 --- a/internal/auth/password/errors.go +++ b/internal/auth/password/errors.go @@ -14,4 +14,8 @@ var ( // ErrInvalidConfiguration results from attempting to perform an // operation that sets a password configuration with invalid settings. ErrInvalidConfiguration = errors.New("invalid configuration") + + // ErrPasswordsEqual is returned from ChangePassword when the old and + // new passwords are equal. + ErrPasswordsEqual = errors.New("old and new password are equal") ) diff --git a/internal/auth/password/query.go b/internal/auth/password/query.go index ec3802ed95..f1fa90e852 100644 --- a/internal/auth/password/query.go +++ b/internal/auth/password/query.go @@ -11,6 +11,7 @@ select acct.name, -- Account.Name acct.create_time, -- Account.CreateTime acct.update_time, -- Account.UpdateTime cred.private_id as credential_id, -- Account.CredentialId + cred.private_id, -- Argon2Credential.PrivateId cred.password_conf_id, -- Argon2Credential.PasswordConfId cred.salt, -- Argon2Credential.CtSalt/Salt cred.derived_key, -- Argon2Credential.DerivedKey diff --git a/internal/auth/password/repository_password.go b/internal/auth/password/repository_password.go index 73148797e3..dd263a4d3d 100644 --- a/internal/auth/password/repository_password.go +++ b/internal/auth/password/repository_password.go @@ -10,6 +10,13 @@ import ( "golang.org/x/crypto/argon2" ) +type authAccount struct { + *Account + *Argon2Credential + *Argon2Configuration + IsCurrentConf bool +} + // Authenticate authenticates userName and password match for userName in // authMethodId. The account for the userName is returned if authentication // is successful. Returns nil if authentication fails. @@ -32,14 +39,121 @@ func (r *Repository) Authenticate(ctx context.Context, authMethodId string, user return nil, fmt.Errorf("password authenticate: no password: %w", db.ErrInvalidParameter) } - type AuthAccount struct { - *Account - *Argon2Credential - *Argon2Configuration - IsCurrentConf bool + acct, err := r.authenticate(ctx, authMethodId, userName, password) + if err != nil { + return nil, fmt.Errorf("password authenticate: %w", err) } + if acct == nil { + return nil, nil + } + + if !acct.IsCurrentConf { + cc, err := r.currentConfig(ctx, authMethodId) + if err != nil { + return acct.Account, fmt.Errorf("password authenticate: retrieve current password configuration: %w", err) + } + cred, err := newArgon2Credential(acct.PublicId, password, cc.argon2()) + if err != nil { + return acct.Account, fmt.Errorf("password authenticate: update credential: %w", err) + } - var accts []AuthAccount + // do not change the Credential Id + cred.PrivateId = acct.CredentialId + if err := cred.encrypt(ctx, r.wrapper); err != nil { + return acct.Account, fmt.Errorf("password authenticate: update credential: encrypt: %w", err) + } + + fields := []string{"CtSalt", "DerivedKey", "PasswordConfId"} + metadata := cred.oplog(oplog.OpType_OP_TYPE_UPDATE) + + _, err = r.writer.DoTx(ctx, db.StdRetryCnt, db.ExpBackoff{}, + func(_ db.Reader, w db.Writer) error { + rowsUpdated, err := w.Update(ctx, cred, fields, nil, db.WithOplog(r.wrapper, metadata)) + if err == nil && rowsUpdated > 1 { + return db.ErrMultipleRecords + } + return err + }, + ) + if err != nil { + return acct.Account, fmt.Errorf("password authenticate: update credential: %w", err) + } + } + return acct.Account, nil +} + +// ChangePassword updates the password for userName in authMethodId to new +// if old equals the stored password. The account for the userName is +// returned with a new CredentialId if password is successfully changed. +// +// Returns nil if old does not match the stored password for userName. +// Returns ErrPasswordsEqual if old and new are equal. +func (r *Repository) ChangePassword(ctx context.Context, authMethodId string, userName string, old, new string) (*Account, error) { + if authMethodId == "" { + return nil, fmt.Errorf("change password: no authMethodId: %w", db.ErrInvalidParameter) + } + if userName == "" { + return nil, fmt.Errorf("change password: no userName: %w", db.ErrInvalidParameter) + } + if old == "" { + return nil, fmt.Errorf("change password: no old password: %w", db.ErrInvalidParameter) + } + if new == "" { + return nil, fmt.Errorf("change password: no new password: %w", db.ErrInvalidParameter) + } + if old == new { + return nil, fmt.Errorf("change password: %w", ErrPasswordsEqual) + } + + acct, err := r.authenticate(ctx, authMethodId, userName, old) + if err != nil { + return nil, fmt.Errorf("change password: %w", err) + } + if acct == nil { + return nil, nil + } + + cc, err := r.currentConfig(ctx, authMethodId) + if err != nil { + return nil, fmt.Errorf("change password: retrieve current password configuration: %w", err) + } + if cc.MinPasswordLength > len(new) { + return nil, fmt.Errorf("change password: %w", ErrTooShort) + } + newCred, err := newArgon2Credential(acct.PublicId, new, cc.argon2()) + if err != nil { + return nil, fmt.Errorf("change password: %w", err) + } + + if err := newCred.encrypt(ctx, r.wrapper); err != nil { + return nil, fmt.Errorf("change password: encrypt: %w", err) + } + + oldCred := acct.Argon2Credential + + _, err = r.writer.DoTx(ctx, db.StdRetryCnt, db.ExpBackoff{}, + func(_ db.Reader, w db.Writer) error { + rowsDeleted, err := w.Delete(ctx, oldCred, db.WithOplog(r.wrapper, oldCred.oplog(oplog.OpType_OP_TYPE_DELETE))) + if err == nil && rowsDeleted > 1 { + return db.ErrMultipleRecords + } + if err != nil { + return err + } + return w.Create(ctx, newCred, db.WithOplog(r.wrapper, newCred.oplog(oplog.OpType_OP_TYPE_CREATE))) + }, + ) + if err != nil { + return nil, fmt.Errorf("change password: %w", err) + } + + // change the Credential Id + acct.Account.CredentialId = newCred.PrivateId + return acct.Account, nil +} + +func (r *Repository) authenticate(ctx context.Context, authMethodId string, userName string, password string) (*authAccount, error) { + var accts []authAccount tx, err := r.reader.DB() if err != nil { @@ -51,77 +165,32 @@ func (r *Repository) Authenticate(ctx context.Context, authMethodId string, user } defer rows.Close() for rows.Next() { - var aa AuthAccount + var aa authAccount if err := r.reader.ScanRows(rows, &aa); err != nil { return nil, err } accts = append(accts, aa) } - var acct AuthAccount + var acct authAccount switch { case len(accts) == 0: return nil, nil case len(accts) > 1: // this should never happen - return nil, fmt.Errorf("authenticate: multiple accounts returned for user name") + return nil, fmt.Errorf("multiple accounts returned for user name") default: acct = accts[0] } if err := acct.decrypt(ctx, r.wrapper); err != nil { - return nil, fmt.Errorf("authenticate: credential: cannot decrypt value: %w", err) + return nil, fmt.Errorf("cannot decrypt credential: %w", err) } inputKey := argon2.IDKey([]byte(password), acct.Salt, acct.Iterations, acct.Memory, uint8(acct.Threads), acct.KeyLength) if subtle.ConstantTimeCompare(inputKey, acct.DerivedKey) == 0 { + // authentication failed, password does not match return nil, nil } - - if !acct.IsCurrentConf { - cc, err := r.currentConfig(ctx, authMethodId) - if err != nil { - return acct.Account, fmt.Errorf("authenticate: retrieve current password configuration: %w", err) - } - cred, err := newArgon2Credential(acct.PublicId, password, cc.argon2()) - if err != nil { - return acct.Account, fmt.Errorf("authenticate: rehash current password: %w", err) - } - cred.PrivateId = acct.CredentialId - cred.PasswordMethodId = cc.PasswordMethodId - - var newCred *Argon2Credential - _, err = r.writer.DoTx(ctx, db.StdRetryCnt, db.ExpBackoff{}, - func(_ db.Reader, w db.Writer) error { - newCred = cred.clone() - if err := newCred.encrypt(ctx, r.wrapper); err != nil { - return err - } - rowsUpdated, err := w.Update( - ctx, - newCred, - []string{"CtSalt", "DerivedKey", "PasswordConfId"}, - nil, - db.WithOplog(r.wrapper, cred.oplog(oplog.OpType_OP_TYPE_UPDATE)), - ) - if err == nil && rowsUpdated > 1 { - return db.ErrMultipleRecords - } - return err - }, - ) - if err != nil { - return acct.Account, fmt.Errorf("authenticate: update rehashed password: %w", err) - } - } - return acct.Account, nil -} - -// ChangePassword updates the password for userName in authMethodId to new -// if old equals the stored password. The account for the userName is -// returned with a new CredentialId if password is successfully changed. -// -// Returns nil if old does not match the stored password for userName. -func (r *Repository) ChangePassword(ctx context.Context, authMethodId string, userName string, old, new string) (*Account, error) { - panic("not implemented") + return &acct, nil } diff --git a/internal/auth/password/repository_password_test.go b/internal/auth/password/repository_password_test.go index d72b0c41b4..960bf01824 100644 --- a/internal/auth/password/repository_password_test.go +++ b/internal/auth/password/repository_password_test.go @@ -4,10 +4,12 @@ import ( "context" "errors" "testing" + "time" "github.com/hashicorp/watchtower/internal/auth/password/store" "github.com/hashicorp/watchtower/internal/db" "github.com/hashicorp/watchtower/internal/iam" + "github.com/hashicorp/watchtower/internal/oplog" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -114,6 +116,9 @@ func TestRepository_Authenticate(t *testing.T) { assert.NotEmpty(authAcct.CredentialId, "CredentialId") assert.Equal(tt.args.authMethodId, authAcct.AuthMethodId, "authMethodId") assert.Equal(tt.args.userName, authAcct.UserName, "UserName") + err = db.TestVerifyOplog(t, rw, authAcct.CredentialId, db.WithOperation(oplog.OpType_OP_TYPE_UPDATE), db.WithCreateNotBefore(10*time.Second)) + assert.Error(err) + assert.True(errors.Is(db.ErrRecordNotFound, err)) }) } } @@ -230,4 +235,179 @@ func TestRepository_AuthenticateRehash(t *testing.T) { assert.NotEqual(origCred.PasswordConfId, auth2Cred.PasswordConfId, "the configuration Id should be different") assert.NotEqual(origCred.Salt, auth2Cred.Salt, "a new salt value should be generated") assert.NotEqual(origCred.DerivedKey, auth2Cred.DerivedKey, "the derived key should be different") + + assert.NoError(db.TestVerifyOplog(t, rw, auth2Cred.PrivateId, db.WithOperation(oplog.OpType_OP_TYPE_UPDATE), db.WithCreateNotBefore(10*time.Second))) +} + +func TestRepository_ChangePassword(t *testing.T) { + conn, _ := db.TestSetup(t, "postgres") + rw := db.New(conn) + wrapper := db.TestWrapper(t) + + repo, err := NewRepository(rw, rw, wrapper) + require.NoError(t, err) + require.NotNil(t, repo) + + inAcct := &Account{ + Account: &store.Account{ + UserName: "kazmierczak", + }, + } + passwd := "12345678" + + type args struct { + authMethodId string + userName string + old, new string + } + + var tests = []struct { + name string + args args + useAuthMethodId bool + wantAccount bool + wantIsErr error + }{ + { + name: "invalid-no-authMethodId", + args: args{ + authMethodId: "", + userName: inAcct.UserName, + old: passwd, + new: "12345678-changed", + }, + useAuthMethodId: true, + wantIsErr: db.ErrInvalidParameter, + }, + { + name: "invalid-no-userName", + args: args{ + userName: "", + old: passwd, + new: "12345678-changed", + }, + wantIsErr: db.ErrInvalidParameter, + }, + { + name: "invalid-no-old-password", + args: args{ + userName: inAcct.UserName, + old: "", + new: "12345678-changed", + }, + wantIsErr: db.ErrInvalidParameter, + }, + { + name: "invalid-no-new-password", + args: args{ + userName: inAcct.UserName, + old: passwd, + new: "", + }, + wantIsErr: db.ErrInvalidParameter, + }, + { + name: "invalid-same-passwords", + args: args{ + userName: inAcct.UserName, + old: passwd, + new: passwd, + }, + wantIsErr: ErrPasswordsEqual, + }, + { + name: "auth-failure-unknown-authMethodId", + args: args{ + authMethodId: "not-an-authMethod-Id", + userName: inAcct.UserName, + old: passwd, + new: "12345678-changed", + }, + useAuthMethodId: true, + wantAccount: false, + wantIsErr: nil, + }, + { + name: "auth-failure-wrong-old-password", + args: args{ + userName: inAcct.UserName, + old: "wrong-password", + new: "12345678-changed", + }, + wantAccount: false, + wantIsErr: nil, + }, + { + name: "password-to-short", + args: args{ + userName: inAcct.UserName, + old: passwd, + new: "1", + }, + wantIsErr: ErrTooShort, + }, + { + name: "valid", + args: args{ + userName: inAcct.UserName, + old: passwd, + new: "12345678-changed", + }, + wantAccount: true, + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + assert, require := assert.New(t), require.New(t) + + o, _ := iam.TestScopes(t, conn) + authMethod := TestAuthMethods(t, conn, o.GetPublicId(), 1)[0] + inAcct.AuthMethodId = authMethod.PublicId + outAcct, err := repo.CreateAccount(context.Background(), inAcct, WithPassword(passwd)) + require.NoError(err) + require.NotNil(outAcct) + + if !tt.useAuthMethodId { + tt.args.authMethodId = authMethod.PublicId + } + // Calls to Authenticate should always succeed in these tests + authFn := func(pwd string, name string) *Account { + acct, err := repo.Authenticate(context.Background(), inAcct.AuthMethodId, inAcct.UserName, pwd) + require.NotNilf(acct, "%s: Authenticate should return an account", name) + require.NoErrorf(err, "%s: Authenticate should succeed", name) + return acct + } + // authenticate with original password + authAcct1 := authFn(passwd, "original account") + + chgAuthAcct, err := repo.ChangePassword(context.Background(), tt.args.authMethodId, tt.args.userName, tt.args.old, tt.args.new) + if tt.wantIsErr != nil { + assert.Error(err) + assert.Truef(errors.Is(err, tt.wantIsErr), "want err: %q got: %q", tt.wantIsErr, err) + assert.Nil(chgAuthAcct, "returned account") + authAcct2 := authFn(passwd, "error changing password: using old password") + assert.Equal(authAcct1.CredentialId, authAcct2.CredentialId, "CredentialId should not change") + return + } + assert.NoError(err) + if !tt.wantAccount { + assert.Nil(chgAuthAcct) + authAcct2 := authFn(passwd, "no account from changing password: using old password") + assert.Equal(authAcct1.CredentialId, authAcct2.CredentialId, "CredentialId should not change") + return + } + require.NotNil(chgAuthAcct, "returned account") + assert.NotEmpty(chgAuthAcct.CredentialId, "CredentialId") + assert.NotEqual(authAcct1.CredentialId, chgAuthAcct.CredentialId, "CredentialId should change") + assert.Equal(authAcct1.AuthMethodId, chgAuthAcct.AuthMethodId, "authMethodId") + assert.Equal(authAcct1.UserName, chgAuthAcct.UserName, "UserName") + + authAcct2 := authFn(tt.args.new, "successful change password: using new password") + assert.Equal(chgAuthAcct.CredentialId, authAcct2.CredentialId, "CredentialId should not change") + + assert.NoError(db.TestVerifyOplog(t, rw, authAcct1.CredentialId, db.WithOperation(oplog.OpType_OP_TYPE_DELETE), db.WithCreateNotBefore(10*time.Second))) + assert.NoError(db.TestVerifyOplog(t, rw, authAcct2.CredentialId, db.WithOperation(oplog.OpType_OP_TYPE_CREATE), db.WithCreateNotBefore(10*time.Second))) + }) + } } diff --git a/internal/db/migrations/postgres.gen.go b/internal/db/migrations/postgres.gen.go index 1641fcd786..9040b351e9 100644 --- a/internal/db/migrations/postgres.gen.go +++ b/internal/db/migrations/postgres.gen.go @@ -1836,6 +1836,20 @@ begin; end; $$ language plpgsql; + -- delete_auth_password_credential_subtype() is an after delete trigger + -- function for subtypes of auth_password_credential + create or replace function + delete_auth_password_credential_subtype() + returns trigger + as $$ + begin + delete + from auth_password_credential + where private_id = old.private_id; + return null; -- result is ignored since this is an after trigger + end; + $$ language plpgsql; + -- -- triggers for time columns -- @@ -1993,6 +2007,11 @@ begin; after update on auth_password_argon2_cred for each row execute procedure update_auth_password_credential_subtype(); + create trigger + delete_auth_password_credential_subtype + after delete on auth_password_argon2_cred + for each row execute procedure delete_auth_password_credential_subtype(); + -- -- triggers for time columns -- diff --git a/internal/db/migrations/postgres/12_auth_password.up.sql b/internal/db/migrations/postgres/12_auth_password.up.sql index deae259854..858c77bf14 100644 --- a/internal/db/migrations/postgres/12_auth_password.up.sql +++ b/internal/db/migrations/postgres/12_auth_password.up.sql @@ -204,6 +204,20 @@ begin; end; $$ language plpgsql; + -- delete_auth_password_credential_subtype() is an after delete trigger + -- function for subtypes of auth_password_credential + create or replace function + delete_auth_password_credential_subtype() + returns trigger + as $$ + begin + delete + from auth_password_credential + where private_id = old.private_id; + return null; -- result is ignored since this is an after trigger + end; + $$ language plpgsql; + -- -- triggers for time columns -- diff --git a/internal/db/migrations/postgres/13_auth_password_argon.up.sql b/internal/db/migrations/postgres/13_auth_password_argon.up.sql index 920a989ad9..743f3a7089 100644 --- a/internal/db/migrations/postgres/13_auth_password_argon.up.sql +++ b/internal/db/migrations/postgres/13_auth_password_argon.up.sql @@ -88,6 +88,11 @@ begin; after update on auth_password_argon2_cred for each row execute procedure update_auth_password_credential_subtype(); + create trigger + delete_auth_password_credential_subtype + after delete on auth_password_argon2_cred + for each row execute procedure delete_auth_password_credential_subtype(); + -- -- triggers for time columns --