[#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 chains
error 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
ProfileStore
to do this. it is pretty straightforward. notably we:- introduce a new convention of calling the identifier (number or uuid) of another signal account a
contactId
rather 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
SignalReceiver
layer, but opted for here to avoid cluttering up#relay
with 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
profileKey
field on ourAccount
data class to represent profile keys at rest as byte arrays rather thanProfileKey
object 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
equals
andhashCode
methods for all account subclasses to satisfactorily compare theprofileKeyBytes
fields for equality (since they are now arrays)
- in order to do this, we had to handroll
core flow
SignalReceiver
(1) storing profile keys in - 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_SENDER
in the set of envelope types that we handle - as noted above: we push profile key validation down into the
ProfileKeyStore
layer (open to feedback on prefering something else!) - side note on switch from
SignalServiceAddress
toSignalcAddress
in 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
asSignalcAddress
because as a result of the convention shift noted above, we now simply return aSignalcAddress
from theAccount#address
property getter, making such conversions unnecessary
SignalSender
(2) retrieving profile keys in - whenever we hit
#sendDataMesssage
(for either a message or an expiry time update), we try to derive theUnidentifiedAccessPair
necessary 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
#getUnidentifiedAccessPair
function 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#sendMessage
and a sealed sender message is generated - if we cannot derive the necessary sealed-sender token/cert materials (indicated by a null
UnidentifiedAccessPair
returned 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#sendDataMessage
in cases of block, necessitated creating a way to represent aBlocked
variant of libsignal'sSendMessageResult
- to accomplish that, I created a
SignalcSendResult
algebraic data type, withBlocked
as one of its variants and all of the 5 variants of libsignal'sSendMessageResult
as its remaining variants - to convert from a
SendMessageResult
to aSignalcSendResult
, i borrowed the code that we used to use inSendResultType
to convert from libsignal's representation into a simple enum class that we could use inwhen
statements - 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
send
function), which had a number of nice spillover simplifying effects:- the
SendResultType
can be completely eliminated (as can all calls toSendMessageResult.type()
to produce it) -
SocketReceiver
can now switch directly off of theSignalcSendResult
returned 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.SendResult
without 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
SignalcAddress
es everywhere isntead ofSignalServiceAddress
es. - i realized that the main reason was because
SignalcAddress
es 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
SignalcAddress
was 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
UUIDSerializer
class, used it to make actualUUID
fields on ourSignalcAddress
data classes serializable, and updated our code to preferSignalcAddress
es everywhere they can up until the interface boundary with libsignal code expecting aSignalServiceAddress
at 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#identifier
will 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
#id
property 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#id
wherever we can instead ofaccount.username
tooling upgrade
- i added
make sc.restart.rebuild
which stops signalc, rebuilds it, and starts it again. it's much nicer than manually doing the same!