%environment during 'singularity build' executed in host namespace instead of container namespace

[olifre]

Version of Singularity:

2.4.

Building a "sandbox" type container using -vvv and -d, I observe the following:

DEBUG *** FINISHING DOCKER IMPORT PYTHON PORTION ****
Exploding layer: sha256:a75635ba8c7caab6b9d746c2a2773d3379302ab74ca84c50d5ec879ebf7eb501.tar.gz
VERBOSE [U=0,P=28444]      message_init()                            Set messagelevel to: 5
DEBUG   [U=0,P=28444]      main()                                    Iterating through file looking for sections matching: %runscript
Adding files to container
VERBOSE [U=0,P=28457]      message_init()                            Set messagelevel to: 5
DEBUG   [U=0,P=28457]      main()                                    Iterating through file looking for sections matching: %files
Copying 'etc/profile.d/setupATLAS.sh' to '/etc/profile.d/setupATLAS.sh'
Copying 'etc/profile.d/setupBelle.sh' to '/etc/profile.d/setupBelle.sh'
Adding environment to container
VERBOSE [U=0,P=28461]      message_init()                            Set messagelevel to: 5
DEBUG   [U=0,P=28461]      main()                                    Iterating through file looking for sections matching: %environment
/var/lib/singularity/mnt/container/.singularity.d/env/90-environment.sh: line 3: /etc/profile.d/setupATLAS.sh: No such file or directory
/var/lib/singularity/mnt/container/.singularity.d/env/90-environment.sh: line 4: /etc/profile.d/setupBelle.sh: No such file or directory
Running post scriptlet
VERBOSE [U=0,P=28468]      message_init()                            Set messagelevel to: 5
DEBUG   [U=0,P=28468]      main()                                    Iterating through file looking for sections matching: %post

According to this output, %files is executed first. Only then, %environment is executed, however, it does not find the files that %files claimed to have copied into the container.

Checking in the container later, I find the files are present as expected.

Is the actual execution order different than the output messages?

Or, and that would be a heavy bug: Is %environment during build by any chance executed on the host machine, and only later executed inside the container?

[olifre]

Just to make sure this is clear, my recipe contains:

%environment
source /etc/profile.d/setupATLAS.sh
source /etc/profile.d/setupBelle.sh

%files
etc/profile.d/setupATLAS.sh /etc/profile.d/setupATLAS.sh
etc/profile.d/setupBelle.sh /etc/profile.d/setupBelle.sh

[olifre]

And to add:
Actually I was not even aware that %environment would already be evaluated during build...

[vsoch]

can you give the whole recipe and build command so I can reproduce?

[olifre]

Hi @vsoch ,
of course - I just cooked up a minimal failing example.

You need minimal.def:

BootStrap: docker
From: cern/slc6-base:latest

%post
echo "Conserve some space."
yum clean all
rm -rf /var/cache/yum

%environment
source /etc/profile.d/setupATLAS.sh

%files
setupATLAS.sh /etc/profile.d/setupATLAS.sh

And also setupATLAS.sh:

function setupATLAS() {
  echo "This should do something."
}

Then you run:

singularity build foobar.simg minimal.def

Then you will get:

Adding files to container
Copying 'setupATLAS.sh' to '/etc/profile.d/setupATLAS.sh'
Adding environment to container
/var/lib/singularity/mnt/container/.singularity.d/env/90-environment.sh: line 3: /etc/profile.d/setupATLAS.sh: No such file or directory

My main issue here is that I do not understand why the environment is being sourced at all at this point.
Since (if I got that right) the %files phase is still running on the host, I would expect the same to be happen for the %environment phase if it is executed here.

I just did a test, created a /etc/profile.d/setupATLAS.sh on my host, containing:

echo "That's the wrong one!"

and actually, running another:

singularity build foobar.simg minimal.def

I get:

Adding files to container
Copying 'setupATLAS.sh' to '/etc/profile.d/setupATLAS.sh'
Adding environment to container
That's the wrong one!

