override consul-template blacklist configuration

[tgross]

By default consul-template allows any template rendering function it supports to be used. This PR drops the plugin and file functions by default for Nomad templates. Includes:

• Agent-level configuration option for enabling rendering w/ these functions in case a cluster operator wants to permit them.
• Pulls in Knob to blacklist some template functions consul-template#1243 and allow return of TemplateConfigMapping to be comparable consul-template#1246

[tgross]

(I seemed to have screwed something up in the vendoring of the updated consul-template here so it's failing CI. Let me fix that 😊 )

[cgbaker]

this line and the change to the documentation suggest that this is supposed to be in the client config, but the change to the parsing show it in the top-level config. as such, right now it's not possible to change this to a non-default value of true.

[tgross]

Ah yes, thanks for catching that.

[cgbaker]

this needs to move below into ClientConfig

[cgbaker]

verified that this is working in the protective sense... the error message returned by consul-template in the allocation status is somewhat unpleasant, although it gets the point across:

2019-08-05T21:01:46Z Killing Template failed: (dynamic): execute: template: :1:3: executing "" at <file "/tmp/foo">: error calling file: function is disabled

config parsing means that this protection cannot currently be disabled. otherwise, looks good to me.

[jippi]

Can we make this a feature flag? Especially for file, there is plenty of use-cases in the world where a template is kinda like this

template {
    destination = "pki.json"
    data = <<<EOF
{{ with secret "pki/issue/my-domain-dot-com" "common_name=foo.example.com" }}
{{ .Data | toJSON }}{{ end }}
EOF;
}

template {
    destination = "certificate.tls"
    data = <<<EOF
{{ file "pki.json" | parseJSON | .certificate }}
EOF;
}

template {
    destination = "stuff.tls"
    data = <<<EOF
{{ file "pki.json" | parseJSON | .other_field }}
EOF;
}

I wonder if a better long term fix would be to provide a list of folders where file would be allowed to read from, and default to /local - since it's very all-or-nothing breaking a safe-common pattern without being able to be protected from the root cause that made you implement this whitelisting of functions

For example, I would like to disable plugin but not file - current implementation do not allow for that

[jippi]

would like to be able to disable plugin but keep file around - maybe expose a []string for the functions I want to blacklist manually as well?

[cgbaker]

i still can't reproduce the previous behavior with enable_insecure_template_functions = true, it looks like there is some conversion needed in:

nomad/command/agent/agent.go

Line 424 in fc55f65

func convertClientConfig(agentConfig *Config) (*clientconfig.Config, error) {

[tgross]

@cgbaker I'm still working on this but in the meantime I believe I've fixed the configuration problem at this point. If I build the current PR as make dev and use the following configuration file:

bind_addr = "0.0.0.0"
data_dir = "/tmp/nomad"
log_level = "DEBUG"

server {
  enabled = true
  bootstrap_expect = 1
}

client {
  enabled = true
  enable_insecure_template_functions = true
}

I can confirm the correct behavior when I toggle our boolean value.

[cgbaker]

LGTM!

[tgross]

Can we make this a feature flag?

Hello @jippi, thanks for your feedback on this! When I worked on this I had the same concern you do here that some operators would want to be able to be more selective about what functions they disallow. There’s a bit of a trade-off to make between configuration complexity and secure defaults. That being said, after seeing your example and having some other discussions I think I'm leaning towards having a more fine-grained control.

I wonder if a better long term fix would be to provide a list of folders where file would be allowed to read from, and default to /local - since it’s very all-or-nothing breaking a safe-common pattern without being able to be protected from the root cause that made you implement this whitelisting of functions

I would definitely like to see this myself! If we could sandbox consul-template to the same context as the task we wouldn’t really need to worry about the file function at all. But existing running jobs will be broken if we change the behavior, so we'll need to consider how to do this. I'm chatting with one of the consul-template developers about how the best way forward on that.

[tgross]

Just pushed a set of commits leveraging the work done in this draft PR hashicorp/consul-template#1249 for discussion. Demo of what this looks like:

---

default.hcl

bind_addr = "0.0.0.0"
data_dir = "/tmp/nomad"
log_level = "DEBUG"

server {
  enabled = true
  bootstrap_expect = 1
}

client {
  enabled = true
  template {
    function_blacklist = ["plugin"]
    disable_sandbox = false
  }
}

file-leak.nomad

job "example" {
  datacenters = ["dc1"]

  type = "service"

  group "cache" {
    count = 1

    task "redis" {
      driver = "docker"
      config {
        image = "redis:3.2"
        port_map {
          db = 6379
        }
      }
      resources {
        cpu    = 500 # 500 MHz
        memory = 256 # 256MB
        network {
          mbits = 10
          port "db" {}
        }
      }


      template {
        data          = "{{ file \"/etc/passwd\" }}"
        destination   = "local/file.yml"
      }
    }
  }
}

output:

    2019/08/07 11:24:04.940971 [ERR] (runner) watcher reported error: file(/tmp/nomad/alloc/a6dc1786-2e98-3fcf-7029-a24a97ab0ec6/redis/etc/passwd): stat /tmp/nomad/alloc/a6dc1786-2e98-3fcf-7029-a24a97ab0ec6/redis/etc/passwd: no such file or directory

[tgross]

We're getting a lot of timeouts on Travis builds here. I've opened #6094 to help mitigate some of that.

[tgross]

@cgbaker @jippi I think I'm happy with where this has landed at this point. The configuration values and their defaults are:

client {
  template {
    function_blacklist = ["plugin"]
    disable_file_sandbox = false
  }
}

Although I know @jippi mentioned wanting to sandbox tasks to specific directories outside the task directory, we're going to punt on that for now and have a flag to disable the sandbox. We can return to this in later work and see if it makes sense to include, but we need the all-or-nothing flag for backwards compatibility. For now, this new configuration gives folks a bunch of different options depending on the level of protection they need:

• function_blacklist = ["file", "plugin"]: disable the file function entirely
• disable_file_sandbox = true: allow the file function and allow it access to arbitrary files on the client
• disable_file_sandbox = false + docker { volume ...: use a docker bind mount to include specific files from the client into the task directory sandbox where the file function can read them.
• disable_file_sandbox = false + docker volumes enabled=false: disallow docker bind mounts and sandbox the file function to the task directory.

What do we think?

[tgross]

Rebased this on master to pick up #6095 and hopefully bring test run times down.

[schmichael]

"leak information" is too weak to describe plugin, so maybe we should expand with:

By default the plugin function is disallowed as it allows running arbitrary commands on the host as root (unless Nomad is configured to run as a non-root user).

[tgross]

Done.

[tgross]

Looks like we can avoid breaking existing uses of NOMAD_TASK_DIR in the file parameter of templates if we use the patch to consul-template in hashicorp/consul-template#1254.

[notnoop]

lgtm - but would be good to remove burstsushi casing.

[tgross]

This work has been cherry-picked into the release-095 branch.

[github-actions]

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.