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

Add support for running Docker container as non-root user #1891

Closed
wants to merge 1 commit into from

Conversation

lmmendes
Copy link
Contributor

@lmmendes lmmendes commented Jun 9, 2024

This pull request introduces the capability to run the listmonk Docker container as non-root user by specifying the UID and GID through environment variables.

The following changes have been made:

  • Updated the Dockerfile to create a non-root user (app) and group (app) using the specified UID=1001 and GID=1001.
  • Introduced default values for UID and GID, with the option to override them using build arguments:
     docker build --build-arg UID=2001 --build-arg GID=2001 -t listmonk .

Advantages:

  • Improved Security: Running containers as non-root users reduces the potential impact of security vulnerabilities. It limits the access permissions of the container, minimizing the risk of privilege escalation attacks.
  • Compliance: Many security guidelines, best practices and bigger organizations don't allow running applications as the root user. This change helps in complying with such security standards.

Breaking changes

Impacts for people running previous versions (using root, before non root user UID and GID)

  • Existing Files: Accessing files with another user that is not the root (now UID and GID) may result in permission errors when the non-root user tries to access these files.
  • Configuration files: Configuration files that need to be read or written by the application may not be accessible if their ownership is not updated to match the new UID and GID.

Testing

Built the project using

make dist
goreleaser --parallelism 1 --snapshot --clean 

Tested running the docker arm64 locally without issues.

Future work

  • I'm going to open a pull-request to "recommended" Helm chart author to introduce the spec.securityContext.runAsUser , spec.securityContext.runAsGroup including spec.containers[*].securityContext.allowPrivilegeEscalation=false in his deployment to close the loop once this pull-request gets accepted.

@lmmendes lmmendes changed the title Add support for running Docker containers as non-root users Add support for running Docker container as non-root user Jun 9, 2024
@knadh
Copy link
Owner

knadh commented Jun 10, 2024

Thanks for the PR @lmmendes. Could this affect backwards compatibility in existing installations somehow?

@mr-karan could you please take a look?

@lmmendes
Copy link
Contributor Author

Hi @knadh for people using mount volumes there will be a impact, since their files user id (uid) and group id (gid) would most probably be different from the new specified one (uid=1001 and gid=1001), by default the it would be the
Id of the docker process of root in worst case.

For people using local filesystems / mount volumes they would need to change the ownership of their files at filesystem level before updating, by doing something similar to:

chown -R 1001:1001 /path/to/volume

For people using Kubernetes they would been to update their deployment specifying the new uid and gid with something similar to (important section is spec.containers.securityContext.*):

apiVersion: v1
kind: Pod
metadata:
  name: listmonk
spec:
  containers:
  - name: listmonk
    image: listmonk/listmonk
    securityContext:
      runAsUser: 1001
      runAsGroup: 1001

Lastly if people are using Kubernetes and Persistent Volume Claims (PVCs) they would been to change they file ownership and permissions of their files, they could do it by hand but I would recommend using a initContainer, the initContainer would apply the change to file permissions before initializing listmonk container.

In the example bellow I'm running a InitContainernamedinit-permissionsthat runs abusyboxdocker image just to run thechown -R 1001:1001 /path/to/mount"` command line similar to the one that we run above:

apiVersion: v1
kind: Pod
metadata:
  name: listmonk
spec:
  initContainers:
  - name: init-permissions
    image: busybox
    command: ["sh", "-c", "chown -R 1001:1001 /path/to/mount"]
    volumeMounts:
    - name: listmonk-uploads
      mountPath: /path/to/mount
  containers:
  - name: listmonk
    image: listmonk/listmonk
    securityContext:
      runAsUser: 1001
      runAsGroup: 1001
    volumeMounts:
    - name: listmonk-uploads
      mountPath: /path/to/mount
  volumes:
  - name: listmonk-uploads
    persistentVolumeClaim:
      claimName: listmonk-pvc

@knadh This pull-request assumes that you would try to keep things simple and avoid me introducing complexity to the docker image run process.

But if you allow I can create a entrypoint.sh script that can do the following:

  • If user provides GID and UID It will setup up and assume the user and group supplied via environment.
  • if user doesn't supply a GID and UID I will run the code with the user id and group id of the docker process.

With a bit of magic on the side of the entrypoint.sh we could probably achieve a zero migration process, unless users then opt to change their UID and GID to a custom one due to security requirements.

@knadh
Copy link
Owner

knadh commented Jun 10, 2024

Hi @lmmendes. Please add the script to ensure backwards compatibility.

@lmmendes
Copy link
Contributor Author

@knadh I'm going to place this pull-request in Draft while I work on in just for people to know that this issue is being worked on while I don't create and update the pull-request.

Or if you wish just close the pull-request and I will open a new one once done.

@lmmendes
Copy link
Contributor Author

Closing this pull-request in favor of #1892 that implements a different approach to solve the problem and ensures backwards comparability as requested.

@lmmendes lmmendes closed this Jun 10, 2024
@lmmendes lmmendes deleted the feature/non-root-docker-user branch June 10, 2024 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants