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

Fix SSE-C source decryption handling #6671

Merged
merged 2 commits into from Oct 19, 2018
Merged

Conversation

harshavardhana
Copy link
Member

@harshavardhana harshavardhana commented Oct 19, 2018

Description

Fix SSE-C source decryption handling

Motivation and Context

Without this fix, we have room for two different type of errors.

  • The source is encrypted and we didn't provide any source encryption keys

This results in Incomplete body error to be returned back to the client
since the source is encrypted and we gave the reader as is to the object
layer which was of a decrypted value leading to "IncompleteBody"

  • The source is not encrypted and we provided source encryption keys.

This results in a corrupted object on the destination which is
considered encrypted but cannot be read by the server and returns
the following error.

<Error><Code>XMinioObjectTampered</Code><Message>The requested object
was modified and may be compromised</Message><Resource>/id-platform-gamma/
</Resource><RequestId>155EDC3E86BFD4DA</RequestId><HostId>3L137</HostId>
</Error>

Regression

Maybe

How Has This Been Tested?

Using the following example code. To start with let's create an encrypted object and an unencrypted object

~ mc cp --encrypt-key="myminio/=32byteslongsecretkeymustbegiven2" /etc/issue myminio/id-platform-gamma/issue-enc
~ mc cp /etc/issue myminio/id-platform-gamma/issue-noenc

Use the following code on the master now if you can see since we are not passing CopySSE keys for an encrypted object, it should throw an error which it does but the error is weird and returns "IncompleteBody" and we can see log messages regarding this. This is the first variation of the bug. There is no destination object that is created. With this PR a proper error is returned requesting proper encryption parameters.

package main

import (
	"crypto/tls"
	"fmt"
	"net/http"

	"github.com/aws/aws-sdk-go/aws"
	"github.com/aws/aws-sdk-go/aws/credentials"
	"github.com/aws/aws-sdk-go/aws/session"
	"github.com/aws/aws-sdk-go/service/s3"
)

func main() {
	creds := credentials.NewStaticCredentials("minio", "minio123", "")
	newSession := session.New()
	http.DefaultTransport.(*http.Transport).TLSClientConfig = &tls.Config{InsecureSkipVerify: true}
	s3Config := &aws.Config{
		Credentials:      creds,
		Endpoint:         aws.String("https://127.0.0.1:9012"),
		Region:           aws.String("us-east-1"),
		S3ForcePathStyle: aws.Bool(true),
		LogLevel:         aws.LogLevel(aws.LogDebug),
	}
	// Create an S3 service object in the default region.
	s3Client := s3.New(newSession, s3Config)
	params := &s3.CopyObjectInput{
		SSECustomerAlgorithm:           aws.String("AES256"),                           // header x-amz-server-side-encryption-customer-algorithm
		SSECustomerKey:                 aws.String("32byteslongsecretkeymustbegiven2"), // header x-amz-server-side-encryption-customer-key
		CopySource:                     aws.String("/id-platform-gamma/issue-enc"),
		Bucket:                         aws.String("id-platform-gamma"),
		Key:                            aws.String("issue1"),
	}
	req, output := s3Client.CopyObjectRequest(params)
	err := req.Send()
	if err != nil {
		fmt.Println(err)
		return
	}
	fmt.Println(output)
}

Use the following code on the master now if you can see since we are passing CopySSE keys for an unencrypted object, it should throw an error but it doesn't and we see that destination object gets created nevertheless and we cannot read the object anymore. But with this PR this doesn't happen and we cannot read the object anymore. This is the second variation of the bug.

package main

import (
	"crypto/tls"
	"fmt"
	"net/http"

	"github.com/aws/aws-sdk-go/aws"
	"github.com/aws/aws-sdk-go/aws/credentials"
	"github.com/aws/aws-sdk-go/aws/session"
	"github.com/aws/aws-sdk-go/service/s3"
)

func main() {
	creds := credentials.NewStaticCredentials("minio", "minio123", "")
	newSession := session.New()
	http.DefaultTransport.(*http.Transport).TLSClientConfig = &tls.Config{InsecureSkipVerify: true}
	s3Config := &aws.Config{
		Credentials:      creds,
		Endpoint:         aws.String("https://127.0.0.1:9012"),
		Region:           aws.String("us-east-1"),
		S3ForcePathStyle: aws.Bool(true),
		LogLevel:         aws.LogLevel(aws.LogDebug),
	}
	// Create an S3 service object in the default region.
	s3Client := s3.New(newSession, s3Config)
	params := &s3.CopyObjectInput{
		CopySourceSSECustomerKey:       aws.String("32byteslongsecretkeymustbegiven2"),
		CopySourceSSECustomerAlgorithm: aws.String("AES256"),
		SSECustomerAlgorithm:           aws.String("AES256"),                           // header x-amz-server-side-encryption-customer-algorithm
		SSECustomerKey:                 aws.String("32byteslongsecretkeymustbegiven2"), // header x-amz-server-side-encryption-customer-key
		CopySource:                     aws.String("/id-platform-gamma/issue-noenc"),
		Bucket:                         aws.String("id-platform-gamma"),
		Key:                            aws.String("issue1"),
	}
	req, output := s3Client.CopyObjectRequest(params)
	err := req.Send()
	if err != nil {
		fmt.Println(err)
		return
	}
	fmt.Println(output)
}

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added unit tests to cover my changes.
  • I have added/updated functional tests in mint. (If yes, add mint PR # here: )
  • All new and existing tests passed.

cmd/object-handlers.go Outdated Show resolved Hide resolved
@harshavardhana
Copy link
Member Author

Similarly, with a little bigger file lets say 3mb, we can reproduce this

$ mc cat --encrypt-key="myminio-local1-ssl/=32byteslongsecretkeymustbegiven2" myminio-local1-ssl/id-platform-gamma/issue1 > /dev/null
mc: <ERROR> Unable to read from `myminio-local1-ssl/id-platform-gamma/issue1`. Input reader closed pre-maturely. Expected `3144192` bytes, but only received `3080192` bytes.

Without this fix we have room for two different type of
errors.
- Source is encrypted and we didn't provide any source encryption keys

This results in Incomplete body error to be returned back to the client
since source is encrypted and we gave the reader as is to the object
layer which was of a decrypted value leading to "IncompleteBody"

- Source is not encrypted and we provided source encryption keys.

This results in a corrupted object on the destination which is
considered encrypted but cannot be read by the server and returns
the following error.

```
<Error><Code>XMinioObjectTampered</Code><Message>The requested object
was modified and may be compromised</Message><Resource>/id-platform-gamma/
</Resource><RequestId>155EDC3E86BFD4DA</RequestId><HostId>3L137</HostId>
</Error>
```
@aead
Copy link
Member

aead commented Oct 19, 2018

@harshavardhana Can you re-run CI? Failures again probably not related...

@codecov
Copy link

codecov bot commented Oct 19, 2018

Codecov Report

Merging #6671 into master will decrease coverage by 0.03%.
The diff coverage is 20%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6671      +/-   ##
==========================================
- Coverage   54.98%   54.94%   -0.04%     
==========================================
  Files         259      259              
  Lines       41317    41326       +9     
==========================================
- Hits        22718    22708      -10     
- Misses      16607    16624      +17     
- Partials     1992     1994       +2
Impacted Files Coverage Δ
cmd/object-handlers.go 54.7% <20%> (-0.24%) ⬇️
cmd/xl-sets.go 56.24% <0%> (-1.53%) ⬇️
cmd/fs-v1-helpers.go 70.03% <0%> (+0.91%) ⬆️

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 0edfb32...15918ac. Read the comment docs.

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.

LGTM, minio-py/minio-go/aws-sdk-java with TLS tests were ran against it

@minio-ops
Copy link

Mint Automation

Test Result
mint-tls.sh ✔️
mint-xl.sh ✔️
mint-gateway-nas.sh ✔️
mint-large-bucket.sh ✔️
mint-worm.sh ✔️
mint-fs.sh ✔️
mint-dist-xl.sh ✔️

@kannappanr kannappanr merged commit 62b5605 into minio:master Oct 19, 2018
@harshavardhana harshavardhana deleted the fix-sse branch October 19, 2018 17:55
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.

None yet

6 participants