search

gravitational / teleport

The easiest, and most secure way to access and protect all of your infrastructure.
https://goteleport.com
GNU Affero General Public License v3.0
16.94k stars 1.7k forks source link

fix: fix `nextKey` values when using multiple prefixes #43486

Open tigrato opened 3 days ago

tigrato commented 3 days ago

This PR makes generic.Service correctly implementing List* functions when multiple key prefixes are defined

fspmarshall commented 3 days ago

nit: I'd suggest double-checking the access-control logic for for any user-facing resources that use this implementation. With the old calculated style, if the user was allowed to see the last resource on the page, they were also allowed to see the pagination key. With this new style, it's never safe to bubble up the pagination key to the user since RBAC checks haven't been performed on the resource from which it is derived. We're usually pretty good about that these days, but it's always good to double-check when touching this stuff.

tigrato commented 3 days ago

nit: I'd suggest double-checking the access-control logic for for any user-facing resources that use this implementation. With the old calculated style, if the user was allowed to see the last resource on the page, they were also allowed to see the pagination key. With this new style, it's never safe to bubble up the pagination key to the user since RBAC checks haven't been performed on the resource from which it is derived. We're usually pretty good about that these days, but it's always good to double-check when touching this stuff.

@fspmarshall

don't our rbac check enforces that we check the resource itself because the where condition can also be performed with labels?

From what I know, the access check won't work only for names (nextKey) so we must always pull +1 and ensure the RBAC for the extra resource instead of relying on the key itself