Added support for fingerprinting key lists

dkonigsberg commented 7 years ago

Hey, I just wanted to follow up on this. Why was this pull request submitted, and then immediately closed? This feature is actually the next item on the to-do list for this library, so it definitely caught my eye.

WhichKatieDid commented 7 years ago

Oh! I was trying to make a pull request to my own fork first, and have not had time to clean up the code and prepare this for submission. Reopening for comments/fixes:

Not sure if the new function names meet the naming conventions elsewhere.
Also likely that keys are being leaked
Scannable fingerprint only captures the first key of each, since the structure is not designed for lists.

dkonigsberg commented 7 years ago

Okay, so its really still a work-in-progress...

There are a few things I would need this code to do, in addition to the above-mentioned cleanup...

It needs to support both versions of the scannable fingerprint (libsignal-protocol-java made a change to drop the stable identifier and bump the version number. I want this version to be capable of generating/comparing both the old and new formats)
It needs unit test coverage (tests/test_fingerprint.c) to verify that it works with itself, and that its compatible with the version in libsignal-protocol-java

I'm not sure whether or not some refactoring is in order (the Java version has been extensively refactored, but in a way that may not necessarily make sense for the C version).

Just let me know what your plans are, so I know whether to implement this myself, use your commit as a starting point for further work, or wait for you to complete more of the implementation. (You'll also need to sign the CLA, see above, before I can actually accept any of your code.)

Mattmlm commented 7 years ago

@dkonigsberg This PR is primarily intended as a starting point for further work. On our side, we only have a requirement for the displayable fingerprint, and not the scannable fingerprint. As such, in the mean time, we only plan to put in the work to get that part working.

As I'm finding bugs in our current implementation of the displayable fingerprint, I'm going to continue working on this, and we may end up writing unit test coverage to ensure that it works properly. Thanks.

dkonigsberg commented 7 years ago

Just wanted to let you know that I finished my own implementation of this feature, which is a combination of key-list fingerprints and scannable fingerprint v1 support. Its now checked in, so please review and let me know if it looks good to you.

I was unfortunately unable to use your code because you hadn't yet signed the CLA, and there doesn't appear to be much overlap between how I did it and how you did it.

WhichKatieDid commented 7 years ago

Apologies for not responding sooner, but frustratingly enough, I still have yet to get clearance to sign the CLA. A cleaner version getting in with scannable fingerprint is preferable, anyways.

My implementation was a quick and dirty hack based on what C code was originally there, combined with the logic of what Android did for key lists. So no overlap, and they have the same general algorithm only in matching Android's implementation.

Your implementation is much nicer. Mind if I close and withdraw this pull request?

dkonigsberg commented 7 years ago

Sure, go ahead and withdraw it.

signalapp / libsignal-protocol-c

Added support for fingerprinting key lists #73