Cli: any "pass" command in $PATH is executed when running docker-cli

[eipiminus1]

Description

Whenever I run any docker command from the terminal, the pass command is executed. In my case I had a bash script that opens keepass in a folder I added to my $PATH variable.

Steps to reproduce the issue:
I could not yet reproduce the behaviour on a debian or ubuntu server. So far I only noticed this on my machine.

1. Put a bash script named "pass" with touch /tmp/IWASHERE in a folder in you $PATH
2. make the file executable
3. run docker ps or any similar command
4. find a file IWASHERE in /tmp/

Describe the results you received:

any pass command in $PATH is executed

Describe the results you expected:

I did not expect this kind of side effect when running docker-cli

Additional information you deem important (e.g. issue happens only occasionally):

So far I could only reproduce this issue on my personal machine (archlinux) not on an ubuntu or debian server.

Output of docker version:

Client:
 Version:      17.10.0-ce
 API version:  1.33
 Go version:   go1.9.1
 Git commit:   f4ffd2511c
 Built:        Wed Oct 18 23:08:56 2017
 OS/Arch:      linux/amd64

Server:
 Version:      17.10.0-ce
 API version:  1.33 (minimum version 1.12)
 Go version:   go1.9.1
 Git commit:   f4ffd2511c
 Built:        Wed Oct 18 23:09:11 2017
 OS/Arch:      linux/amd64
 Experimental: false

Output of docker info:

Containers: 48
 Running: 5
 Paused: 0
 Stopped: 43
Images: 1061
Server Version: 17.10.0-ce
Storage Driver: overlay2
 Backing Filesystem: extfs
 Supports d_type: true
 Native Overlay Diff: false
Logging Driver: json-file
Cgroup Driver: cgroupfs
Plugins:
 Volume: local
 Network: bridge host macvlan null overlay
 Log: awslogs fluentd gcplogs gelf journald json-file logentries splunk syslog
Swarm: inactive
Runtimes: runc
Default Runtime: runc
Init Binary: docker-init
containerd version: 06b9cb35161009dcb7123345749fef02f7cea8e0
runc version: 0351df1c5a66838d0c392b4ac4cf9450de844e2d
init version: 949e6fa
Security Options:
 seccomp
  Profile: default
Kernel Version: 4.13.8-1-ARCH
Operating System: Arch Linux
OSType: linux
Architecture: x86_64
CPUs: 4
Total Memory: 15.12GiB
Name: ***
ID: ***
Docker Root Dir: /var/lib/docker
Debug Mode (client): false
Debug Mode (server): false
Registry: https://index.docker.io/v1/
Experimental: false
Insecure Registries:
 127.0.0.0/8
Live Restore Enabled: false

Additional environment details (AWS, VirtualBox, physical, etc.):

[eipiminus1]

Only hints I could find by searching through the code so far where here and here

[thaJeztah]

Yes, looks like this is to detect if the pass utility is present on the system (see pass/pass_linux.go#L49; looks like your custom script is using the same name as that utility.

@tych0 @n4ss should we detect if ~/.password-store (and/or docker-credential-helpers/docker-pass-initialized-check) is present first? Looks like it's currently checking irregardless of the previous state; https://github.com/docker/docker-credential-helpers/blob/master/pass/pass_linux.go#L35-L45

[thaJeztah]

But, I guess this would be similar to having a custom script named bash in your $PATH

[eipiminus1]

@thaJeztah I agree. I was not aware of the pass password manager when I created my script. I was pretty confused when keepass kept popping up for no obvious reason but I guess it was a combination of coincidences that made it hard to trace for me. Thanks for the clarification.

[attie]

This has just bitten me too, though my experience was that any docker command was taking _forever_ to execute... looking at htop showed that there was a fork-bomb in process.

In my case pass is in my $PATH, and is actually a little wrapper / shim script - because my real pass (I'm actually using password-store, as per this driver) is inside a docker container, and is accessed by running something akin to:

docker exec -it -u pass ${CONTAINER} pass "${@}"

... running docker calls pass, which calls on docker which trys to run pass, and on we go...

For now I've added the following to my pass shim / script:

[ "$(ps --no-headers -p $PPID -o comm)" == "docker" ] && exit 0

---

I'd like to point out that this init function is crazy...!

My password-store / GPG key is secured with a passphrase, and I don't use an agent meaning that if I were on this version, and had pass installed "normally", then I'd need to enter the GPG key on _every_ invocation of docker, so that it can insert, check and remove a password... what?? You're planning to call pass three times just to check it works, one instance of which will require the user to enter a passphrase.

I'm actually running the pre-c2eec53 version, which is only slightly better, but even so in "normal" use it would list off the names of all my stored credentials - the default behaviour of running pass with no arguments. This is unnecessary, and potentially concerning!

My suggestions:

• Integration with pass / password-store should be an "_enable it if you want it_" feature
• This negates the need to "_check that it works properly_"
• If the user has explicitly enabled it, then the init() function could do either:
  
  
  
  1. nothing - let the errors occur later if / when you need to call on pass
  
  
  2. a simple check that pass is in the $PATH and is executable... if not, then throw a warning and disable itself
  
  

[cpuguy83]

Agreed that init should not be there.

[thaJeztah]

ping @n4ss could you have a look at this? Also see my earlier comment

[n4ss]

1)
I'll fix the binary path & password-db file checks as well as the test-password check. The original idea from Tycho was that the user can have pass anywhere iirc but this needs to be fixed, maybe also checking the format of pass version

2)

