Restrict loading to root or below. #700
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: monopole The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
monopole
requested review from
Liujingfang1
and removed request for
droot and
mengqiy
| func (l *fileLoader) Load(path string) ([]byte, error) { | ||
| if filepath.IsAbs(path) { | ||
| return nil, fmt.Errorf( | ||
| "must use relative path; '%s' is absolute", path) | ||
| return nil, l.loadOutOfBounds(path) |
There was a problem hiding this comment.
The previous error message looks better. We can leave it unchanged.
There was a problem hiding this comment.
Message updated.
This is now a change from
must use relative path; '/etc/tls.key' is absolute.
to
"security; file `/etc/tls.key` is not in or below `/the/kustomization/root`,
How's that?
| if l.Root() == string(filepath.Separator) { | ||
| return true | ||
| } | ||
| return strings.HasPrefix( |
There was a problem hiding this comment.
this needs a warning so that people don't strip off the trailing slash (which would make it more incorrect that in currently is). This is technically still wrong (https://codereview.appspot.com/5712045, golang/dep#296, golang/go#18358 (comment)) due to filesystem case sensitivity, etc. The fully correct answer appears to be https://github.com/mem/dep/blob/c409fbd7e3dc2df0422061f4a5a28fd6555a78ba/internal/fs.go#L44, but it's tricky. If you decide to stick with this, note the caveats (since this is even less correct than filepath.HasPrefix on Windows).
There was a problem hiding this comment.
Thanks much for those links.
I've updated this comment and the overall class doc.
monopole
force-pushed
the
restrictLoading
branch
from
03fbc2b to
905125d
Compare
monopole
force-pushed
the
restrictLoading
branch
from
905125d to
14af70d
Compare
|
new comments look good |
k8s-ci-robot
added
the
lgtm
* Provide first controller implemenation * Finialize slack reporter implementation * Fixing deploy resources * Fix rbac * Add test coverage for controller * Change the golang builder image version * Change the way who mock are generated * kustomization.yaml was moved outside the /default folder, see kubernetes-sigs/kustomize#700 * Change Gopkg.lock to work with dep 0.5.x * Add fetching commit author * Add more info in README.md * Apply changes after Adam review * Add milv ignore
Only `bases` can be accessed in higher directories. kubernetes-sigs#700 kubernetes-sigs#766
Kustomization files refer to two kinds of files outside the kustomization file itself:
bases (other kustomizations)
These can be git URLs, or paths specified relative to the root (as long as they don't go strictly "up").
data paths for the current kustomization
Things like resources, patches, data for configmaps, etc.
These must be paths specified relative to the root
where root is the directory containing the kustomization file specifying these things.
This PR imposes an additional requirement on data paths.
They must still be root relative paths, but additionally they cannot go up out of root, either explicitly, or by following symbolic links. They can only refer to files in or below root.
This will require an increment in semver major, since it disallows certain file loading patterns.
No existing examples or documentation need to change, since this file arrangement wasn't used.
Example of disallowed behavior and how to fix:
Supposed a single data file
foo/common/data.txtis referenced in two similar configmap generator rules in two different directoriesfoo/overlay1andfoo/overlay2. This is now disallowed, because the data is outside the two respective kustomization roots.The fix is to simply add a kustomization file to the directory containing the data, and refer to that as a base from the kustomizations in the two overlays (i.e. refer to the base, not the data). It remains OK to go up and over when referring to a base (though not directly up, since that can result in cycles).
A followup PR should include an example discussing/defining this.
Fixes #693