spec icon indicating copy to clipboard operation
spec copied to clipboard

Should the platform spec guarantee `$HOME` != `$CNB_APP_DIR`

Open edmorley opened this issue 4 years ago • 7 comments

Currently the CNB platform spec says:

The platform MUST ensure that:

  • The image config's User field is set to a non-root user with a writable home directory.

...

The following variables SHOULD be set in the lifecycle execution environment and SHALL be directly inherited by the buildpack without modification: ... HOME | Current user's home directory

However it doesn't make any guarantees as to whether $HOME is set to the same path as $CNB_APP_DIR or not.

This might seem like a trivial implementation detail, but with classic buildpacks on Heroku, several buildpacks write to $HOME during the build, treating it as an ephemeral directory that won't be included in the build output.

For example the heroku-buildpack-github-netrc buildpack intentionally writes to $HOME, knowing that various tooling automatically uses such a .netrc for authentication: https://github.com/timshadel/heroku-buildpack-github-netrc/blob/5e417127367e49fdf4243da4798d89be474bf709/bin/compile#L30-L40

I'm concerned about this scenario:

  • platform A has $HOME and $CNB_APP_DIR as different paths on the filesystem
  • platform B has $HOME and $CNB_APP_DIR being equivalent
  • a buildpack author writes a netrc buildpack for platform A and it works as expected
  • the netrc buildpack is then used on platform B, where the .netrc is then saved in the runtime image (and potentially served publicly, if their web server is configured suboptimally)
  • the users and buildpack authors don’t notice this since it’s subtle/undefined behaviour, particularly if platforms don’t call out explicitly what paths are used for $HOME etc (and why would they, since on the most part the exact path used for $HOME is irrelevant for builds)

There are also cases where buildpacks unknowingly write to $HOME. For example, imagine a Python buildpack that intentionally caches only site-packages and not pip's cache (so doesn't explicitly change the pip cache directory to be in <layers>/...), but forgets to pass --no-cache-dir to the pip install invocation. This would results in pip's http/wheel cache being saved to the default path of $HOME/.cache/pip, which could easily go unnoticed on platform A in the scenario above. However on platform B, this would result in the pip cache being included in the runtime image, bloating it (which at least isn't a security issue, but still not ideal).

As such I was wondering whether the platform spec should define whether $HOME is (or is not) equivalent to the $CNB_APP_DIR path? I'm somewhat undecided to to whether "the same path" or "different paths" is best -- so long as the choice is consistent across platforms to improve buildpack portability and prevent platform-specific security issues.

This issue is something that's also come up for Heroku classic buildpacks, since we're exploring moving the build directory from a directory under /tmp to /app (so we have path parity between build-time and run-time, to resolve relocatability issues). However currently we have $HOME set to /app, so unless we change $HOME to something else (which itself will cause compatibility issues), we're going to have the same path for both $HOME and the build directory and so have to perform outreach for the netrc buildpacks et al.

cc @hone @jabrown85

edmorley avatar Feb 03 '21 20:02 edmorley

I've updated this issue to leave the choice of "same paths" vs "different paths" open (since I think there are arguments for and against both), so long as the spec defines which platforms should pick, in order to improve buildpack portability and prevent platform-specific security issues.

edmorley avatar Feb 04 '21 09:02 edmorley

The spec required that the lifecycle does not modify the value of $HOME and instead respects the values provided by the stack images.

I could see an argument for explicitly forbidding the value of $HOME on the stack image to match $CNB_APP_DIR or other directories that have a specific purpose in the spec such as the /cnb <layers> or <platform> directory to prevent unexpected behavior of the type you described above.

What interesting is that setting $HOME is a responsibility of the stack author. And setting the app dir etc is the responsibility of the platform. We currently blur the responsibilities of stack and platform author in this spec and call it all "platform" but I think we would like to introduce a cleaner separation in the future. Therefore we probably need to word this as a requirement of platforms that the value of <app> etc. provided to the lifecycle never matches $HOME (and the lifecycle should return an error if this requirement is not met).

Maybe we should also forbid buildpacks from setting $HOME for similar reasons?

ekcasey avatar Feb 10 '21 16:02 ekcasey

requirement of platforms that the value of etc. provided to the lifecycle never matches $HOME

I agree

forbid buildpacks from setting $HOME for similar reasons

That makes sense. Are any other env vars we should consider putting on a deny list. Setting $TMPDIR or $TMP to $CNB_APP_DIR could cause some unexpected behavior as well.

jabrown85 avatar Feb 10 '21 21:02 jabrown85

:wave: I'd like to suggest making it clear what changes/restrictions will be applied to the build environment and what will be applied to the launch environment. With classic Heroku buildpacks, $HOME seems to be treated as ephemeral storage during build like @edmorley said, while at launch $HOME is expected to be equivalent to the $CNB_APP_DIR directory.

I opened https://github.com/heroku/cnb-shim/pull/23 because we need to make sure that $HOME points to $CNB_APP_DIR at launch, but if buildpacks will be forbidden from setting $HOME in both build and launch, it will make that issue trickier to address. Especially because CNB exporter assumes that same $CNB_APP_DIR path on the exported image as the build environment.

kamaln7 avatar Feb 16 '21 14:02 kamaln7

I've updated this issue to leave the choice of "same paths" vs "different paths" open (since I think there are arguments for and against both)

Having worked through more of the compatibility analysis for the Heroku classic buildpack migration from building under /tmp to building under /app, we've settled on changing the build time value forHOME such that it's explicitly not equal to the build directory. We've had to leave the runtime value for HOME at /app for now, since changing it would break too many things for classic buildpacks, however if we were starting from scratch I would want to change the runtime HOME too.

As such, I would advocate for the CNB spec saying that HOME (both build time and runtime) must be a different path than the app directory.

Maybe we should also forbid buildpacks from setting $HOME for similar reasons?

Agree

I opened heroku/cnb-shim#23 because we need to make sure that $HOME points to $CNB_APP_DIR at launch, but if buildpacks will be forbidden from setting $HOME in both build and launch, it will make that issue trickier to address.

Just to close the loop here -- the CNB shim PR has since been updated to use an approach that wouldn't be affected by buildpacks not being allowed to set HOME.

edmorley avatar Mar 24 '21 10:03 edmorley

Thanks for the update @edmorley!

kamaln7 avatar Mar 29 '21 15:03 kamaln7

For our upcoming Ubuntu 24.04-based images, we've stopped overriding HOME, and so for both the build and run images HOME != CNB_APP_DIR: https://github.com/heroku/base-images/blob/main/heroku-24/Dockerfile https://github.com/heroku/base-images/blob/main/heroku-24-build/Dockerfile

This may require some apps to make changes when migrating from our classic buildpacks to CNBs, but I think it's for the best long term.

edmorley avatar Mar 17 '24 23:03 edmorley