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

Accept SECRET_KEY using prompt and standard input. #2785

Closed
wants to merge 1 commit into from

Conversation

sinhaashish
Copy link
Contributor

@sinhaashish sinhaashish commented May 31, 2019

The ACCESS_KEY , SECRET_KEY is taken as an input . This is done to avoid the credentials shown
in ps aux command.
Adding a new user asks for their credentials.

To test:

  1. mc config host add myminio http://192.168.86.149:9000
Enter Access Key : BKIKJAA5BMMU2RHO6IBB
Enter Secret Key : V8f1CwQqAcwo80UEIJEjc5gVQUSSx5ohQ9GSrr12
Added `myminio` successfully.
  1. mc admin user add myminio foobar readonly
Enter Access Key : testuser 
Enter Secret Key : testuser123

Fixes #2651

@codecov
Copy link

codecov bot commented May 31, 2019

Codecov Report

Merging #2785 into master will decrease coverage by 0.13%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2785      +/-   ##
==========================================
- Coverage    10.5%   10.37%   -0.14%     
==========================================
  Files         129      127       -2     
  Lines       12751    12738      -13     
==========================================
- Hits         1339     1321      -18     
- Misses      11245    11258      +13     
+ Partials      167      159       -8
Impacted Files Coverage Δ
cmd/config-host-add.go 0% <0%> (ø) ⬆️
cmd/admin-user-add.go 0% <0%> (ø) ⬆️
cmd/client-fs.go 27.3% <0%> (-1.72%) ⬇️
cmd/client-url.go 66.66% <0%> (-1.45%) ⬇️
cmd/config.go 33.33% <0%> (-0.52%) ⬇️
cmd/fs-pathutils_nix.go
cmd/client-fs_linux.go

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6756185...54da186. Read the comment docs.

cmd/admin-user-add.go Outdated Show resolved Hide resolved
cmd/config-host-add.go Outdated Show resolved Hide resolved
docs/minio-client-complete-guide.md Outdated Show resolved Hide resolved
cmd/config-host-add.go Outdated Show resolved Hide resolved
cmd/admin-user-add.go Outdated Show resolved Hide resolved
Copy link
Member

@vadmeste vadmeste left a comment

Choose a reason for hiding this comment

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

Some comments

cmd/admin-user-add.go Outdated Show resolved Hide resolved
cmd/admin-user-add.go Outdated Show resolved Hide resolved
cmd/admin-user-add.go Outdated Show resolved Hide resolved
cmd/admin-user-add.go Outdated Show resolved Hide resolved
@harshavardhana
Copy link
Member

Okay so @abperiasamy confirmed two things

  • preserve the current syntax when all arguments are provided and no change there
  • show a prompt if user doesn't type all the input options, let's say alias is available and rest are not available throw a prompt. Where each prompt takes accessKey, secretKey etc..

So you need to redo this PR to have this behavior.

@sinhaashish
Copy link
Contributor Author

Okay so @abperiasamy confirmed two things

  • preserve the current syntax when all arguments are provided and no change there
  • show a prompt if user doesn't type all the input options, let's say alias is available and rest are not available throw a prompt. Where each prompt takes accessKey, secretKey etc..

So you need to redo this PR to have this behavior.

👍

@sinhaashish sinhaashish force-pushed the secret-key-prompt branch 5 times, most recently from 12bcc47 to 54da186 Compare June 26, 2019 08:05
@sinhaashish
Copy link
Contributor Author

@harshavardhana @vadmeste @poornas modifed the PR. PTAL

cmd/admin-user-add.go Show resolved Hide resolved
cmd/admin-user-add.go Outdated Show resolved Hide resolved
docs/minio-admin-complete-guide.md Outdated Show resolved Hide resolved
cmd/admin-user-add.go Outdated Show resolved Hide resolved
@harshavardhana
Copy link
Member

One more thing this PR also needs to do is when --json, --quiet or if the tty is not a terminal - it should turn off prompt. This is a script mode or non-interactive.

@sinhaashish
Copy link
Contributor Author

One more thing this PR also needs to do is when --json, --quiet or if the tty is not a terminal - it should turn off prompt. This is a script mode or non-interactive.

Done with the other changes . Can you elaborate if the tty is not a terminal . This PR prompts for access and secret key only if it is not passed. IMO in script mode access and secret key will be passed so it wont prompt for user input.

Copy link
Member

@vadmeste vadmeste left a comment

Choose a reason for hiding this comment

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

Some comments

*Example: Add a new user 'testuser' on MinIO, with 'writeonly' using standard input.*

```sh
$ set -o history
Copy link
Member

Choose a reason for hiding this comment

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

I think we don't have to disable history if we are entering access & secret keys in the prompt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

{{.EnableHistory}}
$ set -o history
$ {{.HelpName}} myminio foobar foo12345
$ set +o history
Copy link
Member

Choose a reason for hiding this comment

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

There is some extra leading spaces here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

corrected

Comment on lines 147 to 148
Enter API signature. An optional argument. Default value 'S3v4' :
Enter lookup. An optional argument. Default value 'auto' :
Copy link
Member

Choose a reason for hiding this comment

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

I believe both of these prompts are not there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, removed.

The access and secret key is taken as an input from promt . This is done to avoid the credetials shown
in ps aux command. Adding a new user asks for a user credentials.
value, _, _ := reader.ReadLine()
accessKey = string(value)
fmt.Printf("%s", console.Colorize(cred, "Enter Secret Key : "))
bytePassword, _ := terminal.ReadPassword(int(syscall.Stdin))
Copy link
Member

Choose a reason for hiding this comment

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

Missing proper error handling

Copy link
Member

Choose a reason for hiding this comment

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

Also according to Terminal.ReadPassword we are supposed to pass the prompt. In any case we should use os.File.Fd() instead of the syscall package.

case numberOfArgs == 2:
accessKey = args.Get(1)
fmt.Printf("%s", console.Colorize(cred, "Enter Secret Key : "))
bytePassword, _ := terminal.ReadPassword(int(syscall.Stdin))
Copy link
Member

Choose a reason for hiding this comment

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

Missing proper error handling

@harshavardhana
Copy link
Member

are we going to implement this? @kannappanr @sinhaashish - if there are no plans can we close them out?

@kannappanr
Copy link
Collaborator

ping @sinhaashish

@h0tw1r3
Copy link
Contributor

h0tw1r3 commented Feb 10, 2020

please implement this. it is not uncommon for security policies to require (or force) shell logging.

h0tw1r3 added a commit to h0tw1r3/mc that referenced this pull request Feb 16, 2020
h0tw1r3 added a commit to h0tw1r3/mc that referenced this pull request Feb 16, 2020
h0tw1r3 added a commit to h0tw1r3/mc that referenced this pull request Feb 16, 2020
h0tw1r3 added a commit to h0tw1r3/mc that referenced this pull request Feb 16, 2020
@h0tw1r3 h0tw1r3 mentioned this pull request Feb 16, 2020
@sinhaashish
Copy link
Contributor Author

Closing this for now . Will send another PR for this

@harshavardhana
Copy link
Member

@sinhaashish would you take a look at #3083

h0tw1r3 added a commit to h0tw1r3/mc that referenced this pull request Mar 7, 2020
h0tw1r3 added a commit to h0tw1r3/mc that referenced this pull request Mar 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider password prompt or env variable when using mc admin user add
7 participants