ntrepid8 / ex_crypto

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

Add generate_key and pem_encode wrappers #13

Closed barttenbrinke closed 7 years ago

barttenbrinke commented 7 years ago

Hi, I was working with encryption and saw that these to methods from the Erlang PublicKey module where not wrapped, so I added them.

Great work so far!

ntrepid8 commented 7 years ago

Looks great! Thanks for the PR, not sure why Travis hasn't finished the tests yet but if it's still hung up in a little bit I'll poke it and get it running.

ntrepid8 commented 7 years ago

It looks like there are some conflicts after I merged #12 so a few additional tweaks may be required before I can look at merging this.

I check on the Travis-CI logs and for some reason they detected "abuse" on the last commit so the job didn't run. If it happens again when the next commit comes in to fix the merge conflicts I'll try emails their help team. They are having some service issues at the moment with their help app, so I'll try and hold off until we know it fails again.

barttenbrinke commented 7 years ago

I’ll rebase it first thing tomorrow! Sent from my iPhone

On 23 Oct 2017, at 18:00, Josh Austin notifications@github.com wrote:

It looks like there are some conflicts after I merged #12 so a few additional tweaks may be required before I can look at merging this.

I check on the Travis-CI logs and for some reason they detected "abuse" on the last commit so the job didn't run. If it happens again when the next commit comes in to fix the merge conflicts I'll try emails their help team. They are having some service issues at the moment with their help app, so I'll try and hold off until we know it fails again.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

barttenbrinke commented 7 years ago

I've rebased it, so it should merge cleanly with master.

ntrepid8 commented 7 years ago

I'm still not sure why Travis is not running the tests. I've emailed their support to see if they can help. When I look at the request logs I see this:

selection_348

@barttenbrinke ever had anything like this happen before with Travis?

barttenbrinke commented 7 years ago

No, but my guess is that generating ssh keys might be an abuse indicator.

Sent from my iPhone

On 24 Oct 2017, at 16:46, Josh Austin notifications@github.com wrote:

I'm still not sure why Travis is not running the tests. I've emailed their support to see if they can help. When I look at the request logs I see this:

@barttenbrinke ever had anything like this happen before with Travis?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

ntrepid8 commented 7 years ago

Not sure why that would be considered abuse. Openssl is a fairly standard thing to use. These tests don't put much strain on the entropy pool or on the CPU. It's not like we are consuming hundreds of MBs of entropy or something.

barttenbrinke commented 7 years ago

No, but there are people making prs on opensource projects to mine things like bitcoins. And generating ssh keys might Therfore be flagged.

Sent from my iPhone

On 24 Oct 2017, at 19:42, Josh Austin notifications@github.com wrote:

Not sure why that would be considered abuse. Openssl is a fairly standard thing to use. These tests don't put much strain on the entropy pool or on the CPU. It's not like we are consuming hundreds of MBs of entropy or something.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

ntrepid8 commented 7 years ago

Perhaps, but I would think it would trigger more frequently on this project if that were the case. This is the first time I've seen it, but ¯\_(ツ)_/¯

ntrepid8 commented 7 years ago

I'm going to close this and re-open it to try and re-trigger the Travis build. I just got an email from them saying they have fixed the false-positive.

ntrepid8 commented 7 years ago

Re-opening...

ntrepid8 commented 7 years ago

trying again...

ntrepid8 commented 7 years ago

Cool, Travis ran the build correctly this time :)

It does seem like some tests are failing though: https://travis-ci.org/ntrepid8/ex_crypto/builds/292262872

Seeing anything like that when you run the tests locally?

barttenbrinke commented 7 years ago

Well yesterday it was all green, let me look at it :)

Sent from my iPhone

On 24 Oct 2017, at 21:43, Josh Austin notifications@github.com wrote:

Cool, Travis ran the build correctly this time :)

It does seem like some tests are failing though: https://travis-ci.org/ntrepid8/ex_crypto/builds/292262872

Seeing anything like that when you run the tests locally?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

barttenbrinke commented 7 years ago

@ntrepid8 So the core issue was that Travis was building against otp 18, and there are a lot of crypto changes on otp20 (which I am running). I've bumped the Erlang dependency to 20 and also updated elxir to 1.4.5 (lowest version that supports otp20). I also added some module and method docs to the ExPublicKey module, so that the exdoc is now a bit more usefull.

barttenbrinke commented 7 years ago

After this is merged, you are getting a PR where all methods/moduels have a basic ex_doc documentation, because I can't stand the docs being as empty as they are :)

ntrepid8 commented 7 years ago

One issue is that requiring a library like this to run the latest OTP creates a lot of issues for projects that use it. I even have some other projects with requirements that are not compatible with OTP 20. I would very much like to avoid making OTP 20 an absolute requirement.

Which crypto change are you bumping up against? Perhaps there is a way that we can support the new features w/o breaking backward compatibility to OTP 18.

ntrepid8 commented 7 years ago

Also, any documentation help is always appreciated :)

barttenbrinke commented 7 years ago

I think we need to support both 18 and 20.. we can do something like

: :application.get_key(:crypto, :vsn)

And then call the function with the correct parameters.

And then let Travis test that? I can build that tomorrow.

Sent from my iPhone

On 25 Oct 2017, at 16:57, Josh Austin notifications@github.com wrote:

Also, any documentation help is always appreciated :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

ntrepid8 commented 7 years ago

Sounds good.

barttenbrinke commented 7 years ago

According to jose valim we should call if if Application.spec(:crypto, :vsn) at runtime and use that to either call A or B.

barttenbrinke commented 7 years ago

Turns out OTP < 20 has no rsa keygen support, so I'll try falling back to openssl when needed..

barttenbrinke commented 7 years ago

@ntrepid8 I think this is the best I can do.

barttenbrinke commented 7 years ago

Or we could just return an :unsupported atom, like excrypto does.

ntrepid8 commented 7 years ago

Sweet, thanks! I'll take a look today and try to get it released.

barttenbrinke commented 7 years ago

👍 That was interesting :)

ntrepid8 commented 7 years ago

I'll get this released today on hex.pm as v0.7.0

ntrepid8 commented 7 years ago

published as 0.7.0: https://hex.pm/packages/ex_crypto/0.7.0