Skip to content
This repository has been archived by the owner on Jan 30, 2023. It is now read-only.

replace msgpack #41

Closed
rhelmer opened this issue Sep 18, 2018 · 9 comments
Closed

replace msgpack #41

rhelmer opened this issue Sep 18, 2018 · 9 comments
Labels
enhancement New feature or request

Comments

@rhelmer
Copy link
Contributor

rhelmer commented Sep 18, 2018

We've found at least one drawback to msgpack so far:
https://bugzilla.mozilla.org/show_bug.cgi?id=1486478

I think we could go with something simpler/smaller since libprio only uses it internally and not on the wire.

One of my colleagues suggested we could potentially use Rust for this, since doing this sort of thing is super easy and can be done in safe rust code, you just derive(Serialize, Deserialize) on your structs for example: https://github.com/mozilla/sccache/blob/master/src/protocol.rs

This could be done as a small library which exposes a C API, however I do worry a bit about whether that might expose us to timing or other side-channel since the codegen for Rust is different than C and (presumably) not as well understood by the crypto community at large.

In any case, we could probably go with something simpler/smaller than msgpack, and maybe not even have to write it :)

@henrycg
Copy link
Collaborator

henrycg commented Sep 22, 2018

How about using libmpack instead? It's written in C89 and doesn't apparently use any OS-specific features, so it should compile with MSVC and on aarch64. This would also let us use libprio as a standalone C library.

If this seems like a plausible option, I could check to see how difficult it would be to port the serialization code over to libmpack.

@rhelmer
Copy link
Contributor Author

rhelmer commented Sep 22, 2018

I think @froydnj already figured out the bug I linked to, so probably not a ton of urgency if msgpack is easy to fix.

libmpack looks pretty cool as a replacement though! I think it's too ambitious to try to use Rust for this now, especially if we want libprio to work old-ish platforms and compilers etc (like MSVC in issue 17).

I think it'd be a good thing generally for libprio to work on as many active OSes and compilers as possible, even if Firefox doesn't need it.

@rhelmer
Copy link
Contributor Author

rhelmer commented Sep 22, 2018

@henrycg So I noticed libmpack includes RPC support too... do you think it'd be useful for libprio to provide a basic API for cross-server communication when we actually have A and B servers up?

@mreid was interested in how we'd do this part.

I'd imagine instead of doing all the networking code and such in C, we could easily use some higher-level/safer language to do it, e.g. a Python service that embeds libprio using the library @acmiyaguchi has started in python-libprio.

libprio would just be responsible for un/marshaling the data structures it wants into the RPC wire format, and whatever is embedding it can figure out how to move the packets around.

@rhelmer
Copy link
Contributor Author

rhelmer commented Sep 23, 2018

What about CBOR? There's an RFC for it, and I think it's similar to MessagePack in that it is inspired by JSON but a compact binary representation.

It's used in a few places in Firefox, although not in a way that's particularly useful to libprio sadly:

libprio would probably instead want a plain C implementation. We could at least land it in a common place in mozilla-central so other folks could use it if they want, although I wouldn't fault people for preferring the C++ implementation when using it from C++ code.

I don't have any personal preference for MessagePack via libmpack vs. CBOR, but if the latter is the way things are going generally might be nice for interop later.

@rhelmer
Copy link
Contributor Author

rhelmer commented Sep 23, 2018

libprio would probably instead want a plain C implementation.

Intel's TinyCBOR looks like the most promising from a quick skim.

@henrycg
Copy link
Collaborator

henrycg commented Sep 24, 2018

Yes, TinyCBOR looks good to me. Even though it's compiled as C99 code, it looks like it will compile as C90 as well and also works with MSVC. It might take me a few days to get to this, but I can work on porting the stuff in serial.c over to TinyCBOR.

Is the MIT license okay for Firefox?

@henrycg
Copy link
Collaborator

henrycg commented Sep 24, 2018

By the way, if we make this change, we will break backwards compatibility: a Prio server using the CBOR format won't be able to read Prio client packets encoded using the msgpack implementation.

Should we hold off on this until we have v0 of the server-side code working? Or are we okay with not being able to decode the first couple of weeks of data?

@rhelmer
Copy link
Contributor Author

rhelmer commented Sep 24, 2018

Oh yeah, there's no rush on this at all. I don't think there's a pressing need to switch off msgpack. When I filed this it was unsure if we could patch upstream to support the new arch we needed, and if we submitted a patch if it'd be accepted etc. but that worked out fine.

Let's talk time to talk and think about it, no imminent action needed. I think as far as Firefox is concerned we could easily just switch to a new format along with a new version of Firefox, and use the appropriate processor on the server-side.

However there's no reason to make any extra work for anyone right now :)

@rhelmer rhelmer added the enhancement New feature or request label Oct 12, 2018
@henrycg
Copy link
Collaborator

henrycg commented Feb 18, 2021

Since this library is deprecated in favor of libprio-s, I'm closing this issue.

@henrycg henrycg closed this as completed Feb 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants