Provide centralized management of user short name resolution #573

martbab · 2017-03-13T12:13:43Z

This PR implement an initial version of AD user short name resolution
infrastructure consumable by SSSD.[1]

Most of the stuff described in the design page[2] is in-place except of hooks
that would refresh the domain resolution orders after trust domain removal or
disablement. I would like to do them in a separate PR.

Also some edge cases like specifying only separator (':') or an empty domain
('dom1::dom2') have no special treatment, the current code will just complain
about empty DNS labels. Should I improve this behavior?

[1] https://pagure.io/freeipa/issue/6372
[2] https://www.freeipa.org/page/V4/AD_User_Short_Names

MartinBasti · 2017-03-13T12:27:20Z

ipapython/ldapattrs.py

@@ -0,0 +1,135 @@
+#
+# Copyright (C) 2016 FreeIPA Contributors see COPYING for license


2017 ehm ehm

MartinBasti · 2017-03-13T12:30:28Z

ipapython/ldapattrs.py

+attributes in the framework
+"""
+
+from __future__ import print_function, unicode_literals


unicode_literals doesn't sound right to me

MartinBasti · 2017-03-13T12:33:09Z

ACIs? AFAIK SSSD should be able to read this

HonzaCholasta · 2017-03-13T12:39:37Z

I would rather avoid the refactoring in 4.5 - this is fragile code you are touching and I'm afraid it might break in some cases (think different client / server version combinations, thin client vs fat client, etc.).

As for the edge case values, IMO we should allow : without complaining as a special case to support "no domains in the list" configuration, and otherwise require known domain names (like in certmaprule-add).

martbab · 2017-03-13T12:44:48Z

Updated PR, added ACIs and fixed Py2/Py3 compatibility of doctests.

abbra · 2017-03-13T12:50:22Z

I don't see ACI.txt regenerated.

MartinBasti · 2017-03-13T12:50:19Z

ipaserver/plugins/idviews.py

@@ -94,7 +95,7 @@ class idview(LDAPObject):
    container_dn = api.env.container_views
    object_name = _('ID View')
    object_name_plural = _('ID Views')
-    object_class = ['ipaIDView', 'top']
+    object_class = ['ipaIDView', 'ipaNameResolutionData', 'top']


IMO ipaNameResolutionData must be in possible_object_class, because idviews after upgrade doesn't have it

Correct, it is auxilliary object class, so it should be in the possible_object_class and default_attributes should contain ipaNameResolutionOrder to be able to return the value if it is there.

pvoborni · 2017-03-13T17:28:46Z

ipaserver/plugins/config.py

+            'ipadomainresolutionorder?',
+            cli_name='domain_resolution_order',
+            label=_('Domain resolution order'),
+            doc=_('colon-separated list of domains used for short name'


nitpick: all other doc strings in this plugin start with capital letter

martbab · 2017-03-14T08:05:40Z

@HonzaCholasta I agree, I have removed the commit which introduces special param handling and resorted to simple splitting in validator. I have also regenerated ACIs in the respective commits.

HonzaCholasta · 2017-03-14T09:42:33Z

VERSION.m4

@@ -73,8 +73,8 @@ define(IPA_DATA_VERSION, 20100614120000)
 #                                                      #
 ########################################################
 define(IPA_API_VERSION_MAJOR, 2)
-define(IPA_API_VERSION_MINOR, 220)
-# Last change: Add whoami command
+define(IPA_API_VERSION_MINOR, 222)


220 + 1 != 222

HonzaCholasta · 2017-03-14T09:43:40Z

install/share/60basev3.ldif

@@ -84,5 +85,6 @@ objectClasses: (2.16.840.1.113730.3.8.12.24 NAME 'ipaPublicKeyObject' DESC 'Wrap
 objectClasses: (2.16.840.1.113730.3.8.12.25 NAME 'ipaPrivateKeyObject' DESC 'Wrapped private keys' SUP top AUXILIARY MUST ( ipaPrivateKey $ ipaWrappingKey $ ipaWrappingMech ) X-ORIGIN 'IPA v4.1' )
 objectClasses: (2.16.840.1.113730.3.8.12.26 NAME 'ipaSecretKeyObject' DESC 'Wrapped secret keys' SUP top AUXILIARY MUST ( ipaSecretKey $ ipaWrappingKey $ ipaWrappingMech ) X-ORIGIN 'IPA v4.1' )
 objectClasses: (2.16.840.1.113730.3.8.12.34 NAME 'ipaSecretKeyRefObject' DESC 'Indirect storage for encoded key material' SUP top AUXILIARY MUST ( ipaSecretKeyRef ) X-ORIGIN 'IPA v4.1' )
+objectClasses: (2.16.840.1.113730.3.8.12.39 NAME 'ipaNameResolutionData' DESC 'Data used to resolve short names to fully-qualified form' SUP top AUXILIARY MAY ( ipaDomainResolutionOrder ) X-ORIGIN 'IPA v4.5')


ipaNameResolutionConfig would make more sense to me (I know, I know).

I have already registered this OID and name so we are out of luck I think.

HonzaCholasta · 2017-03-14T09:45:16Z

ipaserver/plugins/config.py

+                }
+            )
+
+        return trusted_domains


It would be nice to share code with certmap.check_associateddomain_is_trusted. Not a blocker.

Nice catch, I will try to think about plugging the trustdomain retrieval magic there.

HonzaCholasta

Server upgrade is missing. See inline comments for some nitpicks. Otherwise LGTM.

martbab · 2017-03-14T10:46:30Z

Server upgrade consists only from adding the objectclass to ipaConfig which is taken care of in the update file. The idview object schema is modified on-demand when the attribute is set. Is there something else I need to take care of?

HonzaCholasta · 2017-03-14T11:12:55Z

IMO you should add the object class to all existing idviews on upgrade rather than add it on-demand.

MartinBasti · 2017-03-14T11:29:20Z

@HonzaCholasta it will break in case when idview entry is created on older replica, so it is more safe to appending the objectclass dynamically

HonzaCholasta · 2017-03-14T11:33:21Z

Ah, right.

Add ipaDomainResolutionOrder and ipaNameResolutionData to IPAv3 schema. Extend ipaConfig object with ipaNameResolutionData objectclass during update. https://pagure.io/freeipa/issue/6372

optional attribute was added to config object along with validator that check for valid domain names and also checks whether the specified domains exist in FreeIPA or in trusted forests and, in case of trusted domains, are not disabled. Part of http://www.freeipa.org/page/V4/AD_User_Short_Names https://pagure.io/freeipa/issue/6372

`idview-add` and `idview-mod` can now set and validate the attribute. The required objectclass is added on-demand after modification https://pagure.io/freeipa/issue/6372

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

martbab · 2017-03-14T15:18:59Z

PR rebased, I have fixed bugs in ID view objectclass handling and re-used the trusted domain retrieval code in certmap plugin. This is a separate commit so it can be removed if necessary.

I have noticed that with current PR we can not add the domain resolution order to Default Trust View, as it is protected from both modification and removal. @abbra is this expected also in this case?

abbra · 2017-03-14T15:24:37Z

Yes, it is expected too. Remember that 'Default Trust View' is a view that applies globally. You have already global setting to apply.

martbab · 2017-03-14T15:49:08Z

Ok thanks for explanation.

MartinBasti · 2017-03-14T17:37:35Z

master:

594c87d Short name resolution: introduce the required schema
1b5f56d ipaconfig: add the ability to manipulate domain resolution order
544d66b idview: add domain_resolution_order attribute
4e5e3ee Re-use trust domain retrieval code in certmap validators

martbab requested review from abbra and HonzaCholasta March 13, 2017 12:19

MartinBasti reviewed Mar 13, 2017

View reviewed changes

martbab force-pushed the ad-user-short-names branch from 2566716 to 87f626f Compare March 13, 2017 12:44

MartinBasti suggested changes Mar 13, 2017

View reviewed changes

pvoborni reviewed Mar 13, 2017

View reviewed changes

martbab force-pushed the ad-user-short-names branch from 87f626f to 3a29b48 Compare March 14, 2017 07:55

HonzaCholasta reviewed Mar 14, 2017

View reviewed changes

HonzaCholasta approved these changes Mar 14, 2017

View reviewed changes

MartinBasti approved these changes Mar 14, 2017

View reviewed changes

MartinBasti self-assigned this Mar 14, 2017

martbab force-pushed the ad-user-short-names branch from 3a29b48 to a6158e9 Compare March 14, 2017 15:13

Martin Babinsky added 4 commits March 14, 2017 16:15

Short name resolution: introduce the required schema

abb237c

Add ipaDomainResolutionOrder and ipaNameResolutionData to IPAv3 schema. Extend ipaConfig object with ipaNameResolutionData objectclass during update. https://pagure.io/freeipa/issue/6372

idview: add domain_resolution_order attribute

7af58bb

`idview-add` and `idview-mod` can now set and validate the attribute. The required objectclass is added on-demand after modification https://pagure.io/freeipa/issue/6372

Re-use trust domain retrieval code in certmap validators

8ac3fb1

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

martbab force-pushed the ad-user-short-names branch from a6158e9 to 8ac3fb1 Compare March 14, 2017 15:16

MartinBasti added ack Pull Request approved, can be merged pushed Pull Request has already been pushed labels Mar 14, 2017

MartinBasti closed this Mar 14, 2017

martbab deleted the ad-user-short-names branch March 15, 2017 06:16

Provide centralized management of user short name resolution #573

Provide centralized management of user short name resolution #573

martbab commented Mar 13, 2017 •

edited by MartinBasti

MartinBasti Mar 13, 2017

MartinBasti Mar 13, 2017

MartinBasti commented Mar 13, 2017

HonzaCholasta commented Mar 13, 2017

martbab commented Mar 13, 2017

abbra commented Mar 13, 2017

MartinBasti Mar 13, 2017

abbra Mar 13, 2017

pvoborni Mar 13, 2017

martbab commented Mar 14, 2017

HonzaCholasta Mar 14, 2017

HonzaCholasta Mar 14, 2017

martbab Mar 14, 2017

HonzaCholasta Mar 14, 2017 •

edited

martbab Mar 14, 2017

HonzaCholasta left a comment

martbab commented Mar 14, 2017

HonzaCholasta commented Mar 14, 2017

MartinBasti commented Mar 14, 2017

HonzaCholasta commented Mar 14, 2017

martbab commented Mar 14, 2017

abbra commented Mar 14, 2017

martbab commented Mar 14, 2017

MartinBasti commented Mar 14, 2017

		@@ -0,0 +1,135 @@
		#
		# Copyright (C) 2016 FreeIPA Contributors see COPYING for license

Provide centralized management of user short name resolution #573

Provide centralized management of user short name resolution #573

Conversation

martbab commented Mar 13, 2017 • edited by MartinBasti

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinBasti commented Mar 13, 2017

HonzaCholasta commented Mar 13, 2017

martbab commented Mar 13, 2017

abbra commented Mar 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martbab commented Mar 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HonzaCholasta Mar 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HonzaCholasta left a comment

Choose a reason for hiding this comment

martbab commented Mar 14, 2017

HonzaCholasta commented Mar 14, 2017

MartinBasti commented Mar 14, 2017

HonzaCholasta commented Mar 14, 2017

martbab commented Mar 14, 2017

abbra commented Mar 14, 2017

martbab commented Mar 14, 2017

MartinBasti commented Mar 14, 2017

martbab commented Mar 13, 2017 •

edited by MartinBasti

HonzaCholasta Mar 14, 2017 •

edited