From 3fc7673b56604f1eed51ac4e85d258929804bb83 Mon Sep 17 00:00:00 2001 From: Tim Ross Date: Fri, 8 Nov 2024 10:24:03 -0500 Subject: [PATCH] Restore S3 FIPS and custom endpoint capabilities When migrating the S3 events handler to use aws-sdk-go-v2 applying the FIPS settings and custom endpoint were inadvertently dropped. This restores the functionality, while also adding tests to ensure that they are always respected going forward. A similar test was added to the dynamodb events handler as well to prevent any regressions with FIPS settings there. --- lib/events/dynamoevents/dynamoevents.go | 31 +++--- lib/events/dynamoevents/dynamoevents_test.go | 76 +++++++++++---- lib/events/s3sessions/s3handler.go | 95 +++++++++++-------- .../s3sessions/s3handler_config_test.go | 64 +++++++++++++ lib/events/s3sessions/s3handler_test.go | 7 -- .../s3sessions/s3handler_thirdparty_test.go | 9 +- lib/modules/test.go | 4 +- lib/utils/aws/s3.go | 7 ++ 8 files changed, 206 insertions(+), 87 deletions(-) diff --git a/lib/events/dynamoevents/dynamoevents.go b/lib/events/dynamoevents/dynamoevents.go index ba55ef5068768..dcfccc7713e20 100644 --- a/lib/events/dynamoevents/dynamoevents.go +++ b/lib/events/dynamoevents/dynamoevents.go @@ -43,7 +43,6 @@ import ( "github.com/aws/aws-sdk-go-v2/service/dynamodb" dynamodbtypes "github.com/aws/aws-sdk-go-v2/service/dynamodb/types" "github.com/aws/smithy-go" - smithyendpoints "github.com/aws/smithy-go/endpoints" "github.com/google/uuid" "github.com/gravitational/trace" "github.com/jonboulle/clockwork" @@ -148,6 +147,9 @@ type Config struct { // EnableAutoScaling is used to enable auto scaling policy. EnableAutoScaling bool + + // CredentialsProvider if supplied is used to override the credentials source. + CredentialsProvider aws.CredentialsProvider } // SetFromURL sets values on the Config from the supplied URI @@ -282,24 +284,20 @@ func New(ctx context.Context, cfg Config) (*Log, error) { config.WithAPIOptions(dynamometrics.MetricsMiddleware(dynamometrics.Backend)), } - awsConfig, err := config.LoadDefaultConfig(ctx, opts...) - if err != nil { - return nil, trace.Wrap(err) + if cfg.CredentialsProvider != nil { + opts = append(opts, config.WithCredentialsProvider(cfg.CredentialsProvider)) } - otelaws.AppendMiddlewares(&awsConfig.APIOptions, otelaws.WithAttributeSetter(otelaws.DynamoDBAttributeSetter)) - var dynamoOpts []func(*dynamodb.Options) // Override the service endpoint using the "endpoint" query parameter from // "audit_events_uri". This is for non-AWS DynamoDB-compatible backends. if cfg.Endpoint != "" { - u, err := url.Parse(cfg.Endpoint) - if err != nil { + if _, err := url.Parse(cfg.Endpoint); err != nil { return nil, trace.BadParameter("configured DynamoDB events endpoint is invalid: %s", err.Error()) } - dynamoOpts = append(dynamoOpts, dynamodb.WithEndpointResolverV2(&staticResolver{endpoint: u})) + opts = append(opts, config.WithBaseEndpoint(cfg.Endpoint)) } // FIPS settings are applied on the individual service instead of the aws config, @@ -311,6 +309,13 @@ func New(ctx context.Context, cfg Config) (*Log, error) { }) } + awsConfig, err := config.LoadDefaultConfig(ctx, opts...) + if err != nil { + return nil, trace.Wrap(err) + } + + otelaws.AppendMiddlewares(&awsConfig.APIOptions, otelaws.WithAttributeSetter(otelaws.DynamoDBAttributeSetter)) + b := &Log{ logger: l, Config: cfg, @@ -324,14 +329,6 @@ func New(ctx context.Context, cfg Config) (*Log, error) { return b, nil } -type staticResolver struct { - endpoint *url.URL -} - -func (s *staticResolver) ResolveEndpoint(ctx context.Context, params dynamodb.EndpointParameters) (smithyendpoints.Endpoint, error) { - return smithyendpoints.Endpoint{URI: *s.endpoint}, nil -} - type tableStatus int const ( diff --git a/lib/events/dynamoevents/dynamoevents_test.go b/lib/events/dynamoevents/dynamoevents_test.go index 27804a14143a8..2d92ae6ab7432 100644 --- a/lib/events/dynamoevents/dynamoevents_test.go +++ b/lib/events/dynamoevents/dynamoevents_test.go @@ -32,6 +32,7 @@ import ( "testing" "time" + "github.com/aws/aws-sdk-go-v2/aws" "github.com/google/uuid" "github.com/gravitational/trace" "github.com/jonboulle/clockwork" @@ -43,6 +44,7 @@ import ( apievents "github.com/gravitational/teleport/api/types/events" "github.com/gravitational/teleport/lib/events" "github.com/gravitational/teleport/lib/events/test" + "github.com/gravitational/teleport/lib/modules" "github.com/gravitational/teleport/lib/session" "github.com/gravitational/teleport/lib/utils" ) @@ -604,24 +606,62 @@ func randStringAlpha(n int) string { return string(b) } -func TestCustomEndpoint(t *testing.T) { - ctx := context.Background() - t.Setenv("AWS_ACCESS_KEY", "llama") - t.Setenv("AWS_SECRET_KEY", "alpaca") +func TestEndpoints(t *testing.T) { + tests := []struct { + name string + fips bool + }{ + { + name: "fips", + fips: true, + }, + { + name: "without fips", + }, + } - mux := http.NewServeMux() - mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { - w.WriteHeader(http.StatusTeapot) - }) - srv := httptest.NewServer(mux) - defer srv.Close() + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { - b, err := New(ctx, Config{ - Tablename: "teleport-test", - UIDGenerator: utils.NewFakeUID(), - Endpoint: srv.URL, - }) - assert.Error(t, err) - assert.Nil(t, b) - require.ErrorContains(t, err, fmt.Sprintf("StatusCode: %d", http.StatusTeapot)) + fips := types.ClusterAuditConfigSpecV2_FIPS_DISABLED + if tt.fips { + fips = types.ClusterAuditConfigSpecV2_FIPS_ENABLED + modules.SetTestModules(t, &modules.TestModules{ + FIPS: true, + }) + } + + var request *http.Request + mux := http.NewServeMux() + mux.Handle("/", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + request = r + w.WriteHeader(http.StatusTeapot) + })) + + server := httptest.NewServer(mux) + t.Cleanup(server.Close) + + b, err := New(context.Background(), Config{ + Region: "us-west-1", + Tablename: "teleport-test", + UIDGenerator: utils.NewFakeUID(), + Endpoint: server.URL, + UseFIPSEndpoint: fips, + CredentialsProvider: aws.CredentialsProviderFunc(func(ctx context.Context) (aws.Credentials, error) { + return aws.Credentials{}, nil + }), + }) + // FIPS mode should fail because it is a violation to enable FIPS + // while also setting a custom endpoint. + if tt.fips { + require.Error(t, err) + return + } + + assert.Error(t, err) + assert.Nil(t, b) + require.ErrorContains(t, err, fmt.Sprintf("StatusCode: %d", http.StatusTeapot)) + require.Equal(t, "/", request.URL.Path) + }) + } } diff --git a/lib/events/s3sessions/s3handler.go b/lib/events/s3sessions/s3handler.go index 3aec6ab233774..b202e78422c8d 100644 --- a/lib/events/s3sessions/s3handler.go +++ b/lib/events/s3sessions/s3handler.go @@ -43,6 +43,7 @@ import ( "github.com/gravitational/teleport/api/types" "github.com/gravitational/teleport/lib/defaults" "github.com/gravitational/teleport/lib/events" + "github.com/gravitational/teleport/lib/modules" awsmetrics "github.com/gravitational/teleport/lib/observability/metrics/aws" "github.com/gravitational/teleport/lib/session" awsutils "github.com/gravitational/teleport/lib/utils/aws" @@ -77,8 +78,6 @@ type Config struct { Endpoint string // ACL is the canned ACL to send to S3 ACL string - // AWSConfig is an optional existing AWS client configuration - AWSConfig *aws.Config // CredentialsProvider if supplied is used in tests or with External Audit Storage. CredentialsProvider aws.CredentialsProvider // SSEKMSKey specifies the optional custom CMK used for KMS SSE. @@ -157,55 +156,66 @@ func (s *Config) CheckAndSetDefaults() error { return trace.BadParameter("missing parameter Bucket") } - if s.AWSConfig == nil { - var err error - opts := []func(*config.LoadOptions) error{ - config.WithRegion(s.Region), - } + return nil +} - if s.Insecure { - opts = append(opts, config.WithHTTPClient(&http.Client{ - Transport: &http.Transport{ - TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, - }, - })) - } else { - hc, err := defaults.HTTPClient() - if err != nil { - return trace.Wrap(err) - } +// NewHandler returns new S3 uploader +func NewHandler(ctx context.Context, cfg Config) (*Handler, error) { + if err := cfg.CheckAndSetDefaults(); err != nil { + return nil, trace.Wrap(err) + } - opts = append(opts, config.WithHTTPClient(hc)) - } + opts := []func(*config.LoadOptions) error{ + config.WithRegion(cfg.Region), + } - if s.CredentialsProvider != nil { - opts = append(opts, config.WithCredentialsProvider(s.CredentialsProvider)) + if cfg.Insecure { + opts = append(opts, config.WithHTTPClient(&http.Client{ + Transport: &http.Transport{ + TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, + }, + })) + } else { + hc, err := defaults.HTTPClient() + if err != nil { + return nil, trace.Wrap(err) } - opts = append(opts, config.WithAPIOptions(awsmetrics.MetricsMiddleware())) + opts = append(opts, config.WithHTTPClient(hc)) + } - awsConfig, err := config.LoadDefaultConfig(context.Background(), opts...) - if err != nil { - return trace.Wrap(err) + if cfg.CredentialsProvider != nil { + opts = append(opts, config.WithCredentialsProvider(cfg.CredentialsProvider)) + } + + opts = append(opts, config.WithAPIOptions(awsmetrics.MetricsMiddleware())) + + var s3Opts []func(*s3.Options) + if cfg.Endpoint != "" { + if _, err := url.Parse(cfg.Endpoint); err != nil { + return nil, trace.BadParameter("configured S3 endpoint is invalid: %s", err.Error()) } - s.AWSConfig = &awsConfig + opts = append(opts, config.WithBaseEndpoint(cfg.Endpoint)) + + s3Opts = append(s3Opts, func(options *s3.Options) { + options.UsePathStyle = true + }) } - return nil -} -// NewHandler returns new S3 uploader -func NewHandler(ctx context.Context, cfg Config) (*Handler, error) { - if err := cfg.CheckAndSetDefaults(); err != nil { + if modules.GetModules().IsBoringBinary() && cfg.UseFIPSEndpoint == types.ClusterAuditConfigSpecV2_FIPS_ENABLED { + s3Opts = append(s3Opts, func(options *s3.Options) { + options.EndpointOptions.UseFIPSEndpoint = aws.FIPSEndpointStateEnabled + }) + } + + awsConfig, err := config.LoadDefaultConfig(context.Background(), opts...) + if err != nil { return nil, trace.Wrap(err) } // Create S3 client with custom options - client := s3.NewFromConfig(*cfg.AWSConfig, func(o *s3.Options) { - if cfg.Endpoint != "" { - o.UsePathStyle = true - } - }) + client := s3.NewFromConfig(awsConfig, s3Opts...) uploader := manager.NewUploader(client) downloader := manager.NewDownloader(client) @@ -382,14 +392,17 @@ func (h *Handler) ensureBucket(ctx context.Context) error { Bucket: aws.String(h.Bucket), }) err = awsutils.ConvertS3Error(err) - // assumes that bucket is administered by other entity - if err == nil { + switch { + case err == nil: + // assumes that bucket is administered by other entity return nil - } - if !trace.IsNotFound(err) { + case trace.IsBadParameter(err): + return trace.Wrap(err) + case !trace.IsNotFound(err): h.logger.ErrorContext(ctx, "Failed to ensure that S3 bucket exists. S3 session uploads may fail. If you've set up the bucket already and gave Teleport write-only access, feel free to ignore this error.", "bucket", h.Bucket, "error", err) return nil } + input := &s3.CreateBucketInput{ Bucket: aws.String(h.Bucket), ACL: awstypes.BucketCannedACLPrivate, diff --git a/lib/events/s3sessions/s3handler_config_test.go b/lib/events/s3sessions/s3handler_config_test.go index fac2754afcb6d..3e421ca76f85f 100644 --- a/lib/events/s3sessions/s3handler_config_test.go +++ b/lib/events/s3sessions/s3handler_config_test.go @@ -20,13 +20,17 @@ package s3sessions import ( "context" + "net/http" + "net/http/httptest" "net/url" "os" "testing" + "github.com/aws/aws-sdk-go-v2/aws" "github.com/stretchr/testify/require" "github.com/gravitational/teleport/api/types" + "github.com/gravitational/teleport/lib/modules" "github.com/gravitational/teleport/lib/utils" ) @@ -133,3 +137,63 @@ func TestUploadMetadata(t *testing.T) { meta := handler.GetUploadMetadata("test-session-id") require.Equal(t, "s3://teleport-unit-tests/test/test-session-id", meta.URL) } + +func TestEndpoints(t *testing.T) { + tests := []struct { + name string + fips bool + }{ + { + name: "fips", + fips: true, + }, + { + name: "without fips", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fips := types.ClusterAuditConfigSpecV2_FIPS_DISABLED + if tt.fips { + fips = types.ClusterAuditConfigSpecV2_FIPS_ENABLED + modules.SetTestModules(t, &modules.TestModules{ + FIPS: true, + }) + } + + var request *http.Request + mux := http.NewServeMux() + mux.Handle("/", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + request = r + w.WriteHeader(http.StatusTeapot) + })) + + server := httptest.NewServer(mux) + t.Cleanup(server.Close) + + handler, err := NewHandler(context.Background(), Config{ + Region: "us-west-1", + Path: "/test/", + Bucket: "teleport-unit-tests", + Endpoint: server.URL, + UseFIPSEndpoint: fips, + Insecure: true, + CredentialsProvider: aws.CredentialsProviderFunc(func(ctx context.Context) (aws.Credentials, error) { + return aws.Credentials{}, nil + }), + }) + // FIPS mode should fail because it is a violation to enable FIPS + // while also setting a custom endpoint. + if tt.fips { + require.Error(t, err) + return + } + + require.NoError(t, err) + defer handler.Close() + + require.Equal(t, "/teleport-unit-tests", request.URL.Path) + }) + } +} diff --git a/lib/events/s3sessions/s3handler_test.go b/lib/events/s3sessions/s3handler_test.go index 4058b168bda4d..08c8c37b5719b 100644 --- a/lib/events/s3sessions/s3handler_test.go +++ b/lib/events/s3sessions/s3handler_test.go @@ -25,20 +25,13 @@ import ( "context" "fmt" "net/url" - "os" "testing" "github.com/stretchr/testify/require" "github.com/gravitational/teleport/lib/events/test" - "github.com/gravitational/teleport/lib/utils" ) -func TestMain(m *testing.M) { - utils.InitLoggerForTests() - os.Exit(m.Run()) -} - // TestStreams tests various streaming upload scenarios func TestStreams(t *testing.T) { handler, err := NewHandler(context.Background(), Config{ diff --git a/lib/events/s3sessions/s3handler_thirdparty_test.go b/lib/events/s3sessions/s3handler_thirdparty_test.go index 9ee89adbb30ac..16c7b487dda14 100644 --- a/lib/events/s3sessions/s3handler_thirdparty_test.go +++ b/lib/events/s3sessions/s3handler_thirdparty_test.go @@ -25,7 +25,6 @@ import ( "testing" "github.com/aws/aws-sdk-go-v2/aws" - "github.com/aws/aws-sdk-go-v2/credentials" "github.com/aws/aws-sdk-go-v2/service/s3" "github.com/google/uuid" "github.com/gravitational/trace" @@ -48,7 +47,9 @@ func TestThirdpartyStreams(t *testing.T) { bucketName := fmt.Sprintf("teleport-test-%v", uuid.New().String()) config := aws.Config{ - Credentials: credentials.NewStaticCredentialsProvider("YOUR-ACCESSKEYID", "YOUR-SECRETACCESSKEY", ""), + Credentials: aws.CredentialsProviderFunc(func(ctx context.Context) (aws.Credentials, error) { + return aws.Credentials{}, nil + }), Region: "us-west-1", BaseEndpoint: aws.String(server.URL), } @@ -64,12 +65,14 @@ func TestThirdpartyStreams(t *testing.T) { require.NoError(t, err) handler, err := NewHandler(context.Background(), Config{ - AWSConfig: &config, Region: "us-west-1", Path: "/test/", Bucket: bucketName, Endpoint: server.URL, DisableServerSideEncryption: true, + CredentialsProvider: aws.CredentialsProviderFunc(func(ctx context.Context) (aws.Credentials, error) { + return aws.Credentials{}, nil + }), }) require.NoError(t, err) diff --git a/lib/modules/test.go b/lib/modules/test.go index ac25205d47895..82a4afd24d53f 100644 --- a/lib/modules/test.go +++ b/lib/modules/test.go @@ -41,6 +41,8 @@ type TestModules struct { TestBuildType string // TestFeatures is returned from the Features function. TestFeatures Features + // FIPS allows tests to toggle fips behavior. + FIPS bool defaultModules @@ -80,7 +82,7 @@ func (m *TestModules) PrintVersion() { // IsBoringBinary checks if the binary was compiled with BoringCrypto. func (m *TestModules) IsBoringBinary() bool { - return m.defaultModules.IsBoringBinary() + return m.FIPS } // Features returns supported features. diff --git a/lib/utils/aws/s3.go b/lib/utils/aws/s3.go index f42871fb417d7..f54f7f4c68e4c 100644 --- a/lib/utils/aws/s3.go +++ b/lib/utils/aws/s3.go @@ -23,6 +23,7 @@ import ( "errors" "io" "net/http" + "strings" awsv2 "github.com/aws/aws-sdk-go-v2/aws" managerv2 "github.com/aws/aws-sdk-go-v2/feature/s3/manager" @@ -30,6 +31,7 @@ import ( s3types "github.com/aws/aws-sdk-go-v2/service/s3/types" "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/service/s3" + "github.com/aws/smithy-go" "github.com/gravitational/trace" ) @@ -84,6 +86,11 @@ func ConvertS3Error(err error, args ...interface{}) error { return trace.NotFound(notFound.Error(), args...) } + var opError *smithy.OperationError + if errors.As(err, &opError) && strings.Contains(opError.Err.Error(), "FIPS") { + return trace.BadParameter(opError.Error()) + } + return err }