From 4430013baec02986416dbd8741642f3b8514a3b8 Mon Sep 17 00:00:00 2001 From: Daniel Krol Date: Wed, 13 Jul 2022 12:32:48 -0400 Subject: [PATCH] KDF for server password. Save salt in DB. --- auth/auth.go | 51 ++++++++++-- auth/auth_test.go | 70 ++++++++++++++++ go.mod | 8 ++ go.sum | 10 +++ main.go | 1 + server/account.go | 1 + server/auth.go | 1 + server/auth_test.go | 5 +- server/server.go | 1 + server/wallet.go | 1 + store/account_test.go | 64 ++++++++------ store/store.go | 188 ++++++++++++++++++++++-------------------- store/store_test.go | 14 ++-- 13 files changed, 288 insertions(+), 127 deletions(-) diff --git a/auth/auth.go b/auth/auth.go index c0ac176..49a2c9a 100644 --- a/auth/auth.go +++ b/auth/auth.go @@ -2,16 +2,19 @@ package auth import ( "crypto/rand" - "crypto/sha256" "encoding/hex" "fmt" "time" + + "golang.org/x/crypto/scrypt" ) type UserId int32 type Email string type DeviceId string type Password string +type KDFKey string // KDF output +type Salt string type TokenString string type AuthScope string @@ -62,8 +65,46 @@ func (at *AuthToken) ScopeValid(required AuthScope) bool { return at.Scope == ScopeFull || at.Scope == required } -func (p Password) Obfuscate() string { - // TODO KDF instead - hash := sha256.Sum256([]byte(p)) - return hex.EncodeToString(hash[:]) +const SaltLength = 8 + +// https://words.filippo.io/the-scrypt-parameters/ +func passwordScrypt(p Password, saltBytes []byte) ([]byte, error) { + scryptN := 32768 + scryptR := 8 + scryptP := 1 + keyLen := 32 + return scrypt.Key([]byte(p), saltBytes, scryptN, scryptR, scryptP, keyLen) +} + +// Given a password (in the same format submitted via request), generate a +// random salt, run the password and salt thorugh the KDF, and return the salt +// and kdf output. The result generally goes into a database. +func (p Password) Create() (key KDFKey, salt Salt, err error) { + saltBytes := make([]byte, SaltLength) + if _, err := rand.Read(saltBytes); err != nil { + return "", "", fmt.Errorf("Error generating salt: %+v", err) + } + keyBytes, err := passwordScrypt(p, saltBytes) + if err == nil { + key = KDFKey(hex.EncodeToString(keyBytes[:])) + salt = Salt(hex.EncodeToString(saltBytes[:])) + } + return +} + +// Given a password (in the same format submitted via request), a salt, and an +// expected kdf output, run the password and salt thorugh the KDF, and return +// whether the result kdf output matches the kdf test output. +// The salt and test kdf output generally come out of the database, and is used +// to check a submitted password. +func (p Password) Check(checkKey KDFKey, salt Salt) (match bool, err error) { + saltBytes, err := hex.DecodeString(string(salt)) + if err != nil { + return false, fmt.Errorf("Error decoding salt from hex: %+v", err) + } + keyBytes, err := passwordScrypt(p, saltBytes) + if err == nil { + match = KDFKey(hex.EncodeToString(keyBytes[:])) == checkKey + } + return } diff --git a/auth/auth_test.go b/auth/auth_test.go index 1d4ca06..31a753d 100644 --- a/auth/auth_test.go +++ b/auth/auth_test.go @@ -53,3 +53,73 @@ func TestAuthScopeInvalid(t *testing.T) { t.Fatalf("Expected banana to be an invalid scope for carrot") } } + +func TestCreatePassword(t *testing.T) { + // Since the salt is randomized, there's really not much we can do to test + // the create function other than to check the length of the outputs and that + // they're different each time. + + const password = Password("password") + + key1, salt1, err := password.Create() + if err != nil { + t.Error("Error creating password") + } + if len(key1) != 64 { + t.Error("Key has wrong length", key1) + } + if len(salt1) != 16 { + t.Error("Salt has wrong length", salt1) + } + + key2, salt2, err := password.Create() + if err != nil { + t.Error("Error creating password") + } + if key1 == key2 { + t.Error("Key is not random", key1) + } + if salt1 == salt2 { + t.Error("Salt is not random", key1) + } +} + +func TestCheckPassword(t *testing.T) { + const password = Password("password 1") + const key = KDFKey("b9a3669973fcd2da3625e84da9d9a2da87bd280bcb02586851e1cb5bee1efa10") + const salt = Salt("080cbdf6d247c665") + + match, err := password.Check(key, salt) + if err != nil { + t.Error("Error checking password") + } + if !match { + t.Error("Expected password to match correct key and salt") + } + + const wrongKey = KDFKey("0000000073fcd2da3625e84da9d9a2da87bd280bcb02586851e1cb5bee1efa10") + match, err = password.Check(wrongKey, salt) + if err != nil { + t.Error("Error checking password") + } + if match { + t.Error("Expected password to not match incorrect key") + } + + const wrongSalt = Salt("00000000d247c665") + match, err = password.Check(key, wrongSalt) + if err != nil { + t.Error("Error checking password") + } + if match { + t.Error("Expected password to not match incorrect salt") + } + + const invalidSalt = Salt("Whoops") + match, err = password.Check(key, invalidSalt) + if err == nil { + // It does a decode of salt inside the function but not the key so we won't + // test invalid hex string with that + t.Error("Expected password check to fail with invalid salt") + } +} diff --git a/go.mod b/go.mod index 9c31d91..48e61b6 100644 --- a/go.mod +++ b/go.mod @@ -3,3 +3,11 @@ module lbryio/lbry-id go 1.17 require github.com/mattn/go-sqlite3 v1.14.9 + +require ( + golang.org/x/crypto v0.0.0-20220622213112-05595931fe9d // indirect + golang.org/x/net v0.0.0-20220708220712-1185a9018129 // indirect + golang.org/x/sys v0.0.0-20220712014510-0a85c31ab51e // indirect + golang.org/x/term v0.0.0-20220526004731-065cf7ba2467 // indirect + golang.org/x/text v0.3.7 // indirect +) diff --git a/go.sum b/go.sum index 91694bf..ca464a4 100644 --- a/go.sum +++ b/go.sum @@ -1,2 +1,12 @@ github.com/mattn/go-sqlite3 v1.14.9 h1:10HX2Td0ocZpYEjhilsuo6WWtUqttj2Kb0KtD86/KYA= github.com/mattn/go-sqlite3 v1.14.9/go.mod h1:NyWgC/yNuGj7Q9rpYnZvas74GogHl5/Z4A/KQRfk6bU= +golang.org/x/crypto v0.0.0-20220622213112-05595931fe9d h1:sK3txAijHtOK88l68nt020reeT1ZdKLIYetKl95FzVY= +golang.org/x/crypto v0.0.0-20220622213112-05595931fe9d/go.mod h1:IxCIyHEi3zRg3s0A5j5BB6A9Jmi73HwBIUl50j+osU4= +golang.org/x/net v0.0.0-20220708220712-1185a9018129 h1:vucSRfWwTsoXro7P+3Cjlr6flUMtzCwzlvkxEQtHHB0= +golang.org/x/net v0.0.0-20220708220712-1185a9018129/go.mod h1:XRhObCWvk6IyKnWLug+ECip1KBveYUHfp+8e9klMJ9c= +golang.org/x/sys v0.0.0-20220712014510-0a85c31ab51e h1:NHvCuwuS43lGnYhten69ZWqi2QOj/CiDNcKbVqwVoew= +golang.org/x/sys v0.0.0-20220712014510-0a85c31ab51e/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/term v0.0.0-20220526004731-065cf7ba2467 h1:CBpWXWQpIRjzmkkA+M7q9Fqnwd2mZr3AFqexg8YTfoM= +golang.org/x/term v0.0.0-20220526004731-065cf7ba2467/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8= +golang.org/x/text v0.3.7 h1:olpwvP2KacW1ZWvsR7uQhoyTYvKAupfQrRGBFM352Gk= +golang.org/x/text v0.3.7/go.mod h1:u+2+/6zg+i71rQMx5EYifcz6MCKuco9NR6JIITiCfzQ= diff --git a/main.go b/main.go index 32539fd..1cc33a4 100644 --- a/main.go +++ b/main.go @@ -2,6 +2,7 @@ package main import ( "log" + "lbryio/lbry-id/auth" "lbryio/lbry-id/server" "lbryio/lbry-id/store" diff --git a/server/account.go b/server/account.go index 9490ac0..2dfb8df 100644 --- a/server/account.go +++ b/server/account.go @@ -4,6 +4,7 @@ import ( "encoding/json" "fmt" "net/http" + "lbryio/lbry-id/auth" "lbryio/lbry-id/store" ) diff --git a/server/auth.go b/server/auth.go index b92536f..8e90e1e 100644 --- a/server/auth.go +++ b/server/auth.go @@ -4,6 +4,7 @@ import ( "encoding/json" "fmt" "net/http" + "lbryio/lbry-id/auth" "lbryio/lbry-id/store" ) diff --git a/server/auth_test.go b/server/auth_test.go index 3372669..14aa498 100644 --- a/server/auth_test.go +++ b/server/auth_test.go @@ -7,10 +7,11 @@ import ( "io/ioutil" "net/http" "net/http/httptest" - "lbryio/lbry-id/auth" - "lbryio/lbry-id/store" "strings" "testing" + + "lbryio/lbry-id/auth" + "lbryio/lbry-id/store" ) func TestServerAuthHandlerSuccess(t *testing.T) { diff --git a/server/server.go b/server/server.go index 7f95ba8..0f34f2e 100644 --- a/server/server.go +++ b/server/server.go @@ -6,6 +6,7 @@ import ( "log" "net/http" "net/mail" + "lbryio/lbry-id/auth" "lbryio/lbry-id/store" ) diff --git a/server/wallet.go b/server/wallet.go index abc9865..da4ba97 100644 --- a/server/wallet.go +++ b/server/wallet.go @@ -4,6 +4,7 @@ import ( "encoding/json" "fmt" "net/http" + "lbryio/lbry-id/auth" "lbryio/lbry-id/store" "lbryio/lbry-id/wallet" diff --git a/store/account_test.go b/store/account_test.go index d4eb504..6cd1daf 100644 --- a/store/account_test.go +++ b/store/account_test.go @@ -10,40 +10,44 @@ import ( ) func expectAccountMatch(t *testing.T, s *Store, email auth.Email, password auth.Password) { - rows, err := s.db.Query( - `SELECT 1 from accounts WHERE email=? AND password=?`, - email, password.Obfuscate(), - ) + var key auth.KDFKey + var salt auth.Salt + + err := s.db.QueryRow( + `SELECT key, salt from accounts WHERE email=?`, + email, + ).Scan(&key, &salt) if err != nil { t.Fatalf("Error finding account for: %s %s - %+v", email, password, err) } - defer rows.Close() - for rows.Next() { - return // found something, we're good + match, err := password.Check(key, salt) + if err != nil { + t.Fatalf("Error checking password for: %s %s - %+v", email, password, err) + } + if !match { + t.Fatalf("Expected account for: %s %s", email, password) } - - t.Fatalf("Expected account for: %s %s", email, password) } -func expectAccountNotMatch(t *testing.T, s *Store, email auth.Email, password auth.Password) { +func expectAccountNotExists(t *testing.T, s *Store, email auth.Email) { rows, err := s.db.Query( - `SELECT 1 from accounts WHERE email=? AND password=?`, - email, password.Obfuscate(), + `SELECT 1 from accounts WHERE email=?`, + email, ) if err != nil { - t.Fatalf("Error finding account for: %s %s - %+v", email, password, err) + t.Fatalf("Error finding account for: %s - %+v", email, err) } defer rows.Close() for rows.Next() { - t.Fatalf("Expected no account for: %s %s", email, password) + t.Fatalf("Expected no account for: %s", email) } // found nothing, we're good } -// Test CreateAccount, using GetUserId as a helper +// Test CreateAccount // Try CreateAccount twice with the same email and different password, error the second time func TestStoreCreateAccount(t *testing.T) { s, sqliteTmpFile := StoreTestInit(t) @@ -52,7 +56,7 @@ func TestStoreCreateAccount(t *testing.T) { email, password := auth.Email("abc@example.com"), auth.Password("123") // Get an account, come back empty - expectAccountNotMatch(t, &s, email, password) + expectAccountNotExists(t, &s, email) // Create an account if err := s.CreateAccount(email, password); err != nil { @@ -70,14 +74,12 @@ func TestStoreCreateAccount(t *testing.T) { t.Fatalf(`CreateAccount err: wanted "%+v", got "%+v"`, ErrDuplicateAccount, err) } - // Get the email and same *first* password we successfully put in, but not the second + // Get the email and same *first* password we successfully put in expectAccountMatch(t, &s, email, password) - expectAccountNotMatch(t, &s, email, newPassword) } -// Test GetUserId, using CreateAccount as a helper -// Try GetUserId before creating an account (fail), and after (succeed) -func TestStoreGetUserId(t *testing.T) { +// Test GetUserId for nonexisting email +func TestStoreGetUserIdAccountNotExists(t *testing.T) { s, sqliteTmpFile := StoreTestInit(t) defer StoreTestCleanup(sqliteTmpFile) @@ -85,16 +87,26 @@ func TestStoreGetUserId(t *testing.T) { // Check that there's no user id for email and password first if userId, err := s.GetUserId(email, password); err != ErrWrongCredentials || userId != 0 { - t.Fatalf(`CreateAccount err: wanted "%+v", got "%+v. userId: %v"`, ErrWrongCredentials, err, userId) + t.Fatalf(`GetUserId error for nonexistant account: wanted "%+v", got "%+v. userId: %v"`, ErrWrongCredentials, err, userId) } +} - // Create the account - _ = s.CreateAccount(email, password) +// Test GetUserId for existing account, with the correct and incorrect password +func TestStoreGetUserIdAccountExists(t *testing.T) { + s, sqliteTmpFile := StoreTestInit(t) + defer StoreTestCleanup(sqliteTmpFile) + + createdUserId, email, password := makeTestUser(t, &s) // Check that there's now a user id for the email and password - if userId, err := s.GetUserId(email, password); err != nil || userId == 0 { + if userId, err := s.GetUserId(email, password); err != nil || userId != createdUserId { t.Fatalf("Unexpected error in GetUserId: err: %+v userId: %v", err, userId) } + + // Check that it won't return if the wrong password is given + if userId, err := s.GetUserId(email, password+auth.Password("_wrong")); err != ErrWrongCredentials || userId != 0 { + t.Fatalf(`GetUserId error for wrong password: wanted "%+v", got "%+v. userId: %v"`, ErrWrongCredentials, err, userId) + } } func TestStoreAccountEmptyFields(t *testing.T) { @@ -109,7 +121,7 @@ func TestStoreAccountEmptyFields(t *testing.T) { email: "", password: "xyz", }, - // Not testing empty password because it gets obfuscated to something + // Not testing empty key and salt because they get generated to something // non-empty in the method } diff --git a/store/store.go b/store/store.go index d7285ce..4c3d4b3 100644 --- a/store/store.go +++ b/store/store.go @@ -111,11 +111,13 @@ func (s *Store) Migrate() error { ); CREATE TABLE IF NOT EXISTS accounts( email TEXT NOT NULL UNIQUE, - password TEXT NOT NULL, + key TEXT NOT NULL, + salt TEXT NOT NULL, user_id INTEGER PRIMARY KEY AUTOINCREMENT, CHECK ( email <> '' AND - password <> '' + key <> '' AND + salt <> '' ) ); ` @@ -328,13 +330,23 @@ func (s *Store) SetWallet(userId auth.UserId, encryptedWallet wallet.EncryptedWa } func (s *Store) GetUserId(email auth.Email, password auth.Password) (userId auth.UserId, err error) { + var key auth.KDFKey + var salt auth.Salt err = s.db.QueryRow( - `SELECT user_id from accounts WHERE email=? AND password=?`, - email, password.Obfuscate(), - ).Scan(&userId) + `SELECT user_id, key, salt from accounts WHERE email=?`, + email, + ).Scan(&userId, &key, &salt) if err == sql.ErrNoRows { err = ErrWrongCredentials } + if err != nil { + return + } + match, err := password.Check(key, salt) + if err == nil && !match { + err = ErrWrongCredentials + userId = auth.UserId(0) + } return } @@ -343,10 +355,14 @@ func (s *Store) GetUserId(email auth.Email, password auth.Password) (userId auth ///////////// func (s *Store) CreateAccount(email auth.Email, password auth.Password) (err error) { + key, salt, err := password.Create() + if err != nil { + return + } // userId auto-increments _, err = s.db.Exec( - "INSERT INTO accounts (email, password) VALUES(?,?)", - email, password.Obfuscate(), + "INSERT INTO accounts (email, key, salt) VALUES(?,?,?)", + email, key, salt, ) var sqliteErr sqlite3.Error @@ -375,75 +391,14 @@ func (s *Store) ChangePasswordWithWallet( sequence wallet.Sequence, hmac wallet.WalletHmac, ) (err error) { - var userId auth.UserId - - tx, err := s.db.Begin() - if err != nil { - return - } - - // Lots of error conditions. Just defer this. However, we need to make sure to - // make sure the variable `err` is set to the error before we return, instead - // of doing `return `. - endTxn := func() { - if err != nil { - tx.Rollback() - } else { - tx.Commit() - } - } - defer endTxn() - - err = tx.QueryRow( - "SELECT user_id from accounts WHERE email=? AND password=?", - email, oldPassword.Obfuscate(), - ).Scan(&userId) - if err == sql.ErrNoRows { - err = ErrWrongCredentials - return - } - if err != nil { - return - } - - res, err := tx.Exec( - "UPDATE accounts SET password=? WHERE user_id=?", - newPassword.Obfuscate(), userId, + return s.changePassword( + email, + oldPassword, + newPassword, + encryptedWallet, + sequence, + hmac, ) - if err != nil { - return - } - numRows, err := res.RowsAffected() - if err != nil { - return - } - if numRows == 0 { - // Very unexpected error! - err = fmt.Errorf("Password failed to update") - return - } - - res, err = tx.Exec( - `UPDATE wallets SET encrypted_wallet=?, sequence=?, hmac=? - WHERE user_id=? AND sequence=?`, - encryptedWallet, sequence, hmac, userId, sequence-1, - ) - if err != nil { - return - } - numRows, err = res.RowsAffected() - if err != nil { - return - } - if numRows == 0 { - err = ErrWrongSequence - return - } - - // Don't care how many I delete here. Might even be zero. No login token while - // changing password seems plausible. - _, err = tx.Exec("DELETE FROM auth_tokens WHERE user_id=?", userId) - return } // Change password, but with no wallet currently saved. Since there's no @@ -456,6 +411,25 @@ func (s *Store) ChangePasswordNoWallet( email auth.Email, oldPassword auth.Password, newPassword auth.Password, +) (err error) { + return s.changePassword( + email, + oldPassword, + newPassword, + wallet.EncryptedWallet(""), + wallet.Sequence(0), + wallet.WalletHmac(""), + ) +} + +// Common code for for WithWallet and WithNoWallet password change functions +func (s *Store) changePassword( + email auth.Email, + oldPassword auth.Password, + newPassword auth.Password, + encryptedWallet wallet.EncryptedWallet, + sequence wallet.Sequence, + hmac wallet.WalletHmac, ) (err error) { var userId auth.UserId @@ -476,21 +450,35 @@ func (s *Store) ChangePasswordNoWallet( } defer endTxn() + var oldKey auth.KDFKey + var oldSalt auth.Salt + err = tx.QueryRow( - "SELECT user_id from accounts WHERE email=? AND password=?", - email, oldPassword.Obfuscate(), - ).Scan(&userId) + `SELECT user_id, key, salt from accounts WHERE email=?`, + email, + ).Scan(&userId, &oldKey, &oldSalt) if err == sql.ErrNoRows { err = ErrWrongCredentials + } + if err != nil { return } + match, err := oldPassword.Check(oldKey, oldSalt) + if err == nil && !match { + err = ErrWrongCredentials + } + if err != nil { + return + } + + newKey, newSalt, err := newPassword.Create() if err != nil { return } res, err := tx.Exec( - "UPDATE accounts SET password=? WHERE user_id=?", - newPassword.Obfuscate(), userId, + "UPDATE accounts SET key=?, salt=? WHERE user_id=?", + newKey, newSalt, userId, ) if err != nil { return @@ -505,17 +493,39 @@ func (s *Store) ChangePasswordNoWallet( return } - // Assert we have no wallet for this version of the password change function. - var dummy string - err = tx.QueryRow("SELECT 1 FROM wallets WHERE user_id=?", userId).Scan(&dummy) - if err != sql.ErrNoRows { - if err == nil { - // We expected no rows - err = ErrUnexpectedWallet + if encryptedWallet != "" { + // With a wallet expected: update it. + + res, err = tx.Exec( + `UPDATE wallets SET encrypted_wallet=?, sequence=?, hmac=? + WHERE user_id=? AND sequence=?`, + encryptedWallet, sequence, hmac, userId, sequence-1, + ) + if err != nil { + return + } + numRows, err = res.RowsAffected() + if err != nil { + return + } + if numRows == 0 { + err = ErrWrongSequence + return + } + } else { + // With no wallet expected: assert we have no wallet. + + var dummy string + err = tx.QueryRow("SELECT 1 FROM wallets WHERE user_id=?", userId).Scan(&dummy) + if err != sql.ErrNoRows { + if err == nil { + // We expected no rows + err = ErrUnexpectedWallet + return + } + // Some other error return } - // Some other error - return } // Don't care how many I delete here. Might even be zero. No login token while diff --git a/store/store_test.go b/store/store_test.go index a763336..0e9feda 100644 --- a/store/store_test.go +++ b/store/store_test.go @@ -35,22 +35,26 @@ func StoreTestCleanup(tmpFile *os.File) { func makeTestUser(t *testing.T, s *Store) (userId auth.UserId, email auth.Email, password auth.Password) { email, password = auth.Email("abc@example.com"), auth.Password("123") + key, salt, err := password.Create() + if err != nil { + t.Fatalf("Error creating password") + } rows, err := s.db.Query( - "INSERT INTO accounts (email, password) values(?,?) returning user_id", - email, password.Obfuscate(), + "INSERT INTO accounts (email, key, salt) values(?,?,?) returning user_id", + email, key, salt, ) if err != nil { - t.Fatalf("Error setting up account") + t.Fatalf("Error setting up account: %+v", err) } defer rows.Close() for rows.Next() { err := rows.Scan(&userId) if err != nil { - t.Fatalf("Error setting up account") + t.Fatalf("Error setting up account: %+v", err) } return } - t.Fatalf("Error setting up account") + t.Fatalf("Error setting up account - no rows found") return }