Security Model #4198
Comments
|
@tokoko we started thinking about a possible solution, that we can share after completing the internal review, but first we'd like to ask a few questions to verify our understanding of the initial requirements (*):
(*) We also like option 2 for the reasons you mentioned above. Additionally, we can share a reviewed definition to better align with the usual RBAC permission models coming from our previous work with Keycloak permission features. |
I'm not sure I'll be able to list everything comprehensively here, but I think there're two major sets of permissions (and protected resources as a result) to consider here. The first part is CRUD-like permissions for manipulating objects in feast registry: Another aspect is managing access to the underlying data.
You mean SDK usage without setting individual components as
I'm with you on that one. I think we should start by agreeing on some sort of
Maybe, I'm not sure what that would look like though. Do you mean listing defined roles or permissions that can be specified in those roles?
Cool, we can start there then. |
|
@dmartinol Sorry, I just took a look at pycasbin. I guess it's a rules engine, so disregard my answer above. It looks promising, but I'm fine with home-grown option as well, depends on how complicated our version will be to maintain I guess. |
Yep, something like |
So you mean the FeastObjectType enum (to be extended to support the map also BTW: what about the |
|
Yes, that sounds about right. not sure what you mean about |
🤔 yes, seeing it from this perspective, this is fine. So, for completeness we probably need all the CRUD actions like: |
|
I'm undecided between having |
|
@tokoko on the implementation side, do you think a programmatic solution is mandatory (e.g., like the PyCasbin enforcer), or can we avoid changing the code and instead use decorators to enforce permission policies? |
|
@dmartinol @tmihalac I think the most logical points where permission enforcement should happen is in the methods of the major feast abstract classes ( |
|
@tokoko we want to share with you a gist describing a proposal to implement this functionality. The modelling part follows your initial requirement but tries to adapt it to some standards that we've found in Keycloak. |
|
@dmartinol Can you clarify what's
|
|
Also, what's To me something like this makes more sense instead of And then we would have concrete classes like |
Quoting Keycloak docs,
So, the reason for having read_policy = RoleBasedPolicy(['reader', 'viewer'])
admin_policy = RoleBasedPolicy(['admin'])
permission1 = FeastPermission(...,policies=[read_policy, admin_policy],...)
permission2 = FeastPermission(...,policies=[read_policy],...)
permission3 = FeastPermission(...,policies=[admin_policy],...)
It is not a
The model that you are proposing describes the permissions allowed for any given role instead of the roles/policies allowed for any given permission. Since the cardinality is N-N, it probably doesn't change much, but please consider that roles are not policies, and in the future we could extend the concept and manage policies that are not based on roles. @jeremyary do you have any modelling preferences here? |
The idea here is that another entity like the Maybe |
|
@dmartinol thanks, that makes sense and looks like keycloak uses those terms so mustn't confusing for everyone. I'm probably influenced by AWS terms where policies and permissions aren't really separated in any way, policies are more or less just a collection of permissions. My only concern is that if we end up sticking with just |
I may be the one missing some context here, sure. The way I think about it when a user authenticates in some way, auth provider is also usually the one that can provide additional info regarding roles and attributes. mlflow has something similar where |
@tokoko I would suggest each, as the flexibility will be great for users |
|
I am also pro Option 2 at the moment |
|
@dmartinol Looks really good overall, thanks. Obviously still going through it, but I'll leave remarks here along the way:
|
Agree, there are no keycloak-specific details, and the required URLs can be retrieved from the discovery URL like |
This was also my initial concern (see previous comment, and you suggested to use decorators, which instead may be difficult to apply to
because the decorator, IIUC, needs to identify the input arguments containing the protected resources (e.g. For this reason, we can adopt a programmatic approach (mentioned in the POC) at all identified entry points, allowing us to apply precise validation, such as: a : ResourceA = ...
_get_security_manager().assert_permissions(a, AuthzedAction.EDIT)I've updated the POC with an BTW, this approach works if we agree on the above assumption that we:
We are aware that client code can bypass these start methods and directly access the inner objects; however, this is outside the scope of our investigation. |
|
I guess I was referring to a more complicated decorator, something that would know how to inspect values passed in |
There's a def assert_permissions(
self,
resource: Resource,
actions: Union[AuthzedAction, List[AuthzedAction]],
):
return TrueOf course, it's a POC, and we can elaborate on it further |
|
thanks, overlooked that part. Another question, I understand that role manager and policy enforcer should be global objects, but isn't |
Agree, we'll write a test to verify that this use case is denied for secured deployments |
|
@dmartinol I'm fine with either approach. If you think some of the design needs more work, we can start there and integrate later. In the meantime, another late bright idea from me.. 😄 What do you think about putting policy enforcement logic behind registry as well? We could implement it under a new |
|
@tokoko we're starting to create an initial PR with the agreed model and an implementation for the existing servers. wrt to moving the validation API to the security_manager.permissions = registry.list_permissions() # proxy for remote endpoint, if needed |
|
@tokoko, Assuming that the store admin creates the project with |
|
@tokoko a couple of points to clarify with you that were raised during our internal discussions: Permission(
name="read-from-any-A",
resources=[FeatureView, FeatureService],
policies=[RoleBasedPolicy(roles=["a-reader"]), RoleBasedPolicy(roles=["any-reader"])],
actions=[AuthzedAction.READ],
)could be simplified as: Permission(
name="read-any",
resources=[FeatureView, FeatureService],
policy=RoleBasedPolicy(roles=["a-reader", "any-reader"]),
actions=[AuthzedAction.READ],
)2- Second, also related to the previous, do you think the store admins want to have reusable Policies shared amongst different Permissions? e.g., do we need to have independent policies with their own (unique) name and manage them as independent Feast resources? p = RoleBasedPolicy(roles=["a-reader", "any-reader"])
Permission(
name="read-any",
resources=[FeatureView, FeatureService],
policy=p,
actions=[AuthzedAction.READ],
)
Permission(
name="read-data",
resources=[DataSource],
policy=p,
actions=[AuthzedAction.READ],
)BTW: my personal answers are |
Yeah, since
I think I suggested turning it into a
The only upside is a protection against typos here, so +1 for embedded as well, one feast resource (
My point is that if permission check logic is part of security manager rather than the registry, other feature servers (go/java) will have to reimplement it as security manager class won't be reusable across languages. Still, It's not yet critically important, I agree. Let's implement it first and reassess how much of a problem that will be after. |
|
@tokoko about the actual validation logic, you wrote
Since most of these methods (e.g. in [BaseRegistry]
# Remove abstractmethod
def apply_entity(self, entity: Entity, project: str, commit: bool = True):
_assert_permissions(resource=entity, actions=[AuthzedAction.CREATE, AuthzedAction.UPDATE, AuthzedAction.DELETE])
_apply_entity(self, entity: Entity, project: str, commit: bool = True):
# Renamed abstract method
@abstractmethod
_def apply_entity(self, entity: Entity, project: str, commit: bool = True):
raise NotImplementedError(*) In case it happens after, we must guarantee that the execution had no unauthorized side effects on the data stores. As an alternative, we could invent a new decorator where you can specify when to run (BEFORE or AFTER the actual method) and on what parameter to apply, but even this solution needs a new abstract method to be introduced, as both decorator cannot cohexist AFAIK (and tested 😞): [BaseRegistry]
# 'what' is the input arg to validate against permissions
@require_permission(when=BEFORE, what="entity", actions=[AuthzedAction.CREATE, AuthzedAction.UPDATE, AuthzedAction.DELETE])
def apply_entity(self, entity: Entity, project: str, commit: bool = True):
_apply_entity(self, entity: Entity, project: str, commit: bool = True):
@abstractmethod
_def apply_entity(self, entity: Entity, project: str, commit: bool = True):
raise NotImplementedError
# 'what' not needed for validation AFTER the execution, it applies on the returned value
@require_permission(when=AFTER, actions=[AuthzedAction.CREATE, AuthzedAction.UPDATE, AuthzedAction.DELETE])
def get_entity(self, name: str, project: str, allow_cache: bool = False) -> Entity:
return self._get_entity(name=name, project=project, allow_cache = allow_cache)
@abstractmethod
def _get_entity(self, name: str, project: str, allow_cache: bool = False) -> Entity:
raise NotImplementedErrorIn case the decorator cannot apply, we can use the previous programmatic option. CC: @redhatHameed |
|
I did something similar for CachingRegistry class previously. Didn't use decorators there, but had to introduce new abstract methods. The only reason I did that in a new class was to avoid changes in the abstract one. Unfortunately, I don't think we can do the same here once again as two wrapper classes won't really get along with one another. Maybe we can do it in I like the decorator design here, but keep in mind that simple BEFORE/AFTER approach might not cut it for |
IIUC, we can move all the abstract methods from return super().get_data_source(name, project)This should allow to execute the pipeline of caching logic -> validation check -> actual method |
Decorators can only implement validation checks before or after the actual implementation, so on method arguments or return values: if the validation process must happen in the middle of it, the programmatic approach is the only solution I can see. As you said, it's probably the case of most methods of the |
|
Let me do a complete 180 for a moment. Maybe the best place for enforcement are server endpoints after all.. Other than the simplicity of the implementation, I'm thinking of a scenario when a user has |
|
1- about your proposal, isn't the same as saying we need to restrict the permissions at the methods exposed by server endpoints and validate only the input parameters (or those that are directly derived from them, coming from the server request)? the advantage is to apply it once for all the servers, for each exposed API, and avoid changing the server code in case they're not holding a reference of the protected resource. 2- let me do a 180 myself (it's viral 😉): is the requirement to secure individual instances with different permissions coming from the field or is it a dev's proposal? If we're looking for simplicity, wouldn't be much easier to protect the endpoints with system-defined roles instead? @roles_required('offline_store_query', message="cannot query the offline store")
def get_historical_features(
...The only configuration needed for the admins would be to assign roles to the users (maybe the "system-roles" could be customized further to isolate the server instances with different roles), with the drawback of not configuring security on individual instances. |
I'm not sure this is much of an advantage in reality, though. Most of our servers reflect abstract class methods one to one anyway (in order to provide
It is a dev's proposal 😄 but I'd like to think it's an informed one. I'm basing the need for granular control on the fact that very often feast deployment 1) will be centralized across teams and 2) feast feature views will contain sensitive information. From security perspective, that makes feature store equivalent to a data warehouse of sorts. Having an all or nothing approach there seems inadequate. |
|
@tokoko FYI @redhatHameed is validating the approach to enforce permissions on the servers, as per the updated requirement. Thanks for clarifying your point of view, we'll probably share an initial PR with model changes and an implementation on a selected server in the next few days/early next week. |
|
@dmartinol @redhatHameed sounds good. btw, to continue the above discussion regarding sometimes having to check permissions in the middle of execution rather than before/after. I think in practice it would make sense to try to fully decouple that process from the execution even if it means that some of the objects will need to be fetched twice from the registry (once during permission check by server code and again during execution by provider code). In most scenarios the whole registry object will be cached on the server so I think performance penalty incurred should be negligible. |
This is exactly the way we are experimenting it right now. |
|
@tokoko one more doubt about how to validate the user roles. If a permission is defined with roles "reader" and "writer", does it mean that any of them are required for the current user, or both are instead required? |
|
I assumed "any" as well, seems more practical. |
|
(oops, this thread never ends) Follow two possible example for the k8s and oidc authorization servers: offline_store:
type: remote
host: localhost
port: 8815
auth:
type: kubernetesoffline_store:
type: remote
host: localhost
port: 8815
auth:
type: oidc
server: 'http://0.0.0.0:8080'
realm: 'poc'
client-id: 'app'
client-secret: 'mqAzX7zDalQ1a3BZRWs7Pi5JRqCq7h4z'
username: 'username'
password: 'password' |
|
more the merrier. Yeah, once servers are secured, we will have to enable P.S. This probably wasn't the point, but I'm not sure I follow your |
What I mean is that the client app has to pass authentication token to the server in the http header (also for grpc servers) and since the header management is not or grpc endpoints directly. |
We’re not implementing authentication in Feast, right? So the client has to request the token to the Oidc server and it needs all these settings for that. The server, in turn, needs some of these settings to extract the user roles from the token. |
@tokoko opened PR that asserts/checks permissions on remote servers (offline, online, registry) by utilizing the permission/security model framework (still work in progress). Just sharing this with you to get initial feedback and to ensure we are moving in the right direction. |
@dmartinol Sure, never mind. I thought the way to implement it would be for a client to ask the server for the oidc server url, sort of mimicking the browser flow where it's the job of the server to redirect.
@redhatHameed Thanks, looks good so far. I'll leave the comments in the PR and let's continue there. |
|
@tokoko we're defining the integration with authorization servers for both Feast servers and clients, and I believe we could have a common auth:
type: kubernetesor: auth:
type: oidc
server: 'http://0.0.0.0:8080'
realm: 'poc'
client-id: 'app'
client-secret: 'mqAzX7zDalQ1a3BZRWs7Pi5JRqCq7h4z'
username: 'username' # only for client mode
password: 'password' # only for client modeThis way we can reuse the same configuration/ adding @lokeshrangineni who's looking at the client side |
Is your feature request related to a problem? Please describe.
This is an offshoot ticket from #4032. Feast is slowly approaching a state in which all major components can be deployed as remote servers. This enables us to start thinking about a comprehensive security model for access to each of them (registry, online store, offline store)
Describe the solution you'd like
Here's a high-level overview of what I'm expecting from the security model:
I'd avoid incorporating user management into feast as much as possible. We should probably have a pluggable authentication module (LDAP, OIDC, etc...) that takes user/password (or token), validates it and spits out the roles that have been assigned to this particular user. Each server will have to integrate with this module separately, http feature server will get user/pass from basic auth, grpc and flight will get them according to their own standard conventions and pass credentials to the module to get the list of assigned roles. (Somewhat inspired by Airflow security model)
(Option 1) We enrich Feature Registry to also contain information about the roles available in the system and each feast object should be annotated with permissions. In other words, the user would run
feast applywith something like thisThe upside of the second approach is that it's a lot less invasive than the first one. You could potentially end up with a setup where permissions and objects are managed with some level of separation between them. I think I'm more in favor of this.
The text was updated successfully, but these errors were encountered: