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

feat(clamav): clamav arm64 support using debian based image #4483

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

jogerj
Copy link

@jogerj jogerj commented Apr 2, 2024

Solves #4223

Implementation based on docker/buildx#805 (comment)

edit: sorry for force pushes, not used to using DCO and needed a couple retries

@szaimen szaimen added 3. to review Waiting for reviews enhancement New feature or request dependencies labels Apr 2, 2024
@szaimen szaimen added this to the next milestone Apr 2, 2024
Signed-off-by: Jonathan Joewono <[email protected]>
Signed-off-by: jogerj <[email protected]>
Signed-off-by: Jonathan Joewono <[email protected]>
Signed-off-by: Jonathan Joewono <[email protected]>
Signed-off-by: jogerj <[email protected]>
Signed-off-by: Jonathan Joewono <[email protected]>
@Zoey2936
Copy link
Collaborator

Zoey2936 commented Apr 2, 2024

I would maybe add --platform (https://docs.docker.com/reference/dockerfile/#from) to both arch FROM steps

@Zoey2936
Copy link
Collaborator

Zoey2936 commented Apr 2, 2024

Also ARG TARGETARCH should be added to the beginning of the Dockerfile

@jogerj
Copy link
Author

jogerj commented Apr 2, 2024

hadolint reports DL3006 but it's a false positive due to arg-generated image name. See hadolint/hadolint#979

we can fix this with

# hadolint ignore=DL3006
FROM build-${TARGETARCH}

Also ARG TARGETARCH should be added to the beginning of the Dockerfile

BuildKit should already resolve this value so we don't need to redefine this. docker/buildx#574 (comment)

TODO

So for amd64, it will retain its current behavior with alpine images but for arm64 this will be based off debian image

  • How do we test clamav on arm64? Will this be released as beta or something?
  • Update docs that says "clamav on arm64 not supported"

Signed-off-by: Jonathan Joewono <[email protected]>
@Zoey2936
Copy link
Collaborator

Zoey2936 commented Apr 2, 2024

{% if isAnyRunning == true or is_x64_platform == false %}
you also need to remove this if part and move the script line to the other script lines in the isAnyRunning part below

@Zoey2936
Copy link
Collaborator

Zoey2936 commented Apr 2, 2024

BuildKit should already resolve this value so we don't need to redefine this. docker/buildx#574 (comment)

didn't know that, but for RUN steps it seems to be needed

@szaimen
Copy link
Collaborator

szaimen commented Apr 2, 2024

Honestly, I am still not sure about this. I am wondering if we should rather go the Mailcow way: https://github.com/mailcow/mailcow-dockerized/blob/master/data/Dockerfiles/clamd/Dockerfile because of security and size concerns. However this would put the maintenance on our burden. So both possibilities are not great imho...

@Zoey2936
Copy link
Collaborator

Zoey2936 commented Apr 2, 2024

Honestly, I am still not sure about this. I am wondering if we should rather go the Mailcow way: https://github.com/mailcow/mailcow-dockerized/blob/master/data/Dockerfiles/clamd/Dockerfile because of security and size concerns. However this would put the maintenance on our burden. So both possibilities are not great imho...

yes I would also prefer downloading clamav from alpine, but that would remove version pinning

@jogerj
Copy link
Author

jogerj commented Apr 2, 2024

Honestly, I am still not sure about this. I am wondering if we should rather go the Mailcow way: https://github.com/mailcow/mailcow-dockerized/blob/master/data/Dockerfiles/clamd/Dockerfile because of security and size concerns. However this would put the maintenance on our burden. So both possibilities are not great imho...

well we can just use the compiled image from mailcow https://hub.docker.com/r/mailcow/clamd then the question just becomes do we want to have mailcow as a dependency 🤔 after careful delibration, this is likely not what we want as mailcow's clamd includes custom rules and whitelists specific for emails.

mailcow is GPLv3 licensed -> should be compatible to Nextcloud if we choose to reuse and modify it.

@jogerj
Copy link
Author

jogerj commented Apr 2, 2024

I would maybe add --platform (https://docs.docker.com/reference/dockerfile/#from) to both arch FROM steps

it seems adding this breaks a different hadolint rule DL3029, although I'm not sure if this should be added as exception.

@Zoey2936
Copy link
Collaborator

Zoey2936 commented Apr 2, 2024

it seems adding this breaks a different hadolint rule DL3029, although I'm not sure if this should be added as exception.

can be ignored

@jogerj
Copy link
Author

jogerj commented Apr 3, 2024

It seems that clamav-docker maintainers refuse to add alpine arm64 images. If we fully commit to just use clamav-debian images, the changes would've been very minimal. Size delta is ~60MB while security wise it's just as good imo.

# syntax=docker/dockerfile:latest
# Probably from this file: https://github.com/Cisco-Talos/clamav-docker/blob/main/clamav/1.3/debian/Dockerfile
FROM clamav/clamav-debian:1.3.0-24

COPY clamav.conf /tmp/clamav.conf

# DL3008 warning: Pin versions in apt get install. Instead of `apt-get install <package>` use `apt-get install <package>=<version>`
# hadolint ignore=DL3008
RUN set -ex; \
    apt-get update; \
    apt-get install --no-install-recommends -y \
        tzdata \
    ; \
    rm -vrf /var/lib/apt/lists/*; \
    cat /tmp/clamav.conf >> /etc/clamav/clamd.conf; \
    rm /tmp/clamav.conf; \
    mkdir -p /var/run/clamav /run/lock; \
    chown -R clamav:clamav /var/run/clamav /run/clamav /var/log/clamav /var/lock /run/lock; \
    chmod 777 -R /var/run/clamav /run/clamav /var/log/clamav /var/lock /run/lock /tmp

VOLUME /var/lib/clamav

USER clamav

LABEL com.centurylinklabs.watchtower.enable="false"

P.S. I experienced issues when building the image on my Windows machine. I would recommend adding .gitattributes file to ensure line endings of clamav.conf remain LF, otherwise clamav won't parse the resulting conf file correctly

@szaimen szaimen modified the milestones: v8.3.0, next May 30, 2024
@szaimen szaimen modified the milestones: v9.0.0, next Jun 7, 2024
@szaimen szaimen modified the milestones: v9.1.0, next Jun 25, 2024
@szaimen szaimen modified the milestones: v9.2.0, next Jul 11, 2024
@szaimen szaimen modified the milestones: v9.3.0, next Jul 19, 2024
@szaimen szaimen modified the milestones: v9.4.0, next, v9.4.1 Jul 29, 2024
@szaimen szaimen modified the milestones: v9.5.0, next, v9.5.1 Sep 4, 2024
@szaimen szaimen modified the milestones: v9.6.0, next Sep 18, 2024
@szaimen szaimen modified the milestones: v9.7.0, next Oct 10, 2024
@szaimen szaimen added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Oct 21, 2024
@szaimen szaimen marked this pull request as draft October 22, 2024 09:44
@szaimen szaimen modified the milestones: v9.8.0, next Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress dependencies enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants