[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ferhatelmas
To complete the pull request process, please assign ahmetb
You can assign the PR to them by writing /assign @ahmetb in a comment when ready.

The full list of commands accepted by this bot can be found here.

Codecov Report

Merging #382 into master will decrease coverage by 0.54%.
The diff coverage is 63.41%.

@@            Coverage Diff             @@
##           master     #382      +/-   ##
==========================================
- Coverage   57.11%   56.56%   -0.55%     
==========================================
  Files          19       19              
  Lines         914      861      -53     
==========================================
- Hits          522      487      -35     
+ Misses        339      330       -9     
+ Partials       53       44       -9

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b9a0e88...c95f3c8. Read the comment docs.

@ferhatelmas Can you link the PR you are talking about?

sure, mholt/archiver#169

Hmm.. strange. The repo seems pretty active but this CVE fix is from May 12. What's wrong there?

I guess the owner was busy with caddy 2.0 release and there is no other maintainer.

Can we please preserve signatures of something like extractZIP and extractTARGZ and/or write interface wrappers around the mholt/archiver package please?

I know this is not super necessary but for example see how we have wrappers around similar packages such as semver package and yaml packages.

This is a pretty critical dependency so I'd love to have an option to flip between implementations if it comes to that.

I'm a bit worried that the maintainer has not managed to merge a proposed fix for a CVE in half a year.

Besides, archiver just unpacks and creates symlinks:
https://github.com/mholt/archiver/blob/6041b49af61de1075782610aaee8c6c53ba73c45/zip.go#L207-L215
and
https://github.com/mholt/archiver/blob/6041b49af61de1075782610aaee8c6c53ba73c45/tar.go#L232-L245

Thus, it's a piece of cake to escape from the target directory and write to arbitrary paths (you don't even have to chain symlinks).

To me it clearly looks like archiver was built with ease in mind, but not security. It's also not possible to configure its security level, so I'm afraid we can't use archiver.

@erain What is your opinion about the symlink handling in archiver?

@corneliusweig You are talking about the security implication in #359 right?

I am not the expert on the topic. Some research showed that archiver has symlink support built-in: mholt/archiver#92

I am not sure about whether the symlink handling is ideal (e.g. only relative symlink and not go beyond the base dir) yet. But we can do more research here.

I vote -1 for using archiver. It’s not in a good state. We seem to be caring about security more. And we have open PRs like #360 which will rely on our ability to correctly and securely implement symlinks.

Good to know. Sorry for missing the context on this.

@erain no worries at all. There is a history about escaping the unpack directory, so that is something to keep an eye out for.

/close
The current state of archiver does not meet our security requirements. Therefore we cannot integrate it into our code-base.

@corneliusweig: Closed this PR.

@ferhatelmas Thanks though for investigating and integrating into krew. Maybe there is another better alternative?

For now, I recommend we consider creating an interface, and extracting unarchive functionality to its new pkg that we can more easily test/maintain.

We can even make tar/zip methods smaller as there are a lot of commonalities during extraction (mkdir, write file, symlink...).

Let's just start with simply moving the code, and keep PRs small/reviewable.

Thanks though for investigating and integrating into krew.

No worries.

Let's just start with simply moving the code, and keep PRs small/reviewable

I will try this. Then, I will investigate another option to offload implementation.

By the way, #360 should go in first, right?