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

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

Open
kcfedun-fincad opened this issue Oct 17, 2022 · 25 comments · May be fixed by #1985
Open

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

kcfedun-fincad opened this issue Oct 17, 2022 · 25 comments · May be fixed by #1985
Assignees
Labels
nostale This issue or pull request is not stale, keep it open question Further information is requested

Comments

@kcfedun-fincad
Copy link

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?

@kcfedun-fincad kcfedun-fincad added the question Further information is requested label Oct 17, 2022
@Kurt-von-Laven
Copy link
Collaborator

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

@Kurt-von-Laven Kurt-von-Laven self-assigned this Oct 17, 2022
@kcfedun-fincad
Copy link
Author

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Member

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
Copy link
Collaborator

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
Copy link
Author

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
Copy link
Member

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

Kurt-von-Laven added a commit that referenced this issue Oct 20, 2022
Previously, mega-linter-runner ran the MegaLinter Docker image as root.
Users whose files became owned by root as a consequence of this
behavior will need to chown them to be owned by the appropriate user.
This change only affects POSIX platforms, because process.getuid and
process.getgid are only available there.
@Kurt-von-Laven Kurt-von-Laven linked a pull request Oct 20, 2022 that will close this issue
4 tasks
Kurt-von-Laven added a commit that referenced this issue Oct 21, 2022
Previously, mega-linter-runner ran the MegaLinter Docker image as root.
Users whose files became owned by root as a consequence of this
behavior will need to chown them to be owned by the appropriate user.
This change only affects POSIX platforms, because process.getuid and
process.getgid are only available there.
Kurt-von-Laven added a commit that referenced this issue Oct 22, 2022
Previously, mega-linter-runner ran the MegaLinter Docker image as root.
Users whose files became owned by root as a consequence of this
behavior will need to chown them to be owned by the appropriate user.
This change only affects POSIX platforms, because process.getuid and
process.getgid are only available there.
@Kurt-von-Laven
Copy link
Collaborator

Kurt-von-Laven commented Oct 23, 2022

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
Copy link
Contributor

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 github-actions bot added the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Nov 22, 2022
@Kurt-von-Laven Kurt-von-Laven removed the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Nov 25, 2022
@github-actions
Copy link
Contributor

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 github-actions bot added the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Dec 26, 2022
@Kurt-von-Laven Kurt-von-Laven added nostale This issue or pull request is not stale, keep it open and removed O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity labels Dec 27, 2022
@BryanQuigley
Copy link
Contributor

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

@bittner
Copy link
Contributor

bittner commented Jan 9, 2023

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 added a commit that referenced this issue Jan 10, 2023
Previously, mega-linter-runner ran the MegaLinter Docker image as root.
Users whose files became owned by root as a consequence of this
behavior will need to chown them to be owned by the appropriate user.
This change only affects POSIX platforms, because process.getuid and
process.getgid are only available there.
@Kurt-von-Laven
Copy link
Collaborator

@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 added a commit that referenced this issue Jan 31, 2023
Previously, mega-linter-runner ran the MegaLinter Docker image as root.
Users whose files became owned by root as a consequence of this
behavior will need to chown them to be owned by the appropriate user.
This change only affects POSIX platforms, because process.getuid and
process.getgid are only available there.
Kurt-von-Laven added a commit that referenced this issue Jan 31, 2023
Previously, mega-linter-runner ran the MegaLinter Docker image as root.
Users whose files became owned by root as a consequence of this
behavior will need to chown them to be owned by the appropriate user.
This change only affects POSIX platforms, because process.getuid and
process.getgid are only available there.
Kurt-von-Laven added a commit that referenced this issue Feb 19, 2023
Previously, mega-linter-runner ran the MegaLinter Docker image as root.
Users whose files became owned by root as a consequence of this
behavior will need to chown them to be owned by the appropriate user.
This change only affects POSIX platforms, because process.getuid and
process.getgid are only available there.
@Kurt-von-Laven
Copy link
Collaborator

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
Copy link
Contributor

bittner commented Feb 20, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator

@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
Copy link
Collaborator

echoix commented Feb 26, 2023

@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..

Kurt-von-Laven added a commit that referenced this issue Apr 3, 2023
Create megalinter user and group in Docker image, both with ID 1000, and
activate this user after dependencies have been installed. Run Docker
container as current user via mega-linter-runner. The change to
mega-linter-runner only affects POSIX platforms, because process.getuid
and process.getgid are only available there. Previously,
mega-linter-runner ran the MegaLinter Docker image as root. Users whose
files became owned by root as a consequence of this behavior will need
to chown them to be owned by the appropriate user when upgrading
MegaLinter.
Kurt-von-Laven added a commit that referenced this issue Apr 3, 2023
Create megalinter user and group in Docker image, both with ID 1000, and
activate this user after dependencies have been installed. Run Docker
container as current user via mega-linter-runner. The change to
mega-linter-runner only affects POSIX platforms, because process.getuid
and process.getgid are only available there. Previously,
mega-linter-runner ran the MegaLinter Docker image as root. Users whose
files became owned by root as a consequence of this behavior will need
to chown them to be owned by the appropriate user when upgrading
MegaLinter.
Kurt-von-Laven added a commit that referenced this issue Apr 3, 2023
Create megalinter user and group in Docker image, both with ID 1000, and
activate this user after dependencies have been installed. Run Docker
container as current user via mega-linter-runner. The change to
mega-linter-runner only affects POSIX platforms, because process.getuid
and process.getgid are only available there. Previously,
mega-linter-runner ran the MegaLinter Docker image as root. Users whose
files became owned by root as a consequence of this behavior will need
to chown them to be owned by the appropriate user when upgrading
MegaLinter.
Kurt-von-Laven added a commit that referenced this issue Apr 4, 2023
Previously, mega-linter-runner ran the MegaLinter Docker image as root.
In the Docker image, chown the /megalinter, /megalinter-descriptors, and
/action/lib/.automation directories to be owned by user and group 1000.
Users whose files became owned by root as a consequence of having run a
previous version of MegaLinter will need to chown them to be owned by
user 1000 when upgrading MegaLinter.
Kurt-von-Laven added a commit that referenced this issue Apr 4, 2023
Previously, mega-linter-runner ran the MegaLinter Docker image as root.
In the Docker image, chown the /megalinter, /megalinter-descriptors, and
/action/lib/.automation directories to be owned by user and group 1000.
Users whose files became owned by root as a consequence of having run a
previous version of MegaLinter will need to chown them to be owned by
user 1000 when upgrading MegaLinter.
Kurt-von-Laven added a commit that referenced this issue Apr 4, 2023
Previously, mega-linter-runner ran the MegaLinter Docker image as root.
In the Docker image, chown the /megalinter, /megalinter-descriptors, and
/action/lib/.automation directories to be owned by user and group 1000.
Users whose files became owned by root as a consequence of having run a
previous version of MegaLinter will need to chown them to be owned by
user 1000 when upgrading MegaLinter.
Kurt-von-Laven added a commit that referenced this issue Apr 7, 2023
Previously, mega-linter-runner ran the MegaLinter Docker image as root.
In the Docker image, chown the /megalinter, /megalinter-descriptors, and
/action/lib/.automation directories to be owned by user and group 1000.
Users whose files became owned by root as a consequence of having run a
previous version of MegaLinter will need to chown them to be owned by
user 1000 when upgrading MegaLinter.
Kurt-von-Laven added a commit that referenced this issue Apr 8, 2023
Previously, mega-linter-runner ran the MegaLinter Docker image as root.
In the Docker image, chown the /megalinter, /megalinter-descriptors, and
/action/lib/.automation directories to be owned by user and group 1000.
Users whose files became owned by root as a consequence of having run a
previous version of MegaLinter will need to chown them to be owned by
user 1000 when upgrading MegaLinter.
Kurt-von-Laven added a commit that referenced this issue Apr 9, 2023
Previously, mega-linter-runner ran the MegaLinter Docker image as root.
In the Docker image, chown the /megalinter, /megalinter-descriptors, and
/action/lib/.automation directories to be owned by user and group 1000.
Users whose files became owned by root as a consequence of having run a
previous version of MegaLinter will need to chown them to be owned by
user 1000 when upgrading MegaLinter.
Kurt-von-Laven added a commit that referenced this issue Apr 9, 2023
Previously, mega-linter-runner ran the MegaLinter Docker image as root.
In the Docker image, chown the /megalinter, /megalinter-descriptors, and
/action/lib/.automation directories to be owned by user and group 1000.
Users whose files became owned by root as a consequence of having run a
previous version of MegaLinter will need to chown them to be owned by
user 1000 when upgrading MegaLinter.
@sanmai-NL
Copy link
Contributor

sanmai-NL commented Jun 3, 2023

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
Copy link

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
Copy link

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
Copy link
Collaborator

I recommend setting REPORT_OUTPUT_FOLDER: none for now.

@PawelHaracz
Copy link

PawelHaracz commented Sep 1, 2023

@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
Copy link
Collaborator

Clever; thanks for sharing your solution with the community.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
nostale This issue or pull request is not stale, keep it open question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants