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

Dockerfile*: default non-privileged user #16052

Merged
merged 4 commits into from
Apr 5, 2018
Merged

Dockerfile*: default non-privileged user #16052

merged 4 commits into from
Apr 5, 2018

Conversation

fgimenez
Copy link
Contributor

@fgimenez fgimenez commented Feb 9, 2018

Dockerfile*: default non-provileged user

Creates a geth user and group (1000:1000) and defaults to it for regular and alltools images. Also updated suggested docker command in README to mount volume in the non-privileged user home.

Closes: #15816

@GitCop
Copy link

GitCop commented Feb 9, 2018

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

  • Commit: 13b269fb290a7a56b378fd4d5cae9577d42031fb
  • Commits must be prefixed with the package(s) they modify

Please check the contribution guidelines for more details.


This message was auto-generated by https://gitcop.com

@fgimenez fgimenez changed the title Use non-privileged user in Dockerfile Dockerfile*: default non-privileged user Feb 9, 2018
@karalabe
Copy link
Member

karalabe commented Feb 9, 2018

What would the benefit of this change be?

A significant drawback is that this changes implicit APIs about the datadir location. E.g. any script mounting /root/.ethereum to external folders will immediately and silently break. For example, all private networks managed by puppeth.

@GitCop
Copy link

GitCop commented Feb 9, 2018

Thank you for your contribution! Your commits seem to not adhere to the repository coding standards

  • Commit: 21ec2cdb3d6e14f171fc969bdeeb264377c0338f
  • Commits must be prefixed with the package(s) they modify

Please check the contribution guidelines for more details.


This message was auto-generated by https://gitcop.com

@fgimenez
Copy link
Contributor Author

fgimenez commented Feb 9, 2018

Hey @karalabe thx for your comments :)

The main benefit is increased security, running a container's process as root is similar to running any other host process as root. It lets you do things like this:

fgimenez@dunwich:~$ sudo -s
root@dunwich:# echo "my very secret password" > /root/secret.txt
root@dunwich:# chmod 0600 /root/secret.txt 
root@dunwich:# exit
exit
fgimenez@dunwich:~$ cat /root/secret.txt
cat: /root/secret.txt: Permission denied
fgimenez@dunwich:~$ docker run --entrypoint /bin/cat -v /root:/root ethereum/client-go /root/secret.txt
my very secret password

A compromised container with its process running as root can potentially be very harmful. If the geth process doesn't require root IMO it would be good if it can run with the least privileges possible. I've pushed changes to overcome the issue you mentioned, changing the ownership of /root to geth:geth, let me know WDYT. Are there any other known tools that assume root?

@karalabe
Copy link
Member

karalabe commented Feb 9, 2018

Your fix doesn't help the issue. Currently the internally running geth process uses /root/.ethereum as the data directory. If you change the username, the default datadir location changes too. Any tool wanting to map the internal geth's datadir to an outside folder will break.

@fgimenez
Copy link
Contributor Author

fgimenez commented Feb 9, 2018

Aha, I've pushed changes to set geth's home to /root, just checked and with the default entrypoint the data directory is created at /root/.ethereum as you pointed out, PTAL.

@ekryski
Copy link

ekryski commented Feb 20, 2018

I'd love to see this land. 😄

What would the benefit of this change be?

This is a Docker best practice for the exact reason that @fgimenez wrote. There are ways that you can get access to the host machine as well when the container is run as root.

@fjl fjl merged commit 50dbe8e into ethereum:master Apr 5, 2018
@fgimenez fgimenez deleted the non-privileged-container-user branch April 5, 2018 12:41
@karalabe karalabe added this to the 1.8.4 milestone Apr 17, 2018
@jeanlucmongrain
Copy link

jeanlucmongrain commented Apr 22, 2018

Fatal: Failed to create the protocol stack: mkdir /root/.ethereum: permission denied

Who decided to map /root to a non-uid 0 user don't know what he's doing and ignore the basics of Linux.
And who merged that don't know either.

Why break the standard? What benefit it bring? NONE!

https://refspecs.linuxfoundation.org/FHS_3.0/fhs-3.0.pdf

Say that

3.14. /root : Home directory for the root user
(optional)
3.14.1. Purpose
The root account's home directory may be determined by developer or local preference, but this is the recommended default location.

An existing SYSTEM DIRECTORY used for an other purpose?

/var/lib/geth make sense, daemons use /var/lib/ to store data, like PostgreSQL Official Dockerfile and MySQL Official Dockerfile. geth is a daemon, even if it's often used by users on desktop system and data is stored in their equivalent of $HOME that doesn't mean it's not a daemon.
/home/geth for those who respect Linux but ignore the standard
/data could have been used, at some extend... because docker... it's used by Redis Official Docker and make 1000 times more sense than /root

/root terrible choice, this is not a bug it's a self inflicted wound

@fgimenez
Copy link
Contributor Author

Apologies for the ill-defined PR.

@karalabe
Copy link
Member

I tried to tell people this whole PR is a bad idea. Apparently running with custom users is more important than preserving APIs.

@jeanlucmongrain
Copy link

Because of this I'm stuck with 1.8.3

@karalabe
Copy link
Member

I also don't get the thing with user and group 1000. What happens if user 1000 is a plain user and not even my user?

@karalabe
Copy link
Member

I've reverted this PR (along with the rest of the cascading changes caused by it) and pushed a new release https://github.com/ethereum/go-ethereum/releases/tag/v1.8.6.

@fgimenez I'm sorry I had to revert, but the fallout was too large. I really appreciate the attempt, but we need to figure out a better was that doesn't go belly up on every user of the docker images.

@fgimenez
Copy link
Contributor Author

@karalabe np at all, my fault. Tests worked fine, I wish I could have caught the issue earlier. Anyway, lesson learned.

TuitionCoin added a commit to FinTechToken/go-ethereum that referenced this pull request May 8, 2018
* params, VERSION: v1.8.4 unstable

* core/vm: Fixed typos in core/vm/interpreter.go

* light: new CHT for ropsten (ethereum#16393)

* whisper: fix issue in topic list copy (ethereum#16381)

- Fixes ethereum#16271. What was appeneded was a pointer to
an object that changes during the iteration.
- The topic is allocated as a 4-byte array, fill partial topics
with 0s. Partial topics are currently disabled, but would
crash as they rely on the presence of byte number 3.

* core/state: uniform parameter style (ethereum#16398)

- Uniform code style.

* core/state: rework dirty handling to avoid quadratic overhead

* core/state: avoid linear overhead on journal dirty listing

* travis.yml: remove sudo requirement for PPA and Azure purge builders (ethereum#16404)

This is supposed to fix the FTP upload issue according to
travis-ci/travis-ci#9391.

* .gitattributes: enable solidity highlighting on github (ethereum#16425)

* crypto/secp256k1: catch curve parameter parse errors (ethereum#16392)

* core/state: avoid redundant addition to code size cache (ethereum#16427)

* cmd/geth: remove relOracle variable (ethereum#16434)

* eth: fix typos (ethereum#16414)

* accounts/abi: improve test coverage (ethereum#16044)

* README: change 'built in' to 'built-in'

* core/types: remove String methods from struct types (ethereum#16205)

Most of these methods did not contain all the relevant information
inside the object and were not using a similar formatting type.
Moreover, the existence of a suboptimal String method breaks usage
with more advanced data dumping tools like go-spew.

* Dockerfile: use non-privileged user account (ethereum#16052)

* cmd/geth: update template for 'geth bug' command (ethereum#16350)

* cmd/evm: print vm output when debug flag is on (ethereum#16326)

* bmt: fix comment typos (ethereum#16461)

* ethclient: remove empty object in newHeads subscription call (ethereum#16454)

* compression/rle: delete RLE compression (ethereum#16468)

* eth/downloader: flush state sync data before exit (ethereum#16280)

* state: handle nil in journal dirties

* core: add blockchain benchmarks

* cmd/puppeth: fix node deploys for updated dockerfile user

* Dockerfile.alltools: fix invalid command

* common: delete StringToAddress, StringToHash (ethereum#16436)

* common: delete StringToAddress, StringToHash

These functions are confusing because they don't parse hex, but use the
bytes of the string. This change removes them, replacing all uses of
StringToAddress(s) by BytesToAddress([]byte(s)).

* eth/filters: remove incorrect use of common.BytesToAddress

* build: add -e and -X flags to get more information on ethereum#16433 (ethereum#16443)

* core: remove stray account creations in state transition (ethereum#16470)

The 'from' and 'to' methods on StateTransitions are reader methods and
shouldn't have inadvertent side effects on state.

It is safe to remove the check in 'from' because account existence is
implicitly checked by the nonce and balance checks. If the account has
non-zero balance or nonce, it must exist. Even if the sender account has
nonce zero at the start of the state transition or no balance, the nonce
is incremented before execution and the account will be created at that
time.

It is safe to remove the check in 'to' because the EVM creates the
account if necessary.

Fixes ethereum#15119

* travis, appveyor: bump to Go 1.10.1

* travis.yml: add TEST_PACKAGES to speed up swarm testing (ethereum#16456)

This commit is meant to allow ecosystem projects such as ethersphere
to minimize CI build times by specifying an environment variable with
the packages to run tests on.

If the environment variable isn't defined the build script will test
all packages so this shouldn't affect the main go-ethereum repository.

* les: add ps.lock.Unlock() before return (ethereum#16360)

* core/state: fix bug in copy of copy State

* core/state: fix ripemd-cornercase in Copy

* core: txpool stable underprice drop order, perf fixes

* miner: remove contention on currentMu for pending data retrievals (ethereum#16497)

* ethdb: add leveldb write delay statistic (ethereum#16499)

* eth/downloader: wait for all fetcher goroutines to exit before terminating (ethereum#16509)

* cmd/clef, signer: initial poc of the standalone signer (ethereum#16154)

* signer: introduce external signer command

* cmd/signer, rpc: Implement new signer. Add info about remote user to Context

* signer: refactored request/response, made use of urfave.cli

* cmd/signer: Use common flags

* cmd/signer: methods to validate calldata against abi

* cmd/signer: work on abi parser

* signer: add mutex around UI

* cmd/signer: add json 4byte directory, remove passwords from api

* cmd/signer: minor changes

* cmd/signer: Use ErrRequestDenied, enable lightkdf

* cmd/signer: implement tests

* cmd/signer: made possible for UI to modify tx parameters

* cmd/signer: refactors, removed channels in ui comms, added UI-api via stdin/out

* cmd/signer: Made lowercase json-definitions, added UI-signer test functionality

* cmd/signer: update documentation

* cmd/signer: fix bugs, improve abi detection, abi argument display

* cmd/signer: minor change in json format

* cmd/signer: rework json communication

* cmd/signer: implement mixcase addresses in API, fix json id bug

* cmd/signer: rename fromaccount, update pythonpoc with new json encoding format

* cmd/signer: make use of new abi interface

* signer: documentation

* signer/main: remove redundant  option

* signer: implement audit logging

* signer: create package 'signer', minor changes

* common: add 0x-prefix to mixcaseaddress in json marshalling + validation

* signer, rules, storage: implement rules + ephemeral storage for signer rules

* signer: implement OnApprovedTx, change signing response (API BREAKAGE)

* signer: refactoring + documentation

* signer/rules: implement dispatching to next handler

* signer: docs

* signer/rules: hide json-conversion from users, ensure context is cleaned

* signer: docs

* signer: implement validation rules, change signature of call_info

* signer: fix log flaw with string pointer

* signer: implement custom 4byte databsae that saves submitted signatures

* signer/storage: implement aes-gcm-backed credential storage

* accounts: implement json unmarshalling of url

* signer: fix listresponse, fix gas->uint64

* node: make http/ipc start methods public

* signer: add ipc capability+review concerns

* accounts: correct docstring

* signer: address review concerns

* rpc: go fmt -s

* signer: review concerns+ baptize Clef

* signer,node: move Start-functions to separate file

* signer: formatting

* light: new CHTs (ethereum#16515)

* params: release Geth v1.8.4

* VERSION, params: begin v1.8.5 release cycle

* build: enable goimports and varcheck linters (ethereum#16446)

* core/asm: remove unused condition (ethereum#16487)

* cmd/utils: fix help template issue for subcommands (ethereum#16351)

* rpc: clean up IPC handler (ethereum#16524)

This avoids logging accept errors on shutdown and removes
a bit of duplication. It also fixes some goimports lint warnings.

* core/asm: accept uppercase instructions (ethereum#16531)

* all: fix various typos (ethereum#16533)

* fix typo

* fix typo

* fix typo

* rpc: handle HTTP response error codes (ethereum#16500)

* whisper/whisperv6: post returns the hash of sent message (ethereum#16495)

* ethclient: add DialContext and Close (ethereum#16318)

DialContext allows users to pass a Context object for cancellation.
Close closes the underlying RPC connection.

* vendor: update elastic/gosigar so that it compiles on OpenBSD (ethereum#16542)

* eth/downloader: fix for Issue ethereum#16539 (ethereum#16546)

* params: release Geth v1.8.5 - Dirty Derivative²

* VERSION, params: begin Geth 1.8.6 release cycle

* cmd/geth: update the copyright year in the geth command usage (ethereum#16537)

* Revert "Dockerfile.alltools: fix invalid command"

* Revert "cmd/puppeth: fix node deploys for updated dockerfile user"

* Dockerfile: revert the user change PR that broke all APIs

* Dockerfile: drop legacy discovery v5 port mappings

* params: release v1.8.6 to fix docker images

* VERSION, params: begin release cycle 1.8.7

* cmd/geth, mobile: add memsize to pprof server (ethereum#16532)

* cmd/geth, mobile: add memsize to pprof server

This is a temporary change, to be reverted before the next release.

* cmd/geth: fix variable name

* core/types: avoid duplicating transactions on changing signer (ethereum#16435)

* core/state: cache missing storage entries (ethereum#16584)

* cmd/utils: point users to --syncmode under DEPRECATED (ethereum#16572)

Indicate that --light and --fast options are replaced by --syncmode

* trie: remove unused `buf` parameter (ethereum#16583)

* core, eth: fix tracer dirty finalization

* travis.yml: remove obsolete brew-cask install

* whisper: Golint fixes in whisper packages (ethereum#16637)

* vendor: fix leveldb crash when bigger than 1 TiB

* core: ensure local transactions aren't discarded as underpriced

This fixes an issue where local transactions are discarded as
underpriced when the pool and queue are full.

* evm/main: use blocknumber from genesis

* accounts: golint updates for this or self warning (ethereum#16627)

* tests: golint fixes for tests directory (ethereum#16640)

* trie: golint iterator fixes (ethereum#16639)

* internal: golint updates for this or self warning (ethereum#16634)

* core: golint updates for this or self warning (ethereum#16633)

* build: Add ldflags -s -w when building aar

Smaller size on mobile is always good.
Might also solve our maven central upload problem

* cmd/clef: documentation about setup (ethereum#16568)

clef: documentation about setup

* params: release geth 1.8.7

* Zero Block Reward Post Byzantium

* Remove Difficulty Bomb

* 1 Second Block

* Lower Minimum Difficulty (#4)

* 1 Second Blocks (#5)

* Lower Minimum Difficulty

* One Second Blocks

* Lower minimum difficulty (#6)

* Enable geth compile.solidity for rpc (#7)
hackmod pushed a commit to OpenCommunityCoin/go-esn that referenced this pull request Jul 9, 2018
mariameda pushed a commit to NiluPlatform/go-nilu that referenced this pull request Aug 23, 2018
firmianavan pushed a commit to firmianavan/go-ethereum that referenced this pull request Aug 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants