[ASCII-1615] Replace `github.com/mholt/archiver/v3` third party library with custom package `pkg/util/archive` #25498

GustavoCaso · 2024-05-10T08:37:02Z

What does this PR do?

Remove the third part library github.com/mholt/archiver/v3 from the flare package

Motivation

The library includes the vulnerability CVE-2024-0406. Technically speaking we are not affected by this in production as we do not uncompress tar files. Still, it is a good opportunity to evaluate if we need a third party library for zip opertaions.

We decided to remove the third party dependency by vendoring some of its code.

That has the benefit that we are no longer affected by the CVE-2024-0406 and that we not longer depend on a third party dependency. Which removes a few packages from the main agent binary 🪄

Additional Notes

Possible Drawbacks / Trade-offs

Describe how to test/QA your changes

Create a flare
Get the flare ZIP file
Uncompress the ZIP
Validate the file structure is correct and all the files are present

…package pkg/util/archive The libray includes the vulnerability CVE-2024-0406. Technically speaking we are not affected by this in production as we do not uncompress tar files. Still, it is a good opportunity to evaluate if we need a third party library for zip opertaions. We decided to remove the third party dependency by vendoring some of its code. That has the benefit that we are no longer affected by the CVE-2024-0406 and that we not longer depend on a third party dependency.

cit-pr-commenter · 2024-05-10T08:48:21Z

Go Package Import Differences

Baseline: 4faaee8
Comparison: 14c6666

binary	os	arch	change
agent	linux	amd64	+1, -0 +github.com/DataDog/datadog-agent/pkg/util/archive
agent	linux	arm64	+1, -0 +github.com/DataDog/datadog-agent/pkg/util/archive
agent	windows	amd64	+1, -24 +github.com/DataDog/datadog-agent/pkg/util/archive -github.com/andybalholm/brotli -github.com/dsnet/compress -github.com/dsnet/compress/bzip2 -github.com/dsnet/compress/bzip2/internal/sais -github.com/dsnet/compress/internal -github.com/dsnet/compress/internal/errors -github.com/dsnet/compress/internal/prefix -github.com/klauspost/compress/flate -github.com/klauspost/compress/gzip -github.com/klauspost/compress/zip -github.com/klauspost/pgzip -github.com/mholt/archiver/v3 -github.com/nwaples/rardecode -github.com/pierrec/lz4/v4 -github.com/pierrec/lz4/v4/internal/lz4block -github.com/pierrec/lz4/v4/internal/lz4errors -github.com/pierrec/lz4/v4/internal/lz4stream -github.com/pierrec/lz4/v4/internal/xxh32 -github.com/ulikunitz/xz -github.com/ulikunitz/xz/internal/hash -github.com/ulikunitz/xz/internal/xlog -github.com/ulikunitz/xz/lzma -github.com/xi2/xz -hash/crc64
agent	windows	386	+1, -24 +github.com/DataDog/datadog-agent/pkg/util/archive -github.com/andybalholm/brotli -github.com/dsnet/compress -github.com/dsnet/compress/bzip2 -github.com/dsnet/compress/bzip2/internal/sais -github.com/dsnet/compress/internal -github.com/dsnet/compress/internal/errors -github.com/dsnet/compress/internal/prefix -github.com/klauspost/compress/flate -github.com/klauspost/compress/gzip -github.com/klauspost/compress/zip -github.com/klauspost/pgzip -github.com/mholt/archiver/v3 -github.com/nwaples/rardecode -github.com/pierrec/lz4/v4 -github.com/pierrec/lz4/v4/internal/lz4block -github.com/pierrec/lz4/v4/internal/lz4errors -github.com/pierrec/lz4/v4/internal/lz4stream -github.com/pierrec/lz4/v4/internal/xxh32 -github.com/ulikunitz/xz -github.com/ulikunitz/xz/internal/hash -github.com/ulikunitz/xz/internal/xlog -github.com/ulikunitz/xz/lzma -github.com/xi2/xz -hash/crc64
agent	darwin	amd64	+1, -25 -archive/tar +github.com/DataDog/datadog-agent/pkg/util/archive -github.com/andybalholm/brotli -github.com/dsnet/compress -github.com/dsnet/compress/bzip2 -github.com/dsnet/compress/bzip2/internal/sais -github.com/dsnet/compress/internal -github.com/dsnet/compress/internal/errors -github.com/dsnet/compress/internal/prefix -github.com/klauspost/compress/flate -github.com/klauspost/compress/gzip -github.com/klauspost/compress/zip -github.com/klauspost/pgzip -github.com/mholt/archiver/v3 -github.com/nwaples/rardecode -github.com/pierrec/lz4/v4 -github.com/pierrec/lz4/v4/internal/lz4block -github.com/pierrec/lz4/v4/internal/lz4errors -github.com/pierrec/lz4/v4/internal/lz4stream -github.com/pierrec/lz4/v4/internal/xxh32 -github.com/ulikunitz/xz -github.com/ulikunitz/xz/internal/hash -github.com/ulikunitz/xz/internal/xlog -github.com/ulikunitz/xz/lzma -github.com/xi2/xz -hash/crc64
agent	darwin	arm64	+1, -25 -archive/tar +github.com/DataDog/datadog-agent/pkg/util/archive -github.com/andybalholm/brotli -github.com/dsnet/compress -github.com/dsnet/compress/bzip2 -github.com/dsnet/compress/bzip2/internal/sais -github.com/dsnet/compress/internal -github.com/dsnet/compress/internal/errors -github.com/dsnet/compress/internal/prefix -github.com/klauspost/compress/flate -github.com/klauspost/compress/gzip -github.com/klauspost/compress/zip -github.com/klauspost/pgzip -github.com/mholt/archiver/v3 -github.com/nwaples/rardecode -github.com/pierrec/lz4/v4 -github.com/pierrec/lz4/v4/internal/lz4block -github.com/pierrec/lz4/v4/internal/lz4errors -github.com/pierrec/lz4/v4/internal/lz4stream -github.com/pierrec/lz4/v4/internal/xxh32 -github.com/ulikunitz/xz -github.com/ulikunitz/xz/internal/hash -github.com/ulikunitz/xz/internal/xlog -github.com/ulikunitz/xz/lzma -github.com/xi2/xz -hash/crc64
iot-agent	linux	amd64	+1, -0 +github.com/DataDog/datadog-agent/pkg/util/archive
iot-agent	linux	arm64	+1, -0 +github.com/DataDog/datadog-agent/pkg/util/archive
heroku-agent	linux	amd64	+1, -0 +github.com/DataDog/datadog-agent/pkg/util/archive
cluster-agent	linux	amd64	+2, -0 +github.com/DataDog/datadog-agent/pkg/util/archive +github.com/cyphar/filepath-securejoin
cluster-agent	linux	arm64	+2, -0 +github.com/DataDog/datadog-agent/pkg/util/archive +github.com/cyphar/filepath-securejoin
cluster-agent-cloudfoundry	linux	amd64	+2, -0 +github.com/DataDog/datadog-agent/pkg/util/archive +github.com/cyphar/filepath-securejoin
cluster-agent-cloudfoundry	linux	arm64	+2, -0 +github.com/DataDog/datadog-agent/pkg/util/archive +github.com/cyphar/filepath-securejoin
security-agent	linux	amd64	+2, -0 +github.com/DataDog/datadog-agent/pkg/util/archive +github.com/cyphar/filepath-securejoin
security-agent	linux	arm64	+2, -0 +github.com/DataDog/datadog-agent/pkg/util/archive +github.com/cyphar/filepath-securejoin

