Page MenuHomePhabricator

Sign concatenation of public and notif keypairs using private key of public identity
ClosedPublic

Authored by marcin on Feb 21 2023, 10:43 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Mar 28, 5:24 PM
Unknown Object (File)
Thu, Mar 28, 5:24 PM
Unknown Object (File)
Thu, Mar 28, 5:24 PM
Unknown Object (File)
Thu, Mar 28, 5:24 PM
Unknown Object (File)
Thu, Mar 28, 5:24 PM
Unknown Object (File)
Thu, Mar 28, 5:24 PM
Unknown Object (File)
Thu, Mar 28, 5:16 PM
Unknown Object (File)
Mar 1 2024, 8:40 PM
Subscribers

Details

Summary

This differential implements creating as passing a signature of concatenated olm identity keys to JS.

Test Plan

Modify any place in JS where CommCoreModule.initializeCryptoAccount is called, by loggind ed25519Signature.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
848 ↗(On Diff #22875)

cc @ashoat, @atul
Here is the issue I was explicitly advised to concatenate all keys to generate signature: https://linear.app/comm/issue/ENG-2901/add-signed-notif-public-key-to-mysql-cookies-table#comment-c721bc40.
However after today's encryption sync discussion, I want to confirm, that we want to generate signature out of stringified JSON of a structure like this:

{
    notifCurve25519: xxxxx...,
    ed25519: xxxxx....,
}
native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
848 ↗(On Diff #22875)

Yeah, we should do a JSON blob. I responded on the Linear issue

Here's what the message/payload looks like on the web side:

{
  "primaryIdentityPublicKeys": {
    "ed25519": "bbuak18zkZDrvV/kpzYx02LOfNu/A8WN9OPdida0HXU",
    "curve25519": "LdX2vxtgffJod2I2voieKqEuTW5ZUpmP/XbTy5ksJns"
  },
  "notificationIdentityPublicKeys": {
    "ed25519": "7uiRAOqZe0HwzaG6/KXy8zDrT8713Anezvw8aGlAhXg",
    "curve25519": "kafUrgUra6qy4t3xv3AefAMad8241uklNzDXPCDzAgM"
  }
}

That stringified payload gets bundled with the signature into type SignedIdentityKeysBlob:

c940f5.png (226×816 px, 37 KB)

which looks something like this:

{
  "payload": "{\"primaryIdentityPublicKeys\":{\"ed25519\":\"Ok3f6Wypanf674HlCrL5KvWijFMakLTrSox61Nje6A0\",\"curve25519\":\"TygagUrYYw22ckucuEMCtQRfeANsHDoHPOpIvU9n4xk\"},\"notificationIdentityPublicKeys\":{\"ed25519\":\"vV4ATS5QT1qXnyMx/YiPU/Hncj8tgeNeRvWee9kQRTM\",\"curve25519\":\"78fhUKKAVrKwuIzRJpezbOxeFB4yytO0OE3f6t6HAhI\"}}",
  "signature": "pdvLQxeTPX8ATKjv8vp2K2C7RzfJX6IkEnk81KZVCNvX+2yu5qgouSIdXSnu1dNbany0+rh8nJZhDVbUMQvEAQ"
}

Sign stringified JSON with nested structure

This revision is now accepted and ready to land.Feb 27 2023, 7:25 AM
This comment has been deleted.
native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
843

This code has serious issues and should have never been landed, and is the reason behind ENG-3242, an Urgent issue where we shipped a crash-on-log-in to prod.

A basic issue is that we're proceeding through this block even if we failed to set primaryKeysResult and instead set error.

A second issue is that we aren't wrapping our calls to parseJson and aren't checking for errors. This wouldn't be a problem if it wasn't for the first issue, but it's always good practice to catch exceptions for parseJson. (This was initially introduced by @atul in D6115, and I should've called it out there.)

It's not clear to me why it works on iOS but doesn't on Android. Outside of the poor error-handling behavior as noted above, there is a third issue: why are we seeing errors on Android at all?

native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
843

A second issue is that we aren't wrapping our calls to parseJson and aren't checking for errors. This wouldn't be a problem if it wasn't for the first issue, but it's always good practice to catch exceptions for parseJson. (This was initially introduced by @atul in D6115, and I should've called it out there.)

I was passing the JSON blob from OLM's identity_keys() directly to folly::parseJson(...) which I trusted (and still trust) would be valid JSON as long as generateIdentityKeys completed successfully (didn't throw an error):

cca333.png (330×1 px, 61 KB)

The code has changed significantly since then.