KK-1417 | Fix SonarCloud security issues & some of the hotspots #483
Conversation
|
KUKKUU-API branch is deployed to platta: https://kukkuu-pr483.api.dev.hel.ninja 🚀🚀🚀 |
TestCafe result is success for https://kukkuu-pr483.api.dev.hel.ninja 😆🎉🎉🎉 |
|
KUKKUU-API branch is deployed to platta: https://kukkuu-pr483.api.dev.hel.ninja 🚀🚀🚀 |
TestCafe result is success for https://kukkuu-pr483.api.dev.hel.ninja 😆🎉🎉🎉 |
karisal-anders
force-pushed
the
KK-1417-fix-security-issues
branch
from
d9c1271 to
58b50c1
Compare
|
KUKKUU-API branch is deployed to platta: https://kukkuu-pr483.api.dev.hel.ninja 🚀🚀🚀 |
TestCafe result is success for https://kukkuu-pr483.api.dev.hel.ninja 😆🎉🎉🎉 |
karisal-anders
force-pushed
the
KK-1417-fix-security-issues
branch
2 times, most recently
from
5cacee4 to
7736ae0
Compare
|
KUKKUU-API branch is deployed to platta: https://kukkuu-pr483.api.dev.hel.ninja 🚀🚀🚀 |
TestCafe result is success for https://kukkuu-pr483.api.dev.hel.ninja 😆🎉🎉🎉 |
karisal-anders
force-pushed
the
KK-1417-fix-security-issues
branch
from
7736ae0 to
849fc40
Compare
|
KUKKUU-API branch is deployed to platta: https://kukkuu-pr483.api.dev.hel.ninja 🚀🚀🚀 |
TestCafe result is success for https://kukkuu-pr483.api.dev.hel.ninja 😆🎉🎉🎉 |
|
KUKKUU-API branch is deployed to platta: https://kukkuu-pr483.api.dev.hel.ninja 🚀🚀🚀 |
1 similar comment
|
KUKKUU-API branch is deployed to platta: https://kukkuu-pr483.api.dev.hel.ninja 🚀🚀🚀 |
TestCafe result is success for https://kukkuu-pr483.api.dev.hel.ninja 😆🎉🎉🎉 |
karisal-anders
force-pushed
the
KK-1417-fix-security-issues
branch
from
0916d26 to
9648bb7
Compare
|
KUKKUU-API branch is deployed to platta: https://kukkuu-pr483.api.dev.hel.ninja 🚀🚀🚀 |
TestCafe result is success for https://kukkuu-pr483.api.dev.hel.ninja 😆🎉🎉🎉 |
karisal-anders
force-pushed
the
KK-1417-fix-security-issues
branch
from
9648bb7 to
41ae1e7
Compare
karisal-anders
changed the title
karisal-anders
changed the title
karisal-anders
force-pushed
the
KK-1417-fix-security-issues
branch
from
41ae1e7 to
e7c4f80
Compare
karisal-anders
changed the title
|
KUKKUU-API branch is deployed to platta: https://kukkuu-pr483.api.dev.hel.ninja 🚀🚀🚀 |
TestCafe result is success for https://kukkuu-pr483.api.dev.hel.ninja 😆🎉🎉🎉 |
karisal-anders
force-pushed
the
KK-1417-fix-security-issues
branch
from
e7c4f80 to
93fe9c8
Compare
karisal-anders
changed the title
|
KUKKUU-API branch is deployed to platta: https://kukkuu-pr483.api.dev.hel.ninja 🚀🚀🚀 |
TestCafe result is success for https://kukkuu-pr483.api.dev.hel.ninja 😆🎉🎉🎉 |
Dockerfile
Outdated
| # chmod=755 = rwxr-xr-x i.e. owner can read, write and execute, group and others can read and execute | ||
| COPY --chown=root:root --chmod=755 requirements.txt /app/requirements.txt | ||
| COPY --chown=root:root --chmod=755 requirements-not-from-pypi.txt /app/requirements-not-from-pypi.txt | ||
| COPY --chown=root:root --chmod=755 requirements-prod.txt /app/requirements-prod.txt |
There was a problem hiding this comment.
Every time a COPY is called, a new layer is created. This reduces the number of layers in Docker image, making it smaller and faster to build:
| # chmod=755 = rwxr-xr-x i.e. owner can read, write and execute, group and others can read and execute | |
| COPY --chown=root:root --chmod=755 requirements.txt /app/requirements.txt | |
| COPY --chown=root:root --chmod=755 requirements-not-from-pypi.txt /app/requirements-not-from-pypi.txt | |
| COPY --chown=root:root --chmod=755 requirements-prod.txt /app/requirements-prod.txt | |
| # chmod=755 = rwxr-xr-x i.e. owner can read, write and execute, group and others can read and execute | |
| COPY --chown=root:root --chmod=755 requirements.txt requirements-not-from-pypi.txt requirements-prod.txt /app/ |
All these 3 copies share a same theme: "copy the requirements".
There was a problem hiding this comment.
Actually, I think it could help in some cases (e.g. when working inside pod locally), that also the requirements-dev would already be included in the base with other requirements. It's not a huge file, it can be helpful and it does not harm anybody. Then it's COPY could be removed from development stage.
There was a problem hiding this comment.
See #483 (comment)
Dockerfile
Outdated
| # "Recursively copying context directories is security-sensitive" i.e. | ||
| # https://rules.sonarsource.com/docker/RSPEC-6470/ | ||
| # see .dockerignore for info on what is not copied here: | ||
| COPY --chown=root:root --chmod=755 . /app/ |
There was a problem hiding this comment.
Doesn't this include also the dev requirements copied in the previous COPY command? Should the previous COPY be replaced with this one?
There was a problem hiding this comment.
Moved the COPY command to the appbase stage and removed it from development and production stages.
README.md
Outdated
| ### Generating secret key for Django | ||
|
|
||
| Django needs a value for [SECRET_KEY](https://docs.djangoproject.com/en/4.2/ref/settings/#secret-key) to start. | ||
| You can generate a value for it using Python & Django: | ||
| ```python | ||
| from django.core.management.utils import get_random_secret_key | ||
| get_random_secret_key() | ||
| ``` | ||
|
|
||
| or just Python (adapted from Django's | ||
| [get_random_secret_key](https://github.com/django/django/blob/5.1.6/django/core/management/utils.py#L79C5-L84) & | ||
| [get_random_string](https://github.com/django/django/blob/5.1.6/django/utils/crypto.py#L51-L62)): | ||
| ```python | ||
| import secrets, string | ||
| allowed_chars = string.ascii_lowercase + string.digits + "!@#$%^&*(-_=+)" | ||
| "".join(secrets.choice(allowed_chars) for i in range(50)) | ||
| ``` |
There was a problem hiding this comment.
While it's good to mention that these variables are really important in live server usage, I would say that this all is too much in local development use. While developing locally, the secret key can be pretty much anything, so this makes the process overwhelming and "scary". However, it's not good practise to have even a local SECRET_KEY in version control.
Could we add something like
If you prefer, you can use a shorter, manually generated key for local development. Just remember to use a strong, long, randomly generated key in production.
There was a problem hiding this comment.
That sounds reasonable, and I thought about something like this, but didn't come up with a good way to say this. I'll revamp this section a bit.
There was a problem hiding this comment.
Simplified and clarified the section in a force push.
karisal-anders
force-pushed
the
KK-1417-fix-security-issues
branch
from
93fe9c8 to
d5779ce
Compare
also: - add explicit dependency for cryptography as it is now used in tests - update all dependencies to latest refs KK-1417
karisal-anders
force-pushed
the
KK-1417-fix-security-issues
branch
from
d5779ce to
24a5b21
Compare
See SonarCloud rule "Django secret keys should not be disclosed" i.e. https://rules.sonarsource.com/secrets/RSPEC-6687/ refs KK-1417
Fixes SonarCloud security hotspot python:S3752: "Make sure allowing safe and unsafe HTTP methods is safe here. Allowing both safe and unsafe HTTP methods is security-sensitive": https://rules.sonarsource.com/python/RSPEC-3752/ refs KK-1417
Removes the SonarCloud security hotspot python:S2245: "Make sure that using this pseudorandom number generator is safe here. Using pseudorandom number generators (PRNGs) is security-sensitive": https://rules.sonarsource.com/python/RSPEC-2245/ The bottom line here is that it's safe to use pseudorandom number generator here as this is only for generating test data. refs KK-1417
Fixes SonarCloud security hotspot python:S5443: "Make sure publicly writable directories are used safely here. Using publicly writable directories is security-sensitive": https://rules.sonarsource.com/python/RSPEC-5443/ refs KK-1417
Fixes the following SonarCloud security hotspots in Dockerfile: "Make sure no write permissions are assigned to the copied resource. Allowing non-root users to modify resources copied to an image is security-sensitive" i.e. SonarCloud rule docker:S6504 i.e. https://rules.sonarsource.com/docker/RSPEC-6504/ Also: - Update `.dockerignore` file to be more general - Add comments to `Dockerfile` related to SonarCloud security hotspot docker:S6470 i.e. "Recursively copying context directories is security-sensitive" https://rules.sonarsource.com/docker/RSPEC-6470/ refs KK-1417
karisal-anders
force-pushed
the
KK-1417-fix-security-issues
branch
from
24a5b21 to
19d8ba2
Compare
|
KUKKUU-API branch is deployed to platta: https://kukkuu-pr483.api.dev.hel.ninja 🚀🚀🚀 |
TestCafe result is success for https://kukkuu-pr483.api.dev.hel.ninja 😆🎉🎉🎉 |
|
@nikomakela @jakezu This is ready for code review, and the points raised in the comments so far have been addressed. |
fix https://rules.sonarsource.com/secrets/RSPEC-6698/ by not using a hardcoded DATABASE_URL in compose.yaml or settings.py but having a default value for it in `.env.example` file also: - combine `.env.keycloak.example` and `docker-compose.env.example` into `.env.example` - make Docker Compose use `.env` for environment variables - update README related to environment variable files - add more .env.* files explicitly into .gitignore - fix comment in docker-entrypoint.sh (was the other way around) refs KK-1417
|
Added a single commit i.e. 1960eaa |
karisal-anders
changed the title
|
KUKKUU-API branch is deployed to platta: https://kukkuu-pr483.api.dev.hel.ninja 🚀🚀🚀 |
|
TestCafe result is failed for https://kukkuu-pr483.api.dev.hel.ninja 😿💢💥💥 |
TestCafe result is success for https://kukkuu-pr483.api.dev.hel.ninja 😆🎉🎉🎉 |
Description
Please read commit messages for details about what has been done.
Fix SonarCloud security issues & some of the hotspots.
Related