[#483] resolve "Support sealed sender messages"
closes #483 (closed)
context
- as is thoroughly documented in #483 (closed), we need to support sealed sender messages in order to comply with Signal's spam-filtering rate limits, which interpret sealed messages as indicating consent to communicate and unsealed messages as likely spam
- this MR accomplishes this, while also adding metrics to allow us to monitor how many unsealed messages continue to be sent due to missing profile keys and a feature toggle to start blocking all unsealed messages if that number gets too high
qa notes:
- i verified that this flow was able to generate sealed cipher session for an outgoing messsage (to an admin on channel creation) and an incoming message (from a subscriber on channel subscription)
- however, all subsequent messages in each QA session generated the persistent
'invalid ciphertext message', had 0 receiver chainserror the task of which it is #478 to fix - as such, i will flag that we should circle back to to extra QA on sealed sender message handing during that card, and fix any defects in this MR as such QA is able to turn up
changes
NOTE: There is a lot of noise in this MR due to some refactors to extend SignalServiceSendResult that were necessary to support a toggle that would block all unsealed messages. We'll try to explain the core flow first, then circle back to the refactor!
data model
- in order to send sealed sender messages, we need to store profile keys for contacts, so we add a
ProfileStoreto do this. it is pretty straightforward. notably we:- introduce a new convention of calling the identifier (number or uuid) of another signal account a
contactIdrather thanname(as signald did). We plan to spread this convention to the protocol store in subsequent MRs. - we perform a check to ensure the profile key has a correct size before storing and log if it does not in order to prevent storing legacy keys. (note: we considered doing this in the
SignalReceiverlayer, but opted for here to avoid cluttering up#relaywith logic about valid keys that we didn't think it should need to know about)
- introduce a new convention of calling the identifier (number or uuid) of another signal account a
- we also modify the
profileKeyfield on ourAccountdata class to represent profile keys at rest as byte arrays rather thanProfileKeyobject to avoid unnecessary serialize/deserialize roundtrips on the critical "send path" that is our main perf bottleneck- in order to do this, we had to handroll
equalsandhashCodemethods for all account subclasses to satisfactorily compare theprofileKeyBytesfields for equality (since they are now arrays)
- in order to do this, we had to handroll
core flow
(1) storing profile keys in SignalReceiver
- whenever we receive a
DataMesssage(which might arrive in any envelope with content: a ciphertext, sealed-sender message, or prkey bundle), inspect it for a profile key, and it it has one, store it. checking on every incoming message allows us to detect profile key changes (which happen when a user blocks someone) - as a result of this, we can now handle sealed sender messages, so we include
UNIDENTIFIED_SENDERin the set of envelope types that we handle - as noted above: we push profile key validation down into the
ProfileKeyStorelayer (open to feedback on prefering something else!) - side note on switch from
SignalServiceAddresstoSignalcAddressin this class:- refactors in this card led to adopting a convention of always preferring signalc addresses (and send results) up until the call that crosses the boundary with libsignal code (to avoid confusion over where to use which representation -- if it's in our code, we use our representation. we only switch representations at the very last moment when we have to in order to call libsignal functions)
- as a result, you see a lot of removed calls to
asSignalcAddressbecause as a result of the convention shift noted above, we now simply return aSignalcAddressfrom theAccount#addressproperty getter, making such conversions unnecessary
(2) retrieving profile keys in SignalSender
- whenever we hit
#sendDataMesssage(for either a message or an expiry time update), we try to derive theUnidentifiedAccessPairnecessary to do so. (note: this "pair" is a pair of tuples containing a delivery token and a sender certificate for both the sender and recipient of a message, as documented in: )- to derive this pair of token/cert tuples, we call into a new
#getUnidentifiedAccessPairfunction inAccountManager, which:- tries to retrieve a profile key for the contact and returns null if it can't
- otherwise derives the access pair from the contact's profile key and the profile key for the sender account (which we created when creating the account during the registration process and stored in the
AccountStore)
- back in
SignalSender: if we can generate the needed key/cert material, we pass it along to#sendMessageand a sealed sender message is generated - if we cannot derive the necessary sealed-sender token/cert materials (indicated by a null
UnidentifiedAccessPairreturned from theAccountManager), we have a choice of what to do... - by default we will pass an
Optional.absent()value to#sendMessage, allowing an unsealed-sender message to be sent. (note: this is necessary b/c we currently are missing thousands of profile keys. to backfill them, we need to send a bunch of outgoing unsealed messages to people to ask them to respond so we can store their profile key) - since sending an excessive number of unsealed messages could lead to our service being blocked, we take the following precautions to prevent this:
- add a counter to track how many unsealed messages are being sent
- add a config toggle to allow us to start blocking all unsealed messages if this number gets too high. [1][2]
- add another counter to allow us to monitor how many sealed sender messages are being produced (but not sent) to see if it is safe to turn the toggle back off
- to derive this pair of token/cert tuples, we call into a new
[1] in order to "block" unsealed messages, we return early whenever we can't derive an UnsealedAccessPair for an outgoing message. In order to support returning early without resorting to making the return type of all of our SignalSender methods nullable (which is both messy in the first instance and has spillover effects in forcing handling code in SocketReceiver to handle nullable return types, we wanted to extend libsignal's SendMessageResult to be able to represent a Blocked message, and return that in case the toggle to block unsealed messages was off. We wound up doing that, which wound up being hard, and we write about it below!
[2] right now, the block toggle is derrived from a config set in the boot sequence by reading an env var, which means it requires a restart to change. we could improve upon this with a dynamic config. but i deferred any such work to #492
refactors
SignalcSendResult
- as noted above, returning early from
SignalSender#sendDataMessagein cases of block, necessitated creating a way to represent aBlockedvariant of libsignal'sSendMessageResult - to accomplish that, I created a
SignalcSendResultalgebraic data type, withBlockedas one of its variants and all of the 5 variants of libsignal'sSendMessageResultas its remaining variants - to convert from a
SendMessageResultto aSignalcSendResult, i borrowed the code that we used to use inSendResultTypeto convert from libsignal's representation into a simple enum class that we could use inwhenstatements - with this change, the enum itself was no longer necessary (since all the information that is needed to type switch is now contained in the type returned from the
sendfunction), which had a number of nice spillover simplifying effects:- the
SendResultTypecan be completely eliminated (as can all calls toSendMessageResult.type()to produce it) -
SocketReceivercan now switch directly off of theSignalcSendResultreturned fromSignalSender, without having to use an intermediary enum type that doesn't carry useful information (like a failed identity fingerprint, etc.) - it can also now pass such a result directly to
SocketSender, which can be cleanly converted directly to aSocketResponse.SendResultwithout dozens of lines of confusing conversion code (since we already did conversion once and for all when constructing ourSingalcSendResult), allowing us to delete all this confusing (and now unnecessary) code
- the
- in sum: by consolidating around one type that represents exactly what we want, we were able to elminate duplicate conversion code in both the intermediary enum type that we can now delete entirely and the serialization step, which now becomes trivially straightforward (as serialization code should be). this took up a lot of space in this MR (which was not about the refactor), but given the wins, i thought it was worth it! :)
SignalcAddress & uuids
- somewhere in the middle of this I got curious as to why we weren't similarly preferring
SignalcAddresses everywhere isntead ofSignalServiceAddresses. - i realized that the main reason was because
SignalcAddresses represented UUIDs as strings, which presented impedence mismatches at points where we needed to have an actual UUID object - i in turn realized that we only represent uuids as strings in
SignalcAddresswas because they need to be serializable, and UUIDs are not by default serializable - since at the time i didn't know how to make custom serializers, i simply opted for the easier option. but now i do! so i made a custom
UUIDSerializerclass, used it to make actualUUIDfields on ourSignalcAddressdata classes serializable, and updated our code to preferSignalcAddresses everywhere they can up until the interface boundary with libsignal code expecting aSignalServiceAddressat which point we return as before
Account#id (new convention!)
- during this card, I spend a lot of time (1) looking at our addresses so much, but also (2) becoming conscious of how unsealed sender messages don't by default provide phone numbers, which drew my attention to how (3) libsignal's
Envelope#identifierwill first look for a UUID before providing a phone number string, whereas signalc code in general assumes that a user's id will be there phone number not their UUID. - noting that we might at some point want to change this convention (particularly if we elimiante phone numbers altogether), I wanted to start being more conscious of which id we prefer and start building in a seam at the type level for providing either phone number or UUID in a way that is opaque to callers, and easily modifiable in one place in the future. (this instead of simply always hardcoding a preference for numbers via calls to, e.g.
Account#username, which will likely be a mess to cutover from later) - to open the possibility for adopting such a new convention, I added an
#idproperty toVerifiedAccount, and used it for both storing a user's profile key (inSignalReceiver#relay) and retrieving one (inSignalSender#sendDataMessage) - in general, i'd like to start using
account#idwherever we can instead ofaccount.username
tooling upgrade
- i added
make sc.restart.rebuildwhich stops signalc, rebuilds it, and starts it again. it's much nicer than manually doing the same!