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

Config-as-code for roles and access controls #34718

Closed
1 of 2 tasks
hterik opened this issue Oct 2, 2023 · 8 comments
Closed
1 of 2 tasks

Config-as-code for roles and access controls #34718

hterik opened this issue Oct 2, 2023 · 8 comments
Labels
kind:feature Feature Requests needs-triage label for new issues that we didn't triage yet pending-response stale Stale PRs per the .github/workflows/stale.yml policy file

Comments

@hterik
Copy link
Contributor

hterik commented Oct 2, 2023

Description

We want to create custom roles in airflow that inherit the standard roles described on https://airflow.apache.org/docs/apache-airflow/2.7.1/security/access-control.html,

More specifically, we want a derivative of User that can not delete dags and that has access to cluster activity dashboard. Otherwise all the permissions from User+Viewer should be inherited. (Recommendation from page above is to not alter the existing roles, instead create new ones)

Creating new roles using the web ui is very tedious and error prone, without good review-process or git log history.
Moreover, our entire Airflow installation is based on config-as-code so nobody has the admin role in Airflow and can create new roles easily, without having to login as root under the hood.

Using configuration as code to create the roles would make this process a lot easier.
Proposal is a new python-function in airflow_local_settings.py, where one can read the AirflowSecurityManager.USER_PERMISSIONS programmatically to add/exclude permissions from the new returned role. These would take effect on next webserver restart.
I don't know if there is any requirements on history for removed permissions/roles that could cause problem if one simply adds/removes roles there?

As workaround i'm now considering a combo of airflow roles list -p -o json + processing + airflow roles import.
Is it possible to maybe just call the internal functions patch_role post_role? In that case when should one do that? Just once and then it will be persisted in DB?

Use case/motivation

No response

Related issues

No response

Are you willing to submit a PR?

  • Yes I am willing to submit a PR!

Code of Conduct

@hterik hterik added kind:feature Feature Requests needs-triage label for new issues that we didn't triage yet labels Oct 2, 2023
@troxil
Copy link

troxil commented Oct 8, 2023

We do this in webserver_config.py - our version (2.5.3) adds new roles to AirflowSecurityManager.ROLE_CONFIGS based on permissions : AirflowSecurityManager.VIEWER_PERMISSIONS + AirflowSecurityManager.USER_PERMISSIONS exactly as you describe.

    add_roles = ["abc", "def", "ghi"]
    AirflowSecurityManager.ROLE_CONFIGS.extend(
        [
            {"role": role, "perms": AirflowSecurityManager.VIEWER_PERMISSIONS + AirflowSecurityManager.USER_PERMISSIONS}
            for role in add_roles
        ]
    )

This is supported as much as we're using these primitives available to us.

FYI we use a custom SecurityManager (parent is of course AirflowSecurityManager) and override in flask's SECURITY_MANAGER_CLASS and set this new security managers ROLE_CONFIGS to be a union of the new configs (as above) + existing ones.

Hope this helps.

@RNHTTR
Copy link
Contributor

RNHTTR commented Oct 18, 2023

@hterik Does @troxil 's solution render this Issue moot?

@hterik
Copy link
Contributor Author

hterik commented Oct 18, 2023

Thanks a lot @troxil, this solution is a good step in the right direction.
I've found a few rough edges though.

  1. This should be documented on https://airflow.apache.org/docs/apache-airflow/2.7.1/security/access-control.html
    There is some info on https://airflow.apache.org/docs/apache-airflow/2.7.1/security/webserver.html#web-authentication, i think the first page should link to the second at least.

  2. Removing permissions from ROLE_CONFIG does not sync to the database. AirflowSecurityManager.bulk_sync_roles only creates missing permissions, it does not remove excessive permissions that no longer exist in ROLE_CONFIG.

  3. Modifying the default USER_PERMISSION when inheriting AirflowSecurityManager is not possible by just modifying the array, because the ROLE_CONFIGS is created in the class constructor. Only solution was copying or mutating the entire ROLE_CONFIG after it has been populated with USER_PERMISSION etc.
    I came up with the solution below to solve it.

