guardian icon indicating copy to clipboard operation
guardian copied to clipboard

Import pre-existing access from providers

Open Chief-Rishab opened this issue 2 years ago • 11 comments

Requirements

  • Guardian to collect all pre-existing access from each resource in the provider
  • User (admin) to be able to revoke that imported access if needed

Approach:

1. Fetching access

  • ~~Option 1: add a flag in the provider config to import access~~ Pros : Under the assumption that after the provider is onboarded to guardian there will be no access created outside guardian, this approach would be just sufficient (simpler process) Cons : Will only run once when the provider is just created
  • Option 2: provide an API endpoint to trigger import access Pros : Can be triggered at any time Cons : Need to be triggered manually
  • ~~Option 3: regularly collect existing access with jobs~~ Pros : Continuously control those access created outside guardian Cons : Might increase the scope to the Track access drift feature

2. Creating the appeals

The appeal going to be like this:

{
  "id": "", // auto-generated
  "resource_id": "", // added by guardian
  "policy_id": null,
  "policy_version": 0,
  "status": "active",
  "account_type": "", // imported from provider
  "account_id": "", // imported from provider
  "role": "custom", // TODO: need to find a way to map the existing/imported permissions with the roles user defined in the provider
  "options": {},
  "resource": {}, // added by guardian,
  "approvals": [], // depends on the policy
  "details": {},
  "created_by": "", // TODO: for "user" account type, we can use that as the value here, but can't for serviceAccount or other account types
  "creator": null, // depends on the policy. Might be empty because iam config defined in the policy

  // new field(s):
  "imported": true, // imported flag to differentiate with normal (user-created) appeal
  "permissions": [] // https://github.com/odpf/guardian/issues/205 
}

Things to note

Policy

Policy is going to be null since there's no approval flow for the imported access

Account Types

We are going to import pre-existing access for account types that are defined in the allowed_account_types field in the provider config only.

Role

Assuming this bug has been resolved,

  • Case 1: the imported access are predefined roles in the Provider Config
    1. Suppose in the provider config we have defined a role named bq-admin which has two permissions: roles/bigquery.dataViewer and roles/bigquery.dataEditor.
    2. In case a user has following access granted outside Guardian: roles/bigquery.dataViewer and roles/bigquery.dataEditor
    3. Those access will be imported and mapped as an appeal with role:bq-admin only. Note that it will also have "permissions": ["roles/bigquery.dataViewer", "roles/bigquery.dataEditor"] in the appeal object
  • Case 2: the imported access don't have permissions defined in the provider config
    1. A user has roles/bigquery.metadataViewer access in the bigquery, but that permission was not defined in the provider config.
    2. In that case, each access will be mapped into a single appeal with "role" = "roles/bigquery.metadataViewer" and "permissions" = ["roles/bigquery.metadataViewer"]

Chief-Rishab avatar Jul 07 '22 09:07 Chief-Rishab

@ravisuhag @AkarshSatija @mabdh @bsushmith @singhvikash11 need your help to go through this rfc 🙏

rahmatrhd avatar Jul 07 '22 09:07 rahmatrhd

Draft PR which addresses this issues. https://github.com/odpf/guardian/pull/167

ravisuhag avatar Jul 07 '22 09:07 ravisuhag

@rahmatrhd I think option 2 is better to start with. we can have an API to import access which can be triggered manually and a corresponding cli command e.g.guardian provider import <provide-name> -r <resources>

Flag to identify whether it is imported or not can be generalized. maybe something like. grant: import/policy. There can be a better name for it.

ravisuhag avatar Jul 07 '22 10:07 ravisuhag

@Chief-Rishab I think adding no policies or no approvers associated with these appeals would not be ideal. There should be an admin page and the admin/approver could revoke this access from users later.

@rahmatrhd let's set up some time to discuss RFC, we can catch up and go through it.

singhvikash11 avatar Jul 08 '22 08:07 singhvikash11

There should be an admin page and the admin/approver could revoke this access from users later.

@singhvikash11 currently anyone with the revoke API access can revoke any appeals, it's not depending on the policy. Apart from that, a policy is being used to determine the approvers, while those imported access are already active access, so we no longer have to set the approvers.

rahmatrhd avatar Jul 11 '22 03:07 rahmatrhd

found an edge case: since provider resources are not synced to guardian in real time, when importing existing access from provider, there might be a chance it contains access from newly created resources that haven't been synced to guardian yet. Those access can't be added to guardian unless we also add the new resources to the guardian which will add a side-effect to import access api and makes it not clean (first option). Or the easiest one is, to ignore the access from resources that haven't been synced to guardian. cc @ravisuhag @bsushmith @singhvikash11 @Chief-Rishab

rahmatrhd avatar Jul 29 '22 05:07 rahmatrhd

Can we make a cron job/API to regularly fetch resources first rather than ignoring resources not synced with Guardian.? That would increase the scope for drift management as well but will be good for the end user

Chief-Rishab avatar Jul 29 '22 05:07 Chief-Rishab

@rahmatrhd As part of import access, we should ignore the access of resources that are not registered in Guardian. The drift of resources can be handled separately.

ravisuhag avatar Jul 29 '22 05:07 ravisuhag

Have thought of merging this import access with the existing FetchResources process since for import access we are doing it for every resource in the provider. We can rename FetchResources to Sync and what happen in that is we are syncing the resources and the access. In this case, since fetch resources already happen through a job, then option 1 and 2 will be implemented altogether. The API that initially only for import access could become /providers/:id/sync (still can have option to sync resources only or access only)

Reason I come with this proposal is, when fetching access from provider we are also fetching the resources again to get the access entries for each. And I think it could be a signal that these process can be merged together

rahmatrhd avatar Aug 04 '22 04:08 rahmatrhd

as discussed, we will keep both processes decoupled until we see better use cases or requirements for that.

Import Access will still be done through an API but user can add filter for resources: GET /providers/:id/import-access?resource_types=t1&resource_types=t2&resource_urns=r1. Or if user wants to import access for all resources query ?all=true also can be added.

The function signature for ImportAccess in the provider client would be like this: ImportAccess(*domain.ProviderConfig, []*domain.Resource) error (not final) to give flexibility for user to filter the resources and future use cases

rahmatrhd avatar Aug 04 '22 07:08 rahmatrhd

In addition to this, at provider level provider will provide a method that takes resources as input and import access for the provided resources.

ravisuhag avatar Aug 21 '22 17:08 ravisuhag