Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Return token expiry with presigned URLs #6337

Merged
merged 9 commits into from
Aug 8, 2023
Merged

Return token expiry with presigned URLs #6337

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9f1741d
Add "ExpiresAt" to S3 clients
arielshaqed Aug 7, 2023
15793db
Log expiry time of clients when fetching from cache
arielshaqed Aug 7, 2023
2ab772e
Add expiry time to presigned URLs to adaptors
arielshaqed Aug 7, 2023
76ed9d1
Add presigned URL expiry to API
arielshaqed Aug 7, 2023
024ab78
Return expiry time on presigned URLs
arielshaqed Aug 7, 2023
039ed14
Lint fixes
arielshaqed Aug 7, 2023
29d7cde
Report physical address expiry in `lakectl fs stat --pre-signed`
arielshaqed Aug 7, 2023
7da818a
Select good golden file for `lakectl fs stat --pre-sign` test
arielshaqed Aug 7, 2023
8cd2a01
[CR] Add TODO to support expiry on Azure & GCS
arielshaqed Aug 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 47 additions & 3 deletions pkg/block/s3/client_cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,13 @@ package s3

import (
"context"
"errors"
"fmt"
"sync"
"time"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/aws/awserr"
"github.com/aws/aws-sdk-go/aws/session"
"github.com/aws/aws-sdk-go/service/s3"
"github.com/aws/aws-sdk-go/service/s3/s3iface"
Expand All @@ -15,8 +18,25 @@ import (
"github.com/treeverse/lakefs/pkg/stats"
)

// 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.
Comment on lines +21 to +23
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

var ErrDoesntExpire = errors.New("no access expiry")

type Expirer interface {
// ExpiresAt returns an expiry time or an error. It returns
// a ErrDoesntExpire if it cannot determine expiry times -- for
// instance, if AWS is configured using an access key.
ExpiresAt() (time.Time, error)
}

type S3APIWithExpirer interface {
s3iface.S3API
Expirer
}

type (
clientFactory func(awsSession *session.Session, cfgs ...*aws.Config) s3iface.S3API
clientFactory func(awsSession *session.Session, cfgs ...*aws.Config) S3APIWithExpirer
s3RegionGetter func(ctx context.Context, sess *session.Session, bucket string) (string, error)
)

Expand All @@ -39,8 +59,32 @@ func getBucketRegionFromSession(ctx context.Context, sess *session.Session, buck
return region, nil
}

func newS3Client(sess *session.Session, cfgs ...*aws.Config) s3iface.S3API {
return s3.New(sess, cfgs...)
type s3Client struct {
s3iface.S3API
awsSession *session.Session
}

func newS3Client(sess *session.Session, cfgs ...*aws.Config) S3APIWithExpirer {
return &s3Client{
S3API: s3.New(sess, cfgs...),
awsSession: sess,
}
}

func (c *s3Client) ExpiresAt() (time.Time, error) {
creds := c.awsSession.Config.Credentials
if creds == nil {
return time.Time{}, ErrDoesntExpire
}
expiryTime, err := creds.ExpiresAt()
if err != nil {
if awsErr, ok := err.(awserr.Error); ok {
if awsErr.Code() == "ProviderNotExpirer" {
err = ErrDoesntExpire
}
}
}
return expiryTime, err
}

func NewClientCache(awsSession *session.Session) *ClientCache {
Expand Down
22 changes: 19 additions & 3 deletions pkg/block/s3/client_cache_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"errors"
"testing"
"time"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/aws/session"
Expand All @@ -13,7 +14,22 @@ import (
"github.com/treeverse/lakefs/pkg/testutil"
)

var errRegion = errors.New("failed to get region")
var (
errRegion = errors.New("failed to get region")
errFakeExpires = errors.New("fake Expirer")
)

type FakeS3APIWithExpirer struct {
s3iface.S3API
}

func NewFakeS3APIWithExpirer() FakeS3APIWithExpirer {
return FakeS3APIWithExpirer(struct{ s3iface.S3API }{})
}

func (f FakeS3APIWithExpirer) ExpiresAt() (time.Time, error) {
return time.Time{}, errFakeExpires
}

func TestClientCache(t *testing.T) {
defaultRegion := "us-west-2"
Expand Down Expand Up @@ -57,7 +73,7 @@ func TestClientCache(t *testing.T) {
expectedRegionFetch := make(map[string]bool)
c := s3.NewClientCache(sess)

c.SetClientFactory(func(sess *session.Session, cfgs ...*aws.Config) s3iface.S3API {
c.SetClientFactory(func(sess *session.Session, cfgs ...*aws.Config) s3.S3APIWithExpirer {
region := sess.Config.Region
for _, cfg := range cfgs {
if cfg.Region != nil {
Expand All @@ -68,7 +84,7 @@ func TestClientCache(t *testing.T) {
t.Fatalf("client created more than once for a region")
}
actualClientsCreated[*region] = true
return struct{ s3iface.S3API }{}
return NewFakeS3APIWithExpirer()
})
c.SetS3RegionGetter(func(ctx context.Context, sess *session.Session, bucket string) (string, error) {
if actualRegionFetch[bucket] {
Expand Down