Panic on empty input for Secure Cell

[ilammy]

It does not make much sense to use an empty master key for encryption, or encrypt or decrypt empty messages. Secure Cell does not allow such inputs and returns errors if the user insists. Additionally, context imprint mode of Secure Cell requires a non-empty user context.

The programmers should never really use Secure Cell in this way. If an empty key, or message, or token, or user context is used anywhere then it's most likely to be a programming error. Therefore treat it as such — like using an invalid index with a slice — and panic instead of returning an error code if we encounter empty inputs.

[vixentael]

Are you sure that the code should panic rather than return error if input parameters are not valid?

I've read only one chapter of Rust book about panicking, but I'm not sure how typical this behavior is for Rust-devs.

Few usage examples:

1. Wrong configuration. Developers sets up SecureCell without master key. On the runtime SecureCell returns panic to indicate that master key is required. Panic looks appropriate, developers add master key in code.

2. Data got from network. Application got master key / message from network requests and called SecureCell to decrypt message. Due to network encoding or failure, key or message might be corrupted. SecureCell responds with panic, which might be unexpected for developers and be found only during app usage.

To make sure that code works properly, developers should handle both panics and errors. Panic looks as (unexpected) additional check-to-make. Is it ok for Rust ecosystem?

[ilammy]

Are you sure that the code should panic rather than return error if input parameters are not valid?

No, I'm not completely sure.

The approach depends on the severity of the error condition which I would like to discuss.

On panics

Libraries should rarely panic because this is a decision that affects the whole application rather than only the library code and its direct users. By panicking the library forces the whole program to handle an error in a certain way and does not give the programmer an option for different behavior.

One does not really handle panics (though there are mechanisms for that). Instead panics are to be prevented. So the idea is that the developer has to either perform necessary safety checks before calling the code that might panic, or use some other means to prevent a possible panic (e.g., iterator API makes it impossible to use invalid array indices in the first place).

Errors signaled by panics should be so severe that killing the thread justifies the prevented harm. Now, after considering your example with keys obtained externally I'm not really sure that we need to panic at all, not just in this situation. Using invalid or corrupted data is certainly a preventable error condition. It can be avoided by performing a check. It can be handled after-the-fact by inspecting the returned result. And it does not really jeopardize the process as a whole if it happens, in contrast with using an out-of-bounds array index for example.

So it seems it would be fine to leave the situation as it is now (without panics). The user is technically allowed to pass empty keys, tokens, contexts, messages to Secure Cell. However, they will get an error in result. Even if that's a configuration error then it has to be handled explicitly.

Opinions?

On further actions

One of my concerns was about the master key. It is currently possible to construct a Secure Cell with an empty master key, but the error will be reported only when the cell is actually used to encrypt or decrypt data. The construction site and the usage site may be quite distant in code and time which can make it hard to track this error.

I think it would be nice to prevent this kind of errors earlier. I'd like to change the Secure Cell constructor to return a Result instead which should be an error if the key is empty. This will force the users to handle this possible error at the construction site.

That can be done in a different pull request and this one can be closed if we choose to not add any new panics to the code.

Alternatives

Some libraries provide “dual interface”. For Secure Cell it would be an encrypt() method that returns Vec<u8> directly and panics on any error whatsoever, accompanied by its cousin try_encrypt() that returns an explicit Result<Vec<u8>> and never panics.

It looks a bit nicer in the code but I find this approach to have poor discipline. It's not that hard to add an .unwrap(), or .expect(), or even just ? to the line to handle the error in a suitable way.

[vixentael]

The user is technically allowed to pass empty keys, tokens, contexts, messages to Secure Cell. However, they will get an error in result.

agree

I'd like to change the Secure Cell constructor to return a Result instead which should be an error if the key is empty

agree.

instead of panicking, can we add length-checks? If no token/context/masterkey/message - return Error as result?

[ilammy]

@vixentael, I've opened a new pull request with early length checks for master key during Secure Cell construction. This one PR will be closed as there will be no expected panics in the code.

Rust-Themis already reports an error for invalid length of other parameters. The checks are actually performed by Themis core. We have tests that verify this (e.g., for message).