pr-commenter · 2024-05-10T09:12:10Z

Test changes on VM

Use this command from test-infra-definitions to manually test this PR changes on a VM:

inv create-vm --pipeline-id=34338108 --os-family=ubuntu

pr-commenter · 2024-05-10T09:44:40Z

Regression Detector

Regression Detector Results

Run ID: f16f125c-1af9-43b8-bf26-7d1982d41983
Baseline: 1f10c8a
Comparison: 9f4f978

Performance changes are noted in the perf column of each table:

✅ = significantly better comparison variant performance
❌ = significantly worse comparison variant performance
➖ = no significant change in performance

No significant changes in experiment optimization goals

Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%

There were no significant changes in experiment optimization goals at this confidence level and effect size tolerance.

Experiments ignored for regressions

Regressions in experiments with settings containing erratic: true are ignored.

perf	experiment	goal	Δ mean %	Δ mean % CI
✅	file_to_blackhole	% cpu utilization	-43.87	[-48.81, -38.92]

Fine details of change detection per experiment

perf	experiment	goal	Δ mean %	Δ mean % CI
➖	basic_py_check	% cpu utilization	+1.61	[-0.91, +4.13]
➖	pycheck_1000_100byte_tags	% cpu utilization	+1.14	[-3.63, +5.91]
➖	idle	memory utilization	+0.10	[+0.07, +0.13]
➖	uds_dogstatsd_to_api	ingress throughput	+0.00	[-0.20, +0.20]
➖	trace_agent_msgpack	ingress throughput	-0.00	[-0.00, +0.00]
➖	trace_agent_json	ingress throughput	-0.01	[-0.02, +0.01]
➖	file_tree	memory utilization	-0.01	[-0.10, +0.08]
➖	tcp_dd_logs_filter_exclude	ingress throughput	-0.01	[-0.05, +0.02]
➖	otel_to_otel_logs	ingress throughput	-0.15	[-0.53, +0.23]
➖	uds_dogstatsd_to_api_cpu	% cpu utilization	-0.56	[-3.39, +2.27]
➖	tcp_syslog_to_blackhole	ingress throughput	-5.98	[-26.28, +14.31]
✅	file_to_blackhole	% cpu utilization	-43.87	[-48.81, -38.92]

