Skip to content

Commit 7c6d261

Browse files
shanduurshanduur
committed
fix: set content-disposition on S3
S3 does not set proper Content-Disposition, and this header might be ignored by e.g. browsers if set before redirect. We need to include it when adding new object to the cache. Additionally, if the object was previously cached, we need to ensure that Content-Disposition header will be properly updated. Signed-off-by: Mateusz Urbanek <mateusz.urbanek@siderolabs.com>
1 parent f3e97df commit 7c6d261

File tree

9 files changed

+40
-14
lines changed

9 files changed

+40
-14
lines changed

‎internal/asset/asset.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ func (b *Builder) getBuildAsset(ctx context.Context, versionString, arch string,
118118
//
119119
// First, check if the asset has already been built and cached then use the cached version.
120120
// If the asset hasn't been built yet, build it and cache it honoring the concurrency limit, and push it to the cache.
121-
func (b *Builder) Build(ctx context.Context, prof profile.Profile, versionString string) (BootAsset, error) {
121+
func (b *Builder) Build(ctx context.Context, prof profile.Profile, versionString, filename string) (BootAsset, error) {
122122
profileHash, err := factoryprofile.Hash(prof)
123123
if err != nil {
124124
return nil, err
@@ -138,7 +138,7 @@ func (b *Builder) Build(ctx context.Context, prof profile.Profile, versionString
138138

139139
// nothing in cache, so build the asset, but make sure we do it only once
140140
ch := b.sf.DoChan(profileHash, func() (any, error) { //nolint:contextcheck
141-
return b.buildAndCache(profileHash, prof, versionString)
141+
return b.buildAndCache(profileHash, prof, versionString, filename)
142142
})
143143

144144
select {
@@ -162,7 +162,7 @@ func (b *Builder) Build(ctx context.Context, prof profile.Profile, versionString
162162
}
163163

164164
// buildAndCache builds the asset and pushes it to the cache.
165-
func (b *Builder) buildAndCache(profileHash string, prof profile.Profile, versionString string) (BootAsset, error) {
165+
func (b *Builder) buildAndCache(profileHash string, prof profile.Profile, versionString, filename string) (BootAsset, error) {
166166
// detach the context to make sure the asset is built no matter if the request is canceled
167167
ctx, cancel := context.WithTimeout(context.Background(), 20*time.Minute)
168168
defer cancel()
@@ -175,7 +175,7 @@ func (b *Builder) buildAndCache(profileHash string, prof profile.Profile, versio
175175
b.metricAssetsBuilt.WithLabelValues(versionString, prof.Output.Kind.String(), prof.Arch).Inc()
176176
b.metricAssetBytesBuilt.WithLabelValues(versionString, prof.Output.Kind.String(), prof.Arch).Add(float64(asset.Size()))
177177

178-
if err = b.cache.Put(ctx, profileHash, asset); err != nil {
178+
if err = b.cache.Put(ctx, profileHash, asset, filename); err != nil {
179179
b.logger.Error("error putting asset to cache", zap.Error(err), zap.String("profile_hash", profileHash))
180180
}
181181

‎internal/asset/cache/cache.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,5 +40,5 @@ type RedirectableAsset interface {
4040
// Cache is an interface for a cache that stores boot assets.
4141
type Cache interface {
4242
Get(ctx context.Context, profileID string) (BootAsset, error)
43-
Put(ctx context.Context, profileID string, asset BootAsset) error
43+
Put(ctx context.Context, profileID string, asset BootAsset, filename string) error
4444
}

‎internal/asset/cache/cdn/cdn.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,6 @@ func (c *Cache) Get(ctx context.Context, profileID string) (cache.BootAsset, err
6868
}
6969

7070
// Put uploads the boot asset to the registry.
71-
func (c *Cache) Put(ctx context.Context, profileID string, asset cache.BootAsset) error {
72-
return c.underlying.Put(ctx, profileID, asset)
71+
func (c *Cache) Put(ctx context.Context, profileID string, asset cache.BootAsset, filename string) error {
72+
return c.underlying.Put(ctx, profileID, asset, filename)
7373
}

‎internal/asset/cache/registry/registry.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,7 @@ func (c *Cache) Get(ctx context.Context, profileID string) (cache.BootAsset, err
139139
}
140140

141141
// Put uploads the boot asset to the registry.
142-
func (c *Cache) Put(ctx context.Context, profileID string, asset cache.BootAsset) error {
142+
func (c *Cache) Put(ctx context.Context, profileID string, asset cache.BootAsset, _ string) error {
143143
taggedRef := c.cacheRepository.Tag(profileID)
144144

145145
c.logger.Info("pushing cached image", zap.Stringer("ref", taggedRef))

‎internal/asset/cache/s3/s3.go‎

Lines changed: 23 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"context"
1010
"errors"
1111
"fmt"
12+
"net/http"
1213
"path"
1314
"time"
1415

@@ -115,6 +116,11 @@ func (c *Cache) Get(ctx context.Context, profileID string) (cache.BootAsset, err
115116
return nil, fmt.Errorf("error getting object stat for object %q: %w", key, err)
116117
}
117118

119+
// we need to fix metadata if it's not present
120+
if !hasRequiredMetadata(stat.Metadata) {
121+
return nil, cache.ErrCacheNotFound
122+
}
123+
118124
redirect, err := c.s3cli.PresignedGetObject(ctx, c.bucketName, key, expires, nil)
119125
if err != nil {
120126
var minioErr minio.ErrorResponse
@@ -148,15 +154,17 @@ func (c *Cache) Get(ctx context.Context, profileID string) (cache.BootAsset, err
148154
}
149155

150156
// Put uploads the boot asset to the registry.
151-
func (c *Cache) Put(ctx context.Context, profileID string, asset cache.BootAsset) error {
157+
func (c *Cache) Put(ctx context.Context, profileID string, asset cache.BootAsset, filename string) error {
152158
key := path.Join(prefix, profileID)
153159

154160
data, err := asset.Reader()
155161
if err != nil {
156162
return fmt.Errorf("error getting reader for asset from profile %q: %w", profileID, err)
157163
}
158164

159-
stat, err := c.s3cli.PutObject(ctx, c.bucketName, key, data, asset.Size(), minio.PutObjectOptions{})
165+
stat, err := c.s3cli.PutObject(ctx, c.bucketName, key, data, asset.Size(), minio.PutObjectOptions{
166+
ContentDisposition: fmt.Sprintf(`attachment; filename="%s"`, filename),
167+
})
160168
if err != nil {
161169
var minioErr minio.ErrorResponse
162170
if errors.As(err, &minioErr) {
@@ -184,9 +192,21 @@ func (c *Cache) Put(ctx context.Context, profileID string, asset cache.BootAsset
184192
return fmt.Errorf("error creating reference asset for profile %q: %w", profileID, err)
185193
}
186194

187-
if err := c.signingCache.Put(ctx, profileID, ref); err != nil {
195+
if err := c.signingCache.Put(ctx, profileID, ref, filename); err != nil {
188196
return fmt.Errorf("error putting asset reference for profile %q to registry: %w", profileID, err)
189197
}
190198

191199
return nil
192200
}
201+
202+
func hasRequiredMetadata(metadata http.Header) bool {
203+
for _, key := range []string{
204+
"Content-Disposition",
205+
} {
206+
if metadata.Get(key) == "" {
207+
return false
208+
}
209+
}
210+
211+
return true
212+
}

‎internal/frontend/http/image.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ func (f *Frontend) handleImage(ctx context.Context, w http.ResponseWriter, r *ht
5757
return fmt.Errorf("error validating profile: %w", err)
5858
}
5959

60-
asset, err := f.assetBuilder.Build(ctx, prof, version.String())
60+
asset, err := f.assetBuilder.Build(ctx, prof, version.String(), path)
6161
if err != nil {
6262
return err
6363
}

‎internal/frontend/http/pxe.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ func (f *Frontend) handlePXE(ctx context.Context, w http.ResponseWriter, _ *http
6363
}
6464

6565
// build the cmdline
66-
asset, err := f.assetBuilder.Build(ctx, prof, version.String())
66+
asset, err := f.assetBuilder.Build(ctx, prof, version.String(), path)
6767
if err != nil {
6868
return err
6969
}

‎internal/frontend/http/registry.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -254,7 +254,7 @@ func (f *Frontend) buildInstallImage(ctx context.Context, img requestedImage, sc
254254

255255
var asset asset.BootAsset
256256

257-
asset, err = f.assetBuilder.Build(ctx, prof, version.String())
257+
asset, err = f.assetBuilder.Build(ctx, prof, version.String(), img.Name())
258258
if err != nil {
259259
return v1.Hash{}, err
260260
}

‎internal/integration/download_test.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,7 @@ func downloadAssetAndMatchSize(ctx context.Context, t *testing.T, baseURL string
104104
body := resp.Body
105105

106106
require.Equal(t, http.StatusOK, resp.StatusCode)
107+
require.Contains(t, resp.Header, "Content-Disposition")
107108

108109
size := matchSizeAndType(t, body, fileType, expectedSize)
109110

@@ -128,6 +129,7 @@ func downloadDiskImageMatchSizeAndPartitions(ctx context.Context, t *testing.T,
128129
body := io.TeeReader(resp.Body, out)
129130

130131
require.Equal(t, http.StatusOK, resp.StatusCode)
132+
require.Contains(t, resp.Header, "Content-Disposition")
131133

132134
size := matchSizeAndType(t, body, fileType, expectedSize)
133135

@@ -188,6 +190,7 @@ func downloadAssetAndValidateInitramfs(ctx context.Context, t *testing.T, baseUR
188190
body := resp.Body
189191

190192
require.Equal(t, http.StatusOK, resp.StatusCode)
193+
require.Contains(t, resp.Header, "Content-Disposition")
191194

192195
d := t.TempDir()
193196
initramfsPath := filepath.Join(d, "initramfs.xz")
@@ -212,6 +215,7 @@ func downloadAssetAndValidateUKI(ctx context.Context, t *testing.T, baseURL stri
212215
body := resp.Body
213216

214217
require.Equal(t, http.StatusOK, resp.StatusCode)
218+
require.Contains(t, resp.Header, "Content-Disposition")
215219

216220
d := t.TempDir()
217221
ukiPath := filepath.Join(d, "uki.efi")
@@ -236,6 +240,7 @@ func downloadInstallerAndValidateUKI(ctx context.Context, t *testing.T, baseURL
236240
body := resp.Body
237241

238242
require.Equal(t, http.StatusOK, resp.StatusCode)
243+
require.Contains(t, resp.Header, "Content-Disposition")
239244

240245
d := t.TempDir()
241246
installerPath := filepath.Join(d, "installer.tar")
@@ -260,6 +265,7 @@ func downloadCmdlineAndMatch(ctx context.Context, t *testing.T, baseURL string,
260265
body := resp.Body
261266

262267
require.Equal(t, http.StatusOK, resp.StatusCode)
268+
require.Contains(t, resp.Header, "Content-Disposition")
263269

264270
cmdlineBytes, err := io.ReadAll(body)
265271
require.NoError(t, err)

0 commit comments

Comments
 (0)