change encryption key parsing to allow spaces in sse-c key #2408
Conversation
cmd/cat-main.go
Outdated
| @@ -243,7 +243,7 @@ func mainCat(ctx *cli.Context) error { | |||
| } | |||
|
|
|||
| encKeyDB, err := parseEncryptionKeys(sseKeys) | |||
| fatalIf(err, "Unable to parse encryption keys") | |||
| fatalIf(err, "Unable to parse encryption keys:") | |||
There was a problem hiding this comment.
You don't need to add ":" we do not follow that syntax. any particular reason for this change?
There was a problem hiding this comment.
looked more readable. can revert
cmd/utils.go
Outdated
| vs := 0 // start index of sse-c key | ||
| sseKeyLen := 32 | ||
| delim := 1 | ||
| l := len(sseKeys) |
There was a problem hiding this comment.
suggestion: can we use a different single letter variable? l can be easily mistaken for 1.
cmd/utils.go
Outdated
| encMap[alias] = append(encMap[alias], ps) | ||
| // advance index 33 bytes for the next key start |
There was a problem hiding this comment.
the comment should be,
// advance index by (sseKeyLen + delim) bytes for the next key start index
|
We can use https://golang.org/pkg/encoding/csv/ which supports comma-separated values with spaces in them. This will simplify our encryption keys parsing code. |
|
@krisis, the encryption key can come in with "," as part of the values - which is why the current implementation has been used to reliably take the first 32 bytes from encountering the first '=' sign as the encryption key. |
Codecov Report
@@ Coverage Diff @@
## master #2408 +/- ##
==========================================
+ Coverage 10.2% 10.63% +0.42%
==========================================
Files 107 107
Lines 8347 8362 +15
==========================================
+ Hits 852 889 +37
+ Misses 7351 7325 -26
- Partials 144 148 +4
Continue to review full report at Codecov.
|
poornas
force-pushed
the
encrypt
branch
4 times, most recently
from
2ced834
to
59a39a3
Compare
|
@krisis, added unit tests - PTAL. |
cmd/client-s3.go
Outdated
| for k, v := range key.GetSSEHeaders() { | ||
| opts.Set(k, v) | ||
| } | ||
| key := encrypt.DefaultPBKDF([]byte(sseKey), []byte(bucket+object)) |
There was a problem hiding this comment.
this is not correct @poornas DefaultPBKDF means we are taking password not sseKey as input. This would mean that 32byte long string is no longer a requirement. So you can simplify the code even further if you wish to use password-based style. If not you should use encrypt.NewSSEC for now and we need to think about the UI again to support both key and password styles with @abperiasamy
There was a problem hiding this comment.
fixed. will discuss password-based style with @abperiasamy
appveyor.yml
Outdated
| @@ -17,6 +17,7 @@ install: | |||
| - go version | |||
| - go env | |||
| - python --version | |||
| - go get -u golang.org/x/crypto/argon2 | |||
There was a problem hiding this comment.
this is not needed if you have vendorized it already what is the error that you see? @poornas
poornas
force-pushed
the
encrypt
branch
2 times, most recently
from
088f080
to
02bf49c
Compare
Also vendoring in minio-go updates
The base64 encryption key passed in command line could have spaces. Earlier implementation was splitting on spaces, which could lead to erroneous parsing.