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

Should the platform spec guarantee $HOME != $CNB_APP_DIR #186

Open
edmorley opened this issue Feb 3, 2021 · 7 comments
Open

Should the platform spec guarantee $HOME != $CNB_APP_DIR #186

edmorley opened this issue Feb 3, 2021 · 7 comments
Labels
api/buildpack enhancement New feature or request

Comments

@edmorley
Copy link
Contributor

edmorley commented Feb 3, 2021

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 edmorley changed the title Should the platform spec guarantee $HOME be a different path to $CNB_APP_DIR? Should the platform spec guarantee whether $HOME and $CNB_APP_DIR are/aren't the same? Feb 4, 2021
@edmorley
Copy link
Contributor Author

edmorley commented Feb 4, 2021

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.

@ekcasey
Copy link
Member

ekcasey commented Feb 10, 2021

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?

@jabrown85
Copy link
Contributor

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.

@kamaln7
Copy link

kamaln7 commented Feb 16, 2021

👋 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 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. Especially because CNB exporter assumes that same $CNB_APP_DIR path on the exported image as the build environment.

@edmorley
Copy link
Contributor Author

edmorley commented Mar 24, 2021

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.

@kamaln7
Copy link

kamaln7 commented Mar 29, 2021

Thanks for the update @edmorley!

@edmorley
Copy link
Contributor Author

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 edmorley changed the title Should the platform spec guarantee whether $HOME and $CNB_APP_DIR are/aren't the same? Should the platform spec guarantee $HOME != $CNB_APP_DIR Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api/buildpack enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants