Sync Exchange v2 algorithm encryption foundation work#8683
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
CDRussell
force-pushed
the
feature/craig/sync_3p_browser_chat_sync_foundation
branch
5 times, most recently
from
fddeb98 to
cfc8a4d
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit cfc8a4d. Configure here.
CDRussell
force-pushed
the
feature/craig/sync_3p_browser_chat_sync_foundation
branch
from
cfc8a4d to
31d26c9
Compare
cmonfortep
left a comment
cmonfortep
left a comment
There was a problem hiding this comment.
Testing happy path works, and approving as this is first version and nothing blocking.
There are a few things we need to discuss and decide next steps, so they might be easier on follow up PRs. Not specific suggestions as they rely on specifications and here we are just doing foundation work:
- I'm interested on error handling and how to clear store. Right now we have a few new endpoints, they can error, or BE can return some specific errors. It's not clear how to react to each of them.
- I'm curious what's the plan after enabling the flag for accounts that already exist. They won't have credential Id, SP, etc.
|
|
||
| is Success -> { | ||
| syncStore.storeCredentials(account.userId, deviceId, deviceName, account.primaryKey, account.secretKey, result.data.token) | ||
| if (syncFeature.canUseV2ConnectFlow().isEnabled()) { |
There was a problem hiding this comment.
we should move this above storeCredentials or inside it. storeCrendentials will emit signed in and best if all the store has all the expected data before emit.
There was a problem hiding this comment.
Good spot; makes sense to re-order as you propose.
|
|
||
| syncStore.storeCredentials(userId, deviceId, deviceName, preLogin.primaryKey, decryptResult.decryptedData, result.data.token) | ||
|
|
||
| if (syncFeature.canUseV2ConnectFlow().isEnabled()) { |
There was a problem hiding this comment.
we should move this above storeCredentials or inside it. storeCrendentials will emit signed in and best if all the store has all the expected data before emit.
I think is better to compute values before, and passing new params to the method so we can do the write internally.
| val privateKeyBase64: String, | ||
| ) | ||
|
|
||
| @ContributesBinding(AppScope::class) |
There was a problem hiding this comment.
Add @SingleInstanceIn(AppScope::class) since this is stateless and it can be a singleton
| val builder = Jwts.builder() | ||
| if (kid != null) builder.header().keyId(kid).and() | ||
| return builder | ||
| .content(plaintext, JWE_CONTENT_TYPE_OCTET_STREAM) |
There was a problem hiding this comment.
This was raised by claude, posting here for review. Seems not harmful to keep, but checking if intentional (if so, we should guard with test)
jweEncryptSymmetric calls Jwts.builder()...content(plaintext, JWE_CONTENT_TYPE_OCTET_STREAM).... Per jjwt's API, the
two-argument .content(bytes, contentType) form sets the cty header. The TD reference envelope header in tdTest3 is:
{"alg":"dir","enc":"A256GCM","kid":"ddg"}
— no cty. Android's produced header is likely:
{"alg":"dir","enc":"A256GCM","kid":"ddg","cty":"application/octet-stream"}
This isn't a decryption problem — the protected-header bytes are AAD for AES-GCM, and the decryptor uses the
received bytes (not an expected shape). So FE decrypting an Android-produced envelope will compute the same AAD
Android wrote, and decryption succeeds, if the FE impl tolerates unknown headers (which RFC 7516 requires).
But: tdTest3_envelopeHeaderHasExpectedFields only asserts alg/enc/kid are present + segment count = 5. It doesn't
assert cty is absent, nor that the keyset is exactly those three. The "Android-produces an envelope FE can decrypt"
direction isn't unit-tested anywhere — only verified at integration level via the PR's manual test plan with a
3rd-party browser.
Two cheap improvements:
- Switch to single-argument content(bytes) so jjwt doesn't emit cty, making Android's header byte-for-byte identical
to the TD reference. - Tighten tdTest3_envelopeHeaderHasExpectedFields to assert the exact keyset (e.g.
assertEquals(setOf("alg","enc","kid"), header.keys().asSequence().toSet())) so any future header drift is caught.
There was a problem hiding this comment.
I think it shouldn't matter, but that dropping cty would be fine. as long as it works against the reference impl i'm good with it
| private const val JWE_CONTENT_TYPE_OCTET_STREAM = "application/octet-stream" | ||
|
|
||
| interface SyncJweCrypto { | ||
| fun generateRsaKeyPair(): RsaKeyPair |
There was a problem hiding this comment.
Add a @workerthread annotation on generateRsaKeyPair() (or the whole interface)
There was a problem hiding this comment.
👍 whole interface works for me
|
|
||
| val existing = when (val r = syncApi.getAccessCredentials(token)) { | ||
| is Success -> r.data.find { it.id == CREDENTIAL_ID_3PARTY } ?: run { | ||
| logcat { "Sync-ScopedToken: no 3party credential on server — nothing to refresh" } |
There was a problem hiding this comment.
If the server says "no 3party credential exists" but syncStore.scopedPassword is non-null locally, the local state is stale. A subsequent
getRecoveryCode() would emit a recovery code that no browser could use to sync with the account (the credential is gone server-side).
Probably should clear local SP on this branch:
syncStore.scopedPassword = null
return Success(false)
| companion object { | ||
| private val moshi = Moshi.Builder().build() | ||
| val recoveryCodeAdapter: JsonAdapter<LinkCode> = moshi.adapter(LinkCode::class.java) | ||
| val thirdPartyRecoveryCodeAdapter: JsonAdapter<ThirdPartyRecoveryCodeWrapper> = moshi.adapter(ThirdPartyRecoveryCodeWrapper::class.java) |
There was a problem hiding this comment.
This is unused as you have the lazy adapter in ThirdPartyCredentialManager
| return Success(base64UrlStringToStandardBase64(decryptedBase64Url)) | ||
| } | ||
|
|
||
| override fun refresh(): Result<Boolean> { |
There was a problem hiding this comment.
is refresh only expected to refresh scoped password? or also the keys? while this is documented, I'd consider adding it into the name. But I'm nor sure if SP and keys should be in sync, meaning they need to be refreshed at the same time.
| // ddg-side wrap: libsodium-secretbox(privateKey, secretKey), base64url-encoded. | ||
| // Matches duck.ai's EncryptWithSyncMasterKeyHandler so native clients read keys uniformly. | ||
| val privateKeyBytes = kotlin.runCatching { | ||
| Base64.decode(rsaKeyPair.privateKeyBase64, Base64.URL_SAFE or Base64.NO_WRAP or Base64.NO_PADDING) |
There was a problem hiding this comment.
I think will be best if we can reuse Base64.URL_SAFE or Base64.NO_WRAP or Base64.NO_PADDING and give it a name. Also to understand why sometimes we do Base64.NO_WRAP).applyUrlSafetyFromB64()
There was a problem hiding this comment.
Summarizing from our call for posterity:
depends what you're starting out with, whether you have a plain string needing encoded or already starting out with a base64 (not base64url) encoded string:
applyUrlSafetyFromB64was added for when we already receive a base64 encoded string that we want to take that one step further and convert to base64url encoding- if you're starting with the plain string rather than a base64-encoded one already, this should one-shot it:
Base64.URL_SAFE or Base64.NO_WRAP or Base64.NO_PADDING
| val encryptedSp = thirdParty?.encryptedCredential | ||
| if (encryptedSp != null) { | ||
| // Decrypt with the DDG MEK; convert wire base64url to standard base64 for storage. | ||
| val decryptedSp = kotlin.runCatching { |
There was a problem hiding this comment.
All the logic related to SP decrypt should live inside ThirdPartyCredentialManager, which is where you already have similar logic. we should expose a method for that.
Asked claude recommendation for this:
Better shape: have the manager expose applyLoginResponse(response, primaryKey, userId) that handles SP decrypt, keys filter, and (in the
proposed atomic-refactor world) returns a V2Credentials bundle. The repo then:
val v2 = thirdPartyCredentialManager.buildV2FromLoginResponse(result.data, preLogin.primaryKey, userId)
syncStore.storeCredentials(userId, deviceId, deviceName, preLogin.primaryKey, decryptResult.decryptedData, result.data.token, v2 = v2)
This consolidates the SP-decrypt logic, makes the manager the single source of truth, and naturally enables the atomic-write refactor.
There was a problem hiding this comment.
That sounds good to me.
2 participants
Task/Issue URL: https://app.asana.com/1/137249556945/project/1203822806345703/task/1215115673129510?focus=true
Description
Foundational encryption work for supporting duck chat syncing from 3rd party browsers.
The main goal of this PR is to add the building blocks we'll need for supporting the functionality in a way that:
Steps to test this PR
Ability to create protected keys and 3p access credentials
Tip
Logcat filter:
message~:"Sync-Scoped"Tip
There is a screen recording of the required setup provided as a reference in Video recording of setting up Android to sync with 3rd party browser using dev tools
Testing refusal when FF disabled
internalvariantCreate account; wait a few seconds and make sure account settings populateV2 connect flow flagssection, tap onFetch Access Credentialsand verify you seeid=ddg,scope=(unrestricted)as you don't currently have any 3rd party access credentialsFetch Keysand verify you seeNo keys foundas you don't have any created yetCreate Protected Key(leave purpose set to defaultai_chats) and verify this fails because the flag is disabled.Create 3party Credentialand verify it fails because the flag is disabledCreating protected key
canUseV2ConnectFlow (master kill switch)Create Protected Key(leave purpose set to defaultai_chats) and verify it succeeds, verifying that underFetch Keysyou see something withpurpose=ai_chats, encrypted_with=ddgCreating 3party credential
Refresh 3party Credential from Serverand verify there are noneCreate 3party Credentialand verify it's successfulSyncing chats with 3rd party browser using native's 3p recovery code
Note
You'll need a 3rd party browser to test this out alongside your Android device, e.g., Chrome on desktop.
Important
This builds on the above scenario so make sure that's been done
Android
Open Duck.ai Dev Screen->URL Settingsand enter duck.ai chat URL from Test Server details. Save and it'll restart some things.Open Duck.ai Setup URL. Authenticate with SSO. When prompted, chooseDEVELOPMENTserver.3rd party browser (e.g., Chrome desktop)
3rd Party Setupin Test Server details, again choosingDEVELOPMENTserver when promptedAndroid
Copy 3party Recovery Code3rd party browser (e.g., Chrome desktop)
Settings & More->Settings->Turn On Sync & BackupSync With Another DeviceAndroid
3rd party browser (e.g., Chrome desktop)
Tip
If all of the above is working, then you have established duck chat between Android and a 3rd party browser 🎉. That's largely it for this PR for now, other than ensuring nothing in prod is broken (see below)
Ensuring prod behavior is ok
Below is some of the main smoke testing of prod that is required, but isn't exhaustive. Any further smoke testing of typical prod sync pairing is helpful to ensure no regressions.
Note
You'll need two emulators/devices for this
internalvariants on two emulators/devicesSync and Back Up This DeviceSync With Another DeviceSync With Another DeviceNote
High Risk
Touches account signup/login, secret derivation, and new encrypted credential/key handling; mitigated by a default-off master flag but any mis-gating could affect sync auth or key material.
Overview
Adds v2 sync scoped-access plumbing behind
canUseV2ConnectFlow(default off): JWE/HKDF crypto (SyncJweCrypto, jjwt), new sync API models and remote calls for access credentials and protected keys, plusThirdPartyCredentialManagerandProtectedKeyManagerwired throughSyncAccountRepository.When the flag is on, signup/login send/store
credential_id=ddg, persist 3party SP and protected keys from login, and support creating/adopting the 3party credential, v2 3party recovery codes, and purpose-based protected RSA keys (e.g.ai_chats).SyncStoregainscredentialId,scopedPassword, andprotectedKeysJson.Sync internal dev settings expose toggles and buttons to exercise credentials, keys, scoped tokens, and Duck.ai setup—intended for internal testing, not production UI changes.
Reviewed by Cursor Bugbot for commit cfc8a4d. Configure here.