Skip to content

Commit

Permalink
[access_graph] s3: report s3 bucket when failed to pull bucket tags (#…
Browse files Browse the repository at this point in the history 
…46592)

When the bucket tags pull failes due to missing permissions, we returned early and existing was empty. This caused the s3 buckets not being reported to TAG.

Signed-off-by: Tiago Silva <tiago.silva@goteleport.com>
  • Loading branch information
@tigrato
tigrato authored Sep 13, 2024
1 parent 4398749 commit a0a903f
Show file tree
Hide file tree
Showing 2 changed files with 159 additions and 11 deletions.
63 changes: 52 additions & 11 deletions lib/srv/discovery/fetchers/aws-sync/s3.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"github.com/aws/aws-sdk-go/service/s3"
"github.com/gravitational/trace"
"golang.org/x/sync/errgroup"
"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/types/known/timestamppb"

accessgraphv1alpha "github.com/gravitational/teleport/gen/proto/go/accessgraph/v1alpha"
Expand Down Expand Up @@ -94,6 +95,8 @@ func (a *awsFetcher) fetchS3Buckets(ctx context.Context) ([]*accessgraphv1alpha.
for _, bucket := range rsp.Buckets {
bucket := bucket
eG.Go(func() error {
var failedReqs failedRequests
var errs []error
existingBucket := sliceFilterPickFirst(existing.S3Buckets, func(b *accessgraphv1alpha.AWSS3BucketV1) bool {
return b.Name == aws.ToString(bucket.Name) && b.AccountId == a.AccountID
},
Expand All @@ -102,24 +105,30 @@ func (a *awsFetcher) fetchS3Buckets(ctx context.Context) ([]*accessgraphv1alpha.
Bucket: bucket.Name,
})
if err != nil {
collect(existingBucket, trace.Wrap(err, "failed to fetch bucket %q inline policy", aws.ToString(bucket.Name)))
return nil
errs = append(errs,
trace.Wrap(err, "failed to fetch bucket %q inline policy", aws.ToString(bucket.Name)),
)
failedReqs.policyFailed = true
}

policyStatus, err := s3Client.GetBucketPolicyStatusWithContext(ctx, &s3.GetBucketPolicyStatusInput{
Bucket: bucket.Name,
})
if err != nil {
collect(existingBucket, trace.Wrap(err, "failed to fetch bucket %q policy status", aws.ToString(bucket.Name)))
return nil
errs = append(errs,
trace.Wrap(err, "failed to fetch bucket %q policy status", aws.ToString(bucket.Name)),
)
failedReqs.failedPolicyStatus = true
}

acls, err := s3Client.GetBucketAclWithContext(ctx, &s3.GetBucketAclInput{
Bucket: bucket.Name,
})
if err != nil {
collect(existingBucket, trace.Wrap(err, "failed to fetch bucket %q acls policies", aws.ToString(bucket.Name)))
return nil
errs = append(errs,
trace.Wrap(err, "failed to fetch bucket %q acls policies", aws.ToString(bucket.Name)),
)
failedReqs.failedAcls = true
}

tagsOutput, err := s3Client.GetBucketTaggingWithContext(ctx, &s3.GetBucketTaggingInput{
Expand All @@ -132,13 +141,14 @@ func (a *awsFetcher) fetchS3Buckets(ctx context.Context) ([]*accessgraphv1alpha.
err = nil
}
if err != nil {
collect(existingBucket, trace.Wrap(err, "failed to fetch bucket %q tags", aws.ToString(bucket.Name)))
return nil
errs = append(errs,
trace.Wrap(err, "failed to fetch bucket %q tags", aws.ToString(bucket.Name)),
)
failedReqs.failedTags = true
}

collect(
awsS3Bucket(aws.ToString(bucket.Name), policy, policyStatus, acls, tagsOutput, a.AccountID),
nil)
newBucket := awsS3Bucket(aws.ToString(bucket.Name), policy, policyStatus, acls, tagsOutput, a.AccountID)
collect(mergeS3Protos(existingBucket, newBucket, failedReqs), trace.NewAggregate(errs...))
return nil
})
}
Expand Down Expand Up @@ -196,3 +206,34 @@ func awsACLsToProtoACLs(grants []*s3.Grant) []*accessgraphv1alpha.AWSS3BucketACL
}
return acls
}

type failedRequests struct {
policyFailed bool
failedPolicyStatus bool
failedAcls bool
failedTags bool
}

func mergeS3Protos(existing, new *accessgraphv1alpha.AWSS3BucketV1, failedReqs failedRequests) *accessgraphv1alpha.AWSS3BucketV1 {
if existing == nil {
return new
}
if new == nil {
return existing
}
clone := proto.Clone(new).(*accessgraphv1alpha.AWSS3BucketV1)
if failedReqs.policyFailed {
clone.PolicyDocument = existing.PolicyDocument
}
if failedReqs.failedPolicyStatus {
clone.IsPublic = existing.IsPublic
}
if failedReqs.failedAcls {
clone.Acls = existing.Acls
}
if failedReqs.failedTags {
clone.Tags = existing.Tags
}

return clone
}
107 changes: 107 additions & 0 deletions lib/srv/discovery/fetchers/aws-sync/s3_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
"github.com/gravitational/trace"
"github.com/stretchr/testify/require"
"google.golang.org/protobuf/testing/protocmp"
"google.golang.org/protobuf/types/known/timestamppb"

accessgraphv1alpha "github.com/gravitational/teleport/gen/proto/go/accessgraph/v1alpha"
"github.com/gravitational/teleport/lib/cloud"
Expand Down Expand Up @@ -200,3 +201,109 @@ func s3Buckets(bucketNames ...string) []*s3.Bucket {
}
return output
}

// Helper function to create AWSS3BucketV1 for testing
func createAWSS3Bucket(name, accountID string, policyDocument []byte, isPublic bool, lastSync time.Time) *accessgraphv1alpha.AWSS3BucketV1 {
return &accessgraphv1alpha.AWSS3BucketV1{
Name: name,
AccountId: accountID,
PolicyDocument: policyDocument,
IsPublic: isPublic,
LastSyncTime: timestamppb.New(lastSync),
}
}

func TestMergeS3Protos(t *testing.T) {
// Define a common time for the test
lastSync := time.Now()

// Define test cases in a table-driven format
tests := []struct {
name string
existing *accessgraphv1alpha.AWSS3BucketV1
new *accessgraphv1alpha.AWSS3BucketV1
failedReqs failedRequests
expected *accessgraphv1alpha.AWSS3BucketV1
}{
{
name: "Both existing and new are nil",
existing: nil,
new: nil,
failedReqs: failedRequests{
policyFailed: false,
failedPolicyStatus: false,
failedAcls: false,
failedTags: false,
},
expected: nil,
},
{
name: "Existing is nil, new is non-nil",
existing: nil,
new: createAWSS3Bucket("new-bucket", "account-1", []byte("policy"), true, lastSync),
failedReqs: failedRequests{
policyFailed: false,
failedPolicyStatus: false,
failedAcls: false,
failedTags: false,
},
expected: createAWSS3Bucket("new-bucket", "account-1", []byte("policy"), true, lastSync),
},
{
name: "New is nil, existing is non-nil",
existing: createAWSS3Bucket("existing-bucket", "account-1", []byte("existing-policy"), false, lastSync),
new: nil,
failedReqs: failedRequests{
policyFailed: false,
failedPolicyStatus: false,
failedAcls: false,
failedTags: false,
},
expected: createAWSS3Bucket("existing-bucket", "account-1", []byte("existing-policy"), false, lastSync),
},
{
name: "New and existing both non-nil, no failures",
existing: createAWSS3Bucket("existing-bucket", "account-1", []byte("existing-policy"), false, lastSync),
new: createAWSS3Bucket("new-bucket", "account-2", []byte("new-policy"), true, lastSync),
failedReqs: failedRequests{
policyFailed: false,
failedPolicyStatus: false,
failedAcls: false,
failedTags: false,
},
expected: createAWSS3Bucket("new-bucket", "account-2", []byte("new-policy"), true, lastSync),
},
{
name: "Policy merge failed",
existing: createAWSS3Bucket("existing-bucket", "account-1", []byte("existing-policy"), false, lastSync),
new: createAWSS3Bucket("new-bucket", "account-2", []byte("new-policy"), true, lastSync),
failedReqs: failedRequests{
policyFailed: true,
failedPolicyStatus: false,
failedAcls: false,
failedTags: false,
},
expected: createAWSS3Bucket("new-bucket", "account-2", []byte("existing-policy"), true, lastSync),
},
{
name: "Policy status merge failed",
existing: createAWSS3Bucket("existing-bucket", "account-1", []byte("existing-policy"), false, lastSync),
new: createAWSS3Bucket("new-bucket", "account-2", []byte("new-policy"), true, lastSync),
failedReqs: failedRequests{
policyFailed: false,
failedPolicyStatus: true,
failedAcls: false,
failedTags: false,
},
expected: createAWSS3Bucket("new-bucket", "account-2", []byte("new-policy"), false, lastSync),
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
result := mergeS3Protos(tt.existing, tt.new, tt.failedReqs)

require.Empty(t, cmp.Diff(tt.expected, result, protocmp.Transform()))
})
}
}

0 comments on commit a0a903f

Please sign in to comment.