fix: encrypt stored server credentials at rest - #917
Open
sinful1992 wants to merge 3 commits into
Open
Conversation
Server credentials were persisted in plaintext in two places: the default SharedPreferences (password/token/salt) and the Room `server` table's password column. Anything able to read app storage (rooted device, backup extraction) could recover the user's Navidrome/Subsonic password. Add a Keystore-backed CryptoUtil (AES/GCM) and route both stores through it: - Preferences password/token/salt are encrypted on write and decrypted on read. A Keystore-wrapped key is used instead of the now-deprecated androidx.security EncryptedSharedPreferences, keeping the key in secure hardware where available and avoiding a deprecated dependency. - ServerRepository.insert() is the single choke point that encrypts the password before it reaches the database; LoginFragment decrypts at the one read site. Reads tolerate values written before this change (returned as-is), and a one-time, idempotent migration on startup re-encrypts existing plaintext in both stores so the raw values no longer sit on disk. Decryption failure (e.g. an invalidated Keystore key) returns null so the app prompts a re-login rather than crashing. Storage layout is unchanged (same columns, same keys, password still kept): credential-lifecycle changes belong to the Login milestone (eddyizm#829). Closes eddyizm#911
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
encryptStoredPasswords runs on every startup after the update; if the Keystore key is unavailable, CryptoUtil.encrypt returns null and feeding that into the non-null Server.password column (or overwriting a working preference value) would crash on the background thread or drop the credential. Fall back to leaving the existing value untouched — reads already tolerate plaintext — so a transient Keystore failure can't turn into a launch crash or a lost login.
Contributor
Author
Owner
|
@tvillega is this superceded by your login activity update? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #911 (split out from #858).
Problem
Server credentials are persisted in plaintext in two places:
SharedPreferences—password,token,salt(util/Preferences.kt).servertablepasswordcolumn (ServerSignupDialog→ServerRepository.insert), which survives even in secure/token mode becauseLoginFragment.saveServerPreferencecopies the raw password across.Anything able to read app storage (rooted device, backup extraction) recovers the user's Navidrome/Subsonic password.
Change
Encrypt at rest without restructuring the storage model:
util/CryptoUtil.kt— a Keystore-backed AES/GCM helper. Credentials are encrypted on write, decrypted on read.Preferencespassword/token/saltgo throughCryptoUtil.ServerRepository.insert()is the single choke point that encrypts the DB password;LoginFragmentdecrypts at the one read site.Why Keystore and not
EncryptedSharedPreferencesThe issue suggested
androidx.securityEncryptedSharedPreferencesor a Keystore-wrapped key. I went with the Keystore key: the androidx Security-Crypto library is deprecated by Google, and a small self-contained helper covers both the prefs values and the Room column with one mechanism and no new dependency. The key stays in secure hardware where available (min SDK 24).Migration & safety
null, so the app prompts a fresh login instead of crashing.Deliberately out of scope
The storage layout is unchanged — same columns, same pref keys, password still kept. Dropping the raw password / adding per-server token+salt columns is a credential-lifecycle change that belongs to the Login milestone (#829), where
LoginActivitywill own persistence; doing it here would mean migrating storage twice.Milestone placement
Per the #858 thread, this fits under #829 — encryption is best done at the same time
LoginActivitytakes over credential persistence. Flagging for @eddyizm / Tom to slot as they see fit.Testing
Not build-verified locally (I don't have the signing/build env set up here) — CI/maintainer build appreciated. Logic is a straight encrypt-on-write / decrypt-on-read with legacy-tolerant reads; happy to adjust.