Skip to content
This repository has been archived by the owner on Feb 27, 2020. It is now read-only.

Run tasks with their own user context. #77

Merged
merged 2 commits into from Aug 2, 2016
Merged

Conversation

walac
Copy link
Contributor

@walac walac commented Jun 1, 2016

To run tasks concurrently in the same environment and make sure they don't
mess up with each other, we run each of them with a different user, which
also prevents tasks from doing fancy things to get secret information.

For each new task, we create a new user on the fly, associate it to the
staff group member and create a home directory. The task is triggered
from user's home directory. After task is done, we remove the user as
well as its home directory.

In the case we fail to create a new user, we log the error but don't
fail the task. This behavior is to let the tests pass even when
running without capability to create users.

@coveralls
Copy link

Coverage Status

Coverage decreased (-4.7%) to 72.186% when pulling e872209 on walac:macosx into a08a481 on taskcluster:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-4.6%) to 72.222% when pulling bc22b89 on walac:macosx into a08a481 on taskcluster:master.

@walac walac changed the title [Do not merge yet] Run tasks with their own user context. Run tasks with their own user context. Jun 24, 2016
@coveralls
Copy link

coveralls commented Jun 24, 2016

Coverage Status

Coverage decreased (-4.3%) to 73.416% when pulling 4778003 on walac:macosx into 8a0ef9a on taskcluster:master.

// in in a development environment, so do not fail the task to tests
// run successfully.
u := user{}
if err = u.create(); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I think that this can be rather dangerous. If for some reason creating a user fails, this could be run under escalated permissions without knowing it.

I think it should be an either/or thing. Either the engine is configured to run a process under a temp user and fails if that user can't be used, or the engine is configured to not create a temp user and run under the worker user.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about it a little more, i wonder if it would be possible to have the engine configuration option to either use a module for creating the user, or using the user that the engine is running under. If that config setting says to create a user using some other module, then this .create() stuff can be called and the user returned from that used for the process. Also would allow someone to specify different creation scripts depending on need. Like if there are some special groups that the user should be part of on one type of worker type but not another.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we talked earlier, I implemented a environment variable based configuration. If TASKCLUSTER_WORKER_ENV=Production (non case sensitive), the task fails if it can't create the user.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer that we used a configuration file exclusively, rather than a combination of env vars and a config file. This also has the advantage that we will have json schema for the config file, so it can be validated reasonably easily, and we'll detect problems on startup, rather than at task execution time.

@coveralls
Copy link

coveralls commented Jun 27, 2016

Coverage Status

Coverage decreased (-0.6%) to 77.137% when pulling ac9193f on walac:macosx into 8a0ef9a on taskcluster:master.

@coveralls
Copy link

coveralls commented Jun 29, 2016

Coverage Status

Changes Unknown when pulling aa3f920 on walac:macosx into * on taskcluster:master*.

@coveralls
Copy link

coveralls commented Jun 30, 2016

Coverage Status

Coverage decreased (-0.6%) to 77.137% when pulling 4843f14 on walac:macosx into 8b7a43e on taskcluster:master.

@coveralls
Copy link

coveralls commented Jul 5, 2016

Coverage Status

Coverage decreased (-0.6%) to 77.137% when pulling b0f416f on walac:macosx into 8b7a43e on taskcluster:master.

@walac walac mentioned this pull request Jul 7, 2016
Merged
@walac
Copy link
Contributor Author

walac commented Jul 14, 2016

@gregarndt may I merge this? (well, once I rebase the code)

@gregarndt
Copy link
Collaborator

do eeet

To run tasks concurrently in the same environment and make sure they don't
mess up with each other, we run each of them with a different user, which
also prevents tasks from doing fancy things to get secret information.

For each new task, we create a new user on the fly, associate it to the
staff group member and create a home directory. The task is triggered
from user's home directory. After task is done, we remove the user as
well as its home directory.

In the case we fail to create a new user, we log the error but don't
fail the task. This behavior is to let the tests pass even when
running without capability to create users.
@coveralls
Copy link

coveralls commented Aug 1, 2016

Coverage Status

Changes Unknown when pulling d3c67e0 on walac:macosx into * on taskcluster:master*.

Some language features may only be implemented in some Operating
Systems, making builds fail on other OSes. We make the osxnative engine
only builds on darwin kernel, where it is the only relevant OS for this
engine.
@coveralls
Copy link

coveralls commented Aug 1, 2016

Coverage Status

Changes Unknown when pulling f1a722c on walac:macosx into * on taskcluster:master*.

@walac
Copy link
Contributor Author

walac commented Aug 2, 2016

I rebased the branch, but for some reason travis doesn't understand that, going to merge it as fails are unrelated to this patch.

@walac walac merged commit c52a836 into taskcluster:master Aug 2, 2016
jonasfj pushed a commit that referenced this pull request Aug 22, 2016
Run tasks with their own user context.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants