-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Feature/s3 bucket encryption - Implements PR #4235 #5194
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,16 +40,21 @@ var ( | |
s3UrlRegexp = regexp.MustCompile(`s3([-.](?P<region>\w{2}-\w+-\d{1})|[-.](?P<bucket>[\w.\-\_]+)|)?.amazonaws.com(.cn)?(?P<path>.*)?`) | ||
) | ||
|
||
type S3BucketDetails struct { | ||
region string | ||
defaultEncryption bool | ||
} | ||
|
||
type S3Context struct { | ||
mutex sync.Mutex | ||
clients map[string]*s3.S3 | ||
bucketLocations map[string]string | ||
mutex sync.Mutex | ||
clients map[string]*s3.S3 | ||
bucketDetails map[string]*S3BucketDetails | ||
} | ||
|
||
func NewS3Context() *S3Context { | ||
return &S3Context{ | ||
clients: make(map[string]*s3.S3), | ||
bucketLocations: make(map[string]string), | ||
clients: make(map[string]*s3.S3), | ||
bucketDetails: make(map[string]*S3BucketDetails), | ||
} | ||
} | ||
|
||
|
@@ -106,26 +111,29 @@ func getCustomS3Config(endpoint string, region string) (*aws.Config, error) { | |
return s3Config, nil | ||
} | ||
|
||
func (s *S3Context) getRegionForBucket(bucket string) (string, error) { | ||
region := func() string { | ||
func (s *S3Context) getDetailsForBucket(bucket string) (*S3BucketDetails, error) { | ||
bucketDetails := func() *S3BucketDetails { | ||
s.mutex.Lock() | ||
defer s.mutex.Unlock() | ||
return s.bucketLocations[bucket] | ||
return s.bucketDetails[bucket] | ||
}() | ||
|
||
if region != "" { | ||
return region, nil | ||
if bucketDetails != nil && bucketDetails.region != "" { | ||
return bucketDetails, nil | ||
} | ||
|
||
bucketDetails = &S3BucketDetails{region: "", defaultEncryption: false} | ||
|
||
// Probe to find correct region for bucket | ||
endpoint := os.Getenv("S3_ENDPOINT") | ||
if endpoint != "" { | ||
// If customized S3 storage is set, return user-defined region | ||
region = os.Getenv("S3_REGION") | ||
if region == "" { | ||
region = "us-east-1" | ||
bucketDetails.region = os.Getenv("S3_REGION") | ||
if bucketDetails.region == "" { | ||
bucketDetails.region = "us-east-1" | ||
bucketDetails.defaultEncryption = s.checkDefaultEncryption(bucketDetails.region, bucket) | ||
} | ||
return region, nil | ||
return bucketDetails, nil | ||
} | ||
|
||
awsRegion := os.Getenv("AWS_REGION") | ||
|
@@ -134,7 +142,7 @@ func (s *S3Context) getRegionForBucket(bucket string) (string, error) { | |
} | ||
|
||
if err := validateRegion(awsRegion); err != nil { | ||
return "", err | ||
return bucketDetails, err | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Usually better to return |
||
} | ||
|
||
request := &s3.GetBucketLocationInput{ | ||
|
@@ -144,7 +152,7 @@ func (s *S3Context) getRegionForBucket(bucket string) (string, error) { | |
|
||
s3Client, err := s.getClient(awsRegion) | ||
if err != nil { | ||
return "", fmt.Errorf("error connecting to S3: %s", err) | ||
return bucketDetails, fmt.Errorf("error connecting to S3: %s", err) | ||
} | ||
// Attempt one GetBucketLocation call the "normal" way (i.e. as the bucket owner) | ||
response, err = s3Client.GetBucketLocation(request) | ||
|
@@ -156,26 +164,61 @@ func (s *S3Context) getRegionForBucket(bucket string) (string, error) { | |
} | ||
|
||
if err != nil { | ||
return "", err | ||
return bucketDetails, err | ||
} | ||
|
||
if response.LocationConstraint == nil { | ||
// US Classic does not return a region | ||
region = "us-east-1" | ||
bucketDetails.region = "us-east-1" | ||
bucketDetails.defaultEncryption = s.checkDefaultEncryption(bucketDetails.region, bucket) | ||
} else { | ||
region = *response.LocationConstraint | ||
bucketDetails.region = *response.LocationConstraint | ||
// Another special case: "EU" can mean eu-west-1 | ||
if region == "EU" { | ||
region = "eu-west-1" | ||
if bucketDetails.region == "EU" { | ||
bucketDetails.region = "eu-west-1" | ||
} | ||
bucketDetails.defaultEncryption = s.checkDefaultEncryption(bucketDetails.region, bucket) | ||
} | ||
glog.V(2).Infof("Found bucket %q in region %q", bucket, region) | ||
glog.V(2).Infof("Found bucket %q in region %q with default encryption set to %t", bucket, bucketDetails.region, bucketDetails.defaultEncryption) | ||
|
||
s.mutex.Lock() | ||
defer s.mutex.Unlock() | ||
s.bucketLocations[bucket] = region | ||
s.bucketDetails[bucket] = bucketDetails | ||
|
||
return bucketDetails, nil | ||
} | ||
|
||
func (s *S3Context) checkDefaultEncryption(region string, bucket string) bool { | ||
client, err := s.getClient(region) | ||
if err != nil { | ||
glog.Warningf("Unable to read bucket encryption policy in region %q: will encrypt using AES256", region) | ||
return false | ||
} | ||
|
||
glog.V(4).Infof("Checking default bucket encryption %q", bucket) | ||
|
||
request := &s3.GetBucketEncryptionInput{} | ||
request.Bucket = aws.String(bucket) | ||
|
||
glog.V(8).Infof("Calling S3 GetBucketEncryption Bucket=%q", bucket) | ||
|
||
result, err := client.GetBucketEncryption(request) | ||
if err != nil { | ||
// the following cases might lead to the operation failing: | ||
// 1. A deny policy on s3:GetEncryptionConfiguration | ||
// 2. No default encryption policy set | ||
glog.Warningf("Unable to read bucket encryption policy: will encrypt using AES256") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As a style thing, IMO it's often clearer to have the function return the value & error, and then have the caller decide whether to ignore the error. |
||
return false | ||
} | ||
|
||
// currently, only one element is in the rules array, iterating nonetheless for future compatibility | ||
for _, element := range result.ServerSideEncryptionConfiguration.Rules { | ||
if element.ApplyServerSideEncryptionByDefault != nil { | ||
return true | ||
} | ||
} | ||
|
||
return region, nil | ||
return false | ||
} | ||
|
||
/* | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,11 +34,11 @@ import ( | |
) | ||
|
||
type S3Path struct { | ||
s3Context *S3Context | ||
bucket string | ||
region string | ||
key string | ||
etag *string | ||
s3Context *S3Context | ||
bucket string | ||
bucketDetails *S3BucketDetails | ||
key string | ||
etag *string | ||
|
||
// scheme is configurable in case an S3 compatible custom | ||
// endpoint is specified | ||
|
@@ -134,7 +134,9 @@ func (p *S3Path) WriteFile(data io.ReadSeeker, aclObj ACL) error { | |
request.Body = data | ||
request.Bucket = aws.String(p.bucket) | ||
request.Key = aws.String(p.key) | ||
if p.sse { | ||
|
||
// only support SSE if a custom endpoint is not provided | ||
if !p.bucketDetails.defaultEncryption { | ||
request.ServerSideEncryption = aws.String(sse) | ||
} | ||
|
||
|
@@ -153,7 +155,11 @@ func (p *S3Path) WriteFile(data io.ReadSeeker, aclObj ACL) error { | |
|
||
// We don't need Content-MD5: https://github.com/aws/aws-sdk-go/issues/208 | ||
|
||
glog.V(8).Infof("Calling S3 PutObject Bucket=%q Key=%q SSE=%q ACL=%q", p.bucket, p.key, sse, acl) | ||
if p.bucketDetails.defaultEncryption { | ||
glog.V(8).Infof("Calling S3 PutObject Bucket=%q Key=%q ACL=%q with DefaultBucketEncryption", p.bucket, p.key, acl) | ||
} else { | ||
glog.V(8).Infof("Calling S3 PutObject Bucket=%q Key=%q SSE=%q ACL=%q", p.bucket, p.key, sse, acl) | ||
} | ||
|
||
_, err = client.PutObject(request) | ||
if err != nil { | ||
|
@@ -315,14 +321,16 @@ func (p *S3Path) ReadTree() ([]Path, error) { | |
|
||
func (p *S3Path) client() (*s3.S3, error) { | ||
var err error | ||
if p.region == "" { | ||
p.region, err = p.s3Context.getRegionForBucket(p.bucket) | ||
if p.bucketDetails == nil || p.bucketDetails.region == "" { | ||
bucketDetails, err := p.s3Context.getDetailsForBucket(p.bucket) | ||
|
||
p.bucketDetails = bucketDetails | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah I see what you're doing now returning the bucketDetails always. It's a slightly hard pattern to reason about though - because a retry will succeed I think. |
||
if err != nil { | ||
return nil, err | ||
} | ||
} | ||
|
||
client, err := p.s3Context.getClient(p.region) | ||
client, err := p.s3Context.getClient(p.bucketDetails.region) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we set defaultEncryption if S3_REGION is set? Also we aren't caching it here...