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

Run as non root #606

Closed
wants to merge 13 commits into from
Closed

Run as non root #606

wants to merge 13 commits into from

Conversation

bmonty
Copy link

@bmonty bmonty commented Apr 21, 2022

bmonty ✨ Feature Medium bmonty /run_as_non_root → Lissy93/dashy Commits: 13 | Files Changed: 12 | Additions: 39 Unchecked Tasks 🚫 Merge Conflicts Powered by Pull Request Badge

Category:
Feature

Overview

WHY
The process serving the web application DOES NOT have access to change any of the files being used to serve the dashboard. The web application DOES need to be able to modify the config.yml and manage backup files. The vue-cli-service DOES need to be able to modify the site's files, but DOES NOT serve files over the network.

This change is to mitigate risk in the event there was a flaw in the node http module or the express framework that allowed someone to take over the server process. If this happened, the process wouldn't be able to change any of Dashy's site content or add files to the server.

HOW
Bump node image version to node:16.14.2-alpine.

Changes to the Docker image build so Dashy can run as a non-root user.

  • package.json added the option --no-clean to the build-watch script. This is required to prevent the startup scripts from removing and recreating the destination folder (set to /app/dist). While running as a non-root user, the app doesn't have permissions to do this.
  • Dockerfile
    • Change the port to 8080. Port 80 is privileged and the non-root user doesn't have access to open it.
    • Add UID and GID variables. The ID value isn't that important as long as it's not 0.
    • Add a DEST_DIR variable to control where build output goes.
    • Add a PUBLIC_DIR variable to control where the site's content goes.
    • Add a RUN block to set permissions on the DEST_DIR and PUBLIC_DIR so the non-root user can write.
    • Add a USER directive to make the process run as the non-root user.

*** This change will likely break existing installs because Dashy will no longer be running on port 80.

Issue Number
#495 #502

New Vars

These variables are only used in the Dockerfile. Since they can't be changed post-build, they aren't included in the docs.

UID - User ID for the non-root user.
GID - Group ID for the non-root user.
DEST_DIRECTORY - Added to the Dockerfile. This is the directory where the vie-cli-service build command outputs results. This environment variable is used in the Dockerfile build process to update the permissions on this folder so the non-root user has access to write to this location.

Code Quality Checklist (Please complete)

  • All changes are backwards compatible
  • All lint checks and tests are passing
  • There are no (new) build warnings or errors
  • (If a new config option is added) Attribute is outlined in the schema and documented
  • (If a new dependency is added) Package is essential, and has been checked out for security or performance
  • Bumps version, if new feature added

@bmonty bmonty requested a review from Lissy93 as a code owner April 21, 2022 12:24
@netlify
Copy link

netlify bot commented Apr 21, 2022

Deploy Preview for dashy-dev ready!

Name Link
🔨 Latest commit 8d9c43c
🔍 Latest deploy log https://app.netlify.com/sites/dashy-dev/deploys/6273c95e3fef010008d8c289
😎 Deploy Preview https://deploy-preview-606--dashy-dev.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@viezly
Copy link

viezly bot commented Apr 21, 2022

Changes preview:

Legend:

👀 Review pull request on Viezly

Copy link
Owner

@Lissy93 Lissy93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, this is awesome :)
I'm just thinking what the best way to roll this out without breaking everyone's instance likely is...

@Lissy93
Copy link
Owner

Lissy93 commented Apr 21, 2022

Were you able to get the saving config working?
I'm getting EACCES: permission denied, copyfile './public/conf.yml' -> './public/conf-1650552258.backup.yml'"

@Lissy93 Lissy93 added the ✨ New Feature [PR] Contains implementation of a new feature label Apr 21, 2022
@bmonty
Copy link
Author

bmonty commented Apr 22, 2022

Were you able to get the saving config working?
I'm getting EACCES: permission denied, copyfile './public/conf.yml' -> './public/conf-1650552258.backup.yml'"

I didn't see that issue. It definitely won't work the way the image is being built because the process won't have write access to /public.

Would it be possible to have backups written to a different path?

@Lissy93
Copy link
Owner

Lissy93 commented Apr 22, 2022

Would it be possible to have backups written to a different path?

Wherever they're written to, would need to be served up by the server, so public is the best place. But I'm aware that if I change the directory, it's going to break everybody's instance over-night, which isn't going to be popular.

Is it not possible to have write permissions on a given file or directory, in a similar way to how the dist dir is handled? (I couldn't actually get that working though, but am maybe missing something)

@bmonty
Copy link
Author

bmonty commented Apr 22, 2022

Is it not possible to have write permissions on a given file or directory, in a similar way to how the dist dir is handled? (I couldn't actually get that working though, but am maybe missing something)

I can give the non-root user access to the public folder.

The big question I have is around security best practices for Vue projects. My main goal was to limit the process from having any kind of write access. Since the process actually needs to be able to do that, what are the appropriate places to allow it? I don't know enough about the project to answer the question.

I generally want to have the permissions set up so the process serving the site can't modify any of the content of the site. If the config files and backups live in their own folder that can't be accessed from the web, I think that's a good solution for security and it makes it easier to set up that folder as a volume when running on docker or Kubernetes.

@bmonty
Copy link
Author

bmonty commented Apr 23, 2022

I updated the Dockerfile so the filesystem permissions allow for the dashy process to be able to read/write in the dist and public folders. I did some testing and I don't see any file access errors in the logs.

I'd still prefer to find a solution where the node server.js process can't write in dist and public and can only update the config file and backups.

@bmonty
Copy link
Author

bmonty commented Apr 23, 2022

I'm thinking about the goal of implementing a way for the process serving the web application to not have access to change any of the files being used to serve the dashboard. The main case is in the event there was a flaw in the node's http module or express that allowed someone to take over the server process, they wouldn't be able to modify any files served to browsers. However, the web application DOES need to be able to modify the config.yml and manage backup files. In addition, the current setup has an always-running Vue process that's updating the site content. This process DOES need to be able to modify the site's files, but DOES NOT serve files over the network.

Here's a couple of ideas I'm thinking about on how to achieve this:

  1. Use a process manager like supervisord in the container to run two separate processes. One process is the vue-cli-service binary that updates the site content and has permissions to update the dist and public directories. The other process is the node server process and it only has permissions to update the config files and it's backups.
  2. Run two containers. Same idea as above, but removes the requirement for a process manager.

I'm going to play around with the first option over the weekend and see how it works.

@Lissy93 Lissy93 mentioned this pull request Apr 27, 2022
6 tasks
@Lissy93
Copy link
Owner

Lissy93 commented Apr 27, 2022

Okay, I've been thinking about this a lot, and you are right- the users files should be separate from the app's files. And since changing the internal port is effectively a breaking change, it makes sense to address both issues at once. And with all customizable assets in a single directory, this should also be easier for the user with just one volume to mount.

So, does it sound okay for me to create a new directory (in the project root, such as /app/user-data), which contains all custom user content (config file, custom icons, additional CSS, etc).

And then, for the time being, it would still be served up with the current Node server, so that directory would need write permissions. But I plan on re-writing this in Go pretty soon, then things can be properly separated with updated permissions, and providing the file paths stay the same, this later change wouldn't break anything for the user.

You're other suggestions are also interesting, but the problem with having two processes, in it's current state, is that Node.js is really not very lightweight. And I worry that having two containers may overcomplicate things for the end user. But if I can sort the directories out now, then I can re-write the server later on, and update the permissions, and this PR would be the only breaking change.

I have to admit (other than running containers on my homelab, and Dockerizing a few simple apps), I am still very much a Docker noob (as is probably reflected here!), so I really appreciate your knowledge, help and patience with this :)

@Lissy93
Copy link
Owner

Lissy93 commented Apr 27, 2022

If I could go back in time, I would definitely architecture things differently. Probably have config stored in a database, and accessible via a Go-based REST API, then have the app served up via NGINX and fetch data from the API on the fly.

I do have a Docker lite version, which simply serves up the built app with NGINX, without any of the editing config features, which is better for static or public dashboards.

@bmonty
Copy link
Author

bmonty commented May 1, 2022

One of the things I like about Dashy is the lack of external dependencies. For my needs, I don't want to stand up a database just so I can have a dashboard page for my home network services. I like your ideas on changing around how the files are organized. I'd add the concept that processes used for serving content to the network should not have the ability to modify files. Or as close to this concept as the frameworks in use will allow. Also, I think separating paths for the site content will be good for running dashy in a container.

I don't fully understand how the web app side of Dashy works. I've played around with some of the frameworks awhile ago but I really don't know how to go about implementing some of the changes we're talking about.

I don't like the two containers idea either.

@liss-bot liss-bot added the 🚫 Merge Conflicts [PR] Submitted code needs rebasing label May 1, 2022
@liss-bot liss-bot removed the 🚫 Merge Conflicts [PR] Submitted code needs rebasing label May 6, 2022
@liss-bot liss-bot added the 🚫 Merge Conflicts [PR] Submitted code needs rebasing label May 20, 2022
@faangbait
Copy link

I do have a Docker lite version, which simply serves up the built app with NGINX, without any of the editing config features, which is better for static or public dashboards.

It'd be cool if this was built/tagged/pushed to Dockerhub in CI.

@Lissy93 Lissy93 mentioned this pull request Jun 23, 2022
@maxisam
Copy link

maxisam commented Jun 26, 2022

a database doesn't have to be a big one. An embedded database will do. Sqlite is born for this kinda task.

@liss-bot
Copy link
Collaborator

This PR is stale because it has been open 6 weeks with no activity. Either remove the stale label or comment below with a short update, otherwise this PR will be closed in 5 days.

@liss-bot liss-bot added the ⚰️ Stale [ISSUE] [PR] No activity for over 1 month label Jul 27, 2022
@Lissy93 Lissy93 added 📌 Keep Open [ISSUE][PR] Prevent auto-closing and removed ⚰️ Stale [ISSUE] [PR] No activity for over 1 month labels Jul 27, 2022
@Lissy93
Copy link
Owner

Lissy93 commented Apr 28, 2024

Implemented the port change in 3.0.0

@Lissy93 Lissy93 closed this Apr 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📌 Keep Open [ISSUE][PR] Prevent auto-closing 🚫 Merge Conflicts [PR] Submitted code needs rebasing ✨ New Feature [PR] Contains implementation of a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants