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

Secret key prompt #3083

Merged
merged 5 commits into from
Mar 14, 2020
Merged

Secret key prompt #3083

merged 5 commits into from
Mar 14, 2020

Conversation

h0tw1r3
Copy link
Contributor

@h0tw1r3 h0tw1r3 commented Feb 16, 2020

Minor cleanup of #2785

Not directly documented, but also supports standard input of just the secret key.

mc config host add minio http://192.168.1.51 myuser --api S3v4
Enter Secret Key: 

or

cat secret_key_file | mc config host add minio http://192.168.1.51 myuser --api S3v4

@ebozduman
Copy link
Collaborator

ebozduman commented Feb 25, 2020

I've also tested it. I see 3 different behavior depending on how access and secret keys are specified.
Let "minio" be my access key and "minio123" be my secret key.

  1. The old way with both access key and secret key explicitly specified in the command line:
$ mc config host add mygcs http://localhost:9000 minio minio123 --api s3v4
Added `mygcs` successfully.
$ mc config host add mygcs http://localhost:9000 minio minio1234 --api s3v4
Added `mygcs` successfully.
$ mc config host add mygcs http://localhost:9000 minio2 minio1234 --api s3v4
Added `mygcs` successfully.

As it can be seen above, providing both access and secret keys, valid or invalid, goes through the addition process successfully and hence no key verification. User will catch invalid keys, if any during mc command run.

  1. Pipe method
  • echo command syntax with "\n" added into the documentation by this PR does not expand as intended. The example in the doc about "minio\nminio123", needs to be fixed.
$ echo "minio\nminio123" | mc config host add mygcs http://localhost:9000
mc: <ERROR> Unable to initialize new config from the provided credentials. Access Denied.

.... while the following notation works just fine:

echo "minio123" | mc config host add mygcs http://localhost:9000 minio
  • However, invalid credentials in this same format causes some kind of a time-out and only after around 3:+ minutes, the control comes back to the user with some error message about whatever is wrong with the credentials.
$ cat secret_key_file | mc config host add mygcs http://localhost:9000
mc: <ERROR> Unable to initialize new config from the provided credentials. Access Denied.

or

echo "minio123" | mc config host add mygcs http://localhost:9000
  1. Prompt approach
  • Prompt approach catches invalid keys right away when user tries to add a new host. This is inconsistent with the "Old Approach" explained above in number 1, and the old approach can also be fixed for consistency.

cmd/config-host-add.go Outdated Show resolved Hide resolved
@h0tw1r3
Copy link
Contributor Author

h0tw1r3 commented Mar 6, 2020

  1. The old way with both access key and secret key explicitly specified in the command line:
$ mc config host add mygcs http://localhost:9000 minio minio123 --api s3v4
Added `mygcs` successfully.
$ mc config host add mygcs http://localhost:9000 minio minio1234 --api s3v4
Added `mygcs` successfully.
$ mc config host add mygcs http://localhost:9000 minio2 minio1234 --api s3v4
Added `mygcs` successfully.

As it can be seen above, providing both access and secret keys, valid or invalid, goes through the addition process successfully and hence no key verification. User will catch invalid keys, if any during mc command run about the invalid secret and/or access keys.

My test results are different than yours. Validation behavior is exactly the same in master as this branch.

./mc config host add s4 http://localhost:9000 accesskey secretkey
Added `s4` successfully.
./mc config host add s4 http://localhost:9000 accesskey invalidkey
mc: <ERROR> Unable to initialize new config from the provided credentials. The request signature we calculated does not match the signature you provided. Check your key and signing method.
./mc config host add s4 http://localhost:9000 accesskey secretkey --api s3v4
Added `s4` successfully.
./mc config host add s4 http://localhost:9000 accesskey invalidkey --api s3v4
Added `s4` successfully.

If you use the same arguments between all tests, the behaviors are identical. For example, if I add --api s3v4, mc does not reach out and try to connect to the host (ie. any credentials will work).

If you do not add --api s3v4, all the "empty" credential scenarios result in mc hanging as you describe. This is existing behavior.

These commands hang, consistent between branch and master.

./mc config host add s4 http://localhost:9000 "" ""
echo -e "" | ./mc config host add s4 http://localhost:9000

@h0tw1r3
Copy link
Contributor Author

h0tw1r3 commented Mar 6, 2020

The hang described seems to be a minio specific bug. When I change the URL to aws or gcs, empty credentials are accepted, never hang.

@harshavardhana
Copy link
Member

The hang described seems to be a minio specific bug. When I change the URL to aws or gcs, empty credentials are accepted, never hang.

There is no bug server just returns "Access Denied" for empty credentials @h0tw1r3 and mc is perhaps retrying. Can you do --debug ?

@harshavardhana
Copy link
Member

~ mc config host add test https://play.minio.io:9000 "" ""
mc: <ERROR> Unable to initialize new config from the provided credentials. Access Denied.

@h0tw1r3
Copy link
Contributor Author

h0tw1r3 commented Mar 6, 2020

~ mc config host add test https://play.minio.io:9000 "" ""
mc: <ERROR> Unable to initialize new config from the provided credentials. Access Denied.

Hangs for me.

$ make clean
Cleaning up all the generated files
$ git checkout master
Already on 'master'
Your branch is up-to-date with 'remotes/upstream/master'.
$ make
Checking dependencies
Building mc binary to './mc'
$ ./mc config host add test https://play.minio.io:9000 "" ""
^C

Debug does show the 403, but it just keeps retrying.

@harshavardhana
Copy link
Member

Hangs for me.

$ make clean
Cleaning up all the generated files
$ git checkout master
Already on 'master'
Your branch is up-to-date with 'remotes/upstream/master'.
$ make
Checking dependencies
Building mc binary to './mc'
$ ./mc config host add test https://play.minio.io:9000 "" ""
^C

Yes it retries 10 times @h0tw1r3

@harshavardhana
Copy link
Member

Here is the result from your PR

~ mc config host add test https://play.minio.io:9000 "" "" --debug
mc: <DEBUG> GET /probe-bucket-sign-0mj2n44ak14d/?location= HTTP/1.1
Host: play.minio.io:9000
User-Agent: MinIO (linux; amd64) minio-go/v6.0.49
Accept-Encoding: gzip

mc: <DEBUG> HTTP/1.1 403 Forbidden
Content-Length: 312
Accept-Ranges: bytes
Content-Security-Policy: block-all-mixed-content
Content-Type: application/xml
Date: Fri, 06 Mar 2020 04:08:44 GMT
Server: MinIO/DEVELOPMENT.2020-03-03T20-16-50Z
Vary: Origin
X-Amz-Request-Id: 15F99B816A7D547D
X-Xss-Protection: 1; mode=block

<?xml version="1.0" encoding="UTF-8"?>
<Error><Code>AccessDenied</Code><Message>Access Denied.</Message><BucketName>probe-bucket-sign-0mj2n44ak14d</BucketName><Resource>/probe-bucket-sign-0mj2n44ak14d/</Resource><RequestId>15F99B816A7D547D</RequestId><HostId>96fa3866-7ee6-4546-87d9-4283e3def6c3</HostId></Error>mc: <DEBUG> Response Time:  82.197334ms

mc: <DEBUG> HEAD /probe-bucket-sign-0mj2n44ak14d/ HTTP/1.1
Host: play.minio.io:9000
User-Agent: MinIO (linux; amd64) minio-go/v6.0.49

mc: <DEBUG> HTTP/1.1 403 Forbidden
Accept-Ranges: bytes
Content-Security-Policy: block-all-mixed-content
Date: Fri, 06 Mar 2020 04:08:44 GMT
Server: MinIO/DEVELOPMENT.2020-03-03T20-16-50Z
Vary: Origin
X-Amz-Request-Id: 15F99B816C230E14
X-Xss-Protection: 1; mode=block
Content-Length: 0

mc: <DEBUG> Response Time:  27.458197ms

mc: <DEBUG> HEAD /probe-bucket-sign-0mj2n44ak14d/ HTTP/1.1
Host: play.minio.io:9000
User-Agent: MinIO (linux; amd64) minio-go/v6.0.49

mc: <DEBUG> HTTP/1.1 403 Forbidden
Accept-Ranges: bytes
Content-Security-Policy: block-all-mixed-content
Date: Fri, 06 Mar 2020 04:08:44 GMT
Server: MinIO/DEVELOPMENT.2020-03-03T20-16-50Z
Vary: Origin
X-Amz-Request-Id: 15F99B816D441DC2
X-Xss-Protection: 1; mode=block
Content-Length: 0

mc: <DEBUG> Response Time:  17.418491ms

mc: <ERROR> Unable to initialize new config from the provided credentials. Access Denied.
 (3) config-host-add.go:282 cmd.mainConfigHostAdd(..) Tags: [test, https://play.minio.io:9000, , ]
 (2) config-host-add.go:224 cmd.buildS3Config(..) Tags: [https://play.minio.io:9000, , , , auto]
 (1) client-s3.go:1240 cmd.(*s3Client).Stat(..) Tags: [probe-bucket-sign-0mj2n44ak14d]
 (0) client-s3.go:1786 cmd.(*s3Client).bucketStat(..)
 Release-Tag:DEVELOPMENT.2020-03-06T04-08-29Z | Commit:6a5047a5f735 | Host:backspace | OS:linux | Arch:amd64 | Lang:go1.13.8 | Mem:2.0 MB/72 MB | Heap:2.0 MB/66 MB.

@harshavardhana
Copy link
Member

Also, we are just discussing unnecessary hyperbole there is no reason anyone would pass empty creds unless they want anonymous access, we provide that as a feature.

It is okay to avoid doing validation for empty credentials because there is no point validating them in the first place, because they are empty. We can also assume any signature because anonymous has no signature due to a lack of keys.

@h0tw1r3
Copy link
Contributor Author

h0tw1r3 commented Mar 6, 2020

Thanks for confirming. I am able to reproduce the "retry forever" behavior with the current release from dl.minio.io, but agree it is unrelated.

@h0tw1r3 h0tw1r3 force-pushed the secret-key-prompt branch 2 times, most recently from 64541bf to b22f89e Compare March 7, 2020 13:29
@h0tw1r3
Copy link
Contributor Author

h0tw1r3 commented Mar 7, 2020

rebased on master

@h0tw1r3 h0tw1r3 requested a review from ebozduman March 7, 2020 13:30
Copy link
Collaborator

@ebozduman ebozduman left a comment

Choose a reason for hiding this comment

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

LGTM

@ebozduman
Copy link
Collaborator

A final minor note:
Just to eliminate any confusion, you may want to remove the cat secret_key_file | mc config ... example from the description section of this PR.

@kannappanr kannappanr merged commit bcbbfa7 into minio:master Mar 14, 2020
cesnietor added a commit to cesnietor/mc that referenced this pull request Apr 2, 2020
* Fix zh_CN doc about policy errors (minio#2927)

* Fix Windows config path (minio#2898)

Remove '.exe' (case insensitive) from executable name for consistent config folder.

Potentially breaking: If people (for whatever reason) used 'mc.exe', this will potentially break their setup.

* Fix removing empty prefixes  with `mc rm ` command (minio#2912)

Fixes: 2909

* Add --type flag to `mc admin console` (minio#2914)

This allows filtering error logs by type

* Update mc to use secure-io package (minio#2931)

* Refactor mc cp and make session optional. (minio#2910)

Now `mc cp --recursive --continue` creates copy session on failure or
resumes if previous failed session is found.

Fixes minio#2893

* continue flag: Fix inconsistent punctuation in usage text (minio#2933)

* fix mc cp termination on signals (minio#2934)

* Adapt new changes in StorageInfo (minio#2889)

* Fix arrow signs to use more common U+2191 & U+2193 (minio#2942)

* Avoid index out of range errors (minio#2943)

Fixes minio#2941

* Fix typo in error message (minio#2945)

Change the error message from `Cannot remove new user` to 
`Cannot remove <User>`, when a non-existent user is removed.

* Add options to preserve filesystem attributes (minio#2938)

* mc admin config set/get/del/history/help (minio#2835)

Related to minio/minio#8392

* print disk usage as a plain integer when formatting as json (minio#2946)

* Fix the storage prints for xl/fs disks in `admin info server` (minio#2947)

* Fix config history to list in git style format (minio#2951)

```
~ mc admin config history list -n 2 myminio
RestoreId: df0ebb1e-69b0-4043-b9dd-ab54508f2897
Date: Mon, 04 Nov 2019 17:27:27 GMT

region name="us-east-1" state="on"
region name="us-east-1" state="on"
region name="us-east-1" state="on"
region name="us-east-1" state="on"

RestoreId: ecc6873a-0ed3-41f9-b03e-a2a1bab48b5f
Date: Mon, 04 Nov 2019 17:28:23 GMT

region name=us-east-1 state=off
```

* add lock command (minio#2880)

`lock` command enables to set, get and clear object lock configuration
of given bucket.

* update to latest minio (minio#2956)

* Fix encrypt-key flag parsing (minio#2955)

* update to latest minio (minio#2957)

* Rectify usage section of policy help (minio#2959)

* help command is now part of `mc admin config get/set` (minio#2961)

* mirror object lock configuration to dest bucket (minio#2964)

if bucket does not exist on the target side.

* Add default values in 'get' (minio#2962)

* Add mirror command to compare metadata and copy (minio#2965)

mirror now uses the newly introduced
ListObjectsV2WithMetadata API in minio-go

* Handle PutObjectRetention notification events correctly (minio#2966)

* Retention command to apply retention to objects with a Prefix (minio#2963)

* Fix totalSize and totalCounts calculation (minio#2969)

* Add import/export command (minio#2974)

* Add support for auto-complete in 'mc admin config' commands (minio#2975)

* Remove duplicated API method(s) for object retention (minio#2973)

* Fix progress bar update regression (minio#2976)

Regression introduced in cf6defe

* Update export command to print disabled sub-sys as comments (minio#2978)

* Add finalized command line options (minio#2979)

* Avoid re-parsing config history elements (minio#2981)

* change target to name in config help (minio#2983)

* fix: Source content can be nil when updating progress bar (minio#2984)

* Continue mirroring inspite of error events (minio#2986)

* watch: Add site wide level support (minio#2987)

* Continue listing upon error during diff and mirror (minio#2988)

* Support for multi master sites (minio#2991)

* Retry mirror forever on watch (minio#2992)

* Fix multi-master setup help text (minio#2993)

* Do full scan diff+mirror in multi-site mirror mode (minio#2994)

* update minio-go to master (minio#2995)

* Avoid mirror fatalIf in multi-master (minio#2998)

* Mirror with fallback list in multimaster mode, copy missing files (minio#2999)

* Fix mirroring of PutObjectRetention event (minio#3004)

* Multi-master only run single startMirror at a time (minio#3003)

Also randomize the sleep appropriately based
on Unix() time source.

* mc sql: Use compression parameter (minio#2996)

The compression parameter is not actually used, but always guessed from the file extension.

Only guess the compression type if it is not explicitly set.

Uploading a file called `out.csv.geezipd`:

Before:
```
λ mc sql -e="SELECT * from s3object LIMIT 1" --compression=GZIP --csv-input "rd=\n,fh=USE,fd=;" local/test/out.csv.geezipd
�""���k↔��Xs��W.v�      ��)��{2#��,r���:�|D▲�.��f♫��→�i�p�2l�♫��ʭ���Q��v▲�5ر�c
```
(content actually not ungzipped)

After:
```
λ mc sql -e="SELECT * from s3object LIMIT 1" --compression=GZIP --csv-input "rd=\n,fh=USE,fd=;" local/test/out.csv.geezipd
"281,1285,159625,159627,2637,20000827.0117590018,1239029,1663,-6.7535419,1533,0.53597438,8"
```

We leave validation of the compression value to the server.

* Replace Timestamp separator   (minio#3002)

Replace time stamp separator to use `-`

* New admin info (minio#2982)

* Stops showing usage info if Bucket#=0 (minio#3007)

* Extraneous line in 'mc admin info' output (minio#3010)

* cp: Only use session when -c is passed (minio#2989)

* Upgrade to new Listen bucket API (minio#3008)

* fix: ignore APINotImplemented errors from filesystem (minio#3018)

* Update to latest minio/minio (minio#3020)

* Mirror: Generate STag and ETag on destination only in Multimaster mode (minio#3021)

* Update mc dependencies to recent dep change on MinIO (minio#3026)

* Use colorjson to indent json properly in cp (minio#3030)

Additionally also do not initialize progress bar in json
or quiet mode in mirror.

Fixes minio#3029
Fixes minio#3017

* fix default flag behaviour in du command (minio#3031)

- `mc du` without args should print help like other `mc` commands
- `mc du` should behave like `du -h --max-depth=1` as default
- optional `--recursive` if they want capacity for each folder.
fixes minio#2871

* Add lock and retention docs (minio#3034)

* Only rename config directories (minio#3032)

If the user placed an `mc.exe` in the user directory and wasn't used it, it would be renamed.

* fix: crash in cp when cpURLs return error (minio#3035)

* fix: du to ignore too many recursive symlinks (minio#3033)

* fix: update docs for --attr command (minio#3040)

Also canonicalize metadata headers automatically
before, minio-go consumes it.

* retention: Error out early when url argument does not point to S3 server (minio#3041)

* fix: update minio-go to support snowball data-transfer (minio#3042)

* cp: add missing } in help template (minio#3047)

* Update pkg/madmin to support multiple profilers (minio#3048)

* admin/console: Fix a crash in console string generation (minio#3049)

* fix: best effort save permissions always (minio#3050)

* upgrade to latest server to remove replace tags (minio#3051)

* fix: update admin docs for latest changes (minio#3054)

* fix: handle symlinks by reading/following as needed (minio#3053)

Avoid using filepath.EvalSymlinks and let the caller
fail appropriately for symlinks.

Fixes minio#3011

* check for bucket exists in 'admin heal' (minio#3059)

* Ensure 64bit alignment on 32bit builds for atomics (minio#3065)

Keep elements that are used with atomic.* functions as first element
of struct because it guarantees 64bit alignment on 32 bit machines.
atomic.* functions crash if operand is not aligned at 64bit.
See golang/go#599

* add the `mc admin kms` subcommands (minio#2882)

This commit adds the first set of KMS admin
commands.

For new there is only the
`mc admin kms key status <endpoint> [key-ID]` command
that can be used to fetch status information for a
KMS master key. Listing and rotation commands are
planed.

* Fix go.sum file missing entries (minio#3071)

* add debug support for 'config host add' (minio#3076)

* Fix crash in mc admin console (minio#3079)

Fixes : minio#3078

* use latest minio-go version (minio#3080)

* allow mirror to copy zero byte file (minio#3084)

this PR also cleans up code and avoids
extra calls for source target

* go mod tidy (minio#3081)

* honor prefix for object and directory names in 'rm -r' (minio#3012)

* fix go mod tidy again

* added pkg/disk/stat_freebsd.go (minio#3090)

* fix handling of directories in Stat (minio#3092)

Fixes minio#3089
Fixes minio#3091

* retention - cleanup unused code (minio#3094)

* Use listObjectsV1 for GCS S3 implementation (minio#3101)

Fixes minio#3073

* Support arm32, 386 compilation: (minio#3103)

* deprecate 'mc session' documentation (minio#3104)

* Add support of threads & goroutines profiling type (minio#3108)

* fix go mod tidy

* Update minio-go to bring some fixes (minio#3106)

* resolve build issues on illumos (minio#3098)

* Update docs for mc admin update (minio#3110)

* fix: Make sure Ctrl-C cancels `mc cp` right away (minio#3113)

* mc cp -continue: Fix escape/colorization of JSON (minio#3116)

* info: Fix showing offline nodes in the pretty output (minio#3112)

Also do some simplification in the code.

* Secret key prompt (minio#3083)

* heal: Show scheduled next healing (minio#3111)

mc admin heal alias command will show the next healing time.

Also, show 'Never completed' when the server returns zero last healing activity

* mc cp: Fix prefix handling in TypeC (minio#3115)

for copy from filesystem to objectstorage.

Avoid prefixing dirname to file on target side
when doing a `mc cp /tmp/dir myminio/bucket --r`
to make `mc mirror` and `mc cp` behavior consistent

* info: Add missing new lines in the output (minio#3119)

* fix for incorrect mtime handling (minio#3121)

* mc retention: add --recursive and --bypass flags. (minio#3100)

--bypass flag allows overriding governance if user
has override permissions

* cleanup legacy metadata handling (minio#3118)

* Add global context and adapt context change in admin API (minio#3125)

This commit adds a new global context, monitors os signals
and cancels the global context in such cases to exit
on going process gracefully.

It will also update minio-go and adapts the code with
the new context changes in the admin API.

* Add tag command (minio#3117)

* cleanup tag command (minio#3126)

* Add legalhold command (minio#3095)

* mc watch windows URL/absolute path event path (minio#3133)

* Onboard Diagnostics (minio#3114)

Also colorize JSON output in all admin commands

* Make BuildS3Config public (minio#3132)

* add build trimpaths to remove gopath (minio#3137)

* add --disable-multipart flag in cp (minio#3135)

* set S3Client struct as public

* set s3client struct as public to be used by mcs

Co-authored-by: yiranzai <[email protected]>
Co-authored-by: Klaus Post <[email protected]>
Co-authored-by: poornas <[email protected]>
Co-authored-by: Harshavardhana <[email protected]>
Co-authored-by: Bala FA <[email protected]>
Co-authored-by: Ashish Kumar Sinha <[email protected]>
Co-authored-by: Praveen raj Mani <[email protected]>
Co-authored-by: Nitish Tiwari <[email protected]>
Co-authored-by: kannappanr <[email protected]>
Co-authored-by: Wesley Collin Wright <[email protected]>
Co-authored-by: Krishna Srinivas <[email protected]>
Co-authored-by: Anis Elleuch <[email protected]>
Co-authored-by: BigUstad <[email protected]>
Co-authored-by: ebozduman <[email protected]>
Co-authored-by: Maël Valais <[email protected]>
Co-authored-by: Reinhard Koehn <[email protected]>
Co-authored-by: Andreas Auernhammer <[email protected]>
Co-authored-by: Dmitry Wagin <[email protected]>
Co-authored-by: Kody A Kantor <[email protected]>
Co-authored-by: Jeffrey Clark <[email protected]>
Co-authored-by: Sidhartha Mani <[email protected]>
Co-authored-by: Daniel Valdivia <[email protected]>
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.

4 participants