oxsecurity / megalinter

🦙 MegaLinter analyzes 50 languages, 22 formats, 21 tooling formats, excessive copy-pastes, spelling mistakes and security issues in your repository sources with a GitHub Action, other CI tools or locally.
https://megalinter.io
GNU Affero General Public License v3.0
1.81k stars 215 forks source link

MegaLinter applying fixes sets root:root as the file owner #1975

Open kcfedun-fincad opened 1 year ago

kcfedun-fincad commented 1 year ago

When running MegaLinter in "fix" mode, it will chown the fixed files to root:root as the file owner. This causes permission issues later when I attempt to make changes to the file through my IDE. Is there a way to have MegaLinter maintain the file user and group ownership when it "fixes" the linting issues?

Kurt-von-Laven commented 1 year ago

I recommend running Docker in rootless mode if possible. See ScribeMD/rootless-docker for a GitHub Action that does this.

kcfedun-fincad commented 1 year ago

I recommend running Docker in rootless mode if possible. See ScribeMD/rootless-docker for a GitHub Action that does this.

This might work for running in GitHub, however, most of us work locally in an Ubuntu 20 WSL development environment. I've tried rootless mode there and had no success. Also, bespoke developer environment tweaks to Docker to get a linter tool to work seem difficult to support long term. Any other less invasive suggestions?

Kurt-von-Laven commented 1 year ago

Yeah, I haven't had any luck with rootless mode in WSL either. I would see if there is an existing issue on either WSL or Docker that you can upvote for rootless mode support. (Feel free to link it here if you find it, and I'll happily upvote it as well.) I'm not sure why, but I haven't encountered the issue with file permissions being modified when running MegaLinter as a pre-commit hook. Otherwise, I think reviving #1485 is the next best option? Not that this is relevant to this problem, but why Ubuntu 20.04 WSL as opposed to Ubuntu 22.04 WSL?

kcfedun-fincad commented 1 year ago

Not that this is relevant to this problem, but why Ubuntu 20.04 WSL as opposed to Ubuntu 22.04 WSL?

Ubuntu 20.04 WSL only for the reason that we haven't moved to 22.x yet.

nvuillam commented 1 year ago

hmmm I'm no linux expert, but what about if we add a config param that would change back updated files ?

Something like:

UPDATED_FILES_CHOWN: toto

And if such config is found, we do chown toto thefile.xxx

Kurt-von-Laven commented 1 year ago

I wouldn't be surprised if there are some security concerns with chowning to an arbitrary string, but regardless it seems unlikely that all developers on a project would share a username in the case where they are running MegaLinter locally?

kcfedun-fincad commented 1 year ago

Work in progress, but I've added these Pre and Post commands to .mega-linter.yml. The basic theory is that the permissions on .gitignore are likely to be correctly set for the developer's workspace, so we scrape the user and group IDs from inside the container (they'll resolve to UIDs and GIDs, not friendly names because these users don't exist inside the container). We save the UID:GID combination into a temporary txt file. The linter then runs and fixes files as necessary, but changes those fixed file's ownership to root:root. We can then run the post command to find these files and chown them back, then delete the temporary perms.txt file.

.mega-linter.yml
---

...
PRE_COMMANDS:
  - command: |-
      echo $(ls -lah .gitignore | sed 's/\s\+/ /g' | cut -d ' ' -f3,4 | sed 's/ /\:/') > perms.txt
    cwd: "workspace"

POST_COMMANDS:
  - command: |-
      find . -user root -group root -exec chown $(cat perms.txt) {} \;
      rm perms.txt
    cwd: "workspace"
...
nvuillam commented 1 year ago

If it works that could be a nice workaround, but having such behavior directly embedded within megalinter would feel more optimal ^^

Kurt-von-Laven commented 1 year ago

There are some test failures associated with my PR that will require some debugging, but I believe it is conceptually correct to simply instruct Docker not to run as root on the command line. #1485 is also conceptually correct, and ideally we would do both, but I suspect that will require significantly more work to iron out all the details and get the tests passing.

This doesn't relate to the permissions problem directly, but folks who are using WSL may be interested in the "clone of clones" development technique we use to boost local MegaLinter performance while retaining access to Windows-only tools.

github-actions[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

If you think this issue should stay open, please remove the O: stale 🤖 label or comment on the issue.

github-actions[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

If you think this issue should stay open, please remove the O: stale 🤖 label or comment on the issue.

BryanQuigley commented 1 year ago

Please leave this open, having a better fix is important for the ability of formatting to work.

bittner commented 1 year ago

Docker's Best practices for writing Dockerfiles recommend using a non-root user in the Dockerfile. If I'm not mistaken that's what #1485 was trying to get fixed.

As a (temporary) workaround you can run any container image with the -u option, e.g.

docker run --rm -u $UID:$(id -g $UID) -v $PWD:/tmp/lint oxsecurity/megalinter

This will "usually" work, because your UID will be 1000 on most personal systems. (You may try to replace $UID by 1000 in the command above, otherwise.)

Kurt-von-Laven commented 1 year ago

@bittner, yes, that was the issue #1485 sought to fix, and I attempted your approach in #1985. I got stuck on a test failure, so I rebased and will see if I am able to figure out what went wrong this time.

Kurt-von-Laven commented 1 year ago

I have reproduced the current test failure below. We create certain directories, such as /megalinter-descriptors, as root in the Dockerfile. Hence, when we modify the docker run command to run MegaLinter as the current (non-root) user, we experience various crashes when attempting to write to said directories. I wonder if it would make sense to create a non-root user (e.g., megalinter) with a fixed UID (e.g., 1000) and either switch to root only when needed or chown all the pertinent directories to be owned by the created user. Curious what others think.

Starting new HTTPS connection (1): raw.githubusercontent.com:443
[https://raw.githubusercontent.com:443](https://raw.githubusercontent.com/) "GET /oxsecurity/megalinter/main/.automation/test/mega-linter-plugin-test/test.megalinter-descriptor.yml HTTP/1.1" 200 208
Traceback (most recent call last):
  File "/megalinter/plugin_factory.py", line 59, in load_plugin
    with open(descriptor_file, "w") as outfile:
         ^^^^^^^^^^^^^^^^^^^^^^^^^^
PermissionError: [Errno 13] Permission denied: '/megalinter-descriptors/test.megalinter-descriptor.yml'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<frozen runpy>", line 198, in _run_module_as_main
  File "<frozen runpy>", line 88, in _run_code
  File "/megalinter/run.py", line 9, in <module>
    linter = megalinter.Megalinter({"cli": True})
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/megalinter/MegaLinter.py", line 115, in __init__
    plugin_factory.initialize_plugins()
  File "/megalinter/plugin_factory.py", line 22, in initialize_plugins
    descriptor_file = load_plugin(plugin)
                      ^^^^^^^^^^^^^^^^^^^
  File "/megalinter/plugin_factory.py", line 65, in load_plugin
    raise Exception(
Exception: [Plugins] Unable to load remote plugin https://raw.githubusercontent.com/oxsecurity/megalinter/main/.automation/test/mega-linter-plugin-test/test.megalinter-descriptor.yml:
[Errno 13] Permission denied: '/megalinter-descriptors/test.megalinter-descriptor.yml'
    1) (Module) run on own code base

  2 passing (3s)
  1 failing

  1) Module
       (Module) run on own code base:

      AssertionError [ERR_ASSERTION]: status is 0 (1 returned)
      + expected - actual

      -false
      +true

      at Context.<anonymous> (test/megalinter-module.test.js:54:5)
      at processTicksAndRejections (internal/process/task_queues.js:97:5)

npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! mega-linter-runner@6.19.0 test:module: `mocha test/megalinter-module.test.js`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the mega-linter-runner@6.19.0 test:module script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/runner/.npm/_logs/2023-02-19T09_21_52_696Z-debug.log
bittner commented 1 year ago
npm ERR! A complete log of this run can be found in:
npm ERR!     /home/runner/.npm/_logs/2023-02-19T09_21_52_696Z-debug.log

Is runner maybe already the unprivileged user we want to build on?

What if we change /megalinter-descriptors to /home/runner/megalinter-descriptors to make that user operate only inside its own home directory? (related code)

(Following the FHS is likely a good idea. Unfortunately, violating it has become a wide-spread bad habit by container image developers, that's why directories like /app and so forth are a common pattern in container images. Not because it works well.)

Kurt-von-Laven commented 1 year ago

No, the runner user is specific to GitHub Actions, specifically actions/runner-images, and not associated with MegaLinter. Similarly, /home/runner is a path on the GitHub Actions runner images. I don't have any particular objection to following FHS, although I'm not sure I understand the value well enough to prioritize it effectively. We mount the repository under test to /tmp/lint and need to write log files there that should be owned by the current user rather than root. That does mean that we wouldn't be able to assume that any user created beforehand in the image would have the correct user ID though.

Kurt-von-Laven commented 1 year ago

@echoix, do you have an opinion regarding my comment above? My instinct is to try switching to root as needed first and seeing how much work that ends up being, but there could be a far better path I'm ignorant of.

echoix commented 1 year ago

@Kurt-von-Laven I'd need to take a really deep look to understand correctly what is going on and search a bit on what are rootless images particularities and if it's a fit for this problem. And maybe refresh myself a bit more on Linux user handling/permissions, I might understand way more now than when I learned about it. My quick instinct is that trying to change whether we use root or not dynamicly is that we've got the solution wrong, a path for unexpected bugs... the solution shouldn't look like that I'm sure..

So it's not a real opinion yet, if you need me to really work on it I might need a couple weekends to get to it..

sanmai-NL commented 1 year ago

Can you use the setgid bit on directories you create? You would then require chowns in fewer cases. See https://linuxconfig.org/how-to-use-special-permissions-the-setuid-setgid-and-sticky-bits.

cicorias commented 10 months ago
docker run --rm -u $UID:$(id -g $UID) -v $PWD:/tmp/lint oxsecurity/megalinter

unfortunately, this doesn't work on wsl

latest: Pulling from oxsecurity/megalinter
Digest: sha256:dac46ccf0547f7e1659e50635f710c2a31674b500653181b12edf67baa2075fb
Status: Downloaded newer image for oxsecurity/megalinter:latest
Skipped setting git safe.directory DEFAULT_WORKSPACE:  ...
Setting git safe.directory default: /github/workspace ...
error: could not lock config file //.gitconfig: Permission denied
Setting git safe.directory to /tmp/lint ...
error: could not lock config file //.gitconfig: Permission denied
[MegaLinter init] ONE-SHOT RUN
[config] /tmp/lint/.mega-linter.yml + Environment variables
PawelHaracz commented 10 months ago

There is a huge problem with self-hosted agents on the Azure pipelines. Azure pipelines use AzureDevOps user and cannot reuse the same VM because mounted files are owned by root, and We can't change it because we cannot use sudo and escalate privileges for it

Kurt-von-Laven commented 10 months ago

I recommend setting REPORT_OUTPUT_FOLDER: none for now.

PawelHaracz commented 10 months ago

@Kurt-von-Laven, good tip. Thank you! We figured out how to work around it.

I left example:

  - script: |
      mkdir $(Agent.WorkFolder)/report
      docker run -v $(System.DefaultWorkingDirectory):/tmp/lint \
        -v $(Agent.WorkFolder)/report:/tmp/report \
        --env-file <(env | grep -e SYSTEM_ -e BUILD_ -e TF_ -e AGENT_) \
        -e SYSTEM_ACCESSTOKEN=$(System.AccessToken) \
        -e GIT_AUTHORIZATION_BEARER=$(System.AccessToken) \
        oxsecurity/megalinter:v7

  - task: PublishPipelineArtifact@1
    condition: succeededOrFailed()
    displayName: Upload MegaLinter reports
    inputs:
      targetPath:  $(Agent.WorkFolder)/report"
      artifactName: MegaLinterReport

and on the linter config.yaml We set this: REPORT_OUTPUT_FOLDER: /tmp/report

Kurt-von-Laven commented 10 months ago

Clever; thanks for sharing your solution with the community.