Skip to content

Commit 81f8013

Browse files
arepala-umlarepala-uml
authored
fix(pkgmgr): address PR #561 review follow-ups (#586)
* fix(pkgmgr): address PR #561 review follow-ups Closes #562 - activate() built the file-install-step symlink source from the raw, un-rendered filename while the destination used the rendered path, producing a dangling symlink for any package with a templated (e.g. per-OS/ARCH) filename. Both now use the rendered path. - PackageManager.Install() and Upgrade() never ran Package.validate() before installing, so a malformed package (bad archive config, missing docker image, etc.) skipped straight to install() instead of failing with a clear validation error. Upgrade() in particular could tear down the working installed version before hitting the failure. - url-sourced file installs used context.Background() with no deadline, leaving a stalled server able to hang the install indefinitely despite the existing size cap. - The tar.gz decompression cap was a fixed 512 MiB applied to the whole archive, with no way for a package to raise it for a legitimately large multi-arch/bundle release. Added an optional archiveMaxSize file-install-step field (capped at 4 GiB) that raises the effective decompression limit for that package's archive. - Cleaned up stale doc comments and pre-existing (from #561) lines over the 80-char guideline in pkgmgr/archive.go and its tests. Signed-off-by: Akhil Repala <arepala@blinklabs.io> * fix(pkgmgr): address review feedback on PR #561 follow-ups - TestUpgradeRejectsInvalidNewPackageVersion's upgrade-target package had no filePath set, so Package.validate() rejected it on the file-path check before ever reaching the missing content/source/url check the test's comment describes. Set a valid filePath so the test exercises the path it claims to. - Documented that archiveMaxSize bounds cumulative decompressed data for tar.gz/tgz, not just the selected entry (only ZIP's central directory lets extraction skip decompressing non-matching entries). Signed-off-by: Akhil Repala <arepala@blinklabs.io> * fix(pkgmgr): fix golangci-lint failures on archive extraction helpers - gosec G115: guard extractZipFileWithLimit against a negative maxSize before the int64 -> uint64 conversion used to compare against UncompressedSize64. maxSize is always non-negative on every current call path, but the linter can't prove that once it's a parameter rather than a constant. - unused: extractArchiveFile, extractZipFile, and extractTarGzFile were only reachable from _test.go files after the archiveMaxSize refactor routed production code through the ...WithLimit variants directly. This repo's .golangci.yml sets run.tests: false, so the linter never saw those test-only call sites. Removed the dead wrappers and updated their test call sites to call the ...WithLimit functions directly with maxArchiveEntrySize. Verified with golangci-lint run ./... locally: 0 issues. Signed-off-by: Akhil Repala <arepala@blinklabs.io> * fix(pkgmgr): validate the whole install/upgrade plan before mutating - Install() and Upgrade() validated each resolved package one at a time, inside the same loop that installs/uninstalls it. A resolver-ordered plan with multiple entries (e.g. a requested package's dependencies, or a new dependency pulled in by an upgrade) could partially apply: earlier, valid entries would install and persist - or, on upgrade, deactivate/uninstall their old version - before a later, invalid entry in the same plan was ever rejected. Reproduced with a focused probe (a "dep" that installs and persists before an invalid "app" that depends on it is rejected) before fixing. Both now validate every entry in the resolved plan upfront, before any install/uninstall/deactivate work begins. - The url-download-timeout test called install() synchronously, so a regressed production timeout would hang the call forever (the test server's handler only unblocks after install() returns) rather than failing the test - confirmed by reverting the production fix and measuring the test hang. Now runs install() in a goroutine under a bounded watchdog and asserts the returned error is actually context.DeadlineExceeded, not just any error. Both new regression tests were verified to actually fail without their corresponding fix (probed manually, then confirmed via the automated test), then verified to pass once restored. Addresses review feedback from @wolf31o2 on PR #586. Signed-off-by: Akhil Repala <arepala@blinklabs.io> * fix(pkgmgr): render templated filename in uninstall(), not just install/activate PackageInstallStepFile.uninstall() built the delete path from the raw, un-rendered Filename while install() and activate() already used the rendered path. For a package with a templated (e.g. per-OS/ARCH) filename, os.Remove targeted a literal path like ".../{{ .System.OS }}" that never existed, silently no-op'd on fs.ErrNotExist, and left the real rendered file (e.g. binary-linux) behind. This mattered most on upgrade: Upgrade() calls uninstallPackage with keepData=true, which skips the whole-directory cleanup, so the per-step uninstall() was the only thing that could have removed the old version's rendered file - and didn't. Render the filename the same way install()/activate() do before building the delete path. Added a focused unit test on uninstall() directly, and a production-shaped Install-then-Upgrade regression test matching the exact scenario reported in review. Both verified to fail without the fix and pass with it. Addresses review feedback from @wolf31o2 on PR #586. Signed-off-by: Akhil Repala <arepala@blinklabs.io> --------- Signed-off-by: Akhil Repala <arepala@blinklabs.io>
1 parent 3bbc496 commit 81f8013

