Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Docker plugin #386

Closed
wants to merge 7 commits into from
Closed

Add Docker plugin #386

wants to merge 7 commits into from

Conversation

dethancosta
Copy link
Contributor

Overview

Adds a plugin to authenticate the Docker CLI using a username and secret (a password or an access token).

Type of change

  • Created a new plugin
  • Improved an existing plugin
  • Fixed a bug in an existing plugin
  • Improved contributor utilities or experience

Related Issue(s)

How To Test

docker login

Changelog

Authenticate to a Docker registry using Touch ID and other unlock options with 1Password Shell Plugins.

@dethancosta
Copy link
Contributor Author

Note that from the Docker binary's perspective, the password is being entered in plaintext on the command line, so it will emit a warning.
The Docker binary provides the --password-stdin flag for passing in the password to stdin, but I'm not sure that this is possible from within the provisioner code.
Another method that Docker provides is through a credential helper, which would require a rewrite of op-cli to add the necessary subcommands.

@accraw
Copy link
Contributor

accraw commented Oct 13, 2023

Thank you very much for your contribution!

It doesn't look like this approach will work consistently. We like the third option you suggested, and have even sketched out a solution in this comment: #301 (comment)

I don't think this will require work on the cli side, it should be possible with just shell-plugins, but that's something that would be figured out once someone really starts to dig into this. If you don't want to take this on, it's on our radar to add but not in the immediate future, in case that influences your decision.

@dethancosta
Copy link
Contributor Author

Ah, apologies, I didn't realize there was a previous PR with the same approach. I'll take a crack at the sketched out solution possibly after the weekend, if that's alright.

@accraw
Copy link
Contributor

accraw commented Oct 13, 2023

Absolutely, that would be awesome!

@accraw accraw added the in-progress this PR is being worked on/comments are in the process of being addressed by the contributor label Oct 13, 2023
@dethancosta
Copy link
Contributor Author

Alright, I think I've implemented all the credential helper functionality, but with a few issues/caveats that I'm not sure how to resolve:

  • I had to use manual creation of the config file and executable rather than the provision sub-package's Add*File, otherwise the docker binary hangs. Because of this, I couldn't include the files in the expected sections of the test file. This also means that the config file needs to be deleted or temporarily removed prior to the provisioner running.
  • If os.MkDirAll isn't called, an error is returned that ~/.docker/config.json is a non-existent file or directory.
  • For some reason, the provisioner isn't triggered when the plugin condition needsAuth.ForCommand("login","push","pull") is present, even though 'docker login' is being called
  • the executable emits error unsupported operation as docker tries to save the credentials; I'm unsure of what the desired behaviour would be for this, it could either be changed to not emit anything and return exit code 0, or to emit a message to the user that the credentials don't need to be saved (or whatever message is appropriate)

This PR likely isn't mergeable, but I figured I'd at least document the issues I ran into attempting this approach.

@accraw
Copy link
Contributor

accraw commented Nov 14, 2023

Thank you so much for taking the time to try this out! You're making a compelling case for docker to be added as a plugin internally, for now I'm going to close this PR and anyone who picks it up can reference it.

@accraw accraw closed this Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-progress this PR is being worked on/comments are in the process of being addressed by the contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New plugin: Docker
2 participants