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

Heroku-24: Implications of the switch to separate Linux users for build vs run images #268

Closed
edmorley opened this issue Mar 19, 2024 · 10 comments · Fixed by #281
Closed
Assignees

Comments

@edmorley
Copy link
Member

edmorley commented Mar 19, 2024

As part of the initial Heroku-24 PR, we switched to using separate Linux users for the run and build images:
#245 (comment)

This change was made because the spec says:

  • The image config's User field is set to a user with a DIFFERENT user UID/SID as the build image.

This upstream spec change was made as a result of RFC-85, which proposed the change for security reasons. See also the discussion in buildpacks/rfcs#146 and the working group recording.

The implications of the change is that at runtime the /app (or /workspace) directory and /layers directories are read-only to the default run image user. Meaning that in order to perform writes at run-time, either:

  1. The app must write to /tmp or $HOME instead
  2. The image must instead be run with the user that performed the build (eg docker run --user heroku-build)
  3. A buildpack (or inline buildpack) must chmod specific directories/files as group-writeable during the build (which allows writes, since both the heroku and heroku-build users are in the same group).

This is more secure, but will mean:

As such it seems like quite a substantial change to make, given that we don't have control over app code, and that some ecosystems heavily rely upon writing to the app directory (for example Ruby Rails apps writing to <app>/tmp/). We could try things like having the Ruby buildpack chmod <app>/tmp/ if present, however, there is likely a long tail of other cases that will break, and the error messages are not the easiest for customers to figure out.

The spec only says SHOULD, so we could choose not to make this change. (And perhaps consider it for Heroku-26, or else make read-only images at runtime an opt-in platform feature in the future.)

However, some other platforms out there already use separate users at runtime (as seen in heroku/buildpacks-nodejs#800), and so regardless of what we chose for Heroku, if we want our buildpacks to work elsewhere, we’ll need to make sure they do not write to /layer or the app directory. We could always test in CI that this works, even if we chose not to have separate user (and this read-only /layer and app dir) for the Heroku base images themselves.

@heroku/languages Thoughts?

GUS-W-15342842.

@schneems
Copy link
Contributor

I would like to preserve existing behavior of heroku-22 (same user for build and runtime) as I don't think it's tennable to support Rails apps out of the box unless I chmod the whole app dir. Even then, I would be surprised if there wasn't some edge case that wasn't uncovered.

Ideally: If we want to transition to a more restrictive set of runtime permissions, I would like some exit metrics to see what folders people interact with (though we don't have that capability today).

I'm in favor of adding some smoke tests to our various CNBs so they won't block a compliant app from running on more restrictive infrastructure.

Essentially I support Ed's position here.

@joshwlewis
Copy link
Member

I really like the security posture of not being able to write to /app or /layers at runtime. It basically means the build result is untouched by things happening at runtime, which seems correct and more secure for production images.

Not being able to write to /app is a big blocker, though. Folks will certainly be doing that (especially folks transitioning from the current Heroku platform where /app is one of the few writeable locations). It'd be really annoying for (probably a fair number of) developers to move writes to /tmp or $HOME.

If there is anyway we could make /app (and /workspace) group writeable, so runtime users could write to /app, but not /layers, that would be a decent compromise. But I'm not sure we can instruct the CNB platform to do that?

If not, I think I favor one user. I don't see the value in causing a bunch of pain (which may slow the adoption rate of Heroku CNBs) for what I view as an ivory tower security restriction.

@edmorley
Copy link
Member Author

If there is anyway we could make /app (and /workspace) group writeable, so runtime users could write to /app, but not /layers, that would be a decent compromise. But I'm not sure we can instruct the CNB platform to do that?

I don't believe there is a way to do this properly currently. It would require discussion/RFCs/spec work upstream (which is perhaps something worth doing, but IMO not something we can achieve before Heroku-24 GA).

The only workaround we could do in the meantime would be to say have Kodon adjust permissions itself, but then we lose Kodon vs Pack parity, which doesn't seem worth it.

@colincasey
Copy link
Contributor

What about an inline buildpack that adjusts permissions for /workspace that's placed at the end of all our buildpack groups? We do something similar with the builder EOL deprecation message. It could also be toggled on/off with an environment variable for those who want that increased security.

@edmorley
Copy link
Member Author

Yeah that's mentioned here:

  1. A buildpack (or inline buildpack) must chmod specific directories/files as group-writeable during the build (which allows writes, since both the heroku and heroku-build users are in the same group).

However, until we have system buildpacks it would be kinda clunky (eg when someone adds an unrelated buildpack so switches from the buildpack default to their own list in project.toml, the feature will stop working, unless they know to add this special buildpack to their buildpacks list too).

@colincasey
Copy link
Contributor

Oops, I picked up the thread in the middle and missed that (but had remembered you mentioning something along these lines) 😅

Despite the potential clunkiness, I think it would be an acceptable transitional step. Since option 1 (app must write to /tmp or $HOME) forces developers to adopt the increased security posture and option 2 (build and run user are the same) forces developers to adopt the decreased security posture.

Plus, we could reduce that clunkiness with documentation and by helping upstream get system buildpacks implemented.

@edmorley
Copy link
Member Author

I agree using a buildpack would be a good way for people to opt-into read-only layers/workspace, as a transitional step.

However, I'm not sure about adding to the builder (and then having it toggled via env var) - IMO it would make more sense (and avoid the footgun mentioned above) if people opted in by adding the buildpack to their buildpacks list in project.toml.

The main question we need to answer now is "what should the default behaviour be for Heroku-24?". If the answer is "same user; keep app source + layers writable for now", then we can figure out how we might offer this as opt-in later :-)

@joshwlewis
Copy link
Member

If there is anyway we could make /app (and /workspace) group writeable, so runtime users could write to /app, but not /layers, that would be a decent compromise. But I'm not sure we can instruct the CNB platform to do that?

I looked into this a bit. pack and lifecycle isn't doing anything to /workspace / /app permissions right now. pack just mounts the local app directory into /workspace as is. So a very simple solution for a user is to run chmod g+w . once in their repo, if they want to enable writes in /workspace at runtime. This seems nice from a security standpoint, the app doesn't modify it's workspace at runtime without the developer explicitly allowing it. And users wouldn't need to run a buildpack to do this for every build.

@edmorley
Copy link
Member Author

So a very simple solution for a user is to run chmod g+w . once in their repo, if they want to enable writes in /workspace at runtime.

That won't work, since Git doesn't support full file modes (it would likely get messy), but only the executable bit. Therefore, whilst users could chmod locally when using Pack CLI, it wouldn't help when they git push to Heroku/other platforms.

See:
https://git-scm.com/docs/user-manual#tree-object

Note that the files all have mode 644 or 755: Git actually only pays attention to the executable bit.

@joshwlewis
Copy link
Member

That won't work, since Git doesn't support full file modes (it would likely get messy), but only the executable bit.

Fair enough. I'll stick with my earlier statement:

I favor one user. I don't see the value in causing a bunch of pain (which may slow the adoption rate of Heroku CNBs) for what I view as an ivory tower security restriction.

@edmorley edmorley self-assigned this Mar 27, 2024
edmorley added a commit that referenced this issue Mar 27, 2024
The upstream CNB spec recently changed to say that build and run
images `SHOULD` use a separate Linux user for each image:
https://github.com/buildpacks/rfcs/blob/main/text/0085-run-uid.md
https://github.com/buildpacks/spec/blob/platform/0.13/platform.md#run-image

However, this causes a number of compatibility issues with existing
apps and parts of the ecosystem (see #268).

Whilst we can (and will) adjust our own buildpacks to do the right thing
(not write to `/layers/` or the app source directory at runtime), it's
going to be some time before existing apps/frameworks/... make similar
changes. In addition, the failure modes are not easy for users to debug
or solve (they will have to know that seeing access denied errors means
needing to use `chmod` to make directories group writeable in an inline
buildpack step or similar).

As such, we're deferring making this switch for now, and will revisit in the
future (either for Heroku-26, or as an opt-in feature for Heroku-24), when
the various third party language ecosystems are more ready for this.

We will still be in compliance with the spec, since it says `SHOULD` not
`MUST`.

We will also add integration testing to our own CNBs to ensure that they
operate correctly in environments that do run split build/run users.

As part of this change, I've also switched the `heroku` user's ID back to
1000, for consistency with the Heroku-20/22 CNB base images.

I've also switched back to the `USER <name>` syntax instead of `USER <id>`,
since both are permitted by the OCI and CNB specs, and the former is
(a) IMO more intuitive (eg for users needing to switch to `root` and back
in their own `Dockerfile`), (b) matches what Heroku-20/22 do.

See also:
https://manpages.ubuntu.com/manpages/noble/en/man8/userdel.8.html
https://manpages.ubuntu.com/manpages/noble/en/man8/groupadd.8.html

Closes #268.
GUS-W-15342842.
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 a pull request may close this issue.

4 participants