Secure valueFrom resolution helper utility function

[coreydaley]

Addresses the following comments against the first pull request (#14143):
#14143 (review)
#14143 (review)

[coreydaley]

@smarterclayton Could you expand on your comments (#14143 (review)) about the security escalation attack and what kind of tests you are referring to?

cc @bparees

[smarterclayton]

If someone accidentally allows a namespace value to be overridden, you'll let the build controller "escalate" - it will fetch a secret using its credentials and then give it to that build. So this code is inherently dangerous, and anyone testing or modifying it can accidentally lead to secret leakage.

…

On Mon, Jun 19, 2017 at 2:05 PM, Corey Daley ***@***.***> wrote: @smarterclayton <https://github.com/smarterclayton> Could you expand on your comments (#14143 (review) <#14143 (review)>) about the security escalation attack and what kind of tests you are referring to? cc @bparees <https://github.com/bparees> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14749 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_pzuJWVCeJTZCV0uBXNTLcUUCgmdXks5sFrhdgaJpZM4N-lHM> .

[coreydaley]

@smarterclayton I understand that, but how would you write a test that would check for that anywhere in the code?

[bparees]

If someone accidentally allows a namespace value to be overridden, you'll let the build controller "escalate" - it will fetch a secret using its credentials and then give it to that build. So this code is inherently dangerous, and anyone testing or modifying it can accidentally lead to secret leakage.

yeah i think we understood that... basically the caller needs to be sure the namespace they are passing in is the right one (the same one the build belongs to) because we're going to fetch whatever secret they asked for, using whatever credentials were passed in (assuming those credentials are able to fetch the secret).

But i'm not sure how we can introduce tests to verify no one is doing otherwise? Presumably we have the same problem in all sorts of other places like where the build controller resolves imagestream references.

[coreydaley]

@smarterclayton Any additional thoughts on mine and @bparees comments?

[bparees]

@smarterclayton i think i understood from our conversation that your main concern is we've moved this code from a relatively contained location where it would only be used by CLI logic (where the "client" would never be privileged and thus couldn't inadvertently resolve secrets the user wasn't able to see themselves), to a location where anyone might stumble upon it and say "oh, this is a handy way to resolve secret references" at which point they might write code that resolves secrets on behalf of the user, when the user isn't permitted to do so themselves.

it's worth noting that that is exactly what the buildcontroller is doing (resolving secrets for the user, using privileged permissions).

The assumption is we're confident the buildcontroller is using this power "safely" (always passing in the namespace associated with the Build, never passing in a random/untrusted namespace.

so the concern is someone else finds this utility and uses it in an unsafe way (passing in a random/untrusted namespace value and a privileged client).

You had suggested adding tests which ensured that the CLI behavior continues to be "safe" (unable to resolve things from other namespaces) to ensure no one abuses this in the future but that doesn't seem sufficient. I see two options:

1. move this utility all the way into the controller and allow the CLI to depend on it (again assuming the CLI client is always sufficiently unprivileged) so at least it lives in an area where people assume the client is privileged and must be careful what it does
2. duplicate the function, one version for the CLI (as it was) and a second one for the controller (again, living w/ the controller so it's more evident the intended use).

And of course either way, big scary godoc around the function about callers needing to use caution about what namespace they pass in, if they are using a privileged client.

Would either (1) or (2) be acceptable?

[coreydaley]

@smarterclayton Any thoughts on the comments from @bparees ?

[smarterclayton]

This addresses my current concerns, I'm fine with just this

[severity:blocker] for the security related refactor to prevent accidental abuse [merge]

[openshift-bot]

Evaluated for origin merge up to cf32caf

[openshift-bot]

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/1267/) (Base Commit: 1251240) (PR Branch Commit: cf32caf) (Extended Tests: blocker) (Image: devenv-rhel7_6436)