document a few constants #473
Conversation
|
I'm aware that constants like these are supposed to be implementation details, but I like the idea of bringing these out into the open because some of them have implications on security (e.g. |
cosmicexplorer
force-pushed
the
document-constants
branch
from
43a03dd to
75d19fb
Compare
cosmicexplorer
force-pushed
the
document-constants
branch
from
75d19fb to
23f17d0
Compare
cosmicexplorer
force-pushed
the
document-constants
branch
from
23f17d0 to
9d6cce5
Compare
cosmicexplorer
force-pushed
the
document-constants
branch
from
9d6cce5 to
7a1c5be
Compare
rust/protocol/src/consts.rs
Outdated
| //! Magic numbers. | ||
|
|
||
| /// Various positive integers bounding the maximum size of other data structures. | ||
| pub mod limits { |
There was a problem hiding this comment.
If they're all limits then maybe consts should just change to limits.
There was a problem hiding this comment.
Great idea! Done!
rust/protocol/src/consts.rs
Outdated
| /// The maximum number of encrypted messages that the client chain which decrypts Signal | ||
| /// messages in a [Double Ratchet] instance can retrieve at once (tracked in | ||
| /// [crate::proto::storage::session_structure::chain::ChainKey::index] as well as a separate | ||
| /// `counter`). |
There was a problem hiding this comment.
This isn't really about how many messages you can decrypt at once; it's about how many messages you can not receive and still decrypt further messages on an existing chain. Hard to explain, though!
There was a problem hiding this comment.
I have spent some time to distill what it actually does, which ended up taking a couple paragraphs. Please let me know if I should move that documentation elsewhere.
There was a problem hiding this comment.
(this is now in limits.rs)
rust/protocol/src/consts.rs
Outdated
| /// The maximum number of per-message keys that can be retained to decrypt messages within | ||
| /// a specific chain from `message_keys` in [crate::proto::storage::session_structure::Chain]. | ||
| pub const MAX_MESSAGE_KEYS: usize = 2000; |
There was a problem hiding this comment.
Similarly, this one is about when you do encounter such a jump, how many past keys will the session retain in case you receive some of those missing messages (due to some kind of out-of-order delivery, I guess).
There was a problem hiding this comment.
I have spent a little time to rewrite this, referring to the documentation for MAX_FORWARD_JUMPS I rewrote above.
There was a problem hiding this comment.
(this is now in limits.rs)
rust/protocol/src/consts.rs
Outdated
| /// The maximum number of temporary backup chains to allow for `receiver_chains` in | ||
| /// [crate::proto::storage::SessionStructure]. These backup chains corresponds to the [Sesame] | ||
| /// protocol for syncing a Double Ratchet chain between two users. | ||
| /// | ||
| /// [Sesame]: https://signal.org/docs/specifications/sesame/#server | ||
| pub const MAX_RECEIVER_CHAINS: usize = 5; |
There was a problem hiding this comment.
This is just about past chains within the Double Ratchet protocol, again to handle out-of-order delivery.
There was a problem hiding this comment.
I rewrote this to clarify that this only occurs for double ratchet chains.
There was a problem hiding this comment.
(this is now in limits.rs)
cosmicexplorer
force-pushed
the
document-constants
branch
from
7a1c5be to
f657c79
Compare
|
So I spent some time to understand what each of these limits actually represents, and I think the more involved explanations I have now are useful, in particular the description of what each limit actually bounds and why that matters (avoiding denial-of-service attacks, for one). I'm going to spend some more time understanding what an archived session state is. While these descriptions are quite useful for me to understand what the library is doing (as implementation notes for some parts which aren't fully defined in the Double Ratchet spec), they may be too verbose for this file, so one possibility might be to move this more involved documentation to If that arrangement seems useful, then we might also consider making |
|
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. |
|
This issue has been closed due to inactivity. |
Broken out from #287.
limitssubmodule (this part can be reverted if desired).