many: container validation improvements #13682
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #13682 +/- ##
==========================================
+ Coverage 78.87% 78.90% +0.02%
==========================================
Files 1039 1040 +1
Lines 133603 133966 +363
==========================================
+ Hits 105381 105704 +323
- Misses 21637 21665 +28
- Partials 6585 6597 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
LGTM! |
| func evalSymlink(c Container, path string) (symlinkInfo, error) { | ||
| var naiveTarget string | ||
|
|
||
| const maxDepth = 10 |
There was a problem hiding this comment.
This limit should be documented somewhere. It would be good if there was a matching kernel-side limit so that our limit is not surprising to people.
|
|
||
| const maxDepth = 10 | ||
| currentDepth := 0 | ||
| for currentDepth < maxDepth { |
There was a problem hiding this comment.
Maybe use the long form of for here?
| target = filepath.Join(filepath.Dir(path), target) | ||
|
|
||
| // target escapes container, cannot evaluate further, let's return | ||
| if strings.Split(target, string(os.PathSeparator))[0] == ".." { |
There was a problem hiding this comment.
This is somewhat costly for what is requested. Is this somehow different than strings.HasPrefix or am I missing something subtle?
| func shouldValidateSymlink(path string) bool { | ||
| // we only check meta directory for now | ||
| pathTokens := strings.Split(path, string(os.PathSeparator)) | ||
| if pathTokens[0] == "meta" { |
| } | ||
|
|
||
| func evalAndValidateSymlink(c Container, path string) (symlinkInfo, error) { | ||
| pathTokens := strings.Split(path, string(os.PathSeparator)) |
There was a problem hiding this comment.
Is this strings.HasPrefix again?
| return symlinkInfo{}, fmt.Errorf("external symlink found: %s -> %s", path, info.naiveTarget) | ||
| } | ||
|
|
||
| // symlinks like this don't look innocent |
There was a problem hiding this comment.
Can you clarify what is innocent here?
| if mode&os.ModeSymlink != 0 && shouldValidateSymlink(path) { | ||
| symlinkInfo, err := evalAndValidateSymlink(c, path) | ||
| if err != nil { | ||
| logf("%s", err) |
There was a problem hiding this comment.
I would add a context here, like "Cannot evaluate and validate symbolic link: %s"
ZeyadYasser
force-pushed
the
refactor-container-validation
branch
from
7be84b7 to
358ca95
Compare
Signed-off-by: Zeyad Gouda <zeyad.gouda@canonical.com>
ZeyadYasser
force-pushed
the
refactor-container-validation
branch
from
358ca95 to
fb95190
Compare
No description provided.