Merge libsignal-jni #3

jack-signal · 2020-10-15T20:30:55Z

755cc05 is current HEAD of libsignal-jni

Makes the diff against libsignal-protocol-java easier to read

Initial commit

Add SenderKeyStore handling

Also fix isTrustedIdentity which was still just a stub Fix some bugs around error reporting and choice of exception

Add storage layer interactions, session encryption/decryption

The current uppercase leading char is unusual for Java, and having a `native` prefix makes it more clear what is happening on the Java side.

Add 'native' prefix to all JNI functions

Allows simplifying some of the Java-side logic

Add offset argument for PublicKey deserialization

Cannot build due to needing to reference the private libsignal-protocol-rust

Add CI using Github actions

Add ECPublicKey comparison

(Namely the exception types and the callback which failed)

Update with Context (unused) and capture more debug info

jack-signal · 2020-10-15T20:58:43Z

Maybe we want this in rust/ffi/jni instead? IDK

jrose-signal

Several questions and comments but of course this code is already being tested from its original repo.

(also discussing the directory layout question elsewhere, don't think it has to block this)

jrose-signal · 2020-10-15T22:45:07Z

rust/jni/src/util.rs

+    let data: &[u8] = data.as_ref();
+    let out = env.new_byte_array(data.len() as i32)?;
+    let buf: Vec<i8> = data.iter().map(|i| *i as i8).collect();
+    env.set_byte_array_region(out, 0, buf.as_slice())?;


Looks like we can use let out = env.byte_array_from_slice(data.as_ref())?; for this.

jrose-signal · 2020-10-15T22:55:10Z

rust/jni/src/util.rs

+                let message: String = env.get_string(JString::from(o))?.into();
+                return Err(SignalJniError::Signal(
+                    SignalProtocolError::ApplicationCallbackThrewException(
+                        fn_name, exn_type, message,


(non-blocking) Can we keep around the entire exception here instead of just the name and message? (cf. the "cause" field in java.lang.Exception).

Yes that would definitely make debugging simpler. #5

jrose-signal · 2020-10-15T23:04:57Z

rust/jni/src/util.rs

+
+pub fn jint_to_u32(v: jint) -> Result<u32, SignalJniError> {
+    if v < 0 {
+        return Err(SignalJniError::IntegerOverflow(format!("{} to u32", v)));


Are you sure we don't use all bits of any ints or longs? If they're opaque numeric identifiers it'd be better to reinterpret them than fail.

jrose-signal · 2020-10-15T23:14:59Z

rust/jni/src/lib.rs

+) -> ObjectHandle {
+    run_ffi_safe(&env, || {
+        let offset = jint_to_u32(offset)? as usize;
+        let data = env.convert_byte_array(data)?;


(non-blocking) I feel like there ought to be a zero-copy version of this as well (cf. get_auto_byte_array_elements) but I see it's non-trivial, what with the mismatched signedness between u8 and jbyte.

jrose-signal · 2020-10-15T23:19:58Z

rust/jni/src/lib.rs

+    _class: JClass,
+) -> ObjectHandle {
+    run_ffi_safe(&env, || {
+        let mut rng = rand::rngs::OsRng;


This is in libsignal-ffi as well, but why OsRng over ThreadRng? They're both listed as cryptographically secure, and OsRng will generally be slower since it's calling out to the OS every time instead of just periodically.

ThreadRng is probably fine. It needed to be avoided until recently because before rand 0.7 it used HC-128 which is vulnerable to side channel attacks (https://cr.yp.to/streamciphers/leaks.html). However it appears that rand 0.7 has switched to ChaCha20 which should be fine. #6

We should probably have a way of toggling this across all crates.

jrose-signal · 2020-10-15T23:49:02Z

rust/jni/src/lib.rs

+jni_fn_get_new_boxed_optional_obj!(Java_org_whispersystems_libsignal_groups_state_SenderKeyState_nativeGetSigningKeyPrivate(PrivateKey) from SenderKeyState,
+                                   |sks: &SenderKeyState| sks.signing_key_private());
+
+fn sender_key_name_to_jobject<'a>(


(non-blocking) Maybe worth breaking the stores and the supporting logic for them out into another file?

jrose-signal · 2020-10-15T23:52:00Z

rust/jni/src/lib.rs

+    ];
+
+    let sender_key_name_ctor_sig = "(Ljava/lang/String;Ljava/lang/String;I)V";
+    let sender_key_name_jobject = env.new_object(


Why make SenderKeyName et al self-contained Java objects instead of wrappers around native handles like the others? (Does this predate adding clone() to all the types, so they can be escaped more easily?)

SenderKeyName is a wrapper around a handle like the others. But the Java callback is expecting to be passed not a handle but an actual Java object.

Hm, is there no way to make an object from the handle you already have, rather than asking it to make a new one? I guess that would be a more intrusive change.

For most types in Java I added a constructor which takes a long (aka ObjectHandle) which takes ownership of said handle. I'm not sure why I didn't do that here, probably just an oversight.

jrose-signal · 2020-10-15T23:53:23Z

rust/jni/src/lib.rs

+            "org/whispersystems/libsignal/IdentityKey",
+            identity.serialize().as_ref(),
+        )?;
+        let callback_sig = "(Lorg/whispersystems/libsignal/SignalProtocolAddress;Lorg/whispersystems/libsignal/IdentityKey;)Z";


(non-blocking) Probably worth making a macro to build these signatures (later).

Absolutely agree. Unlike the function names, this probably doesn't even require a proc macro.

jrose-signal · 2020-10-15T23:55:49Z

rust/jni/src/lib.rs

+    }
+}
+
+fn create_identity_store<'a>(


Seems redundant with the new method, but is it a convention to do error checking before that?

Yes I think this is just an artifact of how the code evolved. It would be better to move these checks to new. #7

jrose-signal · 2020-10-15T23:59:44Z

rust/jni/src/lib.rs

+    })
+}
+
+// The following are just exposed to make it possible to retain some of the Java tests:


This could actually be useful for the iOS client too, but the alternative would just be shoving this logic up into the Java layer.

IIRC the reason not to do so was because nothing outside of tests, and the core protocol library itself, need to know about the {Alice,Bob}SignalProtocolParameters types. Really the tests could have just been removed from the Java side but I wanted to minimize the diff there to make review simpler. I would rather we have higher level tests (that verify actual behavior as expected by the clients) rather than what are effectively unit tests of protocol level details.

Use 'bool' for several more Boolean results

GitHub: run tests on PRs

jack-signal · 2020-10-16T22:36:25Z

Moved to rust/bridge/jni to match ffi. I believe all comments are captured in tickets now so I think this is good to merge and we can proceed from there.

…leases-url Option to define github account for dowloading release artifacts

jack-signal added 24 commits August 11, 2020 18:20

Initialize repository

b158ff7

Initial commit

b03ce6b

Rename to org.whispersystems

8e9d035

Makes the diff against libsignal-protocol-java easier to read

Add key generation JNI funcs

420ba84

Merge pull request #1 from signalapp/jack/jni

69b6769

Initial commit

Add SenderKeyStore handling

32a6933

Merge pull request #2 from signalapp/jack/sender-key-store

003c689

Add SenderKeyStore handling

Stubs for all the storage interfaces

f2d5978

PreKey and SignedPreKey storage

d1f421c

Add the other storage layer types

7badc34

Remove unused endpoints

380356f

Also fix isTrustedIdentity which was still just a stub Fix some bugs around error reporting and choice of exception

Merge pull request #3 from signalapp/jack/storage-layer

1c4ffa3

Add storage layer interactions, session encryption/decryption

add 'native' prefix to all JNI functions

944bc99

The current uppercase leading char is unusual for Java, and having a `native` prefix makes it more clear what is happening on the Java side.

Merge pull request #4 from signalapp/jack/add-native-prefix

43e9ca9

Add 'native' prefix to all JNI functions

Add offset argument for PublicKey deserialization

d72b126

Allows simplifying some of the Java-side logic

Merge pull request #5 from signalapp/jack/decode-with-offset

a378dca

Add offset argument for PublicKey deserialization

Update libsignal-protocol-rust reference

eee7f6c

Add CI using Github actions

4935c95

Cannot build due to needing to reference the private libsignal-protocol-rust

cargo fmt

a64c893

Merge pull request #6 from signalapp/jack/github-actions

3295172

Add CI using Github actions

Add ECPublicKey comparison

32b5e4a

Merge pull request #7 from signalapp/jack/eckey-compare

54d81d6

Add ECPublicKey comparison

Update with Context (unused) and capture more debug info

a854bf7

(Namely the exception types and the callback which failed)

Merge pull request #8 from signalapp/jack/add-context

755cc05

Update with Context (unused) and capture more debug info

jack-signal requested review from ehrenkret-signal and jrose-signal October 15, 2020 20:30

jrose-signal reviewed Oct 16, 2020

View reviewed changes

jrose-signal added a commit that referenced this pull request Oct 16, 2020

Merge pull request #3 from signalapp/jrose/c99-bool-again

ea0d4ba

Use 'bool' for several more Boolean results

jrose-signal added a commit that referenced this pull request Oct 16, 2020

Merge pull request #3 from signalapp/jrose/pr-testing

fa5657c

GitHub: run tests on PRs

jack-signal added 3 commits October 16, 2020 18:17

Move libsignal-jni to rust/bridge/jni

e584064

Merge libsignal-jni history into libsignal-client

52ae600

Update paths and workspace config

3d24f94

jack-signal force-pushed the jack/merge-jni branch from 5c36459 to 3d24f94 Compare October 16, 2020 22:26

Add license headers

c1331eb

jrose-signal approved these changes Oct 16, 2020

View reviewed changes

jack-signal merged commit c0b6076 into master Oct 19, 2020

jack-signal deleted the jack/merge-jni branch October 19, 2020 20:53

expiringplatform mentioned this pull request Feb 23, 2023

[Rust][Fuzzing] ERROR libsignal_protocol::session_cipher No valid session for recipient #514

Open

johanvos pushed a commit to johanvos/libsignal-client that referenced this pull request May 16, 2023

Merge pull request signalapp#3 from tiainen/gradle-download-github-re…

cdf6fa4

…leases-url Option to define github account for dowloading release artifacts

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge libsignal-jni #3

Merge libsignal-jni #3

jack-signal commented Oct 15, 2020

jack-signal commented Oct 15, 2020

jrose-signal left a comment

jrose-signal Oct 15, 2020

jack-signal Oct 16, 2020

jrose-signal Oct 15, 2020

jack-signal Oct 16, 2020

jrose-signal Oct 15, 2020

jrose-signal Oct 15, 2020

jrose-signal Oct 15, 2020

jack-signal Oct 16, 2020

jrose-signal Oct 15, 2020

jrose-signal Oct 15, 2020

jack-signal Oct 16, 2020

jrose-signal Oct 16, 2020

jack-signal Oct 16, 2020

jrose-signal Oct 15, 2020

jack-signal Oct 16, 2020

jack-signal Oct 16, 2020

jrose-signal Oct 15, 2020

jack-signal Oct 16, 2020

jrose-signal Oct 15, 2020

jack-signal Oct 16, 2020

jack-signal commented Oct 16, 2020 •

edited

Loading

Merge libsignal-jni #3

Merge libsignal-jni #3

Conversation

jack-signal commented Oct 15, 2020

jack-signal commented Oct 15, 2020

jrose-signal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jack-signal commented Oct 16, 2020 • edited Loading

jack-signal commented Oct 16, 2020 •

edited

Loading