Add branch for CiphertextMessageType::SenderKey in sealed_sender_decrypt

[gferon]

While starting to use sender keys, I realized that the helper function sealed_sender_decrypt doesn't have a branch for CiphertextMessageType::SenderKey which pushes the responsibility of the (correct) implementation to the caller. This PR fixes this issue.

I looked at the source of official Signal clients, and it looks like a few of them call sealed_sender_decrypt and then handle these messages themselves on error.

There might be a reason why this is designed like this (maybe avoiding breaking the API?) and I'd be glad to hear about it if you don't want to go forward with this change!

[jrose-signal]

Avoiding breaking the API is the initial reason, but the other thing that's held this back is error reporting: if you manage to decrypt the sealed sender wrapper but fail to decrypt the inner message, you probably want to know who claimed to send the inner message (though at that point you probably shouldn't 100% trust that information, since things are already weird). The Java API deals with this through its set of Protocol*Exceptions, but the TypeScript error reporting doesn't currently do anything like that (though it could), and the Swift error reporting has to squeeze itself through a C interface.

Of course, someone could always call sealed_sender_decrypt and follow it up with a call to sealed_sender_decrypt_to_usmc on failure, but the wasted work doesn't make me happy. (Nor does it in Signal's official clients!) So I think if we're going to overhaul this API, we should make sure the error provides the USMC when possible. For Rust, that could mean changing the error type to something like

struct SealedSenderDecryptionError {
  underlying_error: SignalProtocolError,
  usmc: Option<UnidentifiedSenderMessageContents>,
}

or perhaps

enum SealedSenderDecryptionError {
  SealedSender(SignalProtocolError),
  InnerMessage {
    underlying_error: SignalProtocolError,
    usmc: UnidentifiedSenderMessageContents
  }
}

but getting this up to Java, Swift, and TypeScript is an extra challenge, and it also reminds me that I've learned a lot more about Rust error handling practices since co-designing libsignal-protocol. :sweat_smile:

Finally, what if we have another message type some day with its own store? Will we change the method again? I think probably the answer should be that it should just take a single SignalProtocolStore, which today is a trait combination but could really be its own trait (with default implementations, once those are stable).

All that said, it's certainly convenient not to have to reimplement this match in all clients, which is why the method got added at all. So…if you want to figure out some of this stuff together, we can certainly do so! But otherwise I recommend just using sealed_sender_decrypt_to_usmc.

[rubdos]

but the other thing that's held this back is error reporting: if you manage to decrypt the sealed sender wrapper but fail to decrypt the inner message, you probably want to know who claimed to send the inner message (though at that point you probably shouldn't 100% trust that information, since things are already weird).

What if you model this as a happy path instead of the failure path? The SealedSenderDecryptionResult could become an enum SealedSenderDecryptionResult, with a variant for decryption failure (for which you can handle the claim) and a variant for successful decryption? Or a Result<Result<, _>, _>. Just some thoughts about a potential API, if we're to break it.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This issue has been closed due to inactivity.