7 files changed

Lines changed: 927 additions & 82 deletions

File tree

‎README.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -367,6 +367,7 @@ installSteps:
367367
| `binary` | | Whether this file is an executable file for the package (expects bool, defaults to `false`) |
368368
| `archive` | | Archive format that `source` or `url` content should be extracted from. One of `zip`, `tar.gz`, or `tgz` |
369369
| `archivePath` | | Path of the file within the archive to extract as the destination file content. Required if `archive` is set. Supports templating |
370+
| `archiveMaxSize` | | Maximum decompressed size in bytes. For `zip`, this limits the selected entry. For `tar.gz` and `tgz`, this limits total decompressed archive data. Overrides the default of 512 MiB. Only valid if `archive` is set. Capped at 4 GiB |
370371

371372
###### `config`
372373

‎pkgmgr/archive.go‎

Lines changed: 61 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -24,22 +24,45 @@ import (
2424
"io"
2525
"path/filepath"
2626
"strings"
27+
"time"
2728
)
2829

2930
// Supported values for the file install step's archive field
3031
const (
31-
archiveTypeZip = "zip"
32-
archiveTypeTarGz = "tar.gz"
33-
archiveTypeTgz = "tgz"
34-
maxArchiveEntrySize = int64(512 * 1024 * 1024) // 512 MiB
32+
archiveTypeZip = "zip"
33+
archiveTypeTarGz = "tar.gz"
34+
archiveTypeTgz = "tgz"
3535
)
3636

37-
// maxDownloadSize bounds how much of a url-sourced file install step is
38-
// buffered into memory, so an oversized or malicious response can't exhaust
39-
// process memory before archive extraction limits even apply. It's a var
40-
// rather than a const so tests can lower it without downloading 512MiB+.
37+
// maxArchiveEntrySize bounds the decompressed size of any single file read
38+
// from an archive or the local filesystem. A file install step can raise
39+
// this for its own archive via archiveMaxSize, up to maxArchiveSizeCeiling.
40+
// It's a var rather than a const so tests can lower it without needing
41+
// multi-GiB test data.
42+
var maxArchiveEntrySize = int64(512 * 1024 * 1024) // 512 MiB
43+
44+
// maxArchiveSizeCeiling bounds how high a package's archiveMaxSize override
45+
// can raise maxArchiveEntrySize. tar.gz extraction has to bound decompressed
46+
// bytes as it streams through the archive looking for the requested entry
47+
// (there's no central directory to consult up front, unlike ZIP), so this
48+
// ceiling keeps a careless or malicious override from turning that bound
49+
// into an effectively unbounded decompression sink. It's a var rather than
50+
// a const so tests can lower it without needing multi-GiB test data.
51+
var maxArchiveSizeCeiling = int64(4 * 1024 * 1024 * 1024) // 4 GiB
52+
53+
// maxDownloadSize bounds how much of a url- or source-sourced file install
54+
// step is buffered into memory, so an oversized or malicious response or
55+
// local file can't exhaust process memory before archive extraction limits
56+
// even apply. It's a var rather than a const so tests can lower it without
57+
// downloading 512MiB+.
4158
var maxDownloadSize = maxArchiveEntrySize
4259

60+
// downloadTimeout bounds how long a url-sourced file install step's HTTP
61+
// request may run, closing the DoS gap that maxDownloadSize's size cap
62+
// leaves open against a slow or stalling server. It's a var rather than a
63+
// const so tests can lower it without waiting out the full timeout.
64+
var downloadTimeout = 5 * time.Minute
65+
4366
// validArchiveType returns whether archiveType is a supported archive type
4467
// for the file install step
4568
func validArchiveType(archiveType string) bool {
@@ -51,25 +74,38 @@ func validArchiveType(archiveType string) bool {
5174
}
5275
}
5376

54-
// extractArchiveFile returns the content of the file at archivePath within
55-
// the archive represented by data. The archive is expected to be in the
56-
// format specified by archiveType (zip, tar.gz, or tgz)
57-
func extractArchiveFile(
77+
// extractArchiveFileWithLimit returns the content of the file at
78+
// archivePath within the archive represented by data, bounded by maxSize.
79+
// The archive is expected to be in the format specified by archiveType
80+
// (zip, tar.gz, or tgz). A file install step can raise maxSize for its own
81+
// archive via archiveMaxSize.
82+
func extractArchiveFileWithLimit(
5883
archiveType string,
5984
archivePath string,
6085
data []byte,
86+
maxSize int64,
6187
) ([]byte, error) {
6288
switch strings.ToLower(archiveType) {
6389
case archiveTypeZip:
64-
return extractZipFile(archivePath, data)
90+
return extractZipFileWithLimit(archivePath, data, maxSize)
6591
case archiveTypeTarGz, archiveTypeTgz:
66-
return extractTarGzFile(archivePath, data)
92+
return extractTarGzFileWithLimit(archivePath, data, maxSize)
6793
default:
6894
return nil, fmt.Errorf("unsupported archive type %q", archiveType)
6995
}
7096
}
7197

72-
func extractZipFile(archivePath string, data []byte) ([]byte, error) {
98+
func extractZipFileWithLimit(
99+
archivePath string,
100+
data []byte,
101+
maxSize int64,
102+
) ([]byte, error) {
103+
if maxSize < 0 {
104+
return nil, fmt.Errorf(
105+
"invalid maxSize %d: must not be negative",
106+
maxSize,
107+
)
108+
}
73109
zipReader, err := zip.NewReader(bytes.NewReader(data), int64(len(data)))
74110
if err != nil {
75111
return nil, fmt.Errorf("failed to read zip archive: %w", err)
@@ -85,31 +121,23 @@ func extractZipFile(archivePath string, data []byte) ([]byte, error) {
85121
if filepath.Clean(zipFile.Name) != cleanPath {
86122
continue
87123
}
88-
if zipFile.UncompressedSize64 > uint64(maxArchiveEntrySize) {
124+
if zipFile.UncompressedSize64 > uint64(maxSize) {
89125
return nil, fmt.Errorf(
90126
"file %q exceeds maximum allowed size of %d bytes",
91127
archivePath,
92-
maxArchiveEntrySize,
128+
maxSize,
93129
)
94130
}
95131
zf, err := zipFile.Open()
96132
if err != nil {
97133
return nil, err
98134
}
99135
defer zf.Close()
100-
return readArchiveEntry(zf, archivePath, maxArchiveEntrySize)
136+
return readArchiveEntry(zf, archivePath, maxSize)
101137
}
102138
return nil, fmt.Errorf("file %q not found in zip archive", archivePath)
103139
}
104140

105-
func extractTarGzFile(archivePath string, data []byte) ([]byte, error) {
106-
return extractTarGzFileWithLimit(
107-
archivePath,
108-
data,
109-
maxArchiveEntrySize,
110-
)
111-
}
112-
113141
func extractTarGzFileWithLimit(
114142
archivePath string,
115143
data []byte,
@@ -140,17 +168,17 @@ func extractTarGzFileWithLimit(
140168
if filepath.Clean(header.Name) != cleanPath {
141169
continue
142170
}
143-
if header.Size > maxArchiveEntrySize {
171+
if header.Size > maxDecompressedSize {
144172
return nil, fmt.Errorf(
145173
"file %q exceeds maximum allowed size of %d bytes",
146174
archivePath,
147-
maxArchiveEntrySize,
175+
maxDecompressedSize,
148176
)
149177
}
150178
content, err := readArchiveEntry(
151179
tarReader,
152180
archivePath,
153-
maxArchiveEntrySize,
181+
maxDecompressedSize,
154182
)
155183
if err != nil {
156184
return nil, err
@@ -216,8 +244,10 @@ func drainTarGzArchive(
216244
return nil
217245
}
218246

219-
// readArchiveEntry limits extraction even if an archive reports an incorrect
220-
// uncompressed size in its metadata.
247+
// readArchiveEntry reads from reader up to maxSize bytes, returning an error
248+
// if more remains. It's a generic bounded reader used for archive entries
249+
// (even when one reports an incorrect uncompressed size in its metadata), a
250+
// raw local file opened via source, and a raw HTTP response body.
221251
func readArchiveEntry(
222252
reader io.Reader,
223253
archivePath string,

‎pkgmgr/archive_test.go‎

Lines changed: 53 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,11 @@ func TestExtractZipFile(t *testing.T) {
9494
"README.md": "docs",
9595
})
9696

97-
content, err := extractZipFile("bin/mybinary", data)
97+
content, err := extractZipFileWithLimit(
98+
"bin/mybinary",
99+
data,
100+
maxArchiveEntrySize,
101+
)
98102
if err != nil {
99103
t.Fatalf("unexpected error: %s", err)
100104
}
@@ -114,7 +118,8 @@ func TestExtractZipFileNotFound(t *testing.T) {
114118
"bin/mybinary": testArchiveFileContent,
115119
})
116120

117-
if _, err := extractZipFile("bin/missing", data); err == nil {
121+
_, err := extractZipFileWithLimit("bin/missing", data, maxArchiveEntrySize)
122+
if err == nil {
118123
t.Fatal("expected error for missing file in archive, got nil")
119124
}
120125
}
@@ -131,7 +136,8 @@ func TestExtractZipFileSkipsDirs(t *testing.T) {
131136
t.Fatalf("unexpected error closing zip writer: %s", err)
132137
}
133138

134-
if _, err := extractZipFile("bin", buf.Bytes()); err == nil {
139+
_, err := extractZipFileWithLimit("bin", buf.Bytes(), maxArchiveEntrySize)
140+
if err == nil {
135141
t.Fatal("expected error when requested path is a directory, got nil")
136142
}
137143
}
@@ -157,7 +163,12 @@ func TestExtractZipFileSkipsSymlinks(t *testing.T) {
157163
t.Fatalf("unexpected error closing zip writer: %s", err)
158164
}
159165

160-
if _, err := extractZipFile("bin/mybinary", buf.Bytes()); err == nil {
166+
_, err = extractZipFileWithLimit(
167+
"bin/mybinary",
168+
buf.Bytes(),
169+
maxArchiveEntrySize,
170+
)
171+
if err == nil {
161172
t.Fatal("expected error when requested path is a symlink, got nil")
162173
}
163174
}
@@ -170,7 +181,11 @@ func TestExtractTarGzFile(t *testing.T) {
170181
"README.md": "docs",
171182
})
172183

173-
content, err := extractTarGzFile("bin/mybinary", data)
184+
content, err := extractTarGzFileWithLimit(
185+
"bin/mybinary",
186+
data,
187+
maxArchiveEntrySize,
188+
)
174189
if err != nil {
175190
t.Fatalf("unexpected error: %s", err)
176191
}
@@ -190,7 +205,12 @@ func TestExtractTarGzFileNotFound(t *testing.T) {
190205
"bin/mybinary": testArchiveFileContent,
191206
})
192207

193-
if _, err := extractTarGzFile("bin/missing", data); err == nil {
208+
_, err := extractTarGzFileWithLimit(
209+
"bin/missing",
210+
data,
211+
maxArchiveEntrySize,
212+
)
213+
if err == nil {
194214
t.Fatal("expected error for missing file in archive, got nil")
195215
}
196216
}
@@ -203,7 +223,12 @@ func TestExtractTarGzFileCorruptChecksum(t *testing.T) {
203223
})
204224
data[len(data)-8] ^= 0xff
205225

206-
if _, err := extractTarGzFile("bin/mybinary", data); err == nil {
226+
_, err := extractTarGzFileWithLimit(
227+
"bin/mybinary",
228+
data,
229+
maxArchiveEntrySize,
230+
)
231+
if err == nil {
207232
t.Fatal("expected error for invalid gzip checksum, got nil")
208233
}
209234
}
@@ -228,8 +253,14 @@ func TestExtractTarGzFileCumulativeSizeLimit(t *testing.T) {
228253
// TestExtractArchiveFileDispatch checks that each supported archive name is
229254
// routed to the correct extractor, including aliases and different casing.
230255
func TestExtractArchiveFileDispatch(t *testing.T) {
231-
zipData := buildTestZip(t, map[string]string{"mybinary": testArchiveFileContent})
232-
tarGzData := buildTestTarGz(t, map[string]string{"mybinary": testArchiveFileContent})
256+
zipData := buildTestZip(
257+
t,
258+
map[string]string{"mybinary": testArchiveFileContent},
259+
)
260+
tarGzData := buildTestTarGz(
261+
t,
262+
map[string]string{"mybinary": testArchiveFileContent},
263+
)
233264

234265
testDefs := []struct {
235266
archiveType string
@@ -241,7 +272,12 @@ func TestExtractArchiveFileDispatch(t *testing.T) {
241272
{archiveType: "tgz", data: tarGzData},
242273
}
243274
for _, testDef := range testDefs {
244-
content, err := extractArchiveFile(testDef.archiveType, "mybinary", testDef.data)
275+
content, err := extractArchiveFileWithLimit(
276+
testDef.archiveType,
277+
"mybinary",
278+
testDef.data,
279+
maxArchiveEntrySize,
280+
)
245281
if err != nil {
246282
t.Fatalf(
247283
"unexpected error for archive type %q: %s",
@@ -263,7 +299,13 @@ func TestExtractArchiveFileDispatch(t *testing.T) {
263299
// TestExtractArchiveFileUnsupportedType checks that extraction fails clearly
264300
// when an unsupported archive format is requested.
265301
func TestExtractArchiveFileUnsupportedType(t *testing.T) {
266-
if _, err := extractArchiveFile("rar", "mybinary", nil); err == nil {
302+
_, err := extractArchiveFileWithLimit(
303+
"rar",
304+
"mybinary",
305+
nil,
306+
maxArchiveEntrySize,
307+
)
308+
if err == nil {
267309
t.Fatal("expected error for unsupported archive type, got nil")
268310
}
269311
}

0 commit comments

Comments
 (0)