[RRFC] Clean up file ownership story #546
Comments
|
Auditing and aligning this sounds great. I would expect that npm has full ownership (and thus, full freedom to change whatever it likes) of any Things a user is explicitly asking npm to change - I would also expect that any other file is something npm is not "allowed" to change in any way. |
|
AFAIK the existing file ownership changes are so that things like This doesn't feel like something we should be attempting to forcibly fix on a user's behalf. The stat calls to determine ownership, and the chown calls to modify the ownership when it differs from what we infer are not free. We are most certainly taking a performance hit with these, especially in environments like Docker where filesystem i/o can be significantly slower. Attempting to change permissions of things like the directory global bin scripts are located in can be absolutely disastrous. Is there prior art for the type of permissions changes we attempt? I suspect there's not, but personally I also pretty strongly believe we shouldn't be attempting to change ownership ourselves. |
|
Is it possible to make |
|
potentially, but that wouldn't stop things like a user elevating their permissions to root manually and installing there. we absolutely can't explode when npm is run as root unconditionally, as that's often the case in a containerized environment. |
|
a suggestion: what if |
|
after actually running so all that's missing is making our error output suggest running doctor, and doctor telling you how to fix the problem |
|
We could have a |
|
adding this as another data point npm/cli#4312 |
|
Rather than dropping perms to nobody in this circumstance (which breaks
compilation or yields surprising ownership issues), can we drop perms to
$SUDO_USER instead?
…Sent from my iPhone
On Mar 3, 2022, at 19:08, Nathan Fritz ***@***.***> wrote:
We could have a --no-root-user default for non-global installs, and then ci
users would have to run --root-user. In the same release, we'd drop all
ownership checks, and just write files as the current user. This would, of
course, be a major breaking change.
ENOROOT: running `npm` as root is discouraged. Use `--root-user`to
override. [link to docs]
—
Reply to this email directly, view it on GitHub
<#546 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABTIUCYLNTMJ6UH4VVY2R3U6FH7LANCNFSM5P3WILHA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
I think that's not a good idea, not everyone is using sudo. If I |
|
how about this for a default behavior: flowchart TD
A[root runs command] --> B{SUDO_UID/SUDO_GID are set?};
B -- Yes --> C[setuid/setgid to SUDO_UID/SUDO_GID] --> E;
B -- No --> D{--allow-root passed?};
D -- Yes --> Z[log warning] --> E[proceed running command];
D -- No --> F[log error, exit];
when logging an error and exiting, we could use a message along the lines of and the warning could read something like |
|
@nlf You didn't wrote it, but I assume that you are suggesting the above default behavior + removing npm chmod ? If that's the case it seems pretty nice ! It will break a lot of misconfigured docker containers but I think this is not a problem since no use application should be run as root, even in a container. People will discover the USER docker directive and that's for the best :) |
|
IMO a flag would be a simple yet effective solution to this problem. I'm thinking about something like |
|
Leaving this on the agenda for today, @nlf would be great if you could speak bit to your diagram/approach discussed here in the call but understand if you can't make it. |
|
I'm extremely frustrated with this permission decision. Docker volume mounts use the UID:GID of the |
to be very clear this permissions decision was made many years ago, it is by no means a new thing, and that's exactly why this issue exists. this is something that we know to cause problems, so we're trying to determine the best way to fix that without immediately causing builds to break for people who have worked around the problem already. we'll be starting to roll out some changes to help folks in your situation in the near future. |
Actions
|
|
Hey all, having spent a whole day debugging an issue caused by silently setting the effective UID of npm subprocesses to
That's fair, but how about exploding when npm is run as root and the current directory is owned by non-root? This seems strictly preferable to silently switching users. |
|
I also agree that silently switching users is a great source of pain. I for example lost about one hour because of this. Please find a solution that stops doing this, thank you all very much :) |
|
I think npm shouldn't be looking at its own UID, it should not modify file ownerships and permissions, and it should specially not switch users after it's called. That last point, when run as root, breaks long standing assumptions of a process behavior. It's up to the caller of npm to decide what user they want to run it as, even if their decision is bad. If I run |
|
@javabudd We face(d) a similar issue.
This is the workaround we cooked up on a ubuntu based docker (so your path may be different) It modifies the behavior of how npm detects which user it should run as. We this issue when using webpack in a VOLUME mount, while writing the output to a another, in-container only folder owned by root. In our case, forcing UID=0 is enough, so we echo append that to the module. (We are still evaluating this solution, but looks workable) |
|
Still unclear, why NPM tries to lchown files that have nothing to do with Node. In my case, it is a link in /usr/local/bin to Microsoft Defender Antivirus. % sudo npm install -g yarn@1.22.19 % ls -l /usr/local/bin/mdatp |
|
From what I can see, it goes back to the times when it was suggested that Was that a good idea or not?.. I personally think that I was beaten by it a couple of times, and it always took me a while to figure out what's going on. Today it was like this (in a
So let's say I have a
see the above case
That's a rather radical way to deal with it, but in a way I like it :)
Do you have an idea how to work around it? (Or you're just trying to make it as close to the previous behavior as possible?) After dropping |
This is going to be the behavior in npm 9. Wednesday's prerelease should have this available for testing. |
This solves a lot of permission issues. See: - npm/rfcs#546 (comment) - https://stackoverflow.com/questions/70298238/getting-eaccess-when-running-npm-8-as-root
|
we have stopped altering file ownership at runtime on the user's behalf in npm@9 entirely. i think we have some follow up work to do here to try to help users not end up in a situation where they have files unexpectedly owned by root. things like disabling the cache and log files if the cache directory is not owned by the same user that's running npm would go a long way towards not making users have to keep running |
|
Thanks for implementing this long overdue change. At last npm is more predictable if ran from root. However, this is caused us a lot of pain. :) Indeed, our docker builds started failing as soon as we started using npm v9. For others, who stumbled upon Docker build errors like or (for multi stage builds) here is how I fixed it. The build container should start from this: And end with this: The next container in the multi-stage docker build should be like this: Hope this helps someone in the future. |
|
@koresar Copying files from Debian to Alpine? That won't work if any of your packages contain anything binary.
A lot of comments (Clean Code, chapter 4) but the really unclear part is uncommented: That is, there's a comment but it doesn't tell why one should avoid doing that. From what I can see, there should be no problem with copying files owned by non- The way I'd do it (a gist): FROM node:18-alpine as node_modules
WORKDIR /app
COPY package.json package-lock.json ./
RUN npm install
FROM node:18-alpine
WORKDIR /app
RUN chown node: .
COPY --chown=node --from=node_modules /app/node_modules node_modules
COPY --chown=node . .
USER nodeBut okay. That's FROM alpine:3.17 AS node_modules
RUN apk add nodejs npm
WORKDIR /app
COPY package.json package-lock.json ./
RUN npm install
FROM alpine:3.17
RUN apk add nodejs shadow && useradd node
WORKDIR /app
RUN chown node: .
COPY --chown=node --from=node_modules /app/node_modules node_modules
COPY --chown=node . .
USER nodeAll in all, I don't rule out that I'm missing something here, but the way it looks to me at the moment, in addition to the first issue (Debian -> Alpine), there's a lot of unnecessary movement in your solution. And mine is a better starting point. But if I'm missing something, can you possibly tell under which conditions it'll fail? How to reproduce the issues you mentioned? Imagine a person running into them. Should he/she change the whole file to fix them? Instead of "add this line," "change this line", or "don't do this because..."? |
|
For past 9 years of using Dockerfile(s) I thought I tried My base image is Thank you for your Let's not drive away the discussion in this issue from the I appreciate your help. |
FROM node:18-alpine
USER nonexistentuser
RUN set -x && whoami && idAlso, from the official docs:
--
As you probably noticed, my main suggestion was to use
Coding habits probably? Actually, I was planning to give a more detailed reply back than, but then decided to focus on what I thought was important. Also, I think it's often hard to draw the line between what one thinks is important and what is considered important. So everything here should be taken as my personal opinion, which you might share or not. One more thing that came to mind. You said, "For those who stumbled into the following issues... Here's how I fixed it: ..." And then you provide what looks like a The point being, if you wanted to help people who stumbled into specific issues, you should have told what you changed. Like, "I had this line here, and after I changed it to this, it resolved." Ideally, you should have provided "steps to reproduce the issues," or something along those lines. If you wanted to provide a And to me it looks like you more or less failed at both (no offense meant). The solution is not clear (what exactly solves the issues). And it's not clear what's missing in my version of |
With NPM v7 and v8, when running as root, NPM uses the UID and GID of the current working directory owner to execute scripts (see [npm/cli#4095][]). This can cause a problem if `npm install` is run as root in a directory owned by another user because module installation scripts will instead use that user's UID and GID but may still try to write to root's home directory. For example, Puppeteer will try to download Chromium to `/root/.cache/puppeteer/chrome`, raising "EACCES: permission denied". Similarly, this can cause a problem if `npm install` is run as root in a directory owned by root, and then that directory is later `chown`ed to another user. Subsequent NPM commands (e.g. `npm run build`) run as root will instead use that user's UID and GID, and will raise "EACCES: permission denied" when touching anything in the root-owned `node_modules` directory (e.g. `node_modules/.cache`). NPM v9 removed this behavior (see [npm/rfcs#546][]), but v9 was first released on 2022-10-24, and v8 is still widespread. Therefore, as a workaround, instead of `chown`ing the application directory to a non-root user, this commit `chmod`s the application directory to grant owner-equivalent permissions to non-root users. Additionally, this commit `chown`s and `chmod`s the `PACKAGE_JSON_DIR` directory in the same way. [npm/cli#4095]: npm/cli#4095 [npm/rfcs#546]: npm/rfcs#546
We currently have an entire category of issue reports around unexpected changing of file ownership. We need a place to start the discussion on what we can do to improve the story around file ownership management: Maybe we need less of it these days?
Opening this RRFC so that we can have a place to share thoughts, link related issues to and hopefully eventually evolve towards an RFC if we come up to the decision that changes are necessary.
cc @nlf @fritzy
The text was updated successfully, but these errors were encountered: