Skip to main content

max / synckit

Run the rotation suite against the keychain the client ships with The rotation integration tests were gated `cfg(not(feature = "keychain"))`, so the five of them ran only under `--no-default-features`. In that config `keystore::store_key` is the no-op stub and `cache_key` cannot return false, which left the whole failure branch of rotate_key's step 6 unexecuted by anything: the stale-key-drop that a cold launch depends on. Install keyring-core's in-memory mock as the process default store instead. `keystore::entry` already prefers an installed store over the platform one, so the shipping code path runs with no secret-service daemon, and every integration client now gets a hermetic keychain rather than the developer's login keyring. Two tests pin the guard from both sides, since dropping the negation would satisfy either alone: a rotation whose cache write fails must clear the entry holding the pre-rotation key, and one whose write succeeds must leave the new key in place.
Co-Authored-By
Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session
https://claude.ai/code/session_01MptwXZ8k65v19rFmdGAyki
Author: Max Johnson <me@maxj.phd> · 2026-08-31 22:28 UTC
Signed with PGP, not checked
Commit: 68955ab00102b64d0563e36de60e33123fffeb2a
Parent: d85f755
4 files changed, +196 insertions, -13 deletions
M synckit-client/tests/integration/common.rs +29
@@ -81,3 +81,32 @@
81 81 "created_at": "2025-01-01T00:00:00Z",
82 82 })
83 83 }
84 +
85 + /// Install the `keyring_core` in-memory mock as the process-global keychain
86 + /// store, once.
87 + ///
88 + /// `rotate_key` finishes by caching the new master key through
89 + /// `keystore::store_key`, which under the `keychain` feature reaches the host's
90 + /// real secret service. That is unavailable on a headless box, and writing a
91 + /// test key into a developer's login keyring is not wanted either. The mock is a
92 + /// store like any other and `keystore::entry` prefers an already-installed one
93 + /// over the platform default, so installing it here puts the shipping keychain
94 + /// code path under test with no daemon and no host state.
95 + ///
96 + /// It is process-global and installed for the life of the binary, so a test that
97 + /// asserts on keychain contents must use its own `(app_id, user_id)` pair: the
98 + /// suite's shared [`test_ids`] entry is written by every rotation test that runs.
99 + #[cfg(feature = "keychain")]
100 + pub(crate) fn ensure_mock_keystore() {
101 + static STORE: std::sync::Once = std::sync::Once::new();
102 + STORE.call_once(|| {
103 + keyring_core::set_default_store(
104 + keyring_core::mock::Store::new().expect("the mock keychain store"),
105 + );
106 + });
107 + }
108 +
109 + /// No store to install: without the `keychain` feature `keystore::store_key` is
110 + /// the no-op stub, so the same orchestration runs with the cache write neutered.
111 + #[cfg(not(feature = "keychain"))]
112 + pub(crate) fn ensure_mock_keystore() {}
M synckit-client/tests/integration/main.rs +6 -6
@@ -20,12 +20,12 @@
20 20 mod group_rotation;
21 21 mod groups;
22 22 mod ota;
23 - // `rotate_key` finishes by caching the new key through `keystore::store_key`,
24 - // which hits the OS secret service under `keychain` and is unavailable on a
25 - // headless host. With the feature off it is the no-op stub, so the orchestration
26 - // runs hermetically. Run with:
27 - // cargo test --no-default-features --features store,testing
28 - #[cfg(not(feature = "keychain"))]
23 + // `rotate_key` finishes by caching the new key through `keystore::store_key`.
24 + // Under `keychain` that would reach the OS secret service, so the harness
25 + // installs `keyring_core`'s in-memory mock as the process default store
26 + // (`common::ensure_mock_keystore`) and the shipping code path runs hermetically.
27 + // Runs in both feature configurations; the two tests that assert on keychain
28 + // contents are gated to the one where there is a keychain to assert on.
29 29 mod rotation;
30 30 mod subscribe;
31 31 mod subscription;
M synckit-client/tests/integration/mockkit.rs +3 -1
@@ -25,7 +25,7 @@
25 25
26 26 use synckit_client::{SyncKitClient, SyncKitConfig};
27 27
28 - use crate::common::{ensure_crypto_provider, fresh_token, test_ids};
28 + use crate::common::{ensure_crypto_provider, ensure_mock_keystore, fresh_token, test_ids};
29 29
30 30 /// A wiremock server plus the wiring around it.
31 31 ///
@@ -80,6 +80,7 @@
80 80 /// No session: the client a test uses to assert `NotAuthenticated`.
81 81 pub(crate) fn client(&self) -> SyncKitClient {
82 82 ensure_crypto_provider();
83 + ensure_mock_keystore();
83 84 SyncKitClient::new(self.config())
84 85 }
85 86
@@ -112,6 +113,7 @@
112 113 /// set after the fact.
113 114 pub(crate) fn authed_with_http(&self, http: reqwest::Client) -> SyncKitClient {
114 115 ensure_crypto_provider();
116 + ensure_mock_keystore();
115 117 let client = SyncKitClient::with_http_client(self.config(), http);
116 118 let (user_id, app_id) = test_ids();
117 119 client.restore_session(&fresh_token(), user_id, app_id);
M synckit-client/tests/integration/rotation.rs +158 -6
@@ -4,12 +4,15 @@
4 4 //
5 5 // These drive `rotate_key()` through the full server protocol against wiremock:
6 6 // fetch key -> begin -> re-encrypt loop -> complete, plus the straggler retry on
7 - // a 409. They are gated `#[cfg(not(feature = "keychain"))]` because `rotate_key`
8 - // finishes by caching the new key with `keystore::store_key`, which hits the OS
9 - // secret-service under the `keychain` feature and is unavailable on a headless
10 - // test host. With keychain off, `store_key` is the no-op stub, so the test
11 - // exercises the orchestration hermetically. Run with:
12 - // cargo test --no-default-features --features store,testing
7 + // a 409, and finally what it does with the OS keychain.
8 + //
9 + // They run in both feature configurations. `rotate_key` finishes by caching the
10 + // new key with `keystore::store_key`, which under `keychain` would hit the OS
11 + // secret service; `common::ensure_mock_keystore` installs `keyring_core`'s
12 + // in-memory mock as the process default store instead, so the shipping path runs
13 + // with no daemon. Without the feature `store_key` is the no-op stub and the
14 + // orchestration still runs, minus the keychain interaction; the two tests that
15 + // assert on keychain contents are gated to the config that has one.
13 16 use crate::common::*;
14 17
15 18 const KEYS_PATH: &str = "/api/v1/sync/keys";
@@ -559,6 +562,7 @@
559 562 use std::sync::atomic::{AtomicUsize, Ordering};
560 563
561 564 ensure_crypto_provider();
565 + ensure_mock_keystore();
562 566 let old_key = synckit_client::crypto::generate_master_key();
563 567
564 568 // Recording off: see `Counted`.
@@ -638,3 +642,151 @@
638 642 "one initial re-encrypt pass plus one per straggler round short of the cap"
639 643 );
640 644 }
645 +
646 + // ── What rotation leaves in the OS keychain ──
647 + //
648 + // `rotate_key` step 6 caches the new master key and, if that write fails, drops
649 + // the entry rather than leaving the pre-rotation key in it. Both halves of that
650 + // guard were unreachable before these tests existed: the suite ran only with
651 + // `keychain` off, where `cache_key` cannot report false and `delete_key` is a
652 + // no-op stub, so the failure branch had never been executed by anything.
653 + //
654 + // The two tests below pin the guard from both sides, which is what it takes:
655 + // dropping the negation would satisfy either one alone.
656 +
657 + /// Address the entry `keystore` writes for this session.
658 + ///
659 + /// `keystore::entry` is private, so its naming (`synckit:<app_id>` as the
660 + /// service, the user id as the user) is restated here. Anything else addresses a
661 + /// different credential and the assertions would pass vacuously.
662 + #[cfg(feature = "keychain")]
663 + fn keychain_entry(app_id: AppId, user_id: UserId) -> keyring_core::Entry {
664 + keyring_core::Entry::new(&format!("synckit:{app_id}"), &user_id.to_string())
665 + .expect("the mock store builds an entry")
666 + }
667 +
668 + /// A client with a keychain identity of its own.
669 + ///
670 + /// The mock store is process-global and its credentials are keyed on
671 + /// (service, user), so a test asserting on keychain contents cannot share
672 + /// `common::test_ids()` with every other rotation test in the binary.
673 + #[cfg(feature = "keychain")]
674 + fn client_with_own_keychain(kit: &MockKit, n: u128) -> (SyncKitClient, AppId, UserId) {
675 + let app_id = AppId::new(Uuid::from_u128(n));
676 + let user_id = UserId::new(Uuid::from_u128(n + 1000));
677 + let client = kit.client();
678 + client.restore_session(&fresh_token(), user_id, app_id);
679 + (client, app_id, user_id)
680 + }
681 +
682 + /// Mount the whole happy-path rotation protocol, resuming onto `new_key` so the
683 + /// caller knows the key the client will end up holding.
684 + #[cfg(feature = "keychain")]
685 + async fn mount_resumed_rotation(kit: &MockKit, old_key: &[u8; 32], new_key: &[u8; 32]) {
686 + kit.get(KEYS_PATH)
687 + .json(json!({
688 + "encrypted_key": synckit_client::crypto::wrap_master_key(old_key, ROTATE_PW).unwrap(),
689 + "key_version": 1,
690 + "key_id": 1,
691 + "pending_key": {
692 + "encrypted_key": synckit_client::crypto::wrap_master_key(new_key, ROTATE_PW).unwrap(),
693 + "key_id": 2,
694 + },
695 + }))
696 + .await;
697 + kit.post(ROTATE_PATH).json(begin_body(0)).await;
698 + kit.post(ENTRIES_PATH)
699 + .json(json!({ "entries": [], "has_more": false }))
700 + .await;
701 + kit.post(COMPLETE_PATH).empty().await;
702 + }
703 +
704 + /// A rotation whose cache write fails must clear the keychain entry, because
705 + /// what is in it is the *pre-rotation* key.
706 + ///
707 + /// This is the case the guard at `rotation.rs` step 6 exists for, and it is
708 + /// worse than an empty cache: a cold launch that loaded the stale entry would
709 + /// decrypt nothing at all, with no password prompt to recover through. Deleting
710 + /// it makes the next launch fall through to the password path.
711 + ///
712 + /// The mock clears its armed error after one call, so the sequence the client
713 + /// actually walks is the real one: `store_key` fails, `cache_key` reports false,
714 + /// and the `delete_key` that follows succeeds.
715 + #[cfg(feature = "keychain")]
716 + #[tokio::test]
717 + async fn a_rotation_that_cannot_cache_the_new_key_drops_the_stale_one() {
718 + let kit = MockKit::start().await;
719 + let old_key = synckit_client::crypto::generate_master_key();
720 + let new_key = synckit_client::crypto::generate_master_key();
721 + mount_resumed_rotation(&kit, &old_key, &new_key).await;
722 +
723 + let (client, app_id, user_id) = client_with_own_keychain(&kit, 9_001);
724 + synckit_client::keystore::store_key(app_id, user_id, &old_key)
725 + .expect("seed the keychain with the pre-rotation key");
726 +
727 + // Arm the next write to fail, which is `cache_key` returning false.
728 + let entry = keychain_entry(app_id, user_id);
729 + let cred: &keyring_core::mock::Cred = entry
730 + .as_any()
731 + .downcast_ref()
732 + .expect("the mock store yields mock credentials");
733 + cred.set_error(keyring_core::Error::NoStorageAccess(Box::new(
734 + std::io::Error::other("keychain is locked"),
735 + )));
736 +
737 + client
738 + .rotate_key(DeviceId::new(Uuid::new_v4()), ROTATE_PW)
739 + .await
740 + .expect("a failed cache write must not fail the rotation: the key itself is fine");
741 +
742 + match keychain_entry(app_id, user_id).get_password() {
743 + Err(keyring_core::Error::NoEntry) => {}
744 + Ok(held) => {
745 + let stale = base64::engine::general_purpose::STANDARD.encode(old_key);
746 + assert_ne!(
747 + held, stale,
748 + "the keychain still holds the PRE-ROTATION key: a cold launch would load it and decrypt nothing"
749 + );
750 + panic!("the stale entry was not dropped; it holds an unexpected value instead");
751 + }
752 + Err(e) => panic!("unexpected keychain error: {e}"),
753 + }
754 + }
755 +
756 + /// A rotation whose cache write succeeds must leave the NEW key in the
757 + /// keychain, and must not delete what it just wrote.
758 + ///
759 + /// The other side of the same guard. Without this, dropping the negation on the
760 + /// `cache_key` check would wipe the entry on every successful rotation, which is
761 + /// the very failure the sibling test's comment describes, arrived at from the
762 + /// opposite direction.
763 + #[cfg(feature = "keychain")]
764 + #[tokio::test]
765 + async fn a_rotation_that_caches_the_new_key_keeps_it() {
766 + let kit = MockKit::start().await;
767 + let old_key = synckit_client::crypto::generate_master_key();
768 + let new_key = synckit_client::crypto::generate_master_key();
769 + assert_ne!(
770 + old_key, new_key,
771 + "the two keys must differ or the assertion below proves nothing"
772 + );
773 + mount_resumed_rotation(&kit, &old_key, &new_key).await;
774 +
775 + let (client, app_id, user_id) = client_with_own_keychain(&kit, 9_002);
776 + synckit_client::keystore::store_key(app_id, user_id, &old_key)
777 + .expect("seed the keychain with the pre-rotation key");
778 +
779 + client
780 + .rotate_key(DeviceId::new(Uuid::new_v4()), ROTATE_PW)
781 + .await
782 + .expect("the rotation should complete");
783 +
784 + let held = keychain_entry(app_id, user_id)
785 + .get_password()
786 + .expect("the new key must still be cached after a successful rotation");
787 + assert_eq!(
788 + held,
789 + base64::engine::general_purpose::STANDARD.encode(new_key),
790 + "the keychain does not hold the post-rotation key"
791 + );
792 + }