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 support for docker cli login using Username and Email plugin #301

Closed
wants to merge 5 commits into from

Conversation

itsCheithanya
Copy link
Contributor

Overview

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)

  • Resolves: #
  • Relates: #

How To Test

Install docker cli
run docker login to import username and password of user logins
and run other command like docker search for searching the Docker Hub for images

image

image

Changelog

@AndyTitu AndyTitu added the in-progress this PR is being worked on/comments are in the process of being addressed by the contributor label Jun 22, 2023
@arunsathiya
Copy link
Contributor

Hi @itsCheithanya! 👋🏼 Since it has been a while since the PR was opened, I wanted to reach out and confirm if this PR is nearly ready for review?

If it is, could you add a test config file at plugins/docker/test-fixtures/config.json, and share some overview of what the shell plugin is about, what Docker commands are supported and other specifics/thought process that went into the building of this shell plugin? Those details can go under the "Overview" section of the PR body and will significantly help align our expectations of the shell plugin.

I'd love to see similar details for the other contributions too:

Happy to take a look soon!

Copy link
Member

@hculea hculea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution!

I have one question for now, and there are still some open TODOs in your submission. I'd love if you could have a look over them, before the next review round. 😄

Feel free to tag either me or any of my colleagues for another look. Thanks!

)}
}

var defaultEnvVarMapping = map[string]sdk.FieldName{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not able to find anywhere documentation related to this authentication method for docker against DockerHub (or other platforms).

Can you please provide some documentation on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hculea
Copy link
Member

hculea commented Jun 24, 2023

run docker login to import username and password of user logins

One of the promises that shell plugins offer is that they leave no credential on disk, or outside 1Password. docker login will defy this promise, as it would save the credentials in the keychain/on disk.

I would like to explore alternatives for this workflow, that do not require any credentials to land on disk. For example, I imagine that provisioning a configuration file that contains the credentials in a similar fashion that docker login would save them could do the trick! Let me know if you'd like any guidance to look into that, we're around to help 😄

@itsCheithanya
Copy link
Contributor Author

run docker login to import username and password of user logins

One of the promises that shell plugins offer is that they leave no credential on disk, or outside 1Password. docker login will defy this promise, as it would save the credentials in the keychain/on disk.

I would like to explore alternatives for this workflow, that do not require any credentials to land on disk. For example, I imagine that provisioning a configuration file that contains the credentials in a similar fashion that docker login would save them could do the trick! Let me know if you'd like any guidance to look into that, we're around to help smile

Yess that would be great,I would require guidance for that

@hculea
Copy link
Member

hculea commented Jun 26, 2023

Happy to offer some tips.

From what I can see, docker relies on a config-file based authentication, when credential stores are not used.

That is, the plugin would need to provision a file with this format:

{
	"auths": {
		"<host>": {
			"auth": "<username:password-base64-encoded>"
		}
	}
}

and add --config <path-to-above-file> to the docker pull or docker push commands. (there may be others as well)

You can do this using our helper provisioner functions, such as:

DefaultProvisioner: provision.TempFile(dockerConfig, provision.AddArgs("--config={{ .Path }}")),

Have a look at the mysql plugin for a similar provisioning mechanism.

Let me know if this helps, would love to chat more if you have other questions!

@AndyTitu AndyTitu added the hashnode hackathon Ideas and inspiration for the hackathon running from June 1st - June 30th label Jun 26, 2023
@itsCheithanya
Copy link
Contributor Author

Happy to offer some tips.

From what I can see, docker relies on a config-file based authentication, when credential stores are not used.

That is, the plugin would need to provision a file with this format:

{
	"auths": {
		"<host>": {
			"auth": "<username:password-base64-encoded>"
		}
	}
}

and add --config <path-to-above-file> to the docker pull or docker push commands. (there may be others as well)

You can do this using our helper provisioner functions, such as:

DefaultProvisioner: provision.TempFile(dockerConfig, provision.AddArgs("--config={{ .Path }}")),

Have a look at the mysql plugin for a similar provisioning mechanism.

Let me know if this helps, would love to chat more if you have other questions!

Heyy yess thankyouu for that ,yes will look into mysql plugin for a similar provisioning mechanism.

@AndyTitu
Copy link
Contributor

AndyTitu commented Jun 27, 2023

From a first round of local testing, I have reasons to believe that our current FIFO approach might not work for the way Docker interacts with this config file. Could be wrong though.

@itsCheithanya
Copy link
Contributor Author

itsCheithanya commented Jun 27, 2023

From a first round of local testing, I have reasons to believe that our current FIFO approach might not work for the way Docker interacts with this config file. Could be wrong though.

Yeah so how do think we should take this forward?
I found that docker uses https://github.com/docker/docker-credential-helpers method to safely store in platform keystores

@hculea
Copy link
Member

hculea commented Jun 28, 2023

I hopped on a call with Andi to try to figure this out - looks to be a discrepancy between our local setup.
While on my machine it works as expected, we weren't able to trigger the same faulty behaviour on mine.
We'll investigate more today, but for the time being, the alternative that I proposed should work, with a one caveat: it looks like docker doesn't like to read config.json from an alternative folder, so we should write the file using the provision.AtFixedPath option.

@itsCheithanya
Copy link
Contributor Author

I hopped on a call with Andi to try to figure this out - looks to be a discrepancy between our local setup. While on my machine it works as expected, we weren't able to trigger the same faulty behaviour on mine. We'll investigate more today, but for the time being, the alternative that I proposed should work, with a one caveat: it looks like docker doesn't like to read config.json from an alternative folder, so we should write the file using the provision.AtFixedPath option.

I see,I tried using tempFile but its giving out errors
image

@hculea
Copy link
Member

hculea commented Jun 28, 2023

This happens because the --config flag is registered under the root command, but not the subcommands.

So the correct syntax would be docker --config <path> <subcommand> not docker <subcommand> --config <path>. While this can be achieved with functionality currently getting added (#276), this should no longer be necessary:

DefaultProvisioner: provision.TempFile(dockerConfig, provision.AtFixedPath("~/.docker/config.json")),

should do the trick.

@arunsathiya arunsathiya mentioned this pull request Jun 28, 2023
4 tasks
@AndyTitu
Copy link
Contributor

@itsCheithanya let us know if you're getting stuck anywhere. We know temp file provisioning has been historically tricky to get working.

@itsCheithanya
Copy link
Contributor Author

itsCheithanya commented Jun 29, 2023

@itsCheithanya let us know if you're getting stuck anywhere. We know temp file provisioning has been historically tricky to get working.

Yeah so I was wondering how to write test case for func TestUserLoginProvisioner(t *testing.T) {} , since we are provisioning using tempFile
image
image

@hculea
Copy link
Member

hculea commented Jun 29, 2023

Have a look at the gitea or akamai test suites, they do a similar thing.

@itsCheithanya
Copy link
Contributor Author

Have a look at the gitea or akamai test suites, they do a similar thing.

Okayy thankyouu the test cases have been passed
image

@itsCheithanya
Copy link
Contributor Author

@hculea @AndyTitu @arunsathiya changes done and test cases passed

@hculea
Copy link
Member

hculea commented Jun 30, 2023

@techcraver I had a chat with @itsCheithanya and we decided we are going to leave this submission in this state, for the time being. @AndyTitu and I need to do some more research in order to see whether the current way the CLI works fully supports this plugin.

We're going to count this towards the hackathon, but I'd love to talk about the blogpost requirement - I think there are a few unknowns that prohibit us from fully documenting this, at this point.

@hculea hculea removed the request for review from arunsathiya July 5, 2023 07:42
@AndyTitu
Copy link
Contributor

AndyTitu commented Sep 1, 2023

@itsCheithanya I did some research on the docker plugin and here's what I came up with:

Limitations: None of our existing provisioning solutions would fit: docker doesn't offer support for env vars, and after testing the FIFO approach for a while, it was apparent that it won't work 100% of the time.

Proposed path forward: I found out that docker uses custom credential helpers as their go-to method to securely fetch credentials (docs here). If you're familiar with the docker credentials helper, wow you're a docker expert, but if you're not this cred helpers are basically just programs that need to respect a contract of returning a {"ServerURL":"%s","Username":"%q","Secret":"%q" json object when a get command is invoked (the full interface is get , store , delete). There exist credential helpers for a lot of various storages such as the mac os keychain. Also each cloud provider created their own docker credentials helper (medium blog that solves the validated problem of having to manage all various cloud providers' docker cred helpers). Anyhow, enough with the background discussion.
The proposed solution would be to have our plugin:

  1. provision a docker-credential-1password executable in the temp dir of the user's machine. This executable is just a bash script that takes values from some env vars or fifo or anything, and returns the secret in the json format from above. We can re-use the credential helper built by Jenkins or create ours as it's straightforward.
    And temporarily add this executable at the end of the PATH (this is how docker finds it)
  2. provision .docker/config.json with { "credsStore": "1password" } (this is what tell docker to look for the docker-credential-1password executable)
  3. set some env vars with the secrets we extracted from 1P so that docker-credential-1password can consume them.
    And voila we have a docker plugin. The nice thing is that in the future we could also have logic for automatically using the AWS Access Keys stored in 1Password in order to retrieve the credentials to authenticate to an ECR registry, and do similar things for Google GCR or Azure's ACR, which will really make people not have to worry about the whole credential helper shabang at all.

@hculea
Copy link
Member

hculea commented Sep 19, 2023

It looks like unfortunately this is going to be a more involved change than we first anticipated.

I'm going to close this PR for the time being. Once we prioritise making the required changes for this plugin, we'll make sure to update here as well.

@hculea hculea closed this Sep 19, 2023
@accraw accraw mentioned this pull request Oct 13, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hashnode hackathon Ideas and inspiration for the hackathon running from June 1st - June 30th 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.

4 participants