Remove UID/GID hack script

[zachmullen]

This is a better cleanup solution; we run a very small container
to clean up the temp directory for us as root.

[kotfic]

I know generally we want to avoid replicating information, but it seems like hard coding '/mnt/girder_worker/data' would avoid potential attack vectors here

[zachmullen]

What sort of attack? So long as DATA_VOLUME is only ever a string constant, wouldn't this be fine?

[kotfic]

maybe i'm not reading the right, but if I can modify _tempdir so it is equal to '/' and DATA_VOLUME so it is equal to "/" won't I blow away your whole OS?

[zachmullen]

I think if anyone managed to change the value of these variables, we have already been compromised to the point that an attacker could likely do whatever they want on the filesystem within the constraints of the worker user.

[jbeezley]

I would say the problem is less about an attacker changing it than a user or developer accidentally doing something wrong.

[zachmullen]

Either way, the tmpdir variable is the one that presents the danger more than the DATA_VOLUME since that's the one referring to the host filesystem.

[manthey]

Instead of rm -rf DATA_VOLUME, we could chmod -R a+x DATA_VOLUME, and then let the temp directory get cleaned up via the normal job cleanup. This has the virtue that the cleanup flag would be honored.

[zachmullen]

Well, if that got compromised in the same way, then instead of deleting our root filesystem, we've just given write permission to every user on every file... I'd rather just have them delete the files since it's an ephemeral processing node.

[manthey]

I was suggesting it to preserve the cleanup flag behavior, rather than as a security measure.

[zachmullen]

Oh, right, that makes sense. I'll change to set o+w.

[manthey]

Should this be a+rw rather than o+rw? If the docker container happens to be the same UID or GID as girder_worker and leaves the files unwritable or unreadable, then wouldn't we fail to remove them otherwise?

[zachmullen]

Is that true? I assumed o meant any user, regardless of whether they are the owner or in the owning group

[zachmullen]

(Just tested, turns out you are right! I learned something new today)

[manthey]

I thought so (but I was testing it myself to make sure).

[zachmullen]

@cjh1 this is the branch I mentioned earlier that will conflict with the docker-py transition. Once you create the integration testing for docker mode, a lot of this testing mock stuff should go away since it's mostly testing mocked internal details and is a pain to maintain.

[cjh1]

@zachmullen Working on that at the moment, for now I am adding an additional test, rather the converting over the mocked version.

[manthey]

This works nicely with my docker-in-docker case now.

[zachmullen]

Thanks @manthey