search

freedomlayer / offset

Offset payment engine
https://www.offsetcredit.org
Other
163 stars 20 forks source link

Upgrade to latest version of the ring crate #167

Closed realcr closed 4 years ago

realcr commented 5 years ago

We currently use an old version of the ring crate, due to problems with deterministic testing. (See https://github.com/freedomlayer/offst/issues/64). It seems like other parts of the ring API has changed, and some work might be needed to use the latest version of ring. Most of the modifications should be at the offst-crypto crate. If no other solutions comes up, we might need to give up the determinism in some of our cryptographic unit tests (For example, in offst-secure-channel). Relevant issue in the ring respository: https://github.com/briansmith/ring/issues/661

pzmarzly commented 5 years ago

For now, I tried updating offst in:

Approaches 1 and 2 probably could work if SecureRandom was Clone - issue at https://github.com/briansmith/ring/issues/808

realcr commented 5 years ago

@pzmarzly : I think that this issue is something we can still make progress on while the new atomic payments redesign is being developed.

We had lots of trouble with Ring over the last year. The API keeps changing and it becomes less and less flexible to use. One of the main problems we get by using Ring is that we can't write deterministic tests. Currently instead of having Ring as a normal dependency we depend on a fork of Ring we maintain ourselves. This also complicates our build scripts in the CI.

I am not sure if things are going to get any better for us with Ring, so I thought that we might be able to use another library. We currently rely on the following cryptographic primitives:

In the future we will probably also need bcrypt.

I checked a few crates, and one that looked reasonably maintained is sodiumoxide. I searched for other Rust cryptography projects but I couldn't find anything else that looks like it is well maintained, tested and written by people that know cryptography.

If we choose to do this, I think we might be able to make a gradual transition. We can add Sodiumoxide as a dependency, and start porting submodules inside offst-crypto one by one to use from using Ring to Sodiumoxide. The external interface of offst-crypto can probably stay exactly the same until we finish all the porting.

I'm still not sure if this is a good idea, so I wanted to know your opinion. In addition, if you have other ideas or know of other well maintained cryptographic libraries we can use I would also like to know.

pzmarzly commented 5 years ago

As you may know, I've been trying to do get rid of Offst crypto + net modules. I was writing a crate (codename fes) that could offer encryption, DH and signatures. I tried to make it integrate well with protocol and romio, have a hard-to-misuse API, and just require that one side (or both) have other's public key (which is also used for signatures). It still needs some work, but for now I uploaded what I have: repo.

Furthermore, while building it I tried using many crates, and finally ended up with:

x25519-dalek = "0.5.2"
ed25519-dalek = "0.9.1"
rand = "0.6.5"
sha2 = "0.8.0"
chacha20-poly1305-aead = "0.1.2"

All of these are latest version. They have their gimmicks (e.g. x25519-dalek API is not that similar to ed25519-dalek despite coming from the same author), but seem to be working (especially when you slap abstraction layer like fes on top of them).

Sadly, some of them haven't been updated lately. That doesn't necessarily mean they are unmaintained, but I hope, just in case, that crates with such small scope of features don't contain critical bugs. Most of sodiumoxide commits seem to be related to FFI improvements.

Both sodiumoxide and dalek's libraries require C compiler (dalek needs it for clear_on_drop), though dalek is thread-safe (while sodiumoxide requires calling init()).

I don't know what the right approach is, so I will let you decide. Personally, I don't think this is urgent (other things to be done include having node connect to more than 1 index server at a time). I'd prefer to try finishing fes, testing it and hopefully integrating it with Offst.

pzmarzly commented 5 years ago

SystemRandom implements Clone now but it doesn't help as much as I thought. It would probably be possible to get rid of arc_rng (RngContainer), and make a custom trait for it, but ring methods still expect SecureRandom. So the comment about 3 methods is wrong.

I dropped the work on fes (crypto library), but it could be completed one day (perhaps I will come back to it when I work on decentralized systems again, for now I moved to full-stack freelancing).

BTW congratulations on getting to HN front page.

realcr commented 5 years ago

Thanks! I think I'm going to wait a bit with the the cryptography library. Either things get better with ring or we probably change it to something else.

for now I moved to full-stack freelancing

Great to hear! If you ever need a recommendation for your work I will be more than glad to help.

BTW congratulations on getting to HN front page.

You have a big part in this. Thanks!

realcr commented 4 years ago

Solved by https://github.com/freedomlayer/offset/pull/300