Deploy id character blacklist needs expanding

[nyarly]

Experimenting with Deploy ids and redeploys (specifically, I was looking for a delimiter to use in machine-generated deploy ids), I found that a Deploy id that Singulary's API accepted still crashed the Executor:

Failed to launch container: Failed to create container: Failed to chown: Failed to execute 'chown -R 0:0 '/mnt/mesos-slave/slaves/f38ff857-4831-46dc-b143-d8aaa0c44d06-S0/frameworks/Singularity/executors/github.com>opentable>sous-demo::ci-sf-<>|=()'_+{}"^&df1d7456ac654e4097ae451b0dc6c0fa-1485370286019-1-[REDACTED]-FIXME/runs/9967afcc-0feb-4421-baa7-094d6d8896fb'' (exit status: 512): Success; Abnormal executor termination

I'm concerned further that one explanation for this problem is that the ' ends the chown command, and the shell treats the rest as garbage. (I can only guess here: it looks like exec(commandString) rather than exec([array, of, command, parts])). If that's the case, there's the possibility of an RCE against Mesos agents for anyone with access to Singularity. It's tricky to test for this, because the restriction limits us to single word commands... like halt.

[ssalinas]

Hey @nyarly can you provide a few more bits of information here:

• Are you using the SingularityExecutor or the default mesos one?
• In your deploy json, what was the command you set, and what did you have the shell property set to (if anything)?

[nyarly]

The container type is "DOCKER", and we use the Mesos Docker executor.

There's no "shell" set in the deploy.

[ssalinas]

ok, in your deploy did you have just a command set or did you also have args? This is what Singularity uses to decide if we should be sending mesos an array of command parts or if it should be more like exec(commandString)

[stevenschlansker]

(you might also just paste the whole json in one go)

[nyarly]

HYG:

{
  "deployResult": {
    "deployState": "SUCCEEDED",
    "deployFailures": [],
    "timestamp": 1485370424344
  },
  "deployMarker": {
    "requestId": "github.com>opentable>sous-demo::ci-sf",
    "deployId": "=()_+{}df1d7456ac654e4097ae451b0dc6c0fa",
    "timestamp": 1485370361173,
    "user": "asdf"
  },
  "deploy": {
    "requestId": "github.com>opentable>sous-demo::ci-sf",
    "id": "=()_+{}df1d7456ac654e4097ae451b0dc6c0fa",
    "containerInfo": {
      "type": "DOCKER",
      "volumes": [],
      "docker": {
        "image": "docker.otenv.com/sous-demo@sha256:c3b52e949485ee90108223d32317854495df7892c7e7776664be3bbd0e104f85",
        "privileged": false,
        "network": "BRIDGE",
        "portMappings": [],
        "forcePullImage": false,
        "dockerParameters": []
      }
    },
    "resources": {
      "cpus": 0.1,
      "memoryMb": 10,
      "numPorts": 1,
      "diskMb": 0
    },
    "env": {
      "OT_DISCO_INIT_URL": "discovery-ci-sf.otenv.com",
      "OT_ENV": "ci-sf"
    }
  }
}

[ssalinas]

Ok, cool. After some digging on docker, seems correct that the only allowed values for container names are: [a-zA-Z0-9][a-zA-Z0-9_.-]

Since mesos docker executor names the container after the task (which in our case contains the deploy ID), yes we need to limit those. Thanks for posting. Seems we should probably limit request and deploy IDs to that same character set that docker uses for safety