pass: return an error when a cred doesn't exist #221
Conversation
fixes docker#174 Signed-off-by: Nick Santos <nick.santos@docker.com>
| if _, err := os.Stat(path.Join(getPassDir(), PASS_FOLDER, encoded)); err != nil { | ||
| if os.IsNotExist(err) { | ||
| return "", "", nil | ||
| return "", "", credentials.NewErrCredentialsNotFound() | ||
| } | ||
|
|
||
| return "", "", err |
There was a problem hiding this comment.
Looks like this whole bock was explicitly added in #84 (linked to docker/cli#451).
I don't recall the specifics though; this may have been related to other binaries named pass on Linux, which would get called, so the check for that was to see if a credentials store was initialised, and otherwise fall back to regular credentials storage 🤔
There was a problem hiding this comment.
The PR's description is:
It turns out the cred helpers protocol is to return "" and not an error when a credential isn't present, so let's do that.
Is it possible the protocol changed since then?
I have a hard time reconciling that with the test. I didn't write this new test; I copied it from the other helpers:
https://github.com/docker/docker-credential-helpers/blob/master/wincred/wincred_windows_test.go#L238
https://github.com/docker/docker-credential-helpers/blob/master/osxkeychain/osxkeychain_darwin_test.go#L207
which confirm that this is supposed to return an error.
There was a problem hiding this comment.
maybe the API contract changed to return a standardized error instead of an implementation-specific one?
I also considered refactoring the tests into a "API invariants" test suite that we share across all the helpers. happy to do that in this PR (or in a follow-up one)
There was a problem hiding this comment.
I'm not aware of changes in the API, but must really admit that at the time I wasn't closely involved in development of the "credential-helpers".
Thinking out loud here; as that patch was opened related to docker/cli#451, could it be that (as, unlike macOS and Windows) Linux had multiple possible helpers, that it was implemented as a "hack" to;
- check if the
passbackend can be supported (don't error on "missing") - and only fall back to the second alternative (
secretservicehelper) otherwise - and as last resort, fallback to "store inside the config-file"
There was a problem hiding this comment.
i think that's a reasonable hypothesis, but i don't think that's what's happening. The switching code is pretty straightforward. It calls Get() here:
https://github.com/docker/cli/blob/master/cli/config/credentials/native_store.go#L118
and the switch between pass and secretservice is here:
https://github.com/docker/cli/blob/master/cli/config/credentials/default_store_linux.go#L8
And as far as I can tell, pass/pass.go NEVER implemented this protocol correctly. Here's the first commit in August 2017:
1ab1037 (i.e., it doesn't return ErrCredentialsNotFound)
even though the macos credential helper returned this error in at least February 2016:
4b716da
|
Hm... GitHub actions having issues? I see it was triggered, but didn't complete 😞. Let me try the "good'ol" close & reopen. |
There was a problem hiding this comment.
Thanks for also doing a bit of digging! Yes, it appears that this error is not needed for the selection, so I think this would be ok to go in (famous last words).
We should probably do a bit of cleaning up on the cli side as well (was thinking we should reverse the lookup to first check if we have a helper installed, and only then try if the pass binary is found)
Either way; that's in another repository.
We're planning to do a -beta release to test the latest changes in this repository, so let's get this one in, so that we can give it a try 👍
LGTM
|
(Asking @crazy-max to have a last look before we merge; just in case) |
There was a problem hiding this comment.
Yes as @thaJeztah said, lookup should be the other way around to make sure pass folder exists first.
LGTM in the meantime.
fixes #220