Implement PNI

[rubdos]

Fix #206, continuation of #275

TODO:

• Update pre key bundle should take into account last-resort Kyber pre keys
• Linking manager should use the account manager for pre key generation
• Send the actual PNI distribution message
• Use the supplied identity key pair in the refactored linking code
• Consider refactoring NewDeviceRegistration to use IdentityKey instead of PublicKey or even straight IdentityKeyPair. Strong types ftw.

Maybe TODO:

• Phone number sharing is something that's stored on the profile. Add to the profile cipher.

[rubdos]

@gferon: in the linking fix #271, you set some Kyber last resort keys:

        let aci_pq_last_resort_pre_key =
            generate_last_resort_kyber_key(aci_store, &aci_key_pair).await?;
        let pni_pq_last_resort_pre_key =
            generate_last_resort_kyber_key(pni_store, &pni_key_pair).await?;

The problem is that the storage does not realize this is a last resort key, and we should probably make sure to tag it as such.

Second, this pre key generation code is duplicate with AccountManager, we might want to clean that up.

[gferon]

@gferon: in the linking fix #271, you set some Kyber last resort keys:

        let aci_pq_last_resort_pre_key =
            generate_last_resort_kyber_key(aci_store, &aci_key_pair).await?;
        let pni_pq_last_resort_pre_key =
            generate_last_resort_kyber_key(pni_store, &pni_key_pair).await?;

The problem is that the storage does not realize this is a last resort key, and we should probably make sure to tag it as such.

Second, this pre key generation code is duplicate with AccountManager, we might want to clean that up.

I remember adding a comment about this but didn't see some bad side effects 😅 and decided to go forward with the fix since everything was broken. Thanks for that!

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 29 lines in your changes are missing coverage. Please review.

Project coverage is 3.34%. Comparing base (a2e7540) to head (d6a6fc2).
Report is 11 commits behind head on main.

❗ Current head d6a6fc2 differs from pull request most recent head 4255520. Consider uploading reports for the commit 4255520 to get more accurate results

Files	Patch %	Lines
libsignal-service/src/push_service.rs	0.00%	24 Missing ⚠️
libsignal-service/src/pre_keys.rs	0.00%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #285       +/-   ##
==========================================
- Coverage   17.12%   3.34%   -13.78%     
==========================================
  Files          38      37        -1     
  Lines        3078    2898      -180     
==========================================
- Hits          527      97      -430     
- Misses       2551    2801      +250     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[rubdos]

1c3d6dd refactors this, but is currently incorrect, as it relies on the AccountManager having knowledge of the newly provided identity key pairs.

EDIT c2ceb80 should fix that.

[rubdos]

Good for a first look. We'll test it thoroughly (as thoroughly as this complex thing can get tested...) with Whisperfish before getting this in.

[rubdos]

Since messages are always sent using the account identity^{[citation needed]}, we could instead pass the Aci and Pni stores separately into the MessageSender, and instantiate the ServiceCipher internally? That could reduce the number of arguments, because we won't have to pass the aci_identity, pni_identity nor the cipher anymore.

[rubdos]

Imagine. This works.

[gferon]

Amazing, I'll integrate and test it in presage today!

[rubdos]

Amazing, I'll integrate and test it in presage today!

Quite a bit of work on the caller side, as usual, you can check out WF:
https://gitlab.com/whisperfish/whisperfish/-/merge_requests/550

[direc85]

I've been using this with Whisperfish for a day or two now, and there haven't been anything weird to report about (after the initial shenanigans)!

[gferon]

Just started the integration with presage, may I push small adjustment commits directly on this branch?

[direc85]

I guess so, further fixes are always welcome. I'd still prefer the trial-and-error part happening in a fork or local check-out. Having said that, me and Ruben did exactly that a few days back 😅 Some final cleanup may take place regardless.

[rubdos]

Just started the integration with presage, may I push small adjustment commits directly on this branch?

As long as you don't squash anything, I think that'd be fine.

[rubdos]

@gferon I'm going to take the liberty to pull this in; @direc85 and I have been daily-driving this. It's not complete, but from here it's all incremental anyway.

[gferon]

Go for it, I have a bunch of changes that I'll try to commit in separate changes, since they are mostly quality of life improvements and I went a little bit extra with my changes 😂