Prefer BUILDKITE_BUILD_CHECKOUT_PATH when fixing permissions

[jamison-lahman-ai]

https://github.com/buildkite/elastic-ci-stack-for-aws/blob/f8ce90fae0399180493236e12683bb51102538bc/packer/linux/conf/buildkite-agent/scripts/fix-buildkite-agent-builds-permissions#L76-L81

Buildkite supplies a BUILDKITE_BUILD_CHECKOUT_PATH which makes this path creation unnecessary which is also less extensible given this path can be changed in the environment hook.

[keithduncan]

Hi @jl-applied, thank you for taking the time to open this issue 🙌

I don’t think this is something we’d look to change, nor would we look to support a change-able BUILDKITE_BUILD_CHECKOUT_PATH. The script goes to great pains to ensure it isn’t being used as a vector in a privilege escalation attack to take ownership of an arbitrary path.

Could you share some more details on a use case for this for us to look into?

[jamison-lahman-ai]

Hi,

Thanks for the response. This doesn't break any flows, but was looking into extending this flow. Use-case is mostly covered in https://forum.buildkite.community/t/easily-configure-one-repository-per-agent/1694. (I think biggest reason to properly resolve that ticket is to make this BUILDKITE_BUILD_CHECKOUT_PATH immutable and therefore more useful for cases like here.)

My intention was to decompose this BUILDKITE_BUILD_CHECKOUT_PATH rather than try to compose and guess the actual path. Something like,

FOUND_BUILDS=0
REMAINING_PATH="$BUILDKITE_BUILD_CHECKOUT_PATH"
while [ "$FOUND_BUILDS" == "0" ]; do
  if [ -z "$REMAINING_PATH" ]; then
    echo "No more remaining path and unable to find BUILDKITE_BUILD_PATH on BUILDKITE_BUILD_CHECKOUT_PATH"
    exit 4
  fi
  DIR=$(basename "$REMAINING_PATH")
  
  exit_if_contains_slashes "$DIR"
  exit_if_contains_traversal "$DIR"
  exit_if_blank "$DIR"
  REMAINING_PATH=$(dirname "$REMAINING_PATH")
  if [ "$REMAINING_PATH" == "$BUILDKITE_BUILD_PATH" ]; then
    FOUND_BUILDS=1
  fi
done

Alternatively, this BUILDKITE_BUILD_PATH is only mutable at agent start; so do you think this could just become sudo chown -R buildkite-agent:buildkite-agent "$BUILDKITE_BUILD_PATH"?

[keithduncan]

While I noodle on the best way to support this, does BuildkiteAdditionalSudoPermissions help unblock you by allowing additional sudo permissions outside the one included for this command with strict path checking?

[jamison-lahman-ai]

It would be sufficient for my use-case to simply omit any values that are currently appended to the PIPELINE_PATH if they don't exists on this BUILDKITE_BUILDS_CHECKOUT_PATH. That way, we should only ever use a subset of values which are currently considered as secure.

Let me know if that would still be undesirable otherwise should be able to get you a pull request soon.

[jamison-lahman-ai]

Sad. Now looking at it, seems even worse either of the other options, https://github.com/jmelahman/elastic-ci-stack-for-aws/pull/1/files.