Return token expiry with presigned URLs #6337

arielshaqed · 2023-08-07T12:32:05Z

Fixes #6328.

Only S3 works, the others just return a zero time.Time for "not implemented".

github-actions · 2023-08-07T14:26:24Z

🎊 PR Preview 8cd2a01 has been successfully built and deployed to https://treeverse-lakeFS-preview-pr-6337.surge.sh

🕐 Build time: 0.015s

_{🤖 By surge-preview}

N-o-Z · 2023-08-07T16:01:52Z

Removing myself from reviewers as I currently don't have the bandwidth to review this PR- sorry

Useful, and also lets me test that a deployment on lakeFS.Cloud staging works! >:-)

arielshaqed · 2023-08-07T16:57:29Z

Here's what it looks like (with 29d7cde) on lakeFS.Cloud (I shortened the presigned URL and removed all the interesting parts that were actually signed...):

$ lakectl fs stat --pre-sign lakefs://sample-repo/main/boo
Path: boo
Modified Time: 2023-08-07 15:03:28 +0300 IDT
Size: 4 bytes
Human Size: 4 B
Physical Address: https://lakefs-sample-us-east-1-staging.s3.amazonaws.com/tokenpathsigsyzzy?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=ASIAAFRICAAUSTRALIA%2F20230807%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20230807T164925Z&X-Amz-Expires=900&X-Amz-Security-Token=shhh&X-Amz-SignedHeaders=host&X-Amz-Signature=123456
Physical Address Expires: 2023-08-07 20:03:54 +0300 IDT
Checksum: 90580754da3ed1b3e4be38c9b277bc9b
Content-Type: application/octet-stream

Some lakeFS block adaptors report expiry, others do not. Test lakectl using an appropriate golden file

arielshaqed · 2023-08-08T08:09:22Z

I will pull with a single approval -- unless you ask!

guy-har

LGTM

guy-har · 2023-08-08T08:33:26Z

pkg/block/s3/client_cache.go

+// ErrDoesntExpire is returned by an Expirer if expiry times cannot be
+// determined.  For instance, if AWS is configured using an access key then
+// Expirer cannot determine expiry.


Is it returned if it doesn't expire or in case it cannot be determined?

Kinda both. If it is impossible to determine expiry it is actually quite hard to determine whether the token has expired, see below. So if the credentials used to configure AWS don't support expiry, either they will never expire, or they will and lakeFS will never know. But note that when this happens all is not lost: lakeFS will still report the built-in 15-minute expiry that was placed on the URL.

Impossible to determine expiry: I can find no AWS SDK call that gives the time remaining on a given authenticated client. The only built-in credentials provder there that supports ExpiresAt seems to be ssocreds -- which reads a token file But e.g. EnvProvider can read an STS token -- and does not support ExpiresAt.

guy-har · 2023-08-08T09:24:26Z

pkg/block/azure/adapter.go

+	url, err := a.getPreSignedURL(ctx, obj, permissions)
+	return url, time.Time{}, err


Why isn't this supported on azure? Do we plan to? maybe add a TODO

Absolutely! Just immediate issue is on AWS. Opened #6347 and added TODOs here and in GCS.

guy-har · 2023-08-08T09:25:02Z

pkg/block/gs/adapter.go

 	}
-	return k, nil
+	return k, time.Time{}, nil


Why isn't this supported on GCS, do we plan to? Maybe add a TODO

guy-har · 2023-08-08T09:26:25Z

pkg/block/adapter.go

+	// Returns a presigned URL for accessing obj with mode, and the
+	// expiry time for this URL.  The expiry time IsZero() if reporting
+	// expiry is no supported.  The expiry time will be sooner than
+	// Config.*.PreSignedExpiry if an auth token is about to expire.


Suggested change

// Returns a presigned URL for accessing obj with mode, and the

// expiry time for this URL. The expiry time IsZero() if reporting

// expiry is no supported. The expiry time will be sooner than

// Config.*.PreSignedExpiry if an auth token is about to expire.

// Returns a presigned URL for accessing obj with mode, and the

// expiry time for this URL. The expiry time IsZero() if reporting

// expiry is not supported. The expiry time will be sooner than

// Config.*.PreSignedExpiry if an auth token is about to expire.

guy-har · 2023-08-08T10:06:06Z

pkg/block/s3/adapter.go

+	expiry := time.Now().Add(a.preSignedExpiry)
+	clientExpiry, clientExpiryErr := client.ExpiresAt()
+	switch {
+	case clientExpiryErr == nil:
+		if clientExpiry.Before(expiry) {
+			expiry = clientExpiry
+		}
+	case errors.Is(clientExpiryErr, ErrDoesntExpire):
+		break
+	default:
+		log.WithFields(logging.Fields{
+			"namespace":  obj.StorageNamespace,
+			"identifier": obj.Identifier,
+		}).WithError(err).Warning("Failed to get client (token) expiry: URL expiry may be too high")
+	}
+	return preSignedURL, expiry, err


Just a thought:
If the Block Adapter would have a method that returns the client expiry we could run this logic only once in the controller, I think it might make things a bit simple

At least for S3 we support multiple AWS clients, per namespace. So this method would need to be called with a namespace parameter. At which point it becomes simpler to push it to presigning.

Meanwhile, the controller uses this in multiple places, and we have a few more non-controller calls. At most we'll be able to extract this logic to the adaptor level -- let's think about refactoring after adding support for Azure and GCS.

Also fix tpyo.

arielshaqed

Thanks!

arielshaqed · 2023-08-08T11:22:54Z

pkg/block/azure/adapter.go

+	url, err := a.getPreSignedURL(ctx, obj, permissions)
+	return url, time.Time{}, err


Absolutely! Just immediate issue is on AWS. Opened #6347 and added TODOs here and in GCS.

arielshaqed · 2023-08-08T11:23:46Z

pkg/block/gs/adapter.go

 	}
-	return k, nil
+	return k, time.Time{}, nil


arielshaqed · 2023-08-08T11:26:47Z

pkg/block/s3/adapter.go

+	expiry := time.Now().Add(a.preSignedExpiry)
+	clientExpiry, clientExpiryErr := client.ExpiresAt()
+	switch {
+	case clientExpiryErr == nil:
+		if clientExpiry.Before(expiry) {
+			expiry = clientExpiry
+		}
+	case errors.Is(clientExpiryErr, ErrDoesntExpire):
+		break
+	default:
+		log.WithFields(logging.Fields{
+			"namespace":  obj.StorageNamespace,
+			"identifier": obj.Identifier,
+		}).WithError(err).Warning("Failed to get client (token) expiry: URL expiry may be too high")
+	}
+	return preSignedURL, expiry, err


At least for S3 we support multiple AWS clients, per namespace. So this method would need to be called with a namespace parameter. At which point it becomes simpler to push it to presigning.

Meanwhile, the controller uses this in multiple places, and we have a few more non-controller calls. At most we'll be able to extract this logic to the adaptor level -- let's think about refactoring after adding support for Azure and GCS.

arielshaqed · 2023-08-08T11:34:16Z

pkg/block/s3/client_cache.go

+// ErrDoesntExpire is returned by an Expirer if expiry times cannot be
+// determined.  For instance, if AWS is configured using an access key then
+// Expirer cannot determine expiry.


Kinda both. If it is impossible to determine expiry it is actually quite hard to determine whether the token has expired, see below. So if the credentials used to configure AWS don't support expiry, either they will never expire, or they will and lakeFS will never know. But note that when this happens all is not lost: lakeFS will still report the built-in 15-minute expiry that was placed on the URL.

Impossible to determine expiry: I can find no AWS SDK call that gives the time remaining on a given authenticated client. The only built-in credentials provder there that supports ExpiresAt seems to be ssocreds -- which reads a token file But e.g. EnvProvider can read an STS token -- and does not support ExpiresAt.

arielshaqed added 5 commits August 7, 2023 11:08

Add "ExpiresAt" to S3 clients

9f1741d

Log expiry time of clients when fetching from cache

15793db

Add expiry time to presigned URLs to adaptors

2ab772e

Only S3 works, the others just return a zero time.Time for "not implemented".

Add presigned URL expiry to API

76ed9d1

Return expiry time on presigned URLs

024ab78

arielshaqed added area/API Improvements or additions to the API new-feature Issues that introduce new feature or capability include-changelog PR description should be included in next release changelog AWS labels Aug 7, 2023

Lint fixes

039ed14

arielshaqed requested a review from N-o-Z August 7, 2023 15:04

N-o-Z removed their request for review August 7, 2023 16:01

Report physical address expiry in lakectl fs stat --pre-signed

29d7cde

Useful, and also lets me test that a deployment on lakeFS.Cloud staging works! >:-)

arielshaqed force-pushed the feature/6328-token-expiry-in-presigned-urls branch from 5024ae4 to f369b04 Compare August 7, 2023 19:20

Select good golden file for lakectl fs stat --pre-sign test

7da818a

Some lakeFS block adaptors report expiry, others do not. Test lakectl using an appropriate golden file

arielshaqed force-pushed the feature/6328-token-expiry-in-presigned-urls branch from f369b04 to 7da818a Compare August 7, 2023 19:20

arielshaqed requested review from johnnyaug and guy-har August 8, 2023 08:08

guy-har approved these changes Aug 8, 2023

View reviewed changes

arielshaqed mentioned this pull request Aug 8, 2023

Report pre-signed URL expiry on Azure, GCS #6347

Open

[CR] Add TODO to support expiry on Azure & GCS

8cd2a01

Also fix tpyo.

arielshaqed commented Aug 8, 2023

View reviewed changes

arielshaqed merged commit accced2 into master Aug 8, 2023
33 checks passed

arielshaqed deleted the feature/6328-token-expiry-in-presigned-urls branch August 8, 2023 12:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return token expiry with presigned URLs #6337

Return token expiry with presigned URLs #6337

arielshaqed commented Aug 7, 2023

github-actions bot commented Aug 7, 2023 •

edited

Loading

N-o-Z commented Aug 7, 2023

arielshaqed commented Aug 7, 2023

arielshaqed commented Aug 8, 2023

guy-har left a comment

guy-har Aug 8, 2023

arielshaqed Aug 8, 2023

guy-har Aug 8, 2023

arielshaqed Aug 8, 2023

guy-har Aug 8, 2023

arielshaqed Aug 8, 2023

guy-har Aug 8, 2023

guy-har Aug 8, 2023

arielshaqed Aug 8, 2023

arielshaqed left a comment

arielshaqed Aug 8, 2023

arielshaqed Aug 8, 2023

arielshaqed Aug 8, 2023

arielshaqed Aug 8, 2023

		url, err := a.getPreSignedURL(ctx, obj, permissions)
		return url, time.Time{}, err

Return token expiry with presigned URLs #6337

Return token expiry with presigned URLs #6337

Conversation

arielshaqed commented Aug 7, 2023

github-actions bot commented Aug 7, 2023 • edited Loading

N-o-Z commented Aug 7, 2023

arielshaqed commented Aug 7, 2023

arielshaqed commented Aug 8, 2023

guy-har 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

arielshaqed 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

github-actions bot commented Aug 7, 2023 •

edited

Loading