Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

@erictune @thockin

Assigning @erictune for now, but will try to find time to look at this.

@liggitt fyi

I'm trying to figure out what about this is Secret specific. You might want to set an env var from a file in any sort of volume. In particular, we've talked about a "Config" volume type that would be very similar to a secret volume.

@erictune You make a good point, I will think about how to generalize this;
perhaps we could re-use the same API w/in a ConfigVolumeSource?

On Thu, Mar 5, 2015 at 5:06 PM, Eric Tune notifications@github.com wrote:

I'm trying to figure out what about this is Secret specific. You might
want to set an env var from a file in any sort of volume. In particular,
we've talked about a "Config" volume type that would be very similar to a
secret volume.

—
Reply to this email directly or view it on GitHub
#4890 (comment)
.

Could you explore the idea of making this part of type EnvVar instead of part of the volume.
Not sure which way is better, but seems worth comparing before we commit to one or the other.

Maybe instead of specifying EnvVar.Value, the user could specify EnvVar.FromFileContents and kubelet would read that file after attaching volumes but before starting the container.

One difference that occurs to me is EnvVar is per container while this approach seems to apply to all containers in a pod.

@erictune I hadn't thought of that particular mechanism for env adaptation,
let me think about that.

On Fri, Mar 6, 2015 at 5:04 PM, Eric Tune notifications@github.com wrote:

Could you explore the idea of making this part of type EnvVar instead of
part of the volume.
Not sure which way is better, but seems worth comparing before we commit
to one or the other.

Maybe instead of specifying EnvVar.Value, the user could specify
EnvVar.FromFileContents and kubelet would read that file after attaching
volumes but before starting the container.

One difference that occurs to me is EnvVar is per container while this
approach seems to apply to all containers in a pod.

—
Reply to this email directly or view it on GitHub
#4890 (comment)
.

@erictune I was thinking about the approach you mentioned and while I like the API better I think there are a couple non-starters with modifying the api.EnvVar type to allow population from a file. Changing api.EnvVar in this manner seems to imply that the kubelet would read the information from the file and use it to populate the container's environment, which has a number of undesirable effects for secrets specifically:

1. Secrets would be exposed in the docker container's metadata and therefore available via docker inspect, captured by docker commit
2. The environment of the container process would contain secret data which would be readable from /proc/<pid>/environ

Other questions that came to mind:

1. I believe the environment would have to be set on container create; I don't think there is a way to create a container, do a copy out of it, then mutate the env, then start. I think you would need to determine the path on the host to the file instead of reading it from the container's fs. I might be wrong on this, let me know if you know differently.
2. Would you be able to use this feature to populate an env-var from an arbitrary file on the host, or just ones in volumes?

I agree that we don't want the secret env vars in the docker metadata, since that may hit disk.

I'm not sure I understand the threat for /proc/$PID/env.

• If the attacker is root, then they could get the secret other ways.
• If the attacker can escape a container, or is not contained, and can act as a different, non-root uid/gid as the victim, then won't default permissions on /proc/$PID/env protect the secret?
• If the attacker can escape a container and act as the same unix uid/gid as the victim, then the attacker could just read from tmpfs, right?

Also, assuming there is a threat via /proc/$PID/env, I think both approaches have that vulnerability. I'm not clear how you envision the user causes the env file to get into his environment; please say how. Here are some options:

1. command/entrypoint does source /path/to/envfile?
   • then won't the env vars appear in /proc/$PID/env of the processes in the container?
2. Program in container reads env file and then sets its env vars à la os.SetEnv(...)
   • then won't the secret appear in any child processes of that process?
3. Program in container reads env file, but does not set env vars à la os.SetEnv(...), instead parses out needed content.
   • why use bash syntax? why not read direct from the secret volume.

Remaining comments assume we don't consider /proc/$PID/env a problem, and that the command/entrypoint has to source /path/to/envfile.

