Fix env variables to include existing environment variables, then override

[brendandburns]

Fixes #243

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns

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 [brendandburns]

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

[drubin]

I wonder if this is really expected behaviour? Obviously this fixes the bug (we should also add a test for it)

This running locally on a developer's laptop might make sense but do we really want to pass the servers env to commands?

Just a question I can see this leading some security issue later down the line.... Where by this library is exposing env.

[michaelgeorgeattard]

I am trying to understand the fix.

From what I understand, the environment variables are being read from process.env. Is this the expected behavior or should we read from kube config instead?

[drubin]

@michaelgeorgeattard #243 (comment)

So it is passing your existing env variables from your current shell/process to the command in addition to the ones defined in the kubeconfig.

It is merging the current proccess.env with the ones from the kubeconfig (kubeconfig having higher precedence) which should fix the issue.

[brendandburns]

@drubin this is yet another undocumented part of Kubeconfig handling...

When we look at the original go code for this, this is what they are doing.

The override is here:
https://github.com/kubernetes/client-go/blob/master/plugin/pkg/client/auth/exec/exec.go#L290

And the loading in of the shell's environment variables is here:
https://github.com/kubernetes/client-go/blob/master/plugin/pkg/client/auth/exec/exec.go#L133

So whether this is secure or not, this is the current (and expected) behavior.

I'm actually not sure there's a security problem, since these aren't sent to the server, they're only used to invoke the shell executable that returns the security token.

[drubin]

It's trivial and lgtm but would prefer if we had a test but feel free to Merge if adding a test is too difficult.

[drubin]

Also thanks for the awesome detailed reply and I fully agree we should stick expected behaviour.

[brendandburns]

@drubin test added. took more work than it should have, but the next one will be easier :)

[drubin]

@brendanburns your linting is off again, but thanks so much for adding a test!

The tests pass so /lgtm but you need to fix the linting but if that's the only change I am fine with merging once the linter passes.

Maybe we should add husky as a pre-push check to include the tests and linting? it seems like this happens often.

[brendandburns]

@drubin this is what I get for working on an airplane via 'vi'

I'd be happy to see a pre-push check :)

Oh, and I fixed the lint issues :)

[drubin]

/lgtm

Sorry about the back and forth will try setup pre commit hooks soon