build_env: allowlist rather than blocklist

[trws]

Stop removing environment variables to fix problems and instead completely annihilate the entire environment, it's the only way to be sure.

Less flippantly, this adds a helper function to the env utility to
restore an environment variable to its current state after it's been
cleared. Then build_environment clears all environment variables, and
restores only those variables that we explicitly allow to persist.

This solves much of the fragility of building packages that take poorly
named common environment variables silently as arguments, such as
OpenSSL, but will likely cause breakage for cases where packages
happened to accidentally work due to re-using external state that will
no longer be visible.

[tgamblin]

Not clear to me that we should keep any of these except SPACK_*. Is there some reason not to start PATH out empty or with some very small explicit set of paths?

[trws]

At the time, we relied so heavily on leaking that it seemed necessary, now I'm not so sure. It would be worth testing for sure.

[tgamblin]

I think we should wall the whole env off and be purely constructive. Things can still leak in through externals, but that's mostly unavoidable.

[trws]

I agree, but we are missing tons of dependencies on basic system tools like sed, awk, etc. It might be a big lift if /usr/bin and /bin are gone. Maybe we can include those for now and start tracking how many things like that are left?

[tgamblin]

I think it's worth running a pipeline to see what happens... I agree we don't test everything but I wonder if we can handle a lot of these at the build tool level (e.g. for autotools packages). I think we have to get them updated at some point.

[trws]

It occurred to me we could test a version of path clearing with the spackos container. I was pleasantly surprised with the minimal set in path, but the minimal set is missing a couple of things to actually go from nothing. This is the set I found required to run spack and build basically anything:
(edited: fix awful formatting)

• binutils
• gcc (clang would work, might need patches for some packages)
• m4
• ncurses
• bash: dash might do it, but untested
• coreutils
• diffutils
• file
• findutils
• gawk
• grep
• bzip2
• gzip
• gmake
• patch
• sed
• tar
• xz
• gettext: I'm pretty sure we don't actually need this, only if developing on packages, but it's in the dep set
• bison
• perl
• python

If any of this stuff is missing, by which I mean they are installed by spack but not in path, things don't build. Like zlib doesn't build because it doesn't have an explicit dep on tar to add it to path. Similar issues happen for many of the others. Good thing is this isn't all that different from the normal requirements list, maybe worth just adjusting that and then making these things available in a separate bin as was suggested? In the end it's not all that different from how the spackos container works by default, all of that stuff (plus some bootstrap deps and an editor) get shoved into an env that isn't loaded but has its view in path.

[scheibelp]

Some changes here appear to have been semantically superseded, like those in lib/spack/spack/util/path.py, which added some fallback logic for determining the user:

    try:
        user = getpass.getuser()
    except ModErrType:
        user = os.getlogin()

but that logic has since changed the mechanism of determining user (admittedly, getpass.getuser is a fallback for a 3rd mechanism, so it's not clear to me if os.getlogin() should remain as a fallback for this fallback).

[trws]

Honestly I'm not sure, depends on what the third way is. That went in because of the loss of the USER environment variable causing getuser to fail. If the third is fine and doesn't cause issues then it can probably go.

[scheibelp]

I've synced this with develop but it

• seems risky: all I want to do is clear PATH, I don't know the effects of clearing all env variables and would probably want to consider the effects one by one. Moreover, if users want to set a single environment variable themselves, which is then passed to the build, then they have to perform dirty installs, so overall I'm not sure about the whitelist approach here
• I was mainly drawn here with hopes of sanitizing path, which is entirely TBD (although there is an excellent list of all the needed utilities here)
• I'm not clear on what the expectations are WRT restore

[scheibelp]

@trws nothing actually handles this request, does it? In that sense it seems incomplete

[trws]

The enclosing class is our list of environment modifications to be made after clearing the environment, so this re-uses the normal set method to add the contents of the current environment to the list to be restored. If there's something wrong with it I'm not sure what.

[scheibelp]

Thanks, that makes sense, e.g. for

env.restore("CRAY_LD_LIBRARY_PATH")

you are adding the original value as a new set operation after unsetting it, the important thing being that the env modifications haven't been executed on the current environment so we are recording the unset followed by the set. Is there a reason to just not unset them in the first place (e.g. include them in the whitelist)?

[trws]

Exactly. I had to dig back through it a bit to try to remember. The end effect is very similar to adding it to the whitelist, I think it was meant to allow packages to explicitly do this in things like setup_build_environment or similar without having to pass around the whitelist. I can't remember why that particular one is done that way though.

[scheibelp]

As observed in #30015 (comment), we'd have to construct a minimal PATH with a set of utilities in order to clean it (i.e. point to some new PATH dir with the minimal set of executables, and start with PATH set to this one entry to achieve the best isolation we can).

[trws]

FWIW, I started tilting at this particular windmill because I had an environment variable I use all the time called something like SYS that happens to be reused unsanitized by scripts in the openssl build, and caused it to fail in incredibly strange ways, sometimes. There are all kinds of issues like this because of the way that configure and make work, so I think we would be a whole lot better off clearing them all and reintroducing only if provably necessary. Otherwise our builds are unreproduceable, and there's no fixing them.