Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add documentation for error cases #471

Closed
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
136 changes: 133 additions & 3 deletions rust/protocol/src/error.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
//
// Copyright 2020-2021 Signal Messenger, LLC.
// Copyright 2020-2022 Signal Messenger, LLC.
// SPDX-License-Identifier: AGPL-3.0-only
//

//! Errors that may occur during various stages of the Signal Protocol.

#![warn(missing_docs)]

use crate::curve::KeyType;

use displaydoc::Display;
Expand All @@ -11,82 +15,208 @@ use uuid::Uuid;

use std::panic::UnwindSafe;

#[cfg(doc)]
pub use crate::{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: does it have to be pub for rustdoc to pick it up?

address::ProtocolAddress,
curve::{PrivateKey, PublicKey},
group_cipher::{group_decrypt, group_encrypt},
protocol::{SenderKeyMessage, SignalMessage, CIPHERTEXT_MESSAGE_CURRENT_VERSION},
sealed_sender::{sealed_sender_decrypt, sealed_sender_multi_recipient_encrypt},
sender_keys::SenderKeyRecord,
session_cipher::{message_decrypt, message_encrypt},
state::{PreKeyId, PreKeyRecord, SignedPreKeyId, SignedPreKeyRecord},
storage::{IdentityKeyStore, PreKeyStore, SenderKeyStore, SessionStore, SignedPreKeyStore},
};

/// Return type for all fallible operations in this crate.
pub type Result<T> = std::result::Result<T, SignalProtocolError>;

/// Error states recognized by the Signal Protocol.
#[derive(Debug, Display, Error)]
#[ignore_extra_doc_attributes]
pub enum SignalProtocolError {
Comment on lines +34 to 37
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heh, this sounds better designed than what's actually going on: the "SignalProtocol" here is really just about the name of the crate, libsignal-protocol. Arguably it could just be Error or something like that; I'd probably document it as "Possible errors produced by the operations in this crate" to match what you did for the Result alias.

/// invalid argument: {0}
///
/// Raised if an invalid argument is provided to any Signal API methods.
///
/// Prefer to use lifetimes, static-sized slices, and dedicated wrapper structs in API
/// signatures to minimize the need to raise this error to FFI boundaries.
Comment on lines +42 to +43
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation should be client-facing, so comments about API design don't really fit.

InvalidArgument(String),
Comment on lines 38 to 44
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Probably every single one of these can be an assertion, since they should all indicate programmer error… Something to maybe clean up later.)