Explanation

A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".

For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:

Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
Its configuration does not mark it "erratic".

pgimalac

LGTM, can we add (a lot of) tests though ?

pkg/util/archive/zip.go

pgimalac

Would be nice to have more tests around error paths, but this is good enough to merge

pgimalac · 2024-05-10T15:43:05Z

pkg/util/archive/zip_test.go

+	assert.Error(t, err, "must have a .zip extension")
+}
+
+func TestZip_ExtistingDestuination(t *testing.T) {


Suggested change

func TestZip_ExtistingDestuination(t *testing.T) {

func TestZip_ExistingDestination(t *testing.T) {

sgnn7

@GustavoCaso Great work! Left a lot of comments but majority of them are naming non-blocking nits.

pkg/util/archive/zip.go

sgnn7 · 2024-05-10T18:44:10Z

pkg/util/archive/zip.go

+
+	zipW := zip.NewWriter(out)
+	zipW.RegisterCompressor(zip.Deflate, func(out io.Writer) (io.WriteCloser, error) {
+		return flate.NewWriter(out, flate.DefaultCompression)


Is there any downside to using BestCompression?

The previous code was using DefaultCompression that is why I decided to leave as it was. https://github.com/mholt/archiver/blob/e0c66cf15ec46b2e04e0c280b0b8178d14aa55dc/zip.go#L651

I'm ok changing it to BestCompression.

Fair. Feel free to choose if you want to change it or not

sgnn7 · 2024-05-10T19:06:58Z

pkg/util/archive/zip.go

+
+	zipW := zip.NewWriter(out)
+	zipW.RegisterCompressor(zip.Deflate, func(out io.Writer) (io.WriteCloser, error) {
+		return flate.NewWriter(out, flate.DefaultCompression)


Big question here - were the flares compressed using custom/optimized Deflate through a dependency? If so, we may be regressing in speed and file size in the output so it would be good to do at least a side-by-side size comparison. The changeover is likely acceptable but we should at least understand the impact.

That is a great point.

There are two side to that question:

Do we want to reduce the number of imports?
Yes. In that case use the archive/zip package from the std
No. In that case use github.com/klauspost/compress/zip

Are performance degradation an issue for creating flares?
Yes. Use github.com/klauspost/compress/zip
No. Use archive/zip package from the std

in any case having a comparison table would be useful to answer the questions above. I will work on a comparison table 😄

having a comparison table would be useful to answer the questions above
I think just a simple flare from both implementations should be fine and we can just check the file size.

For your questions:

Do we want to reduce the number of imports? Yes

Are performance degradation an issue for creating flares? No, as long as they're not drastic

main_branch -> main custom_branch|custom_version -> this branch
Both archive are the same size. Uncompressed custom branch is 0.1 MB smaller.

I think we good with the change 😄

sgnn7 · 2024-05-10T19:38:30Z

pkg/util/archive/zip_test.go

+
+func TestZip_DoNotZipSymlinks(t *testing.T) {
+	tmpDestinationDir := t.TempDir()
+	zipTempLocation := filepath.Join(tmpDestinationDir, "destination.zip")


Not a blocker I feel like a lot of these tests have some common code that could be deduped

pkg/util/archive/zip_test.go

sgnn7 · 2024-05-10T19:44:11Z

pkg/util/archive/zip_test.go

+		{"bad/file.txt", "Mwa-ha-ha"},
+	}
+
+	if createFileOutsideRoot {


We should also try at least minimally testing absolute paths (e.g. /usr/bin/badfile.sh)

GustavoCaso · 2024-05-15T08:44:47Z

/merge

dd-devflow · 2024-05-15T08:44:54Z

🚂 MergeQueue

This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals.
Note: if you pushed new commits since the last approval, you may need additional approval.
You can remove it from the waiting list with /remove command.

Use /merge -c to cancel this operation!

dd-devflow · 2024-05-15T09:28:29Z

🚂 MergeQueue

Pull request added to the queue.

There are 2 builds ahead! (estimated merge in less than 1h)

Use /merge -c to cancel this operation!

GustavoCaso requested a review from a team as a code owner May 10, 2024 08:37

GustavoCaso added changelog/no-changelog team/agent-shared-components labels May 10, 2024

GustavoCaso added this to the 7.55.0 milestone May 10, 2024

GustavoCaso force-pushed the replace-archive-package-to-create-zip-file branch from c714578 to 23014bf Compare May 10, 2024 08:40

GustavoCaso changed the title ~~Replace github.com/mholt/archiver/v3 third party library with custom package pkg/util/archive~~ Replace github.com/mholt/archiver/v3 third party library with custom package pkg/util/archive May 10, 2024

pgimalac reviewed May 10, 2024

View reviewed changes

pkg/util/archive/zip.go Outdated Show resolved Hide resolved

pkg/util/archive/zip.go Outdated Show resolved Hide resolved

GustavoCaso added 2 commits May 10, 2024 16:08

Add tests for archive.Zip and archive.UnZip

04bdce8

Add copyright lines

b0fd718

GustavoCaso force-pushed the replace-archive-package-to-create-zip-file branch from dce7de9 to b0fd718 Compare May 10, 2024 14:08

pgimalac approved these changes May 10, 2024

View reviewed changes

sgnn7 reviewed May 10, 2024

View reviewed changes

GustavoCaso added 2 commits May 13, 2024 12:49

PR feedback

f92c34e

ensure file is closed when the test finish

9f4f978

GustavoCaso force-pushed the replace-archive-package-to-create-zip-file branch from fa1547c to 9f4f978 Compare May 13, 2024 12:57

GustavoCaso changed the title ~~Replace github.com/mholt/archiver/v3 third party library with custom package pkg/util/archive~~ [ASCII-1615] Replace github.com/mholt/archiver/v3 third party library with custom package pkg/util/archive May 13, 2024

brycekahle mentioned this pull request May 13, 2024

use internal .tar.xz extract #25549

Merged

GustavoCaso added 2 commits May 14, 2024 17:11

Ensure we write the Zip header file for folders

d5e8e21

use SecureJoin to extract file from archive

14c6666

dd-mergequeue bot merged commit 0ae8904 into main May 15, 2024
191 of 196 checks passed

dd-mergequeue bot deleted the replace-archive-package-to-create-zip-file branch May 15, 2024 09:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ASCII-1615] Replace `github.com/mholt/archiver/v3` third party library with custom package `pkg/util/archive` #25498

[ASCII-1615] Replace `github.com/mholt/archiver/v3` third party library with custom package `pkg/util/archive` #25498

GustavoCaso commented May 10, 2024 •

edited

cit-pr-commenter bot commented May 10, 2024 •

edited

pr-commenter bot commented May 10, 2024 •

edited

pr-commenter bot commented May 10, 2024 •

edited

Experiments ignored for regressions

Fine details of change detection per experiment

Explanation

pgimalac left a comment

pgimalac left a comment

pgimalac May 10, 2024

sgnn7 left a comment

sgnn7 May 10, 2024

GustavoCaso May 13, 2024

sgnn7 May 14, 2024

sgnn7 May 10, 2024

GustavoCaso May 13, 2024 •

edited

sgnn7 May 14, 2024

GustavoCaso May 15, 2024

sgnn7 May 10, 2024

sgnn7 May 10, 2024

GustavoCaso commented May 15, 2024

dd-devflow bot commented May 15, 2024

dd-devflow bot commented May 15, 2024

	func TestZip_ExtistingDestuination(t *testing.T) {
	func TestZip_ExistingDestination(t *testing.T) {

[ASCII-1615] Replace github.com/mholt/archiver/v3 third party library with custom package pkg/util/archive #25498

[ASCII-1615] Replace github.com/mholt/archiver/v3 third party library with custom package pkg/util/archive #25498

Conversation

GustavoCaso commented May 10, 2024 • edited

What does this PR do?

Motivation

Additional Notes

Possible Drawbacks / Trade-offs

Describe how to test/QA your changes

cit-pr-commenter bot commented May 10, 2024 • edited

Go Package Import Differences

pr-commenter bot commented May 10, 2024 • edited

Test changes on VM

pr-commenter bot commented May 10, 2024 • edited

Regression Detector

Regression Detector Results

No significant changes in experiment optimization goals

Experiments ignored for regressions

Fine details of change detection per experiment

Explanation

pgimalac left a comment

Choose a reason for hiding this comment

pgimalac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgnn7 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

GustavoCaso May 13, 2024 • edited

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

GustavoCaso commented May 15, 2024

dd-devflow bot commented May 15, 2024

dd-devflow bot commented May 15, 2024

[ASCII-1615] Replace `github.com/mholt/archiver/v3` third party library with custom package `pkg/util/archive` #25498

[ASCII-1615] Replace `github.com/mholt/archiver/v3` third party library with custom package `pkg/util/archive` #25498

GustavoCaso commented May 10, 2024 •

edited

cit-pr-commenter bot commented May 10, 2024 •

edited

pr-commenter bot commented May 10, 2024 •

edited

pr-commenter bot commented May 10, 2024 •

edited

GustavoCaso May 13, 2024 •

edited