Skip to content

[#483] resolve "Support sealed sender messages"

aguestuser requested to merge 483-support-sealed-sender-messages into main

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 than name (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)
  • we also modify the profileKey field on our Account data class to represent profile keys at rest as byte arrays rather than ProfileKey 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 and hashCode methods for all account subclasses to satisfactorily compare the profileKeyBytes fields for equality (since they are now arrays)

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_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 to SignalcAddress 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 a SignalcAddress from the Account#address property 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 the UnidentifiedAccessPair 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 in AccountManager, 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 the AccountManager), 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

[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 a Blocked variant of libsignal's SendMessageResult
  • to accomplish that, I created a SignalcSendResult algebraic data type, with Blocked as one of its variants and all of the 5 variants of libsignal's SendMessageResult as its remaining variants
  • to convert from a SendMessageResult to a SignalcSendResult, i borrowed the code that we used to use in SendResultType to convert from libsignal's representation into a simple enum class that we could use in when 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 to SendMessageResult.type() to produce it)
    • SocketReceiver can now switch directly off of the SignalcSendResult returned from SignalSender, 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 a SocketResponse.SendResult without dozens of lines of confusing conversion code (since we already did conversion once and for all when constructing our SingalcSendResult), allowing us to delete all this confusing (and now unnecessary) code
  • 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 of SignalServiceAddresses.
  • 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 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 actual UUID fields on our SignalcAddress data classes serializable, and updated our code to prefer SignalcAddresses everywhere they can up until the interface boundary with libsignal code expecting a SignalServiceAddress 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 to VerifiedAccount, and used it for both storing a user's profile key (in SignalReceiver#relay) and retrieving one (in SignalSender#sendDataMessage)
  • in general, i'd like to start using account#id wherever we can instead of account.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!
Edited by aguestuser

Merge request reports