Remove local load restrictions.

[monopole]

Remove load restrictions when the command line target is local.

#700 was merged to mitigate the remote kustomization risk described in #693. Under #700, a kustomization file may not load local resources from outside the directory rooted at the kustomization root (the directory in which a kustomization file is located) unless it's a base (another kustomization root).

This was to mirror existing kubectl resource discovery, which remains limited to the directory tree below the -f argument specified on the kubectl apply -f $dir command line.

The risk is that a bad actor places an malicious kustomization in the repo
https://github.com/evilOrg/evilRepo and gets someone to enter

kustomize build https://github.com/evilOrg/evilRepo

or in the case of kubectl,

kubectl apply -k https://github.com/evilOrg/evilRepo

If the remote kustomization were able to read outside its root, it could, say, incorporate the contents of /etc/passwd into a ConfigMap and send it to the cluster.

This PR does not remove that restriction - all kustomizations loaded from remote sites remain jailed.

This PR does, however, remove the restriction when the build or apply -k argument is a local directory (not a URI requiring download / unpacking).

This is because people want to create different kustomizations that share patch files. A patch file describes a transformation applied to resources available to a kustomization. A kustomization cannot, under the current rules, load a patch from some outside directory. This PR relaxes that constraint as long as the kustomization wasn't directly loaded from a URI. The same relaxation applies to resource files, custom resource definitions, legacy transformation configuration files, etc.

The bad actor will now have to get his victim to first clone evilRepo locally, then run kustomize on it, applying the output to a cluster under the bad actor's control.

A person sharing patches or resources this way sacrifices the relocatability of their kustomization directory. To avoid this, one will soon be able to load tranformers and generators using a base like notation, making sharing possible without losing relocatabiity.

[k8s-ci-robot]

[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:

• OWNERS [monopole]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Liujingfang1]

The change looks good to me overall.

The bad actor will now have to get his victim to first clone evilRepo locally, then run kustomize on it, applying the output to a cluster under the bad actor's control.

Do you need to document this somewhere?

Can you also add a test in rarget directory which uses a patch from a file above the loader root?

[monopole]

good idea re the test. let me merge #996 first, rebase, then change it in this pr

[monopole]

The rule is: don't clone some repo you don't know, unpack it in precisely the right spot that a relative path to some valuable file happens to work, fail to inspect it, then run kustomize on it and apply it to a cluster that someone else controls so they can nab the data.

Because bad guys are super excited about this precise form of data theft on a kubernetes cluster operator of all things, since all other forms of attack are harder?

seems far fetched.

Should i add a flag so the behavior has to be turned on? Even for a local directory? It's already one for remote ones.

[monopole]

If we add a flag to allow this notion of outside-of-root (shared) patches or resources, it would look like

kustomize build --loadRestriction none $target

or

kubectl apply --loadRestriction none -k $traget

[Liujingfang1]

Since the existing flag is --enable_alpha_goplugins_accept_panic_risk, --load_restriction looks better.

We're adding a new flag again. Our previously policy was not to allow adding flags that can affect the output.
Can we change that policy to that a flag is only allowed when it is to address a security concern?

[monopole]

i'll add the flag.

It doesn't change the output for anything that already works, so we're OK. It will only allow things that don't presently work. It will require a minor version increment. But we already have to do that for prune.

[Liujingfang1]

/lgtm

[sascha-coenen]

This is a very important and very welcome correction to the 2.0 restrictions.

What I need to be able to do more than anything else is to refer to common configuration files like so:

configMapGenerator:
- name: common-config
  behavior: merge
  files:
  - file.config=../../common-configs/file.config

Will this pull request also allow me again to specify such relative filepaths again within configMapGenerators ?

The proposal from PR 700 for dealing with common configs quoted below does not work.

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

consider the following usecase:

base
   complex-application
      common
          config.properties
       component-a
          component-a.properties
       component-b
          component-b.properties
overlay
   complex-application
      common
          config.properties
       component-a
          component-a.properties
       component-b
          component-b.properties

Imagine a complex application that consists of several components like a database that has a master and worker apps or in my case a product named Apache Druid which breaks up into several applications like middlemanager, overlord, broker, historical, metastore etc.

All application components need some common configurations and in addition to that also have further individual and specific configs.

On first glance it seems possible to define a kustomization.yaml for the common configs and then to . refer to it, but when you actually try to do this, you will run into trouble.

base/common   <--------------  overlay/common
             ^-- base/app1                                   ^
                              ^------------- overlay/app1             

The overlay/app1 application component should logically pull in common properties from its own overlay and at the same time it should extend the base/app1 setup. So it would have to declare two bases, namely . the overlay/common and the base/app1
But by doing this the base/common setup gets pulled in twice, once via the route base/app1--overlay/app1 and then also via the route overlay/common--overlay/app1

Being able to refer up the directory tree within file references of a configMapGenerator solves this issue. So the new approach of prohibiting relative paths that reach outside of the root is really a deal-breaker for our whole configuration tree. We have an extremely large config tree with many complex setups and trying to arrange it to be kustomize 2.0 compliant is not working no matter how hard I try.

I sincerely hope that this PR reverts this behaviour and makes this also the default.
But I'm not sure whether I understand this PR. Can someone confirm whether relative paths in configMapGenerators will afterwards become possible again?

Also, is there any info on when this will be released and as which version of kustomize and kubectl ?
It would help me a lot to get some intel on this because I would like to migrate to kustomize 2 asap but the current restrictions are a dealbreaker for me. However, I also feel the need to migrate because of the changes in the secret management.

thanks

[sascha-coenen]

kustomize build --loadRestriction none $target

no no no, PLEASE make it the default to NOT have any restrictions.
It is very important that kustomize and kubectl stay simple to use. So far I never had to specify any flags. That was always the beauty of it. All could be done with kubectl apply -f

Now these latest motions with the restrictions and the flags are tearing it all down.
I'm very unhappy about these changes.

First off there are no intelligible explanations of why there is actually a security risk. I believe there is none. There are several implicit ways to deal with the problem too.

The kustomization.yaml that gets pointed to should not even be looked at as being the root. It is the entrypoint into a configuration tree that has its root elsewhere. Having relative paths in patches and even more so config maps is not only more than reasonable but also essential.
Relaxing the restrictions should be automatic for local cases and should not require an explicit flag.

Even for the remote case, there is no security issue. If a kubectl command points to a github path then all relative paths should also be relative to that github repo, not the local disk. So there would never be a case where someone could access something like /etc/...

You can also very easily restrict relative paths to a certain depth. Let people who have extreme feelings about security place their safeguards if they think they have to. Let them specify one, two or two dozent command line arguments to define whatever sandbox restrictions they want, but please don't make folks with normal usecases go through the bother of adding this stuff.

If you allow yourselves to do this once, you will do it again and again. kustomize is SO beautiful. lets keep it this way!