Move docs for sealed sender v1/v2 to the appropriate method docstrings

[cosmicexplorer]

Hello! I am going to finish up my other pending PRs to this repo soon :).

Problem

The sealed sender single-recipient and multi-recipient KEM schemes in Sealed Sender v1/v2 are super neat, and documentation for how they work exists in some (very helpful!) source comments. However, it's not immediately clear e.g. that the Signal server receives the result of sealed_sender_multi_recipient_encrypt(), splits the output into constituent sealed-sender v2 messages, then routes them individually to clients for sealed_sender_multi_recipient_decrypt(). Additionally, the design tradeoffs between v1 and v2 sealed sender mechanisms in general are not made clear in documentation of these public methods.

Solution

1. Move the (very useful!) documentation for the mechanism of v1 and v2 sealed sender into the docstrings for methods sealed_sender_encrypt_from_usmc() and sealed_sender_multi_recipient_encrypt().
2. Clarify the role of sealed_sender_multi_recipient_fan_out() in testing.
   • Clarify how the Signal server will split out sealed sender v2 bulk messages and route them to the appropriate recipients on the backend in sealed_sender_multi_recipient_encrypt().
3. Produce a section "Contrast with Sealed Sender v1" in the docstring for sealed_sender_multi_recipient_encrypt() which describes the pros and cons of the single-recipient vs multi-recipient KEM schemes implemented in the sealed_sender_v1 and sealed_sender_v2 modules.
4. Modify the definition of sealed_sender_v1::{EphemeralKeys,StaticKeys} to avoid repeated Result<Box<...>>ing, and make StaticKeys::calculate() accept &IdentityKeyPair and &IdentityKey instead of structs from curve.rs.
5. Make sealed_sender_v2::{apply_agreement_xor,compute_authentication_tag} accept and return fixed-size arrays instead of unsized slices.
6. Avoid separating the AES-GCM-SIV authentication tag from the rest of the ciphertext in sealed_sender_multi_recipient_encrypt() in order to differentiate it from the asymmetric authentication tags at_i computed later in the function when looping over the recipients.

Result

A rendered version of the docs can be seen here: https://cosmicexplorer.github.io/libsignal-client/target/doc/libsignal_protocol/.

This is a great codebase!!

[jrose-signal]

Thank you!

[jrose-signal]

There's a public/private distinction that's important here: sealed_sender_multi_recipient_encrypt and sealed_sender_decrypt are public, but sealed_sender_v1 and sealed_sender_v2 are implementation details. Whether or not the public entry points mention the specific cryptography involved could go either way, but it can't reference private stuff if we're documenting this like a usual crate.

As an aside, there's nothing wrong with Sealed Sender v1. In fact, it's a smaller payload on the receiving side than Sealed Sender v2. But its encryption key is derived at least partially from the recipient's identity key, so even if the same content is sent to multiple recipients, it has to be re-encrypted each time. SSv2's real change is separating the encryption key from the recipient's identity key, so that only a per-recipient needs to be encrypted differently.

[cosmicexplorer]

SSv2's real change is separating the encryption key from the recipient's identity key

Ok, thanks! That was super helpful! In light of this I have expanded the scope of this PR to also cover documenting the mechanics of v1 sealed sender in sealed_sender_encrypt_to_usmc(), as well as a quick comparison of the design tradeoffs of the two mechanisms that you've just summarized!

I also made a quick change just now in a3e0031 to explicitly append the AES-GCM-SIV authentication tag to the end of the symmetric ciphertext in sealed_sender_multi_recipient_encrypt(), which I thought made it more clear that that authentication tag has nothing to do with the asymmetric authentication tags at_i computed later in the method.

There's a public/private distinction that's important here

Thanks! In light of this, all of the docs I've just added also explicitly avoid referring to "v1" or "v2" sealed sender mechanisms, instead referring to the single- and multi-recipient KEM schemes described in the docstrings for sealed_sender_encrypt_to_usmc() and sealed_sender_multi_recipient_encrypt().

It was a fun exercise writing these docs but if they're more than you'd like to review / aren't appropriate for the repo for some reason feel free to tell me to cut them down!

[cosmicexplorer]

I've pushed a rendered version of the docs from cargo doc to my fork's gh-pages branch, viewable at https://cosmicexplorer.github.io/libsignal-client/target/doc/libsignal_protocol/. The two method docstrings which are the subject of this PR:

• sealed_sender_encrypt_to_usmc()
• sealed_sender_multi_recipient_encrypt()

[jrose-signal]

Pile of comments but I appreciate this work! I think you've done well separating the public and private parts.

[jrose-signal]

Mm, I think I got this wrong too; the symmetric key is separately derived from (1). At some point we really are just spelling out the pseudocode, though…

[cosmicexplorer]

Yeah, I think I got to that realization as well. I tried deleting this numbered list section and I think I still prefer having it, but we can probably afford to be more concise since the pseudocode + paper link is actually extremely useful.

[jrose-signal]

Eh. It could have been protobuf, but that's just wasting space in a format with so many fixed-width fields. We went back and forth but decided that the only thing protobuf would buy us is extensibility, and we could add another version instead.

I wrote up this document internally on the format; maybe some form of it belongs here or in the repository somewhere as well:
multi_recipient_sealed_sender.md.

[cosmicexplorer]

The rustdoc book mentions it does not have support for bare markdown files, which would certainly be convenient here. I think this looks brief and appropriate enough for the "Wire Format" section of the sealed_sender_multi_recipient_encrypt() docstring, so I'll try adapting it there.

[cosmicexplorer]

Hmmmm, rust-lang/rust#76709 certainly seems to imply it is possible to use rustdoc against markdown files directly. I will try to apply that feature.

[cosmicexplorer]

Ok, so it's possible to use rustdoc against markdown files directly as a command-line argument, but it seems that isn't integrated into cargo doc (either providing files as CLI arguments, or using raw markdown files). In 562c1a5 I took the liberty of merging this doc into the "Wire Format" section of sealed_sender_multi_recipient_encrypt(), and linking into the resulting "The version byte" section from the "Wire Format" section of sealed_sender_encrypt_from_usmc().

In these changes I have also referred to "Sealed Sender v1" and "Sealed Sender v2" instead of "the single-/multi-recipient KEM". See the "Wire Format" section for Sealed Sender v2 rendered at https://cosmicexplorer.github.io/libsignal-client/target/doc/libsignal_protocol/fn.sealed_sender_multi_recipient_encrypt.html#wire-format.

[jrose-signal]

I wasn't necessarily thinking "in the repository" had to mean "included in the rustdoc", but it seems to work, especially since functions get their own pages. (Thanks for rendering the docs!) You can include a Markdown file using #[doc = include_str!("foo.md")] (if we're on a new enough Rust, h/t the Rustdoc book), but I think this is better anyway.

[jrose-signal]

Okay, this one's nearly all copyediting-type comments—all the structure looks good to me now. I'm sorry there are so many! But it's definitely converging. And again I appreciate you having generated the docs; that makes it much easier to read and review.

[jrose-signal]

I think it's worth adding something like "For more details on the sealed sender formats, see [sealed_sender_encrypt_from_usmc] and [sealed_sender_multi_recipient_encrypt]."

[jrose-signal]

trust_root isn't actually provided by the server; it's baked into the apps. The root signs the server certificate, which signs the sender certificate.

[cosmicexplorer]

Thank you for this very very helpful clarification!! I have added both of these comments here.

[cosmicexplorer]

Ok, I've addressed most of your comments and left 9 of them unresolved for further review! As before, the compiled cargo doc output has been pushed to https://cosmicexplorer.github.io/libsignal-client/target/doc/libsignal_protocol/, and the focus of this PR is on these two:

• Sealed Sender v1: sealed_sender_encrypt_from_usmc()
• Sealed Sender v2: sealed_sender_multi_recipient_encrypt()

[jrose-signal]

Answered your open questions (mostly in agreement) and I think these'll be my last comments!

[cosmicexplorer]

Ok, cf048dd integrates your notes on &...[..] and .expect("...") usage! It does not change any docstrings, so I have not pushed any docs changes since this comment: #366 (comment).

Of course, please feel free to make any further comments on code or docs changes, it's very easy for me to oblige.

I'll also probably do a once-over again and see if there's anything else I think could be improved. I think the result is DRASTICALLY better than my initial attempt here and I learned quite a lot from this PR :DDDD. Thanks again for that.

[cosmicexplorer]

After reviewing the docs again, I think I can sign off on this so merge it whenever! Thanks again for your time making this amazing, it helped me a lot and I hope many others too :)))