I hope we all agree that's a fatal bug...

[olifre]

Hi @vsoch ,
I adapted the title accordingly.

[vsoch]

I can reproduce this (with sudo for build)

sudo singularity build foobar.simg slc6.def 
Using container recipe deffile: slc6.def
Sanitizing environment
Adding base Singularity environment to container
Docker image path: index.docker.io/library/ubuntu:latest
Cache folder set to /root/.singularity/docker
Exploding layer: sha256:ae79f251470513c2a0ec750117a81f2d58a50727901ca416efecf297b8a03913.tar.gz
Exploding layer: sha256:5ad56d5fc14905886c560200ab69f905b5c5287eaf12f8f761a7ab54f7a61c1b.tar.gz
Exploding layer: sha256:170e558760e8b2e484a022b7d7272cf284fc4e1936ba7a0a671fc586440ad272.tar.gz
Exploding layer: sha256:395460e233f5bdcd910d618a3b615e0d881e09ad27d58f3065eef53ecae6a808.tar.gz
Exploding layer: sha256:6f01dc62e444044e3ce494269837ef0aedb80fef69c679416137f17812d2eb9c.tar.gz
Exploding layer: sha256:f6be9f4f6905406c1e7fd6031ee3104d25ad6a31d10d5e9192e7abf7a21e519a.tar.gz
Adding files to container
Copying 'setupATLAS.sh' to '/etc/profile.d/setupATLAS.sh'
Adding environment to container
/usr/local/var/singularity/mnt/container/.singularity.d/env/90-environment.sh: line 3: /etc/profile.d/setupATLAS.sh: No such file or directory
Running post scriptlet
+ echo Conserve some space.
Conserve some space.
Finalizing Singularity container
Calculating final size for metadata...
Skipping checks
Building Singularity image...
Cleaning up...
Singularity container built: foobar.simg

and then the source doesn't work

singularity shell foobar.simg 
Singularity: Invoking an interactive shell within container...

/.singularity.d/actions/shell: 3: /.singularity.d/env/90-environment.sh: source: not found

but if I change the line to . instead of source, and then make it just an echo instead of a function, we see the expected:

 singularity shell foobar.simg 
Singularity: Invoking an interactive shell within container...

This should do something.
Singularity foobar.simg:~/Desktop> 

but not to get distracted! It is indeed the case that we are in user space during the environment section - I did the same and added a file to my host, and it's executed:

Copying 'setupATLAS.sh' to '/etc/profile.d/setupATLAS.sh'
Adding environment to container
This is on the host, ruhroh  <---

