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

--password-stdin flag in podman login #2320

Merged
merged 1 commit into from
Feb 14, 2019

Conversation

QiWang19
Copy link
Contributor

Support --password-stdin flag, reads a password from STDIN and pass it to podman login.
enhancement mentioned #1675 (comment)
Signed-off-by: Qi Wang qiwan@redhat.com

@TomSweeneyRedHat
Copy link
Member

Test errors look unrelated. Need updates to the man page and bash completions. The code looks good though.

@@ -90,6 +91,19 @@ func loginCmd(c *cliconfig.LoginValues) error {
}

ctx := getContext()

var password string
Copy link
Member

Choose a reason for hiding this comment

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

mind wrapping in a ( ) ?

Copy link
Member

Choose a reason for hiding this comment

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

You wnat a function getPasswordStdin()?

fmt.Fprint(&stdinPasswordStrBuilder, scanner.Text())
}
password = stdinPasswordStrBuilder.String()
}
Copy link
Member

Choose a reason for hiding this comment

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

is there any cli validation that should occur between this flag and the password flag? exclusive?

Copy link
Member

Choose a reason for hiding this comment

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

Please explain, what you re worried about?

Copy link
Member

Choose a reason for hiding this comment

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

We have --password and --password-stdin - they should be mutually exclusive.

Copy link
Member

Choose a reason for hiding this comment

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

They are now.

if c.Username == "" {
return errors.Errorf("Must provide --username with --password-stdin")
}
scanner := bufio.NewScanner(os.Stdin)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a more secure version of this? I'm thinking we probably want a version that replaces what's being typed with * so we don't display passwords as they're being typed in

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this enable

echo MYPASSWORD | podman login --passwd-stdin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rhatdan Yes, it enables that

Support --password-stdin flag, reads a password from STDIN and pass it to `podman login`.

Signed-off-by: Qi Wang <qiwan@redhat.com>
@rhatdan
Copy link
Member

rhatdan commented Feb 14, 2019

@mheon
Copy link
Member

mheon commented Feb 14, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 14, 2019
Copy link
Member

@TomSweeneyRedHat TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

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

LGTM

@rhatdan
Copy link
Member

rhatdan commented Feb 14, 2019

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: QiWang19, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 14, 2019
@QiWang19
Copy link
Contributor Author

/retest

@openshift-merge-robot openshift-merge-robot merged commit 1a9128d into containers:master Feb 14, 2019
@QiWang19 QiWang19 deleted the stdinPW branch June 26, 2020 15:09
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants