-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Swap libsignal-protocol dependency with libsignal-client #83
Conversation
73a9ce5
to
c4110d2
Compare
It's a bit of a rewrite, so I'm pushing broken commits as I pass over code. In the end I'll squash the |
Important detail. |
No, relicensing to AGPL is not needed and can't be done without explicit agreement of everyone who has contributed to this repo:
https://www.gnu.org/licenses/license-compatibility.html Considering only 10 people have contributed code to this repository so far, I think relicensing to AGPL is feasible. I think it would be advisable to avoid confusion. Also, if we want to copy any of this code to send upstream to libsignal-client, they might want it to be AGPL. |
So either we relicense the whole thing, or we go the complicated route as I took with Whisperfish: old source is GPLv3 and new source is AGPLv3. If we don't get the permission of them all, that's what we should do. I'll make an announcement on issue #72. Either way, the whole of libsignal-service-rs will fall under AGPLv3, since it's a combination of (A)GPLv3 and AGPLv3 sources. And wrt. sending to libsignal-client: that's a whole other story, because they require a CLA. AFAICT, for moving code, only my code and @gferon's code is being moved on the short term. |
926d062
to
53dddee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like we're not far off! :-)
libsignal-service/Cargo.toml
Outdated
@@ -7,8 +7,8 @@ license = "GPLv3" | |||
readme = "../README.md" | |||
|
|||
[dependencies] | |||
libsignal-protocol = { git = "https://github.com/Michael-F-Bryan/libsignal-protocol-rs" } | |||
zkgroup = { git = "https://github.com/signalapp/zkgroup" } | |||
libsignal-protocol = { git = "https://github.com/whisperfish/libsignal-client", branch = "make-error-sync" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we wait on signalapp/libsignal#279?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we will wait.
libsignal-service/src/sender.rs
Outdated
if let Some(ref uuid) = recipient.uuid { | ||
self.cipher.store_context.delete_session( | ||
&libsignal_protocol::Address::new( | ||
uuid.to_string(), | ||
*extra_device_id, | ||
), | ||
)?; | ||
if let Some(_uuid) = recipient.uuid { | ||
unimplemented!("deleting session: unimplemented following the switch to the Rust version of libsignal-protocol"); | ||
} | ||
if let Some(e164) = recipient.e164() { | ||
self.cipher.store_context.delete_session( | ||
&libsignal_protocol::Address::new( | ||
&e164, | ||
*extra_device_id, | ||
), | ||
)?; | ||
if let Some(_e164) = recipient.e164() { | ||
unimplemented!("deleting session: unimplemented following the switch to the Rust version of libsignal-protocol"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tracking issue for this? This definitely affects Whisperfish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird, I swear I reverted that. I added SessionStoreExt
so that's possible again.
libsignal-service/src/sender.rs
Outdated
if let Some(ref uuid) = recipient.uuid { | ||
self.cipher.store_context.delete_session( | ||
&libsignal_protocol::Address::new( | ||
uuid.to_string(), | ||
*extra_device_id, | ||
), | ||
)?; | ||
if let Some(_uuid) = recipient.uuid { | ||
unimplemented!("deleting session: unimplemented following the switch to the Rust version of libsignal-protocol"); | ||
} | ||
if let Some(e164) = recipient.e164() { | ||
self.cipher.store_context.delete_session( | ||
&libsignal_protocol::Address::new( | ||
&e164, | ||
*extra_device_id, | ||
), | ||
)?; | ||
if let Some(_e164) = recipient.e164() { | ||
unimplemented!("deleting session: unimplemented following the switch to the Rust version of libsignal-protocol"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird, I swear I reverted that. I added SessionStoreExt
so that's possible again.
Just FYI that's exactly the reason why they have the CLA. They want to own the code so they can release it under a different license or sell it as closed source (like they did to Facebook or so) |
Yes, and that's exactly the reason that I'm against CLA's too. In my opinion, the person that wrote the code should have the right to choose whether a change of license is okay with them, like in this case :-) But let's stick to topic, we're very close having |
Maybe you mean |
potato tomato! |
Merge upstream/master into libsignal-client branch
@rubdos I think this is ready for a second (and hopefully final?) look! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly clippy, there are still some unresolved discussions, @gferon.
Generally looking good, let's catch everything else in prod ;p
@@ -78,9 +75,12 @@ impl<Service: PushService> AccountManager<Service> { | |||
/// Equivalent to Java's RefreshPreKeysJob | |||
/// | |||
/// Returns the next pre-key offset and next signed pre-key offset as a tuple. | |||
pub async fn update_pre_key_bundle( | |||
pub async fn update_pre_key_bundle<R: rand::Rng + rand::CryptoRng>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pub async fn update_pre_key_bundle<R: rand::Rng + rand::CryptoRng>( | |
#[allow(clippy::too_many_arguments)] // See issue https://github.com/Michael-F-Bryan/libsignal-service-rs/issues/92 | |
pub async fn update_pre_key_bundle<R: rand::Rng + rand::CryptoRng>( |
signed_pre_key: signed_pre_key.into(), | ||
identity_key: identity_key_pair.public(), | ||
signed_pre_key: signed_prekey_record.try_into()?, | ||
identity_key: identity_key_pair.public_key().clone(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
identity_key: identity_key_pair.public_key().clone(), | |
identity_key: identity_key_pair.public_key(), |
libsignal-service/src/sender.rs
Outdated
if !self | ||
.cipher | ||
.store_context | ||
.contains_session(&recipient_address)? | ||
.session_store | ||
.load_session(&recipient_address, None) | ||
.await? | ||
.is_some() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion feature on GH doesn't let me, but this should be self.[..].is_none()
instead, apparently
Fixes #72