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

Authselect migration #1862

Closed
wants to merge 4 commits into from

Conversation

flo-renaud
Copy link
Contributor

@flo-renaud flo-renaud commented Apr 26, 2018

This PR replaces PR 1603.
It contains basically cosmetic changes (split into multiple commits), and the enhancements proposed in the comments.

@akokshar I hope this does not bother you but we needed to speed up the process in your absence.

@flo-renaud flo-renaud added prioritized Pull Request has higher priority for PR-CI needs review Pull Request is waiting for a review labels Apr 26, 2018
freeipa.spec.in Outdated
@@ -597,7 +597,7 @@ Requires: python2-sssdconfig
Requires: cyrus-sasl-gssapi%{?_isa}
Requires: chrony
Requires: krb5-workstation >= %{krb5_version}
Requires: authconfig
Requires: authselect >= 0.3.1
Copy link
Member

Choose a reason for hiding this comment

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

Please bump the version. The latest version is 0.4-2 with fix for https://bugzilla.redhat.com/show_bug.cgi?id=1571844


features = re.findall(r"-\s*(\S+)", cfg_params[0][1], re.DOTALL)

return (profile, features)
Copy link
Member

Choose a reason for hiding this comment

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

return profile, features


class RedHatAuthConfig(object):

class RedHatAuthTool(object):
Copy link
Member

Choose a reason for hiding this comment

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

IMHO, the class is over-engineered. A simple module-level function like get_auth_tool is a sufficient abstraction layer.

preconfigured_options = ['with-fingerprint']


def check_authselect_profile(host, expected_profile, expected_options=[]):
Copy link
Member

Choose a reason for hiding this comment

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

Mutable object in argument list, use () instead of [].

assert option in options


def apply_authselect_profile(host, profile, options=[]):
Copy link
Member

Choose a reason for hiding this comment

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

Another mutable object.

return None

cfg_params = re.findall(
r"\s*Profile ID:\s*(\S+)\s*\n\s*Enabled features:\s*(.*)",
Copy link
Member

Choose a reason for hiding this comment

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

The code assumes that the output of authselect won't change in the future. Did somebody talk to the author of the tool?

Copy link
Contributor

Choose a reason for hiding this comment

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

I added --raw parameter to authselect current in 0.4 (already in F28). This will print non-formatted output as it was given on command line with authselect select. I.e.:

$ authselect select sssd with-smartcard
$ authselect current --raw
sssd with-smartcard

You can use that instead.



@pytest.mark.skipif(
ipaplatform.NAME not in ['fedora', 'rhel', 'centos'],
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure that Debian/Ubuntu will never use authselect? I think it would be better to test for ipaplatform.paths.paths.AUTHSELECT is None instead of hard-coding platforms.

The authconfig tool is deprecated and replaced by authselect. Migrate
FreeIPA in order to use the new tool as described in the design page
https://www.freeipa.org/page/V4/Authselect_migration

Fixes:
https://pagure.io/freeipa/issue/7377
Add new test for client and server installation when authselect tool
is used instead of authconfig

Related to
https://pagure.io/freeipa/issue/7377
Commit d705320 was temporarily disabling authconfig backup and restore
because of issue 7478.
With the migration to authselect this is not needed any more

Related to
https://pagure.io/freeipa/issue/7377
ipa-advise config-client-for-smart-card-auth was producing a shell script
calling authconfig.
With the migration from authconfig to authselect, the script needs to
be updated and call authselect enable-feature with-smartcard instead.

Related to
https://pagure.io/freeipa/issue/7377
@flo-renaud
Copy link
Contributor Author

Hi @tiran,
thanks for your comments, I believe I addressed most of your concerns in this new push.
Regarding the output of authselect, the author was not available on irc this morning but I'll make sure to ask him if we can consider this as a stable interface.

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

authselect current --raw will be addressed on a follow-up PR.

@tiran tiran added ack Pull Request approved, can be merged and removed needs review Pull Request is waiting for a review labels Apr 27, 2018
@tiran
Copy link
Member

tiran commented Apr 27, 2018

master:

  • 1df1786 Migration from authconfig to authselect
  • c36bd38 New tests for authselect migration
  • e442464 Revert commit d705320
  • 8fe5f8d ipa-advise: adapt config-client-for-smart-card-auth to authselect

@tiran tiran added the pushed Pull Request has already been pushed label Apr 27, 2018
@tiran tiran closed this Apr 27, 2018
@flo-renaud flo-renaud deleted the authselect_migration branch May 15, 2018 15:40
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 prioritized Pull Request has higher priority for PR-CI pushed Pull Request has already been pushed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants