Add support for passing non-printable character as key #2851

sinhaashish · 2019-08-06T18:29:05Z

This feature enables the users to pass non-printable character as encrytion-key by encoding it.
Example :
If the encryption keys contains a non printable character like tab :
--encrypt-key "play/test/=32byteslongsecretke mustbegiven1".
The user need to encode using base64 and pass the encoded string as the key.
--encrypt-key "play/test=MzJieXRlc2xvbmdzZWNyZXRrZQltdXN0YmVnaXZlbjE="

Tested case:

case 1.  Copy encrypted file from s3 to play using  nonprintable character in key ( "s3/encryptedfile/=32byteslongsecreabcdefg	givenn21,play/encrypt=32byteslongsecretke	mustbegiven1") .
 The key contains tab character

a)  ./mc mb s3/encryptedfile
b)  ./mc cp  --encrypt-key "s3/encryptedfile/=MzJieXRlc2xvbmdzZWNyZWFiY2RlZmcJZ2l2ZW5uMjE=" ~/Downloads/test.csv s3/encryptedfile/test.csv
c) ./mc mb  play/encrypt
d) ./mc cp --recursive --encrypt-key "s3/encryptedfile/=MzJieXRlc2xvbmdzZWNyZWFiY2RlZmcJZ2l2ZW5uMjE=,play/encrypt/=MzJieXRlc2xvbmdzZWNyZXRrZQltdXN0YmVnaXZlbjE="  s3/encryptedfile/   play/encrypt/
e) ./mc head  --encrypt-key "play/encrypt/=MzJieXRlc2xvbmdzZWNyZXRrZQltdXN0YmVnaXZlbjE="  play/encrypt/test.csv
f) ./mc cat  --encrypt-key "play/encrypt/=MzJieXRlc2xvbmdzZWNyZXRrZQltdXN0YmVnaXZlbjE="  play/encrypt/test.csv


Case 2.  Copy encrypted file from s3 to play using  32 byte string  as key and key for play is encoded as it contains a non printable character  ( "s3/documents/=32byteslongsecretkeymustbegiven1,play/normalstr/=32byteslongsecretke	mustbegiven1") .
a) ./mc mb s3/normalstr
b)  ./mc cp  --encrypt-key "s3/normalstr/=32byteslongsecretkeymustbegiven1" ~/Downloads/test.csv s3/normalstr/test.csv
c) ./mc mb  play/normalstr
d) ./mc cp --recursive --encrypt-key "s3/normalstr/=32byteslongsecretkeymustbegiven1,play/normalstr/=MzJieXRlc2xvbmdzZWNyZXRrZQltdXN0YmVnaXZlbjE="  s3/normalstr/   play/normalstr/
e) ./mc head  --encrypt-key "play/normalstr/=MzJieXRlc2xvbmdzZWNyZXRrZQltdXN0YmVnaXZlbjE="  play/normalstr/test.csv
f) ./mc cat  --encrypt-key "play/normalstr/=MzJieXRlc2xvbmdzZWNyZXRrZQltdXN0YmVnaXZlbjE="  play/normalstr/test.csv
g) ./mc stat  --encrypt-key "play/normalstr/=MzJieXRlc2xvbmdzZWNyZXRrZQltdXN0YmVnaXZlbjE="  play/normalstr/test.csv
h) ./mc rm  --encrypt-key "play/normalstr/=MzJieXRlc2xvbmdzZWNyZXRrZQltdXN0YmVnaXZlbjE="  play/normalstr/test.csv
i) cat ~/Downloads/10000.csv | mc pipe  --encrypt-key "play/normalstr/=MzJieXRlc2xvbmdzZWNyZXRrZQltdXN0YmVnaXZlbjE=" play/normalstr/10000.csv
j) ./mc mirror --overwrite --insecure   --encrypt-key "play/normalstr/=MzJieXRlc2xvbmdzZWNyZXRrZQltdXN0YmVnaXZlbjE="  play/normalstr/ myminio/sinha/

Closes #2678

codecov-io · 2019-08-07T00:19:07Z

Codecov Report

Merging #2851 into master will increase coverage by 0.14%.
The diff coverage is 69.69%.

@@            Coverage Diff             @@
##           master    #2851      +/-   ##
==========================================
+ Coverage     9.9%   10.04%   +0.14%     
==========================================
  Files         140      140              
  Lines       13522    13555      +33     
==========================================
+ Hits         1339     1362      +23     
- Misses      12016    12026      +10     
  Partials      167      167

Impacted Files	Coverage Δ
cmd/cat-main.go	`18.39% <ø> (ø)`	⬆️
cmd/mirror-main.go	`0% <ø> (ø)`	⬆️
cmd/stat-main.go	`0% <ø> (ø)`	⬆️
cmd/head-main.go	`0% <ø> (ø)`	⬆️
cmd/cp-main.go	`3.4% <0%> (-0.05%)`	⬇️
cmd/common-methods.go	`9.12% <79.31%> (+9.12%)`	⬆️

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 af7d551...46ae6fe. Read the comment docs.

cmd/stat-main.go

cmd/mirror-main.go

poornas · 2019-08-09T18:11:23Z

cmd/head-main.go

@@ -68,6 +68,10 @@ EXAMPLES:

  2. Display only first line from server encrypted object on Amazon S3.
     $ {{.HelpName}} -n 1 --encrypt-key 's3/csv-data=32byteslongsecretkeymustbegiven1' s3/csv-data/population.csv
+
+  3. Display only first line from server encrypted object on Amazon S3. In case the encryption key contains non-printable characcter like tab, pass the


s/characcter/character

poornas · 2019-08-09T18:13:16Z

cmd/cat-main.go

@@ -69,6 +69,10 @@ EXAMPLES:

  4. Save an encrypted object from Amazon S3 cloud storage to a local file.
     $ {{.HelpName}} --encrypt-key 's3/mysql-backups=32byteslongsecretkeymustbegiven1' s3/mysql-backups/backups-201810.gz > /mnt/data/recent.gz
+
+  5. Display the content of encrypted object. In case the encryption key contains non-printable characcter like tab, pass the


s/characcter/character

poornas · 2019-08-09T18:16:22Z

cmd/cp-main.go

+
+	if sseKeys != "" {
+		sseKeys, err = getDecodedKey(sseKeys)
+		fatalIf(err, "Unable to decrypt encryption keys.")


"Unable to decrypt encryption keys." is incorrect as you are just trying to decode the base64 encoded key.
"Unable to parse encryption key" is better

poornas · 2019-08-09T18:59:44Z

cmd/common-methods.go

+		secretValue := encryptString[1]
+		decodedString, e := base64.StdEncoding.DecodeString(secretValue)
+		if e != nil {
+			return "", probe.NewError(errors.New("key should be 32 bytes long or if the key is encoded; then decoded value must be of 32 bytes long"))


Suggested change

return "", probe.NewError(errors.New("key should be 32 bytes long or if the key is encoded; then decoded value must be of 32 bytes long"))

return "", probe.NewError(errors.New("key should be 32 bytes long or if the key is base64 encoded; then decoded value must be 32 bytes long"))

```

poornas · 2019-08-09T19:15:18Z

cmd/common-methods.go

+		}
+
+		if len(decodedString) != 32 {
+			keyString = keyString + sse


shouldn't you be returning an error here because plain text key is not 32 bytes

That is taken care in parseEncryptionKeys .

poornas · 2019-08-09T19:16:17Z

cmd/common-methods_test.go

+		err    error
+		status bool
+	}{
+		//success scenerio the key contains non printable (tab) character as key


s/scenerio/scenario across this test case

This needs to be fixed in more than one place. Please fix them everywhere

kannappanr · 2019-08-10T22:08:04Z

cmd/common-methods.go

+		encryptString := strings.SplitN(sse, "=", 2)
+		pre := encryptString[0]
+		secretValue := encryptString[1]
+		decodedString, e := base64.StdEncoding.DecodeString(secretValue)


It is better to have this part in a function. Other than that, the first part should be to check if the length of secretValue is 32, then it is plain text and use it as is. Only otherwise do you proceed to decode and if the decode fails, you return error. Else check if the decoded length is 32, then use that, else return error

Now, what will happen is that even if you encode a string of length 10 and pass it in command line, this code is going to use it as plain text as the decoded string length will be 10 and this code will assume that it is a plain text secret key.

Added a function func parseKey(sseKeys string) (sse string, err *probe.Error) to achieve this functionality.

kannappanr

Please fix the code based on the comments made in this PR

kannappanr · 2019-08-14T10:15:27Z

cmd/common-methods.go

+		return sseKeys, nil
+	}
+	decodedString, e := base64.StdEncoding.DecodeString(secretValue)
+	if e != nil || len(decodedString) != 32 || len(secretValue) != 32 {


Suggested change

if e != nil || len(decodedString) != 32 || len(secretValue) != 32 {

if e != nil || len(decodedString) != 32 {

poornas · 2019-08-16T23:00:36Z

cmd/common-methods.go

+	}
+	decodedString, e := base64.StdEncoding.DecodeString(secretValue)
+	if e != nil || len(decodedString) != 32 {
+		return "", probe.NewError(errors.New("key should be 32 bytes long or if the key is base64 encoded; then decoded value must be 32 bytes long"))


Suggested change

return "", probe.NewError(errors.New("key should be 32 bytes long or if the key is base64 encoded; then decoded value must be 32 bytes long"))

return "", probe.NewError(errors.New("Encryption key should be 32 bytes plain text key or 64 bytes base64 encoded key"))

poornas · 2019-08-16T23:01:54Z

cmd/common-methods_test.go

+		// {"play/documents/=32byteslongsecretkeymustbegiven1,s3/documents/=MzJieXRlc2xvbmdzZWNyZWFiY2RlZmcJZ2l2ZW5uMjE=", "play/documents/=32byteslongsecretkeymustbegiven1,s3/documents/=32byteslongsecreabcdefg	givenn21", nil, true},
+		// // decoded key less than 32 char and conatin non printable (tab) character
+		// {"s3/documents/=MzJieXRlc2xvbmdzZWNyZWFiY2RlZmcJZ2l2ZW5uMjE", "", errors.New("key should be 32 bytes long or if the key is base64 encoded; then decoded value must be 32 bytes long"), false},
+		// // normal key less than 32 character


looks like these tests need to be uncommented

sinhaashish · 2019-08-22T15:10:08Z

@poornas @kannappanr PTAL

poornas · 2019-08-22T21:16:08Z

cmd/cp-main_test.go

@@ -28,11 +28,11 @@ func TestParseMetaData(t *testing.T) {
 		err    error
 		status bool
 	}{
-		// success scenerio using ; as delimitter
+		// success scenario using ; as delimitter


please fix typo delimitter -> delimiter wherever they occur

thanks for pointing it out. Done with the change.

poornas

LGTM

kannappanr

Tested. LGTM

harshavardhana force-pushed the master branch from a359537 to 809741e Compare August 6, 2019 22:46

kannappanr requested review from poornas and kannappanr August 6, 2019 23:00

poornas requested changes Aug 9, 2019

View reviewed changes

kannappanr reviewed Aug 10, 2019

View reviewed changes

kannappanr requested changes Aug 10, 2019

View reviewed changes

sinhaashish force-pushed the non-printable-char branch from 132b819 to a958194 Compare August 13, 2019 10:13

kannappanr reviewed Aug 14, 2019

View reviewed changes

sinhaashish force-pushed the non-printable-char branch 2 times, most recently from 30e2bb4 to 65eb30f Compare August 14, 2019 11:43

poornas reviewed Aug 16, 2019

View reviewed changes

sinhaashish force-pushed the non-printable-char branch from 65eb30f to 7770235 Compare August 19, 2019 00:27

poornas reviewed Aug 22, 2019

View reviewed changes

sinhaashish force-pushed the non-printable-char branch from 7770235 to 458b0a4 Compare August 23, 2019 06:55

poornas previously approved these changes Aug 28, 2019

View reviewed changes

kannappanr previously approved these changes Aug 29, 2019

View reviewed changes

Add support for passing non-printable charachter as key

f634d77

sinhaashish dismissed stale reviews from kannappanr and poornas via f634d77 August 29, 2019 06:05

sinhaashish force-pushed the non-printable-char branch from 46ae6fe to f634d77 Compare August 29, 2019 06:05

poornas approved these changes Aug 29, 2019

View reviewed changes

harshavardhana merged commit 1582b1c into minio:master Aug 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for passing non-printable character as key #2851

Add support for passing non-printable character as key #2851

sinhaashish commented Aug 6, 2019

codecov-io commented Aug 7, 2019 •

edited

poornas Aug 9, 2019

sinhaashish Aug 13, 2019

poornas Aug 9, 2019

sinhaashish Aug 13, 2019

poornas Aug 9, 2019

sinhaashish Aug 13, 2019

poornas Aug 9, 2019 •

edited

sinhaashish Aug 13, 2019

poornas Aug 9, 2019

sinhaashish Aug 13, 2019

poornas Aug 9, 2019

kannappanr Aug 10, 2019

sinhaashish Aug 13, 2019

kannappanr Aug 10, 2019

sinhaashish Aug 13, 2019

kannappanr left a comment

kannappanr Aug 14, 2019

poornas Aug 16, 2019

sinhaashish Aug 19, 2019

poornas Aug 16, 2019

sinhaashish Aug 19, 2019

sinhaashish commented Aug 22, 2019

poornas Aug 22, 2019

sinhaashish Aug 23, 2019

poornas left a comment

kannappanr left a comment

	return "", probe.NewError(errors.New("key should be 32 bytes long or if the key is encoded; then decoded value must be of 32 bytes long"))
	return "", probe.NewError(errors.New("key should be 32 bytes long or if the key is base64 encoded; then decoded value must be 32 bytes long"))
	```

	if e != nil \|\| len(decodedString) != 32 \|\| len(secretValue) != 32 {
	if e != nil \|\| len(decodedString) != 32 {

	return "", probe.NewError(errors.New("key should be 32 bytes long or if the key is base64 encoded; then decoded value must be 32 bytes long"))
	return "", probe.NewError(errors.New("Encryption key should be 32 bytes plain text key or 64 bytes base64 encoded key"))

Add support for passing non-printable character as key #2851

Add support for passing non-printable character as key #2851

Conversation

sinhaashish commented Aug 6, 2019

codecov-io commented Aug 7, 2019 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poornas Aug 9, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kannappanr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sinhaashish commented Aug 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poornas left a comment

Choose a reason for hiding this comment

kannappanr left a comment

Choose a reason for hiding this comment

codecov-io commented Aug 7, 2019 •

edited

poornas Aug 9, 2019 •

edited