Will it hinder reuse of community images if user has to add source /path/to/envfile to that container? Should we do that for them using a pre-start hook (#140)? Under that proposal, there would be a generic mechanism for kubernetes to create the container, run a command in the container, and then run the image-specified command.

@erictune I think you convinced me that /proc isn't a threat. What about capturing secret data in a docker commit, though?

@erictune I'm a lot less confident about mutating the command than I was initially. How do you know which shell the user prefers? What do you do for scratch images without a shell? etc.

@erictune I guess if you can opt-in with a pre-start hook, that potentially solves the issue of knowing how to mutate the command just right. What about these questions?

Other questions that came to mind:

1. I believe the environment would have to be set on container create; I don't think there is a way to create a container, do a copy out of it, then mutate the env, then start. I think you would need to determine the path on the host to the file instead of reading it from the container's fs. I might be wrong on this, let me know if you know differently.
2. Would you be able to use this feature to populate an env-var from an arbitrary file on the host, or just ones in volumes?

@vmarmol can you link to your pre-start-hook patch?

@pmorie
I assume that most containers won't be able to cause a docker commit to happen. Maybe just a build-service container? Does that match your expectation?

Reading this issue:
moby/moby#4000
it seems like it is possible to override some env vars when doing a docker commit with the --run={"Env": {"FOO": "bar"}} option. Its a bit gross, but the build server could replace its secret env var with an empty value. I haven't tried it, so it isn't clear whether you can erase an existing env var or just change its value.

@pmorie
The other issue is that, presumably, docker daemon may write things to disk even if we don't ask it to.
That seems harder to fix. Maybe it can be told to put its checkpoint on a tmpfs?

@erictune I don't have a patch, just a proof of concept :) I'm happy to develop that into a working solution. It should be relatively simple, the harder part will be the integration into Kubelet.

Proof of concept from previous post:

"If you override the entrypoint (docker run --entrypoint="") you can run a the pre-start and then have that exec the actual command. It requires us to inject the binary we want to run into the container. We can do it with a volume:

vmarmol@vmarmol-demo:~$ cat scripts/starter.sh
#!/bin/bash

# Do pre-start
echo "pre-start" >> /log

# Exec the actual command
exec $@
vmarmol@vmarmol-demo:~$ sudo docker run --entrypoint="/start/starter.sh" -v /home/vmarmol/scripts:/start:ro ubuntu cat /log
pre-start

A bit hacky, but it does work :) the part we may need to figure out is how to restore the actual entrypoint. I'm pretty sure we can get that from Docker though. If the binary we inject is a statically linked binary this should work just fine. If we're worried about leaving the binary in the container, it can even erase itself once it's done."

@erictune Point taken wrt commit.

The other issue is that, presumably, docker daemon may write things to disk even if we don't ask it to.
That seems harder to fix. Maybe it can be told to put its checkpoint on a tmpfs?

Agree, the sticky bit of having the info in the docker metadata is that the daemon may write it to disk somewhere. By 'checkpoint' did you mean the metadata store? I'm not especially hopeful about getting the daemon to use tmpfs; my gut feeling is that it's more viable to just avoid having these in the metadata.

@vmarmol I agree, the hard part of the approach you outlined above is integration into the kubelet. It sounds like a finalizer to me, with the added challenge of the binary. I'm assuming you meant for the binary to set the environment based on the content of a secret (please correct me if I'm wrong). I think that would result in good behaviors:

1. Container metadata wouldn't include the environment vars
2. /proc wouldn't contain them, if I understand correctly, but I need to try a test for myself

