Make singularity binary runtime relocatable

[chrisburr]

Description of the Pull Request (PR):

On a few occasions there have have been requests for the singularity binaries to be runtime relocatable (when being used with user-namespaces). Personally, this is interesting to me for conda-forge and DIRACOS which both provide software installations which can be relocated without the need for rebuilding.

This is achieved by using os.Executable to determine the path the current running binary and path lookups are then modified accordingly. It's my first time writing go so the code might need some work but I'm primarily interested in if this is a feature that you're willing to support?

This patch has been used by conda-forge and DIRACOS for the past few months without any issues. Within DIRAC it has been used by O(100,000) continuously running jobs.

This fixes or addresses the following GitHub issues:

• Relocatable RPM (or instructions) #48
• ERROR : User namespace not supported, and program not running privileged. #267
• unprivileged singularity code is not relocatable #887

Before submitting a PR, make sure you have done the following:

• Read the Guidelines for Contributing, and this PR conforms to the stated requirements.
• Added changes to the CHANGELOG if necessary according to the Contribution Guidelines
• Added tests to validate this PR and tested this PR locally with a make testall
• Based this PR against the appropriate branch according to the Contribution Guidelines
• Added myself as a contributor to the Contributors File

Attn: @singularity-maintainers

[dtrudg]

Hi @chrisburr - thanks for posting this. There is definitely interest in accomplishing this goal, but it would be a bit more involved than the patch here. A couple of points off the top of my head:

• We would definitely need to have improved testing that is properly verifying that Singularity is only accessing the intended locations of singularity.conf and the other files that are security critical - so that our CI can prove this is the case as changes are made over time.

• When building Singularity currently you can set paths for many components, e.g. localstatedir, sysconfdir, libexecdir and other portions of the installation separately from a single prefix. This means that it is not necessarily the case that looking up from the executable and assuming a fixed hierarchy will work. We'd need to consider how to either accommodate this... or more likely ensure that a build that doesn't use a simple hierarchy is not going to attempt to be relocatable.

I'm going to mark this PR as a request change so it isn't accidentally merged - but it is definitely a good idea if it can be done in a way that handles the various loose ends, and is provably correct through tests.

@cclerget and @ikaneshiro may want to join the discussion here too.

[dtrudg]

Please see notes in the thread. This is a nice goal, but would need to be worked up some more.

[cclerget]

Thanks for the PR @chrisburr.

The relocation of files may not work as expected like @dctrud mentioned, that would probably require some additional checks at compile time in mconfig to make sure an installation is relocatable.

[cclerget]

It would probably make things easier to maintain by calling RelocatePath only for the relevant defined variables, so far those variables are actively used by singularity :

BINDIR
LIBEXECDIR
SYSCONFDIR
SESSIONDIR
SINGULARITY_CONFDIR
PLUGIN_ROOTDIR

[cclerget]

RelocatePath should return original when SINGULARITY_SUID_INSTALL == 1 as the very first check to avoid to reach panic panic("Unrecognized executable: " + executablePath) at line 76 when the executable is starter-suid and to also ignore relocation for setuid installation :

func RelocatePath(original string) (string) {
    if SINGULARITY_SUID_INSTALL == 1 {
        return original
    }
    ...
}

Regarding panic in switch it would be safest to return original as default and could be simplified to:

	switch filepath.Base(executablePath) {
	case "singularity":
		// PREFIX/bin
		prefix = filepath.Join(filepath.Dir(executablePath), "..")
	case "starter":
		// PREFIX/libexec/singularity/bin
		prefix = filepath.Join(filepath.Dir(executablePath), "..", "..", "..")
	default:
		return original
	}

[chrisburr]

Thank you @dctrud and @cclerget for reviewing this PR promptly and I'm sorry for not finding time to address your suggestions until now.

We would definitely need to have improved testing that is properly verifying that Singularity is only accessing the intended locations of singularity.conf and the other files that are security critical - so that our CI can prove this is the case as changes are made over time.

I think this isn't necessary now that relocation is only supported for non-setuid installs. Do you agree or are there other security concerns?

When building Singularity currently you can set paths for many components, e.g. localstatedir, sysconfdir, libexecdir and other portions of the installation separately from a single prefix. This means that it is not necessarily the case that looking up from the executable and assuming a fixed hierarchy will work. We'd need to consider how to either accommodate this... or more likely ensure that a build that doesn't use a simple hierarchy is not going to attempt to be relocatable. This means that it is not necessarily the case that looking up from the executable and assuming a fixed hierarchy will work.

I'm inclined to agree and say it's only supported for simple hierarchies rather than adding the complexity of doing this generically. The confgen/gen.go script can always be updated in another PR if someone is interested in more esoteric layouts.

I've tested this for conda-forge to make sure the relocation still works with the latest changes and the build has succeeded. I'm not sure how this could be tested in the context of your CI pipeline as it would require singularity to be reconfigured/built with the --without-suid flag. Do you have any suggestions? Or do you think it's not worth the hassle required to test this feature explicitly?

[dtrudg]

Thank you @dctrud and @cclerget for reviewing this PR promptly and I'm sorry for not finding time to address your suggestions until now.

Thanks again for the contribution!

We would definitely need to have improved testing that is properly verifying that Singularity is only accessing the intended locations of singularity.conf and the other files that are security critical - so that our CI can prove this is the case as changes are made over time.

I think this isn't necessary now that relocation is only supported for non-setuid installs. Do you agree or are there other security concerns?

I feel will still do need some sort of testing. We need to guard against the case that code is accidentally modified in the future, giving a pathway to it being used in a setuid install. Given that pointing to different locations in a setuid install is pretty dangerous, I feel quite strongly about this.

When building Singularity currently you can set paths for many components, e.g. localstatedir, sysconfdir, libexecdir and other portions of the installation separately from a single prefix. This means that it is not necessarily the case that looking up from the executable and assuming a fixed hierarchy will work. We'd need to consider how to either accommodate this... or more likely ensure that a build that doesn't use a simple hierarchy is not going to attempt to be relocatable. This means that it is not necessarily the case that looking up from the executable and assuming a fixed hierarchy will work.

I'm inclined to agree and say it's only supported for simple hierarchies rather than adding the complexity of doing this generically. The confgen/gen.go script can always be updated in another PR if someone is interested in more esoteric layouts.

I've tested this for conda-forge to make sure the relocation still works with the latest changes and the build has succeeded. I'm not sure how this could be tested in the context of your CI pipeline as it would require singularity to be reconfigured/built with the --without-suid flag. Do you have any suggestions? Or do you think it's not worth the hassle required to test this feature explicitly?

This testing is a bit more difficult... let me think about it a bit.

[chrisburr]

I've added 6602ed1 which prevents the relocation code from being generated at all if it's a setuid install. If the code is accidentally modified to try and relocate a setuid install it should:

1. Fail to compile due to variables being const
2. Fail to compile due to RelocatePath not being defined
3. Skip the relocation due to the SINGULARITY_SUID_INSTALL check in RelocatePath

Actually (3) could be made a panic to prevent it being accidentally relied upon (4f09a9e).

Another crude check could be to grep the binaries to make sure RelocatePath isn't defined:

# Without setuid
$ rg --binary RelocatePath builddir/
Binary file builddir/cmd/starter/c/starter matches (found "\u{0}" byte around offset 7)

Binary file builddir/cmd/starter/c/starter-suid matches (found "\u{0}" byte around offset 7)

Binary file builddir/singularity matches (found "\u{0}" byte around offset 7)
# With setuid (no matches)
$ rg --binary RelocatePath builddir/

[chrisburr]

This testing is a bit more difficult... let me think about it a bit.

It's not ideal but one option is to manually test release candidates using conda-forge like how I've been doing for this PR (without the need to patch of course). The procedure would be:

1. Edit the version and sha256 fields in recipe/meta.yaml
2. Make a draft PR with @conda-forge-admin, please rerender in the description to make sure the CI config is up to date

