Logout rework

.buildkite/pipeline.yml

@@ -6,7 +6,8 @@ steps:
   - label: ":shell: Shellcheck"
     plugins:
       - shellcheck#v1.3.0:
-          files: hooks/**
+          files:
+            - hooks/**
 
   - label: ":sparkles: Lint"
     plugins:

README.md

@@ -6,11 +6,9 @@ A [Buildkite plugin](https://buildkite.com/docs/agent/v3/plugins) to login to do
 
 To avoid leaking your docker password to buildkite.com or anyone with access to build logs, you need to avoid including it in pipeline.yml. This means it needs to be set specifically with an environment variable in an [Agent hook](https://buildkite.com/docs/agent/hooks), or made available from a previous plugin defined on the same step.
 
-### Securing your password between Buildkite Jobs
+### Securing your credentials between Buildkite Jobs
 
-The Elastic CI Stack for AWS automatically creates a [per-job `DOCKER_CONFIG` directory](https://github.com/buildkite/elastic-ci-stack-for-aws/pull/756)
-which is cleaned up after the job. If you are using this plug-in with another agent orchestration
-tool, consider whether you need a per-job `DOCKER_CONFIG` too.
+By default, this plugin creates a step-specific docker config folder that gets removed after it is finished. If you want to disable that behaviour (for example, if you are using the Elastic CI Stack for AWS that already [creates a per-job `DOCKER_CONFIG` directory](https://github.com/buildkite/elastic-ci-stack-for-aws/pull/756)), you may also have to disable the plugin from logging out after it is finished or you may run into race conditions in multi-agent setups.
 
 ## Example
 
@@ -23,30 +21,50 @@ export MY_DOCKER_LOGIN_PASSWORD=mysecretpassword
 steps:
   - command: ./run_build.sh
     plugins:
-      - docker-login#v2.1.0:
+      - docker-login#v3.0.0:
           username: myuser
           password-env: MY_DOCKER_LOGIN_PASSWORD
 ```
 
-## Options
+## Configurations
 
-### `username`
+### Required
+
+There is a single option required to configure and use this plugin.
+
+#### `username`
 
 The username to send to the docker registry.
 
-### `server` (optional)
+### Optional
 
-The server to log in to, if blank or ommitted logs into Docker Hub.
+#### `disable-logout` (boolean, insecure)
+
+Set to `true` if you don't want the plugin to execute a `docker logout` when the step is finished.
+
+Note that doing so **may be a security risk** allowing interaction with a private repository to any script or person with access to the agent if the local docker daemon saves credentials in a local unencrypted store they have access to (as is the default behaviour). If `isolate-config` is set to `true` as well, this would leave temporary files with docker credentials around after the step is finished (and a warning will be printed).
+
+#### `isolate-config` (boolean, insecure)
+
+Set to `false` if you don't want the plugin to create a special docker configuration for the step.
+
+Note that doing so may be a security risk unless you are doing something similar outside of this plugin due to the login credentials being a shared state among all agents and jobs run by them with access to the same docker daemon. This can cause jobs to fail intermmittently or even allow access to private repositories to jobs that shouldn't have such access.
+
+It is safe to disable if you are using the [Elastic CI Stack for AWS 5.0.0 or later](https://github.com/buildkite/elastic-ci-stack-for-aws).
 
 ### `password-env`
 
 The environment variable that the password is stored in.
 
 Defaults to `DOCKER_LOGIN_PASSWORD`.
 
-### `retries` (optional)
+### `retries`
 
-Retries login after a delay N times. Defaults to 0.
+Retries login after a delay N times. Defaults to 0 (no retries).
+
+### `server`
+
+The server to log in to, if blank or ommitted logs into Docker Hub.
 
 ## Windows Issues
 

hooks/environment

@@ -0,0 +1,10 @@
+#!/bin/bash
+
+set -eu -o pipefail
+
+# a clean docker config for each job, for improved isolation
+if [ "${BUILDKITE_PLUGIN_DOCKER_LOGIN_ISOLATE_CONFIG:-true}" = "true" ]; then
+  BUILDKITE_DOCKER_LOGIN_TEMP_CONFIG=$(mktemp -d)
+  export BUILDKITE_DOCKER_LOGIN_TEMP_CONFIG
+  export DOCKER_CONFIG="$BUILDKITE_DOCKER_LOGIN_TEMP_CONFIG"
+fi

hooks/pre-exit

@@ -0,0 +1,22 @@
+#!/bin/bash
+
+set -eu -o pipefail
+
+logout_args=(logout)
+
+if [ -n "${BUILDKITE_PLUGIN_DOCKER_LOGIN_SERVER:-}" ]; then
+  logout_args+=("${BUILDKITE_PLUGIN_DOCKER_LOGIN_SERVER}")
+fi
+
+if [ "${BUILDKITE_PLUGIN_DOCKER_LOGIN_DISABLE_LOGOUT:-false}" = "false" ]; then
+  docker "${logout_args[@]}"
+elif [ "${BUILDKITE_PLUGIN_DOCKER_LOGIN_ISOLATE_CONFIG:-true}" = "true" ]; then
+  echo 'Note: despite not logging out, temporary docker credential file should be removed'
+fi
+
+if [ "${BUILDKITE_PLUGIN_DOCKER_LOGIN_ISOLATE_CONFIG:-true}" = "true" ] && \
+   [ -d "${BUILDKITE_DOCKER_LOGIN_TEMP_CONFIG}" ]; then
+  rm -rf "${BUILDKITE_DOCKER_LOGIN_TEMP_CONFIG}"
+elif [ "${BUILDKITE_PLUGIN_DOCKER_LOGIN_DISABLE_LOGOUT:-false}" = "true" ]; then
+    echo 'WARNING: leaving docker configuration file with credentials around'
+fi

plugin.yml

@@ -1,19 +1,23 @@
 name: Docker Login
 description: Login to Docker registries in your build steps
-author: https://github.com/lox
+author: https://github.com/buildkite
 requirements:
   - bash
   - docker
 configuration:
   properties:
-    username:
-      type: string
-    server:
-      type: string
+    disable-logout:
+      type: boolean
+    isolate-config:
+      type: boolean
     password-env:
       type: string
     retries:
       type: number
       default: 0
+    server:
+      type: string
+    username:
+      type: string
   required:
     - username

tests/pre-command.bats

@@ -0,0 +1,79 @@
+#!/usr/bin/env bats
+
+load "${BATS_PLUGIN_PATH}/load.bash"
+
+# export DOCKER_STUB_DEBUG=/dev/tty
+
+setup() {
+  export BUILDKITE_PLUGIN_DOCKER_LOGIN_USERNAME="blah"
+  export DOCKER_LOGIN_PASSWORD="llamas"
+}
+
+@test "Missing required parameter fails" {
+  unset BUILDKITE_PLUGIN_DOCKER_LOGIN_USERNAME
+
+  run "$PWD/hooks/pre-command"
+
+  assert_failure
+}
+
+@test "Login to single registry with default password" {
+  stub docker \
+    "login --username blah --password-stdin : echo logging in to docker hub; cat"
+
+  run "${PWD}/hooks/pre-command"
+
+  assert_success
+  assert_output --partial "logging in to docker hub"
+  assert_output --partial "llamas"
+
+  unstub docker
+}
+
+@test "Login to single registry with password-env" {
+  export BUILDKITE_PLUGIN_DOCKER_LOGIN_PASSWORD_ENV="CUSTOM_DOCKER_LOGIN_PASSWORD"
+  export CUSTOM_DOCKER_LOGIN_PASSWORD="llamas2"
+
+  stub docker \
+    "login --username blah --password-stdin : echo logging in to docker hub; cat"
+
+  run "${PWD}/hooks/pre-command"
+
+  assert_success
+  assert_output --partial "logging in to docker hub"
+  assert_output --partial "llamas2"
+
+  unstub docker
+}
+
+@test "Multiple retries not used if first command is successful" {
+  export BUILDKITE_PLUGIN_DOCKER_LOGIN_RETRIES=5
+
+  stub docker \
+    "login --username blah --password-stdin : echo logging in to docker hub; cat"
+
+  run "${PWD}/hooks/pre-command"
+
+  assert_success
+  assert_output --partial "logging in to docker hub"
+  assert_output --partial "llamas"
+
+  unstub docker
+}
+
+@test "Multiple retries used until command is successful" {
+  export BUILDKITE_PLUGIN_DOCKER_LOGIN_RETRIES=5
+
+  stub docker \
+    "login --username blah --password-stdin : exit 1" \
+    "login --username blah --password-stdin : exit 1" \
+    "login --username blah --password-stdin : echo logging in to docker hub; cat"
+
+  run "${PWD}/hooks/pre-command"
+
+  assert_success
+  assert_output --partial "logging in to docker hub"
+  assert_output --partial "llamas"
+
+  unstub docker
+}

tests/pre-exit.bats

@@ -0,0 +1,84 @@
+#!/usr/bin/env bats
+
+load "${BATS_PLUGIN_PATH}/load.bash"
+
+# export DOCKER_STUB_DEBUG=/dev/tty
+
+setup () {
+    export BUILDKITE_DOCKER_LOGIN_TEMP_CONFIG="${BATS_TEST_TMPDIR}/test-folder"
+}
+
+@test "Clean logout execution" {
+    stub docker \
+      "echo got \$# args \$@"
+
+    run "$PWD/hooks/pre-exit"
+
+    assert_success
+    assert_output --partial 'got 1 args logout'
+    refute_output --partial "removing ${BUILDKITE_DOCKER_LOGIN_TEMP_CONFIG}"
+
+    unstub docker
+}
+
+@test "Server can be set" {
+    export BUILDKITE_PLUGIN_DOCKER_LOGIN_SERVER='my-server'
+
+    stub docker \
+      "echo got \$# args \$@"
+
+    run "$PWD/hooks/pre-exit"
+
+    assert_success
+    assert_output --partial 'got 2 args logout my-server'
+
+    unstub docker
+}
+
+@test "Docker config is removed if it exists" {
+    mkdir "${BUILDKITE_DOCKER_LOGIN_TEMP_CONFIG}"
+
+    stub docker \
+      "echo got \$# args \$@"
+
+    stub rm \
+      "echo removing \$2"
+
+    run "$PWD/hooks/pre-exit"
+
+    assert_success
+    assert_output --partial 'got 1 args logout'
+    assert_output --partial "removing ${BUILDKITE_DOCKER_LOGIN_TEMP_CONFIG}"
+
+    unstub docker
+    unstub rm
+}
+
+@test "When disable logout is turned on, file is not removed" {
+    mkdir "${BUILDKITE_DOCKER_LOGIN_TEMP_CONFIG}"
+    export BUILDKITE_PLUGIN_DOCKER_LOGIN_DISABLE_LOGOUT='true'
+
+    run "$PWD/hooks/pre-exit"
+
+    assert_success
+
+    # ensure folder is removed anyways
+    ! [ -e "${BUILDKITE_DOCKER_LOGIN_TEMP_CONFIG}" ]
+    refute_output --partial 'WARNING: leaving docker configuration file with credentials around'
+    assert_output --partial 'Note: despite not logging out, temporary docker credential file should be removed'
+}
+
+@test "Can prevent from file to be removed" {
+    mkdir "${BUILDKITE_DOCKER_LOGIN_TEMP_CONFIG}"
+    export BUILDKITE_PLUGIN_DOCKER_LOGIN_ISOLATE_CONFIG='false'
+    export BUILDKITE_PLUGIN_DOCKER_LOGIN_DISABLE_LOGOUT='true'
+
+    run "$PWD/hooks/pre-exit"
+
+    assert_success
+
+    assert_output --partial 'WARNING: leaving docker configuration file with credentials around'
+    [ -d "${BUILDKITE_DOCKER_LOGIN_TEMP_CONFIG}" ] # ensure folder still exists
+
+    refute_output --partial 'Note: despite not logging out, temporary docker credential file should be removed'
+}