-
Notifications
You must be signed in to change notification settings - Fork 60
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
Feature/boefjes to oci images #2709
Conversation
Change the signature to a dictionary for containerized boefjes.
…cess triggered through `make kat`
Use the `nonroot` user in the boefje images. Explanation for setting the DOCKER_DEFAULT_PLATFORM in the boefje Makefile
Handle the boefje api requests in Python as well, using httpx Use the docker_adapter as entrypoint, making the entrypoint redundant.
Handle the boefje api requests in Python as well, using httpx Use the docker_adapter as entrypoint, making the entrypoint redundant
Checklist for QA:
What works:DNSSec boefje works and appear no error messages in the logs. After Review is done it can be moved to Ready to merge. I followed the following approach while timing: I let the DNSrecords and DNSzone boefjes run and finish, then enable the dnssec boefje and start the stopwatch. Data is against mispo.es with L1 and timed until the KAT-NO-DNSSEC Finding Type is completed under the Normalizer tab:
What doesn't work:n/a Bug or feature?:n/a |
domain = input_["name"] | ||
|
||
client = docker.from_env() | ||
|
||
# check for string pollution in domain. This check will fail if anything other characters than a-zA-Z0-9_.- are | ||
# present in the hostname | ||
if not re.search(r"^[\w.]+[\w\-.]+$", domain.lower()): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we checked punicode domainname compatibility here?
* main: (51 commits) Fix static files for container images/Debian packages when DEBUG is on (#2742) OOI selection at Aggregate report does not remember changed selection (#2619) fix schema errors on empty / missing schemas (#2744) Updated `phonenumbers` and `django-phonenumber-field` (#2757) Remove octopoes coverage workflow (#2755) Bump actions/configure-pages from 4 to 5 (#2745) Add xtdb-cli tool to Octopoes (#2733) Dont report vulnerabilities without version info of the software for snyk (#2730) Feature/boefjes to oci images (#2709) Query non-reference fields and subclass-specific fields through path queries (#2662) Fix in System Specific (#2732) Plugins overview in appendix not showing any plugins (#2694) Feat stepper design v2 (#2704) Undo project-directory in Rocky (#2734) Remove Docker Compose: "version" (#2718) Upgrade `pre-commit` hooks (#2729) Fix #1739 (#2705) Improve generate report (#2633) Fix critical vulnerability counter (#2712) Fix pdf alignment (#2674) ...
Changes
This adds Docker (OCI) image builder setups for both python-native boefjes and boefjes that were using docker containers already. As an example for this first version, only
dns-records
anddns-sec
were changed.To get it working for the dnssec image, I had to change the boefje_meta argument to be a dict since else we'd be adding pydantic there and a job_models file. This can be a point of discussion.
I also added a step in the
make build
that we run during installations. We now also domake -C boefjes images
that builds the two images. There the target also provides examples of how to manually run the docker build commands with the right arguments and tags.One issue here is that some alpine images only have python 3.5 available which messes a bit with some of the APIs in the standard lib. Hence the ignoring the dnssec main.py for pre-commit.We'll try to migrate to new python3.11-slim images where possible. In this case, it was possible.I used
boefje.Dockerfile
per the Dockerfile naming convention and the fact that potentially normalizer Dockerfiles could perhaps also live in the same directory with the namenormalizer.Dockerfile
for instance.Follow up: do we still care about the group and user ids? We do not pass them anyways currently and the main reason for implementing them was specific requirements for the apps themselves, if I recall correctly.No, removed.Issue link
Closes #2443
Demo & QA notes
This feature should not change the current flow or setup, but please verify that both the
dns-records
anddns-sec
boefje work properly on a clean install (i.e. aftermake kat
ormake reset
, which should build the images). Perhaps a rough notion of the new performance would also be nice as the risk here is that the new setup will induce significant overhead to smaller boefjes.Code Checklist
Communication
.env
changes files if required and changed the.env-dist
accordingly.Checklist for code reviewers:
Copy-paste the checklist from the docs/source/templates folder into your comment.
Checklist for QA:
Copy-paste the checklist from the docs/source/templates folder into your comment.