The good news is that we get the file we expected in the container. The bad news is that the build has complete access to the host, so a malicious user could share a build script that might execute something there and act maliciously (if they don't check).

[olifre]

The good news is that we get the file we expected in the container. The bad news is that the build has complete access to the host, so a malicious user could share a build script that might execute something there and act maliciously (if they don't check).

Yes, I can confirm that the file ends up at the correct place, and singularity shell will use that.

Apart from the malicious user, this also hinders a non-malicious user. Say I want to do (and actually I think I even want to do that!):

%environment
source /etc/profile

in my build definition, such that the constructed container will in the end always use the environment from the /etc/profile stored within the container.
Doing that, I end up making the result of the build completely dependent on the host machine, since during build, /etc/profile from the host will be sourced.

I don't see a clean workaround for that usecase, apart from fixing Singularity code. Only unclean solutions (like renaming or aliasing /etc/profile in the container) come to mind.

[vsoch]

I'm thinking of things we can do to fix this. If we limit sourcing to during chroot, then it would find the right file under the new root (where the container is being built) but the variables sourced would be lost when we move to the next command no?

Most users wouldn't like this, but I think we should not be sourcing the environment at build time - variables that are needed can be defined in %post. On the other hand, we are pretty lenient when it comes to having %setup - using it period and not supplying the dependencies/files that are needed means a container that is also dependent on the host. What are some other options to fixing this?

[olifre]

I would agree the complete idea to run the %environment section already during build is wrong. Even the documentation states:

As of Singularity 2.3, you can add environment variables to be sourced when the container is used in the %environment section. 

Since I am building and not using the container, why is %environment being executed? And it's even running in the wrong namespace.

For me as user, this is (from the logical point of view) unexpected, undocumented, unsafe, reducing portability, and generally unwanted.

So in my opinion, the best fix would be to just not do that during build.
If there are really users wanting that, maybe you could add some flag like
%environment -b
which then leads to the behaviour that the environment stuff is already executed during build time.

Apart from that...

the variables sourced would be lost when we move to the next command no?

I don't understand - why would they be lost? I did not read through singularity code too much yet, but I would expect the chroot is done once, and then all sections which run in there execute in the very same environment.

[vsoch]

In the case of having two different chroot it would be lost. I think we would just do the sourcing to go along with the primary session for %post. I am +1 about removing the sourcing, period, because I think it's more secure and trivial to ask users to define that is needed in %post. @gmkurtzer your thoughts on this?

[olifre]

In the case of having two different chroot it would be lost.

Of course. Is that how it's done in the code? If so, I would ask why - I don't see any safety or performance gain from that.

because I think it's more secure and trivial to ask users to define that is needed in %post.

👍

[bauerm97]

@olifre This is a really good catch, thanks for bringing it to our attention. I agree with you both, that the default behavior should not be to source the %environment section at build time. It seems like when we re-did the build workflow this got overlooked. @GodloveD could you comment on this more, since I know that you did a lot of the work rewriting the build code?

[vsoch]

also let's be sure to remove the same sourcing of the apps environment at runtime (which I created mimicking the original %environment section.

[GodloveD]

@bauerm97 this was not overlooked. It was consciously added some time ago by @gmkurtzer.

The idea was the people could add variables in the %environment section and those variable would be available during %post. I'm almost sure we had some users asking for this because they thought it was redundant to add the same env vars 2 different sections. (It was from their perspective "unexpected"). But although I do remember discussion I can't find any issues referencing the topic. Maybe it was in the Google group or on Slack.

I think that is a good idea and it works well for the most part. But it obviously fails if your /.singularity.d/env/90-environment.sh sources external scripts. I'm not sure if we really want to remove this since I'm pretty sure users were asking for it. But we should probably do something about the error too. I'm not really sure what the solution would be.

As far as this being an unsafe bug... meh. I don't really think so. If you find some rando script on the internet and run it as root you could break your computer. Singularity recipe files are no exception. The %setup section can do all sorts of bad things to your computer. But that is the nature of running things as root. You are supposed to have some idea of what you are doing.

One workaround would be to copy the contents of the files you are trying to source to /.singularity.d/env/90-environment.sh. If we continue to support this, the code should really be sourcing every file in the /.singularity.d/env/ directory. So if we fixed it to source everything you could just drop the files you are trying to source in /.singularity.d/env/ as well instead of sourcing them from /.singularity.d/env/90-environment.sh.

[olifre]

@GodloveD For very simple variable definition, this might indeed be ok.
But I believe everybody will perfectly agree with me that:

%environment
source /etc/profile

will lead to a completely unexpected, always unwanted, and certainly not documented mess.

That's why I would propose to at least make this optional. Please let me turn this strange behaviour off. If this was discussed on slack with some user's, I don't care too much - if an open source project has to base decisions on some feedback collected on an invite-only platform thus excluding most of the users, I'd say something is going wrong in selecting the audience.

Putting that aside, I would expect singularity follows common behaviour (i.e. "like docker"): Don't allow access to things from the host (or at least outside of the build directory) during build. I don't see how anybody would expect things to work as they work right now.

To get some progress here, what I would propose is the following. This still works for the special users needing that strange behaviour, also works for me, and is not too hard to code.

1. Let %environment run within the chroot during build. Then it can't access anything from outside the build directory.

2. Add parameters to %environment. I propose:

%environment -b -r
export MY_BUILD_VAR=foo

This would run during both build and runtime phase. This helps the users who really want variables defined in both phases.

%environment -r
source /etc/profile

This only runs at runtime, and is not executed during the build phase.

%environment -r
source /etc/profile

%environment -b
export BUILD_VAR=1

This obviously lets one section run during build, and the other during runtime later.

I guess this is very easy and straightforward to implement. If you really already go for backward compatbility (I don't believe that - since you dropped complete commands and fully reworked behaviour with 2.4, and not all have been "deprecation-wrappered"), then the default could be to interpret %environment as %environment -r -b. If you don't go for backwards compatbility, please ask your users (and not only few selected slack people) what they actually expect - and please document whathever is the default.

Does that sound reasonable?

[olifre]

Another alternative would be to offer a new section:
%container-startup
which is meant to run when you enter the container, and only then.
Then you can leave %environment completely as it is, but in any case, please document the strange and unexpected default behaviour of %environment (unexpected since it's completely different from any other container solution on the market).

EDIT: The more and more I think about it, the more reasonable this approach would be.
%environment in the end really should be for environment variables only, maybe you should enforce that.
%container-startup (or whatever it will be called) would then really be allowed to contain shell code to be sourced, or define functions, aliases, whatever you may want.

[olifre]

Until something like this is offered, a viable workaround should be to add to %post:

echo "Adding container startup environment."
cat << EOF >>${SINGULARITY_ENVIRONMENT}
source /etc/profile.d/setupATLAS.sh
source /etc/profile.d/setupBelle.sh
echo "Welcome inside the Ubuntu 16.04 container."
EOF

While this workaround for sure works very well, I don't consider it a "solution".

1. %environment is still documented wrongly (documentation claims it's run to set up the in-container environment), and it's impossible to make it not run on the host. Also, it should accept environment variable definition only, and / or be limited to the chroot environment during build.
2. A separate section like %container-startup is a significantly cleaner solution than this workaround, especially since it could be re-run via singularity build --section container-startup.

Please let me know what you think.

[vsoch]

I just hit another compelling example for why sourcing environment is a terrible idea. Take this runscript it fundamentally changed the entire $PATH so that executables needed at build time were not found, and I got a weird error (that I just spent about an hour debugging) for a missing chroot (which made no sense). Removing that section fixed the bug. On the one hand, we could call it bad hygiene / practice to do such a thing. On the other hand, I don't think that runtime and build environments are really the same. If there are shared variables, then the writer of the recipe can export them twice. I'm up for any of the suggested solutions that get around this. My preference is for the simplest - %environment is for run time and that's it.

[olifre]

My preference is for the simplest - %environment is for run time and that's it.

I'm perfectly with you on this, there are very many reasons I think this is a terrible idea, and I don't really see how this could lead to expected behaviour.

If there's some veto against it by the project leader / majority (reasons could be backwards compatibility, users relying on that strange behavior), my priority for alternative solutions would be:

1. Make %environment runtime only by default, and allow to specify parameters (-r, -b) to allow users to go back to the current behaviour. This allows for the wanted variable definition deduplication, if this really was the original reason. If this is also shot down:
2. Enforce that %environment can only define actual environment variables for runtime and build time, but put a big fat warning in the documentation that this is usually a bad idea, and introduce %container-startup. If this is also not accepted...
3. Leave %environment as it is, put an even bigger fatter warning in the documentation, and introduce %container-startup.

[GodloveD]

Oh sorry @cclerget I just realized that you also have a PR in to address this.

[olifre]

Oh sorry @cclerget I just realized that you also have a PR in to address this.

If I read the changes correctly, this fixes neither the problem @vsoch mentioned,
nor does it allow to turn off sourcing of the environment at build time.
It only enforces that it is done inside the chroot (which is already a significant improvement, I agree).

[GodloveD]

@olifre Yes you are correct. But it seems to me that the main issue here is that sourcing needs to happen in chroot.

I forgot that @vsoch mentioned we need to change sourcing of env vars for the app sections as well (sorry v!). I'm trying to figure out how that code works right now. If it can be done easily with my method I'll just tack into onto my PR. But if it is complicated I see that @cclerget is already addressing those bits so I may just PR him some changes to his PR instead.

As for turning off sourcing of the environment at build time, I think the most straightforward method would be to add a flag to the build command that prevents sourcing the environment(s) during build. I think the --cleanenv option would be a good candidate since it is not yet being used in build. How does that sound?

[vsoch]

hey @GodloveD it's exactly the same, in the deffile-sections file, just go down to %appenv and you will see the same logic as for the main environment.

@olifre it definitely still has some issues, but sourcing in chroot is minimally a start, and then we can look into if it works (or if we want further change). For example, for your main issue since the build is isolated from the host you wouldn't have unknowingly copied your host's files.

@GodloveD here is the line of interest! https://github.com/singularityware/singularity/blob/development/libexec/bootstrap-scripts/deffile-sections.sh#L343 And I also like the idea of --cleanenv - but what if there is a case that the user wants to not source environment variables during the build, but still wants access to some of the hosts to perhaps write to the SINGULARITY_ENVIRONMENT? The most intuitive meaning of cleanenv would be as it is for the other commands - not having the host environment available. But in this case, we do want it available. We just don't want to source it.

[olifre]

@vsoch Agreed, it's a start, but it still means (for me) that code would be executed which I don't want executed during build (even though it will not be harmful anymore, since at least it's the in-container code now). Still, it will also not fix your PATH example.

I think the most straightforward method would be to add a flag to the build command that prevents sourcing the environment(s) during build. I think the --cleanenv option would be a good candidate since it is not yet being used in build. How does that sound?

I thinks that's a pretty strange idea. If I ship a build recipe e.g. via github in which the %environment section is meant for runtime usage, it means I have to tell (in some readme or whatever) any person who wants to use my container recipe to pass a special parameter during singularity build.
This is completely against the idea of having self-contained recipes (+ files to go inside) defining the produced container. Also, how would I tell Singularity Hub to pass that flag to build when building the container?

[vsoch]

yes good call, I concur.

[GodloveD]

@olifre some users want %environment to be accessible from within %post. What I'm hearing you saying is that you do not. So we need to support both methods. And since the default is that %environment is available right now, I don't think it's a great idea to change that behavior. I think it's a better idea to just be able to turn that option off when you want to. If that means you have to pass an extra option at build time, I think that is a small price to pay.

I'm not crazy about changing the sections in Singularity recipes to try to determine which behavior you want. This is deep enough in the weeds that many users will never even have to think about it. Why make them learn about all of this complexity for a few corner cases?

[olifre]

So we need to support both methods.

Agreed.

And since the default is that %environment is available right now, I don't think it's a great idea to change that behavior.

This is why I suggested %container-startup to define a runtime-only %environment.

If that means you have to pass an extra option at build time, I think that is a small price to pay.

If I have to pass that option to the singularity binary (and can't put that in the recipe), it means recipes essentially become useless, since they don't guarantee container state anymore - and Singularity Hub needs funny extensions.

Why make them learn about all of this complexity for a few corner cases?

I have a problem here. Can somebody show me any example when it's actually useful to share %environment for build-time and run-time?
I don't see any, but maybe my imagination is limited.

Examples for %environment use that come to my mind would be:

• Pre-define a PATH the container should set to be used after I have installed something in %post.
• Pre-define an LD_LIBRARY_PATH the container should set to be used after I have installed something in %post.
• Source /etc/profile to pick up any distribution specifics the user may want to have inside an interactive job, e.g. via HTCondor.

Maybe there are good use cases, I just don't see any, please enlighten me.

Also, in case %environment is made runtime only (I'd still prefer that), deduplication of %environment for build and runtime is still perfectly possible:

• Add a separate file containing the special environment settings.
• Copy that into the container via %files.
• Source it in %post and in %environment.

My problem is that I think this strange behaviour (%environment is shared for runtime and build time) is the actual corner case. Please show me why it is not.

[vsoch]

What if we use the same (simple) standard that we have to distinguish between %setup and %post:

• %setup you have access to the host, if environment variables are badly needed from the host they can be added here.
• %post is an isolated build, no access to host system or environment
• %environment is considered a static text file that is only used after building

If a user needs to define an environment variable during build (%post) it's still possible via $SINGULARITY_ENVIRONMENT. If a variable is needed from %environment for build, well they can just copy paste it and use it twice. In my mind this is better overall documentation of the build because it asserts that the variable is necessary for build and/or runtime.

I understand that we had users that wanted the %environment section sourced, but I don't see that need as a deal breaker for this fix. If they need the variable in both, they can just write it twice. Thoughts?

[olifre]

@vsoch Fully agreed. 👍
Especially the better overall documentation, which I did not explicitly think off, is a very strong point. It's much cleaner if the build recipe cleanly shows off which variables are actually used in which section.

And to add, if the users are really against writing it twice, they can use my trick described in the previous comment.

[GodloveD]

OK. So what I'm hearing is that:

1. We should remove these lines and these lines.
2. @vsoch will happily handle any user complaints that arise from reverting to the previous unpopular behavior. 😹

Does that sum up the request?

[olifre]

@GodloveD Exactly. And if any users complain here, feel free to ping me, I'm willing to take any blame that may arise, and help explaining why this was a bad idea.

Additionally, I'd suggest to add some more explicit hints to the documentation of %environment (along the lines "this defines container runtime environment"). Right now, the docs say:

As of Singularity 2.3, you can add environment variables to be sourced when the container is used in the %environment section. 

This actually already describes the behaviour I am asking for, but maybe it needs to be stressed more that this really is for runtime environment only.

And of course, one should mention this (slightly) backwards incompatible case in the release notes. Feel free to blame me whenever somebody complains ;-).

[vsoch]

Docs are updated. That looks perfecto @GodloveD !

http://singularity.lbl.gov/docs-recipes#environment

I think if we look at this from the point of security, it's an easy answer. One approach leads to possibly bad behavior (that the user building may not be aware of) and the other is slightly more annoying sometimes, but is consistently likely to be more secure.

@GodloveD yes, that's what I'm here for!

[vsoch]

This is all fixed. Thanks everyone!

[remyd1]

Hi,

This documentation needs to be updated too : http://singularity.lbl.gov/docs-environment-metadata

I was not aware of this modification. IMHO, I think we need compatibility with older recipes. For me, even if %environment behaviour was strange, we finally get some habit to use it as it was.

From a a syntax and a user point of view, I would prefer using flags -r and/or -b with %environment than using %setup...

This new behaviour is a bit confusing too...

Rémy

[roxma]

So if I need %environment during build time, I'm going to add . /.singularity.d/env/90-environment.sh at the beginning of th %post section?

[vsoch]

I think it would be most clear to just export the variables again in post. If you aren't sure what they are from some previous definition, then source the environment file. Keep in mind this is shell so you might want to use . instead of source explicitly.

[samcmill]

Are the environment file names in /.singularity.d/env reliable? I mean, they are not going to change without some notice?

I observed some different behavior when building Docker images vs. Singularity images for the "same" HPC Container Maker recipe. The root cause turned out to be that an environment variable from the Docker base image was not set in the Singularity build context. So I'd like to add the following %post section immediately after bootstrapping from the Docker image:

%post
    . /.singularity.d/env/10-docker.sh

[MaliRemorker]

As far as I can see, these days (2.5.2-dist build available from Debian Buster (testing)), it is not even possible to issue source commands in %post . Are you aware that there are compilers (AOCC suite, for example) that generate scripts consisting of "export" commands that have to be sourced prior to using them? If I cannot source the generated script during %post, I will also be unable to use this compiler to build dependencies of the containerised applicatoin during %post.

I can think of two workarounds:

1. Install the compiler once, then paste exports to %post directly
2. Call singularity recursively (from within a cointainer)

Both options seem like an overkill. Any suggestions?