IdM Server: list all Employees with matching Smart Card #516

flo-renaud · 2017-02-28T09:49:18Z

Implement a new IPA command allowing to retrieve the list of users matching the provided certificate.
The command is using SSSD Dbus interface, thus including users from IPA domain and from trusted domains. This requires sssd-dbus package to be installed on IPA server.

https://fedorahosted.org/freeipa/ticket/6646

flo-renaud · 2017-02-28T09:51:01Z

Note: this PR is work in progress. It requires PR#398 Support for Certificate Identity Mapping and sssd patches not pushed yet.

abbra · 2017-02-28T10:34:52Z

One thing I don't like is that SELinux policy requirements aren't mentioned. To allow ipaapi user to talk to SSSD dbus interface, you have to have a policy that allows this.

simo5 · 2017-02-28T17:58:41Z

Why do we need to talk to SSSD to do this?
Don't we have all the needed data in LDAP already ?

flo-renaud · 2017-03-01T07:28:23Z

Hi @simo5
The command must also be able to return matching entries coming from trusted domains, and SSSD is able to handle this part for us.

HonzaCholasta · 2017-03-01T08:11:45Z

ipaserver/plugins/certmap.py

+        result['count'] = len(users)
+        result['uids'] = users
+        result['summary'] = u'{count} user{plural} matched'.format(
+            count=len(users), plural='' if len(users) == 1 else 's')


This is not translatable. Instead of filling in summary yourself, you should specify the msg_summary class attribute and have it filled automatically.

HonzaCholasta · 2017-03-01T08:21:30Z

ipaserver/plugins/certmap.py

+        output.summary,
+        output.Output('uids', (list, tuple, type(None)), _('Matched uid')),
+        output.Output('count', int, _('Number of entries returned')),
+        )


Please avoid using custom has_output, as it requires a client-side plugin. Define certmap_match as a search method of a virtual certmap object instead:

from ipalib.crud import Search from ipalib.frontend import Object @register() class certmap(Object): has_params = ( Str( 'uid', ... flags={'no_search'}, ), Bytes( 'certificate', ... ), ) @register() class certmap_match(Search): def get_args(self): for arg in super(certmap_match, self).get_args(): if arg.name == 'criteria': continue yield arg def execute(self, *args, **options): ... result['result'] = [{'uid': user} for user in users] result['count'] = len(users) result['truncated'] = False return result

Also, I don't think the result should be called uid, as that's usually the username without the domain qualifier. You can either rename it or split it into uid and domain (you can do that with the object plugin).

Actually, I think it would make sense to group the results by domain, in which case certmap should look like this:

@register() class certmap(Object): has_params = ( DNSNameParam( 'domain', ... flags={'no_search'}, ), Bytes( 'certificate', ... ), Str( 'uid*', ... flags={'no_search'}, ), )

HonzaCholasta · 2017-03-01T11:05:41Z

ipaserver/plugins/certmap.py

+
+    takes_options = (
+        Bytes(
+            'certificate', validate_certificate,


I guess this should be a positional argument, since it is the only argument and is required.

@HonzaCholasta it would be good to catch these issues in threads like http://www.redhat.com/archives/freeipa-devel/2017-February/msg00878.html which are meant especially for it to avoid rewriting patches.

simo5 · 2017-03-01T18:08:58Z

I am not sure we want to wait for replies from trusted domains, it may be very slow, and in some cases it will just not work right (one way trusts with strict access control on entries).
Active Directory forces users to provide a hint when logging into trusted domains with smart cards and does not query the remote domain. Have we considered this ?

sumit-bose · 2017-03-01T19:18:02Z

Yes, a hint aka user name will be used during authentication. But this PR here is about to get an idea which user is allowed to authenticate based on the current certificate mapping configuration. Since the certificate mapping configuration requires remote domains to be added explicitly to admin can control which domains are included in the search.

flo-renaud · 2017-03-02T12:35:01Z

@abbra ,
Thanks for your comment. Running in permissive mode I did not see any AVC logged in the journal.

@HonzaCholasta
thanks for the tips re. writing API. I have followed your advice and made certificate a positional argument. The output will look like this:

---------------
2 users matched
---------------
  Domain: DOMAIN.EXAMPLE.COM
  Usernames: user1, user2
----------------------------
Number of entries returned 2
----------------------------

HonzaCholasta · 2017-03-06T14:01:10Z

ipaserver/plugins/certmap.py

+
+        cert = args[0]
+        users = sssd.list_users_by_cert(cert)
+        count = sum([len(l) for (_k, l) in users.items()])


count should be the number of entries in the result, i.e. len(result).

If you want to keep the "N users matched" summary intact, you can do it using a get_summary_default method:

def get_summary_default(self, output): count = sum(len(entry['uid']) for entry in output['result']) return self.msg_summary % dict(count=count)

I see you added get_summary_default, but it seems you forgot to update the count line to count = len(results).

HonzaCholasta · 2017-03-06T14:02:29Z

ipaserver/plugins/certmap.py

+        ),
+        Str(
+            'uid*',
+            label=_('Usernames'),


In the user plugin, uid is called "Login" - perhaps we should use the same (singular or plural) here as well?

HonzaCholasta · 2017-03-06T14:03:38Z

@flo-renaud, please rebase.

flo-renaud · 2017-03-06T18:33:41Z

Hi @HonzaCholasta
thank you for your comments. Patch rebased.

flo-renaud · 2017-03-07T08:32:08Z

Hi @HonzaCholasta
sorry I overlooked the change for count. It's updated now, thank you for the review.

HonzaCholasta · 2017-03-07T09:00:24Z

@flo-renaud, thanks, LGTM.

BTW Travis fails because there is no sssd-dbus >= 1.15.1 - submitting a build to freeipa-master now.

ghost · 2017-03-07T16:29:58Z

@flo-renaud While playing with this command I've noticed one disturbing fact. Because we rely on SSSD and SSSD rely its cache we will likely return inaccurate result.
I'm thinking about use-case when admin calls certmap-match to list current users mapped to the certificate. Then he performs some changes and calls certmap-match again to verify his changes. At that point SSSD may use cache and return obsolete result.
One possible solution would be expiring the cache on every certmap-match call but that can easily have serious performance impact.

flo-renaud · 2017-03-07T16:33:59Z

Hi @dkupka
As the goal of this command is to return exactly the same list of users as SSSD would consider for authentication, IMHO it is expected that we may have a cached list instead of an up-to-date list of results, because sssd authentication would have the same result.

ghost · 2017-03-07T16:37:22Z

@flo-renaud That's right but we should probably stress this somehow because it's not intuitive. Also we're returning what SSSD would return on master but we have no idea what it will return on some other host.

sumit-bose · 2017-03-08T08:21:38Z

I agree, it would be good if the help text can mention that cached data is used and maybe even mention the sss_cache utility to invalidate the entry. If the doc team can add this to the official documentation it would be even better.

ghost · 2017-03-08T08:55:20Z

@sumit-bose I agree. If this is in help text we can also display it in WebUI.
@flo-renaud Please add description and explanation of this behaviour into doc for certmap_match. Otherwise the pull request looks good to me and works as expected.

Implement a new IPA command allowing to retrieve the list of users matching the provided certificate. The command is using SSSD Dbus interface, thus including users from IPA domain and from trusted domains. This requires sssd-dbus package to be installed on IPA server. https://fedorahosted.org/freeipa/ticket/6646

flo-renaud · 2017-03-08T12:45:46Z

@dkupka
I added the following explanation in the doc for certmap_match:
"""
Search for users matching the provided certificate.

This command relies on SSSD to retrieve the list of matching users and
may return cached data. For more information on purging SSSD cache,
please refer to sss_cache documentation.
"""

ghost · 2017-03-08T14:08:27Z

@flo-renaud Thank you.

ghost · 2017-03-08T14:09:03Z

master:

ea34e17 IdM Server: list all Employees with matching Smart Card

HonzaCholasta · 2017-03-09T06:42:20Z

I forgot to say that in the CLI, the certificate should be specified using a file. PR #557 implements this.

HonzaCholasta suggested changes Mar 1, 2017

View reviewed changes

HonzaCholasta reviewed Mar 1, 2017

View reviewed changes

flo-renaud force-pushed the t6646 branch from f078584 to 5b6fd5e Compare March 2, 2017 11:31

HonzaCholasta reviewed Mar 6, 2017

View reviewed changes

flo-renaud force-pushed the t6646 branch from 5b6fd5e to b3ec9b9 Compare March 6, 2017 18:30

flo-renaud force-pushed the t6646 branch from b3ec9b9 to 409dbe5 Compare March 7, 2017 08:30

HonzaCholasta approved these changes Mar 7, 2017

View reviewed changes

pvomacka mentioned this pull request Mar 7, 2017

WebUI: certmap match #549

Closed

flo-renaud force-pushed the t6646 branch from 409dbe5 to 18fd166 Compare March 8, 2017 12:44

ghost added the ack Pull Request approved, can be merged label Mar 8, 2017

ghost added the pushed Pull Request has already been pushed label Mar 8, 2017

ghost closed this Mar 8, 2017

flo-renaud deleted the t6646 branch March 14, 2017 07:41

This pull request was closed.

IdM Server: list all Employees with matching Smart Card #516

IdM Server: list all Employees with matching Smart Card #516

flo-renaud commented Feb 28, 2017 •

edited by ghost

flo-renaud commented Feb 28, 2017

abbra commented Feb 28, 2017

simo5 commented Feb 28, 2017

flo-renaud commented Mar 1, 2017

HonzaCholasta Mar 1, 2017

HonzaCholasta Mar 1, 2017

HonzaCholasta Mar 1, 2017

HonzaCholasta Mar 1, 2017

pvoborni Mar 2, 2017

simo5 commented Mar 1, 2017

sumit-bose commented Mar 1, 2017

flo-renaud commented Mar 2, 2017 •

edited

HonzaCholasta Mar 6, 2017

HonzaCholasta Mar 7, 2017

HonzaCholasta Mar 6, 2017

HonzaCholasta commented Mar 6, 2017

flo-renaud commented Mar 6, 2017

flo-renaud commented Mar 7, 2017

HonzaCholasta commented Mar 7, 2017

ghost commented Mar 7, 2017

flo-renaud commented Mar 7, 2017

ghost commented Mar 7, 2017

sumit-bose commented Mar 8, 2017

ghost commented Mar 8, 2017

flo-renaud commented Mar 8, 2017

ghost commented Mar 8, 2017

ghost commented Mar 8, 2017

HonzaCholasta commented Mar 9, 2017

IdM Server: list all Employees with matching Smart Card #516

IdM Server: list all Employees with matching Smart Card #516

Conversation

flo-renaud commented Feb 28, 2017 • edited by ghost

flo-renaud commented Feb 28, 2017

abbra commented Feb 28, 2017

simo5 commented Feb 28, 2017

flo-renaud commented Mar 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simo5 commented Mar 1, 2017

sumit-bose commented Mar 1, 2017

flo-renaud commented Mar 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HonzaCholasta commented Mar 6, 2017

flo-renaud commented Mar 6, 2017

flo-renaud commented Mar 7, 2017

HonzaCholasta commented Mar 7, 2017

ghost commented Mar 7, 2017

flo-renaud commented Mar 7, 2017

ghost commented Mar 7, 2017

sumit-bose commented Mar 8, 2017

ghost commented Mar 8, 2017

flo-renaud commented Mar 8, 2017

ghost commented Mar 8, 2017

ghost commented Mar 8, 2017

HonzaCholasta commented Mar 9, 2017

flo-renaud commented Feb 28, 2017 •

edited by ghost

flo-renaud commented Mar 2, 2017 •

edited