Skip to content
This repository has been archived by the owner on Mar 18, 2019. It is now read-only.

Run the buildkite daemon as a non-root user, buildkite-agent #18

Closed
wants to merge 2 commits into from

Conversation

lox
Copy link
Contributor

@lox lox commented Jan 15, 2016

Dropping privileges of the buildkite-agent provides some extra level of protection against third-party code being executed by the agent.

@dkubb
Copy link

dkubb commented Jan 15, 2016

OT but should https://buildkite.com/buildkite/docker-buildkite-agent/ be publicly visible?

@dkubb
Copy link

dkubb commented Jan 15, 2016

@lox did you mean to commit changes to scripts/build.sh in this PR?

Also, as a test I ran scripts/build.sh through http://www.shellcheck.net/ and it flagged several things. Do you want me to report it as a separate issue? (or does it not matter? I'm not sure about your internal coding guidelines)

&& rm -rf /tmp/*
&& rm -rf /tmp/* \
&& adduser -D -g '' buildkite \
&& chown -R buildkite /buildkite
Copy link

Choose a reason for hiding this comment

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

When I run ./scripts/build.sh locally the alpine edge container fails on this line. If I change it to something like && (chown -R buildkite /buildkite || true) the build passes, but I'm pretty sure this isn't what we want.

I assume the /tmp/install_buildkite.sh script should create this directory but for some reason it's not with edge alpine. Is this a correct assumption?

EDIT: after a bit more digging it looks like the edge agent no longer relies on /buildkite existing? Is this true? If that's the case then maybe my fix is warranted until the edge agent become stable?

@dkubb
Copy link

dkubb commented Jan 15, 2016

I found another problem with this docker container. Specifically the buildkite user's .ssh directory is not executable so the buidkite user can't access it to append to ~/.ssh/known_hosts, which it does when pulling down a repo from github, eg:

$ ls -al /home/buildkite/
total 12
drwxr-sr-x    3 buildkit buildkit      4096 Jan 15 19:53 .
drwxr-xr-x    4 root     root          4096 Jan 15 19:53 ..
drw-------    2 buildkit buildkit      4096 Jan 15 19:53 .ssh

It looks like this is cause by the ssh-env-config.sh script setting the directory explicitly to 0600: https://github.com/buildkite/docker-ssh-env-config/blob/master/ssh-env-config.sh#L17

This probably never caused any issues before when running everything as root, but it would affect the buildkite user.

@dkubb
Copy link

dkubb commented Jan 15, 2016

It looks like this is cause by the ssh-env-config.sh script setting the directory explicitly to 0600

I created a pull request to fix this at: buildkite/docker-ssh-env-config#1

@lox
Copy link
Contributor Author

lox commented Jan 18, 2016

Ooops, I didn't mean to commit the build.sh changes, no.

RE: shellcheck, I get those in my IDE, and AFAIK I've deal with any that aren't spurious (like unquoted $@).

@lox lox changed the title This runs the buildkite daemon as a non-root user, buildkite This runs the buildkite daemon as a non-root user, buildkite-agent Jan 22, 2016
@lox
Copy link
Contributor Author

lox commented Jan 22, 2016

Hrm. I wonder how this should work with docker-in-docker, as the buildkite-agent user would need to be in the docker group, which is effectively root. Thoughts @dkubb?

@lox lox changed the title This runs the buildkite daemon as a non-root user, buildkite-agent Run the buildkite daemon as a non-root user, buildkite-agent Jan 22, 2016
@lox
Copy link
Contributor Author

lox commented Jan 22, 2016

This is ready to go pending review @toolmantim

@toolmantim
Copy link
Contributor

👍 Looks good, let's do it. Thanks for figuring out the magic sudo incantations and handling 1.8, 1.9 and DIND!

@lox
Copy link
Contributor Author

lox commented Feb 10, 2016

@dkubb any feedback on this?

@toolmantim
Copy link
Contributor

is this still a thing?

@lox
Copy link
Contributor Author

lox commented May 4, 2016

It is, yeah, I'm a bit nervous about what effect it will have though. I'll rebase and get it perhaps into some experimental images.

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.

3 participants