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

Hardening of the Frontend docker image #377

Merged
merged 11 commits into from
Aug 26, 2021
Merged

Hardening of the Frontend docker image #377

merged 11 commits into from
Aug 26, 2021

Conversation

chevdor
Copy link
Contributor

@chevdor chevdor commented Aug 24, 2021

The main goal of this PR is to harden the image for the frontend, making it read-only.
It comes with a few challenges since we need to generate files at startup, ensuring that those are neither executable nor modifiable later.

Building

Building the image can be done easily as described in the readme:

DOCKER_USER=chevdor ./scripts/build-docker-frontend.sh

You can swap chevdor for any string, just make sure to use your docker hub username if you wish to push the image.

You can also test the pre-built image: chevdor/telemetry-frontend:pr-377.

Running

The following is taken from the modified readme:

docker run --rm -it -p 80:8000 --name frontend \
  -e SUBSTRATE_TELEMETRY_URL=ws://localhost:1234 \
  --tmpfs /var/cache/nginx:uid=101,gid=101 \
  --tmpfs /var/run:uid=101,gid=101 \
  --tmpfs /app/tmp:uid=101,gid=101 \
  --read-only \
  chevdor/telemetry-frontend

Testing

Assuming you used the command above, you can first test it the frontend works at all at http://localhost:80/. Opening the dev console and printing window.process_env, you should see whatever SUBSTRATE_TELEMETRY_URL you defined above. (I used 9944 which is obviously wrong, we connect to the backend here, not to a node...)

image
(I did not have the backend running during this test)

You can then run a few tests:

We first set the container name (easier if yours is named differently):

CONTAINER=frontend
docker exec -it $CONTAINER bash -c 'touch malicious; if test -f malicious; then echo "Red wins"; else echo "Red lost"; fi' 
docker exec -it $CONTAINER bash -c 'if rm /bin/sh; then echo "Red wins"; else echo "Red lost"; fi'
docker exec -it $CONTAINER bash -c 'if echo ";;" >> /usr/local/bin/start.sh; then echo "Red wins"; else echo "Red lost"; fi'

@chevdor chevdor marked this pull request as ready for review August 24, 2021 15:57
@chevdor
Copy link
Contributor Author

chevdor commented Aug 24, 2021

I am rebasing master.

@chevdor chevdor changed the title Hardening of the docker image Hardening of the Frontend docker image Aug 24, 2021
frontend/nginx/nginx.conf Outdated Show resolved Hide resolved
@jsdw
Copy link
Collaborator

jsdw commented Aug 24, 2021

Everything looks good to me offhand (although I haven't tested that things still work as expected yet)!

There is a part of my brain that's wondering whether the benefit of a read-only image is worth the slight complexity increase, so I'd be interested to know your thoughts around why you ended up making this change?

Anyway, I'll give it a quick test tomorrow and it feels like a net win even with the added complexity, so I'll be happy to approve once I've tested it!

@chevdor
Copy link
Contributor Author

chevdor commented Aug 24, 2021

so I'd be interested to know your thoughts around why you ended up making this change

To enter a cluster running containerized polkadot nodes and telemetry, you may take the front door... or target smaller "utility" images such as telemetry to make your way in.

This PR first of all disallow running as root. Second, it prevents adding or modifying files in the container (removing files would only help disrupt the service...), thus greatly reducing the attack surface.

@chevdor
Copy link
Contributor Author

chevdor commented Aug 24, 2021

Another similar PR #379 is on its way for the backend.

README.md Outdated Show resolved Hide resolved
@ArshamTeymouri
Copy link
Contributor

It looks great to me.

@chevdor
Copy link
Contributor Author

chevdor commented Aug 25, 2021

If that helps for the tests, I can bring a justfile similar to paritytech/substrate-api-sidecar#648. It makes it easier to test.

docker-compose.yml Outdated Show resolved Hide resolved
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 once that docker-compose tmpfs is on the frontend section :)

I merged this with the backend one and forced a rebuild of images, and everything seemed to work as expected!

@jsdw jsdw self-requested a review August 25, 2021 11:23
@chevdor
Copy link
Contributor Author

chevdor commented Aug 25, 2021

LGTM once that docker-compose tmpfs is on the frontend section :)

@jsdw, indeed this is mandatory with using --read-only.

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.

Looks great :)

.gitignore Show resolved Hide resolved
@chevdor chevdor merged commit 238d529 into master Aug 26, 2021
@chevdor chevdor deleted the wk-harden branch August 26, 2021 10:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants