crypto: Rotate fallback keys in a time-based manner #3151
Conversation
Fallback keys until now have been rotated on the basis that the homeserver tells us that a fallback key has been used. Now this leads to various problems if the server tells us too often that it has been used, i.e. if we receive the same sync response too often. It leaves us also open to the homeserver being dishonest and never telling us that the fallback key has been used. Let's avoid all these problems by just rotating the fallback key in a time-based manner.
poljar
force-pushed
the
poljar/fallback-key-time-based-rotation
branch
from
d51f5b9 to
aeec0c9
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3151 +/- ##
==========================================
- Coverage 83.85% 83.85% -0.01%
==========================================
Files 231 231
Lines 23865 23877 +12
==========================================
+ Hits 20011 20021 +10
- Misses 3854 3856 +2 ☔ View full report in Codecov by Sentry. |
|
Looks good to me, from a code point of view i.e. I think it does what the PR description says it does. Not reviewing whether it's the right thing to do, and hoping @dkasak will do that :-) Also made some optional overly-picking punctuation suggestions. |
Co-authored-by: Andy Balaam <andy.balaam@matrix.org> Signed-off-by: Damir Jelić <poljar@termina.org.uk>
| /// | ||
| /// [1]: https://signal.org/docs/specifications/x3dh/#publishing-keys | ||
| fn fallback_key_expired(&self) -> bool { | ||
| const FALLBACK_KEY_MAX_AGE: Duration = Duration::from_secs(3600 * 24 * 7); |
There was a problem hiding this comment.
Please can this be configurable by the caller, as for complement-crypto tests I'd want to set this to be a few seconds to ensure keys are being cycled, which means it needs to be visible to the FFI bindings.
There was a problem hiding this comment.
This is something that really shouldn't be configurable, can't you fake time in complement?
Alternatively we can add a hidden compile-time feature flag, that should work as well since you compile the bindings yourself, right?
There was a problem hiding this comment.
I might be able to use something like https://github.com/wolfcw/libfaketime but it'll be very brittle and likely to break randomly.
A compile-time feature flag works for me, as that can then be used for WASM bindings as well.
There was a problem hiding this comment.
There are other places where this would be nice e.g we cannot test rotation_period_ms because rust SDK has a lower bound of 1 hour.
There was a problem hiding this comment.
+1 for allowing to provide config either at runtime or compile time - the integration tests we are writing use real homeservers in separate processes, so faking out the time at this level is difficult.
It's also arguable, at least in the case of rotation_period_ms that different use-cases might want different limits on these values, so config would be helpful for non-test situations. (E.g. a password manager might have different security requirements from a chat app).
There was a problem hiding this comment.
You can set the rotation_period_ms with the m.room.encryption event, it's just that we clamp the values so you don't get stuck in a loop where by the time you share the room key it already expired.
There was a problem hiding this comment.
Yeah, we are setting the rotation_period_ms with the m.room.encryption event but running into the clamp because we want our test to run in <1hr
Fallback keys until now have been rotated on the basis that the homeserver tells us that a fallback key has been used.
Now this leads to various problems if the server tells us too often that it has been used, i.e. if we receive the same sync response too often. It leaves us also open to the homeserver being dishonest and never telling us that the fallback key has been used.
Let's avoid all these problems by just rotating the fallback key in a time-based manner.
This fixes #3127.