If the CI passes everything then relocation is fine as it has successfully ran the lolcow image and more tests could be added to conda-forge if deemed useful.

[dtrudg]

@cclerget @ikaneshiro - any thoughts on the code now, and how testing it might be relatively easily integrated into our CI? I think we'd need a rather big change to do that... but I'm wary of having this type of stuff not automatically tested.

[dtrudg]

Another issue - this may be be incompatible with the 3.6.4 unsquashfs wrapping, as a user of conda-forge Singularity has reported a panic: #5668 (comment) - not clear whether that issue is relevant in this code.

[chrisburr]

The issue is explained by the warning:

WARNING: Skipping mount proc [kernel]: /proc doesn't exist in container

If /proc is unavailable then os.Executable() can't be used for finding the path to the current executable.

I'm not sure what the changes are in Singularity 3.6.4 but the failure to mount /proc expected? If it is then it's a serious issue for this patch.

[dtrudg]

@chrisburr - The big change in 3.6.4 is that due to a security issue that occurs from unsquashfs not sanitizing paths, we are now executing it in a minimal container sandbox. Looks like /proc is not getting mounted there the way this install is setup.

Pinging @cclerget for any thoughts about this.... should we be making a /proc dir in the unsquashfs container?

[chrisburr]

@dctrud @cclerget Any thoughts on this? I think I could avoid needing /proc to be mounted in the container by passing an environment variable that points to the base of the singularity installation but I'd rather not if it's possible to avoid it.

[chrisburr]

Ping @dtrudg @cclerget

(rebasing has caused a unit test failure to appear, I'm not convinced it's related but I'll look into it once we have a decision on how to proceed with this PR)

[chrisburr]

Hi again @dtrudg @cclerget, do you have any thoughts on making a /proc dir in the unsquashfs container or if I should try to find a way of making singularity relocatable without? I've been stuck on 3.6.0 for a while now and #5340 is quite annoying.

[dtrudg]

Hi @chrisburr - I'll ask @cclerget again if he can respond on that as it's related to the wrapping of unsquashfs for security reasons, which he coded up.

If he's not able to I'll try to take a look soon. Sorry for the inaction on this.

[DrDaveD]

It may or may not be relevant that both docker and kubernetes also prevent unprivileged mounting of /proc by default inside containers. They do provide options to enable it though and I don't think it is a security concern when used along with the no new privileges kernel feature that singularity always enables. For reference, the docker option is --security-opt systempaths=unconfined. The kubernetes option is documented as being in the PodSecurityPolicy under allowedProcMountTypes Unmasked but I haven't so far been able to convince a kurbernetes administrator to configure that to demonstrate it.

[cclerget]

Sorry for the delay, after looking at the issue with /proc missing in the unsquashfs sandbox, it can also be triggered with a container which doesn't have /proc or with a mount override like :

singularity shell -B /tmp/empty:/proc image

This is caused by RelocatePath being called in golang init() context due to the variable assignment in the global context, as we have a portion of Singularity code which is executed in the container context, it may not found /proc/self/exe link.
Since this portion of code should not rely on any buildcfg variables, I think it's safe to return original value in the case of a missing /proc :

	executablePath, err := os.Executable()
	if err != nil {
                if !os.IsNotExist(err) {
		    panic(err)
                }
                return original
	}

I really doubt sane systems doesn't mount /proc, and in the case where Singularity is run inside a container without /proc well I expect the container has a clean installation of Singularity which shouldn't require the relocation path.

[chrisburr]

Hi @cclerget, thanks for taking a look!

Since this portion of code should not rely on any buildcfg variables, I think it's safe to return original value in the case of a missing /proc :

It looks like you're correct and 1990a60 has fixed the issue so that was a much easier fix than I expected. I think this PR is now ready from my perspective.

(It's exposed an unrelated issue, #5858, that affects relocation but that's a task for a separate PR.)

[cclerget]

LGTM, thank you very much @chrisburr for your patience and your work 👍 . Added for 3.8 milestone

[dtrudg]

Opened - apptainer/singularity-admindocs#114 - as a docs reminder for this.