ExWeb3 / ex_keccak

Elixir library for computing Keccak SHA3-256 hashes using a NIF built tiny-keccak Rust crate.
Apache License 2.0
25 stars 20 forks source link

Integrate rustler_precompiled with this project #24

Closed spapas closed 1 year ago

spapas commented 2 years ago

Hello friends, would you consider integrating this project with Rustler precompiled (https://hexdocs.pm/rustler_precompiled/RustlerPrecompiled.html) in order to avoid needing the rust dependency ?

I've already done this integration on my fork of ex_keccak and it is working great: https://github.com/spapas/ex_keccak

I don't want to provide a PR because I've used my own repo and tags there to download the pre-compiled NIFs and I think it would be too intrusive to this repo (i.e you'll see base_url: "https://github.com/spapas/ex_keccak/releases/download/0.3.3"). However you can take a peek at the diffs needed here:

https://github.com/tzumby/ex_keccak/compare/master...spapas:master

The only thing missing is to change it with your repo and tags and create a new hex release :)

TIA

tzumby commented 2 years ago

Hi @spapas, thanks for sharing and working on this!

This looks interesting, I like that there are checksums, but how would I check the hash of a published library ? Do I have to install it and run that in my deps folder ? If that's the case, it sounds pretty cumbersome, i doubt anyone would do that if it's so involved.

Is there a way to make this configurable and allow users to pick if they want it pre-compiled vs compile at install time ?

spapas commented 2 years ago

Hey @tzumby most of the questions on how this works should be explained in the docs. I'll try to answer your questions though:

When you have a dependency that uses rustler_precompiled the correct binaries will be checked and downloaded automatically without any involvement from the user. Ie rustler precompiled integrates with the library, not the project using the library. For example I've got a project that uses ex_keccak. I have only added my version of ex_keccak from my github repo as a dependency and haven't done any other changes to my project

Also if you want to compile the dependency for some reason you can with some simple configuration in your project's config.

tzumby commented 2 years ago

Thanks, I found the force_build setting in the docs. This is great.

What I was asking in my first questions was how I can manually check the checksum of the library binaries. I think this is extremely important given this is a hashing function. Or in other words, what are the guarantees that I don't maliciously push a weak binary in hex and users of this package blindly add it to their projects ?

spapas commented 2 years ago

This is an interesting question! I don't have an answer maybe the authors of rustler precompiled can answer?

However from my experience I think that trusting the library author that he'll provider a correct binary is common. For example python precompiled wheels.

tzumby commented 2 years ago

Yup, unfortunately it is common. But that's a big problem, that's how you get side chain attacks. We've already seen a few because of node_modules in npm. @ayrat555 has a few high download packages using this so I'd love to hear his feedback on this as well.

I'm not necessarily opposed to giving the users an option, but I would certainly not make this the default.

spapas commented 2 years ago

I understand your hesitation on this however precompiled rustler has the force_build option. If you pass true for that then the ex_keccak library will be compiled with rustler by default. People that to use the precompiled binaries will need to configure their project for that.

Also, this may not be important but using the precompiled binaries is good for the adoption of elixir: https://nitter.net/i/status/1463605416861081602

ayrat555 commented 2 years ago

I mostly use rust nifs for cryptography and hash functions (including ex_keccak) so I'd rather not download artefacts from external sources that may compromise the security of my projects. But I think if you have a private library / project it may be fine

I'm not necessarily opposed to giving the users an option, but I would certainly not make this the default.

I have the same opinion, it would be nice if this feature was optional. so users can decide if they want to compile locally or download precompiled artefacts

For rust nifs, you need to install only one dependency - rust. For C nifs usually, you need to install a bunch of dependencies. So I don't think it's a problem to install rust.

tzumby commented 2 years ago

Thank you both for chiming in! I'm sorry it took a while, but I had some time to look into this.

I tried @spapas fork locally and configured the force_build: false option. It does work it looks like the pre-compiled binaries are not copied over at the dependency download stage, but only at compile time. And if that setting is set to false by default, then it will always compile. All good there.

I tried to test the opposite, enabling the pre-compile download from a test app. This didn't override the force_build: false setting though:

config :rustler_precompiled, :force_build, ex_keccak: false

According to the docs, this should have worked, but when setting it, it still force compiles.

If this doesn't work then I don't see a point to adding this.

spapas commented 2 years ago

Hey @tzumby you've set ex_keccak: false, I think the correct thing to do is set ex_keccak: true in the config of your app. If this still ain't working you could raise an issue at the rustler_precompiled github repo.

I am using my local fork for my project and am happy with that for now so I can't research it anymore (also releasing new precompiled NIFs is a PITA so I'd rather avoid it 😁 )

