search

cloudfoundry / cloud_controller_ng

Cloud Foundry Cloud Controller
Apache License 2.0
191 stars 357 forks source link

Allow anyone able to view a service instance to be able to see which spaces are shared with #3930

Closed Benjamintf1 closed 2 weeks ago

Benjamintf1 commented 1 month ago

Issue

Right now, only the service owners(people with access to the origin space) are able to get the service instances 'shared_spaces'. To be able to determine which apps are able to be bound to a service, a user would have to do 1 get for every space they have access to be able to determine which spaces the service is in. The cli also isn't able to view shared spaces when calling cf service <service>

Context

A user who has access to even all of the shared spaces is unable to get shared_spaces. they can use fields to get the name of the space and org it comes from, but they can't get shared_spaces.

Steps to Reproduce

Create multiple spaces. Share service from donor space to multiple shared space. Create a user who only has access to shared spaces. Try and figure out how to determine which spaces the service is shared with.

Expected result

The user should be able to do so just by using the service's shared_spaces endpoint.

Current result

Only a user who has access to the origin space is able to access those details (even if they have access to none of the shared spaces, they can still see them).

Possible Fix

Allow users to get access to shared_spaces if they can access the service.

Gerg commented 1 month ago

I believe @blgm and @FelisiaM originally worked on this

Benjamintf1 commented 1 month ago

https://github.com/cloudfoundry/cloud_controller_ng/pull/3931

blgm commented 1 month ago

Yes, @FelisiaM and I were on the team who worked on the V3 API for a this. I think it was based on an earlier implementation that was partly V2 and partly V3. I don’t think we had strong reasons for not allowing this. We were aiming to match existing behaviour, and leave the door open to improvements in the future. This request sounds very reasonable, and as long as it doesn’t inadvertently create security holes then it makes sense.

philippthun commented 1 month ago

I think exposing the guids of shared spaces is fine, but we should prohibit the exposure of space names to users that are not allowed to read from those spaces. So I guess the corresponding field decorator needs to be adjusted as well.

Benjamintf1 commented 1 month ago

Right now there isn't any filtering at all of allowing or prohibiting space or org names. If you have access to the endpoint, you can ready all the names fields can expose. I don't think this would be a change, but something to consider going forward if we wanted to at some point restrict it more.

Benjamintf1 commented 1 month ago

Just want to clarify what I wrote yesturday. Right now, on existing published versions of capi, users can use fields to access the name of orgs and spaces they don't have access to on the shared_spaces endpoint. Allowing users of a instance in a shared space to access those names would not fundamentally change this fact.

Benjamintf1 commented 1 month ago

any further thoughts anyone?

sethboyles commented 1 month ago

users can use fields to access the name of orgs and spaces they don't have access to on the shared_spaces endpoint. Allowing users of a instance in a shared space to access those names would not fundamentally change this fact.

That would still expose more space names to more users, right? Since right now this only leaks when you have permission to view the originating space?

Benjamintf1 commented 1 month ago

It would allow more users to see more space names. I'm less concerned because I actually don't necessarily see a "correct hirarchy" between spaces shared. The originating space could be a "more secure" space, or a "less secure space". Giving the shared spaces users ability to read feels more like "level setting" then "exposing to less priviledged users" to me to be honest. (and everyone always had access to the name of the space and org of the originating space)

philippthun commented 3 weeks ago

As I changed another decorator a while ago due to a permission issue (see #3692), I created a new PR #3962 (based on #3931, thus needs a rebase later on) that adapts the field decorators and filters out space and organization names based on permissions. This is the relevant commit: ddc380

philippthun commented 2 weeks ago

PR #3931 has been merged, so this issue is resolved.