From 53468aada171a48436ed6f31f7453c6d7e2be908 Mon Sep 17 00:00:00 2001 From: Jonathan Rosenberg <96974219+Jonathan-Rosenberg@users.noreply.github.com> Date: Wed, 18 Sep 2024 13:48:59 +0300 Subject: [PATCH] Add presigned url endpoint configuration to AWS (#8170) * add presigned url endpoint configuration to AWS * add docs * change presigned url endpoint type to string + add it to the s3a adapter builder * fix docs * extract presign client creation * add tests * handle test failures * test presign only on supported cloud providers --- docs/reference/configuration.md | 1 + docs/security/presigned-url.md | 5 +++- pkg/block/azure/adapter_test.go | 2 +- pkg/block/blocktest/adapter.go | 41 ++++++++++++++++++++++++++------- pkg/block/factory/build.go | 3 +++ pkg/block/gs/adapter_test.go | 2 +- pkg/block/local/adapter_test.go | 2 +- pkg/block/params/block.go | 1 + pkg/block/s3/adapter.go | 34 ++++++++++++++++++--------- pkg/block/s3/adapter_test.go | 29 +++++++++++++++++++---- pkg/config/config.go | 15 +++++++----- 11 files changed, 101 insertions(+), 34 deletions(-) diff --git a/docs/reference/configuration.md b/docs/reference/configuration.md index 0c6b907f68f..b0e450e0f92 100644 --- a/docs/reference/configuration.md +++ b/docs/reference/configuration.md @@ -182,6 +182,7 @@ Configuration section when using `database.type="local"` * `blockstore.s3.server_side_encryption` `(string : )` - Server side encryption format used (Example on AWS using SSE-KMS while passing "aws:kms") * `blockstore.s3.server_side_encryption_kms_key_id` `(string : )` - Server side encryption KMS key ID * `blockstore.s3.pre_signed_expiry` `(time duration : "15m")` - Expiry of pre-signed URL. +* `blockstore.s3.pre_signed_endpoint` `(string : )` - Custom endpoint for pre-signed URLs. * `blockstore.s3.disable_pre_signed` `(bool : false)` - Disable use of pre-signed URL. * `blockstore.s3.disable_pre_signed_ui` `(bool : true)` - Disable use of pre-signed URL in the UI. * `blockstore.s3.disable_pre_signed_multipart` `(bool : )` - Disable use of pre-signed multipart upload **experimental**, enabled on s3 block adapter with presign support. diff --git a/docs/security/presigned-url.md b/docs/security/presigned-url.md index c15fe57cafc..88716999b37 100644 --- a/docs/security/presigned-url.md +++ b/docs/security/presigned-url.md @@ -12,7 +12,10 @@ redirect_from: With lakeFS, you can access data directly from the storage and not through lakeFS using a presigned URL. Based on the user's access to an object in the object store, the presigned URL will get read or write access. -The presign support is enabled for block adapter that supports it (S3, GCP, Azure), and can be disabled by the [configuration]({% link reference/configuration.md %}) (`blockstore..disable_pre_signed`). Note that the UI support is disabled by default. +The presign support is enabled for block adapter that supports it (AWS, GCP, Azure), and can be disabled by the [configuration]({% link reference/configuration.md %}) (`blockstore..disable_pre_signed`). Note that the UI support is disabled by default. + +- It is possible to override the default pre-signed URL endpoint in **AWS** by setting the [configuration]({% link reference/configuration.md %}) (`blockstore.s3.pre_signed_endpoint`). +This is useful, for example, when you wish to define a [VPC endpoint](https://docs.aws.amazon.com/AmazonS3/latest/userguide/privatelink-interface-endpoints.html#accessing-s3-interface-endpoints) access for the pre-signed URL. ## Using presigned URLs in the UI For using presigned URLs in the UI: diff --git a/pkg/block/azure/adapter_test.go b/pkg/block/azure/adapter_test.go index 8c1c12b6f0e..5b13deb0357 100644 --- a/pkg/block/azure/adapter_test.go +++ b/pkg/block/azure/adapter_test.go @@ -27,7 +27,7 @@ func TestAzureAdapter(t *testing.T) { Domain: domain, }) require.NoError(t, err, "create new adapter") - blocktest.AdapterTest(t, adapter, localPath, externalPath) + blocktest.AdapterTest(t, adapter, localPath, externalPath, true) } func TestAdapterNamespace(t *testing.T) { diff --git a/pkg/block/blocktest/adapter.go b/pkg/block/blocktest/adapter.go index 1b1e4bf61e6..c1a6301afef 100644 --- a/pkg/block/blocktest/adapter.go +++ b/pkg/block/blocktest/adapter.go @@ -18,12 +18,19 @@ import ( ) // AdapterTest Test suite of basic adapter functionality -func AdapterTest(t *testing.T, adapter block.Adapter, storageNamespace, externalPath string) { +func AdapterTest(t *testing.T, adapter block.Adapter, storageNamespace, externalPath string, presigned bool) { AdapterBasicObjectTest(t, adapter, storageNamespace, externalPath) AdapterMultipartTest(t, adapter, storageNamespace, externalPath) t.Run("Adapter_GetRange", func(t *testing.T) { testAdapterGetRange(t, adapter, storageNamespace) }) t.Run("Adapter_Walker", func(t *testing.T) { testAdapterWalker(t, adapter, storageNamespace) }) - t.Run("Adapter_GetPreSignedURL", func(t *testing.T) { testGetPreSignedURL(t, adapter, storageNamespace) }) + if presigned { + t.Run("Adapter_GetPreSignedURL", func(t *testing.T) { testGetPreSignedURL(t, adapter, storageNamespace) }) + } +} + +func AdapterPresignedEndpointOverrideTest(t *testing.T, adapter block.Adapter, storageNamespace, externalPath string, oe *url.URL) { + AdapterBasicObjectTest(t, adapter, storageNamespace, externalPath) + t.Run("Adapter_GetPreSignedURLEndpointOverride", func(t *testing.T) { testGetPreSignedURLEndpointOverride(t, adapter, storageNamespace, oe) }) } // Parameterized test of the GetRange functionality @@ -156,6 +163,27 @@ func testAdapterWalker(t *testing.T, adapter block.Adapter, storageNamespace str // Test request for a presigned URL for temporary access func testGetPreSignedURL(t *testing.T, adapter block.Adapter, storageNamespace string) { + preSignedURL, exp := getPresignedURLBasicTest(t, adapter, storageNamespace) + require.NotNil(t, exp) + expectedExpiry := expectedURLExp(adapter) + require.Equal(t, expectedExpiry, *exp) + _, err := url.Parse(preSignedURL) + require.NoError(t, err) +} + +// Test request for a presigned URL with an endpoint override +func testGetPreSignedURLEndpointOverride(t *testing.T, adapter block.Adapter, storageNamespace string, oe *url.URL) { + preSignedURL, exp := getPresignedURLBasicTest(t, adapter, storageNamespace) + require.NotNil(t, exp) + expectedExpiry := expectedURLExp(adapter) + require.Equal(t, expectedExpiry, *exp) + u, err := url.Parse(preSignedURL) + require.NoError(t, err) + require.Equal(t, u.Scheme, oe.Scheme) + require.Contains(t, u.Host, oe.Host) +} + +func getPresignedURLBasicTest(t *testing.T, adapter block.Adapter, storageNamespace string) (string, *time.Time) { ctx := context.Background() obj, _ := objPointers(storageNamespace) @@ -163,16 +191,13 @@ func testGetPreSignedURL(t *testing.T, adapter block.Adapter, storageNamespace s if adapter.BlockstoreType() == block.BlockstoreTypeGS { require.ErrorContains(t, err, "no credentials found") - return + return "", nil } else if adapter.BlockstoreType() == block.BlockstoreTypeLocal { require.ErrorIs(t, err, block.ErrOperationNotSupported) - return + return "", nil } require.NoError(t, err) - expectedExpiry := expectedURLExp(adapter) - require.Equal(t, expectedExpiry, exp) - _, err = url.Parse(preSignedURL) - require.NoError(t, err) + return preSignedURL, &exp } func expectedURLExp(adapter block.Adapter) time.Time { diff --git a/pkg/block/factory/build.go b/pkg/block/factory/build.go index 2f8ff21b97a..022af7f2a1e 100644 --- a/pkg/block/factory/build.go +++ b/pkg/block/factory/build.go @@ -115,6 +115,9 @@ func buildS3Adapter(ctx context.Context, statsCollector stats.Collector, params if params.ServerSideEncryptionKmsKeyID != "" { opts = append(opts, s3a.WithServerSideEncryptionKmsKeyID(params.ServerSideEncryptionKmsKeyID)) } + if params.PreSignedEndpoint != "" { + opts = append(opts, s3a.WithPreSignedEndpoint(params.PreSignedEndpoint)) + } adapter, err := s3a.NewAdapter(ctx, params, opts...) if err != nil { return nil, err diff --git a/pkg/block/gs/adapter_test.go b/pkg/block/gs/adapter_test.go index 9afbb212904..bea1bd39146 100644 --- a/pkg/block/gs/adapter_test.go +++ b/pkg/block/gs/adapter_test.go @@ -27,7 +27,7 @@ func TestAdapter(t *testing.T) { require.NoError(t, adapter.Close()) }() - blocktest.AdapterTest(t, adapter, localPath, externalPath) + blocktest.AdapterTest(t, adapter, localPath, externalPath, false) } func TestAdapterNamespace(t *testing.T) { diff --git a/pkg/block/local/adapter_test.go b/pkg/block/local/adapter_test.go index 37bb75768f7..054b13e589c 100644 --- a/pkg/block/local/adapter_test.go +++ b/pkg/block/local/adapter_test.go @@ -22,7 +22,7 @@ func TestLocalAdapter(t *testing.T) { if err != nil { t.Fatal("Failed to create new adapter", err) } - blocktest.AdapterTest(t, adapter, testStorageNamespace, externalPath) + blocktest.AdapterTest(t, adapter, testStorageNamespace, externalPath, false) } // TestAdapterNamespace tests the namespace validity regex with various paths diff --git a/pkg/block/params/block.go b/pkg/block/params/block.go index a1b0da999b5..335a259c808 100644 --- a/pkg/block/params/block.go +++ b/pkg/block/params/block.go @@ -54,6 +54,7 @@ type S3 struct { ServerSideEncryption string ServerSideEncryptionKmsKeyID string PreSignedExpiry time.Duration + PreSignedEndpoint string DisablePreSigned bool DisablePreSignedUI bool DisablePreSignedMultipart bool diff --git a/pkg/block/s3/adapter.go b/pkg/block/s3/adapter.go index 11bce2c2959..7704d6a33ff 100644 --- a/pkg/block/s3/adapter.go +++ b/pkg/block/s3/adapter.go @@ -39,6 +39,7 @@ type Adapter struct { ServerSideEncryption string ServerSideEncryptionKmsKeyID string preSignedExpiry time.Duration + preSignedEndpoint string sessionExpiryWindow time.Duration disablePreSigned bool disablePreSignedUI bool @@ -64,6 +65,12 @@ func WithPreSignedExpiry(v time.Duration) func(a *Adapter) { } } +func WithPreSignedEndpoint(e string) func(a *Adapter) { + return func(a *Adapter) { + a.preSignedEndpoint = e + } +} + func WithDisablePreSigned(b bool) func(a *Adapter) { return func(a *Adapter) { if b { @@ -400,11 +407,7 @@ func (a *Adapter) GetPreSignedURL(ctx context.Context, obj block.ObjectPointer, return "", time.Time{}, err } - client := a.clients.Get(ctx, bucket) - presigner := s3.NewPresignClient(client, - func(options *s3.PresignOptions) { - options.Expires = a.preSignedExpiry - }) + presigner := a.presignerClient(ctx, bucket) captureExpiresPresigner := &CaptureExpiresPresigner{} var req *v4.PresignedHTTPRequest @@ -458,12 +461,7 @@ func (a *Adapter) GetPresignUploadPartURL(ctx context.Context, obj block.ObjectP return "", err } - client := a.clients.Get(ctx, bucket) - presigner := s3.NewPresignClient(client, - func(options *s3.PresignOptions) { - options.Expires = a.preSignedExpiry - }, - ) + presigner := a.presignerClient(ctx, bucket) uploadInput := &s3.UploadPartInput{ Bucket: aws.String(bucket), @@ -953,3 +951,17 @@ func ExtractParamsFromQK(qk block.QualifiedKey) (string, string) { } return bucket, key } + +func (a *Adapter) presignerClient(ctx context.Context, bucket string) *s3.PresignClient { + client := a.clients.Get(ctx, bucket) + return s3.NewPresignClient(client, + func(options *s3.PresignOptions) { + options.Expires = a.preSignedExpiry + if a.preSignedEndpoint != "" { + options.ClientOptions = append(options.ClientOptions, func(o *s3.Options) { + o.BaseEndpoint = &a.preSignedEndpoint + }) + } + }, + ) +} diff --git a/pkg/block/s3/adapter_test.go b/pkg/block/s3/adapter_test.go index abd251febb3..90dde42c06d 100644 --- a/pkg/block/s3/adapter_test.go +++ b/pkg/block/s3/adapter_test.go @@ -12,7 +12,7 @@ import ( s3a "github.com/treeverse/lakefs/pkg/block/s3" ) -func getS3BlockAdapter(t *testing.T) *s3a.Adapter { +func getS3BlockAdapter(t *testing.T, opts []s3a.AdapterOption) *s3a.Adapter { s3params := params.S3{ Region: "us-east-1", Endpoint: blockURL, @@ -23,8 +23,12 @@ func getS3BlockAdapter(t *testing.T) *s3a.Adapter { SecretAccessKey: minioTestSecretAccessKey, }, } + if opts == nil { + opts = make([]s3a.AdapterOption, 0, 1) + } + opts = append(opts, s3a.WithNowFactory(blocktest.NowMockDefault)) - adapter, err := s3a.NewAdapter(context.Background(), s3params, s3a.WithNowFactory(blocktest.NowMockDefault)) + adapter, err := s3a.NewAdapter(context.Background(), s3params, opts...) if err != nil { t.Fatal("cannot create s3 adapter: ", err) } @@ -40,13 +44,28 @@ func TestS3Adapter(t *testing.T) { externalPath, err := url.JoinPath(basePath, "external") require.NoError(t, err) - adapter := getS3BlockAdapter(t) - blocktest.AdapterTest(t, adapter, localPath, externalPath) + adapter := getS3BlockAdapter(t, nil) + blocktest.AdapterTest(t, adapter, localPath, externalPath, true) +} + +// TestS3AdapterPresignedOverride tests basic functionality of the S3 block adapter along with the desired behavior of +// overriding the pre-signed URL endpoint +func TestS3AdapterPresignedOverride(t *testing.T) { + basePath, err := url.JoinPath("s3://", bucketName) + require.NoError(t, err) + localPath, err := url.JoinPath(basePath, "lakefs") + require.NoError(t, err) + externalPath, err := url.JoinPath(basePath, "external") + require.NoError(t, err) + + oeu, _ := url.Parse("https://myendpoint.com") + adapter := getS3BlockAdapter(t, []s3a.AdapterOption{s3a.WithPreSignedEndpoint(oeu.String())}) + blocktest.AdapterPresignedEndpointOverrideTest(t, adapter, localPath, externalPath, oeu) } // TestAdapterNamespace tests the namespace validity regex with various paths func TestAdapterNamespace(t *testing.T) { - adapter := getS3BlockAdapter(t) + adapter := getS3BlockAdapter(t, nil) expr, err := regexp.Compile(adapter.GetStorageNamespaceInfo().ValidityRegex) require.NoError(t, err) diff --git a/pkg/config/config.go b/pkg/config/config.go index 1cced8d1e94..3412f4562e4 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -250,12 +250,14 @@ type Config struct { ServerSideEncryption string `mapstructure:"server_side_encryption"` ServerSideEncryptionKmsKeyID string `mapstructure:"server_side_encryption_kms_key_id"` PreSignedExpiry time.Duration `mapstructure:"pre_signed_expiry"` - DisablePreSigned bool `mapstructure:"disable_pre_signed"` - DisablePreSignedUI bool `mapstructure:"disable_pre_signed_ui"` - DisablePreSignedMultipart bool `mapstructure:"disable_pre_signed_multipart"` - ClientLogRetries bool `mapstructure:"client_log_retries"` - ClientLogRequest bool `mapstructure:"client_log_request"` - WebIdentity *struct { + // Endpoint for pre-signed URLs, if set, will override the default pre-signed URL S3 endpoint (only for pre-sign URL generation) + PreSignedEndpoint string `mapstructure:"pre_signed_endpoint"` + DisablePreSigned bool `mapstructure:"disable_pre_signed"` + DisablePreSignedUI bool `mapstructure:"disable_pre_signed_ui"` + DisablePreSignedMultipart bool `mapstructure:"disable_pre_signed_multipart"` + ClientLogRetries bool `mapstructure:"client_log_retries"` + ClientLogRequest bool `mapstructure:"client_log_request"` + WebIdentity *struct { SessionDuration time.Duration `mapstructure:"session_duration"` SessionExpiryWindow time.Duration `mapstructure:"session_expiry_window"` } `mapstructure:"web_identity"` @@ -485,6 +487,7 @@ func (c *Config) BlockstoreS3Params() (blockparams.S3, error) { ServerSideEncryption: c.Blockstore.S3.ServerSideEncryption, ServerSideEncryptionKmsKeyID: c.Blockstore.S3.ServerSideEncryptionKmsKeyID, PreSignedExpiry: c.Blockstore.S3.PreSignedExpiry, + PreSignedEndpoint: c.Blockstore.S3.PreSignedEndpoint, DisablePreSigned: c.Blockstore.S3.DisablePreSigned, DisablePreSignedUI: c.Blockstore.S3.DisablePreSignedUI, DisablePreSignedMultipart: c.Blockstore.S3.DisablePreSignedMultipart,