twosigma / Cook

Fair job scheduler on Kubernetes and Mesos for batch workloads and Spark
Apache License 2.0
338 stars 63 forks source link

Force docker container user #479

Open m4ce opened 7 years ago

m4ce commented 7 years ago

Hi,

I noticed that following logic in the build-container definition, line 416

    (if (= (clojure.string/lower-case ctype) "docker")
      (let [docker (:docker container)
            params (or (:parameters docker) [])
            port-mappings (or (:port-mapping docker) [])
            user-params (filter #(= (:key %) "user") params)
            expected-user-param (str (uid user) ":" (gid user))]
        (when (some #(not= expected-user-param (:value %)) user-params)
          (throw (ex-info "user parameter must match uid and gid of user submitting."
                          {:expected-user-param expected-user-param
                           :user-params-submitted user-params})))

The only problem with that is that if no user is specified in the docker parameters, the process in the container will run as root by default.

It might be good to force the user-params to be the uid and gid of the user submitting. If the UID and GID cannot be evaluated, error out?

Maybe we can enforce that through a boolean config option like 'force_docker_user'?

dposada commented 7 years ago

@m4ce Just so I understand, is the point that you want to avoid ever running as root inside the container?

m4ce commented 7 years ago

@dposada, yes. It's problematic in terms of security, especially when you bind volumes from the host into the container.. :/

DaoWen commented 7 years ago

I did some reading into this issue, and it looks like the standard solution for docker is to use user namespaces. If I understand the feature correctly, it allows the docker container to think that it's running as root, when really it's running as a less-privileged user on the host machine.

Here are a few of the references I was reading:

Based on that, I think this security issue might better addressed in the docker deployment by ensuring this uid-remapping feature is correctly configured.

@m4ce - What are your thoughts on this?

m4ce commented 7 years ago

@DaoWen - I already new about user namespaces. However, it is my understanding that it a one-to-one mapping. It only works if you always want to re-map for instance uid 0 to a specific uid on the host. However, this won't work if you want to map uid 0 to many uids, which is the case here since we have multi-tenancy.

DaoWen commented 7 years ago

OK, that makes sense. It looks like you can only specify a single remapping, and it must be set statically when the docker daemon is launched. That's probably not ideal for your use case.

I'd prefer not having this user-check enforced by default. See my latest comment on #480 for more details.