Integrate archiver to simplify download #382
Conversation
k8s-ci-robot
added
do-not-merge/invalid-commit-message
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ferhatelmas The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ferhatelmas
force-pushed
the
archiver
branch
from
419cb26
to
f95ba87
Compare
Right now, archiver doesn't handle security issue but there is an existing PR to handle it. Added walker to handle until upstream is fixed. When it's updated with go modules, walker can be dropped.
ferhatelmas
force-pushed
the
archiver
branch
from
f95ba87
to
c95f3c8
Compare
k8s-ci-robot
removed
the
do-not-merge/invalid-commit-message
Codecov Report
@@ 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.
|
|
@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. |
| case zip.FileHeader: | ||
| return suspiciousPath(h.Name) | ||
| default: | ||
| return errors.Errorf("Unknow header type: %T", h) |
There was a problem hiding this comment.
Also: Error strings should not be capitalized
Ref: https://github.com/golang/go/wiki/CodeReviewComments#error-strings
|
Can we please preserve signatures of something like I know this is not super necessary but for example see how we have wrappers around similar packages such as 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, 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 |
| case zip.FileHeader: | ||
| return suspiciousPath(h.Name) | ||
| default: | ||
| return errors.Errorf("Unknow header type: %T", h) |
There was a problem hiding this comment.
Also: Error strings should not be capitalized
Ref: https://github.com/golang/go/wiki/CodeReviewComments#error-strings
|
@erain What is your opinion about the symlink handling in |
|
@corneliusweig You are talking about the security implication in #359 right? I am not the expert on the topic. Some research showed that 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 |
|
@corneliusweig: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@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. |
No worries.
I will try this. Then, I will investigate another option to offload implementation. By the way, #360 should go in first, right? |
Right now, archiver doesn't handle security issue
but there is an existing PR to handle it.
Added walker to handle until upstream is fixed.
fixes #371