Repository navigation
feat: keep platform tokens in the OS keyring only - #16
Merged
Merged
Conversation
The tokens were saved twice: in plain text in the user_profile table and in the keyring, which keyring 3 without features only kept in memory. Saves now write each secret to the OS keyring, check it by reading it back, and leave its database column empty. On start, secrets still in the database are moved to the keyring and their column is cleared once the keyring returns the same value. When no keyring is usable, the secrets stay in the database as before, the failure is logged, the Profile page says the tokens are stored unencrypted, and nothing is deleted from a keyring that could not be read. secure_delete keeps a cleared token from lingering in the file.
They travelled in the token URL's query string, where logs and proxies keep them, and the refresh token was not even URL-encoded. auth.gog.com/token reads the same parameters from an application/x-www-form-urlencoded POST body: with dummy values it gives the same invalid_grant answers as GET, invalid_client for a wrong secret and invalid_request when grant_type is missing.
Nothing called it; it only compiled behind allow(dead_code).
A locked store can report an existing secret as missing (KeePassXC returns no search results for a locked database, which the Secret Service store maps to NoEntry). The secret then loaded as empty, and once the store was unlocked any profile save deleted it, with its database column already cleared. An empty secret now only deletes a keyring entry this session read or stored.
Every keystroke in a token field saves the profile on its own thread. Two saves of the same secret could interleave, and one's read-back then saw the other's value, which switched the session to storing all tokens unencrypted in the database. Saves now take a lock for their whole duration.
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
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.
What changed
1. Platform tokens live in the OS keyring only
Until now every token was saved twice: in plain text in the
user_profiletable, and in the keyring. Withkeyring = "3"and no features that keyring was the in-memory mock, so nothing reached it. Since keyring 4 (#11) the keyring really stores them, but the plaintext copy stayed, so it protected nothing.src/keyring.rsnow owns the profile's secrets (steam_api_key,gog_token,gog_refresh_token,xbox_api_key,psn_npsso,epic_token,epic_refresh_token):keyring::save_profile): each secret is written to the keyring and read back. Only when the keyring returns the same value is its database column written as an empty string. An emptied secret deletes the keyring entry, but only one this session read or stored: a locked store can report an existing secret as missing (KeePassXC returns no search results for a locked database, which the Secret Service store maps toNoEntry), and the empty value loaded then must not delete the real secret on a later save. Saves take a lock for their whole duration, so two saves running at once (every keystroke in a token field saves the profile on its own thread) never mistake each other's read-back for a broken keyring.keyring::load_profile): empty columns are filled from the keyring. A non-empty column (every existing user, or a session without a keyring) is the newest value, so it is moved into the keyring through the same save path, and its column is cleared only after the read-back matches.PRAGMA secure_delete = ONon every connection: without it, the cleared tokens stay readable in the file's free space. Checked locally: after clearing seven 327-byte tokens, all seven were still inspotter.dbwithout the pragma, none with it.keyring_core::Entry(keyring 4's own core, added as a direct dependency, already in the tree) and selects the platform store once at startup throughkeyring::Entry::store_status(). That is what lets the test install keyring-core's mock store: keyring's ownEntry::newalways replaces the default store with the platform one.set_secret) rather thanset_password, which Windows stores as UTF-16 and so halves Credential Manager's 2560-byte limit; Epic'seg1tokens are signed JWTs. No release ever wrote real keyring entries (0.2.1 had the mock), so nothing needs converting.db.rscomments, README (Data Storage, Project Structure, and the Privacy policy under Code signing policy),docs/DATA_MODEL.mdandenv.examplenow say where the tokens are and when they are unencrypted.2. GOG login code and refresh token no longer travel in the URL
src/gog/auth.rscalledGET https://auth.gog.com/token?...&refresh_token=...(the refresh token not even URL-encoded) and the same for the login code. Both now go through onepost_tokenhelper that sends them as anapplication/x-www-form-urlencodedPOST body.Evidence that the endpoint accepts this:
The established open source clients all use GET with query parameters, so they neither prove nor disprove POST: minigalaxy, lgogdownloader, heroic-gogdl, comet. Lutris also uses GET and redacts
refresh_tokenandcodefrom its logs, which is the leak this change avoids.Other clients refresh with a form-encoded POST body: Clarity and Steam-App-ID-Finder (both grants). GOG publishes no documentation for this endpoint.
Direct check against
auth.gog.com/tokenwith dummy values (the public Galaxy client id and secret already in the code, fake code and refresh token), POST with a form body:400 invalid_grant "Invalid refresh token, client_id:46899977096215655", identical to the GET answer400 invalid_grant "Code doesn't exist or is invalid for the client", identical to GET400 invalid_clientgrant_typeleft out:400 invalid_request "Invalid grant_type parameter or parameter missing", and"code" is requiredwhen only the code is left outSo the server reads the client credentials, the grant type, the code and the refresh token from the body. The same answers came back through ureq's
send_form, the call Spotter now makes.3. Dead code
epic::auth::verify_tokenwas unused (only kept alive by#[allow(dead_code)]) and is removed. The unusedkeyring::deleteandCredentialKeywent away with the rewrite.Proven locally
cargo fmt --check,cargo clippy --all-targets -- -D warnings, andcargo test(lib 1, bin 34, integration 53, unit 58 tests, all passing) on Linux.cargo clippy --all-targets --target x86_64-pc-windows-msvc -- -D warnings(clang-cl + xwin) passes.keyring::tests::secrets_move_to_the_keyring_or_stay_in_the_database(mock store) covers: no keyring (secrets stay in the database), migration of legacy columns into the keyring with the columns cleared, save and delete through the keyring only, a keyring read failure (nothing deleted, later saves fall back to the database), a locked keyring that answersNoEntryat load (the secret survives a later save), and 8 threads saving 200 times each at once (the session stays on the keyring). Mutation checks: removing the session guard, the column clearing, the migration or the seen-secrets guard each makes it fail; removing the save lock made it fail in 10 of 10 runs, and it passed 20 of 20 runs with the lock.keyring::init(), which only the app's startup calls.Only CI, a release or a real login can prove
cargo auditis not installed locally; CI runs it.keyring-corewas already in the dependency tree.