Skip to content

Commit

Permalink
Only fix permissions of the pipeline's build directory
Browse files Browse the repository at this point in the history
Currently the fix-buildkite-agent-builds-permissions script, that's run by our
environment hook, chown's the agent’s entire builds directory on every job run.
This means the chown operation gets slower every time a different pipeline is
checked out onto disk.

This changes the permissions fixing script to chown only the running
pipeline’s checkout dir (if it exists), rather than the entire builds dir. The
permissions tests have also been moved from the comment in the bottom of the
file into their own executable .bats tests.
  • Loading branch information
toolmantim committed Sep 8, 2017
1 parent dc335f2 commit 5428dc8
Show file tree
Hide file tree
Showing 5 changed files with 168 additions and 56 deletions.
9 changes: 9 additions & 0 deletions .buildkite/pipeline.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,15 @@ steps:
agents:
queue: "${BUILDKITE_AGENT_META_DATA_QUEUE}"

- name: ":bash: Unit tests"
command: .buildkite/steps/unit-tests.sh
agents:
queue: "${BUILDKITE_AGENT_META_DATA_QUEUE}"
plugins:
docker-compose#v1.5.2:
run: unit-tests
config: docker-compose.unit-tests.yml

- wait
- name: ":packer: Build"
command: .buildkite/steps/packer.sh
Expand Down
9 changes: 9 additions & 0 deletions docker-compose.unit-tests.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
version: '3'

services:
unit-tests:
image: lucor/bats
volumes:
- .:/src:ro
working_dir: /src
command: bats /src/unit-tests/
14 changes: 9 additions & 5 deletions packer/conf/buildkite-agent/hooks/environment
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,9 @@ if [[ -n "${BUILDKITE_SECRETS_BUCKET:-}" && "${SECRETS_PLUGIN_ENABLED:-}" == "1
source /usr/local/buildkite-aws-stack/plugins/secrets/hooks/environment
fi

# We need to scope the next bit to only the currently running agent's builds,
# but we also need to control security and make sure arbitrary folders can't be
# chmoded.
# We need to scope the next bit to only the currently running agent dir and
# pipeline, but we also need to control security and make sure arbitrary folders
# can't be chmoded.
#
# The agent builds path isn't exposed nicely by itself. The agent name also
# doesn't quite map to its builds path. We do have a complete checkout path,
Expand All @@ -64,7 +64,11 @@ BUILD_PATH_SUFFIX="${BUILDKITE_BUILD_CHECKOUT_PATH#${BUILDKITE_BUILD_PATH}/}"
AGENT_BUILD_NAME="${BUILD_PATH_SUFFIX%%/*}"
# => "my-agent-1"

# Then we can figure out the pipeline path component
PIPELINE_DIR_NAME="${BUILD_PATH_SUFFIX#${AGENT_BUILD_NAME}/}"
# => "my-pipeline-blah"

# Now we can pass this to the sudo script which will validate it before safely chmodding:
echo "Fixing permissions for '${AGENT_BUILD_NAME}'..."
sudo /usr/bin/fix-buildkite-agent-builds-permissions "${AGENT_BUILD_NAME}"
echo "Fixing permissions for '${AGENT_BUILD_NAME}/${PIPELINE_DIR_NAME}'..."
sudo /usr/bin/fix-buildkite-agent-builds-permissions "${AGENT_BUILD_NAME}" "${PIPELINE_DIR_NAME}"
echo
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
#!/bin/bash

# To run the unit tests for this file, run the following command in the root of
# the project:
# $ docker-compose -f docker-compose.unit-tests.yml run unit-tests

# Files that are created by Docker containers end up with strange user and
# group ids, usually 0 (root). Docker namespacing will one day save us, but it
# can only map a single docker user id to a given user id (not any docker user
Expand All @@ -12,70 +16,63 @@

set -eu -o pipefail

# We need to scope this to only the currently running agent's builds, but we
# also need to control security and make sure arbitrary folders can't be
# chmoded.
# We need to scope the next bit to only the currently running agent dir and
# pipeline, but we also need to control security and make sure arbitrary folders
# can't be chmoded.
#
# We prepare the agent build directory basename in the environment hook and
# pass it as the first argument. In here we just need to check that it contains
# no slashes and isn't a traversal component.
# pass it as the first argument, and the pipeline dir as the second argument.
#
# In here we need to check that they both don't contain slashes or contain a
# traversal component.

AGENT_BUILDS_NAME="$1"
# => "my-agent-1"

PIPELINE_DIR_NAME="$2"
# => "my-pipeline-blah"

# Make sure it doesn't contain any slashes by substituting slashes with nothing
# and making sure it doesn't change
if [[ "${AGENT_BUILDS_NAME//\//}" != "${AGENT_BUILDS_NAME}" ]]; then
exit 1
fi
function exit_if_contains_slashes() {
if [[ "${1//\//}" != "${1}" ]]; then
exit 1
fi
}

# Now we know this name has only non-slash characters.
#
# We just need to check that it's not "." or ".." to prevent traversal:
if [[ "${AGENT_BUILDS_NAME}" == "." || "${AGENT_BUILDS_NAME}" == ".." ]]; then
exit 2
fi
function exit_if_contains_traversal() {
if [[ "${1}" == "." || "${1}" == ".." ]]; then
exit 2
fi
}

# We also need to make sure it's not empty so we don't collide with other agents:
if [[ -z "${AGENT_BUILDS_NAME}" ]]; then
exit 3
fi
function exit_if_blank() {
if [[ -z "${1}" ]]; then
exit 3
fi
}

# Check both the args for slashes
exit_if_contains_slashes "${AGENT_BUILDS_NAME}"
exit_if_contains_slashes "${PIPELINE_DIR_NAME}"

# Check both the args for traversals
exit_if_contains_traversal "${AGENT_BUILDS_NAME}"
exit_if_contains_traversal "${PIPELINE_DIR_NAME}"

# Check both the args for blank vaues
exit_if_blank "${AGENT_BUILDS_NAME}"
exit_if_blank "${PIPELINE_DIR_NAME}"

# If we make it here, we're safe to go!

# We know the builds path:
BUILDS_PATH="/var/lib/buildkite-agent/builds"

# And now we can reconstruct the full agent builds path:
AGENT_BUILDS_PATH="${BUILDS_PATH}/${AGENT_BUILDS_NAME}"
# => "/var/lib/buildkite-agent/builds/my-agent-1"
PIPELINE_PATH="${BUILDS_PATH}/${AGENT_BUILDS_NAME}/${PIPELINE_DIR_NAME}"
# => "/var/lib/buildkite-agent/builds/my-agent-1/my-pipeline-blah"

if [[ -e "${AGENT_BUILDS_PATH}" ]]; then
/bin/chown -R buildkite-agent:buildkite-agent "${AGENT_BUILDS_PATH}"
if [[ -e "${PIPELINE_PATH}" ]]; then
/bin/chown -R buildkite-agent:buildkite-agent "${PIPELINE_PATH}"
fi

# Manual tests (anybody know a good way to test this?):
#
# ./fix-buildkite-agent-builds-permissions "/"
# => exit 1
#
# ./fix-buildkite-agent-builds-permissions "one/"
# => exit 1
#
# ./fix-buildkite-agent-builds-permissions "/two"
# => exit 1
#
# ./fix-buildkite-agent-builds-permissions "one/two"
# => exit 1
#
# ./fix-buildkite-agent-builds-permissions "one/two/three"
# => exit 1
#
# ./fix-buildkite-agent-builds-permissions "/two/"
# => exit 1
#
# ./fix-buildkite-agent-builds-permissions "."
# => exit 2
#
# ./fix-buildkite-agent-builds-permissions ".."
# => exit 2
#
# ./fix-buildkite-agent-builds-permissions ""
# => exit 3
93 changes: 93 additions & 0 deletions unit-tests/fix-buildkite-agent-builds-permissions.bats
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
#!/usr/bin/env bats

FIX_PERMISSIONS_SCRIPT="/src/packer/conf/buildkite-agent/scripts/fix-buildkite-agent-builds-permissions"

@test "Slashes in the agent arg cause an exit 1" {
run "$FIX_PERMISSIONS_SCRIPT" "/" "abc"
[ "$status" -eq 1 ]
}

@test "Slashes in the agent arg cause an exit 1" {
run "$FIX_PERMISSIONS_SCRIPT" "abc/" "abc"
[ "$status" -eq 1 ]
}

@test "Slashes in the agent arg cause an exit 1" {
run "$FIX_PERMISSIONS_SCRIPT" "/abc" "abc"
[ "$status" -eq 1 ]
}

@test "Slashes in the agent arg cause an exit 1" {
run "$FIX_PERMISSIONS_SCRIPT" "abc/def" "abc"
[ "$status" -eq 1 ]
}

@test "Slashes in the agent arg cause an exit 1" {
run "$FIX_PERMISSIONS_SCRIPT" "abc/def/ghi" "abc"
[ "$status" -eq 1 ]
}

@test "Slashes in the agent arg cause an exit 1" {
run "$FIX_PERMISSIONS_SCRIPT" "/abc/" "abc"
[ "$status" -eq 1 ]
}

@test "Slashes in the pipeline arg cause an exit 1" {
run "$FIX_PERMISSIONS_SCRIPT" "abc" "/"
[ "$status" -eq 1 ]
}

@test "Slashes in the pipeline arg cause an exit 1" {
run "$FIX_PERMISSIONS_SCRIPT" "abc/" "abc"
[ "$status" -eq 1 ]
}

@test "Slashes in the pipeline arg cause an exit 1" {
run "$FIX_PERMISSIONS_SCRIPT" "abc" "/abc"
[ "$status" -eq 1 ]
}

@test "Slashes in the pipeline arg cause an exit 1" {
run "$FIX_PERMISSIONS_SCRIPT" "abc" "abc/def"
[ "$status" -eq 1 ]
}

@test "Slashes in the pipeline arg cause an exit 1" {
run "$FIX_PERMISSIONS_SCRIPT" "abc" "abc/def/ghi"
[ "$status" -eq 1 ]
}

@test "Slashes in the pipeline arg cause an exit 1" {
run "$FIX_PERMISSIONS_SCRIPT" "abc" "/abc/"
[ "$status" -eq 1 ]
}

@test "Single dot traversal in the agent arg cause an exit 2" {
run "$FIX_PERMISSIONS_SCRIPT" "." "abc"
[ "$status" -eq 2 ]
}

@test "Double dot traversal in the agent arg cause an exit 2" {
run "$FIX_PERMISSIONS_SCRIPT" ".." "abc"
[ "$status" -eq 2 ]
}

@test "Single dot traversal in the pipeline arg cause an exit 2" {
run "$FIX_PERMISSIONS_SCRIPT" "abc" "."
[ "$status" -eq 2 ]
}

@test "Double dot traversal in the pipeline arg cause an exit 2" {
run "$FIX_PERMISSIONS_SCRIPT" "abc" ".."
[ "$status" -eq 2 ]
}

@test "Blank agent arg cause an exit 3" {
run "$FIX_PERMISSIONS_SCRIPT" "" "abc"
[ "$status" -eq 3 ]
}

@test "Blank pipeline arg cause an exit 3" {
run "$FIX_PERMISSIONS_SCRIPT" "abc" ""
[ "$status" -eq 3 ]
}

0 comments on commit 5428dc8

Please sign in to comment.