[lib] mark prekeys as published after uploading to identity
ClosedPublic
Actions

Authored by varun on Jun 19 2024, 7:52 PM.

Details

Reviewers

kamil
marcin

Commits

rCOMMa62e9dbba688: [lib] mark prekeys as published after uploading to identity

Summary

These are all the places where we upload prekeys to the identity service. In all these cases, we should mark them as published immediately after uploading.

Depends on D12479

Test Plan

https://gist.github.com/vdhanan/adb274ecea6467d31a45aa6058067917

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

varun created this revision.Jun 19 2024, 7:52 PM

varun held this revision as a draft.

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJun 19 2024, 7:52 PM

Harbormaster completed remote builds in B29805: Diff 41537.Jun 19 2024, 8:09 PM

varun published this revision for review.Jun 20 2024, 9:25 AM

varun edited the test plan for this revision. (Show Details)

What if a user is successfully registered/logged in to Identity but for some reason markPrekeysAsPublished fails? Wondering, if shouldn't catch and ignore errors here and make sure later PrekeysHandler re-uploads the same keys and marks as published (since marking as published is used only for rotation).

I think existing logic should handle this because it uploads not published keys and marks as published (not sure about native but logic should match).

It might be strange if a user sees that registration fails, but after attempting sees that the user already exists because registration worked but marking keys as published failed.

Curious for @marcin's perspective.

Accepting to unblock and I don't have a strong opinion on which approach is better, only pointing out something worth considering.

In D12501#355136, @kamil wrote:

What if a user is successfully registered/logged in to Identity but for some reason markPrekeysAsPublished fails? Wondering, if shouldn't catch and ignore errors here and make sure later PrekeysHandler re-uploads the same keys and marks as published (since marking as published is used only for rotation).

I think existing logic should handle this because it uploads not published keys and marks as published (not sure about native but logic should match).

It might be strange if a user sees that registration fails, but after attempting sees that the user already exists because registration worked but marking keys as published failed.

Curious for @marcin's perspective.

Accepting to unblock and I don't have a strong opinion on which approach is better, only pointing out something worth considering.

fair point. i'm curious what @marcin thinks as well

marcin accepted this revision.Jun 24 2024, 6:52 AM

This revision is now accepted and ready to land.Jun 24 2024, 6:52 AM

I have concern that marking prekeys as published after uploading to identity comes with the risk of delaying next prekey rotation in case app crashes after uploading but before marking as published. I am wondering if letting the prekeys to be in circulation for longer than expected is a security concern. Accepting to unblock identity release. If my reasoning is correct we can just change the order later.

It might be strange if a user sees that registration fails, but after attempting sees that the user already exists because registration worked but marking keys as published failed.

I think this is a good point. I'm going to catch and ignore errors from markPrekeysAsPublished

In D12501#355516, @marcin wrote:

I have concern that marking prekeys as published after uploading to identity comes with the risk of delaying next prekey rotation in case app crashes after uploading but before marking as published. I am wondering if letting the prekeys to be in circulation for longer than expected is a security concern. Accepting to unblock identity release. If my reasoning is correct we can just change the order later.

talked to @marcin about this, decided we can discuss with @ashoat when he's back