chore: Update imap-codec. #246

duesee · 2023-07-04T21:51:27+03:00

duesee commented

2023-07-04 21:51:27 +03:00

Syncing with latest developments in imap-codec :-)

duesee changed title from ~~chore: Update `imap-codec`.~~ to wip: chore: Update `imap-codec`.

2023-07-04 21:54:10 +03:00

duesee force-pushed master from deef64a94c to 9d51b6bd52

2023-07-04 21:56:59 +03:00

Compare

duesee changed title from ~~wip: chore: Update `imap-codec`.~~ to chore: Update `imap-codec`.

2023-07-04 21:58:30 +03:00

Manos Pitsidianakis reviewed 2023-07-05 00:48:46 +03:00

melib/src/backends/imap/connection.rs

													
				@ -531,3 +528,3 @@

				        timeout(self.timeout, async {

				            let command = {

				                let tag = Tag::unchecked(format!("M{}", self.cmd_id));

				                let tag = Tag::unvalidated(format!("M{}", self.cmd_id));

Manos Pitsidianakis commented

2023-07-05 00:48:46 +03:00

(Not a PR review, just a thought)

Now that commands are code-ified instead of handrolled, the M prefix could go away and have an e.g. ascending integer counter instead. That way Tag could be an enum:

enum Tag<'_> {
  String(Cow<'_, str>), // current type
  Int(u64)
}

and a Tag::new_int(self.cmd_id) method would need no allocation. Its Write/Display impl could also use a hex representation to avoid base ten taking lots of ascii digits to be represented in the outgoing command. What do you think?

(Not a PR review, just a thought) Now that commands are code-ified instead of handrolled, the `M` prefix could go away and have an e.g. ascending integer counter instead. That way `Tag` could be an enum: ``` enum Tag<'_> { String(Cow<'_, str>), // current type Int(u64) } ``` and a `Tag::new_int(self.cmd_id)` method would need no allocation. Its `Write`/`Display` impl could also use a hex representation to avoid base ten taking lots of ascii digits to be represented in the outgoing command. What do you think?

duesee commented

2023-07-05 11:09:31 +03:00

Poster

I like the idea but am not sure how we would integrate this. In imap-codec, I try to maintain the "inverse property" of parse/unparse.

So, when we create a Tag::Int(42), serialize it into e.g. 2A, and deserialize it again, we really want to obtain Tag::Int(42) again (and not Tag::String("2A")). So the only way to archive this would be to correctly "partition" the tag space and agree on a special representation. But this would rather not be generic enough.

So, maybe the better way would be not to use Command but only CommandBody and handle tags manually. Or, make Tag generic, a trait, or something other.

I would propose that we postpone this for now until we are sure that it helps us a lot :-)

Some more thoughts: Ideally, we want to have random tags. The reason is that during our research on STARTTLS we experienced that many attacks would have been much harder to execute with random tags. Of course, this is STARTTLS-specific, but it can increase the difficulty for other kinds of attacks and is a good security in-depth measure.

Given that, we still could use numbers. With a bit less entropy per byte but it doesn't really matter.

I like the idea but am not sure how we would integrate this. In imap-codec, I try to maintain the "inverse property" of parse/unparse. So, when we create a `Tag::Int(42)`, serialize it into e.g. `2A`, and deserialize it again, we really want to obtain `Tag::Int(42)` again (and not `Tag::String("2A")`). So the only way to archive this would be to correctly "partition" the tag space and agree on a special representation. But this would rather not be generic enough. So, maybe the better way would be not to use `Command` but only `CommandBody` and handle tags manually. Or, make `Tag` generic, a trait, or something other. I would propose that we postpone this for now until we are sure that it helps us a lot :-) Some more thoughts: Ideally, we want to have random tags. The reason is that during our [research on STARTTLS](https://nostarttls.secvuln.info/) we experienced that many attacks would have been much harder to execute with random tags. Of course, this is STARTTLS-specific, but it can increase the difficulty for other kinds of attacks and is a good security in-depth measure. Given that, we still could use numbers. With a bit less entropy per byte but it doesn't really matter.

Manos Pitsidianakis commented

2023-07-05 11:38:40 +03:00

You're right, I didn't think of the de-serialization! It was just a thought I had at that moment.

epilys marked this conversation as resolved

Manos Pitsidianakis reviewed 2023-07-05 00:51:37 +03:00

Manos Pitsidianakis left a comment

LGTM, not merging now because I don't know if you mean this to be WIP or ready so let me know!

duesee requested review from Manos Pitsidianakis 2023-07-05 09:02:15 +03:00

duesee commented

2023-07-05 09:04:33 +03:00

Poster

LGTM, not merging now because I don't know if you mean this to be WIP or ready so let me know!

I requested a review to signal that this is ready to be reviewed. I can keep doing so in the future :-)

> LGTM, not merging now because I don't know if you mean this to be WIP or ready so let me know! I requested a review to signal that this is ready to be reviewed. I can keep doing so in the future :-)

Manos Pitsidianakis approved these changes 2023-07-05 15:10:31 +03:00

Manos Pitsidianakis merged commit 9d51b6bd52 into master

2023-07-05 15:10:40 +03:00

Sign in to join this conversation.

No reviewers