search

softprops / lambda-rust

🐳 🦀 a dockerized lambda build env for rust applications
MIT License
162 stars 76 forks source link

Run the builds as a non-root user #75

Closed Veetaha closed 4 years ago

Veetaha commented 4 years ago

This is basically the resurrection of https://github.com/softprops/lambda-rust/pull/62 Closes #62

Thanks for the investigation to @medwards, there indeed is a bug in tests right now on master, because this ends with an error:

rm -rf target/lambda/release > /dev/null 2>&1

If you remove > /dev/null 2>&1 part, you will see it:

error: failed to open: /code/target/lambda/release/.cargo-lock

Caused by:
  Permission denied (os error 13)
👎   it compiles the binaries without zipping when PACKAGE=false: fail

whilst it is expected that tests do clean the target dir, but since the target/lambda dir is owned by docker root user this doesn't happen and repeated tests don't do the full recompilation.

With this commit, which is copied from https://github.com/softprops/lambda-rust/pull/62 with small changes (e.g. RUSTUP_HOME=/rustup instead of RUSTUP_HOME=/cargo) the bug has to be resolved and the problem with poisoning the host with artifacts which are owned by root must be gone.

However, the following is still an open problem:

Note: this will still write ~/.cargo/registry and ~/.cargo/git as owned by root if they don't exist when the container runs. May be due to -v mounts ignoring -u when the mountpoint doesn't exist.

But I think we might still merge this as it is since that problem is not a trivial one and the users 99% of the time will already have the cargo home available, however, I did add a note on that caveat to README.md

cc @softprops

medwards commented 4 years ago

Thanks for bringing this back. I let perfect be the enemy of good and then got busy :(

On Thu, 30 Jul 2020 at 13:56, Veetaha notifications@github.com wrote:

This is basically the resurrection of #62 https://github.com/softprops/lambda-rust/pull/62

Thanks for the investigation for @medwards https://github.com/medwards, there indeed is a bug in tests right now on master, because this ends with an error:

rm -rf target/lambda/release > /dev/null 2>&1

If you remove > /dev/null 2>&1 part, you will see it:

error: failed to open: /code/target/lambda/release/.cargo-lock

Caused by:

Permission denied (os error 13)

👎 it compiles the binaries without zipping when PACKAGE=false: fail

whilst it is expected that tests do clean the target dir, but since the target/lambda dir is owned by docker root user this doesn't happen and repeated tests don't do the full recompilation.

With this commit, which is copied from #62 https://github.com/softprops/lambda-rust/pull/62 with small changes (e.g. RUSTUP_HOME=/rustup instead of RUSTUP_HOME=/cargo) the bug has to be resolved and the problem with poisoning the host with artifacts which are owned by root must be gone.

However, the following is still an open problem:

Note: this will still write ~/.cargo/registry and ~/.cargo/git as owned by root if they don't exist when the container runs. May be due to -v mounts ignoring -u when the mountpoint doesn't exist.

But I think we might still merge this as it is since that problem is not a trivial one and the users 99% of the time will already have the cargo home available, however, I did add a note on that caveat to README.md

You can view, comment on, or merge this pull request online at:

https://github.com/softprops/lambda-rust/pull/75 Commit Summary

  • Run the builds as a non-root user

File Changes

Patch Links:

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/softprops/lambda-rust/pull/75, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAABZK77COQCPSIK4UIRXD3R6FNWZANCNFSM4PNSGDIQ .

softprops commented 4 years ago

thanks for bring this back I'll take a closer look this weekend. Is there anything worth calling out in the CHANGELOG file

Veetaha commented 4 years ago

It's worth to mention that cargo and rustup installation paths were changed from default /root/{.cargo/.rustup} to /cargo and /rustup respectively, this is basically the main change

softprops commented 4 years ago

Thanks I'll have to up date that in a sister project https://github.com/softprops/serverless-rust/blob/0e818ddd70319ca213c5e7acc64c479171295972/index.js#L191 and some docs I'm the rust lambda runtime

Veetaha commented 4 years ago

Hi @softprops, I'd suggest publishing the new image version to dockerhub, are there blockers for this?

softprops commented 4 years ago

I'd like to run a few experiments to see if there's a way to do this without breaking existing usages.

Namely those already running -v ${HOME}/.cargo/registry:/root/.cargo/registry in theory it should be possible to to map to the same directory.

The hesitation in the short turn is finding an updating documentation for other repositories referencing the old directory.