search

ntrepid8 / ex_crypto

Wrapper around the Erlang crypto module for Elixir.
MIT License
144 stars 48 forks source link

v1 refactor to include AES256CBC, pending redesign/breaking API changes for new version #5

Closed bglusman closed 7 years ago

bglusman commented 7 years ago

Not ready to merge in, but thought I'd see what you thought of current state, in case you have any feedback aside from "Add tests, make it work for realz"

Actually apparently I did add tests via doctests, I forget if I knew they were a thing in elixir, but awesome! so 2 failing doctests, fix pending/input welcome

bglusman commented 7 years ago

OK, so played a bit, and suspect the failures may be related to padding/unpadding, but a little confused... if you or anyone has time to take a look, feedback or alternative padding scheme/other fixes welcome, otherwise I'll try and revisit this later, but for now may just get this working for my purposes outside the library, not sure.

ntrepid8 commented 7 years ago

I'll take a look and see if I can see what's up with it.

bglusman commented 7 years ago

OK, partly putting this here for a note, partly in case it helps you if you get around to looking before me... I just decided to try and use my fork for the actual decryption I need to do, to see if it worked there, and it doesn't, but this gist's code, which looks identical in process to me, does work... so I'm missing something that's a mismatch :-) That's likely also what's making tests fail. I attempted one fix to simplify and just always base64 encode/decode iv and encrypted data, but didn't help... https://gist.github.com/bglusman/7e1df9efad80845977b60d44d335f3df/edit

bglusman commented 7 years ago

@ntrepid8 a little torn, any chance it's worth merging this in (more or less) as is as a backward compatible version and then start on a new version with ideal interface? No strong opinion but depending on how long it takes and what we decide this might still be a useful first step...

bglusman commented 7 years ago

Hmm, just noticed tests failed on travis, though they pass locally... I tried installing 1.2.6 locally, and changing travis to use 1.3.3 (what I was using passing) but couldn't replicate Travis errors... just managed to sort of psuedo-bisect the problem, but still don't understand the cause, somehow Travis doesnt like :aes_cbc format, even though its fine with :aes_gcm, and :aes_cbc worked fine for me locally... maybe it has something to do with OpenSSL version on Travis being different from my machine? Don't love it being hardcoded to 256, but will leave as is for now with tests passing for you to investigate I guess

ntrepid8 commented 7 years ago

Sorry for delay in looking at this, been out of town. Will look soon...

bglusman commented 7 years ago

@ntrepid8 no worries, let me know if I can help!

ntrepid8 commented 7 years ago

@bglusman this PR mentions some breaking changes, but it doesn't look like any tests were removed and all the old ones are still passing it seems. The PR looks OK and I'd like to merge it, but just wanted to double check once more about the breaking changes....

Again, sorry for delay, catching up now.

bglusman commented 7 years ago

I think we discussed breaking changes and then I decided a first version that didn't have any was better, then can deprecate or rethink the API in follow up work, but I haven't looked at the code in a month, but we're using the firm in production already.

ntrepid8 commented 7 years ago

This should be ready to consume from hex.pm: https://hex.pm/packages/ex_crypto/0.3.0

ntrepid8 commented 6 years ago

closes #4.