Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PVF worker: separate worker binaries and build with musl #650

Closed
4 of 9 tasks
mrcnski opened this issue Apr 23, 2023 · 5 comments · Fixed by #2461
Closed
4 of 9 tasks

PVF worker: separate worker binaries and build with musl #650

mrcnski opened this issue Apr 23, 2023 · 5 comments · Fixed by #2461
Assignees

Comments

@mrcnski
Copy link
Contributor

mrcnski commented Apr 23, 2023

ISSUE

Overview

Per the security/sandboxing work in #882, we want to separate the PVF worker binaries and build them with musl. There are several advantages in doing this:

  • Forcing the use of musl instead of whatever the system C lib is, means we can better control and sandbox the expected syscalls.
  • It is also a win for more deterministic execution, i.e. more homogeneity in execution times and memory used, as opposed to when validators can use different libc's.
  • [nvm: we already use LTO in production builds.] With a separate binary we can always apply LTO optimizations to this binary alone. LTO may make compile times prohibitively long for all of polkadot, but the impact on a small binary is much smaller and there may be performance wins. (Related: Optimize validator binaries with LTO polkadot#4311.)
  • With smaller binaries we minimize the risk of ROP gadgets.

Todo

  • move PVF workers into separate binaries
  • adapt wasm-builder for worker binaries
  • include the binary into polkadot executable at build-time (similar to this)
  • ... some other stuff
  • 1. Update the CI jobs, docker scripts, etc. to build the worker binaries with musl.
  • 2. Update the validator guide with new instructions for building the worker binaries.
  • 3. Announce to validators that the instructions have changed, and non-musl binaries will become a hard error in the next release.
  • 4. Make musl workers a requirement for running secure mode.
  • 5. Finally, enable full seccomp rules (logging violations for a while).
@mrcnski
Copy link
Contributor Author

mrcnski commented Apr 25, 2023

Despite my comment at #652, it seems that something like wasm-builder is the only way to embed executables. I had this hack which seemed to work, until I did cargo clean:

const PREPARE_EXE: &'static [u8] =
	include_bytes!(concat!(env!("OUT_DIR"), "/../../../prepare_worker"));
const EXECUTE_EXE: &'static [u8] =
	include_bytes!(concat!(env!("OUT_DIR"), "/../../../execute_worker"));

And env!("CARGO_BIN_EXE_prepare_worker") only works in integration tests :(. So I will go ahead and implement musl-builder by copy/pasting wasm-builder for now.

mrcnski referenced this issue in paritytech/polkadot Apr 28, 2023
The worker binaries must be built first so that the host could embed them into `polkadot`. Therefore `pvf/worker` could not depend on `pvf`, so to remove the dependency, common functionality was extracted into `pvf/common`.

(NOTE: We already needed to do this host/worker/common separation as part of https://github.com/paritytech/polkadot/issues/7116, it's just unfortunate that it had to be done here and complicate this PR.)

Integration tests were moved from `pvf/worker/tests` to `pvf/tests` because they need the PVF host.
@Polkadot-Forum
Copy link

This issue has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/ux-of-distributing-multiple-binaries-take-2/2854/1

@mrcnski
Copy link
Contributor Author

mrcnski commented Oct 31, 2023

With #2009 merged, I believe this is the only task stopping us from enabling the full seccomp rules (only logging violations for now).

And, with #1663 we have everything we need to build with musl right in the repo.

What's left:

  • 1. Update the CI jobs, docker scripts, etc. to build the worker binaries with musl.
  • 2. Update the validator guide with new instructions for building the worker binaries.
  • 3. Announce to validators that the instructions have changed, and non-musl binaries will become a hard error in the next release.
  • 4. Make musl workers a requirement for running secure mode.
  • 5. Finally, enable full seccomp rules (logging violations for a while).

@mrcnski
Copy link
Contributor Author

mrcnski commented Nov 12, 2023

If possible we should recommend a specific, set version of the musl toolchain for better determinism.

@mrcnski
Copy link
Contributor Author

mrcnski commented Nov 12, 2023

Anouncement / Release Schedule

The musl requirement should be announced at some point. It's not a requirement for on-demand, so a bit lower priority. I'm thinking we give secure-mode some time to settle in before adding the musl requirement, as it will affect all validators.

So if we release secure-mode in 1.5 then maybe we can announce the musl requirement in 1.7, adding a runtime warning. Then enforce it in 1.9 or something like this. Reason being, it might make sense to give more time than for secure-mode, because secure-mode will affect hardly anyone. On the other hand, the musl requirement simply involves doing a different build instruction, and not migrating to a different machine or upgrading the kernel, so while it affects everyone the impact is low.

@github-project-automation github-project-automation bot moved this from In Progress to Done in PVF Security Hardening Nov 25, 2023
claravanstaden added a commit to Snowfork/polkadot-sdk that referenced this issue Dec 8, 2023
* Some tweaks for the demo

* A few more demo tweaks

* Fix execution block hash, now prints out in hex

* Investigating block verification failing

* Fix hex to bytes function and adds a test for it.

* Removes debugging logs.

* Cleans up formatting.

* More cleanup and refactor.

* Import the second last finalized header too.

* Adds more logging for the empty sync aggregate.

Co-authored-by: claravanstaden <Cats 4 life!>
@eskimor eskimor moved this to Completed in parachains team board Jan 30, 2024
helin6 pushed a commit to boolnetwork/polkadot-sdk that referenced this issue Feb 5, 2024
…aritytech#650)

* Do not enter estimate mode when binary flag is enabled

* cfg one liner
bkchr pushed a commit that referenced this issue Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Completed
3 participants