Integration with pass / password-store should be an "enable it if you want it" feature

No, we want credential stores by default. Most users don't dig into security options to enable them, they just use what's available by default 95% of the time. We moved the credentials from the raw config.json to credential stores for obvious security reasons.

We shouldn't compromise on usability regardless so let's try to come up with a better solution in term of UX instead.

---

For the init() function I admit that it's far from optimal, I think that checking ~/.password-store would actually prevent it from doing the silly-but-necessary check more than once.

[attie]

@n4ss fair enough, thanks for pushing back re credential stores and security concerns - I understand.
I have a non-standard setup, so I'm happy to keep my fix in place, but I still think things can be improved.

The password store's location is configured by the following line (bash / here)... this may lead to an unfortunate disconnect if they change their approach, but alas.

PREFIX="${PASSWORD_STORE_DIR:-$HOME/.password-store}"

For init(), would the following approach be acceptable? (please forgive my Go and facetious comments)

func init() {
    // In principle, we could just run `pass init`. However, pass has a bug
    // where if gpg fails, it doesn't always exit 1. Additionally, pass
    // uses gpg2, but gpg is the default, which may be confusing.
    // So let's explictily check that pass actually can store and retreive a
    // password, and remember the fact
    password := "pass is initialized"
    name := path.Join(PASS_FOLDER, "docker-pass-initialized-check")

    // figure out where the password store is kept
    pass_prefix, ok := os.LookupEnv("PASSWORD_STORE_DIR")
    if !ok {
        user, err := user.Current()
        if err != nil {
            panic(err)
        }

        pass_prefix = path.Join(user.HomeDir, ".password-store")
    }

    // check that the password store exists
    if _, err := os.Stat(pass_prefix); err != nil {
        // oh no, password-store isn't set up
        return;
    }

    // check if we've already proven that it works
    if _, err := os.Stat(path.Join(pass_prefix, name)); err == nil {
        PassInitialized = true
        return;
    }

    // warn the user what's about to happen, in case it affects them
    fmt.Println("Dear user, we're just about to check that password-store is operational")
    fmt.Println("Please be aware that you may be asked to provide your GPG passphrase...")
    fmt.Println("This is innocent, I promise...")

    // check that the password store works by inserting and retrieving a dummy credential
    _, err := runPass(password, "insert", "-f", "-m", name)
    if err != nil {
        return
    }

    stored, err := runPass("", "show", name)
    PassInitialized = err == nil && stored == password
}

[thaJeztah]

I don't think this all needs to be in init(). Without having dived into the whole logic, I'm thinking;

• don't perform detection until credentials are needed (docker login, docker logout, docker pull <some private image>)
• detection should already take into account what's stored in ~/.docker/config.json (i.e., if I explicitly set to use "insecure" local credentials, the CLI should respect that
• the credentials-store and credentials-helpers configured in ~/.docker/config.json should (besides just using $PATH as a default), allow setting the full-path to a binary (~/foobar/my-custom-credentials-helper)
• the configuration should allow options for credentials-helpers and config-store.

[onlyjob]

This issue seems relevant: https://github.com/docker/docker-credential-helpers/issues/96