Visitar URL original
Security Model · Issue #4198 · feast-dev/feast · GitHub
Skip to content

Security Model #4198

Description

@tokoko

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:

  1. 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)

  2. (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 apply with something like this

admin_role = FeastUserRole(name='admin')
reader_role = FeastUserRole(name='reader')

FeatureView(
	name=...
	schema=...
	...
	permissions={
		'read': [role],
		'write': [admin_role] 
	}
)
  1. (Option 2) Another option is to try to mimic AWS IAM and brush up on our regexes. In this case instead of annotating objects with permissions, you're annotating roles with policies.
risk_role = FeastUserRole(
	name='team_risk_role',
	permissions=[
		FeastPermission(
			action='read', //read_online, read_offline, write_online, write_offline
			conditions=[{
				'name': 'very_important_*',
				'team': 'risk'
			}]
		)
	]
)

FeatureView(
	name='very_important_fv',
	schema=...
	...
	tags={
		'team': 'risk'
	}
)

The 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.

  1. Once a server gets ahold of user roles and permission information from the registry, all components will apply the same "rules engine" to authorize the requested action.

Activity

  1. dmartinol commented on May 15, 2024

    @dmartinol
    Contributor

    @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 assume that the protected resources are all instances of FeatureView (including OnDemandFeatureView), FeatureService and theFeatureStore, correct?
    • Is the intention to also prevent unauthorized accesses from clients using the Feast SDK, instead of the servers? The reason for asking is that we are also thinking to add similar requirement to prevent undesired updates to the data stores from non-admin personnel. However, we are aware that this may be a corner case scenario in most production deployments, as the feature store definition (e.g. the feature_repo.yaml used to feast apply) should not be accessible to all the users (e.g., in a RHOAI setup, it should live in a separate "admin notebook" or in a well-protected git repo and branch), and maybe it's not a real concern at all.
    • On the implementation side, are you thinking to leverage any existing library like PyCasbin -that may also look overkill for now- or a simplified in-house solution implementing the security model? (personally, we'd avoid this option to avoid any vendor lock-in)
    • Should we also add a feast feast-permissions list [--verbose] to list the existing permissions together (in verbose mode) with the matching resources? (and maybe list the unprotected resources, which can help troubleshooting permission errors)

    (*) 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.

  2. tokoko commented on May 15, 2024

    @tokoko
    CollaboratorAuthor
    • We assume that the protected resources are all instances of FeatureView (including OnDemandFeatureView), FeatureService and theFeatureStore, correct?

    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: Entity, DataSource, FeatureView, StreamFeatureView, OnDemandFeatureView, FeatureService, SavedDataset, ValidationReference. (TAL at registry server proto for reference) These will probably need can_read and can_edit actions (But note that read here refers to reading object definition, not the data).

    Another aspect is managing access to the underlying data. FeatureService and all variations of FeatureViews should probably have can_query action. DataSource and SavedDataset will require can_query and can_write actions and so on. I'm sure I'll miss something here.

    • Is the intention to also prevent unauthorized accesses from clients using the Feast SDK, instead of the servers? The reason for asking is that we are also thinking to add similar requirement to prevent undesired updates to the data stores from non-admin personnel. However, we are aware that this may be a corner case scenario in most production deployments, as the feature store definition (e.g. the feature_repo.yaml used to feast apply) should not be accessible to all the users (e.g., in a RHOAI setup, it should live in a separate "admin notebook" or in a well-protected git repo and branch), and maybe it's not a real concern at all.

    You mean SDK usage without setting individual components as remote, right? no, I don't think that's the intention simply because that would be way too hard to accomplish. In such a case a client application needs direct access to the underlying resources, so we would have to somehow inject ourselves in that, also considering that different implementations of components will have completely different permissions. tbh, I don't think that's even possible. I think we should enforce permissions only on the servers. If someone has access to the underlying resources itself and configures feature_store.yaml with necessary credentials, they will be able to circumvent the security model and I think it might be completely fine for some use cases, for example materialization may be something that's orchestrated by a central ML Platform team only that doesn't really need to care about permissions.

    • On the implementation side, are you thinking to leverage any existing library like PyCasbin -that may also look overkill for now- or a simplified in-house solution implementing the security model? (personally, we'd avoid this option to avoid any vendor lock-in)

    I'm with you on that one. I think we should start by agreeing on some sort of FeastSecurityManager abstract interface (a class with a method that takes user/pass and outputs a list of roles for example) w/o any external dependency. We could then use some off-the-shelf functionality for each auth method implementation.

    • Should we also add a feast feast-permissions list [--verbose] to list the existing permissions together (in verbose mode) with the matching resources? (and maybe list the unprotected resources, which can help troubleshooting permission errors)

    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?

    (*) 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.

    Cool, we can start there then.

  3. tokoko commented on May 15, 2024

    @tokoko
    CollaboratorAuthor

    @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.

  4. dmartinol commented on May 15, 2024

    @dmartinol
    Contributor

    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?

    Yep, something like

    feast feast-permissions list --verbose
    permissions
    ├── feast-admin [feast-admin]
    │   └── FeatureStore
    ├── read-online-stores [role1, role2]
    │   ├── FeatureService:fs1
    │   ├── FeatureView:fv1
    │   └── FeatureView:fv2
    └── write-offline-stores [writer]
        └── FeatureView:fv3
    
  5. dmartinol commented on May 15, 2024

    @dmartinol
    Contributor

    ...manipulating objects in feast registry: Entity, DataSource, FeatureView, StreamFeatureView, OnDemandFeatureView, FeatureService, SavedDataset, ValidationReference. (TAL at registry server proto for reference)

    So you mean the FeastObjectType enum (to be extended to support the map also SavedDataset and ValidationReference types).

    BTW: what about the Registry type instead? e.g., how can we model the permissions to execute feast apply otherwise?

  6. tokoko commented on May 15, 2024

    @tokoko
    CollaboratorAuthor

    Yes, that sounds about right. not sure what you mean about Registry type, can you elaborate? When a user runs feast apply it almost exclusively boils down to crud operations on the above mentioned list of resources applied to the registry. So those are the protected resources, Registry type is just an interface where crud of these objects are applied from. Maybe I'm missing something?

  7. dmartinol commented on May 15, 2024

    @dmartinol
    Contributor

    Yes, that sounds about right. not sure what you mean about Registry type, can you elaborate? When a user runs feast apply it almost exclusively boils down to crud operations on the above mentioned list of resources applied to the registry. So those are the protected resources, Registry type is just an interface where crud of these objects are applied from. Maybe I'm missing something?

    🤔 yes, seeing it from this perspective, this is fine. So, for completeness we probably need all the CRUD actions like:
    create, read, update, delete plus query, query_online, query_offline and write, write_online, write_offline

  8. tokoko commented on May 15, 2024

    @tokoko
    CollaboratorAuthor

    I'm undecided between having create, update, delete vs a single edit action.

  9. dmartinol commented on May 16, 2024

    @dmartinol
    Contributor

    @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?
    BTW, in my opinion, we cannot use decorators because some affected functions manipulate multiple protected resources (e.g., FeatureViews) at the same time. Additionally, the code may not be structured to support such granular configuration at the individual API level. However, I'd like to hear your feedback on this.
    +@tmihalac who raised the question

  10. tokoko commented on May 16, 2024

    @tokoko
    CollaboratorAuthor

    @dmartinol @tmihalac I think the most logical points where permission enforcement should happen is in the methods of the major feast abstract classes (OfflineStore, OnlineStore, BaseRegistry). I think for the registry where CRUD-like operations live, granularity shouldn't be a problem because of how BaseRegistry is designed. For OnlineStore and OfflineStore, yes, sometimes you might get a request for multiple feature views at once, but I don't really see why that would be a problem for a decorator, tbh. decorators are just function preprocessors, right? Maybe I'm missing something, but I don't really see the difference between those options other than that decorators will probably look better...

  11. dmartinol commented on May 16, 2024

    @dmartinol
    Contributor

    @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.
    Apart from that, we also propose a possible architecture of the security management solution and some example of usage in the Feast code (both programmatic and decporation options).

  12. tokoko commented on May 17, 2024

    @tokoko
    CollaboratorAuthor

    @dmartinol Can you clarify what's RoleBasedPolicy exactly for me? Looks like it's a list of roles (extracted from keycloack for example) that have this permission assigned to them. If that's the case:

    • I'm not sure I like the name 😄 Can't this just be a roles parameter that takes a list of strings?
    • Is it a good idea to have a list of roles scattered around with permission objects? Wouldn't it be better to have another FeastRole(name: str, permissions:List[FeastPermission]) resource? The downside is introducing another resource type that needs to be managed, of course.. just a suggestion. wdyt?
  13. tokoko commented on May 17, 2024

    @tokoko
    CollaboratorAuthor

    Also, what's add_roles_for_user method in RoleManager? Does that mean there should be a way to assign roles to the user from feast itself or maybe I'm misunderstanding the class? If we have auth backends like LDAP or OIDC, I was thinking we could delegate role management to them fully, so that assigning roles to the user would happen in AD, Keycloak or somewhere like that.

    To me something like this makes more sense instead of RoleManager:

    class AuthManager:
        """auth management"""
    
        def authenticate(self, user: str, password: str) -> List[str]:
            """
            Returns a list of roles if authentication successful, empty if auth failed.
            """
            return False
    

    And then we would have concrete classes like LdapAuthManager, OidcAuthManager and so on.

  14. dmartinol commented on May 17, 2024

    @dmartinol
    Contributor

    @dmartinol Can you clarify what's RoleBasedPolicy exactly for me? Looks like it's a list of roles (extracted from keycloack for example) that have this permission assigned to them. If that's the case:

    • I'm not sure I like the name 😄 Can't this just be a roles parameter that takes a list of strings?

    Quoting Keycloak docs, policies define the conditions that must be satisfied before granting access to an object, and we could have policies based on different criteria, e.g. (also speaking Keyclock-ish):

    • User-based policy: match the configured user against the user in the client request
    • Role-based policy: match the configured roles against the roles of the user in the client request
    • Attribute-based policy: ...

    So, the reason for having RoleBasedPolicy was to make room for introducing a Policy interface that can be added later with a type field (one of role, user) or even with an enforce method to apply the policy verification.
    These policy entities can be shared by multiple permissions, if it is the (likely) case:

    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],...)
    • Is it a good idea to have a list of roles scattered around with permission objects?

    It is not a a list of roles scattered around with permission objects, but a list of policies to be applied to allow the execution of the protected actions 😉.

    Wouldn't it be better to have another FeastRole(name: str, permissions:List[FeastPermission]) resource? The downside is introducing another resource type that needs to be managed, of course.. just a suggestion. wdyt?

    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?

  15. 30 remaining items

  16. dmartinol commented on Jun 25, 2024

    @dmartinol
    Contributor

    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?
    The solution we're designing offers a fine granular configuration, but may also lead to complex setup where the admins need to ensure that every resource is protected by permissions for all the managed operations. The security configuration would imply a consistent review of the feature store configuration (*.py) and there would be no security enforced OOTB: each team has to define his own permissions.

    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.

  17. tokoko commented on Jun 25, 2024

    @tokoko
    CollaboratorAuthor

    the advantage is to apply it once for all the servers

    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 remote implementations). We'd essentially have to write almost exactly the same amount of code.

    is the requirement to secure individual instances with different permissions coming from the field or is it a dev's proposal?

    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.

  18. dmartinol commented on Jun 26, 2024

    @dmartinol
    Contributor

    @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.

  19. tokoko commented on Jun 26, 2024

    @tokoko
    CollaboratorAuthor

    @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.

  20. dmartinol commented on Jun 26, 2024

    @dmartinol
    Contributor

    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

    This is exactly the way we are experimenting it right now.

  21. dmartinol commented on Jun 26, 2024

    @dmartinol
    Contributor

    @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 was thinking to use "any" rather than "all", which is also the default for Keycloak (which also has the concept of "mandatory" role, where all the mandatory roles must be associated to the user), any thoughts?

  22. tokoko commented on Jun 26, 2024

    @tokoko
    CollaboratorAuthor

    I assumed "any" as well, seems more practical.

  23. dmartinol commented on Jun 26, 2024

    @dmartinol
    Contributor

    (oops, this thread never ends)
    @tokoko to simplify the client's life and adopt the usual proxy pattern for remote clients, shouldn't we also extend the remote clients configuration to specify the authorization server used to fetch the access token?
    Otherwise, there would be no way to connect a secured server from a remote client, devs should invoke the REST/grpc services with their own utility code.

    Follow two possible example for the k8s and oidc authorization servers:

    offline_store:
        type: remote
        host: localhost
        port: 8815
        auth:
            type: kubernetes
    offline_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'

    @redhatHameed @tmihalac

  24. tokoko commented on Jun 26, 2024

    @tokoko
    CollaboratorAuthor

    more the merrier. Yeah, once servers are secured, we will have to enable remote implementations to talk with secured servers and consequently add whatever configurations are necessary. The first naive implementation might mean the user will have to configure auth info separately for each component.

    P.S. This probably wasn't the point, but I'm not sure I follow your oidc example, most of these configs seem like stuff that should be configured for the server, not the client. I'm not sure what's the best practice for setting up a non-interactive programmatic oidc session, maybe I'm missing something.

  25. dmartinol commented on Jun 26, 2024

    @dmartinol
    Contributor

    The first naive implementation might mean the user will have to configure auth info separately for each component.

    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.
    The workaround exists, but it’s a bit expensive 😬

  26. dmartinol commented on Jun 26, 2024

    @dmartinol
    Contributor

    P.S. This probably wasn't the point, but I'm not sure I follow your oidc example, most of these configs seem like stuff that should be configured for the server, not the client

    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.

  27. redhatHameed commented on Jun 28, 2024

    @redhatHameed
    Contributor

    @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.

    @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.

  28. tokoko commented on Jun 28, 2024

    @tokoko
    CollaboratorAuthor

    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.

    @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.

    Just sharing this with you to get initial feedback and to ensure we are moving in the right direction.

    @redhatHameed Thanks, looks good so far. I'll leave the comments in the PR and let's continue there.

  29. dmartinol commented on Jul 2, 2024

    @dmartinol
    Contributor

    @tokoko we're defining the integration with authorization servers for both Feast servers and clients, and I believe we could have a common auth section on top of the store definition that can be used accordingly to the runtime mode (e.g., server and client of a specific feature), like:

    auth:
        type: kubernetes

    or:

    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 mode

    This way we can reuse the same configuration/Config class whatever the application that we are running and avoid having replicated Auth section in all of the interfaces, it will just be fs.get_auth() whatever the runtime. Thoughts?

    adding @lokeshrangineni who's looking at the client side

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions