diff --git a/api/backup_test.go b/api/backup_test.go index 784367f9..27bc112a 100644 --- a/api/backup_test.go +++ b/api/backup_test.go @@ -39,6 +39,12 @@ func TestCreateBackup(t *testing.T) { cfg, err := config.NewConfig(appConfig, gormDB) require.NoError(t, err) + unlockPassword := "" + + // Represent a fully set-up hub: the unlock-password canary is written during + // setup and is required for the password check to pass. + require.NoError(t, cfg.SaveUnlockPasswordCheck(unlockPassword)) + app := &db.App{ Name: "test", AppPubkey: "2b7dea2866958f17c568cf024e113db7a3baa9c253a9016889196b8d0b11c7ae", @@ -64,8 +70,6 @@ func TestCreateBackup(t *testing.T) { albyOAuthSvc: albyOAuthSvc, } - unlockPassword := "" - var buf bytes.Buffer err = theAPI.CreateBackup(unlockPassword, &buf) require.NoError(t, err) diff --git a/config/config.go b/config/config.go index 57a17685..c0434665 100644 --- a/config/config.go +++ b/config/config.go @@ -408,7 +408,18 @@ func (cfg *config) SetAutoUnlockPassword(unlockPassword string) error { func (cfg *config) CheckUnlockPassword(encryptionKey string) bool { decryptedValue, err := cfg.Get("UnlockPasswordCheck", encryptionKey) - return err == nil && (decryptedValue == "" || decryptedValue == unlockPasswordCheck) + // require a non-empty match so an absent or empty canary always fails + return err == nil && decryptedValue != "" && decryptedValue == unlockPasswordCheck +} + +func (cfg *config) IsUnlockPasswordCheckSet() (bool, error) { + // Read the raw value with an empty encryption key so we can detect the + // presence of the canary row without needing the (possibly wrong) password. + value, err := cfg.Get("UnlockPasswordCheck", "") + if err != nil { + return false, fmt.Errorf("read unlock password check: %w", err) + } + return value != "", nil } func (cfg *config) SaveUnlockPasswordCheck(encryptionKey string) error { diff --git a/config/config_test.go b/config/config_test.go index 302c120c..1849e45a 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -56,8 +56,6 @@ func TestCheckUnlockPasswordCache(t *testing.T) { Workdir: ".test", }, db) require.NoError(t, err) - err = cfg.ChangeUnlockPassword("", unlockPassword) - require.NoError(t, err) err = cfg.SaveUnlockPasswordCheck(unlockPassword) require.NoError(t, err) diff --git a/config/models.go b/config/models.go index 0fb88709..48860104 100644 --- a/config/models.go +++ b/config/models.go @@ -94,6 +94,7 @@ type Config interface { GetMempoolUrl() string GetEnv() *AppConfig CheckUnlockPassword(password string) bool + IsUnlockPasswordCheckSet() (bool, error) ChangeUnlockPassword(currentUnlockPassword string, newUnlockPassword string) error SetAutoUnlockPassword(unlockPassword string) error SaveUnlockPasswordCheck(encryptionKey string) error diff --git a/config/tests/config_test.go b/config/tests/config_test.go index ae1bfd0d..c256f542 100644 --- a/config/tests/config_test.go +++ b/config/tests/config_test.go @@ -16,8 +16,6 @@ func TestCheckUnlockPasswordCache_InvalidSecond(t *testing.T) { require.NoError(t, err) defer svc.Remove() - err = svc.Cfg.ChangeUnlockPassword("", unlockPassword) - require.NoError(t, err) err = svc.Cfg.SaveUnlockPasswordCheck(unlockPassword) require.NoError(t, err) @@ -35,8 +33,6 @@ func TestCheckUnlockPasswordCache_InvalidFirst(t *testing.T) { require.NoError(t, err) defer svc.Remove() - err = svc.Cfg.ChangeUnlockPassword("", unlockPassword) - require.NoError(t, err) err = svc.Cfg.SaveUnlockPasswordCheck(unlockPassword) require.NoError(t, err) @@ -58,8 +54,6 @@ func TestCheckUnlockPassword_ChangePassword(t *testing.T) { require.NoError(t, err) defer svc.Remove() - err = svc.Cfg.ChangeUnlockPassword("", unlockPassword) - require.NoError(t, err) err = svc.Cfg.SaveUnlockPasswordCheck(unlockPassword) require.NoError(t, err) @@ -81,6 +75,50 @@ func TestCheckUnlockPassword_ChangePassword(t *testing.T) { assert.True(t, svc.Cfg.CheckUnlockPassword(newUnlockPassword)) } +func TestCheckUnlockPassword_MissingCanaryFailsClosed(t *testing.T) { + svc, err := tests.CreateTestService(t) + require.NoError(t, err) + defer svc.Remove() + + // A fresh hub has not saved the unlock-password canary yet. + set, err := svc.Cfg.IsUnlockPasswordCheckSet() + require.NoError(t, err) + assert.False(t, set) + + // Without the canary, no password may validate - including an empty one. + assert.False(t, svc.Cfg.CheckUnlockPassword("")) + assert.False(t, svc.Cfg.CheckUnlockPassword("any-password")) + + // After the canary is saved, only the correct password validates. + err = svc.Cfg.SaveUnlockPasswordCheck("correct") + require.NoError(t, err) + + set, err = svc.Cfg.IsUnlockPasswordCheckSet() + require.NoError(t, err) + assert.True(t, set) + + assert.True(t, svc.Cfg.CheckUnlockPassword("correct")) + assert.False(t, svc.Cfg.CheckUnlockPassword("wrong")) + assert.False(t, svc.Cfg.CheckUnlockPassword("")) +} + +func TestCheckUnlockPassword_NoPasswordHub(t *testing.T) { + svc, err := tests.CreateTestService(t) + require.NoError(t, err) + defer svc.Remove() + + // A hub configured without an unlock password stores the canary unencrypted; + // the empty password must still validate after the fail-closed change. + err = svc.Cfg.SaveUnlockPasswordCheck("") + require.NoError(t, err) + + set, err := svc.Cfg.IsUnlockPasswordCheckSet() + require.NoError(t, err) + assert.True(t, set) + + assert.True(t, svc.Cfg.CheckUnlockPassword("")) +} + func TestSetIgnore_NoEncryptionKey(t *testing.T) { svc, err := tests.CreateTestService(t) require.NoError(t, err) @@ -219,7 +257,7 @@ func TestJWTSecret_GeneratedOnLoad(t *testing.T) { cfg, err := config.NewConfig(&config.AppConfig{}, svc.DB) require.NoError(t, err) - err = cfg.ChangeUnlockPassword("", "123") + err = cfg.SaveUnlockPasswordCheck("123") require.NoError(t, err) err = cfg.LoadJWTSecret("123") @@ -251,9 +289,6 @@ func TestJWTSecret_WrongPassword(t *testing.T) { cfg, err := config.NewConfig(&config.AppConfig{}, svc.DB) require.NoError(t, err) - err = cfg.ChangeUnlockPassword("", "123") - require.NoError(t, err) - err = cfg.SaveUnlockPasswordCheck("123") require.NoError(t, err) @@ -272,7 +307,7 @@ func TestJWTSecret_ChangePassword(t *testing.T) { cfg, err := config.NewConfig(&config.AppConfig{}, svc.DB) require.NoError(t, err) - err = cfg.ChangeUnlockPassword("", "123") + err = cfg.SaveUnlockPasswordCheck("123") require.NoError(t, err) err = cfg.LoadJWTSecret("123") @@ -282,7 +317,7 @@ func TestJWTSecret_ChangePassword(t *testing.T) { require.NoError(t, err) assert.NotEmpty(t, jwtSecret) - err = cfg.ChangeUnlockPassword("", "1234") + err = cfg.ChangeUnlockPassword("123", "1234") require.NoError(t, err) newJwtSecret, err := cfg.GetJWTSecret() @@ -306,7 +341,7 @@ func TestJWTSecret_ReplaceUnencryptedSecretOnLoad(t *testing.T) { cfg, err := config.NewConfig(&config.AppConfig{}, svc.DB) require.NoError(t, err) - err = cfg.ChangeUnlockPassword("", "123") + err = cfg.SaveUnlockPasswordCheck("123") require.NoError(t, err) // simulate a hub that had an unencrypted JWT secret diff --git a/service/keys/keys.go b/service/keys/keys.go index 039efc1d..1a437d2d 100644 --- a/service/keys/keys.go +++ b/service/keys/keys.go @@ -46,11 +46,15 @@ func NewKeys() *keys { } func (keys *keys) Init(cfg config.Config, encryptionKey string) error { - nostrSecretKey, _ := cfg.Get("NostrSecretKey", encryptionKey) + nostrSecretKey, err := cfg.Get("NostrSecretKey", encryptionKey) + if err != nil { + logger.Logger.WithError(err).Error("Failed to decrypt nostr secret key") + return err + } if nostrSecretKey == "" { nostrSecretKey = nostr.GeneratePrivateKey() - err := cfg.SetUpdate("NostrSecretKey", nostrSecretKey, encryptionKey) + err = cfg.SetUpdate("NostrSecretKey", nostrSecretKey, encryptionKey) if err != nil { logger.Logger.WithError(err).Error("Failed to save generated nostr secret key") return err diff --git a/service/keys/keys_test.go b/service/keys/keys_test.go index 9e8a5131..252fb650 100644 --- a/service/keys/keys_test.go +++ b/service/keys/keys_test.go @@ -110,6 +110,38 @@ func TestGenerateNewMnemonic(t *testing.T) { assert.Equal(t, encryptedChannelsBackupKey.String(), derivedKeyFromKeys.String()) } +func TestInit_WrongPasswordDoesNotOverwriteNostrKey(t *testing.T) { + logger.Init(strconv.Itoa(int(logrus.DebugLevel))) + gormDb, err := db.NewDB(t) + require.NoError(t, err) + defer db.CloseDB(gormDb) + + unlockPassword := "correct" + + cfg, err := config.NewConfig(&config.AppConfig{}, gormDb) + require.NoError(t, err) + + // initialise keys under the correct password, storing an encrypted NostrSecretKey + keys := NewKeys() + err = keys.Init(cfg, unlockPassword) + require.NoError(t, err) + + originalSecret, err := cfg.Get("NostrSecretKey", unlockPassword) + require.NoError(t, err) + require.NotEmpty(t, originalSecret) + + // a wrong password must abort instead of mistaking the failed decrypt for + // "no key yet" and overwriting the stored key with a freshly generated one + keys2 := NewKeys() + err = keys2.Init(cfg, "wrong") + require.Error(t, err) + + // the stored key, decrypted with the correct password, must be unchanged + secretAfter, err := cfg.Get("NostrSecretKey", unlockPassword) + require.NoError(t, err) + assert.Equal(t, originalSecret, secretAfter) +} + func TestGenerateSwapMnemonic(t *testing.T) { logger.Init(strconv.Itoa(int(logrus.DebugLevel))) gormDb, err := db.NewDB(t) diff --git a/service/start.go b/service/start.go index 7ea158a9..8c9786a7 100644 --- a/service/start.go +++ b/service/start.go @@ -273,6 +273,15 @@ func (svc *service) StartApp(encryptionKey string) error { if svc.lnClient != nil { return errors.New("app already started") } + unlockPasswordCheckSet, err := svc.cfg.IsUnlockPasswordCheckSet() + if err != nil { + logger.Logger.WithError(err).Error("Failed to check unlock password check") + return fmt.Errorf("check unlock password check: %w", err) + } + if !unlockPasswordCheckSet { + logger.Logger.Error("Unlock password check is missing from the database") + return errors.New("your wallet data is incomplete and cannot be unlocked. Please restore from a backup") + } if !svc.cfg.CheckUnlockPassword(encryptionKey) { logger.Logger.Errorf("Invalid password") return errors.New("invalid password") diff --git a/tests/mocks/Config.go b/tests/mocks/Config.go index 47adb71c..d10f83c2 100644 --- a/tests/mocks/Config.go +++ b/tests/mocks/Config.go @@ -531,6 +531,59 @@ func (_c *MockConfig_GetRelayUrls_Call) RunAndReturn(run func() []string) *MockC return _c } +// IsUnlockPasswordCheckSet provides a mock function for the type MockConfig +func (_mock *MockConfig) IsUnlockPasswordCheckSet() (bool, error) { + ret := _mock.Called() + + if len(ret) == 0 { + panic("no return value specified for IsUnlockPasswordCheckSet") + } + + var r0 bool + var r1 error + if returnFunc, ok := ret.Get(0).(func() (bool, error)); ok { + return returnFunc() + } + if returnFunc, ok := ret.Get(0).(func() bool); ok { + r0 = returnFunc() + } else { + r0 = ret.Get(0).(bool) + } + if returnFunc, ok := ret.Get(1).(func() error); ok { + r1 = returnFunc() + } else { + r1 = ret.Error(1) + } + return r0, r1 +} + +// MockConfig_IsUnlockPasswordCheckSet_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'IsUnlockPasswordCheckSet' +type MockConfig_IsUnlockPasswordCheckSet_Call struct { + *mock.Call +} + +// IsUnlockPasswordCheckSet is a helper method to define mock.On call +func (_e *MockConfig_Expecter) IsUnlockPasswordCheckSet() *MockConfig_IsUnlockPasswordCheckSet_Call { + return &MockConfig_IsUnlockPasswordCheckSet_Call{Call: _e.mock.On("IsUnlockPasswordCheckSet")} +} + +func (_c *MockConfig_IsUnlockPasswordCheckSet_Call) Run(run func()) *MockConfig_IsUnlockPasswordCheckSet_Call { + _c.Call.Run(func(args mock.Arguments) { + run() + }) + return _c +} + +func (_c *MockConfig_IsUnlockPasswordCheckSet_Call) Return(b bool, err error) *MockConfig_IsUnlockPasswordCheckSet_Call { + _c.Call.Return(b, err) + return _c +} + +func (_c *MockConfig_IsUnlockPasswordCheckSet_Call) RunAndReturn(run func() (bool, error)) *MockConfig_IsUnlockPasswordCheckSet_Call { + _c.Call.Return(run) + return _c +} + // LoadJWTSecret provides a mock function for the type MockConfig func (_mock *MockConfig) LoadJWTSecret(encryptionKey string) error { ret := _mock.Called(encryptionKey)