Deduplicate the logic behind PreKeyStore and SignedPreKeyStore

[Michael-F-Bryan]

These two vtables are identical, modulo the signed_ prefix. For now I've done a straight copy/paste, but it'd be nice if we can unify the two and skip the extra effort.

Note: The original FIXME for this was removed in 11d22b9

[shekohex]

Maybe using a macro to generate the code for both?

gen_key_store!(PreKeyStore);
gen_key_store!(SignedPreKeyStore);

What do you think about this?

[Michael-F-Bryan]

I'm not sure how it'd work out. The sys::signal_protocol_pre_key_store struct has methods like load_pre_key whereas on sys::signal_protocol_signed_pre_key_store it's called load_signed_pre_key. When calling the macro we'd need to pass all those function names as well, so the macro invocation would be really big and noisy 😞

[shekohex]

In fact, i don't love the notion of
https://github.com/Michael-F-Bryan/libsignal-protocol-rs/blob/4a151a0e44c8ae83bb7be42c709d2296d1687f37/libsignal-protocol/src/pre_key_store.rs#L7-L12

as we are working on that implementation, we need to provide a high-level ergonomic rust-ish api for that lib, because eventually i would love to see it written in pure rust, see (shekohex/libsignal-protocol-rs#1), but as i'm working, i had realized that it's hard to achieve that from the start, so making a c-binding at first makes more sense, but providing an idiomatic rust api is MUST.

So Here i would suggest to make this api something like:

pub trait PreKeyStore {
  fn load(&self, pre_key_id: u32) -> Result<&PreKeyRecord, InternalError>;
  fn store(&mut self, pre_key_id: u32, record: &PreKeyRecord) -> Result<(), InternalError>;
  fn contain(&self, pre_key_id: u32) -> bool;
  fn remove(&mut self, pre_key_id: u32) -> Result<(), InternalError>;
}

What do you think ?

[Michael-F-Bryan]

Ooh, I didn't notice you'd already worked with libsignal-protocol before! I agree that rewriting it in Rust could provide many benefits, but crypto code is hard to get right and we already have a really good quality implementation in C.

we need to provide a high-level ergonomic rust-ish api for that lib, ... i had realized that it's hard to achieve that from the start, so making a c-binding at first makes more sense, but providing an idiomatic rust api is MUST.

I agree that having an idiomatic API is a must-have, although we may need to do some iterating as the crate gets used in real-world scenarios to decide what exactly idiomatic looks like.

In this case, even though something like remove() sounds like it should mutate the PreKeyStore, you also have to remember it's behind a shared reference with C having arbitrary many pointers to it. And shared references take &self. If the user needs to do mutation (i.e. because the keys are stored in memory) then I'd say it's their responsibility to add the necessary synchronization.

The reason I originally chose to use the Read and Write traits for storing (Signed)PreKeys is because the Building A Session section in the original readme says:

A libsignal-protocol-c client needs to implement four data store callback interfaces: ... These will manage loading and storing of identity, prekeys, signed prekeys, and session state.

These callback interfaces are designed such that implementations should treat all data flowing through them as opaque binary blobs.

The user definitely needs to be able to use a PreKey as a first class citizen (have getters/setters for accessing properties, methods for operations, etc.) when doing top-level operations, but it sounds like the PreKeyStore and SessionStore are intended to be a mechanism used internally by the sys::signal_protocol_store_context for accessing opaque data via an ID.

---

I hope it doesn't sound like I'm getting defensive here, I'm just trying to explain the logic behind doing funny-looking things like fn store(&self) and using &mut dyn Write. There are several places in the current API which feel a bit funny (e.g. Crypto not providing a compile-time mechanism to make sure hmac_sha256_init/update/final/cleanup() can only be executed in the correct order), so I'm keen to try alternatives if we can think up better ways of doing things.

[shekohex]

That's Seems fair to me, the main reason for writing these high-level api, when i was working on native and pure rust implantation was by seeing how the java implementation doing that, so java has other native implementation with very high-level api, it has the same api like the c is, but with other cool things like we did in the rust side (eg. they make all the store thing as interfaces and they using builder pattern everywhere, with getters and setters.), when i stooped working in rust implementation ( the lack of my free time), i realized that maybe it would be nice to make the ffi bindgen first, but my knowledge of c <-> rust isn't enough to make this happen, so i made a poll on twitter asking rust community about that case, that's where i found your repo :)