Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Import pre-existing access from providers #206

Closed
Chief-Rishab opened this issue Jul 7, 2022 · 11 comments · Fixed by #247 · May be fixed by #167
Closed

Import pre-existing access from providers #206

Chief-Rishab opened this issue Jul 7, 2022 · 11 comments · Fixed by #247 · May be fixed by #167
Assignees
Labels

Comments

@Chief-Rishab
Copy link
Member

Chief-Rishab commented Jul 7, 2022

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"]
@rahmatrhd
Copy link
Member

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

@ravisuhag
Copy link
Member

Draft PR which addresses this issues. #167

@ravisuhag
Copy link
Member

ravisuhag commented Jul 7, 2022

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

@singhvikash11
Copy link
Member

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

@rahmatrhd
Copy link
Member

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 rahmatrhd self-assigned this Jul 14, 2022
@rahmatrhd
Copy link
Member

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

@Chief-Rishab
Copy link
Member Author

Chief-Rishab commented Jul 29, 2022

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

@ravisuhag
Copy link
Member

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

@rahmatrhd
Copy link
Member

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
Copy link
Member

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

@ravisuhag
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 2023
4 participants