search

datrs / hypercore

Secure, distributed, append-only log
https://docs.rs/hypercore
Apache License 2.0
333 stars 37 forks source link

Made Hash store a byte array, so Node can store a Hash, rather than a byte slice #67

Open FreddieRidell opened 5 years ago

FreddieRidell commented 5 years ago

🙋 feature

Key Changes:

yoshuawuyts commented 5 years ago

Thanks heaps for this PR! Unfortunately in its current state we can't accept it, because it opens us up to timing attacks.

Keeping Blake2bResult around was a deliberate choice, as the Eq and PartialEq methods are guaranteed to be constant-time. By moving the bytes over to a regular buffer we drop that property, which isn't great. (ref)

I do really like some of the ergonomics improvements introduced here; typing .as_bytes().into() gets old fast. I wonder if perhaps we introduce some conversion traits we could achieve similar benefits without losing any (cryptographic) properties.

Thanks again so much; overall I really like this direction!

FreddieRidell commented 5 years ago

Ahh, I should have asked before I picked a TODO to work on, is there a better place to ask in future?

It looks like like Blake2BResult uses constant_time_eq to do its equality check, it should be pretty easy to adapt this solution to use that function. If I made that change would that make this PR acceptable?

yoshuawuyts commented 5 years ago

Ahh, I should have asked before I picked a TODO to work on, is there a better place to ask in future?

Opening an issue is a good way to go about this! Otherwise I'm also around on Freenode and Discord for synchronous chat.

If I made that change would that make this PR acceptable?

Possibly! But looking at this patch I'm thinking that adding more conversion traits on the existing codebase could probably get us the closest to our intended goal with the fewest amount of changes required.

FreddieRidell commented 5 years ago

Possibly!

I've switched Hash to manually implementing PartialEq and Eq using constant_time_eq, let me know what you think?

But looking at this patch I'm thinking that adding more conversion traits on the existing codebase could probably get us the closest to our intended goal with the fewest amount of changes required.

Do you mean implementing From for Hash -> Blake2bResult and stuff like that?

bltavares commented 4 years ago

@yoshuawuyts sorry to bother, but I'm wondering how do you feel in regards of the new constant hash verification in this current PR.