Re: [Freeipa-devel] [PATCHES] 91-92 Add support for RFC 6594 SSHFP DNS records
On 31.1.2013 19:59, Rob Crittenden wrote: Jan Cholasta wrote: On 23.1.2013 23:45, Rob Crittenden wrote: Jan Cholasta wrote: On 10.1.2013 05:56, Jan Cholasta wrote: Hi, Patch 91 removes module ipapython.compat. The code that uses it doesn't work with ancient Python versions anyway, so there's no need to keep it around. Patch 92 adds support for automatic generation of RFC 6594 SSHFP DNS records to ipa-client-install and host plugin, as described in http://freeipa.org/page/V3/RFC_6594_SSHFP_DNS_records. Note that https://fedorahosted.org/freeipa/ticket/2642#comment:7 still applies. https://fedorahosted.org/freeipa/ticket/2642 Honza Self-NACK, forgot to actually remove ipapython/compat.py in the first patch. Also removed an unnecessary try block from the second patch. Honza These look good. I'm a little concerned about the magic numbers in the SSHFP code. I know these come from the RFCs. Can you add a comment there so future developers know where the values for key type and fingerprint type come from? rob Comment added. Sorry, I just noticed that this is an RFE and there is no design page. Can you write one up real quick, then I'll push both. Umm.. yes there is, it is linked in the first message of this thread: http://freeipa.org/page/V3/RFC_6594_SSHFP_DNS_records. I went back and forth a few times on whether we should have a ticket on the dropping of compat, if only to codify that we're giving up an python 2.6, but since this has been a given for a while I think we're ok. It's Python 2.5 that we are giving up on, not Python 2.6. In fact, we already gave up on it, our code does not work with it even if we keep compat in (we use some Python features which are not available in 2.5). rob Honza -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 146-164 LDAP code refactoring (Part 4)
On 01/31/2013 07:01 PM, Jan Cholasta wrote: On 31.1.2013 11:00, Petr Viktorin wrote: On 01/30/2013 10:53 AM, Petr Viktorin wrote: On 01/29/2013 04:39 PM, Petr Viktorin wrote: On 01/28/2013 04:09 PM, Petr Viktorin wrote: On 01/28/2013 09:34 AM, Jan Cholasta wrote: On 25.1.2013 14:54, Petr Viktorin wrote: On 01/24/2013 03:06 PM, Petr Viktorin wrote: On 01/24/2013 10:43 AM, Petr Viktorin wrote: On 01/22/2013 04:04 PM, Petr Viktorin wrote: On 01/21/2013 06:38 PM, Petr Viktorin wrote: On 01/17/2013 06:27 PM, Petr Viktorin wrote: Hello, This is the first batch of changes aimed to consolidate our LDAP code. Each should be a self-contained change that doesn't break anything. [...] Since this patchset is becoming unwieldy, I've put it in a public repo that I'll keep updated. The following command will fetch it into your pviktori-ldap-refactor branch: git fetch git://github.com/encukou/freeipa ldap-refactor:pviktori-ldap-refactor [...] I found a bug in patch 143, here is a fixed version. I would prefer if you used the semantics of .get() for .get_single() as well (i.e. when no default value is provided, None is assumed) in patch 152. Or is there a reason not to? I think you should always have to write extra code to supress exceptions (“Errors should never pass silently”). In Python, the easiest/shortest getter you can write will typically raise an exception when the value is missing (e.g. `d[k]` for dicts, `getattr(a, b)` for objects). -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 146-164 LDAP code refactoring (Part 4)
On 1.2.2013 09:47, Petr Viktorin wrote: On 01/31/2013 07:01 PM, Jan Cholasta wrote: On 31.1.2013 11:00, Petr Viktorin wrote: On 01/30/2013 10:53 AM, Petr Viktorin wrote: On 01/29/2013 04:39 PM, Petr Viktorin wrote: On 01/28/2013 04:09 PM, Petr Viktorin wrote: On 01/28/2013 09:34 AM, Jan Cholasta wrote: On 25.1.2013 14:54, Petr Viktorin wrote: On 01/24/2013 03:06 PM, Petr Viktorin wrote: On 01/24/2013 10:43 AM, Petr Viktorin wrote: On 01/22/2013 04:04 PM, Petr Viktorin wrote: On 01/21/2013 06:38 PM, Petr Viktorin wrote: On 01/17/2013 06:27 PM, Petr Viktorin wrote: Hello, This is the first batch of changes aimed to consolidate our LDAP code. Each should be a self-contained change that doesn't break anything. [...] Since this patchset is becoming unwieldy, I've put it in a public repo that I'll keep updated. The following command will fetch it into your pviktori-ldap-refactor branch: git fetch git://github.com/encukou/freeipa ldap-refactor:pviktori-ldap-refactor [...] I found a bug in patch 143, here is a fixed version. I would prefer if you used the semantics of .get() for .get_single() as well (i.e. when no default value is provided, None is assumed) in patch 152. Or is there a reason not to? I think you should always have to write extra code to supress exceptions (“Errors should never pass silently”). In Python, the easiest/shortest getter you can write will typically raise an exception when the value is missing (e.g. `d[k]` for dicts, `getattr(a, b)` for objects). That is true, but I think consistency is more important here - the name suggests the method behaves the same way .get() does. If you insist on keeping the current behavior, would you at least consider renaming the method (perhaps to just single or single_value)? (This is just a nitpick, so don't worry too much about it.) Honza -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 98-101 Preserve case of LDAP attribute names
On 01/31/2013 04:18 PM, Jan Cholasta wrote:
Hi,
these patches implement attribute name case preservation in LDAPEntry.
Apply on top of Petr Viktorin's LDAP code refactoring patchset (up to
part 5).
Honza
Patches 99 101 need some tests to make sure the _names work correctly.
Since you can call LDAPEntry.__init__ in different ways which don't
always correspond to the argument names, it would be nice to explain
them in a docstring.
A few nitpicks below.
freeipa-jcholast-99-Preserve-case-of-attribute-names-in-LDAPEntry.patch
diff --git a/ipaserver/ipaldap.py b/ipaserver/ipaldap.py
index 20c11b4..6268ac0 100644
@@ -650,14 +637,48 @@ class LDAPEntry(dict):
self._orig = self
self._orig = deepcopy(self)
+def _attr_name(self, name):
+if not isinstance(name, (unicode, str)):
Use basestring instead of (unicode, str).
freeipa-jcholast-100-Aggregate-IPASimpleLDAPObject-in-LDAPEntry.patch
diff --git a/ipaserver/ipaldap.py b/ipaserver/ipaldap.py
index 6268ac0..01fa0c1 100644
@@ -595,8 +600,10 @@ class LDAPEntry(dict):
_obj = {}
orig = None
+assert isinstance(_conn, IPASimpleLDAPObject)
assert isinstance(_dn, DN)
+self._conn = lambda: _conn # do not deepcopy me!
This would be better done by overriding __deepcopy__, or just using a
custom method for the copying.
--
Petr³
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 352-354 Add support for AD users to hbactest command
On 01/24/2013 03:04 PM, Simo Sorce wrote:
On Thu, 2013-01-24 at 08:15 +0100, Martin Kosek wrote:
On 01/23/2013 02:23 PM, Simo Sorce wrote:
On Wed, 2013-01-23 at 09:10 +0100, Martin Kosek wrote:
On 01/19/2013 07:35 PM, Simo Sorce wrote:
On Fri, 2013-01-18 at 18:24 +0100, Martin Kosek wrote:
How this works:
1. When a trusted domain user is tested, AD GC is searched
for the user entry Distinguished Name
My head is not clear today but it looks to me you are doing 2 searches.
One to go from samAccountName - DNa dn then a second for DN - SID.
Why are you doing 2 searches ? The first one can return you the
ObjectSid already.
Simo.
I had to do 2 searches because GC refuses to give me tokenGroups attribute
content when I do not search with exact DN and LDAP SCOPE_BASE. So I have
to do
the first search to find out the DN of the searched user and then a second
query to get the tokenGroups (and ObjectSid).
I see, yes that makes sense, would you mind adding a comment to this
effect so we do not try to 'optimize' at some point ?
I have no additional concerns then.
Simo.
Hello Simo,
Thanks for review. Anyway, there is already a relevant comment in dcerpc.py,
where the double search is performed:
...
def get_trusted_domain_user_and_groups(self, object_name):
...
entries = self.get_trusted_domain_objects(components.get('domain'),
components.get('flatname'), filter, attrs,
_ldap.SCOPE_SUBTREE)
# Get SIDs of user object and it's groups
# tokenGroups attribute must be read with scope BASE to avoid search
error
attrs = ['objectSID', 'tokenGroups']
...
I think it's enough to avoid optimizing this process - we would find out
the
optimization soon anyway, as the tokenGroups search would return error :-)
Perfect!
/me just had an eye vision exam, will complain to his doctor :-)
I enhanced the hbactest to also support user SID, not only trusted domain user
name. Updated set of patches attached.
How it works:
# ipa hbactest --user S-1-5-21-3035198329-144811719-1378114514-500 --host
`hostname` --service sshd
Access granted: True
Matched rules: can_login
Martin
From fe9f6ae06d29d058fecce442ed6231ae155fdaeb Mon Sep 17 00:00:00 2001
From: Martin Kosek mko...@redhat.com
Date: Fri, 18 Jan 2013 17:28:39 +0100
Subject: [PATCH 1/3] Generalize AD GC search
Modify access methods to AD GC so that callers can specify a custom
basedn, filter, scope and attribute list, thus allowing it to perform
any LDAP search.
Error checking methodology in these functions was changed, so that it
rather raises an exception with a desription instead of simply returning
a None or False value which would made an investigation why something
does not work much more difficult. External membership method in
group-add-member command was updated to match this approach.
https://fedorahosted.org/freeipa/ticket/2997
---
ipalib/plugins/group.py | 9 +--
ipaserver/dcerpc.py | 143 +++-
2 files changed, 99 insertions(+), 53 deletions(-)
diff --git a/ipalib/plugins/group.py b/ipalib/plugins/group.py
index f86b134e61fc8c7518a64d25329babee3398c6ef..347a7ee9fda9cb574f433dff3a9621d8bffee887 100644
--- a/ipalib/plugins/group.py
+++ b/ipalib/plugins/group.py
@@ -384,11 +384,12 @@ class group_add_member(LDAPAddMember):
if domain_validator.is_trusted_sid_valid(sid):
sids.append(sid)
else:
-actual_sid = domain_validator.get_sid_trusted_domain_object(sid)
-if isinstance(actual_sid, unicode):
-sids.append(actual_sid)
+try:
+actual_sid = domain_validator.get_trusted_domain_object_sid(sid)
+except errors.PublicError, e:
+failed_sids.append((sid, unicode(e)))
else:
-failed_sids.append((sid, 'Not a trusted domain SID'))
+sids.append(actual_sid)
if len(sids) == 0:
raise errors.ValidationError(name=_('external member'),
error=_('values are not recognized as valid SIDs from trusted domain'))
diff --git a/ipaserver/dcerpc.py b/ipaserver/dcerpc.py
index 54a70defc9df52db58054d29c1c9f9189a88cabb..17aae67acd92c567ae6e44d2736cad15749a9159 100644
--- a/ipaserver/dcerpc.py
+++ b/ipaserver/dcerpc.py
@@ -156,16 +156,18 @@ class DomainValidator(object):
except errors.NotFound, e:
return []
-def is_trusted_sid_valid(self, sid):
+def get_domain_by_sid(self, sid):
if not self.domain:
# our domain is not configured or self.is_configured() never run
# reject SIDs as we can't check correctness of them
-return False
+raise
Re: [Freeipa-devel] krb5.conf on IPA server and SSSD setup
On Tue, Jan 29, 2013 at 10:50:02PM +0200, Alexander Bokovoy wrote: Hi! I've been chasing few bugs in FreeIPA's trusted domains support and found out some grave bugs in both SSSD and FreeIPA. On FreeIPA server side we configure krb5.conf using following settings: - includedir /var/lib/sss/pubconf/krb5.include.d [libdefaults] ... default_realm = EXAMPLE.COM dns_lookup_realm = false dns_lookup_kdc = true ... [domain_realm] .example.com = EXAMPLE.COM example.com = EXAMPLE.com -- Then SSSD generates files which contain domain_realm mapping for trusted domains in /var/lib/sss/pubconf/krb5.include.d and libkrb5 will read them as part of the krb5.conf sourcing. Few problems here: 1. KDC needs to know this mapping information in order to issue referrals to the clients. There is heuristic in libkrb5 that uses domain_realm mapping first and default_realm value if mapping didn't catch the principal which was not found in the database. 2. krb5.conf is parsed by applications usually only on startup. KDC is not an exception, so any changes to krb5.conf would require to restart KDC if we want them to be noticed. 3. Adding new trust implies therefore KDC restart. It also implies that SSSD should have updated the mapping which is not neccessary true time-wise. As result, operations like mapping trusted domain users via external groups in IPA might fail as IPA code running on IPA server needs to contact LDAP service at trusted domain's Global Catalog using SASL GSSAPI authentication. When ticket is obtained, we don't specify explicitly the realm of the service principal, it is constructed by underlying libldap/libsasl code. If explicit domain_realm mapping is in place on client side (and here client is the server as request is issued from IPA httpd code), trusted domain's Global Catalog host will be automatically mapped to trusted domain realm. Otherwise KDC will hint the client with referral to proper KDC for trusted domain realm. This is the step that might fail if trusted domain is sub-domain of IPA domain, for example, ad.example.com. In this case our explicit mapping for example.com will prevail and requests will always be sent for principal in EXAMPLE.COM realm. More to that, since client and KDC are the same host, KDC will use domain_realm mapping as well and hint client with referral to itself (since .example.com = EXAMPLE.COM). Obtaining ticket will fail again. So, I was trying to solve this issue and I've got to following setup with Nalin's help: 1. Define following settings in [libdefaults] of krb5.conf default_realm = EXAMPLE.COM dns_lookup_realm = false dns_lookup_kdc = true realm_try_domains = 0 realm_try_domains = 0 forces libkrb5 to fallback discovery of realm to domain of the host via DNS if there is no other explicit mapping. 2. Remove any explicit domain_realm mapping for our default realm since it will be implicitly generated from default_realm value by the fallback code anyway. With these changes both KDC and libkrb5 will be able to properly serve out both own domain and trusted domain requests. At some point SSSD will kick in with its explicit mapping for trusted domain realm. Still, KDC will not be able to see this mapping until restart but in Krb5 1.12 we are getting new pluggable interface that will allow to refresh KDC configuration. If set up an environment like discussed above, and FreeIPA server and an AD server where the AD DNS domain is a sub-domain of the the IPA DNS domain. Then tried to run ldapsearch, smbclient, nsupdate and kvno accessing the AD server. Here are my findings: ldapsearch: - does not work in the default configuration - works even if no domain_realm mapping is available, but in this case the ipa client utility does not work anymore - works with full domain_realm, i.e. IPA and AD DNS domains are listed - setting realm_try_domains or not does not make any difference smbclient: - does not work in the default configuration - does not work with missing domain_realm mapping - works with full domain_realm - setting realm_try_domains or not does not make any difference nsupdate: - does not work in any configuration if the realm option is missing in the input file - works in all configurations if the realm option is given - setting realm_try_domains or not does not make any difference kvno: - I used 'kvno -S server ad-server.ad.domain' - does not work in the default configuration - works even if no domain_realm mapping is available - works with full domain_realm - setting realm_try_domains or not does not make any difference In the case without a domain_realm mapping ldapsearch and kvno first try to get a ticket with the default_realm and the KDC returns UNKNOWN_SERVER. As a second step they try to get a cross realm ticket where the
Re: [Freeipa-devel] krb5.conf on IPA server and SSSD setup
On Fri, 01 Feb 2013, Sumit Bose wrote: If set up an environment like discussed above, and FreeIPA server and an AD server where the AD DNS domain is a sub-domain of the the IPA DNS domain. Then tried to run ldapsearch, smbclient, nsupdate and kvno accessing the AD server. Here are my findings: ldapsearch: - does not work in the default configuration - works even if no domain_realm mapping is available, but in this case the ipa client utility does not work anymore - works with full domain_realm, i.e. IPA and AD DNS domains are listed - setting realm_try_domains or not does not make any difference smbclient: - does not work in the default configuration - does not work with missing domain_realm mapping - works with full domain_realm - setting realm_try_domains or not does not make any difference nsupdate: - does not work in any configuration if the realm option is missing in the input file - works in all configurations if the realm option is given - setting realm_try_domains or not does not make any difference kvno: - I used 'kvno -S server ad-server.ad.domain' - does not work in the default configuration - works even if no domain_realm mapping is available - works with full domain_realm - setting realm_try_domains or not does not make any difference I'm finding hard to parse notes above (what is 'default configuration'?). In the case without a domain_realm mapping ldapsearch and kvno first try to get a ticket with the default_realm and the KDC returns UNKNOWN_SERVER. As a second step they try to get a cross realm ticket where the realm is the uppercase version of the destinations DNS domain. Yes, this is expected behavior and this is what we want to see. Please note that if you put own realm to domain_realm mapping, KDC will use it to build referral and force you to connect to itself rather than the destination KDC. It happens because .example.com takes precedence over .ad.example.com if the latter is not specified. So KDC sees that host does not exist in its own realm but finds mapping in domain_realm section which covers the host foo.ad.example.com (.example.com) and returns that as a referral which then fails because the same KDC is queried on second attempt. With full domain_realm mapping all clients except nsupdate directly ask for the cross realm ticket. For me it looks like realm_try_domains is not needed but domain_realm mappings are. Please note that once you start adding trusted domains, includedir entry in krb5.conf will bring the mappings to them automatically. Since all applications you tested are short-lived, they will read krb5.conf on their startup and those mappings will always be actual for them. For KDC, however, problem is in actualizing domain_realm mapping, as KDC is a long-living process and does not re-read krb5.conf periodically or on any changes. In our case krb5.conf is not changed but some files in includdir are so it is even more complex. I there anything else which I should test? I think we need to find solution that does not force KDC to issue referral to its own domain. Ideally, if we could use separate krb5.conf for KDC where domain_realm mapping for own domain does not exist, we could have solved referral issue. -- / Alexander Bokovoy ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 91-92 Add support for RFC 6594 SSHFP DNS records
Jan Cholasta wrote: On 31.1.2013 19:59, Rob Crittenden wrote: Jan Cholasta wrote: On 23.1.2013 23:45, Rob Crittenden wrote: Jan Cholasta wrote: On 10.1.2013 05:56, Jan Cholasta wrote: Hi, Patch 91 removes module ipapython.compat. The code that uses it doesn't work with ancient Python versions anyway, so there's no need to keep it around. Patch 92 adds support for automatic generation of RFC 6594 SSHFP DNS records to ipa-client-install and host plugin, as described in http://freeipa.org/page/V3/RFC_6594_SSHFP_DNS_records. Note that https://fedorahosted.org/freeipa/ticket/2642#comment:7 still applies. https://fedorahosted.org/freeipa/ticket/2642 Honza Self-NACK, forgot to actually remove ipapython/compat.py in the first patch. Also removed an unnecessary try block from the second patch. Honza These look good. I'm a little concerned about the magic numbers in the SSHFP code. I know these come from the RFCs. Can you add a comment there so future developers know where the values for key type and fingerprint type come from? rob Comment added. Sorry, I just noticed that this is an RFE and there is no design page. Can you write one up real quick, then I'll push both. Umm.. yes there is, it is linked in the first message of this thread: http://freeipa.org/page/V3/RFC_6594_SSHFP_DNS_records. I looked in the ticket. Can you add the link there? I went back and forth a few times on whether we should have a ticket on the dropping of compat, if only to codify that we're giving up an python 2.6, but since this has been a given for a while I think we're ok. It's Python 2.5 that we are giving up on, not Python 2.6. In fact, we already gave up on it, our code does not work with it even if we keep compat in (we use some Python features which are not available in 2.5). Yes, off-by-one. Though in fact the client install, which is all we really care about regarding older systems, still does almost work even in python 2.4 with just a few minor changes. But like I said, its fine. pushed both to master rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 356 Add trusconfig-show and trustconfig-mod commands
On Tue, 29 Jan 2013, Martin Kosek wrote:
trust_output_params = (
@@ -482,3 +499,158 @@ api.register(trust_mod)
api.register(trust_del)
api.register(trust_find)
api.register(trust_show)
+
+
+_trust_type_option = (
+StrEnum('trust_type',
+cli_name='type',
+label=_('Trust type (ad for Active Directory, default)'),
+values=(u'ad',),
+default=u'ad',
+autofill=True,
+),
+)
We already have various trust type definitions in the same file. Maybe
it makes sense to unify those somehow?
+def get_dn(self, *keys, **kwargs):
+trust_type = kwargs.get('trust_type')
+if trust_type is None:
+raise errors.RequirementError(name='trust_type')
+if kwargs['trust_type'] == u'ad':
Perhaps better to define constants for the trust type values...
+except ValueError:
+# The search is performed for groups with posixgroup objectclass
+# and not ipausergroup so that it can also match groups like
+# Default SMG Group which does not have this objectclass.
'Default SM_B_ Group'
Thanks for the unit tests too!
--
/ Alexander Bokovoy
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 356 Add trusconfig-show and trustconfig-mod commands
On 02/01/2013 03:55 PM, Alexander Bokovoy wrote:
On Tue, 29 Jan 2013, Martin Kosek wrote:
trust_output_params = (
@@ -482,3 +499,158 @@ api.register(trust_mod)
api.register(trust_del)
api.register(trust_find)
api.register(trust_show)
+
+
+_trust_type_option = (
+StrEnum('trust_type',
+cli_name='type',
+label=_('Trust type (ad for Active Directory, default)'),
+values=(u'ad',),
+default=u'ad',
+autofill=True,
+),
+)
We already have various trust type definitions in the same file. Maybe
it makes sense to unify those somehow?
Right, I unified those 2 separate trust_type option definitions.
+def get_dn(self, *keys, **kwargs):
+trust_type = kwargs.get('trust_type')
+if trust_type is None:
+raise errors.RequirementError(name='trust_type')
+if kwargs['trust_type'] == u'ad':
Perhaps better to define constants for the trust type values...
I changed it a bit and now it uses a dict instead. I think its now more general
and extensible.
+except ValueError:
+# The search is performed for groups with posixgroup
objectclass
+# and not ipausergroup so that it can also match groups like
+# Default SMG Group which does not have this objectclass.
'Default SM_B_ Group'
Fixed.
Thanks for the unit tests too!
You are welcome! I also generated API.txt which I forgot to do last time.
Updated patch attached.
Martin
From 5296910eef8ef46c179f9cb0b10c3ebcc1d90a9f Mon Sep 17 00:00:00 2001
From: Martin Kosek mko...@redhat.com
Date: Fri, 25 Jan 2013 10:10:17 +0100
Subject: [PATCH] Add trusconfig-show and trustconfig-mod commands
Global trust configuration is generated ipa-adtrust-install script
is run. Add convenience commands to show auto-generated options
like SID or GUID or options chosen by user (NetBIOS). Most of these
options are not modifiable via trustconfig-mod command as it would
break current trusts.
Unit test file covering these new commands was added.
https://fedorahosted.org/freeipa/ticket/
---
API.txt| 24 +
VERSION| 2 +-
ipalib/plugins/trust.py| 181 +++--
tests/test_xmlrpc/test_trust_plugin.py | 159 +
tests/test_xmlrpc/xmlrpc_test.py | 10 ++
5 files changed, 368 insertions(+), 8 deletions(-)
create mode 100644 tests/test_xmlrpc/test_trust_plugin.py
diff --git a/API.txt b/API.txt
index 8fbfe6f5d8da44e991b8d1a36725fc6ace1f0616..6e5c8c5871bcfd320289291114c3c1534c400a54 100644
--- a/API.txt
+++ b/API.txt
@@ -3262,6 +3262,30 @@ option: Str('version?', exclude='webui')
output: Entry('result', type 'dict', Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
output: Output('summary', (type 'unicode', type 'NoneType'), None)
output: Output('value', type 'unicode', None)
+command: trustconfig_mod
+args: 0,9,3
+option: Str('addattr*', cli_name='addattr', exclude='webui')
+option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
+option: Str('delattr*', cli_name='delattr', exclude='webui')
+option: Str('ipantfallbackprimarygroup', attribute=True, autofill=False, cli_name='fallback_primary_group', multivalue=False, required=False)
+option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
+option: Flag('rights', autofill=True, default=False)
+option: Str('setattr*', cli_name='setattr', exclude='webui')
+option: StrEnum('trust_type', autofill=True, cli_name='type', default=u'ad', values=(u'ad',))
+option: Str('version?', exclude='webui')
+output: Entry('result', type 'dict', Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
+output: Output('summary', (type 'unicode', type 'NoneType'), None)
+output: Output('value', type 'unicode', None)
+command: trustconfig_show
+args: 0,5,3
+option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
+option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
+option: Flag('rights', autofill=True, default=False)
+option: StrEnum('trust_type', autofill=True, cli_name='type', default=u'ad', values=(u'ad',))
+option: Str('version?', exclude='webui')
+output: Entry('result', type 'dict', Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
+output: Output('summary', (type 'unicode', type 'NoneType'), None)
+output: Output('value', type 'unicode', None)
command: user_add
args: 1,34,3
arg: Str('uid', attribute=True, cli_name='login', maxlength=255, multivalue=False, pattern='^[a-zA-Z0-9_.][a-zA-Z0-9_.-]{0,252}[a-zA-Z0-9_.$-]?$', primary_key=True, required=True)
diff --git a/VERSION b/VERSION
index 61f578dbfc9415f6f94a6612f198218c5a5e0c9a..37af5ef73b74500e0cd7397fb2c109332c049bc6 100644
--- a/VERSION
+++ b/VERSION
@@ -89,4 +89,4 @@ IPA_DATA_VERSION=2010061412
#
[Freeipa-devel] [PATCH] 1083 improve migration performance
I did some analysis on migration and found several areas impacting
performance:
1. We were calling user_mod to reset the magic value in description to
not create a UPG. This caused a lot of unnecessary queries to display
the user.
2. We check the remote LDAP server to make sure that the GID is valid
and added a cache. We lacked a negative cache.
3. The biggest drag on performance was managing the default users group.
After about 1000 users it would take about half a second to calculate
the modlist and another half second for 389-ds to apply the change.
This patch addresses all of these.
For the last what I do is only do the group addition every 100 records.
A query is run to find all users who aren't in the default users group
and those are added.
I also added a bit of logging so one can better track the progress of
migration.
I migrated 12.5k users with compat enabled in 3 1/2 hours.
I migrated the same 12.5k users and 2k groups with compat disabled in 30
minutes.
By contrast when I started, with compat enabled, I migrated:
1000 users in 7 minutes
2000 users in 27 minutes
3000 users in 1 hour
rob
From f0b8bf2473d63af81b5dc5bef17499fa5e04aa85 Mon Sep 17 00:00:00 2001
From: Rob Crittenden rcrit...@redhat.com
Date: Tue, 29 Jan 2013 11:19:30 -0500
Subject: [PATCH] Improve migration performance
Add new users to the default users group in batches of 100. The
biggest overhead of migration is in calculating the modlist when
managing the default user's group and applying the changes. A
significant amount of time can be saved by not doing this on every
add operation.
Some other minor improvements include:
Add a negative cache for groups not found in the remote LDAP server.
Replace call to user_mod with a direct LDAP update.
Catch some occurances of LimitError and handle more gracefully.
I also added some debug logging to report on migration status and
performance.
https://fedorahosted.org/freeipa/ticket/3386
---
ipalib/plugins/migration.py | 70 +++--
1 file changed, 62 insertions(+), 8 deletions(-)
diff --git a/ipalib/plugins/migration.py b/ipalib/plugins/migration.py
index 81df59a23ff46b770076cb56a7d78fed8db26029..a254d60e742192d60f19c00a918a83df0326d49e 100644
--- a/ipalib/plugins/migration.py
+++ b/ipalib/plugins/migration.py
@@ -31,6 +31,7 @@ if api.env.in_server and api.env.context in ['lite', 'server']:
raise e
from ipalib import _
from ipapython.dn import DN
+import datetime
__doc__ = _(
Migration to IPA
@@ -107,6 +108,7 @@ EXAMPLES:
# USER MIGRATION CALLBACKS AND VARS
_krb_err_msg = _('Kerberos principal %s already exists. Use \'ipa user-mod\' to set it manually.')
+_krb_failed_msg = _('Unable to determine Kerberos principal %s already exists. Use \'ipa user-mod\' to set it manually.')
_grp_err_msg = _('Failed to add user to the default group. Use \'ipa group-add-member\' to add manually.')
_ref_err_msg = _('Migration of LDAP search reference is not supported.')
_dn_err_msg = _('Malformed DN')
@@ -123,13 +125,17 @@ def _pre_migrate_user(ldap, pkey, dn, entry_attrs, failed, config, ctx, **kwargs
has_upg = ctx['has_upg']
search_bases = kwargs.get('search_bases', None)
valid_gids = kwargs['valid_gids']
+invalid_gids = kwargs['invalid_gids']
if 'gidnumber' not in entry_attrs:
raise errors.NotFound(reason=_('%(user)s is not a POSIX user') % dict(user=pkey))
else:
# See if the gidNumber at least points to a valid group on the remote
# server.
-if entry_attrs['gidnumber'][0] not in valid_gids:
+if entry_attrs['gidnumber'][0] in invalid_gids:
+api.log.warn('GID number %s of migrated user %s does not point to a known group.' \
+ % (entry_attrs['gidnumber'][0], pkey))
+elif entry_attrs['gidnumber'][0] not in valid_gids:
try:
(remote_dn, remote_entry) = ds_ldap.find_entry_by_attr(
'gidnumber', entry_attrs['gidnumber'][0], 'posixgroup',
@@ -139,10 +145,13 @@ def _pre_migrate_user(ldap, pkey, dn, entry_attrs, failed, config, ctx, **kwargs
except errors.NotFound:
api.log.warn('GID number %s of migrated user %s does not point to a known group.' \
% (entry_attrs['gidnumber'][0], pkey))
+invalid_gids.append(entry_attrs['gidnumber'][0])
except errors.SingleMatchExpected, e:
# GID number matched more groups, this should not happen
api.log.warn('GID number %s of migrated user %s should match 1 group, but it matched %d groups' \
% (entry_attrs['gidnumber'][0], pkey, e.found))
+except errors.LimitsExceeded, e:
+api.log.warn('Search limit exceeded searching for GID %s' % entry_attrs['gidnumber'][0])
# We don't want to create a UPG so set the magic value in description
# to let
Re: [Freeipa-devel] [PATCHES] 94-96 Remove Entry and Entity classes
Jan Cholasta wrote: On 22.1.2013 15:32, Jan Cholasta wrote: Hi, these patches remove the Entry and Entity classes and move instantiation of LDAPEntry objects to LDAPConnection.make_entry factory method. Apply on top of Petr Viktorin's LDAP code refactoring (part 1 2) patches. Honza Slightly changed patch 95 and rebased all the patches on top of current master and LDAP code refactoring part 1 2. Honza I'm curious why you chose to use __slots__ in the LDAPEntry() class. I'm not too familiar with this directive but I always thought it was memory management thing, or are you trying to purposely limit the capabilities of the class (to prevent us rogue programmers from doing bad things)? rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 0107-0114 Fix Confusing ipa tool online help organization
Petr Viktorin wrote: On 01/31/2013 07:35 PM, Rob Crittenden wrote: Petr Viktorin wrote: On 12/14/2012 01:46 AM, Dmitri Pal wrote: On 12/13/2012 10:21 AM, Petr Viktorin wrote: https://fedorahosted.org/freeipa/ticket/3060 Here is a collection of smallish fixes to `ipa help` and `ipa something --help`. This should address most of Nikolai's proposal. Additionally, it's now possible to run `ipa command --help` without a Kerberos ticket. And there are some new tests. I've not included the Often used commands in `ipa help`; I think that is material for a manual/tutorial, not a help command. Selecting a topic from `ipa topics` and then choosing a command from `ipa help TOPIC` is a better way to use the help than the verbose `ipa help commands` or proposed incomplete Often used commands. Since the ticket has a bit of discussion and you indicate that you did not to address everything can you please extract what have been addressed and put it into a design page. I know it is not RFE but it would help to validate the changes by testers. Please put the wiki link into the ticket. http://freeipa.org/page/V3/Help What is the purpose of the no-option outfile? Do you anticipate at some point opening this up as a real option or making it easier to log while using the api directly? On incorrect invocation (`ipa`, `ipa TOPIC`), the help command is called internally with outfile=sys.stderr. That's true, but this is a lot of code to abstract writing to sys.stderr. I'm just trying to understand the reasoning. Do you imagine that some time in the future we'd want to control where the output goes? The help for help is a little confusing: - Purpose: Display help for a command or topic. Usage: ipa [global-options] help [TOPIC] [options] Positional arguments: TOPIC The topic or command name. Options: -h, --help show this help message and exit - Should [TOPIC] be [TOPIC | COMMAND] or something else? I'm trying to discourage `ipa help COMMAND` in favor of `ipa COMMAND --help`, that way you get the proper help for ambiguous words like ping (https://fedorahosted.org/freeipa/ticket/3247) I also wanted to keep the help simple and not explain this unimportant detail here. If you have better wording I can of course change it. Your reasoning is sound. Im ok with gently pushing users in that direction but leaving undocumented the old behavior. Should we create a ticket to eventually return an error in that case? On my fresh F-18 install one of the new unit tests fails: == FAIL: Test that `help user-add` `user-add -h` are equivalent and contain doc -- Traceback (most recent call last): File /usr/lib/python2.7/site-packages/nose/case.py, line 197, in runTest self.test(*self.arg) File /home/rcrit/redhat/freeipa/tests/test_cmdline/test_help.py, line 111, in test_command_help assert h_ctx.stdout == help_ctx.stdout AssertionError This is weird, it works for me. Could you send me the two outputs so I can get some idea what's wrong? I'm not sure the errors to stderr are working either: $ ipa user-show foo bar baz 2 /dev/null ipa: ERROR: command 'user_show' takes at most 1 argument That's just bash being evil, passing 2 as an argument and redirecting stdout. $ ipa user-show foo bar baz 2/tmp/err $ cat /tmp/err ipa: ERROR: command 'user_show' takes at most 1 argument D'oh, yeah I fat-fingered it. Here is the test output: help_ctx.stdout (len 1805) type unicode Purpose: Add a new user. Usage: nosetests [global-options] user-add LOGIN [options] Positional arguments: LOGIN User login Options: -h, --help show this help message and exit --first=STRFirst name --last=STR Last name --cn=STR Full name --displayname=STR Display name --initials=STR Initials --homedir=STR Home directory --gecos=STRGECOS field --shell=STRLogin shell --principal=STRKerberos principal --email=STREmail address --password Prompt to set the user password --random Generate a random user password --uid=INT User ID Number (system will assign one if not provided) --gidnumber=INTGroup ID Number --street=STR Street address --city=STR City --state=STRState/Province --postalcode=STR ZIP --phone=STRTelephone Number --mobile=STR Mobile Telephone Number --pager=STRPager Number --fax=STR Fax Number --orgunit=STR Org. Unit --title=STRJob Title --manager=STR Manager --carlicense=STR Car License --sshpubkey=STRSSH public key --setattr=STR Set an attribute to a name/value pair. Format is attr=value. For multi-valued attributes, the command replaces the values already present.
Re: [Freeipa-devel] [PATCHES] 0117-0118 Port ipa-replica-prepare to the admintool framework
Petr Viktorin wrote: On 01/31/2013 04:54 PM, Rob Crittenden wrote: Petr Viktorin wrote: On 01/28/2013 04:36 PM, Petr Viktorin wrote: On 01/04/2013 02:43 PM, Petr Viktorin wrote: On 01/03/2013 02:56 PM, John Dennis wrote: On 01/03/2013 08:00 AM, Petr Viktorin wrote: Hello, The first patch implements logging-related changes to the admintool framework and ipa-ldap-updater (the only thing ported to it so far). The design document is at http://freeipa.org/page/V3/Logging_and_output John, I decided to go ahead and put an explicit logger attribute on the tool class rather than adding debug, info, warn. etc methods dynamically using log_mgr.get_logger. I believe it's the cleanest solution. We had a discussion about this in this thread: https://www.redhat.com/archives/freeipa-devel/2012-July/msg00223.html; I didn't get a reaction to my conclusion so I'm letting you know in case you have more to say. I'm fine with not directly adding the debug, info, warn etc. methods, that practice was historical dating back to the days of Jason. However I do think it's useful to use a named logger and not the global root_logger. I'd prefer we got away from using the root_logger, it's continued existence is historical as well and the idea was over time we would slowly eliminate it's usage. FWIW the log_mgr.get_logger() is still useful for what you want to do. def get_logger(self, who, bind_logger_names=False) If you don't set bind_logger_names to True (and pass the class instance as who) you won't get the offensive debug, info, etc. methods added to the class instance. But it still does all the other bookeeping. The 'who' in this instance could be either the name of the admin tool or the class instance. Also I'd prefer using the attribute 'log' rather than 'logger'. That would make it consistent with code which does already use get_logger() passing a class instance because it's adds a 'log' attribute which is the logger. Also 'log' is twice as succinct than 'logger' (shorter line lengths). Thus if you do: log_mgr.get_logger(self) I think you'll get exactly what you want. A logger named for the class and being able to say self.log.debug() self.log.error() inside the class. In summary, just drop the True from the get_logger() call. Thanks! Yes, this works better. Updated patches attached. Here is patch 117 rebased to current master. Rebased again. Just a few minor points. Patch 117: The n-v-r should be -14. Fixed, thanks for the catch. ipa-ldap-updater is no longer runable as non-root. Was this intentional? `ipa-ldap-updater /some/file` works for me. You just can't --upgrade or run the plugins as non-root (and as the help says, --plugins is implied when no files are given). See http://www.redhat.com/archives/freeipa-devel/2012-June/msg00109.html Yeah, I remember the agreed-on behavior. I just re-tested and it works fine, so I must've been having a really bad day yesterday. Patch 118: Seems to work as it did though as a side effect of the new logging some things are displayed that we may want to suppress, specifically: request 'https://dart.example.com:8443/ca/ee/ca/profileSubmitSSLClient' I think changing the log level to DEBUG is probably the way to go. I traced that to the dogtag module. Removing it in separate patch since it will affect other code (though I don't think it will cause trouble). Ok with me. While you're at it you might consider replacing the ipa_replica_prepare remove_file() with the one in installutils. They differ slightly in implementation but basically do the same thing. Done, thanks. ACK. Pushed all three to master. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0028] Prevent backtrace in ipa-replica-prepare
Martin Kosek wrote: On 01/31/2013 12:05 PM, Tomas Babej wrote: On 01/31/2013 12:03 PM, Tomas Babej wrote: Hi, This was a regression due to change from DatabaseError to NetworkError when LDAP server is down. https://fedorahosted.org/freeipa/ticket/2939 Tomas ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Clicking send too soon, patch attached :) Tomas I don't think that removing errors.DatabaseError is necessary. By the way, would this error (and many similar errors) be solved by a server tool refactoring that Petr Viktorin is working on? IIRC, he was about to wrap ipa-replica-prepare in a similar framework like ipa-ldap-updater. With a framework like this one, we would not have to specify separate try..catch lists in all our server manipulation tools. Martin Tomas, I just pushed Petr's ipa-replica-prepare framework patch and from my testing this issue is resolved. Can you confirm this and close your bugs/tickets as appropriate? It is git commit 26c498736ec8eabb8dafbc090811c92c79a8c318 in master, for reference. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
