Retry failed signed key rotation; start rotation when registered #1772
Conversation
scottnonnenberg
force-pushed
the
rotate-keys
branch
from
8ef9ac6
to
d77bacd
Compare
| ); | ||
|
|
||
| if (e instanceof Error && e.name == 'HTTPError' && e.code >= 400 && e.code <= 599) { | ||
| var rejections = 1 + textsecure.storage.get('signedKeyRotationRejected', 0); |
There was a problem hiding this comment.
Can this value overflow? If so, is it a risk?
https://stackoverflow.com/questions/19054891/does-javascript-handle-integer-overflow-and-underflow-if-yes-how
the above JS interpreter does handle overflows and rounds to max_value
There was a problem hiding this comment.
In my testing, adding one to Number.MAX_VALUE results in Number.MAX_VALUE. Which is already super-massive. Even if it did return Infinity it still compares properly to 5, which is what we do elsewhere in the codebase. We reset it back to zero whenever we successfully rotate the keys.
| getAccountManager().rotateSignedPreKey(); | ||
| getAccountManager().rotateSignedPreKey().catch(function() { | ||
| console.log('rotateSignedPrekey() failed. Trying again in five seconds'); | ||
| setTimeout(runWhenOnline, 5000); |
There was a problem hiding this comment.
should the communication be marked as insecure if several prekey rotations fail? Could sb game the system by blocking the rotation?
There was a problem hiding this comment.
Your prekeys are used by others to start communication with you. If you haven't updated them lately, or failed in doing so, others won't be able to start new conversations at all. No need for warnings, just try our best to make those updates happen whey are supposed to to avoid errors.
There was a problem hiding this comment.
I'll clarify: I've seen errors happen most of the time in that no-rotation case. But in theory sessions can still be created, just with fewer guarantees than a standard session.
Windows 7: Use an alternate mechanism for notifications (#1812) Retry failed signed key rotation; start rotation when registered (#1772) Dev: - Update to electron-builder 19.29.0; may allow windows shortcut to stay deleted on update (#1804) - aptly.sh: Instructions for pruning old packages from repo (#1771) - Update development branch to include everything up to v1.0.39 0e328f3
* Retry failed signed key rotation; start rotation when registered (#1772) * rotateSignedPrekeys: Fix 'res is not defined' error * If the server rejects key rotation, don't retry immediately * Force a signed key rotation on launch of any new version
I've noticed that we're not adequately resilient to questionable situations in our signed key rotation. For example, I saw a log last week where the rotation failed because of network difficulties. Today, in that case, we would wait until the next scheduled key rotation time: a couple days.
With this change, after a failed rotation, we try every five seconds until it succeeds.
In this change we also get a little more precise about when we start up the signed key rotation. It doesn't make sense to do it before the user is registered, and it doesn't make sense to do it in the middle of an import, so we only do it if registration is complete, and import is complete.