lightning: encrypt stored seed#4146
Conversation
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis pull request adds Lightning mnemonic encryption support with platform-specific key storage. The ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/bridgecommon/bridgecommon.go`:
- Around line 280-294: The current BackendEnvironment methods
StoreLightningEncryptionKey and LoadLightningEncryptionKey silently
succeed/return empty when their delegates are nil, which can hide
misconfiguration; change them to return a clear error when
StoreLightningEncryptionKeyFunc or LoadLightningEncryptionKeyFunc is nil so
callers (e.g., Lightning.sealMnemonic/Lightning.unsealMnemonic) fail fast.
Specifically, in BackendEnvironment.StoreLightningEncryptionKey return a non-nil
error if StoreLightningEncryptionKeyFunc == nil (rather than nil), and in
BackendEnvironment.LoadLightningEncryptionKey return ("", error) if
LoadLightningEncryptionKeyFunc == nil; reference the methods
StoreLightningEncryptionKey, LoadLightningEncryptionKey and the
CanEncryptLightningMnemonicFunc contract to ensure activation-time
misconfiguration surfaces immediately.
In `@backend/lightning/lightning.go`:
- Around line 297-343: sealMnemonic and unsealMnemonic currently discard
underlying errors and return generic messages; update all failure paths in
sealMnemonic and unsealMnemonic to log the original error using the logger (e.g.
lightning.log.WithError(err).WithField("account", accountCode or
account.Code).Error("...")) before mapping to the user-facing errp.New(...) so
callers get the same generic message while the real cause is recorded; follow
the project's structured logging pattern (logging.Get().WithGroup()/WithField())
and include contextual fields (account code, operation) for each error source
(io.ReadFull, encryptMnemonic/decryptMnemonic, StoreLightningEncryptionKey,
LoadLightningEncryptionKey, base64 decode) prior to returning the mapped error.
In
`@frontends/android/BitBoxApp/app/src/main/java/ch/shiftcrypto/bitboxapp/LightningEncryptionHelper.java`:
- Around line 81-84: Introduce a named constant for the GCM IV length and
replace hard-coded 12 values: add a private static final int GCM_IV_LENGTH_BYTES
= 12 to LightningEncryptionHelper and use GCM_IV_LENGTH_BYTES in both the method
that prepends the IV (storeKey or equivalent) and in loadKey where the IV is
sliced and the ciphertext length computed (currently using new byte[12] and
iv.length assumptions). Update all references (IV allocation, System.arraycopy
offsets/lengths) to use GCM_IV_LENGTH_BYTES so the format coupling is explicit
and maintainable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f102aa11-650c-48fe-ab38-3793974cf7fe
📒 Files selected for processing (13)
backend/backend.gobackend/backend_test.gobackend/bridgecommon/bridgecommon.gobackend/bridgecommon/bridgecommon_test.gobackend/handlers/handlers_test.gobackend/lightning/lightning.gobackend/lightning/lightning_test.gobackend/mobileserver/mobileserver.gocmd/servewallet/main.gofrontends/android/BitBoxApp/app/src/main/java/ch/shiftcrypto/bitboxapp/GoViewModel.javafrontends/android/BitBoxApp/app/src/main/java/ch/shiftcrypto/bitboxapp/LightningEncryptionHelper.javafrontends/ios/BitBoxApp/BitBoxApp/BitBoxAppApp.swiftfrontends/qt/server/server.go
| // StoreLightningEncryptionKey implements backend.Environment. | ||
| func (env *BackendEnvironment) StoreLightningEncryptionKey(accountCode string, encryptionKey string) error { | ||
| if env.StoreLightningEncryptionKeyFunc != nil { | ||
| return env.StoreLightningEncryptionKeyFunc(accountCode, encryptionKey) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // LoadLightningEncryptionKey implements backend.Environment. | ||
| func (env *BackendEnvironment) LoadLightningEncryptionKey(accountCode string) (string, error) { | ||
| if env.LoadLightningEncryptionKeyFunc != nil { | ||
| return env.LoadLightningEncryptionKeyFunc(accountCode) | ||
| } | ||
| return "", nil | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Silent defaults on data-critical store/load could mask misconfiguration.
If a future caller sets CanEncryptLightningMnemonicFunc to return true but forgets to wire StoreLightningEncryptionKeyFunc or LoadLightningEncryptionKeyFunc, Lightning.sealMnemonic will think storage succeeded and Lightning.unsealMnemonic will receive an empty key (then fail base64-decode or AES-GCM open with the generic "reactivation required" message). This is consistent with how the other delegates degrade, but those are UX-only; here the consequence is users locked out of their Lightning seed. Consider returning an error when the func is missing so the misconfiguration surfaces immediately during activation rather than later at unlock time.
🛡️ Proposed defensive fix
// StoreLightningEncryptionKey implements backend.Environment.
func (env *BackendEnvironment) StoreLightningEncryptionKey(accountCode string, encryptionKey string) error {
if env.StoreLightningEncryptionKeyFunc != nil {
return env.StoreLightningEncryptionKeyFunc(accountCode, encryptionKey)
}
- return nil
+ return errp.New("StoreLightningEncryptionKey not implemented in this environment")
}
// LoadLightningEncryptionKey implements backend.Environment.
func (env *BackendEnvironment) LoadLightningEncryptionKey(accountCode string) (string, error) {
if env.LoadLightningEncryptionKeyFunc != nil {
return env.LoadLightningEncryptionKeyFunc(accountCode)
}
- return "", nil
+ return "", errp.New("LoadLightningEncryptionKey not implemented in this environment")
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/bridgecommon/bridgecommon.go` around lines 280 - 294, The current
BackendEnvironment methods StoreLightningEncryptionKey and
LoadLightningEncryptionKey silently succeed/return empty when their delegates
are nil, which can hide misconfiguration; change them to return a clear error
when StoreLightningEncryptionKeyFunc or LoadLightningEncryptionKeyFunc is nil so
callers (e.g., Lightning.sealMnemonic/Lightning.unsealMnemonic) fail fast.
Specifically, in BackendEnvironment.StoreLightningEncryptionKey return a non-nil
error if StoreLightningEncryptionKeyFunc == nil (rather than nil), and in
BackendEnvironment.LoadLightningEncryptionKey return ("", error) if
LoadLightningEncryptionKeyFunc == nil; reference the methods
StoreLightningEncryptionKey, LoadLightningEncryptionKey and the
CanEncryptLightningMnemonicFunc contract to ensure activation-time
misconfiguration surfaces immediately.
There was a problem hiding this comment.
We need this to allow using the seed without encryption in webdev mode. Lightning activation will be hidden in the UI for all the other platforms that don't support encryption in a future PR. I'll keep it as is for now.
There was a problem hiding this comment.
@Beerosagos, understood — since CanEncryptLightningMnemonic() returns false for webdev (and other non-supporting platforms), the Store/Load paths won't be reached during normal flow, and the future UI guard will provide an additional layer of protection. That makes the silent defaults a reasonable design choice here. Thanks for the context!
✏️ Learnings added
Learnt from: Beerosagos
Repo: BitBoxSwiss/bitbox-wallet-app PR: 4146
File: backend/bridgecommon/bridgecommon.go:280-294
Timestamp: 2026-05-14T09:40:51.331Z
Learning: In `backend/bridgecommon/bridgecommon.go`, the `BackendEnvironment` methods `StoreLightningEncryptionKey` and `LoadLightningEncryptionKey` intentionally return `nil`/`("", nil)` when their delegate funcs are not set. This is by design to support webdev mode (and other non-encrypting platforms) where `CanEncryptLightningMnemonic()` returns `false`, gating all encryption code paths. A future PR will hide the Lightning activation UI on platforms that do not support mnemonic encryption, adding another layer of protection.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Beerosagos
force-pushed
the
seed-encryption
branch
from
71f017f to
853b097
Compare
|
|
||
| if lightning.environment.CanEncryptLightningMnemonic() { | ||
| if err := lightning.environment.DeleteLightningEncryptionKey(string(account.Code)); err != nil { | ||
| lightning.log.WithError(err).Warn("Error deleting lightning encryption key") |
There was a problem hiding this comment.
Why not return the error? Seems like a serious enough error.
There was a problem hiding this comment.
Well, even if the encryption key cannot be deleted for any reason, the deactivation basically completes anyway. And I don't think it should bring any issue on a following new activation, unless the Android keystore is completely broken. So I think that returning an error may add more confusion than value
| return context.getSharedPreferences(PREFS_NAME, Context.MODE_PRIVATE); | ||
| } | ||
|
|
||
| private static SecretKey getOrCreateWrappingKey(String accountCode) throws Exception { |
There was a problem hiding this comment.
Codex:
Restore path can strand Lightning accounts.
frontends/android/BitBoxApp/app/src/main/AndroidManifest.xml:32 enables backup, and Android backup includes app data/shared prefs by default. If lightning.json and lightning-encryption.xml restore without
the AndroidKeyStore entry, loadKey() still calls getOrCreateWrappingKey() at frontends/android/BitBoxApp/app/src/main/java/ch/shiftcrypto/bitboxapp/LightningEncryptionHelper.java:82, creating a fresh wrong
key instead of reporting “missing key”. Decryption then fails generically. I’d split create/load paths: load should error if the alias is absent, and backup rules should likely exclude the Lightning prefs
and/or lightning.json.
There was a problem hiding this comment.
Great catch! Should be fixed now 🙏
Add platform hooks for storing a Lightning seed encryption key outside the backend config and use them on Android via the keystore. Activation now derives the mnemonic as before, generates a random 32-byte key in the backend, encrypts the mnemonic with AES-GCM, stores only the ciphertext in the Lightning config, and hands the base64-encoded key to the platform environment. On Android, the environment wraps that key with an account-specific AES key from AndroidKeyStore and stores the wrapped blob in SharedPreferences. Unlocking loads and unwraps the stored key, decrypts the mnemonic in the backend, and then connects Breez. Deactivation removes both the wrapped key and the encrypted mnemonic. Android intentionally treats missing wrapped keys as requiring Lightning reactivation. This can affect existing dev wallets created before this change, but the feature has no production users yet and the migration cost is acceptable at this stage. Other platforms keep returning false or stub implementations for now. This is intentional preparatory work so iOS, Qt, and web can adopt secure storage later without another backend API change.
Beerosagos
force-pushed
the
seed-encryption
branch
from
853b097 to
8d1d453
Compare
|
@benma PTAL 🙏 |
2 participants
Add platform hooks for storing a Lightning seed encryption key outside the backend config and use them on Android via the keystore.
Activation now derives the mnemonic as before, generates a random 32-byte key in the backend, encrypts the mnemonic with AES-GCM, stores only the ciphertext in the Lightning config, and hands the base64-encoded key to the platform environment.
On Android, the environment wraps that key with an account-specific AES key from AndroidKeyStore and stores the wrapped blob in SharedPreferences. Unlocking loads and unwraps the stored key, decrypts the mnemonic in the backend, and then connects Breez. Deactivation removes both the wrapped key and the encrypted mnemonic.
Android intentionally treats missing wrapped keys as requiring Lightning reactivation. This can affect existing dev wallets created before this change, but the feature has no production users yet and the migration cost is acceptable at this stage.
Other platforms keep returning false or stub implementations for now. This is intentional preparatory work so iOS, Qt, and web can adopt secure storage later without another backend API change.
Before asking for reviews, here is a check list of the most common things you might need to consider: