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

replica: Ensure that ipaapi user is allowed to access ifp #4914

Closed
wants to merge 2 commits into from
Closed

replica: Ensure that ipaapi user is allowed to access ifp #4914

wants to merge 2 commits into from

Conversation

jsf9k
Copy link
Contributor

@jsf9k jsf9k commented Jul 12, 2020

ipa-server-install executes ipa-client-install with the --on-master flag set, which causes the ipaclient.install.client.sssd_enable_ifp() function to be called. This function configures sssd so that the ipaapi user is allowed to access ifp. Any FreeIPA replica should also have sssd configured like this, but in that case we cannot simply pass the --on-master flag to ipa-client-install because it has other side effects. The solution is to call the ipaclient.install.client.sssd_enable_ifp() function from inside the ipaserver.install.server.replicainstall.promote_sssd() function.

https://pagure.io/freeipa/issue/8403

ipa-server-install executes ipa-client-install with the --on-master
flag set, which causes the ipaclient.install.client.sssd_enable_ifp()
function to be called.  This function configures sssd so that the
ipaapi user is allowed to access ifp.  Any FreeIPA replica should also
have sssd configured like this, but in that case we cannot simply pass
the --on-master flag to ipa-client-install because it has other side
effects.  The solution is to call the
ipaclient.install.client.sssd_enable_ifp() function from inside the
ipaserver.install.server.replicainstall.promote_sssd() function.

https://pagure.io/freeipa/issue/8403
@tiran tiran added ipa-4-8 Mark for backport to ipa 4.8 re-run Trigger a new run of PR-CI labels Jul 13, 2020
Copy link
Member

@tiran tiran left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Rob, can you look into a test case for the issue?

@freeipa-pr-ci freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Jul 13, 2020
@tiran tiran added the re-run Trigger a new run of PR-CI label Jul 13, 2020
@freeipa-pr-ci freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Jul 13, 2020
@jsf9k
Copy link
Contributor Author

jsf9k commented Jul 13, 2020

I'm happy to try and create a test case if someone can get me pointed in the right direction.

@rcritten
Copy link
Contributor

It would be one or more integration tests. They live in ipatests/test_integration.

Considering we're talking about replica installations test_replica_promotion.py is a candidate for at least one test. I think that the TestReplicaPromotionLevel1 could add a new test function to fetch sssd.conf via:

contents = host.get_file_contents(paths.SSSD_CONF, encoding='utf-8')

and examine the results to ensure that the ifp settings were done.

TestUnprivilegedUserPermissions could be used to do the same ifp check as this is a client -> server promotion.

In fact, the ifp test could be some generic function that given a host fetch the file, load using SSSDConfig and verify the ifp settings.

@rcritten rcritten added the re-run Trigger a new run of PR-CI label Jul 13, 2020
@freeipa-pr-ci freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Jul 13, 2020
@@ -47,6 +48,32 @@ def test_kra_install_master(self):
assert(found > 0), result2.stdout_text


def test_sssd_config_allows_ipaapi_access_to_ifp(host):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you drop the test_ prefix? It makes it look like this is a test rather than a standalone function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this is addressed in commit adc5e44.

# Verify that the allow_uids item's value contains ipaapi
uids = [uid.strip() for uid in ifp_items_dict['allowed_uids'].split(',')]
assert 'ipaapi' in uids

Copy link
Contributor

Choose a reason for hiding this comment

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

Only two blank lines are necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this is addressed in commit adc5e44.

'''
contents = host.get_file_contents(paths.SSSD_CONF, encoding='utf-8')
# Parse the sssd config as a config file
config = ConfigParser()
Copy link
Contributor

@rcritten rcritten Jul 13, 2020

Choose a reason for hiding this comment

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

Strictly speaking, yes, the config is an INI-style configuration file. There is also an SSSD python API and that may be a better long-term solution. Maybe something approaching:

        sssdconfig = SSSDConfig.SSSDConfig()
        sssdconfig.import_config()

        ifp = sssdconfig.get_service('ifp')

        uids = ifp.get_option('allowed_uids').split(',')
        assert 'ipaapi' in uids

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this is addressed in commit adc5e44.

@rcritten rcritten added the re-run Trigger a new run of PR-CI label Jul 14, 2020
@rcritten
Copy link
Contributor

The PR looks outstanding, thanks for the contribution. Triggering full CI. I don't see any other issues. Kudos on finding remote_sssd_config(), I had forgotten about that.

@freeipa-pr-ci freeipa-pr-ci removed the re-run Trigger a new run of PR-CI label Jul 14, 2020
@jsf9k
Copy link
Contributor Author

jsf9k commented Jul 14, 2020

The PR looks outstanding, thanks for the contribution. Triggering full CI. I don't see any other issues. Kudos on finding remote_sssd_config(), I had forgotten about that.

Thanks @rcritten. I couldn't figure out how to use SSSDConfig() remotely at first either, which is why I used ConfigParser the first time around.

@rcritten rcritten added ack Pull Request approved, can be merged pushed Pull Request has already been pushed labels Jul 14, 2020
@rcritten
Copy link
Contributor

master:

  • 12529d7 replica: Ensure the ipaapi user is allowed to access ifp on replicas
  • 2ff1d6b replica: Add tests to ensure the ipaapi user is allowed access to ifp on replicas

@jsf9k
Copy link
Contributor Author

jsf9k commented Jul 14, 2020

Thanks again for all your help @rcritten!

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-8 Mark for backport to ipa 4.8 pushed Pull Request has already been pushed
Projects
None yet
4 participants