/// invalid state for call to {0} to succeed: {1}
///
/// Raised if some optional value was missing before performing some operation which needed it.
///
/// Prefer to avoid returning [std::result::Result] and [Option] from struct methods in cases
/// where they're not necessary for trait polymorphism, as well as using dedicated wrapper
/// structs in API signatures, to minimize the need to raise this error.
InvalidState(&'static str, String),
Comment on lines 45 to 52
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"optional" sounds like it's about Option, but this is really "you were supposed to set up certain state before calling this function, and you didn't". For instance, you can't send a message if you haven't started a session with someone.

(These, likewise, should be audited; many of them could be assertions, but some of them might be expensive to check up front, and should become more specific, more actionable errors.)


/// protobuf encoding was invalid
///
/// Raised if a field in a protobuf is invalid in some way.
///
/// Prefer to raise [Self::InvalidState] except in methods which directly decode protobufs.
InvalidProtobufEncoding,
Comment on lines 54 to 59
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be renamed InvalidEncoding, since the protobuf is an implementation detail. Raising InvalidState would be incorrect since InvalidState is about things the local client can control, and the contents of an incoming message isn't one of those.


/// ciphertext serialized bytes were too short <{0}>
///
/// Raised if some ciphertext was deserialized from a too-small slice.
///
/// Prefer to make API method signatures and wrapper structs consume and produce static-sized
/// byte slices to minimize the need to raise this error.
CiphertextMessageTooShort(usize),
Comment on lines 61 to 67
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be unified with InvalidEncoding above; there's nothing a caller would do differently based on which failure happened. If we think the different kinds of failure are important for debugging, we could add a message to InvalidEncoding. Failing that, there shouldn't be any mention of "slice"; this just indicates a parameter was too small.

/// ciphertext version was too old <{0}>
///
/// Raised if the ciphertext version decoded from a protobuf is older than this client.
///
/// The current client's ciphertext version is at [CIPHERTEXT_MESSAGE_CURRENT_VERSION].
LegacyCiphertextVersion(u8),
Comment on lines +69 to 73
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't mention "protobuf", and the particular current version depends on which protocol is raising this error. Signal, SenderKey, and SealedSender messages all have different versions.

/// ciphertext version was unrecognized <{0}>
///
/// Raised if the ciphertext version decoded from a protobuf is newer than this client.
///
/// The current client's ciphertext version is at [CIPHERTEXT_MESSAGE_CURRENT_VERSION].
UnrecognizedCiphertextVersion(u8),
Comment on lines +75 to 79
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

/// unrecognized message version <{0}>
///
/// Raised if the ciphertext version decoded from a protobuf fails to match the cached version
/// for the message chain that message originates from.
///
/// *TODO: This case should wrap the same numeric type as [Self::LegacyCiphertextVersion] and
/// [Self::UnrecognizedCiphertextVersion]. This dissonance is addressed in
/// <https://github.com/signalapp/libsignal-client/pull/289>.*
UnrecognizedMessageVersion(u32),
Comment on lines +82 to 88
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Protobuf is an implementation detail. The version is associated with a session, not a particular chain.


/// fingerprint version number mismatch them {0} us {1}
///
/// Raised if a fingerprint version decoded from a protobuf has an unexpected value.
FingerprintVersionMismatch(u32, u32),
Comment on lines +92 to 93
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Protobuf is an implementation detail, and the value isn't necessarily "unexpected"; it's just not the same as what the local fingerprint is using.

/// fingerprint parsing error
///
/// Raised if a field in a fingerprint protobuf is invalid in some way.
///
/// Similar to [Self::InvalidProtobufEncoding].
FingerprintParsingError,
Comment on lines +95 to 99
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The long-term thing to do here is give fingerprints their own error enums, but short-term agreed, this is basically the same as InvalidEncoding.


/// no key type identifier
///
/// Raised if a [PublicKey] is deserialized from an empty slice.
///
/// Prefer to use static-sized slices in API method signatures and struct fields to minimize the
/// need to raise this error.
NoKeyTypeIdentifier,
Comment on lines +102 to 107
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is a little silly. No one should mistake an empty slice for a public key.

/// bad key type <{0:#04x}>
///
/// Raised if a [KeyType] decoded from a [u8] has an unrecognized value.
BadKeyType(u8),
Comment on lines 108 to 111
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/// bad key length <{1}> for key with type <{0}>
///
/// Raised if a [PublicKey] or [PrivateKey] is deserialized from a slice of incorrect length.
///
/// Prefer to use static-sized slices in API method signatures and struct fields to minimize the
/// need to raise this error.
BadKeyLength(KeyType, usize),
Comment on lines 112 to 118
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


/// invalid signature detected
///
/// Raised if signature validation fails for a [SignedPreKeyRecord] or a [SenderKeyMessage].
SignatureValidationFailed,
Comment on lines 120 to 123
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: it would be better to make it clear that these are examples; many things have signatures that could fail to validate. These are just the two we surface as "signature validation failed".


/// untrusted identity for address {0}
///
/// Raised if an identity verification check fails in [message_encrypt] or [message_decrypt].
UntrustedIdentity(crate::ProtocolAddress),
Comment on lines 125 to 128
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, feels weirdly specific. Anything that takes an IdentityKeyStore could check identities and decide that one is untrusted.


/// invalid prekey identifier
///
/// Raised if a [PreKeyId] could not be resolved to a [PreKeyRecord] by a [PreKeyStore].
InvalidPreKeyId,
Comment on lines 130 to 133
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/// invalid signed prekey identifier
///
/// Raised if a [SignedPreKeyId] could not be resolved to a [SignedPreKeyRecord] by
/// a [SignedPreKeyStore].
InvalidSignedPreKeyId,
Comment on lines 134 to 138
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


/// invalid MAC key length <{0}>
///
/// Raised if a MAC key is deserialized from an incorrectly-sized slice.
///
/// Prefer to use static-sized slices in API method signatures and struct fields to minimize the
/// need to raise this error.
InvalidMacKeyLength(usize),

/// missing sender key state for distribution ID {distribution_id}
NoSenderKeyState { distribution_id: Uuid },
///
/// Raised if a [SenderKeyStore] is unable to locate a [SenderKeyRecord] for a given
/// *([ProtocolAddress], [Uuid])* pair in [group_encrypt] or [group_decrypt].
NoSenderKeyState {
/// Unique identifier for the sender key session.
distribution_id: Uuid,
},
Comment on lines 148 to +155
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


/// session with {0} not found
///
/// Raised if an [IdentityKeyStore] does not contain an entry for a [ProtocolAddress], or
/// alternately if a [SessionStore] does not contain a session for a given [ProtocolAddress].
SessionNotFound(crate::ProtocolAddress),
Comment on lines 157 to 161
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd switch the order of these; the common use is a missing session (hence the name), but it also gets used for missing identity when one is expected because repairing the session will fetch the identity too. cf. comment in the middle of sealed_sender_multi_recipient_encrypt:

// Returned as a SessionNotFound error because (a) we don't have an identity error
// that includes the address, and (b) re-establishing the session should re-fetch
// the identity.

/// invalid session: {0}
///
/// Raised if a [SessionStore] does not contain a remote identity key to validate.
///
/// Similar to [Self::InvalidState].
InvalidSessionStructure(&'static str),
Comment on lines 162 to 167
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, to clarify: InvalidState is something the caller should have already taken care of, InvalidSessionStructure is more "sorry, your session's borked, throw it out". The specific reason shouldn't be in the documentation.

/// invalid sender key session with distribution ID {distribution_id}
InvalidSenderKeySession { distribution_id: Uuid },
///
/// Raised if a [SenderKeyStore] could not load the session for the given ID.
///
/// Similar to [Self::NoSenderKeyState].
InvalidSenderKeySession {
/// Unique identifier for the sender key session.
distribution_id: Uuid,
},
Comment on lines 168 to +176
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/// session for {0} has invalid registration ID {1:X}
///
/// Raised if a sealed sender message has a registration id that doesn't map to any
/// known session.
InvalidRegistrationId(crate::ProtocolAddress, u32),
Comment on lines 177 to 181
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't have anything to do with sessions, registration IDs just have stronger constraints than can be expressed in protobuf's type system, and we only discovered we weren't enforcing them a while in, so it got a dedicated error to help catch the issue. (I think it would have fallen under InvalidEncoding otherwise.)


/// message with old counter {0} / {1}
///
/// Raised if the same message is decrypted twice so it can be discarded.
DuplicatedMessage(u32, u32),
Comment on lines 183 to 186
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"so it can be discarded" isn't wrong, but I think it's more of a suggested way to handle it than the only possibility.

/// invalid {0:?} message: {1}
///
/// Raised if a [SignalMessage] could not be decrypted or some field had an unexpected value.
///
/// *TODO: what differentiates this from [Self::CiphertextMessageTooShort]?*
InvalidMessage(crate::CiphertextMessageType, &'static str),
Comment on lines 187 to 192
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GOOD QUESTION. The main thing is that if the message is too short, we may not even know what type of message it was. But basically this is InvalidEncoding again, just with some additional context. This "additional context" could probably be handled differently in the future, but for now that's pretty much it. It's not just SignalMessages, though; the point of the type is to differentiate SignalMessages from PreKeySignalMessages, and it's used for SenderKeyMessages as well.


/// error while invoking an ffi callback: {0}
///
/// Raised to propagate an error from an FFI callback.
FfiBindingError(String),
Comment on lines 194 to 197
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not about propagating errors; it's when the callback itself can't be invoked. Think JNI-level errors. (It does not actually come up for pure Rust users and maybe doesn't belong here.)

/// error in method call '{0}': {1}
///
/// Raised to propagate an error through to an FFI exception along with a boxed handle.
ApplicationCallbackError(
&'static str,
#[source] Box<dyn std::error::Error + Send + Sync + UnwindSafe + 'static>,
),
Comment on lines 198 to 204
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the one where there was an error from a callback and we're just propagating it out, whether through FFI or from a Rust client that returns Err.


/// invalid sealed sender message: {0}
///
/// Raised if an [crate::sealed_sender::UnidentifiedSenderMessage] could not be
/// deserialized successfully.
///
/// *TODO: this sounds a lot like [Self::InvalidProtobufEncoding] or [Self::UntrustedIdentity]?*
InvalidSealedSenderMessage(String),
Comment on lines 206 to 212
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, InvalidEncoding with a message again. :-(

/// unknown sealed sender message version {0}
///
/// Raised if an version decoded from a [crate::sealed_sender::UnidentifiedSenderMessage]
/// was unrecognized.
UnknownSealedSenderVersion(u8),
Comment on lines 213 to 217
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't be decoded from UnidentifiedSenderMessage; it's in the outermost envelope.

/// self send of a sealed sender message
///
/// Raised if [sealed_sender_decrypt] finds that the message came from this exact client.
SealedSenderSelfSend,
Comment on lines 218 to 221
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}