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

Sudorules ad users #4792

Closed
wants to merge 10 commits into from
Closed

Sudorules ad users #4792

wants to merge 10 commits into from

Conversation

abbra
Copy link
Contributor

@abbra abbra commented Jun 9, 2020

No description provided.

@tiran tiran self-requested a review June 9, 2020 07:40
@tiran tiran self-assigned this Jun 9, 2020
@tiran tiran added ipa-4-8 Mark for backport to ipa 4.8 needs review Pull Request is waiting for a review labels Jun 9, 2020
@abbra abbra added the re-run Trigger a new run of PR-CI label Jun 9, 2020
@freeipa-pr-ci freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Jun 9, 2020
@abbra
Copy link
Contributor Author

abbra commented Jun 9, 2020

Azure Pipelines failed because https://bodhi.fedoraproject.org/updates/FEDORA-2020-ca8855e4de is currently being pushed to Fedora 32 updates stable and is not accessible in updates testing anymore.

ipaserver/plugins/idviews.py Outdated Show resolved Hide resolved
ipaserver/plugins/idviews.py Outdated Show resolved Hide resolved
@abbra
Copy link
Contributor Author

abbra commented Jun 10, 2020

I had some progress on supporting AD groups via non-POSIX groups syntax in sudoers (%:<group name> in sudoUser field) but then I stumbled upon a bug in SSSD: SSSD/sssd#5199. Once the bug is fixed, we can update and merge this code.

@stale
Copy link

stale bot commented Sep 6, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Stale PR [Bot] label Sep 6, 2020
@abbra abbra removed the stale Stale PR [Bot] label Sep 7, 2020
@abbra
Copy link
Contributor Author

abbra commented Sep 7, 2020

this is not stale, it is contingent on the SSSD release which includes the fix.

@abbra
Copy link
Contributor Author

abbra commented Sep 17, 2020

SSSD 2.3.1 is out and contains the fix. Also, our images are updated to include SSSD 2.3.1. I'll rebase the PR to get this working.

@abbra abbra force-pushed the sudorules-ad-users branch 2 times, most recently from 2d4cc06 to 571a189 Compare September 18, 2020 13:42
@abbra
Copy link
Contributor Author

abbra commented Sep 18, 2020

I tested manually and the change works, including sudo actually accepting AD user in the rules. Somehow, the automated test still fails in PR CI and I need to investigate why that happens.

@abbra
Copy link
Contributor Author

abbra commented Sep 18, 2020

I think I figured out the issue -- the order of tests made sudo test to run within trust to AD with POSIX attributes expected to come from AD where they weren't, so resolving AD user was failing as it never had POSIX attributes.

@abbra
Copy link
Contributor Author

abbra commented Jan 25, 2021

Ok, fixed, will submit an update soon.

# ipa sudorule-add-user testrule --users={foobarz,administrator@win2016.test}
  Rule name: testrule
  Enabled: TRUE
  Host category: all
  External User: administrator@win2016.test
  Sudo Allow Commands: ALL
  Groups of RunAs Users: admins
  External Groups of RunAs Users: %domain admins@win2016.test
  Sudo Option: !authenticate
  Failed users/groups: 
    member user: foobarz: no such entry
    member group: 
-------------------------
Number of members added 1
-------------------------

…d failures

It was possible to add external members without any validation. Any
object that was not found in IPA LDAP was considered an external object
and a command such as sudorule could have added it to the list of values
for externalUser attribute.

With member validator support, real external members from trusted
domains can be differentiated from the objects that were not found in
IPA and in trusted domains.

Use information from the ID Views plugin to treat external objects
accordingly. Not found objects will be part of the error messaging
instead.

Fixes: https://pagure.io/freeipa/issue/3226
Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>
Register extended validator for users from trusted domains to be called
through add_external_pre_callback() in sudorules and other plugins.

The callbacks allow to validate user names as following:

 - if user name passes basic user name validator it is accepted, otherwise
 - if user name can be resolved to any user in IPA or in a trusted
   domain, it is accepted
 - otherwise the name is rejected

Fixes: https://pagure.io/freeipa/issue/3226
Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>
…omains directly

Allow specifying AD users and groups from trusted Active Directory
forests in `ipa sudorule-add/remove-user` family of commands.

SSSD uses single attribute 'externalUser' for IPA to pull 'external'
objects referenced in SUDO rules. This means both users and groups are
represented within the same attribute, with groups prefixed with '%',
as described in sudoers(5) man page.

Add member type validators to 'ipa sudorule-add/remove-user' family
commands and rely on member type validators from 'idviews' plugin to
resolve trusted objects.

Referencing fully qualified names for users and groups from trusted
Active Directory domains in 'externalUser' attribute of SUDO rules is
supported in SSSD 2.4 or later.

RN: IPA now supports adding users and groups from trusted Active
RN: Directory domains in SUDO rules without an intermediate non-POSIX
RN: group membership

Fixes: https://pagure.io/freeipa/issue/3226
Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>
@abbra
Copy link
Contributor Author

abbra commented Jan 25, 2021

On the other hand, this will break a valid use case where a local system user or group is added to SUDO rule globally. We have a test for this in test_sudo_rule_restricted_to_run_as_users_from_local_group.

Should we keep this logic or not?

@abbra
Copy link
Contributor Author

abbra commented Jan 25, 2021

I guess we have to keep the logic that adds whatever is provided to external attributes because otherwise we are totally broken for these use cases. A good chunk of tests in ipatests.test_xmlrpc.test_sudorule_plugin are testing this.

I'm going to refactor the check to allow an easier switch later but we'll keep those non-existing names permitted.

@abbra
Copy link
Contributor Author

abbra commented Jan 25, 2021

rpcclient issue is due to switch to Kerberos which requires us to actually use AD DC host name, not a domain name as a target or otherwise Kerberos service ticket cannot be acquired because Samba does not support domain-wide connection.

@flo-renaud
Copy link
Contributor

I guess we have to keep the logic that adds whatever is provided to external attributes because otherwise we are totally broken for these use cases. A good chunk of tests in ipatests.test_xmlrpc.test_sudorule_plugin are testing this.

I'm going to refactor the check to allow an easier switch later but we'll keep those non-existing names permitted.

Totally agree, I wasn't aware that we allowed non-existing names for local system users. Since it's a feature and not a bug we can keep the behavior.

@abbra abbra force-pushed the sudorules-ad-users branch 2 times, most recently from cb67415 to a031d73 Compare January 25, 2021 21:02
@abbra
Copy link
Contributor Author

abbra commented Jan 26, 2021

The latest changes fixed trust removal situation for all but one test. That one test is testing unreachable scenario. I think we need to restore the correct DNS zone before removing trust in it.

…rectly

Allow specifying AD users and groups from trusted Active Directory
forests in `ipa sudorule-add/remove-runasuser/runasgroup` family of
commands.

IPA provides 'ipasudorunasextuser' and 'ipasudorunasextusergroup' LDAP
attributes to record 'external' objects referenced in SUDO rules for
specifying the target user and group to run the commands allowed in the
SUDO rule.

Use member type validators to 'ipa sudorule-add/remove-runasuser/runasgroup'
family of commands and rely on member type validators from 'idviews'
plugin to resolve trusted objects.

Referencing fully qualified names for users and groups from trusted
Active Directory domains in IPA SUDOERs schema attributes is supported
in SSSD 2.4 or later.

RN: IPA now supports users and groups from trusted Active Directory
RN: domains in SUDO rules to specify runAsUser/runAsGroup properties
RN: without an intermediate non-POSIX group membership

Fixes: https://pagure.io/freeipa/issue/3226
Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>
Fixes: https://pagure.io/freeipa/issue/3226
Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>
Tests test_integration/test_trust.py::TestTrust::test_sudorules_ad_*
check that a user from a trusted AD domain can perform SUDO
authentication without a password for any command based on a direct user
reference or on indirect AD group reference. The test suite also ensures
an AD user and group can be used for runAsUser/runAsGroup settings.

Due to SSSD/sssd#5475 anything added to
'ipaSudoRunAsExtUserGroup' attribute will be prefixed with '%' and thus
any relying on the value of this attribute displayed by 'sudo -l'
command will fail. The test only validates that a proper group name
appears in the 'sudo' output, so we handle both prefixes in the
corresponding test check. It is not possible to differ by the SSSD
version as a fix to the issue is only a patch on top of 2.4.0 in RHEL.

Fixes: https://pagure.io/freeipa/issue/3226
Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>
@abbra abbra force-pushed the sudorules-ad-users branch 2 times, most recently from ec74da1 to d905229 Compare January 26, 2021 14:43
@flo-renaud
Copy link
Contributor

Hi @abbra
the latest changes LGTM. Let's wait for PRCI to complete successfully.

@abbra
Copy link
Contributor Author

abbra commented Jan 26, 2021

Samba 4.13+ in Fedora 33+ and RHEL 8.4+ defaults to Kerberos
authentication. This means user name used for authentication must be
mapped to a target realm.

We have to remove trust on AD side first before removing it locally or
otherwise MIT Kerberos might not be able to locate DCs from AD as
removal of the trust information would cause SSSD to clear the details
for a KDC locator plugin as well.

For the test that modifies AD DNS zone on IPA side to inject unreachable
DCs addresses, the configuration has to be reverted first, to allow
plain 'kinit' during removal of trust to reach AD DCs directly.

Fixes: https://pagure.io/freeipa/issue/8678
Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>
…ernal attr

IPA traditionally allowed to add names not found in IPA LDAP to external
attributes. This is used to allow, for example, a local system user or
group be present in a SUDO rule.

With membership validator, we can actually check validity of the names
against both IPA users/groups and users/groups from trusted domains.
If in future we decide to reject a local system's objects, then all it
would take is to switch reject_failures to True.

Fixes: https://pagure.io/freeipa/issue/3226
Signed-off-by: Alexander Bokovoy <abokovoy@redhat.com>
@abbra abbra removed the needs review Pull Request is waiting for a review label Jan 26, 2021
@flo-renaud flo-renaud added the ack Pull Request approved, can be merged label Jan 26, 2021
@rcritten rcritten added the pushed Pull Request has already been pushed label Jan 26, 2021
@rcritten
Copy link
Contributor

master:

  • afcb060 Add design document for using AD users/groups in SUDO rules
  • 172e4b9 baseldap: refactor validator support in add_external_pre_callback
  • 5fae809 baseldap: when adding external objects, differentiate between them and failures
  • 0ffdfc7 idviews: add extended validator for users from trusted domains
  • a37db29 sudorule-add-user: allow to reference users and groups from trusted domains directly
  • 349322e sudorule runAs: allow to add users and groups from trusted domains directly
  • 09e06e0 ipatests: fix test_sudorule_plugin's wrong argument use
  • 642b81e test_trust: add tests for using AD users and groups in SUDO rules
  • c91a1a0 ipatests: when talking to AD DCs, use FQDN credentials
  • 08d7209 baseldap: allow rejecting unknown objects instead of adding to an external attr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged ipa-4-9 Mark for backport to ipa 4.9 pushed Pull Request has already been pushed
Projects
None yet
5 participants