Remove namespace usage

[migmartri]

Namespaces in Helm are global, not scoped by namespace. Here #781 we have updated the get release code to not to rely on listing and filtering releases, which means that we do not need to filter per namespace anymore.

This patch renames get for getRelease and updates some of those proxy handlers to stop passing the unnecessary namespace.

NOTE: This does not change the tiller proxy API which eventually we might want to revisit

Refs #765

[andresmgot]

I am fine with the change removing the namespace (this was something mostly inherited from HelmCRD) but the problem right now is that an user could make a request GET /namespace/foo/release/bar and we would be returning a release that potentially is not in foo without returning any kind of error.

I know that we are safe because we are doing proper requests from the front end but before we forget and we leave this in a inconsistent state I would remove the namespace from the routes that are not needed (unless you plan to do that in a different PR).

[migmartri]

@andresmgot yes, you are right, that's why I added a comment here https://github.com/kubeapps/kubeapps/pull/782/files#diff-a8ee397eb96fc3f4801cb4e038840e1bR146

I was thinking of tackling this in a different PR #783

[andresmgot]

Yes, what I want to clarify is that we are adding a regression (which was not included in this PR but in #781). This PR leaves the code in a better status from what is in master right now so I am okay merging it.

[migmartri]

A regression? Where? The functionality is the same no? If you meant that we are not using the namespace to filter a release name you are right, but in practice I believe that it does not make any difference.

Am I missing anything?

[prydonius]

but the problem right now is that an user could make a request GET /namespace/foo/release/bar and we would be returning a release that potentially is not in foo without returning any kind of error.

I don't think this is that important, but if we want we can always verify the namespace after getting the release.

I'm also confused about the regression @andresmgot mentions, let's hold this until we find out what he meant.

[prydonius]

actually, all the routes do require the namespace, because we render the manifest with ResolveManifest and go through each kind to ensure a user can get, create, update or delete the kind in the namespace.

[andresmgot]

oh, true, I forgot about that.

[andresmgot]

A regression? Where? The functionality is the same no?
I'm also confused about the regression @andresmgot mentions, let's hold this until we find out what he meant.

Sorry, what I meant is that previously a GET /namespaces/foo/releases/bar returned an error if bar is not in foo while right now it doesn't. As I said, that is not related to this PR but the previous one. That's why I accepted the PR.

As @prydonius comments, the namespace is necessary to check if the user has the necessary permissions to perform an action. I think we can remove the TODO comment and, if we want to keep the previous behavior, we can add a simple check in getRelease that returns an error if the release is not a namespace.

(Unchecked) The above seems important because for example, when upgrading an application, an user could try to upgrade /namespaces/foo/releases/bar. If he has permissions to write in foo but not in the real namespace of bar (bar may belong to a different user) the proxy will do the operation while it should fail for two reasons: bar is not in foo and the user doesn't have permissions to write in the namespace of bar.

[andresmgot]

(see comment above)

[prydonius]

(Unchecked) The above seems important because for example, when upgrading an application, an user could try to upgrade /namespaces/foo/releases/bar. If he has permissions to write in foo but not in the real namespace of bar (bar may belong to a different user) the proxy will do the operation while it should fail for two reasons: bar is not in foo and the user doesn't have permissions to write in the namespace of bar.

Great catch, @andresmgot - you're absolutely right that this change would leave this security hole open. We should definitely ensure we don't introduce this regression in this PR.

[migmartri]

(Unchecked) The above seems important because for example, when upgrading an application, an user could try to upgrade /namespaces/foo/releases/bar. If he has permissions to write in foo but not in the real namespace of bar (bar may belong to a different user) the proxy will do the operation while it should fail for two reasons: bar is not in foo and the user doesn't have permissions to write in the namespace of bar.

Thank you @andresmgot for the explanation, I agree with you that it is a regression so I've rolled back the previous implementation to just focus on fixing it.

Basically what I do is to retrieve the release with the "new way" but then confirm that the namespace of that release is the one we provided (if any) and previously verified in the handler. Let me know if it makes sense.

In any case, I have mixed feelings about the current API, for example I do not think that we should allow/require passing a namespace in the upgrade route but instead get the namespace from the referenced release. Mainly because now it is confusing that even though I am passing a namespace, the only purpose is to verify access but it does not actually have any effect in the upgrade process.

In any case, this is not in my opinion worth discussion now.

Thank you for raising this issue @andresmgot!!

[andresmgot]

Thanks for the changes!

[andresmgot]

In any case, I have mixed feelings about the current API, for example I do not think that we should allow/require passing a namespace in the upgrade route but instead get the namespace from the referenced release

Yes, the only case in which it's necessary is when deploying a new app so I wouldn't mind changing the routes and make the namespace a field of the request body in that case.

Anyway, what I like of the current routes is that they are not helm specific (even though the proxy is specific to tiller). What I mean is that the routes would be the same if the backend is an App CRD controller for example (in case that for App CRDs names are not cluster-unique) so hypothetically we could extend this proxy to use different backends.

[migmartri]

In any case, I have mixed feelings about the current API, for example I do not think that we should allow/require passing a namespace in the upgrade route but instead get the namespace from the referenced release

Yes, the only case in which it's necessary is when deploying a new app so I wouldn't mind changing the routes and make the namespace a field of the request body in that case.

Anyway, what I like of the current routes is that they are not helm specific (even though the proxy is specific to tiller). What I mean is that the routes would be the same if the backend is an App CRD controller for example (in case that for App CRDs names are not cluster-unique) so hypothetically we could extend this proxy to use different backends.

Right, it is just that to me, it is a little bit confusing and personally I prefer to build things from bottom up, starting with the current case unless there is another use case in the horizon.

Also the current implementation is more error prone (as we could see when I added the regression) since we rely on doing a check in two places. Truth is that this would have been discovered if we had a test for it.

In any case I do not think this is worth changing right now and I really appreciate your help on finding this issue. Sorry about that