[WASM-1] Implement the obfuscated MTProto transport #298
Conversation
There was a problem hiding this comment.
Good work!
IIRC at some point Error::MissingBytes also reported how many bytes we were expecting. I probably removed it because it wasn't needed, but now it is.
If we bring that back, then the obfuscated transport could decrypt the first 4 bytes (which contain the entire header for all supported transports), attempt to unpack, and from the reported error we would know how much more we need to decrypt.
Then we can keep a flag "have we decrypted the header yet" (or better yet, store an Option with the last-requested size) to avoid doing the decryption on that again. And once we have enough bytes, we decrypt the rest and return that.
Now we can go back to returning a single Result<Offset> instead of a Vec with the allocations that implies. And it should also mean we don't need to undo decryptions.
WDYT?
|
@Lonami |
We need this to decrypt the buffer in-place when using the obfuscated transport.
| fn unpack(&mut self, buffer: &[u8]) -> Result<UnpackedOffset, Error>; | ||
| /// Subsequent calls to `unpack` should be made with the same buffer, | ||
| /// with the data on the ranges from previous `UnpackedOffset` removed. | ||
| fn unpack(&mut self, buffer: &mut [u8]) -> Result<UnpackedOffset, Error>; |
There was a problem hiding this comment.
There should probably be an explainer (in the comments) on why the buffer is mut here... As I was wondering.
There was a problem hiding this comment.
pack takes a mutable input so in my view it's reasonable for unpack to do so too.
There was a problem hiding this comment.
By why would we alter the buffer itself? Isn't it supposed to be the read buffer?
There was a problem hiding this comment.
Both packing unpacking occur in-place. None of the previous transports had to modify the buffer during unpack, but obfuscated does. The caller doesn't read from the buffer contents until the calls succeed.
There was a problem hiding this comment.
(Forgot to add, but that said, I'm happy to have the documentation improved to leave this in a better place than a random GitHub discussion.)
|
|
||
| fn unpack(&mut self, buffer: &mut [u8]) -> Result<UnpackedOffset, Error> { | ||
| if buffer.len() < self.decrypt_tail { | ||
| panic!("buffer is smaller than what was decrypted"); |
There was a problem hiding this comment.
Should this really be a panic? If so - there should be an explainer in the comments as to where this is checked and why this case can't be handled (i.e. we should crash the whole app here).
There was a problem hiding this comment.
Yes, this should really panic. Reaching here means the user has messed up their buffer in an unexpected way. And there'd be a panic further down anyways, so it's better to provide a user friendly message than to crash with an indexing error.
There's also an explainer in the comments of Transport::unpack() and this will never happen when used with grammers-mtsender. It's only panic!(...) and not unreachable!(...) because the crate can be used standalone.
This PR implements support for transport obfuscation. It is the first step towards a web port.
Changes I had to make to make this feasible:
Transport::unpack()now takes in a mutable reference to the buffer to do the decryption in-place.Transport::unpack()now returnsResult<Vec<UnpackedOffset>>. This is needed since a read might, and frequently will, contain multiple transport payloads.Transport::unpack()now returns an empty Vec instead ofError::MissingByteswhen there's more data needed.ctris a new dependency ingrammers-mtsender.Transport::unpack()must now be called with the same buffer and the buffer must be kept between calls.While changing the return type of unpack to aVecdoesn't also seem good, it was the only compromise I could think of to allow this transport to work without significant performance impact while also keeping the decryption contained inside of the transport implementation.I'm now storing the decryption state inside the transport itself. So should have almost zero overhead other than the encryption itself