Example:
(Note this is leaks some of the original permissions from USER_PERMISSION into Op becasue appending the lists happens in creation of original ROLE_CONFIGS)

RolePermission = tuple[str, str]

def mutate_role_configs(original_role_configs: list[dict[str, str | list[RolePermission]]],
                        rolename: str,
                        *,
                        adds: list[RolePermission],
                        removes: list[RolePermission]):
    new_role_configs = copy.deepcopy(original_role_configs)

    for rc in new_role_configs:
        if rc["role"] != rolename:
            continue
        for r in removes:
            rc["perms"].remove(r)
        rc["perms"].extend(adds)
        return new_role_configs

    raise Exception(f"Did not find role matching name {rolename}")


class MySecurityManager(AirflowSecurityManager):
    # USER_PERMISSIONS = ..... can not be set because ROLE_CONFIGS in parent class already created 

    ROLE_CONFIGS = mutate_role_configs(AirflowSecurityManager.ROLE_CONFIGS,
                                       "User",
                                       adds=[],
                                       removes=[
                                           (permissions.ACTION_CAN_DELETE, permissions.RESOURCE_DAG)
                                       ])


SECURITY_MANAGER_CLASS = MySecurityManager

@potiuk
Copy link
Member

potiuk commented Oct 18, 2023

To be honest - I think instead of making this as a code, you should look at the AIP-56 implementation that is well in progress. https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-56+Extensible+user+management.

We are moving away eventually from FAB authentication and replacing it with Auth Managers. It will require to upgrade to future versions of Airflow (2.8 possibly 2.9). but this is much more future-proof approach eventually.

Once this is implemented what you are trying to do is lilkely you should manage authentication outside of Airflow and rather than syncing it to FAB User/Roles you should write your own custom Auth Manager that will implement whatever access scheme you want there. It will be overall way simpler and will allow you to also manage more complex schemes - depending on your needs.

My advise would be that instead of trying to see how you can fit-in FAB syncing and having it done with FAB you should look (once we release it @hterik / @troxil ) into writing a custom Auth Manager and possibly being the first ones to try it out (and possibly let us improve what we will come up with).

Of course it's much more "future" and maybe you will choose to implement and contribute something to the current FAB manager (we will keep it around and you will still be able to use it and contribute to it), but once we release AIP-56 and prove that it works, I doubt any maintainer focus will be on doing something in FAB as it will be on its way out eventually.

cc: @vincbeck -> Vinc is looking for people who would like to (when ready) make use of the Auth Manager new APIs and provide feedback, so I think it would be a good idea you consider it and talk.

@potiuk
Copy link
Member

potiuk commented Oct 18, 2023

(BTW. And there is also a chance to contribute and give feedback even now - the work on the APIs is in progress so you might also be able to provide valuable feedback now). #35000 and #34924 are the two PRs currently opened.

@vincbeck
Copy link
Contributor

but once we release AIP-56 and prove that it works, I doubt any maintainer focus will be on doing something in FAB as it will be on its way out eventually.

Strongly on this one, sticking with FAB manager works for the short term but eventually, on the long run, we want to get rid of it. Happy to discuss and give you more details if needed

Copy link

github-actions bot commented Nov 2, 2023

This issue has been automatically marked as stale because it has been open for 14 days with no response from the author. It will be closed in next 7 days if no further activity occurs from the issue author.

@github-actions github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Nov 2, 2023
Copy link

github-actions bot commented Nov 9, 2023

This issue has been closed because it has not received response from the issue author.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:feature Feature Requests needs-triage label for new issues that we didn't triage yet pending-response stale Stale PRs per the .github/workflows/stale.yml policy file
Projects
None yet
Development

No branches or pull requests

5 participants