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.

Sign up for GitHub

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

Jump to bottom

Add the ability to disable local authentication #10102

Merged
merged 7 commits into from
May 27, 2021
Merged

Add the ability to disable local authentication #10102

merged 7 commits into from
May 27, 2021

Conversation

jbradberry
Copy link
Contributor

@jbradberry jbradberry commented May 4, 2021 •
edited
Loading

SUMMARY

When an external authentication system is enabled, users would like the ability to disable local authentication for enhanced security.

related #4553

TODO
  • create a configure-Tower-in-Tower setting, DISABLE_LOCAL_AUTH
  • expose the setting in the settings UI
  • be able to query out all local-only users
    • User.objects.filter(Q(profile__isnull=True) | Q(profile__ldap_dn=''), enterprise_auth__isnull=True, social_auth__isnull=True)
    • see: awx/main/utils/common.py, get_external_account
  • write a thin wrapper around the Django model-based auth backend
  • update the UI tests to include the new setting
  • be able to trigger a side-effect when this setting changes
  • revoke all OAuth2 tokens for users that do not have a remote
    auth backend associated with them
  • revoke sessions for local-only users
    • ultimately I did this by adding a new middleware that checks the value of this new setting and force-logouts any local-only user making a request after it is enabled
  • settings API endpoint raises a validation error if there are no external users or auth sources configured
    • The remote user existence validation has been removed, since ultimately we can't know for sure if a sysadmin-level user will still have access to the UI. This is being dealt with by using a confirmation modal, see below.
  • add a modal asking the user to confirm that they want to turn this setting on
ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME
  • API
  • UI
AWX VERSION

@jbradberry jbradberry self-assigned this May 4, 2021
@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

@AlexSCorey AlexSCorey self-requested a review May 7, 2021 20:19
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@jbradberry jbradberry changed the title [WIP] Add the ability to disable local authentication Add the ability to disable local authentication May 11, 2021
@jbradberry jbradberry marked this pull request as ready for review May 11, 2021 14:51
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@jbradberry
Copy link
Contributor Author

jbradberry commented May 12, 2021 •
edited
Loading

Note that it is quite possible even with the setting validation to wind up in a state where all users are locked out. In that case, opening up the shell with awx-manage shell_plus and doing settings.DISABLE_LOCAL_AUTH = False does put things back into allowing local logins again.

@one-t
Copy link
Contributor

one-t commented May 12, 2021

A couple things so far-

  • We should probably add a warning to this help text about the logout implications -
    image
  • I am not able to enable this option with an LDAP Directory configured
    image

@gamuniz
Copy link
Contributor

gamuniz commented May 13, 2021

@one-t are you able to provide the ldap configuration? There shouldn't be any ldap user on the system that doesn't have an ldap_dn associated with it.

jbradberry
jbradberry commented May 13, 2021
View reviewed changes
awx/api/conf.py Outdated
if attrs.get('DISABLE_LOCAL_AUTH', False):
if not any(getattr(settings, s, None) for s in remote_auth_settings):
raise serializers.ValidationError(_("There are no remote authentication systems configured."))
if not User.objects.exclude(profile__ldap_dn='').exclude(enterprise_auth__isnull=True).exclude(social_auth__isnull=True).exists():
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@one-t for a bit more context, the check is here. This should account for every previously logged-in LDAP user.

@one-t
Copy link
Contributor

one-t commented May 13, 2021

This is the LDAP config:

"AUTH_LDAP_SERVER_URI": "ldap://my.ldap.server.redacted",
    "AUTH_LDAP_BIND_DN": "uid=myuser,cn=users,cn=accounts,dc=redacted,dc=domain,dc=com",
    "AUTH_LDAP_BIND_PASSWORD": "$encrypted$",
    "AUTH_LDAP_START_TLS": false,
    "AUTH_LDAP_CONNECTION_OPTIONS": {
        "OPT_REFERRALS": 0,
        "OPT_NETWORK_TIMEOUT": 30
    },
    "AUTH_LDAP_USER_SEARCH": [
        "cn=users,cn=accounts,dc=redacted,dc=domain,dc=com",
        "SCOPE_SUBTREE",
        "(uid=%(user)s)"
    ],
    "AUTH_LDAP_USER_DN_TEMPLATE": null,
    "AUTH_LDAP_USER_ATTR_MAP": {
        "first_name": "givenName",
        "last_name": "sn",
        "email": "mail"
    },
    "AUTH_LDAP_GROUP_SEARCH": [
        "cn=groups,cn=accounts,dc=redacted,dc=domain,dc=com",
        "SCOPE_SUBTREE",
        "(objectClass=posixGroup)"
    ],
    "AUTH_LDAP_GROUP_TYPE": "MemberDNGroupType",
    "AUTH_LDAP_GROUP_TYPE_PARAMS": {
        "name_attr": "cn",
        "member_attr": "member"
    },
    "AUTH_LDAP_REQUIRE_GROUP": null,
    "AUTH_LDAP_DENY_GROUP": null,
    "AUTH_LDAP_USER_FLAGS_BY_GROUP": {},
    "AUTH_LDAP_ORGANIZATION_MAP": {},
    "AUTH_LDAP_TEAM_MAP": {},

@one-t
Copy link
Contributor

one-t commented May 13, 2021

The user does have a DN associated:

{
    "id": 12,
    "type": "user",
    "url": "/api/v2/users/12/",
    "related": {
        "named_url": "/api/v2/users/sarcher/",
        "teams": "/api/v2/users/12/teams/",
        "organizations": "/api/v2/users/12/organizations/",
        "admin_of_organizations": "/api/v2/users/12/admin_of_organizations/",
        "projects": "/api/v2/users/12/projects/",
        "credentials": "/api/v2/users/12/credentials/",
        "roles": "/api/v2/users/12/roles/",
        "activity_stream": "/api/v2/users/12/activity_stream/",
        "access_list": "/api/v2/users/12/access_list/",
        "tokens": "/api/v2/users/12/tokens/",
        "authorized_tokens": "/api/v2/users/12/authorized_tokens/",
        "personal_tokens": "/api/v2/users/12/personal_tokens/"
    },
    "summary_fields": {
        "user_capabilities": {
            "edit": true,
            "delete": true
        }
    },
    "created": "2021-05-12T21:52:36.582671Z",
    "username": "sarcher",
    "first_name": "Sterling",
    "last_name": "Archer",
    "email": "sarcher@redacted.domain.com",
    "is_superuser": true,
    "is_system_auditor": false,
    "ldap_dn": "uid=sarcher,cn=users,cn=accounts,dc=redacted,dc=domain,dc=com",
    "last_login": "2021-05-12T21:52:36.831583Z",
    "external_account": "ldap",
    "auth": []
}

@jbradberry
Copy link
Contributor Author

🤦 I think I see what I did wrong...

@jbradberry
Add a new setting, DISABLE_LOCAL_AUTH
26b7e9d 
and expose it in the settings UI.
@jbradberry
Write a thin wrapper around the standard Django auth backend
5c664ea
@jbradberry
Add a signal handler to invalidate sessions and tokens for local users
9e7f004 
when this setting gets turned on.
@jbradberry
Add a new middleware to force-logout local-only users
81de931 
when the DISABLE_LOCAL_AUTH setting is set.  This avoids the ugliness
of getting a SuspiciousOperation error for any request/response cycles
that are in flight when a user gets bounced.
@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@one-t
Copy link
Contributor

one-t commented May 13, 2021

I confirmed that this last update fixed the issue, the feature can now be enabled when LDAP is the only provider configured.

@jbradberry
Copy link
Contributor Author

@wenottingham @mabashian We need to address the issue of whether and how to do the user validation when toggling this setting on. The options, as I understand them, are:

  1. keep it as-is; there needs to be at least one remote user who has previously logged in, but they do not need to have been a sysadmin-level user
  2. validate that there is at least one remote sysadmin-level user that has previously logged in
  3. do no validation of the existence of remote user accounts, but throw up a confirmation modal
  4. validate the existence of at least one remote account, throw up a confirmation modal
  5. validate the existence of at least one remote sysadmin-level account, throw up a confirmation modal

During the demo questioning, somebody said something that I didn't quite catch about some types of remote accounts not being checkible for sysadmin privileges? Perhaps I misheard, but I'd like to get some clarification about that.

@wenottingham
Copy link
Contributor

During the demo questioning, somebody said something that I didn't quite catch about some types of remote accounts not being checkible for sysadmin privileges? Perhaps I misheard, but I'd like to get some clarification about that.

Certain identity providers map users into roles (and teams) by attributes. Not all remote user sources allow mapping sysadmin, and while we could in theory check that a mapping is defined for some, we can't check that it would ever actually fire.

I'm actually leaning towards 3, then 1, as the preferred options.

@AlexSCorey
Copy link
Member

I believe that we need some sort of confirmation modal when disabling local authentication. This feature allows the user to take some pretty drastic actions that could be catastrophic we need to have some sort of modal that tells them what it means to disable local auth, and ask them to confirm that they want to do that. Perhaps @trahman73 can chime in here.

@trahman73
Copy link

@AlexSCorey I'm in favor of having a confirmation modal as suggested by Jeff's Option 3.

@jbradberry
Copy link
Contributor Author

@mabashian @trahman73 who from the UI team is going to take responsibility for it? According to the spreadsheet, there is no UI dev assigned to this issue.

@AlexSCorey AlexSCorey self-assigned this May 18, 2021
@AlexSCorey
Copy link
Member

AlexSCorey commented May 18, 2021 •
edited
Loading

@jbradberry I've assigned myself to this issue for the modal. Is merge being blocked by that UI modal?

@jbradberry
Copy link
Contributor Author

@AlexSCorey depends on how we want to play it. I could take out the user existence validation (since we are trying to do number 3) and merge, and we could do the UI part as a follow-on PR. Or we could just add your commits here. Either way works for me.

@jbradberry
Remove the remote user existence validation
2aa3fe7 
since we are going to do a confirmation modal dialog instead.
@jbradberry
Copy link
Contributor Author

The remote user existence validation has been removed.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded.

@one-t one-t self-assigned this May 20, 2021
@one-t
Copy link
Contributor

one-t commented May 24, 2021

A couple things that have come to mind while testing this:

Do we want to actually disable the local superuser accounts? The original issue mentions leaving local admin accounts available for backup/restore processes.

As implemented, this makes it impossible to use oauth2 token-based authentication with AWX, since we disable the creation of oauth2 tokens for external accounts (which would be the only type of valid account with this feature enabled). Giving these tokens to superusers isn't a great solution either but we don't really have a concept of a dedicated Service Account in AWX.

One solution might be to add an option to remove the ability for Org Admins to create user accounts. That gets a bit deeper into the RBAC forest but since this option can be disabled and enabled inside awx by a superuser, it doesn't really change the privilege equation.

@wenottingham
Copy link
Contributor

The general request by customers is to disable all local accounts. I'd suspect whatever warning in the docs should note that the user may need to enable tokens for social auth users.

@softwarefactory-project-zuul
Copy link
Contributor

Build succeeded (gate pipeline).

@softwarefactory-project-zuul softwarefactory-project-zuul bot merged commit c29a7cc into ansible:devel May 27, 2021
@jbradberry jbradberry deleted the disable-local-users branch May 27, 2021 18:50
@renovate renovate bot mentioned this pull request Jun 1, 2021
Merged
1 task
@shanemcd shanemcd mentioned this pull request Dec 14, 2021
Closed
@SignFinder
Copy link

Hi team,
Might it is a good idea to have checkbox to enable - disable login and password input fields? They are not necessary when OIDC\SAML using, because there is special button below for login

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

@chrismeyersfsu chrismeyersfsu chrismeyersfsu approved these changes

@mabashian mabashian mabashian approved these changes

@beeankha beeankha beeankha approved these changes

@one-t one-t one-t approved these changes

@AlexSCorey AlexSCorey Awaiting requested review from AlexSCorey

Assignees

@jbradberry jbradberry

@one-t one-t

@AlexSCorey AlexSCorey

Labels
component:api component:ui type:enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.