tzumby commented 2 years ago

I think false is correct since I'm trying to have it default to always compile. unless you set this in the config. But good point, I'll create a simple mix project i can share on the rustler_precompiled repo

spapas commented 2 years ago

@tzumby I'll try to explain my understanding of how this should work: Taking my fork as a base, you'll need to change the https://github.com/spapas/ex_keccak/blob/master/lib/ex_keccak.ex file from

  use RustlerPrecompiled,
    otp_app: :ex_keccak,
    crate: :exkeccak,
    base_url: "https://github.com/spapas/ex_keccak/releases/download/0.3.3",
    version: "0.3.3"

to

  use RustlerPrecompiled,
    otp_app: :ex_keccak,
    crate: :exkeccak,
    base_url: "https://github.com/spapas/ex_keccak/releases/download/0.3.3",
    version: "0.3.3",
    force_build: true

You'll also need to release a new version i.e 0.3.4, add the tags etc. This will setup the ex_keccak lib so it will be compiled by default (instead of downloadded). So you'll have the default behavior you'll want (people that just include the lib in their projects will need to compile it).

If somebody wanted the pre-compiled NIFs instead, he'd have to configure his project (i.e the project that uses the ex_keccak lib) to add the following option to config.exs:

config :rustler_precompiled, force_build: false, your_otp_app: true to your project's config.exs.

tzumby commented 2 years ago

Yup, that's exactly what I tried. Except, force_build is not part of the keyword list. If I try your line, I get an error:

# in config.exs

config :rustler_precompiled, force_build: false, ex_keccak: true
==> ex_keccak
Compiling 1 file (.ex)

== Compilation error in file lib/ex_keccak.ex ==
** (FunctionClauseError) no function clause matching in Access.fetch/2

    The following arguments were given to Access.fetch/2:

        # 1
        false

        # 2
        :ex_keccak

    Attempted function clauses (showing 5 out of 5):

        def fetch(%module{} = container, key)
        def fetch(map, key) when is_map(map)
        def fetch(list, key) when is_list(list) and is_atom(key)
        def fetch(list, key) when is_list(list)
        def fetch(nil, _key)
spapas commented 2 years ago

Hey @tzumby sorry I used a wrong config. The correct config to unset force build is this one (as it is described actually in the docs):

config :rustler_precompiled,
  :force_build, ex_keccak: false

I tested it with my project which has force_build = false by default and setting

config :rustler_precompiled,
  :force_build, ex_keccak: true

in my project's config, it compiled the ex_keccak lib instead of downloading it.

tzumby commented 2 years ago

Does it download if you set it to true ? For me, the false flag works, but when I set it to true it still compiles instead of download

config :rustler_precompiled, :force_build, ex_keccak: true
tzumby commented 2 years ago

Does it download if you set it to true ? For me, the false flag works, but when I set it to true it still compiles instead of download

config :rustler_precompiled, :force_build, ex_keccak: true
spapas commented 2 years ago

@tzumby I think there's some misunderstanding? If you use config :rustler_precompiled, :force_build, ex_keccak: true as you describe it will force-compile it (so compiling it the correct behavior). If you wanted to download instead you should have set config :rustler_precompiled, :force_build, ex_keccak: false in your project's config.

maartenvanvliet commented 1 year ago

Is any help needed to get this across the finish line?

I'm looking to speed up our CI and having all our rust packages precompiled would help with that :)

ayrat555 commented 1 year ago

@maartenvanvliet can CI builds be improved by caching _build directory?

maartenvanvliet commented 1 year ago

Problem is more specifically with Docker. Each docker build apt-get installs the rustc compiler and all the rest which takes time. And then rustler compiles ex_keccak, which also takes time.

There are other ways to improve this but precompiled_rustler is the easiest I think.

spapas commented 1 year ago

@maartenvanvliet I was using my fork at https://github.com/spapas/ex_keccak and worked great!

(please don't use my fork because I haven't updated it recently, fork my fork and integrate latest changes)

tzumby commented 1 year ago

Hey folks, I will get back to this issue soon, sorry for leaving it hanging.

tzumby commented 1 year ago

Thanks for opening this @spapas! I bumped the minor version to 0.7.0 that includes the pre-compiled binaries along with checksums.