Improve Docker configuration #497
Conversation
|
While you're at it , maybe we could fix these misconfigs found via https://github.com/docker/docker-bench-security . Pasting he relevant misconfigs here. |
|
@sbs2001 These results appear to be from the old docker configs. Can you confirm if you used the configs from this branch or from the main ? |
Hritik14
force-pushed
the
docker_bugfix
branch
9 times, most recently
from
8f1fa6a
to
617b758
Compare
|
Btw this has a side effect that it fixes #462 |
|
@rolfschr can you please update the nix tests in this one |
After 312160c docker-compose doesn't load static files as the server runs in development mode without DEBUG enabled. Also, this commit makes sure it is loud and heard that running via docker is not safe. Signed-off-by: Hritik Vijay <[email protected]>
DJANGO_DEV is obsolete and now nowhere exists in the entire codebase Signed-off-by: Hritik Vijay <[email protected]>
The docker setup present earlier used django development server to host vulnerablecode, this is neither recommended nor safe. Also the server did not drop privileges. Docker environment vars are now easily configured via docker.env, also the nginx template present in etc/nginx could be used in a non docker setup with slight modifications. Signed-off-by: Hritik Vijay <[email protected]>
Inspired by ScancodeIO. This Makefile makes the installation easy as a cake. The virtualenv zipapp used in this commit is obtained from https://bootstrap.pypa.io/virtualenv/3.8/virtualenv.pyz Note: sqlite support in this makefile is not implemented yet. It will be done in next commits in this PR. Signed-off-by: Hritik Vijay <[email protected]>
This will be essential after later commits as the existence of SECRET_KEY in .env or environment is going to be non-optional. Signed-off-by: Hritik Vijay <[email protected]>
This is not essential but promotes uniformity. Also, `collectstatic` is removed from this test as this is not really necessary. Signed-off-by: Hritik Vijay <[email protected]>
Inspired by scancodeio, we now have a consistent ALLOWED_HOSTS environment variable support. SECRET_KEY no longer defaults to insecure values and is required to be generated. Static files are now served in /var/vulnerablecode/static which is consistent to scancodeio. Signed-off-by: Hritik Vijay <[email protected]>
File hierarchy inside docker container now looks similar to scancodeio. postgres data is now a docker volume, so it will persist even after recreation. .dockerignores makes sure that we don't overpopulate docker images, improving build times. A common environment variable for containers is kept in docker.env The python image we are using now explicitly states python:3.8 NOTE: We should have a postgres health check script which uses python manage.py check --database default which comes with max_retries and backoffs for `vulnerablecode` container. Currently, docker's `restart:` option is used but it has a caveat that it'll restart on non-db errors as well. Signed-off-by: Hritik Vijay <[email protected]>
This calls for a lot of changes in other parts of documentation as well. This will be done in later PRs. This commit marks the end of this PR. Signed-off-by: Hritik Vijay <[email protected]>
Hritik14
force-pushed
the
docker_bugfix
branch
2 times, most recently
from
1f097f4
to
d696c72
Compare
This commit gets rid of using any type of cache (introduced in 135c95f) This is to ensure having no surprises in future. This was discussed in https://github.com/nexB/vulnerablecode/wiki/WeeklyMeetings#meeting-on-tuesday-2021-07-27-at-1300-utc Signed-off-by: Hritik Vijay <[email protected]>
These are redundant after PR# 295. Signed-off-by: Hritik Vijay <[email protected]>
|
@pombredanne nits resolved. Merging this now |
|
@rolfschr ping... |
| WORKDIR /vulnerablecode | ||
| ADD . /vulnerablecode/ | ||
| RUN pip install -r requirements.txt && \ | ||
| DJANGO_DEV=1 python manage.py collectstatic |
There was a problem hiding this comment.
Why do you remove collectstatic in the dockerfile?
For me it makes more sense to have the collectstatic in the base docker image rather to do it at every startup in the docker compose.
We have vulnerable code setup in kubernetes and this change did break it.
There was a problem hiding this comment.
No specific reason from my side, except that the same structure is being used in our other project.
https://github.com/nexB/scancode.io/blob/main/docker-compose.yml
If we revert, we should revert both.
There was a problem hiding this comment.
I see in docker-compose that you actully do the collectstatic in a shared volume so that the static is served by nginx.
I think this works well for this docker-compose design.
It won't work well in a kubernetes deployment as in kubernetes there is usually several instance of the webapp container which will compete doing the migration and doing the static collect.
In my kubernetes design I didn't bother serving the static files using a dedicated static server.
I think it make sense to ship the docker container with static files inside it, even if in the docker-compose you regenerate it. This will anyway allow the two strategies, with or without static file server.
There was a problem hiding this comment.
well, looks like django is not recommending serving static file by the app. I though this was only a performance best practice (which I don't bother with my 4 users), but they say it is also insecure.
https://docs.djangoproject.com/en/3.2/ref/contrib/staticfiles/#django.contrib.staticfiles.views.serve
So I think I will have to deploy an nginx as well..
There was a problem hiding this comment.
Yes. Staticfiles are only supposed to be served by a proper webserver or cdn. You could serve them via the development server using the --insecure flag, and it is as it sounds like - insecure.
There was a problem hiding this comment.
I see in docker-compose that you actully do the collectstatic in a shared volume so that the static is served by nginx.
I don't think we would have any trouble having the shared volume even if we invoke collectstatic inside the docker image. The best rationale I could think of is, because staticfiles is meant for production, it should be invoked in production.
This PR creates an organized Docker environment with proper documentation inspired from scancodeio.
A
Makefileis introduced which eases the installation greatly. Also, nowSECRET_KEYis no longer optional. Commit messages contain further descriptive text.With Makefile at hand, documentation of other parts also needs to be improved but that is out of realm of this PR.