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

fix: improve security of the docker container #648

Merged
merged 4 commits into from
Aug 26, 2021
Merged

Conversation

chevdor
Copy link
Contributor

@chevdor chevdor commented Aug 25, 2021

In a containerized environment, weaker containers can be used as entry point. Thus the importance to secure Docker images.

What does the PR do:

This PR brings the following:

  • fix the docker-compose.yaml (it was not working with the parity image)
  • no longer run as root in the container, run as user node instead
  • update doc and docker-compose to invite users using --read-onlyso that the root file system cannot be changed
  • add justfile to easily build and test (see notes below)
  • fix a few security issues (see tests below)

Security issues

Assuming you have no docker image named substrate-api-sidecar on your machine to get started. You can cleanup using docker images | grep sidecar | awk '{ print $3 }' | xargs docker rmi -f.
The following tests use just, read below for details about just.

then run:

docker run -it -d --name substrate-api-sidecar parity/substrate-api-sidecar
just docker-test

NOTE: You will see a few errors, ignore them, this is becasue we did not build the new image yet locally.

we get:

$ just docker-test
Unable to find image 'substrate-api-sidecar:latest' locally
docker: Error response from daemon: pull access denied for substrate-api-sidecar, repository does not exist or may require 'docker login': denied: requested access to the resource is denied.
See 'docker run --help'.
❌ Red wins
❌ Red wins
❌ Red wins
❌ Red wins
❌ Red wins
❌ Red wins
substrate-api-sidecar

The red team 🕵🏻‍♂️ wins too much here...

Let's now build the new image:

just docker-build

and we test again:

just docker-test

we get:

just docker-test
39e6e4cc03bff8da96b336fe7ff9b21f5bdf8297bd52cbdb507b140b1e3e71d6
✅ Red lost
✅ Red lost
✅ Red lost
sh: can't create /usr/src/app/build/src/main.js: Permission denied
✅ Red lost
sh: can't create /usr/src/app/index.html: Read-only file system
✅ Red lost
✅ Red lost
substrate-api-sidecar

This time, we make it a little harder for the red team 🕵🏻‍♂️.

NOTE: The errors we see are fine, those are the failed attemtps.

Important note

While this PR makes the Docker image and the container deployed with it more secure, just using the new image does not get all the benefits if the user is not running the container as --read-only. This is why this flag as been set as default in the doc and in the docker-compose. It may be wise to let our known users know about this.

Justfile

The jusfile is a convenience that allows NOT introducing several scripts to build and test the containers.
Having just installed, you can simply use just or just help to discover more about the usable scripts.

Here is a run:

$ just help
just --list
Available recipes:
    docker-build repo=default_repo # Build the docker image
    docker-publish repo=default_repo # Publish the image after building it
    docker-test repo=default_repo  # A few simple security checks on the docker image
    help                           # Shows the list of commands

NOTE: The introduction of just is opinionated and not mandatory for this PR (although makes it much easier to test).

@chevdor chevdor added A2 - Insubstantial Does not affect logic/API (e.g. typo in docs) B0 - Silent PR should not be mentioned in release notes D1 - Docker Issue regarding Docker labels Aug 25, 2021
@chevdor chevdor added the A0 - Please Review PR is ready for review label Aug 25, 2021
@chevdor chevdor requested review from emostov, jsdw and dvdplm and removed request for emostov August 25, 2021 09:25
@chevdor chevdor marked this pull request as ready for review August 25, 2021 09:25
@jsdw jsdw requested a review from TarikGul August 25, 2021 09:52
README.md Outdated Show resolved Hide resolved
@@ -7,11 +7,12 @@ services:
- ~/polkadot-data:/data
# ports:
# - "9944:9944"
command: 'polkadot --chain polkadot --unsafe-ws-external --rpc-cors=all'
command: '--chain polkadot --unsafe-ws-external --rpc-cors=all'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did this line work at all previously (or did older images not default to running polkadot)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not check but it is very likely that I initially used my chevdor/polkadot image here. The difference with the parity one being that the parity image uses polkadot as entrypoint and my image does not. I guess someone updated from chevdor to parity without changing the call.

So the only 2 options are:

  • use chevdor/polkadot and start the command with polkadot
  • use parity/polkadot and omit the polkadot binary in the command

My image is not built from CI so I do NOT recommend any longer using it (expect for people who want to build their own image themself).

Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

LGTM!

I haven't tested the justfile, but I can see that you have, and the other changes look fairly trivial assuming the docker-compose command is all gravy :)

Co-authored-by: James Wilson <james@jsdw.me>
Copy link
Member

@TarikGul TarikGul left a comment

Choose a reason for hiding this comment

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

LGTM

While this PR makes the Docker image and the container deployed with it more secure, just using the new image does not get all the benefits if the user is not running the container as --read-only. This is why this flag as been set as default in the doc and in the docker-compose. It may be wise to let our known users know about this.

👍

@TarikGul TarikGul changed the title improve security of the docker container fix: improve security of the docker container Aug 26, 2021
@chevdor chevdor merged commit bca36aa into master Aug 26, 2021
@chevdor chevdor deleted the wk-docker-hardening branch August 26, 2021 12:19
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0 - Please Review PR is ready for review A2 - Insubstantial Does not affect logic/API (e.g. typo in docs) B0 - Silent PR should not be mentioned in release notes D1 - Docker Issue regarding Docker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants