Add support for presigned URLs in the lakeFS API #4985

ozkatz · 2023-01-09T16:00:19Z

This one requires discussion :)

Currently, lakeFS users have 2 options when reading and writing data:

Involve lakeFS in the data path: this typically means either using the s3 gateway to read/write data, or to use the data operations in the lakeFS API (get_object, put_object)
Use lakeFS for metadata only, with direct read/write to the underlying object store.

This requires a pretty hard tradeoff: (1) has implications regarding security (data going through the lakeFS server) as well as added operational complexity (i.e. the need to size a lakeFS installation for the required bandwidth) and potentially cost implications if the lakeFS installation is outside the AWS account and VPC used by its consumers (such as in lakeFS Cloud).

As for (2), it has a different set of problems: Once the metadata operation is done, direct access to the object store requires authentication and authorization. This means managing 2 different auth* systems: one for lakeFS and one for the underlying object store. Keeping both of them in sync is very hard (i.e. how do I give every user permissions to read only the exact underlying objects in a repository?)

This PR introduces pre-signed URLs as a middle ground. It allows receiving short-lived pre-signed URLs from GetObject, StatObject and ListObjects with the optional presign=true query parameter, as well as limited write support using GetPhysicalAddress where the address returned is pre-signed for a write operation.

This PR currently does not handle Multipart uploads, this could be added later (and is probably a requirement before it resolves #3160)

This is currently in draft and requires discussion. I don't plan on merging this unless we agree this is desired.

arielshaqed · 2023-01-09T16:16:56Z

Yay, this is a great direction!

One worry is how to use this from lakeFSFS, because I would really like to use this from lakeFSFS. While this is certainly not the only client, it's a pretty important one. Here I am not sure how to use this and get the optimizations from S3A and other underlying FileSystems.

arielshaqed

Neat stuff! Can you open an issue optionally to rewrite lakectl fs {upload,cat} --direct to use this redirecting mode? It will make it so much simpler and more natural!

Approving as a satisfied future user of this, obviously not as a versioning engine team member.

(Still not sure how to use it efficiently in lakeFSFS, but that's a different matter!)

arielshaqed · 2023-01-10T19:36:05Z

pkg/api/controller.go

+			writeError(w, r, http.StatusInternalServerError, err)
+			return
+		}
+		response.PresignedUrl = StringPtr(preSignedURL)


Nit: I think

Suggested change

response.PresignedUrl = StringPtr(preSignedURL)

response.PresignedUrl = &preSignedURL

is more readable as well as more efficient.

arielshaqed · 2023-01-10T19:39:22Z

pkg/api/controller.go

+				if authResponse.Allowed {
+					objStat.PhysicalAddress, err = c.BlockAdapter.GetPreSignedURL(ctx, block.ObjectPointer{
+						StorageNamespace: repo.StorageNamespace,
+						Identifier:       entry.PhysicalAddress,
+						IdentifierType:   entry.AddressType.ToIdentifierType(),
+					}, block.PreSignModeRead)
+					if c.handleAPIError(ctx, w, r, err) {
+						return
+					}
+				}


When I think of it this is really neat: I will be able to read all contents of an entire directory with a single listObjects on lakeFS.

arielshaqed · 2023-01-10T19:44:53Z

api/swagger.yml

+      parameters:
+        - in: query
+          name: presign
+          required: false
+          schema:
+            type: boolean


I think that this should be a POST endpoint that redirects!

nopcoder

Very cool! Added some comments.
And general question - do we want the pre sign duration configuration controlled in one location / settings?

api/swagger.yml

esti/presign_test.go

pkg/api/controller.go

nopcoder · 2023-01-11T08:48:27Z

pkg/block/azure/adapter.go

+		Permissions:   permissions.String(),
+	}
+
+	keyCredentials, ok := a.credentials.(*azblob.SharedKeyCredential)


Can be done once at NewAdater and leave here just pointer check.

nopcoder · 2023-01-11T08:50:08Z

pkg/block/azure/adapter.go

+	// Since this is a blob SAS, the URL is to the Azure storage blob.
+	qp := sasQueryParams.Encode()
+	return fmt.Sprintf(preSignedBlobPattern,
+		a.credentials, qualifiedKey.ContainerName, qualifiedKey.BlobURL, qp), nil


We depends on credentials - string format of a type we don't control? Just making sure we do not need to call any explicit function to format the string.

Good catch, it's supposed to be the account name extracted from the credentials! Fixed.

How is it safe to return a string containing the credentials?

@itaiad200 you caught this too, changed :)

pkg/gateway/operations/mock_adapter_test.go

itaiad200

Thanks - I think this can bring value to our users!

itaiad200 · 2023-01-11T09:47:13Z

pkg/api/controller.go

+					return
+				}
+				if authResponse.Allowed {
+					objStat.PhysicalAddress, err = c.BlockAdapter.GetPreSignedURL(ctx, block.ObjectPointer{


IIRC the default is 1000 objects in ListObjects. It means you're adding 1000 sequential calls to aws in a single lakeFS call. Doesn't seem like it can really scale. We've seen cases of users performing a >300 RPS of ListObjects, if we're planning to continue supporting this scale, we need to find other solutions for ListObjects. Can we get all 1000 links in a single API call to aws?

We should also consider the error rate. If AWS has 4 9s', the probability of 1000 calls to succeed is ~90%.

GetPreSignedURL Doesn't do any network calls.
Pre-signed URLs are constructed locally by the adapter

itaiad200 · 2023-01-11T09:51:10Z

pkg/block/azure/adapter.go

@@ -29,21 +29,37 @@ const (
 	defaultMaxRetryRequests = 0
 	AuthMethodAccessKey     = "access-key"
 	AuthMethodMSI           = "msi"
+
+	defaultPreSignedURLDuration = time.Minute * 15


Shouldn't this be a block package-level const?

Good call - moved

itaiad200 · 2023-01-11T09:52:44Z

pkg/api/controller.go

@@ -333,6 +333,21 @@ func (c *Controller) GetPhysicalAddress(w http.ResponseWriter, r *http.Request,
 		PhysicalAddress: StringPtr(qk.Format()),
 		Token:           StringValue(token),
 	}
+
+	if params.Presign != nil && *params.Presign {


Notice that the above was changed with the merge of #4900

itaiad200 · 2023-01-11T10:57:38Z

pkg/block/azure/adapter.go

+	return a.getPreSignedURL(ctx, obj, permissions)
+}
+
+func (a *Adapter) getPreSignedURL(ctx context.Context, obj block.ObjectPointer, permissions azblob.BlobSASPermissions) (string, error) {


Heads up @N-o-Z @guy-har who are working on Azure gen v2 now

itaiad200 · 2023-01-11T10:59:43Z

pkg/block/azure/adapter.go

+	// Since this is a blob SAS, the URL is to the Azure storage blob.
+	qp := sasQueryParams.Encode()
+	return fmt.Sprintf(preSignedBlobPattern,
+		a.credentials, qualifiedKey.ContainerName, qualifiedKey.BlobURL, qp), nil


How is it safe to return a string containing the credentials?

itaiad200 · 2023-01-11T11:00:30Z

pkg/block/gs/adapter.go

@@ -21,6 +21,8 @@ const (
 	delimiter    = "/"
 	partSuffix   = ".part_"
 	markerSuffix = ".multipart"
+
+	defaultPreSignedURLDuration = time.Minute * 15


Same here - if we define it equally in 2 different places, let's move it up the packages stack.

itaiad200 · 2023-01-11T11:02:13Z

pkg/block/s3/adapter.go

@@ -29,6 +29,9 @@ const (

 	// Chunks smaller than that are only allowed for the last chunk upload
 	minChunkSize = 8 * 1024
+
+	// DefaultPreSignedURLDuration determines how long should pre-signed URLs be valid for
+	DefaultPreSignedURLDuration = time.Minute * 15


itaiad200 · 2023-01-11T11:06:17Z

esti/presign_test.go

+	"github.com/treeverse/lakefs/pkg/api"
+)
+
+func TestPreSign(t *testing.T) {


Thanks for adding tests for the Stat operation! Can we also add tests to all other new operations?
Also - let's validate that the links work as expected too

You're right - I'm still trying to get this test to pass via Esti, no luck yet :( will add the additional tests for links as well once I do.

itaiad200 · 2023-01-11T11:11:15Z

api/swagger.yml

@@ -3286,6 +3318,11 @@ paths:
        schema:
          type: string
        description: path to the table data
+      - in: query


Why do we need it in symlinks?

Good catch! We do not. removed

api/swagger.yml

nopcoder · 2023-01-11T14:56:54Z

esti/presign_test.go

+	blockStoreType := viper.GetViper().GetString("blockstore_type")
+	expected := ""
+	switch blockStoreType {
+	case block.BlockstoreTypeS3:
+		expected = "X-Amz-Signature="
+	case block.BlockstoreTypeGS:
+		expected = "X-Goog-Signature="
+	case block.BlockstoreTypeAzure:
+		expected = "sv="
+	default:
+		t.Skip("Only GS, S3 and Azure Blob supported for pre-signed urls")
+	}
+
+	ctx, _, repo := setupTest(t)
+	defer tearDownTest(repo)
+	_, _ = uploadFileRandomData(ctx, t, repo, mainBranch, "foo/bar", true)
+
+	// Get Object
+	preSign := true
+	response, err := client.StatObjectWithResponse(ctx, repo, mainBranch, &api.StatObjectParams{
+		Path:    "foo/bar",
+		Presign: &preSign,
+	})
+	require.NoError(t, err, "failed to stat object with presign=true")
+	signedUrl := response.JSON200.PhysicalAddress
+	if !strings.HasPrefix(signedUrl, "http") {
+		t.Errorf("exected an HTTP(s) URL instead of an object store URI, got %s", signedUrl)
+	}
+
+	if !strings.Contains(signedUrl, expected) {
+		t.Errorf("expected a signed URL, got %s", signedUrl)
+	}


Suggested change

blockStoreType := viper.GetViper().GetString("blockstore_type")

expected := ""

switch blockStoreType {

case block.BlockstoreTypeS3:

expected = "X-Amz-Signature="

case block.BlockstoreTypeGS:

expected = "X-Goog-Signature="

case block.BlockstoreTypeAzure:

expected = "sv="

default:

t.Skip("Only GS, S3 and Azure Blob supported for pre-signed urls")

}

ctx, _, repo := setupTest(t)

defer tearDownTest(repo)

_, _ = uploadFileRandomData(ctx, t, repo, mainBranch, "foo/bar", true)

// Get Object

preSign := true

response, err := client.StatObjectWithResponse(ctx, repo, mainBranch, &api.StatObjectParams{

Path: "foo/bar",

Presign: &preSign,

})

require.NoError(t, err, "failed to stat object with presign=true")

signedUrl := response.JSON200.PhysicalAddress

if !strings.HasPrefix(signedUrl, "http") {

t.Errorf("exected an HTTP(s) URL instead of an object store URI, got %s", signedUrl)

}

if !strings.Contains(signedUrl, expected) {

t.Errorf("expected a signed URL, got %s", signedUrl)

}

blockStoreType := viper.GetString("blockstore.type")

expectedKey := ""

switch blockStoreType {

case block.BlockstoreTypeS3:

expectedKey = "X-Amz-Signature"

case block.BlockstoreTypeGS:

expectedKey = "X-Goog-Signature"

case block.BlockstoreTypeAzure:

expectedKey = "sv"

default:

t.Skip("Only GS, S3 and Azure Blob supported for pre-signed urls")

}

ctx, _, repo := setupTest(t)

defer tearDownTest(repo)

_, _ = uploadFileRandomData(ctx, t, repo, mainBranch, "foo/bar", true)

response, err := client.StatObjectWithResponse(ctx, repo, mainBranch, &api.StatObjectParams{

Path: "foo/bar",

Presign: swag.Bool(true),

})

require.NoError(t, err, "failed to stat object with presign=true")

require.NotNil(t, response.JSON200, "successful response")

signedURL := response.JSON200.PhysicalAddress

parsedSignedURL, err := url.Parse(signedURL)

require.NoErrorf(t, err, "failed to parse url - %s", signedURL)

require.Truef(t, strings.HasPrefix(parsedSignedURL.Scheme, "http"), "URL scheme http(s) - %s", signedURL)

require.NotEmptyf(t, parsedSignedURL.Query().Get(expectedKey), "signature expected in key '%s' - %s", expectedKey, signedURL)

nopcoder

LGTM! - additional comments for your consideration

Changes applied.

itaiad200 · 2023-01-13T12:55:36Z

@ozkatz Still missing tests for most of the APIs to get the links, including validations that links actually work.

ozkatz · 2023-01-13T19:40:18Z

You're right @itaiad200 - Merged too early. Tests added in #5035

ozkatz added 2 commits January 8, 2023 21:23

presign stat option

849146a

added basic presign mode

5233587

ozkatz added the include-changelog PR description should be included in next release changelog label Jan 9, 2023

ozkatz linked an issue Jan 9, 2023 that may be closed by this pull request

Bug: revert path shows up when comparing refs #3161

Closed

ozkatz removed a link to an issue Jan 9, 2023

Bug: revert path shows up when comparing refs #3161

Closed

ozkatz added area/API Improvements or additions to the API new-feature Issues that introduce new feature or capability labels Jan 9, 2023

fixed linting errors

28ac2ae

ozkatz requested review from nopcoder, itaiad200, eden-ohana, N-o-Z, talSofer and arielshaqed January 9, 2023 16:14

ozkatz added 3 commits January 10, 2023 10:25

listing should presign only objects that are readable for user

3c28c4c

fixed go adapter mock

2d762e3

added esti test for presigned urls

874a325

arielshaqed approved these changes Jan 10, 2023

View reviewed changes

ozkatz added 2 commits January 11, 2023 10:27

added presigned tests for all object stores

aefdea1

fixed ptr per review

812d549

nopcoder requested changes Jan 11, 2023

View reviewed changes

fixed nopcoders review comments

32b785e

ozkatz requested a review from nopcoder January 11, 2023 11:03

itaiad200 previously requested changes Jan 11, 2023

View reviewed changes

ozkatz added 2 commits January 11, 2023 14:58

removed presign from symlink endpoint (added by mistake)

bca5e64

moved pre-signed duration const to the block adapter package

1d8d2de

ozkatz requested a review from itaiad200 January 11, 2023 13:11

Merge branch 'master' into feature/presigned

74657c9

ozkatz marked this pull request as ready for review January 11, 2023 14:46

nopcoder reviewed Jan 11, 2023

View reviewed changes

nopcoder approved these changes Jan 11, 2023

View reviewed changes

ozkatz and others added 2 commits January 11, 2023 17:19

fix failing esti test

950966f

Fix resolveBlobURLInfo missing ContainerName

b970b8f

ozkatz enabled auto-merge (squash) January 12, 2023 17:05

ozkatz disabled auto-merge January 12, 2023 17:46

ozkatz merged commit 0d9c665 into master Jan 12, 2023

ozkatz deleted the feature/presigned branch January 12, 2023 17:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for presigned URLs in the lakeFS API #4985

Add support for presigned URLs in the lakeFS API #4985

ozkatz commented Jan 9, 2023 •

edited

Loading

arielshaqed commented Jan 9, 2023

arielshaqed left a comment •

edited

Loading

arielshaqed Jan 10, 2023

arielshaqed Jan 10, 2023

arielshaqed Jan 10, 2023

nopcoder left a comment

nopcoder Jan 11, 2023

ozkatz Jan 11, 2023

nopcoder Jan 11, 2023

ozkatz Jan 11, 2023

itaiad200 Jan 11, 2023

ozkatz Jan 11, 2023

itaiad200 left a comment

itaiad200 Jan 11, 2023

itaiad200 Jan 11, 2023

ozkatz Jan 11, 2023

itaiad200 Jan 11, 2023

ozkatz Jan 11, 2023

itaiad200 Jan 11, 2023

itaiad200 Jan 11, 2023

itaiad200 Jan 11, 2023

itaiad200 Jan 11, 2023

ozkatz Jan 11, 2023

itaiad200 Jan 11, 2023

ozkatz Jan 11, 2023

itaiad200 Jan 11, 2023

ozkatz Jan 11, 2023

itaiad200 Jan 11, 2023

ozkatz Jan 11, 2023

nopcoder Jan 11, 2023 •

edited

Loading

nopcoder left a comment

itaiad200 commented Jan 13, 2023

ozkatz commented Jan 13, 2023 •

edited

Loading

	response.PresignedUrl = StringPtr(preSignedURL)
	response.PresignedUrl = &preSignedURL

Add support for presigned URLs in the lakeFS API #4985

Add support for presigned URLs in the lakeFS API #4985

Conversation

ozkatz commented Jan 9, 2023 • edited Loading

arielshaqed commented Jan 9, 2023

arielshaqed left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nopcoder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaiad200 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nopcoder Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

nopcoder left a comment

Choose a reason for hiding this comment

itaiad200 commented Jan 13, 2023

ozkatz commented Jan 13, 2023 • edited Loading

ozkatz commented Jan 9, 2023 •

edited

Loading

arielshaqed left a comment •

edited

Loading

nopcoder Jan 11, 2023 •

edited

Loading

ozkatz commented Jan 13, 2023 •

edited

Loading