Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Docker: run parity as normal user #9689

Merged
merged 1 commit into from
Oct 3, 2018
Merged

Docker: run parity as normal user #9689

merged 1 commit into from
Oct 3, 2018

Conversation

asymmetric
Copy link
Contributor

@asymmetric asymmetric commented Oct 2, 2018

To avoid users shooting themselves in the foot, it's best practice to avoid running the binary with superuser powers, if possible. This is designed to make privilege escalation harder.

This is actually done in some of the other Dockerfiles already, so this PR applies the same logic to the one that's pushed to Docker Hub.

@parity-cla-bot
Copy link

It looks like this contributor signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

Copy link
Contributor

@jimpo jimpo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good change to make.

scripts/docker/hub/Dockerfile Outdated Show resolved Hide resolved
Copy link
Collaborator

@sorpaas sorpaas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@5chdn 5chdn added this to the 2.2 milestone Oct 3, 2018
@5chdn 5chdn added A8-looksgood 🦄 Pull request is reviewed well. M1-ci 🙉 Continuous integration. labels Oct 3, 2018
@5chdn 5chdn merged commit 1388f4d into openethereum:master Oct 3, 2018
@5chdn 5chdn added B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. B1-patch-beta 🕷 labels Oct 3, 2018
@asymmetric asymmetric deleted the asymmetric/dockerfile-root branch October 3, 2018 14:08
dvdplm added a commit that referenced this pull request Oct 4, 2018
…mon-deps

* origin/master:
  Add Foundation Bootnodes (#9666)
  Docker: run as parity user (#9689)
  ethcore: mcip3 block reward contract (#9605)
  Verify block syncing responses against requests (#9670)
This was referenced Oct 8, 2018
5chdn pushed a commit that referenced this pull request Oct 8, 2018
5chdn added a commit that referenced this pull request Oct 9, 2018
* parity-version: bump beta to 2.1.2

* docs(rpc): push the branch along with tags (#9578)

* docs(rpc): push the branch along with tags

* ci: remove old rpc docs script

* Remove snapcraft clean (#9585)

* Revert " add snapcraft package image (master) (#9584)"

This reverts commit ceaedbb.

* Update package-snap.sh

* Update .gitlab-ci.yml

* ci: fix regex 🙄 (#9597)

* docs(rpc): annotate tag with the provided message (#9601)

* Update ropsten.json (#9602)

* HF in POA Sokol (2018-09-19) (#9607)

poanetwork/poa-chain-spec#86

* fix(network): don't disconnect reserved peers (#9608)

The priority of && and || was borked.

* fix failing node-table tests on mac os, closes #9632 (#9633)

* ethcore-io retries failed work steal (#9651)

* ethcore-io uses newer version of crossbeam && retries failed work steal

* ethcore-io non-mio service uses newer crossbeam

* remove master from releasable branches (#9655)

* remove master from releasable branches

need backporting in beta 
fix https://gitlab.parity.io/parity/parity-ethereum/-/jobs/101065 etc

* add except for snap packages for master

* Test fix for windows cache name... (#9658)

* Test fix for windows cache name...

* Fix variable name.

* fix(light_fetch): avoid race with BlockNumber::Latest (#9665)

* Calculate sha3 instead of sha256 for push-release. (#9673)

* Calculate sha3 instead of sha256 for push-release.

* Add pushes to the script.

* Hardfork the testnets (#9562)

* ethcore: propose hardfork block number 4230000 for ropsten

* ethcore: propose hardfork block number 9000000 for kovan

* ethcore: enable kip-4 and kip-6 on kovan

* etcore: bump kovan hardfork to block 9.2M

* ethcore: fix ropsten constantinople block number to 4.2M

* ethcore: disable difficulty_test_ropsten until ethereum/tests are updated upstream

* ci: fix push script (#9679)

* ci: fix push script

* Fix copying & running on windows.

* CI: Remove unnecessary pipes (#9681)

* ci: reduce gitlab pipelines significantly

* ci: build pipeline for PR

* ci: remove dead weight

* ci: remove github release script

* ci: remove forever broken aura tests

* ci: add random stuff to the end of the pipes

* ci: add wind and mac to the end of the pipe

* ci: remove snap artifacts

* ci: (re)move dockerfiles

* ci: clarify job names

* ci: add cargo audit job

* ci: make audit script executable

* ci: ignore snap and docker files for rust check

* ci: simplify audit script

* ci: rename misc to optional

* ci: add publish script to releaseable branches

* ci: more verbose cp command for windows build

* ci: fix weird binary checksum logic in push script

* ci: fix regex in push script for windows

* ci: simplify gitlab caching

* docs: align README with ci changes

* ci: specify default cargo target dir

* ci: print verbose environment

* ci: proper naming of scripts

* ci: restore docker files

* ci: use docker hub file

* ci: use cargo home instead of cargo target dir

* ci: touch random rust file to trigger real builds

* ci: set cargo target dir for audit script

* ci: remove temp file

* ci: don't export the cargo target dir in the audit script

* ci: fix windows unbound variable

* docs: fix gitlab badge path

* rename deprecated gitlab ci variables

https://docs.gitlab.com/ee/ci/variables/#9-0-renaming

* ci: fix git compare for nightly builds

* test: skip c++ example for all platforms but linux

* ci: add random rust file to trigger tests

* ci: remove random rust file

* disable cpp lib test for mac, win and beta (#9686)

* cleanup ci merge

* ci: fix tests

* fix bad-block reporting no reason (#9638)

* ethcore: fix detection of major import (#9552)

* sync: set state to idle after sync is completed

* sync: refactor sync reset

* Don't hash the init_code of CREATE. (#9688)

* Docker: run as parity user (#9689)

* Implement CREATE2 gas changes and fix some potential overflowing (#9694)

* Implement CREATE2 gas changes and fix some potential overflowing

* Ignore create2 state tests

* Split CREATE and CREATE2 in gasometer

* Generalize rounding (x + 31) / 32 to to_word_size

* make instantSeal engine backwards compatible, closes #9696 (#9700)

* ethcore: delay ropsten hardfork (#9704)

* fix (light/provider) : Make `read_only executions` read-only (#9591)

* `ExecutionsRequest` from light-clients as read-only

This changes so all `ExecutionRequests` from light-clients are executed
as read-only which the `virtual``flag == true ensures.

This boost up the current transaction to always succeed

Note, this only affects `eth_estimateGas` and `eth_call` AFAIK.

* grumbles(revert renaming) : TransactionProof

* grumbles(trace) : remove incorrect trace

* grumbles(state/prove_tx) : explicit `virt`

Remove the boolean flag to determine that a `state::prove_transaction`
whether it should be executed in a virtual context or not.

Because of that also rename the function to
`state::prove_transction_virtual` to make more clear

* CI: Skip docs job for nightly (#9693)

* ci: force-tag wiki changes

* ci: force-tag wiki changes

* ci: skip docs job for master and nightly

* ci: revert docs job checking for nightly tag

* ci: exclude docs job from nightly builds in gitlab script
5chdn pushed a commit that referenced this pull request Oct 9, 2018
5chdn added a commit that referenced this pull request Oct 10, 2018
* parity-version: bump stable to 2.0.7

* HF in POA Sokol (2018-09-19) (#9607)

poanetwork/poa-chain-spec#86

* fix failing node-table tests on mac os, closes #9632 (#9633)

* fix(light_fetch): avoid race with BlockNumber::Latest (#9665)

* CI: Remove unnecessary pipes (#9681)

* ci: reduce gitlab pipelines significantly

* ci: build pipeline for PR

* ci: remove dead weight

* ci: remove github release script

* ci: remove forever broken aura tests

* ci: add random stuff to the end of the pipes

* ci: add wind and mac to the end of the pipe

* ci: remove snap artifacts

* ci: (re)move dockerfiles

* ci: clarify job names

* ci: add cargo audit job

* ci: make audit script executable

* ci: ignore snap and docker files for rust check

* ci: simplify audit script

* ci: rename misc to optional

* ci: add publish script to releaseable branches

* ci: more verbose cp command for windows build

* ci: fix weird binary checksum logic in push script

* ci: fix regex in push script for windows

* ci: simplify gitlab caching

* docs: align README with ci changes

* ci: specify default cargo target dir

* ci: print verbose environment

* ci: proper naming of scripts

* ci: restore docker files

* ci: use docker hub file

* ci: use cargo home instead of cargo target dir

* ci: touch random rust file to trigger real builds

* ci: set cargo target dir for audit script

* ci: remove temp file

* ci: don't export the cargo target dir in the audit script

* ci: fix windows unbound variable

* docs: fix gitlab badge path

* rename deprecated gitlab ci variables

https://docs.gitlab.com/ee/ci/variables/#9-0-renaming

* ci: fix git compare for nightly builds

* test: skip c++ example for all platforms but linux

* ci: add random rust file to trigger tests

* ci: remove random rust file

* disable cpp lib test for mac, win and beta (#9686)

* cleanup ci merge

* parity: bump clib

* ci: fix tests

* ci: disable c++ example

* Docker: run as parity user (#9689)

* CI: Skip docs job for nightly (#9693)

* ci: force-tag wiki changes

* ci: force-tag wiki changes

* ci: skip docs job for master and nightly

* ci: revert docs job checking for nightly tag

* ci: exclude docs job from nightly builds in gitlab script

* fix (light/provider) : Make `read_only executions` read-only (#9591)

* `ExecutionsRequest` from light-clients as read-only

This changes so all `ExecutionRequests` from light-clients are executed
as read-only which the `virtual``flag == true ensures.

This boost up the current transaction to always succeed

Note, this only affects `eth_estimateGas` and `eth_call` AFAIK.

* grumbles(revert renaming) : TransactionProof

* grumbles(trace) : remove incorrect trace

* grumbles(state/prove_tx) : explicit `virt`

Remove the boolean flag to determine that a `state::prove_transaction`
whether it should be executed in a virtual context or not.

Because of that also rename the function to
`state::prove_transction_virtual` to make more clear

* ethcore: fix detection of major import (#9552)

* sync: set state to idle after sync is completed

* sync: refactor sync reset

* parity: revert clib bump and fix tests

* Fix path to parity.h (#9274)

* Fix path to parity.h

* Fix other paths as well

* ethcore-io retries failed work steal (#9651)

* ethcore-io uses newer version of crossbeam && retries failed work steal

* ethcore-io non-mio service uses newer crossbeam
@dschoeni
Copy link

This is a good change, but it broke our kubernetes deployment as ConfigMaps are mounted by root per default, and thus not-readable by the container.

So just for everyone to keep in mind when finding this Issue/PR.

@mmhh1910
Copy link

The change seems to generally interfere with named volumes.

v2.0.6:

> docker run -v parity_data_v2.0.6:/parity_chain -ti parity/parity:v2.0.6 --base-path /parity_chain
2018-10-13 09:52:05 UTC Starting Parity-Ethereum/v2.0.6-stable-549e202-20180919/x86_64-linux-gnu/rustc1.29.0
2018-10-13 09:52:05 UTC Keys path /parity_chain/keys/ethereum
2018-10-13 09:52:05 UTC DB path /parity_chain/chains/ethereum/db/906a34e69aec8c0d
2018-10-13 09:52:05 UTC State DB configuration: fast
2018-10-13 09:52:05 UTC Operating mode: active
2018-10-13 09:52:05 UTC Configured for Foundation using Ethash engine
...

v2.0.7:

> docker run -v parity_data_v2.0.7:/parity_chain -ti parity/parity:v2.0.7 --base-path /parity_chain
Error upgrading parity data: CannotCreateConfigPath

@asymmetric
Copy link
Contributor Author

@mmhh1910 You're asking parity, which is running as user parity, to create directories under /parity, which it might not have write permissions to.

Could you try -v parity_data_v2.0.7:/home/parity/parity_chain and --base-path /home/parity/parity_chain?

@mmhh1910
Copy link

That makes no difference:

> docker run -v parity_data_v2.0.7:/home/parity/parity_chain -ti parity/parity:v2.0.7 --base-path /home/parity/parity_chain
Error upgrading parity data: CannotCreateConfigPath

The problem seems to be that volumes are always mounted as user root:

> docker run -v parity_data_v2.0.7:/home/parity/parity_chain --entrypoint bash -ti parity/parity:v2.0.7
parity@087591af41ff:~$ ls -la
total 32
drwxr-xr-x 1 parity parity 4096 Oct 13 15:43 .
drwxr-xr-x 1 root   root   4096 Oct 11 08:47 ..
-rw-r--r-- 1 parity parity  220 Aug 31  2015 .bash_logout
-rw-r--r-- 1 parity parity 3771 Aug 31  2015 .bashrc
-rw-r--r-- 1 parity parity  655 May 16  2017 .profile
drwxr-xr-x 2 root   root   4096 Oct 11 08:47 bin
-rwxr-xr-x 1 parity parity   24 Oct 11 08:47 entrypoint.sh
drwxr-xr-x 2 root   root   4096 Oct 13 15:36 parity_chain
parity@087591af41ff:~$

I don't think there is a docker volume option to change that.

An ugly workaround for 2.0.7 is to set the user back to root with the -u parameter and change the entrypoint to /home/parity/bin/parity, since the binary parity references in entrypoint.sh is not in the path for user root:

> docker run -u root -v parity_data_v2.0.7:/parity_chain --entrypoint /home/parity/bin/parity -ti parity/parity:v2.0.7 --base-path /parity_chain
2018-10-13 15:53:13 UTC Starting Parity-Ethereum/v2.0.7-stable-db3a989-20181010/x86_64-linux-gnu/rustc1.29.0
2018-10-13 15:53:13 UTC Keys path /parity_chain/keys/ethereum
2018-10-13 15:53:13 UTC DB path /parity_chain/chains/ethereum/db/906a34e69aec8c0d
2018-10-13 15:53:13 UTC State DB configuration: fast
...

My impression is that the consequences running parity as a normal user are a bit drastic with 2.0.7, with not much benefit. If this is really wanted and needed, maybe one way to implement this would be to let the entrypoint.sh script evaluate the parameters passed and "sudo chown" the directories to the user parity that need to be writable for the parity binary.

@asymmetric
Copy link
Contributor Author

I see. What's happening is that Docker is creating the /home/parity/parity_chain directory, and chowning it as root:root.

If instead you mount to a directory that already exists, then it should work: -v foo:/home/parity.

@mmhh1910
Copy link

Yes, this seems to work:

> docker run -v foo:/home/parity -ti parity/parity:v2.0.7 --base-path /home/parity/parity_chain
2018-10-13 18:01:04 UTC Starting Parity-Ethereum/v2.0.7-stable-db3a989-20181010/x86_64-linux-gnu/rustc1.29.0
2018-10-13 18:01:04 UTC Keys path /home/parity/parity_chain/keys/ethereum
2018-10-13 18:01:04 UTC DB path /home/parity/parity_chain/chains/ethereum/db/906a34e69aec8c0d
2018-10-13 18:01:04 UTC State DB configuration: fast
2018-10-13 18:01:04 UTC Operating mode: active
...

However: My understanding is that the original data in /home/parity is copied into the volume upon volume creation, which interferes with my main goal of separating data from code.
Next time, the docker image is changed and has a change in /home/parity, I will miss it and still my use my copy of the code in the volume.

@mmhh1910
Copy link

mmhh1910 commented Oct 13, 2018

One way to solve this would be to create an empty directory "/home/parity/.local/share/io.parity.ethereum" (the default location for the chain data) in the Dockerfile. That way we could mount volumes to the existing directory owned by parity.

EDIT: Just noticed that scripts/docker/alpine/Dockerfile already includes the line:

RUN mkdir -p /home/parity/.local/share/io.parity.ethereum/

But it's not in scripts/docker/hub/Dockerfile.

@asymmetric
Copy link
Contributor Author

Yeah, we should definitely change the Dockerfile and create a directory for the chain. Mounting to /home/parity is a hack.

@MicahZoltu
Copy link
Contributor

Ugh, I just spent a lot of time/energy troubleshooting this because it got backported to parity/parity:stable and broke my environment. I run parity in docker with --mount 'type=volume,source=parity,dst=/mnt and --base-path /mnt. Since I originally was running with a pre 2.0.7 version of parity/parity:stable all of my Parity data was in a mounted volume with root:root as the user:group for every file. Then I pulled latest parity/parity:stable (patch version bump) and now I was getting Error upgrading parity data: CannotWriteVersionFile errors on launch.

The cause of this is because parity is now running inside the container as parity user, so it didn't have write access to any of its files.

It would have saved me a tremendous amount of effort if Parity provided a better error message indicating that the user running the parity process had changed and I needed to chown every file to parity/parity in order to resolve it.

On a separate note, are there any known attack vectors that this change actually protects against? Docker is (should be) sandboxed and running as root inside the sandbox doesn't give elevated access to the host. At best you would have elevated access to the docker container if parity had a vulnerability that allowed the user access outside the process, which at best would let you mess with the docker container files (which would be blown away on a restart).

@asymmetric
Copy link
Contributor Author

asymmetric commented Oct 21, 2018

are there any known attack vectors that this change actually protects against?

If you mount a directory with files/directories owned by root, the container won't be able to read them anymore. So say that you (accidentally?) mounted / in the container: before the process would have had access to the whole filesystem, now it doesn't.

which at best would let you mess with the docker container files (which would be blown away on a restart)

It would let you mess with any volumes mounted into the container, which would persist across restarts.

EDIT: also remember that in most installs, normal users have access to the Docker daemon, so a compromised normal user could've mounted any directory as a volume in a Docker container and the container would've had access to it.

@MicahZoltu
Copy link
Contributor

If someone has access to your docker daemon, then they can run any container, including one that outright is a virus/malware/whatever, so I don't believe that is an attack vector introduced by the parity image.

I'm dubious on the, "what if someone accidentally mounts / into their Parity docker container and then an attacker escapes the parity executable and messes with mounted files". This feels like your problem isn't that someone escaped from the Parity binary as much as it is that you mounted your whole filesystem into a container. Is this a thing people actually do on purpose? Are there valid use cases for people doing this?


All in all, this feels like a breaking change that at the least should have been part of a major version bump, and personally I don't find the attack vectors to be compelling enough to warrant the change at all. If this was a brand new image, then I probably wouldn't care though still would think it was a bit silly. As it is though, it is a change and I don't see the benefits as being worth it.

@asymmetric
Copy link
Contributor Author

I think it boils down to a matter of “principle of least authority”.

The parity binary does not need root, and running it as root does expose some subset of users to vulnerability. We can debate how large that subset is, or that the change should have been rolled out differently (I have no authority on that front), but i think the aforementioned principle is still valuable, and that it applies to this case.

@ddorgan
Copy link
Collaborator

ddorgan commented Oct 21, 2018

@asymmetric agree totally in principal with principle of least authority. But this was a bit of a breaking change for some users and some projects that rely on the docker image. See issues here and on the parity-deploy repo.

Playing devils advocate, can you think of any realistic situation where the container isn't run with --privileged mode and could be exploited to gain privileged access to the host server?

@asymmetric
Copy link
Contributor Author

@ddorgan The scenario I had in mind is the one I described above.

I think the main problem here was miscommunication of this change, rather than the change in itself.

There is still one improvement that can be made, relating to this comment.

@ddorgan
Copy link
Collaborator

ddorgan commented Oct 23, 2018

@asymmetric yes the improvement may help for new setups, but doesn't help with people currently using existing docker containers or containers in kubernetes.

@mtomov
Copy link

mtomov commented Oct 29, 2018

Hello, thank you for all your efforts.

I think that this change needs be reverted until a non-breaking solution exists. Atm, this is wasting people days.

I think that the 5% of people concerned with this possible security issue would be aware of it, and would themselves take care of it in a manner that they see fit; for the rest, this is just causing extra work without benefit.

Thank you.

@ddorgan
Copy link
Collaborator

ddorgan commented Oct 30, 2018

@mtomov agree totally....

@fubhy
Copy link
Contributor

fubhy commented Oct 30, 2018

This change made it really hard to deploy v2.0.7+ on Kubernetes. As far as I can tell there is really no reason to use the home directory (/home/parity) as the working dir. Or to move the binary and entry point into it. All we want is a non-root uid (which is absolutely reasonable). Kubernetes doesn't really care because it runs in a randomized uid anyways but I can understand why you want to add a non-root USER directive in the Dockerfile anyhow for usage outside of Kubernetes.

I opened a PR that removes that part of this pull request again: #9834

@levino
Copy link

levino commented Nov 6, 2018

Running a service in a containeras root is not a security issue. Stop making these "improvements". You are wasting peoples time. Running a service as root is fine if confined in a container. Show me an official best practice guide where this is recommended. You will only find blogposts by self declared "saints".

@levino
Copy link

levino commented Nov 6, 2018

FFS: Make the user configurable on runtime if you must. Then whoever wants to be "super secure" (as in encrypting a harddisk twice or killing a dead pig) can have their fun but the rest of us is not caused such pains. Just try to run parity in a docker swarm setup with load balancing and proper docker volumes (not mounted local directories). You will fail.

@levino
Copy link

levino commented Nov 6, 2018

v2.0.6 works fine, running everything as root, how docker is supposed to be used. Will running v2.0.6 create any other MAJOR issues?

@fubhy
Copy link
Contributor

fubhy commented Nov 6, 2018

You can build your own docker image on top of the current 2.0.7+ image:

FROM parity/parity:stable
WORKDIR /
USER root

RUN mv /home/parity/entrypoint.sh / \
    && mv /home/parity/bin/$TARGET /bin/ \
    && chown root:root /entrypoint.sh \
    && rm -rf /home/parity

@levino
Copy link

levino commented Nov 7, 2018

Or I just override the user setting. I also have to then use the absolute path for the binary instead of the entrypoint.sh file in the users home folder. Here is a working docker-compose file:

version: "3.3"
services:
  parity:
    image: parity/parity:v2.0.9
    ports:
      - 30303:30303
    command: >
               --ws-interface all
               --jsonrpc-interface all
               --base-path /root/parity
               --no-ancient-blocks
               --no-serve-light
               --max-peers 250
               --snapshot-peers 50
               --min-peers 50
               --mode active
               --tracing off
               --pruning fast
               --allow-ips public
               --db-compaction ssd
               --cache-size 4096
               --ws-origins all
               --ws-hosts all
               --auto-update none
    volumes:
      - parity:/root/parity
    working_dir: /root/parity
    entrypoint: /home/parity/bin/parity
    user: root      
volumes:
  parity:
    external:
      name: parity

@nihalmpatel
Copy link

I'm having the same issue on 2.1.3 using:

docker run -ti --name ethereum-node --restart=always -v ~/.local/share/io.parity.ethereum/docker/:/home/parity/.local/share/io.parity.ethereum/ -p 8545:8545 -p 8546:8546 -p 30303:30303 -p 30303:30303/udp parity/parity:v2.1.3 --ui-interface all --jsonrpc-interface all --pruning fast --base-path /home/parity/.local/share/io.parity.ethereum/

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. B0-patch-stable 🕷 Pull request should also be back-ported to the stable branch. M1-ci 🙉 Continuous integration.
Projects
None yet
Development

Successfully merging this pull request may close these issues.