search

fernet / fernet-rb

Delicious HMAC Digest(if) authentication and AES-128-CBC encryption
MIT License
89 stars 22 forks source link

Simplify implementation #21

Closed ryandotsmith closed 11 years ago

ryandotsmith commented 11 years ago

Motivation:

Code that deals with security is hard; it should be easy to read and verify.

Back Story:

A few months ago, the ruby version of fernet fell out of compatibility with kr's Go version. I tried to update fernet but it was unclear what code changes needed to be made. So I wrote my own version. I was please to find that a complete version could be done in < 70 LOC.

Ask:

Would you be willing to accept a pull request that reduced the LOC ~10x? Here is the implementation with tests.

hgmnz commented 11 years ago

I am all for a simple implementation. The latest code on master is, I believe, simpler to understand than it's predecessor and has clearer separation of concerns.

I would not accept one pull request that re-implemented the library, because we run the risk of introducing (or re-introducing) subtle bugs. I would gladly accept, encourage and appreciate any small pull requests that may address specific improvements in the software design.

ryandotsmith commented 11 years ago

I think it is totally understandable for you not to accept a PR which replaces all of the code you have written. I will say that for the type of problem that fernet is solving, it is super important to keep things small so that readers of the code can easily understand what is happening.

ryandotsmith commented 11 years ago

Also, out of curiosity, what bugs have you fixed that were not covered by the spec?

The only thing that I consider dicey about my current implementation is that it is susceptible to a timing attack, but there is a simple solution for that problem. I wonder how we can fit that problem into the spec.

ryandotsmith commented 11 years ago

fwiw: I just fixed the timing problem in my implementation.

hgmnz commented 11 years ago

what bugs have you fixed that were not covered by the spec?

One such bug was a padding oracle attack vulnerability

ryandotsmith commented 11 years ago

Can you point me to the commit that fixed this bug?