ImageUpload: Add presigned URL support for S3 external image storage (#119002)

* feat(imageupload): 🔗 add presigned URL support for S3 external image storage

Add `enable_presigned_urls` and `presigned_url_expiration` config options
to `[external_image_storage.s3]`, bringing S3 to parity with GCS and Azure.

When enabled, objects are uploaded without an ACL header (compatible with
BucketOwnerEnforced buckets that disable ACLs) and a time-limited
presigned GET URL is returned instead of a direct S3 URL.

Closes #82707

Co-Authored-By: Claude <svc-devxp-claude@slack-corp.com>

* refactor(imageupload): 🔧 address PR review feedback for S3 presigned URLs

- Refactor NewS3Uploader to accept S3UploaderOptions struct instead of
  positional parameters
- Remove dead ACL override (ACL was set to "private" but never used
  since upload skips ACL when presigned URLs are enabled)
- Add validation that presigned_url_expiration is >= 0
- Remove tautological constructor tests
- Add S3 client interface (s3ifaces) with stubbable newS3Client for
  testability, mirroring the existing GCS pattern
- Add unit tests for both upload paths: regular (with ACL) and presigned
  URL (without ACL)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude <svc-devxp-claude@slack-corp.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Michael Atkinson 2026-05-06 09:14:06 -04:00 committed by GitHub
parent f07c37c693
commit 473d26d8f0
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
9 changed files with 414 additions and 48 deletions

View file

@ -1975,6 +1975,8 @@ region =
path =
access_key =
secret_key =
enable_presigned_urls = false
presigned_url_expiration =
[external_image_storage.webdav]
url =

View file

@ -1893,6 +1893,8 @@ default_datasource_uid =
;path =
;access_key =
;secret_key =
;enable_presigned_urls = false
;presigned_url_expiration =
[external_image_storage.webdav]
;url =

View file

@ -2534,6 +2534,18 @@ Access key requires permissions to the S3 bucket for the 's3:PutObject' and 's3:
Secret key, for example, AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA.
#### `enable_presigned_urls`
Generate presigned URLs for uploaded images instead of requiring publicly readable objects. When enabled, objects are uploaded without an ACL header, making this compatible with S3 buckets that have `ObjectOwnership` set to `BucketOwnerEnforced` (ACLs disabled). Default is `false`.
#### `presigned_url_expiration`
Duration for which presigned URLs remain valid. Uses Go duration format (e.g., `168h` for 7 days, `6h` for 6 hours). Default is `168h` (7 days).
{{< admonition type="note" >}}
The maximum expiration depends on your AWS credential type: IAM user credentials support up to 7 days, IAM role or STS credentials are limited to the session duration (typically 112 hours), and instance profile credentials are limited to 6 hours.
{{< /admonition >}}
<hr>
### `[external_image_storage.webdav]`

View file

@ -12,8 +12,9 @@ import (
)
const (
pngExt = ".png"
defaultGCSSignedURLExpiration = 7 * 24 * time.Hour // 7 days
pngExt = ".png"
defaultGCSSignedURLExpiration = 7 * 24 * time.Hour // 7 days
defaultS3PresignedURLExpiration = 7 * 24 * time.Hour // 7 days
)
//go:generate mockgen -destination=mock.go -package=imguploader github.com/grafana/grafana/pkg/components/imguploader ImageUploader
@ -48,6 +49,23 @@ func NewImageUploader(cfg *setting.Cfg) (ImageUploader, error) {
bucketUrl := s3sec.Key("bucket_url").MustString("")
accessKey := s3sec.Key("access_key").MustString("")
secretKey := s3sec.Key("secret_key").MustString("")
enablePresignedURLs := s3sec.Key("enable_presigned_urls").MustBool(false)
presignedURLExp := s3sec.Key("presigned_url_expiration").MustString("")
var presignedURLExpiration time.Duration
if presignedURLExp != "" {
presignedURLExpiration, err = time.ParseDuration(presignedURLExp)
if err != nil {
return nil, err
}
if presignedURLExpiration < 0 {
return nil, fmt.Errorf("presigned_url_expiration must be >= 0, got %s", presignedURLExp)
}
} else {
presignedURLExpiration = defaultS3PresignedURLExpiration
}
acl := "public-read"
if path != "" && path[len(path)-1:] != "/" {
path += "/"
@ -62,7 +80,18 @@ func NewImageUploader(cfg *setting.Cfg) (ImageUploader, error) {
region = info.region
}
return NewS3Uploader(endpoint, region, bucket, path, "public-read", accessKey, secretKey, pathStyleAccess), nil
return NewS3Uploader(S3UploaderOptions{
Endpoint: endpoint,
Region: region,
Bucket: bucket,
Path: path,
ACL: acl,
AccessKey: accessKey,
SecretKey: secretKey,
PathStyleAccess: pathStyleAccess,
EnablePresignedURLs: enablePresignedURLs,
PresignedURLExpiration: presignedURLExpiration,
}), nil
case "webdav":
webdavSec, err := cfg.Raw.GetSection("external_image_storage.webdav")
if err != nil {

View file

@ -2,6 +2,7 @@ package imguploader
import (
"testing"
"time"
"github.com/grafana/grafana/pkg/components/imguploader/gcs"
"github.com/grafana/grafana/pkg/setting"
@ -34,10 +35,10 @@ func TestImageUploaderFactory(t *testing.T) {
original, ok := uploader.(*S3Uploader)
require.True(t, ok)
require.Equal(t, "us-east-2", original.region)
require.Equal(t, "foo.bar.baz", original.bucket)
require.Equal(t, "access_key", original.accessKey)
require.Equal(t, "secret_key", original.secretKey)
require.Equal(t, "us-east-2", original.opts.Region)
require.Equal(t, "foo.bar.baz", original.opts.Bucket)
require.Equal(t, "access_key", original.opts.AccessKey)
require.Equal(t, "secret_key", original.opts.SecretKey)
})
t.Run("with bucket url https://s3.amazonaws.com/mybucket", func(t *testing.T) {
@ -55,10 +56,10 @@ func TestImageUploaderFactory(t *testing.T) {
original, ok := uploader.(*S3Uploader)
require.True(t, ok)
require.Equal(t, "us-east-1", original.region)
require.Equal(t, "my.bucket.com", original.bucket)
require.Equal(t, "access_key", original.accessKey)
require.Equal(t, "secret_key", original.secretKey)
require.Equal(t, "us-east-1", original.opts.Region)
require.Equal(t, "my.bucket.com", original.opts.Bucket)
require.Equal(t, "access_key", original.opts.AccessKey)
require.Equal(t, "secret_key", original.opts.SecretKey)
})
t.Run("with bucket url https://s3-us-west-2.amazonaws.com/mybucket", func(t *testing.T) {
@ -76,13 +77,114 @@ func TestImageUploaderFactory(t *testing.T) {
original, ok := uploader.(*S3Uploader)
require.True(t, ok)
require.Equal(t, "us-west-2", original.region)
require.Equal(t, "my.bucket.com", original.bucket)
require.Equal(t, "access_key", original.accessKey)
require.Equal(t, "secret_key", original.secretKey)
require.Equal(t, "us-west-2", original.opts.Region)
require.Equal(t, "my.bucket.com", original.opts.Bucket)
require.Equal(t, "access_key", original.opts.AccessKey)
require.Equal(t, "secret_key", original.opts.SecretKey)
})
})
t.Run("S3ImageUploader with presigned URLs disabled (default)", func(t *testing.T) {
cfg := setting.NewCfg()
err := cfg.Load(setting.CommandLineArgs{
HomePath: "../../../",
})
require.NoError(t, err)
cfg.ImageUploadProvider = "s3"
s3sec, err := cfg.Raw.GetSection("external_image_storage.s3")
require.NoError(t, err)
_, err = s3sec.NewKey("bucket", "test-bucket")
require.NoError(t, err)
_, err = s3sec.NewKey("region", "us-east-1")
require.NoError(t, err)
uploader, err := NewImageUploader(cfg)
require.NoError(t, err)
original, ok := uploader.(*S3Uploader)
require.True(t, ok)
require.False(t, original.opts.EnablePresignedURLs)
require.Equal(t, "public-read", original.opts.ACL)
require.Equal(t, 7*24*time.Hour, original.opts.PresignedURLExpiration)
})
t.Run("S3ImageUploader with presigned URLs enabled and custom expiration", func(t *testing.T) {
cfg := setting.NewCfg()
err := cfg.Load(setting.CommandLineArgs{
HomePath: "../../../",
})
require.NoError(t, err)
cfg.ImageUploadProvider = "s3"
s3sec, err := cfg.Raw.GetSection("external_image_storage.s3")
require.NoError(t, err)
_, err = s3sec.NewKey("bucket", "test-bucket")
require.NoError(t, err)
_, err = s3sec.NewKey("region", "us-east-1")
require.NoError(t, err)
_, err = s3sec.NewKey("enable_presigned_urls", "true")
require.NoError(t, err)
_, err = s3sec.NewKey("presigned_url_expiration", "48h")
require.NoError(t, err)
uploader, err := NewImageUploader(cfg)
require.NoError(t, err)
original, ok := uploader.(*S3Uploader)
require.True(t, ok)
require.True(t, original.opts.EnablePresignedURLs)
require.Equal(t, "public-read", original.opts.ACL)
require.Equal(t, 48*time.Hour, original.opts.PresignedURLExpiration)
})
t.Run("S3ImageUploader with unparseable presigned URL expiration", func(t *testing.T) {
cfg := setting.NewCfg()
err := cfg.Load(setting.CommandLineArgs{
HomePath: "../../../",
})
require.NoError(t, err)
cfg.ImageUploadProvider = "s3"
s3sec, err := cfg.Raw.GetSection("external_image_storage.s3")
require.NoError(t, err)
_, err = s3sec.NewKey("bucket", "test-bucket")
require.NoError(t, err)
_, err = s3sec.NewKey("region", "us-east-1")
require.NoError(t, err)
_, err = s3sec.NewKey("presigned_url_expiration", "notaduration")
require.NoError(t, err)
_, err = NewImageUploader(cfg)
require.Error(t, err)
})
t.Run("S3ImageUploader with negative presigned URL expiration", func(t *testing.T) {
cfg := setting.NewCfg()
err := cfg.Load(setting.CommandLineArgs{
HomePath: "../../../",
})
require.NoError(t, err)
cfg.ImageUploadProvider = "s3"
s3sec, err := cfg.Raw.GetSection("external_image_storage.s3")
require.NoError(t, err)
_, err = s3sec.NewKey("bucket", "test-bucket")
require.NoError(t, err)
_, err = s3sec.NewKey("region", "us-east-1")
require.NoError(t, err)
_, err = s3sec.NewKey("presigned_url_expiration", "-1h")
require.NoError(t, err)
_, err = NewImageUploader(cfg)
require.Error(t, err)
require.Contains(t, err.Error(), "presigned_url_expiration must be >= 0")
})
t.Run("Webdav uploader", func(t *testing.T) {
cfg := setting.NewCfg()
err := cfg.Load(setting.CommandLineArgs{

View file

@ -15,35 +15,65 @@ import (
"github.com/aws/aws-sdk-go/aws/defaults"
"github.com/aws/aws-sdk-go/aws/ec2metadata"
"github.com/aws/aws-sdk-go/aws/session"
"github.com/aws/aws-sdk-go/service/s3"
"github.com/aws/aws-sdk-go/service/s3/s3manager"
"github.com/aws/aws-sdk-go/service/sts"
"github.com/grafana/grafana/pkg/ifaces/s3ifaces"
"github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/util"
)
type S3Uploader struct {
endpoint string
region string
bucket string
path string
acl string
secretKey string
accessKey string
pathStyleAccess bool
log log.Logger
type S3UploaderOptions struct {
Endpoint string
Region string
Bucket string
Path string
ACL string
AccessKey string
SecretKey string
PathStyleAccess bool
EnablePresignedURLs bool
PresignedURLExpiration time.Duration
}
func NewS3Uploader(endpoint, region, bucket, path, acl, accessKey, secretKey string, pathStyleAccess bool) *S3Uploader {
type S3Uploader struct {
opts S3UploaderOptions
log log.Logger
}
// Stubbable by tests.
var newS3Client = func(cfg *aws.Config) (s3ifaces.S3Client, error) {
sess, err := session.NewSession(cfg)
if err != nil {
return nil, err
}
return &s3ClientWrapper{
uploader: s3manager.NewUploader(sess),
svc: s3.New(sess),
}, nil
}
type s3ClientWrapper struct {
uploader *s3manager.Uploader
svc *s3.S3
}
func (w *s3ClientWrapper) Upload(ctx context.Context, input *s3manager.UploadInput) (*s3manager.UploadOutput, error) {
return w.uploader.UploadWithContext(ctx, input)
}
func (w *s3ClientWrapper) PresignGetObject(bucket, key string, expiration time.Duration) (string, error) {
req, _ := w.svc.GetObjectRequest(&s3.GetObjectInput{
Bucket: aws.String(bucket),
Key: aws.String(key),
})
return req.Presign(expiration)
}
func NewS3Uploader(opts S3UploaderOptions) *S3Uploader {
return &S3Uploader{
endpoint: endpoint,
region: region,
bucket: bucket,
path: path,
acl: acl,
accessKey: accessKey,
secretKey: secretKey,
pathStyleAccess: pathStyleAccess,
log: log.New("s3uploader"),
opts: opts,
log: log.New("s3uploader"),
}
}
@ -55,17 +85,17 @@ func (u *S3Uploader) Upload(ctx context.Context, imageDiskPath string) (string,
creds := credentials.NewChainCredentials(
[]credentials.Provider{
&credentials.StaticProvider{Value: credentials.Value{
AccessKeyID: u.accessKey,
SecretAccessKey: u.secretKey,
AccessKeyID: u.opts.AccessKey,
SecretAccessKey: u.opts.SecretKey,
}},
&credentials.EnvProvider{},
webIdentityProvider(sess),
remoteCredProvider(sess),
})
cfg := &aws.Config{
Region: aws.String(u.region),
Endpoint: aws.String(u.endpoint),
S3ForcePathStyle: aws.Bool(u.pathStyleAccess),
Region: aws.String(u.opts.Region),
Endpoint: aws.String(u.opts.Endpoint),
S3ForcePathStyle: aws.Bool(u.opts.PathStyleAccess),
Credentials: creds,
}
@ -73,8 +103,8 @@ func (u *S3Uploader) Upload(ctx context.Context, imageDiskPath string) (string,
if err != nil {
return "", err
}
key := u.path + rand + pngExt
u.log.Debug("Uploading image to s3.", "bucket", u.bucket, "path", key)
key := u.opts.Path + rand + pngExt
u.log.Debug("Uploading image to s3.", "bucket", u.opts.Bucket, "path", key)
// We can ignore the gosec G304 warning on this one because `imageDiskPath` comes
// from alert notifiers and is only used to upload images generated by alerting.
@ -89,22 +119,30 @@ func (u *S3Uploader) Upload(ctx context.Context, imageDiskPath string) (string,
}
}()
sess, err = session.NewSession(cfg)
s3Client, err := newS3Client(cfg)
if err != nil {
return "", err
}
uploader := s3manager.NewUploader(sess)
result, err := uploader.UploadWithContext(ctx, &s3manager.UploadInput{
Bucket: aws.String(u.bucket),
uploadInput := &s3manager.UploadInput{
Bucket: aws.String(u.opts.Bucket),
Key: aws.String(key),
ACL: aws.String(u.acl),
Body: file,
ContentType: aws.String("image/png"),
})
}
if !u.opts.EnablePresignedURLs {
uploadInput.ACL = aws.String(u.opts.ACL)
}
result, err := s3Client.Upload(ctx, uploadInput)
if err != nil {
return "", err
}
if u.opts.EnablePresignedURLs {
return s3Client.PresignGetObject(u.opts.Bucket, key, u.opts.PresignedURLExpiration)
}
return result.Location, nil
}

View file

@ -2,9 +2,18 @@ package imguploader
import (
"context"
"os"
"path/filepath"
"testing"
"time"
"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/s3/s3manager"
"github.com/golang/mock/gomock"
"github.com/grafana/grafana/pkg/ifaces/s3ifaces"
"github.com/grafana/grafana/pkg/mocks/mock_s3ifaces"
"github.com/grafana/grafana/pkg/setting"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
@ -25,3 +34,90 @@ func TestUploadToS3(t *testing.T) {
require.NotEqual(t, "", path)
})
}
func stubS3Client(t *testing.T, mock s3ifaces.S3Client) {
t.Helper()
orig := newS3Client
t.Cleanup(func() { newS3Client = orig })
newS3Client = func(_ *aws.Config) (s3ifaces.S3Client, error) {
return mock, nil
}
}
func TestS3Upload(t *testing.T) {
const (
bucket = "test-bucket"
region = "us-east-1"
imgPath = "images/"
publicURL = "https://test-bucket.s3.amazonaws.com/images/test.png"
)
tmpDir := t.TempDir()
fpath := filepath.Join(tmpDir, "test.png")
require.NoError(t, os.WriteFile(fpath, []byte("fake png"), 0600))
t.Run("without presigned URLs sets ACL and returns location", func(t *testing.T) {
ctx := context.Background()
ctrl := gomock.NewController(t)
m := mock_s3ifaces.NewMockS3Client(ctrl)
m.EXPECT().
Upload(gomock.Eq(ctx), gomock.Any()).
DoAndReturn(func(_ context.Context, input *s3manager.UploadInput) (*s3manager.UploadOutput, error) {
assert.Equal(t, bucket, aws.StringValue(input.Bucket))
assert.Equal(t, "image/png", aws.StringValue(input.ContentType))
assert.NotNil(t, input.ACL, "ACL should be set when presigned URLs are disabled")
assert.Equal(t, "public-read", aws.StringValue(input.ACL))
return &s3manager.UploadOutput{Location: publicURL}, nil
})
stubS3Client(t, m)
uploader := NewS3Uploader(S3UploaderOptions{
Region: region,
Bucket: bucket,
Path: imgPath,
ACL: "public-read",
EnablePresignedURLs: false,
})
url, err := uploader.Upload(ctx, fpath)
require.NoError(t, err)
assert.Equal(t, publicURL, url)
})
t.Run("with presigned URLs omits ACL and returns presigned URL", func(t *testing.T) {
ctx := context.Background()
ctrl := gomock.NewController(t)
const presignedURL = "https://test-bucket.s3.amazonaws.com/images/abc.png?X-Amz-Signature=sig"
expiration := 48 * time.Hour
m := mock_s3ifaces.NewMockS3Client(ctrl)
m.EXPECT().
Upload(gomock.Eq(ctx), gomock.Any()).
DoAndReturn(func(_ context.Context, input *s3manager.UploadInput) (*s3manager.UploadOutput, error) {
assert.Equal(t, bucket, aws.StringValue(input.Bucket))
assert.Nil(t, input.ACL, "ACL should not be set when presigned URLs are enabled")
return &s3manager.UploadOutput{Location: publicURL}, nil
})
m.EXPECT().
PresignGetObject(gomock.Eq(bucket), gomock.Any(), gomock.Eq(expiration)).
Return(presignedURL, nil)
stubS3Client(t, m)
uploader := NewS3Uploader(S3UploaderOptions{
Region: region,
Bucket: bucket,
Path: imgPath,
ACL: "public-read",
EnablePresignedURLs: true,
PresignedURLExpiration: expiration,
})
url, err := uploader.Upload(ctx, fpath)
require.NoError(t, err)
assert.Equal(t, presignedURL, url)
})
}

View file

@ -0,0 +1,17 @@
// Package s3ifaces provides interfaces for AWS S3.
//
//go:generate mockgen -source $GOFILE -destination ../../mocks/mock_s3ifaces/mocks.go S3Client
package s3ifaces
import (
"context"
"time"
"github.com/aws/aws-sdk-go/service/s3/s3manager"
)
// S3Client wraps the S3 operations needed by the image uploader.
type S3Client interface {
Upload(ctx context.Context, input *s3manager.UploadInput) (*s3manager.UploadOutput, error)
PresignGetObject(bucket, key string, expiration time.Duration) (string, error)
}

View file

@ -0,0 +1,68 @@
// Code generated by MockGen. DO NOT EDIT.
// Source: s3ifaces.go
// Package mock_s3ifaces is a generated GoMock package.
package mock_s3ifaces
import (
context "context"
reflect "reflect"
time "time"
gomock "github.com/golang/mock/gomock"
s3manager "github.com/aws/aws-sdk-go/service/s3/s3manager"
)
// MockS3Client is a mock of S3Client interface
type MockS3Client struct {
ctrl *gomock.Controller
recorder *MockS3ClientMockRecorder
}
// MockS3ClientMockRecorder is the mock recorder for MockS3Client
type MockS3ClientMockRecorder struct {
mock *MockS3Client
}
// NewMockS3Client creates a new mock instance
func NewMockS3Client(ctrl *gomock.Controller) *MockS3Client {
mock := &MockS3Client{ctrl: ctrl}
mock.recorder = &MockS3ClientMockRecorder{mock}
return mock
}
// EXPECT returns an object that allows the caller to indicate expected use
func (m *MockS3Client) EXPECT() *MockS3ClientMockRecorder {
return m.recorder
}
// Upload mocks base method
func (m *MockS3Client) Upload(ctx context.Context, input *s3manager.UploadInput) (*s3manager.UploadOutput, error) {
m.ctrl.T.Helper()
ret := m.ctrl.Call(m, "Upload", ctx, input)
ret0, _ := ret[0].(*s3manager.UploadOutput)
ret1, _ := ret[1].(error)
return ret0, ret1
}
// Upload indicates an expected call of Upload
func (mr *MockS3ClientMockRecorder) Upload(ctx, input any) *gomock.Call {
mr.mock.ctrl.T.Helper()
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Upload", reflect.TypeOf((*MockS3Client)(nil).Upload), ctx, input)
}
// PresignGetObject mocks base method
func (m *MockS3Client) PresignGetObject(bucket, key string, expiration time.Duration) (string, error) {
m.ctrl.T.Helper()
ret := m.ctrl.Call(m, "PresignGetObject", bucket, key, expiration)
ret0, _ := ret[0].(string)
ret1, _ := ret[1].(error)
return ret0, ret1
}
// PresignGetObject indicates an expected call of PresignGetObject
func (mr *MockS3ClientMockRecorder) PresignGetObject(bucket, key, expiration any) *gomock.Call {
mr.mock.ctrl.T.Helper()
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "PresignGetObject", reflect.TypeOf((*MockS3Client)(nil).PresignGetObject), bucket, key, expiration)
}