@vmarmol I think we will need to think through the matrix of all the different permutations of CMD and ENTRYPOINT. I also think the binary would have to be able to run the ultimate command, since the image may not have a shell (and thus you wouldn't be able to chain the binary and entrypoint with a semi-colon or &&). Just thinking a little, is the table approximately like so? (Ignoring the variations of array vs. string for now)

@smarterclayton any thoughts about the above re: pre-start hooks?

@pmorie yes this would mean the secrets won't be in proc or the ENV config. We may have to pass them to the binary or place them in disk/tmpfs somewhere for it to access though. The binary should also exec the final command as you mentioned. We may even be able to use this functionality to inject some debugging binaries in the user's container for our use later (but that's unrelated).

The table LGTM as well, in general:

append(entrypoint, cmd...)

and exec that.

@vmarmol I think we're on the same page about the generalities. As always, the devil's in the details.
All other pre-start hook use-cases aside, if we just had a binary as described above that handled reading the secrets and setting the env, what would the API look like? The secret data still has to be somewhere the hook can read in the container; since we already have tmpfs storage for secret volumes, I'm inclined to start sketching with that as a starting point.

Say that:

1. This hook binary reads information from an existing volume
2. We add a PreStart hook to the container's Lifecycle like so:

type Lifecycle struct {
  // other fields omitted
  PreStart *Handler
}

This doesn't get us all the way, though -- for example, where does the env setter binary come from?
I think there will actually need to be two binaries to do this case correctly:

1. Pre-start hook runner (let's call it container-init)
2. env-setter binary

The container-init binary has to run pre-start hooks and invoke the entrypoint. Going back to the definition of PreStart hooks, it seems likely to me that people will want to easily be able to specify multiple handlers.

This seems like it would generalize work with the planned 'config' resource as well.

Any thoughts @vmarmol @thockin @erictune @smarterclayton ?

@pmorie why do they have to be two separate binaries (it shouldn't be a problem to support more than one, I'm just curious why you think we need two). I would image that users would expect the pre-start hook to run with the ENV their container would run with so it should already have the relevant secrets no? The container-init can do the ENV setup on itself and let its children inherit that.

@vmarmol I expect there to be a couple other concerns that will need their
own binaries; I've been thinking of composing those but it's fair to
discuss all-in-one versus separate binaries.
On Wed, Mar 25, 2015 at 11:56 AM Victor Marmol notifications@github.com
wrote:

@pmorie https://github.com/pmorie why do they have to be two separate
binaries (it shouldn't be a problem to support more than one, I'm just
curious why you think we need two). I would image that users would expect
the pre-start hook to run with the ENV their container would run with so it
should already have the relevant secrets no? The container-init can do the
ENV setup on itself and let its children inherit that.

—
Reply to this email directly or view it on GitHub
#4890 (comment)
.

Injecting a binary requires injecting a whole volume, so we should be able to do multiple without a problem if we do go that route.

@vmarmol agree

On Wed, Mar 25, 2015 at 12:37 PM, Victor Marmol notifications@github.com
wrote:

Injecting a binary requires injecting a whole volume, so we should be able
to do multiple without a problem if we do go that route.

—
Reply to this email directly or view it on GitHub
#4890 (comment)
.

@vmarmol I think now you're right regarding the env; the container-init binary will have to have this responsibility, in order for the env to be set in the child process (the entrypoint). Agree? Disagree?

@pmorie agree

I'm unavailable this week. Will take a look when I get back, or reassign to vmarmol.

@erictune this is parked for now, I want to do it another way.

On Mon, Apr 6, 2015 at 4:11 PM, Eric Tune notifications@github.com wrote:

I'm unavailable this week. Will take a look when I get back, or reassign
to vmarmol.

—
Reply to this email directly or view it on GitHub
#4890 (comment)
.

@pmorie I'm closing this for now, since it hasn't been commented on in > 1month and you indicate it's being done a different way.

@brendanburns, my bad, I didn't realize this was still open. Thanks.

On Thu, May 7, 2015 at 1:04 PM, Brendan Burns notifications@github.com
wrote:

Closed #4890 #4890
.

—
Reply to this email directly or view it on GitHub
#4890 (comment)
.