Re: [Freeipa-devel] FreeIPA on RHEL/CentOS 7.0
On 09/24/2014 06:19 PM, Jan Pazdziora wrote: On Wed, Sep 24, 2014 at 11:00:21AM +0200, Martin Kosek wrote: I just rebuilt latest fixed pki-coretomcat for our Copr (http://copr.fedoraproject.org/coprs/mkosek/freeipa/builds/). We are now very close to having a functional repo for RHEL/CentOS 7.0. With couple minor changes to the spec file, I was able to install FreeIPA 4.0.3 What will be the content of these yum repos going forward? Will they be fixated at 4.0.3, or will they always contain the latest greatest release? My current vision for this Copr was for it to have the latest greatest stable (-ish) FreeIPA versino. I.e. as soon as we release 4.1, it would contain 4.1 and it's dependencies. Would it make sense to create one copr repo per release, versioned, so that even when 4.0.4 or 4.1.0 is out, the 4.0.3 content is still available? It makes sense, yes - especially if there would be an interest in this from our users or your Docker use cases - given the maintenance burden. We can build some semi-automatism around it though to make the maintenance easier, I myself have some scripts ready to handle the builds. I'd like to use these yum repos for Docker images and I wonder what naming I should use for the branches and tags -- centos-7-upstream, centos-7-4.0.3, or something else? centos-7-latest (with mkosek/freeipa copr) centos-7-4-0 (with potential future mkosek/freeipa-4-0 copr) centos-7-4-1 (with potential future mkosek/freeipa-4-1 copr) Makes sense? Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] FreeIPA on RHEL/CentOS 7.0
On Thu, Sep 25, 2014 at 08:55:46AM +0200, Martin Kosek wrote: On 09/24/2014 06:19 PM, Jan Pazdziora wrote: On Wed, Sep 24, 2014 at 11:00:21AM +0200, Martin Kosek wrote: I just rebuilt latest fixed pki-coretomcat for our Copr (http://copr.fedoraproject.org/coprs/mkosek/freeipa/builds/). We are now very close to having a functional repo for RHEL/CentOS 7.0. With couple minor changes to the spec file, I was able to install FreeIPA 4.0.3 What will be the content of these yum repos going forward? Will they be fixated at 4.0.3, or will they always contain the latest greatest release? My current vision for this Copr was for it to have the latest greatest stable (-ish) FreeIPA versino. I.e. as soon as we release 4.1, it would contain 4.1 and it's dependencies. We do the same with SSSD 1.11.x and it's been quite a success, we've received several bug reports from people who run this repository. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 749-754 webui: new ID views section
On Wed, 24 Sep 2014, Endi Sukma Dewata wrote: On 9/24/2014 9:43 AM, Petr Vobornik wrote: On 24.9.2014 16:30, Endi Sukma Dewata wrote: On 9/19/2014 7:29 AM, Petr Vobornik wrote: Hello, attached patches implements Web UI part of ID Views. Backend is currently on review as well - thread [PATCHES 247-259] ID views - management part. https://fedorahosted.org/freeipa/ticket/4535 I expect that backed can change and that the UI might influence it as well. Therefore no UI integration tests nor static data files are included in this patchset. They will follow when backend is stable. snip == [PATCH] 754 webui: new ID views section == I'll test this separately. Does your patch #754-1 work with Tomas' latest patches (#247-2 - 270)? It was tested with tomas' git branch which matched http://www.redhat.com/archives/freeipa-devel/2014-September/msg00336.html OK, some comments/questions: 1. For consistency, the ID view should be capitalized into ID View in the navigation tab, page title, and dialog title. See ID Ranges as an example. 2. The tab titles in the ID view details page are quite long, and the User ID overrides and Group ID overrides labels aren't quite appropriate because the ID view can override other attributes too. How about using facet groups like in User Groups? For example: - ID view applies to: - Hosts - ID view overrides: - Users - Groups - Settings 3. Since the tab already says Applied to hosts, the current button labels is kind of redundant. How about renaming and reorder the buttons like this? - Refresh - Remove - Add - Add hosts in host group - Remove hosts in host group 4. If I understand correctly the description field for the User ID Overrides and Group ID Overrides should be optional too because it's also used to optionally override the description attribute of the original entry. No, this is description of the override itself. We don't want to override original description field, if any, we want to provide a way to document why this override was done. 5. Not sure if this is a problem. The search field in User/Group ID Overrides can be used to find the overriding attributes, but not the User/Group to override. 6. Can multiple ID views be applied to the same host? Does the order matter? If so, how would the UI manage the order? No. Single ID view per host. The scheme is actually a bit more complex: - IPA users: data from main tree is overridden with a data from a host-specific ID view - AD users: data from AD is overridden by a data from a default trust view which is then overridden by a data from a host-specific ID view 7. Related to #6, there probably should be a tab in the Host details page showing which ID views apply to it. There is only a single view and yes, it would be good to add a property there, linking it to the ID view tab, if possible. 8. If we implement #7, are the Un-apply from hosts/host groups buttons in the ID views search page still necessary? Or can it be moved into that page (i.e. unapplying one host at a time)? Mass-removal is needed to allow hostgroup management. 9. This probably requires server support. In the Apply to hosts association dialog, if a host is already added it will still appear in the dialog box. As a comparison, a User that has been added into a User Group will not appear in the association dialog anymore. Could be trivially filtered out on Web UI side. 10. The use of association dialog for Apply to host groups and Un-apply from host groups is a bit unusual because it's used to select host groups, and once selected the host groups are not added to/removed from the main list because the main list contains hosts, not host groups. Would it be very common to select multiple host groups at once to apply ID view? If not, it might make more sense to just use a plain dialog to select one host group at a time. The dialog could probably show information about the host groups (i.e. host members) before applying/unapplying the ID view. I think it could be useful to select several hostgroups at the same time, given that 'apply to host groups' operation is one shot. -- / Alexander Bokovoy ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 755 webui-ci: case-insensitive record check
On 09/25/2014 03:30 AM, Fraser Tweedale wrote: On Wed, Sep 24, 2014 at 09:16:52AM -0500, Endi Sukma Dewata wrote: On 9/24/2014 8:26 AM, Petr Vobornik wrote: On 24.9.2014 04:43, Endi Sukma Dewata wrote: On 9/22/2014 9:49 AM, Petr Vobornik wrote: [PATCH] webui-ci: case-insensitive record check Indirect association are no longer lower cased, which caused a issue in CI. Is the use of |= operator intentional? I don't see the has variable defined anywhere else in this method. has |= self.has_record(pkey.lower(), parent, table_name) If this has been tested to work, then ACK. it's defined on the previous line: has = self.has_record(pkey, parent, table_name) has |= self.has_record(pkey.lower(), parent, table_name) Ah, I see. I thought the old line was being replaced. ACK. IMO the following reads better:: has = self.has_record(pkey, parent, table_name) \ || self.has_record(pkey.lower(), parent, table_name) (Just a comment - no reason to block the patch.) Feel free to push the patch as it is, but since we're debating style... Write this: has_record = (self.has_record(pkey, parent, table_name) or self.has_record(pkey.lower(), parent, table_name)) For boolean values, you should prefer `or` and `and` over the bitwise operators, since truthy/falsy values might not be just booleans. It's better illustrated with `and`: `3 and 8` is true, but `3 8` is false. And from PEP8: - The preferred way of wrapping long lines is by using Python's implied line continuation inside parentheses, brackets and braces. Long lines can be broken over multiple lines by wrapping expressions in parentheses. These should be used in preference to using a backslash for line continuation. - The preferred place to break around a binary operator is after the operator, not before it. -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0128] dnszone-remove-permission should raise NotFound if permission doesn't exist
On 09/24/2014 06:22 PM, Martin Basti wrote: On 24/09/14 17:30, Martin Kosek wrote: On 09/24/2014 04:55 PM, Martin Basti wrote: Patch attached This probably should go to 4.0.x, 4.1 and master It is obvious that this interface was designed this way. So you should elaborate more on the should part, list use cases where current approach does not work, link to tickets, ... Sorry for that, I though I broke it during refactoring. Not so fast, pardner :-) I checked with 3.3.3 and this *was* indeed changed during your refactoring. My main point was that you should be clear about your intents and reasons for the patch, that a mere should is not clear to everybody. ACK to your patch though, works fine and restores the behavior - time to add tests?. I just adjusted the commit message a little before pushing. Pushed to: master: 2f1f1221701160ebeb4f23078adce3af59892162 ipa-4-1: 7a99f22ee0a4c5fd1d41722fa0101fee74e0c76e ipa-4-0: 32b6eb5110b2f851fbedb39ac0d853b46087465f Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0128] dnszone-remove-permission should raise NotFound if permission doesn't exist
On 25/09/14 09:59, Martin Kosek wrote: On 09/24/2014 06:22 PM, Martin Basti wrote: On 24/09/14 17:30, Martin Kosek wrote: On 09/24/2014 04:55 PM, Martin Basti wrote: Patch attached This probably should go to 4.0.x, 4.1 and master It is obvious that this interface was designed this way. So you should elaborate more on the should part, list use cases where current approach does not work, link to tickets, ... Sorry for that, I though I broke it during refactoring. Not so fast, pardner :-) I checked with 3.3.3 and this *was* indeed changed during your refactoring. My main point was that you should be clear about your intents and reasons for the patch, that a mere should is not clear to everybody. ACK to your patch though, works fine and restores the behavior - time to add tests?. I just adjusted the commit message a little before pushing. Pushed to: master: 2f1f1221701160ebeb4f23078adce3af59892162 ipa-4-1: 7a99f22ee0a4c5fd1d41722fa0101fee74e0c76e ipa-4-0: 32b6eb5110b2f851fbedb39ac0d853b46087465f Martin Thanks, I will add tests next week. -- Martin Basti ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 756 webui: fix regression in association facet preop
On 24.9.2014 04:43, Endi Sukma Dewata wrote: On 9/22/2014 9:50 AM, Petr Vobornik wrote: Association facet specs use 'add_method' instead of 'add_command' origin: https://fedorahosted.org/freeipa/ticket/4507 ACK. Pushed to: master: a56c1e58696a2489bf5bf8dd5aca603977c76a1c ipa-4-1: 18cb8d7736e433db2f10e3d72f81a295571d3e88 -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 755 webui-ci: case-insensitive record check
On 24.9.2014 16:16, Endi Sukma Dewata wrote: On 9/24/2014 8:26 AM, Petr Vobornik wrote: On 24.9.2014 04:43, Endi Sukma Dewata wrote: On 9/22/2014 9:49 AM, Petr Vobornik wrote: [PATCH] webui-ci: case-insensitive record check Indirect association are no longer lower cased, which caused a issue in CI. Is the use of |= operator intentional? I don't see the has variable defined anywhere else in this method. has |= self.has_record(pkey.lower(), parent, table_name) If this has been tested to work, then ACK. it's defined on the previous line: has = self.has_record(pkey, parent, table_name) has |= self.has_record(pkey.lower(), parent, table_name) Ah, I see. I thought the old line was being replaced. ACK. Pushed to: master: dafdd68a6ed7030d7f7a153144b36443603e884f ipa-4-1: c66b1ec8c81dc65e2807ab54bdb13957eaf55534 -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0116] Refactoring of service autobind
On 19/09/14 14:30, Jan Cholasta wrote: Dne 19.9.2014 v 13:32 Martin Basti napsal(a): On 01/09/14 16:26, Martin Basti wrote: On 28/08/14 14:01, Jan Cholasta wrote: Hi, Dne 27.8.2014 v 15:22 Martin Basti napsal(a): Patch attached. 1) Please rename object_exists to entry_exists. 2) Use empty attribute list in get_entry() in object_exists/entry_exists. 3) Please update LDAPObject.get_dn_if_exists() to use object_exists/entry_exists. 4) I'm not a fan of how do_bind() is laid out, IMHO something like this would be better (untested): +def do_bind(self, dm_password=None, autobind=AUTOBIND_AUTO, timeout=DEFAULT_TIMEOUT): +if dm_password: +self.do_simple_bind(bindpw=dm_password, timeout=timeout) +return + +if autobind != AUTOBIND_DISABLED and os.getegid() == 0 and self.ldapi: +try: +# autobind +pw_name = pwd.getpwuid(os.geteuid()).pw_name +self.do_external_bind(pw_name, timeout=timeout) +return +except errors.NotFound: +if autobind == AUTOBIND_ENABLED: +# autobind was required and failed, raise +# exception that it failed +raise + +# Fall back +self.do_sasl_gssapi_bind(timeout=timeout) Honza 3) skipped as we discuss on IRC Updated patch attached ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Please review, this should be in 4.1 1) The patch need a rebase on top of current ipa-4-1. I can apply it (Am I doing something wrong?) 2) You can remove import pwd from service.py, it is no longer used there. 3) Are named constants for the autobind argument the right thing to do? It is a tri-state which can be expressed with None/True/False. (I'm just asking, I don't have a strong opinion on this.) As we discussed on IRC, using None/True/False, is not good approach. Updated patch attached -- Martin Basti From 60a3e9972e6154ce4628a1d210941e7d621d0a1d Mon Sep 17 00:00:00 2001 From: Martin Basti mba...@redhat.com Date: Wed, 27 Aug 2014 15:06:42 +0200 Subject: [PATCH] Refactoring of autobind, object_exists Required to prevent code duplications ipaldap.IPAdmin now has method do_bind, which tries several bind methods ipaldap.IPAClient now has method object_exists(dn) --- install/tools/ipa-adtrust-install | 4 ++-- ipapython/ipaldap.py | 37 + ipaserver/install/bindinstance.py | 25 + ipaserver/install/cainstance.py | 2 +- ipaserver/install/dsinstance.py | 2 +- ipaserver/install/service.py | 30 -- 6 files changed, 50 insertions(+), 50 deletions(-) diff --git a/install/tools/ipa-adtrust-install b/install/tools/ipa-adtrust-install index 7b616c1b65c60945a2e5dc19c4afc39dad285978..6e55bbe3e57f1c609398dc571e90cb8677d91a33 100755 --- a/install/tools/ipa-adtrust-install +++ b/install/tools/ipa-adtrust-install @@ -25,7 +25,7 @@ from ipaserver.install import adtrustinstance from ipaserver.install.installutils import * from ipaserver.install import service from ipapython import version -from ipapython import ipautil, sysrestore +from ipapython import ipautil, sysrestore, ipaldap from ipalib import api, errors, util from ipapython.config import IPAOptionParser import krbV @@ -405,7 +405,7 @@ def main(): smb = adtrustinstance.ADTRUSTInstance(fstore) smb.realm = api.env.realm -smb.autobind = service.ENABLED +smb.autobind = ipaldap.AUTOBIND_ENABLED smb.setup(api.env.host, ip_address, api.env.realm, api.env.domain, netbios_name, reset_netbios_name, options.rid_base, options.secondary_rid_base, diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py index 2818f787bd8a78b358e27f88de6ccdb214011986..1702daa253d7eb568c27f66fda1810b4661656ad 100644 --- a/ipapython/ipaldap.py +++ b/ipapython/ipaldap.py @@ -27,6 +27,8 @@ from decimal import Decimal from copy import deepcopy import contextlib import collections +import os +import pwd import ldap import ldap.sasl @@ -53,6 +55,10 @@ _debug_log_ldap = False _missing = object() +# Autobind modes +AUTOBIND_AUTO = 1 +AUTOBIND_ENABLED = 2 +AUTOBIND_DISABLED = 3 def unicode_from_utf8(val): ''' @@ -1633,6 +1639,18 @@ class LDAPClient(object): with self.error_handler(): self.conn.delete_s(dn) +def entry_exists(self, dn): + +Test whether the given object exists in LDAP. + +assert isinstance(dn, DN) +try: +self.get_entry(dn, attrs_list=[]) +except errors.NotFound: +return False +else: +return True + class IPAdmin(LDAPClient): @@ -1742,6 +1760,25 @@ class IPAdmin(LDAPClient): self.__bind_with_wait(
Re: [Freeipa-devel] [PATCHES] 0633-0634 Move setting SELinux booleans to platform code; Set SELinux booleans when restoring
On 09/24/2014 06:02 PM, thierry bordaz wrote: On 08/15/2014 10:40 PM, Petr Viktorin wrote: A fix for https://fedorahosted.org/freeipa/ticket/4157 This depends on my patches 0631-0632 (for backup/restore integration tests). Our setsebool code was repeated a few times. Instead of adding another copy, I refactored what we have into a platform task. I fixed two old setsebool tickets while I was at it: https://fedorahosted.org/freeipa/ticket/2519 https://fedorahosted.org/freeipa/ticket/2934 Since ipaplatform should not depend on ipalib, and I needed a new exception type, I added a new module, ipapython.errors. This might not be the best name, since it could be confused with ipalib.errors. Opinions welcome. As for the second patch: ideally, rather than what I do with `if 'ADTRUST' in self.backup_services`, we'd get the list of booleans directly from the *instance modules, or even tell the individual services to restore themselves. But, that refactoring looks like too much to do now. Filed easyfix: https://fedorahosted.org/freeipa/ticket/4571 The first patch looks good to me. Just a minor comment. The test and run of 'paths.SELINUXENABLED' is present several times in tasks.py and fedora. Does it worth to refactor it ? About the second patch, something I do not understand. restore_selinux_booleans resets the selinux boolean to the values that are taken from SELINUX_BOOLEAN_SETTINGS in the instance (http/ad) . Does that mean this dict has been updated with the original values (using 'backup_func' in set_selinux_booleans ?). This is restoring an IPA installation, not restoring the system to a pre-IPA state. The settings need to be the same as if IPA was being installed. -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0126 - 0127] DNS: remove --class option
On 24/09/14 16:07, Martin Basti wrote:
On 23/09/14 18:53, Martin Basti wrote:
On 23/09/14 18:35, Petr Spacek wrote:
On 22.9.2014 19:21, Martin Basti wrote:
On 22/09/14 13:17, Petr Vobornik wrote:
On 19.9.2014 16:15, Martin Basti wrote:
Ticket: https://fedorahosted.org/freeipa/ticket/3414
Patch attached.
Patch 126:
1. I think that just
DeprecatedParam('dnsclass?'),
should be enough.
Also
2. You forgot to update API.txt and VERSION
Patch 127:
ACK
Updated patchset attached
ACK, it works for me.
Please don't push, we discuss this and we will nit use the
DeprecatedParam.
Updated patch attached
I didn't notice, but changes in VERSION is not required anymore.
Updated patch attached
--
Martin Basti
From 1e7c0f78605b74ad90ab4cc9834dc34272a053a9 Mon Sep 17 00:00:00 2001
From: Martin Basti mba...@redhat.com
Date: Thu, 25 Sep 2014 10:55:58 +0200
Subject: [PATCH 1/2] DNS: remove --class option
This option haven't been working, it is time to remove it.
Ticket: https://fedorahosted.org/freeipa/ticket/3414
---
ipalib/plugins/dns.py | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/ipalib/plugins/dns.py b/ipalib/plugins/dns.py
index e5525018a260d541dca6d71d00719ffbcb6ddc42..64dc6f4bd9e1ed93ac325bf66a2d4859b8b03fb9 100644
--- a/ipalib/plugins/dns.py
+++ b/ipalib/plugins/dns.py
@@ -295,6 +295,7 @@ _zone_top_record_types = ('NS', 'MX', 'LOC', )
# attributes derived from record types
_record_attributes = [str('%srecord' % t.lower()) for t in _record_types]
+# Deprecated
# supported DNS classes, IN = internet, rest is almost never used
_record_classes = (u'IN', u'CS', u'CH', u'HS')
@@ -2150,9 +2151,9 @@ class dnszone(DNSZoneBase):
maxvalue=2147483647, # see RFC 2181
),
StrEnum('dnsclass?',
+# Deprecated
cli_name='class',
-label=_('SOA class'),
-doc=_('SOA record class'),
+flags=['no_option'],
values=_record_classes,
),
Str('idnsupdatepolicy?',
@@ -2594,9 +2595,9 @@ class dnsrecord(LDAPObject):
doc=_('Time to live'),
),
StrEnum('dnsclass?',
+# Deprecated
cli_name='class',
-label=_('Class'),
-doc=_('DNS class'),
+flags=['no_option'],
values=_record_classes,
),
) + _dns_record_options
--
1.8.3.1
From 0ef0c8395a21daaa19e4419c96ab704b745779dd Mon Sep 17 00:00:00 2001
From: Martin Basti mba...@redhat.com
Date: Fri, 19 Sep 2014 16:07:40 +0200
Subject: [PATCH 2/2] WebUI: DNS: remove --class option
Ticket: https://fedorahosted.org/freeipa/ticket/3414
---
install/ui/src/freeipa/dns.js | 7 ---
1 file changed, 7 deletions(-)
diff --git a/install/ui/src/freeipa/dns.js b/install/ui/src/freeipa/dns.js
index 3eaad839a8c831b5bf771b870ed2cefd4378c8de..675a2d7e8d2a400df6079ee82e1757c53358cde8 100644
--- a/install/ui/src/freeipa/dns.js
+++ b/install/ui/src/freeipa/dns.js
@@ -154,13 +154,6 @@ return {
'idnssoaminimum',
'dnsttl',
{
-$type: 'combobox',
-name: 'dnsclass',
-options: [
-'IN', 'CS', 'CH', 'HS'
-]
-},
-{
$type: 'radio',
name: 'idnsallowdynupdate',
options: [
--
1.8.3.1
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] FreeIPA on RHEL/CentOS 7.0
On Thu, Sep 25, 2014 at 08:55:46AM +0200, Martin Kosek wrote: I'd like to use these yum repos for Docker images and I wonder what naming I should use for the branches and tags -- centos-7-upstream, centos-7-4.0.3, or something else? centos-7-latest (with mkosek/freeipa copr) centos-7-4-0 (with potential future mkosek/freeipa-4-0 copr) centos-7-4-1 (with potential future mkosek/freeipa-4-1 copr) Makes sense? Yes, thanks. -- Jan Pazdziora Principal Software Engineer, Identity Management Engineering, Red Hat ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 0633-0634 Move setting SELinux booleans to platform code; Set SELinux booleans when restoring
On 09/25/2014 10:58 AM, Petr Viktorin wrote: On 09/24/2014 06:02 PM, thierry bordaz wrote: On 08/15/2014 10:40 PM, Petr Viktorin wrote: A fix for https://fedorahosted.org/freeipa/ticket/4157 This depends on my patches 0631-0632 (for backup/restore integration tests). Our setsebool code was repeated a few times. Instead of adding another copy, I refactored what we have into a platform task. I fixed two old setsebool tickets while I was at it: https://fedorahosted.org/freeipa/ticket/2519 https://fedorahosted.org/freeipa/ticket/2934 Since ipaplatform should not depend on ipalib, and I needed a new exception type, I added a new module, ipapython.errors. This might not be the best name, since it could be confused with ipalib.errors. Opinions welcome. As for the second patch: ideally, rather than what I do with `if 'ADTRUST' in self.backup_services`, we'd get the list of booleans directly from the *instance modules, or even tell the individual services to restore themselves. But, that refactoring looks like too much to do now. Filed easyfix: https://fedorahosted.org/freeipa/ticket/4571 The first patch looks good to me. Just a minor comment. The test and run of 'paths.SELINUXENABLED' is present several times in tasks.py and fedora. Does it worth to refactor it ? About the second patch, something I do not understand. restore_selinux_booleans resets the selinux boolean to the values that are taken from SELINUX_BOOLEAN_SETTINGS in the instance (http/ad) . Does that mean this dict has been updated with the original values (using 'backup_func' in set_selinux_booleans ?). This is restoring an IPA installation, not restoring the system to a pre-IPA state. The settings need to be the same as if IPA was being installed. OK thanks for the explanation. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0126 - 0127] DNS: remove --class option
On 25.9.2014 11:03, Martin Basti wrote: On 24/09/14 16:07, Martin Basti wrote: I didn't notice, but changes in VERSION is not required anymore. Updated patch attached ACK pushed to master: * 7325983a48c9cf9300d046260c98253b6dae2dbc DNS: remove --class option * 180414d64d992f80e03d0627deff7709f81520bd WebUI: DNS: remove --class option ipa-4-1: * 7d61444732d42aed12b0966068dce090c398aec5 DNS: remove --class option * 12c49d88942a45e7c053a6c005ce10f2b8cabe88 WebUI: DNS: remove --class option -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0105 FIX: LDAP_updater
On 24.9.2014 12:06, Petr Viktorin wrote: On 09/23/2014 02:51 PM, Martin Basti wrote: On 22/09/14 14:04, Petr Viktorin wrote: On 09/01/2014 04:31 PM, Martin Basti wrote: On 24/07/14 09:06, Martin Basti wrote: On 23/07/14 15:17, Martin Basti wrote: This patch fixes ordering problem of schema updates Martin should it be in IPA 4.0.x ? It requires rebased ldap_python (will be in Fedora 21) [...] Thank you! updated patch attached. ACK We should wait with pushing until the rebased ldap-python *is* in Fedora, or at least in a COPR. So far it's not even in Koji, AFAICS. https://admin.fedoraproject.org/updates/python-ldap-2.4.16-1.fc20 https://admin.fedoraproject.org/updates/python-ldap-2.4.16-1.fc21 -- Petr^2 Spacek ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] FreeIPA on RHEL/CentOS 7.0
On 09/25/2014 11:09 AM, Jan Pazdziora wrote: On Thu, Sep 25, 2014 at 08:55:46AM +0200, Martin Kosek wrote: I'd like to use these yum repos for Docker images and I wonder what naming I should use for the branches and tags -- centos-7-upstream, centos-7-4.0.3, or something else? centos-7-latest (with mkosek/freeipa copr) centos-7-4-0 (with potential future mkosek/freeipa-4-0 copr) centos-7-4-1 (with potential future mkosek/freeipa-4-1 copr) Makes sense? Yes, thanks. Although now looking at the branch names, people may confused CentOS/RHEL version with FreeIPA version (I am referring to 7-4-0 part). So centos-7-ipa-latest centos-7-ipa-4-1 centos-7-ipa-4-0 may be better + would also reflect the actual branch names. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0105 FIX: LDAP_updater
On 09/25/2014 12:13 PM, Petr Spacek wrote: On 24.9.2014 12:06, Petr Viktorin wrote: On 09/23/2014 02:51 PM, Martin Basti wrote: On 22/09/14 14:04, Petr Viktorin wrote: On 09/01/2014 04:31 PM, Martin Basti wrote: On 24/07/14 09:06, Martin Basti wrote: On 23/07/14 15:17, Martin Basti wrote: This patch fixes ordering problem of schema updates Martin should it be in IPA 4.0.x ? It requires rebased ldap_python (will be in Fedora 21) [...] Thank you! updated patch attached. ACK We should wait with pushing until the rebased ldap-python *is* in Fedora, or at least in a COPR. So far it's not even in Koji, AFAICS. https://admin.fedoraproject.org/updates/python-ldap-2.4.16-1.fc20 https://admin.fedoraproject.org/updates/python-ldap-2.4.16-1.fc21 It is now also being built in the Copr: http://copr.fedoraproject.org/coprs/mkosek/freeipa/build/33650/ Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0105 FIX: LDAP_updater
On 09/25/2014 12:13 PM, Petr Spacek wrote: On 24.9.2014 12:06, Petr Viktorin wrote: On 09/23/2014 02:51 PM, Martin Basti wrote: On 22/09/14 14:04, Petr Viktorin wrote: On 09/01/2014 04:31 PM, Martin Basti wrote: On 24/07/14 09:06, Martin Basti wrote: On 23/07/14 15:17, Martin Basti wrote: This patch fixes ordering problem of schema updates Martin should it be in IPA 4.0.x ? It requires rebased ldap_python (will be in Fedora 21) [...] Thank you! updated patch attached. ACK We should wait with pushing until the rebased ldap-python *is* in Fedora, or at least in a COPR. So far it's not even in Koji, AFAICS. https://admin.fedoraproject.org/updates/python-ldap-2.4.16-1.fc20 https://admin.fedoraproject.org/updates/python-ldap-2.4.16-1.fc21 Pushed to: master: c81acfff43042421b06873e66a04c1aebe89579b ipa-4-1: d8d5b2ea89a83b5b63b2650a421f16f341783126 -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0647 test_permission_plugin: Check legacy permissions
On 09/19/2014 08:31 PM, Petr Viktorin wrote: This has been wrong for some time, now I got around to fixing it properly. It should go to all branches (4.0, 4.1, master). Thank you! This should make our unit tests more stable :-) Worked fine, ACK. Pushed to: master: f3b1471af946c7231447b36ea4196bb3278d6a1b ipa-4-1: 5cae98912de64c793f8589fba0a0521823648bdb ipa-4-0: 6f100c50f0c9d125ae09fd7a84ae2f801a3707fd Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0637 upgradeinstance: Restore listeners on failure
On 09/24/2014 10:43 AM, Martin Kosek wrote: On 08/22/2014 06:07 PM, Petr Viktorin wrote: https://fedorahosted.org/freeipa/ticket/4499 Actually I wonder why we use backup_state/restore_state for these settings. Rob, was there a reason for not just always setting nsslapd-port: 389 and nsslapd-security: on? This works pretty nicely, I liked the service.py extension. My test output: # ipa-ldap-updater --upgrade Upgrading IPA: [1/10]: stopping directory server [2/10]: saving configuration [3/10]: disabling listeners [4/10]: starting directory server [5/10]: preparing server upgrade PRE_SCHEMA_UPDATE [6/10]: updating schema ... [7/10]: upgrading server [error] ValueError: Ha! [cleanup]: stopping directory server [cleanup]: restoring configuration # ipactl start # netstat -putna | grep 389 ... tcp6 0 0 10.16.78.147:38910.16.78.147:37490 ESTABLISHED 5956/ns-slapd So I am willing to ACK if there are no other objections. Martin I read the silence as no objections, so ACK. Pushed to: master: 9a188607fcf68721fc8c38c3c73ee02cac76b58a ipa-4-1: b333e7adc98ff7c5335fbc7ce1bde5b9dfb3f5ef Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0067] Use stack allocation when writing values during otp auth
On 09/19/2014 07:49 PM, Nathaniel McCallum wrote: This is an optimization from patch 0062 (rescinded) which I think is worth keeping. There is no ticket for this. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Hello, That is exact that slapi* are doing a intensive usage of the of alloc/free. Using a stack allocated mods would reduce the number of alloc/free but I afraid it will not have a significant impact. For example further slapi_modify_internal is doing tons of alloc/free. Also I think it is not possible to use stack allocated mods. In fact mods will be modified by internal_mod (for example to add 'modifytimestamp', 'modifiersname') and mods will be reallocated. This could also occurs from plugins. Finally if a modify retry occurs, the original mods are freeed. thanks thierry ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 130] extdom: add support for new version
On Wed, Sep 24, 2014 at 03:23:54PM +0200, Jakub Hrozek wrote:
On Tue, Sep 23, 2014 at 05:11:01PM +0200, Sumit Bose wrote:
Hi,
this patch should fix https://fedorahosted.org/freeipa/ticket/4031 and
with the corresponding SSSD part it would be possible to get the full
list of group memberships with the id command even for user who didn't
log in before.
bye,
Sumit
So far I only read the patch, no testing was done yet (I'm installing a
separate VM where I'll keep this new plugin for easy comparison and
backwards-compatibility testing)
Thank you for the review, please see comments below.
First, there are some Coverity warnings:
Error: USE_AFTER_FREE (CWE-825):
freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:242:
alias: Assigning: groups = new_groups. Now both point to the same
storage.
freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:246:
freed_arg: free(void *) frees groups.
freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:252:
use_after_free: Using freed pointer groups.
fixed
Error: CONSTANT_EXPRESSION_RESULT (CWE-398):
freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:596:
missing_parentheses: !id_type != SSS_ID_TYPE_GID is always true regardless
of the values of its operands. Did you intend to either negate the entire
comparison expression, in which case parentheses would be required around the
entire comparison expression to force that interpretation, or negate the
sense of the comparison (that is, use '==' rather than '!=')? This occurs as
the logical second operand of '||'.
fixed
Error: DEADCODE (CWE-561):
freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:594:
cond_cannot_single: Condition request_type == 1U, taking false branch. Now
the value of request_type cannot be equal to 1.
freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:594:
cond_cannot_set: Condition request_type == 3U, taking false branch. Now
the value of request_type cannot be equal to any of {1, 3}.
freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:606:
cannot_set: At condition request_type == 1U, the value of request_type
cannot be equal to any of {1, 3}.
freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:606:
dead_error_condition: The condition request_type == 1U cannot be true.
freeipa-4.0.0GIT2563ea2/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c:607:
dead_error_line: Execution cannot reach this statement ret =
pack_ber_sid(sid_str,
I thik this is a result of the CONSTANT_EXPRESSION_RESULT issue, since I
fixed it this warning should be gone as well.
See some comments inline.
From 23ff38cdea85995b211e73f474bcb4b0d7fb8039 Mon Sep 17 00:00:00 2001
From: Sumit Bose sb...@redhat.com
Date: Tue, 23 Sep 2014 15:55:43 +0200
Subject: [PATCH] extdom: add support for new version
Currently the extdom plugin is basically used to translate SIDs of AD
users and groups to names and POSIX IDs.
With this patch a new version is added which will return the full member
list for groups and the full list of group memberships for a user.
Additionally the gecos field, the home directory and the login shell of a
user are returned and an optional list of key-value pairs which
currently will contain the SID of the requested object if available.
https://fedorahosted.org/freeipa/ticket/4031
---
.../ipa-extdom-extop/ipa_extdom.h | 29 +-
.../ipa-extdom-extop/ipa_extdom_common.c | 850
+++--
.../ipa-extdom-extop/ipa_extdom_extop.c| 28 +-
3 files changed, 640 insertions(+), 267 deletions(-)
diff --git a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h
b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h
index
5f834a047a579104cd2589ce417c580c1c5388d3..548ee74f561c474854c049726c4c3e71da5cbbea
100644
--- a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h
+++ b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h
@@ -64,6 +64,7 @@
#include sss_nss_idmap.h
#define EXOP_EXTDOM_OID 2.16.840.1.113730.3.8.10.4
+#define EXOP_EXTDOM_V2_OID 2.16.840.1.113730.3.8.10.4.1
It's a bit odd that this control is called V1 in the SSSD tree and V2 in
the IPA tree. It's not wrong, just strange maybe.
you are right, I renamed the versions here.
-int handle_request(struct ipa_extdom_ctx *ctx, struct extdom_req *req,
- struct extdom_res **res)
+int check_request(struct extdom_req *req, enum extdom_version version)
+{
+if (version == EXTDOM_V1) {
+if (req-request_type == REQ_FULL_WITH_GROUPS) {
+return LDAP_PROTOCOL_ERROR;
+}
+}
Any
Re: [Freeipa-devel] [PATCH 0116] Refactoring of service autobind
Dne 25.9.2014 v 10:51 Martin Basti napsal(a): On 19/09/14 14:30, Jan Cholasta wrote: Dne 19.9.2014 v 13:32 Martin Basti napsal(a): On 01/09/14 16:26, Martin Basti wrote: On 28/08/14 14:01, Jan Cholasta wrote: Hi, Dne 27.8.2014 v 15:22 Martin Basti napsal(a): Patch attached. 1) Please rename object_exists to entry_exists. 2) Use empty attribute list in get_entry() in object_exists/entry_exists. 3) Please update LDAPObject.get_dn_if_exists() to use object_exists/entry_exists. 4) I'm not a fan of how do_bind() is laid out, IMHO something like this would be better (untested): +def do_bind(self, dm_password=None, autobind=AUTOBIND_AUTO, timeout=DEFAULT_TIMEOUT): +if dm_password: +self.do_simple_bind(bindpw=dm_password, timeout=timeout) +return + +if autobind != AUTOBIND_DISABLED and os.getegid() == 0 and self.ldapi: +try: +# autobind +pw_name = pwd.getpwuid(os.geteuid()).pw_name +self.do_external_bind(pw_name, timeout=timeout) +return +except errors.NotFound: +if autobind == AUTOBIND_ENABLED: +# autobind was required and failed, raise +# exception that it failed +raise + +# Fall back +self.do_sasl_gssapi_bind(timeout=timeout) Honza 3) skipped as we discuss on IRC Updated patch attached ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Please review, this should be in 4.1 1) The patch need a rebase on top of current ipa-4-1. I can apply it (Am I doing something wrong?) 2) You can remove import pwd from service.py, it is no longer used there. 3) Are named constants for the autobind argument the right thing to do? It is a tri-state which can be expressed with None/True/False. (I'm just asking, I don't have a strong opinion on this.) As we discussed on IRC, using None/True/False, is not good approach. Updated patch attached ACK, but the patch still does not apply cleanly on ipa-4-1: $ git apply freeipa-mbasti-0116.3-Refactoring-of-autobind-object_exists.patch error: patch failed: ipaserver/install/service.py:20 error: ipaserver/install/service.py: patch does not apply -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0645 ipa-replica-prepare: Wait for the DNS entry to be resolvable
On 09/24/2014 02:07 PM, Petr Viktorin wrote:
On 09/24/2014 01:54 PM, Petr Spacek wrote:
On 24.9.2014 13:47, Petr Viktorin wrote:
On 09/23/2014 06:00 PM, Petr Spacek wrote:
On 22.9.2014 14:09, Petr Viktorin wrote:
On 09/22/2014 01:48 PM, Petr Spacek wrote:
On 22.9.2014 10:38, Martin Kosek wrote:
On 09/22/2014 10:31 AM, Petr Spacek wrote:
On 22.9.2014 10:14, Martin Kosek wrote:
On 09/19/2014 07:29 PM, Petr Viktorin wrote:
https://fedorahosted.org/freeipa/ticket/4551
See ticket commit message for details.
Shouldn't we add a 1 sec sleep between tries? Wouldn't current
version just
hammer DNS server with as many DNS queries as it can send?
[...]
LGTM except one nitpick I didn't see before:
+if not options.wait_for_dns or self.check_dns(replica_fqdn):
+self.log.debug('%s A/ record resolvable', replica_fqdn)
+return
This will print message
'%s A/ record resolvable', replica_fqdn
even if I use switch --no-wait-for-dns
This is sooo minor detail that it can be deferred, please open a ticket
if you don't plan to send new version of the patch.
You're right.
Let's do it correctly now; this isn't worth the overhead of a ticket.
Based on discussion more with Petr, I extended the list of handled
exceptions.
--
Petr³
From 8832e243297a8ab5f265798350fd30dfbc3a710f Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Fri, 19 Sep 2014 15:57:44 +0200
Subject: [PATCH] ipa-replica-prepare: Wait for the DNS entry to be resolvable
It takes some time after the DNS record is added until it propagates
to Bind. In automated installations, it might happen that
replica-install is attempted before the hostname is resolvable;
in that case the connection check would fail.
Wait for the name to be resolvable at the end of replica-prepare.
Mention that this can be interrupted (Ctrl+C).
Provide an option to skip the wait.
In case DNS is not managed by IPA, this reminds the admin of the necessary
configuration and checks their work, but it's possible to skip (either by
interrupting it interactively, or by the option).
https://fedorahosted.org/freeipa/ticket/4551
---
ipaserver/install/ipa_replica_prepare.py | 53
1 file changed, 53 insertions(+)
diff --git a/ipaserver/install/ipa_replica_prepare.py b/ipaserver/install/ipa_replica_prepare.py
index 7762614a1174208db0915d4e882212b3f578476e..c8d2e64c2ffc9addbb8cffbd13317ac2f82be5f6 100644
--- a/ipaserver/install/ipa_replica_prepare.py
+++ b/ipaserver/install/ipa_replica_prepare.py
@@ -21,9 +21,12 @@
import os
import shutil
import tempfile
+import time
from optparse import OptionGroup
from ConfigParser import SafeConfigParser
+import dns.resolver
+
from ipaserver.install import certs, installutils, bindinstance, dsinstance
from ipaserver.install.replication import enable_replication_version_checking
from ipaserver.plugins.ldap2 import ldap2
@@ -64,6 +67,9 @@ def add_options(cls, parser):
parser.add_option(--ca, dest=ca_file, default=paths.CACERT_P12,
metavar=FILE,
help=location of CA PKCS#12 file, default /root/cacert.p12)
+parser.add_option('--no-wait-for-dns', dest='wait_for_dns',
+action='store_false', default=True,
+help=do not wait until the replica is resolvable in DNS)
group = OptionGroup(parser, SSL certificate options,
Only used if the server was installed using custom SSL certificates)
@@ -290,6 +296,9 @@ def run(self):
if options.ip_address:
self.add_dns_records()
+if options.wait_for_dns:
+self.wait_for_dns()
+
def copy_ds_certificate(self):
options = self.options
@@ -451,6 +460,50 @@ def add_dns_records(self):
raise admintool.ScriptError(
Could not add reverse DNS record for the replica: %s % e)
+def check_dns(self, replica_fqdn):
+Return true if the replica hostname is resolvable
+resolver = dns.resolver.Resolver()
+exceptions = (dns.resolver.NXDOMAIN, dns.resolver.NoAnswer,
+ dns.resolver.Timeout, dns.resolver.NoNameservers)
+
+try:
+dns_answer = resolver.query(replica_fqdn, 'A', 'IN')
+except exceptions:
+try:
+dns_answer = resolver.query(replica_fqdn, '', 'IN')
+except exceptions:
+return False
+except Exception as e:
+self.log.warn('Exception while waiting for DNS record: %s: %s',
+ type(e).__name__, e)
+
+return True
+
+def wait_for_dns(self):
+options = self.options
+
+# Make sure replica_fqdn has a trailing dot, so the
+# 'search' directive in /etc/resolv.conf doesn't apply
+replica_fqdn = self.replica_fqdn
+if not replica_fqdn.endswith('.'):
+replica_fqdn += '.'
+
+if self.check_dns(replica_fqdn):
+
Re: [Freeipa-devel] [PATCHES 0114-0115] DNS: allow to add root zone '.'
Ticket: https://fedorahosted.org/freeipa/ticket/4149 5. You've removed 'idnssoamname' and 'force' from Web UI but dnszone-add precallback still uses these params. What is the intended purpose? User should use modify dialog in webUI for zones. Precallback fills default value for idnsmname from LDAP. with --force there will be no validation of user specified soa mname Issue with web ui is that it can't call dnszone-mod with --force option. Should be fixed separately. Unrelated to this patch set: b. Web UI doesn't have means to call dnszone-mod with --force option I'm not sure what you mean, it didn't do that before my patches. See #5 Updated patches attached Review of new version: All new issues are nitpicks. If somebody else thinks they are OK and if pspacek's functional tests pass then ACK. Patch: 114: ACK Patch: 115: ACK Patch: 120 1) Why is there: `default=None, # value will be added in precallback from ldap` ? Static 'default' is by default `None` 2) Wonder if RequirementError would be a better fit: +raise errors.ValidationError( +name='name_server', +error=_(uis required)) Patch 121: 3) +if ns_hostname: +ns_hostname = normalize_zone(ns_hostname) +ns_hostname = unicode(ns_hostname) try: api.Command.dnszone_add(unicode(name), -idnssoamname=unicode(ns_main), +idnssoamname=ns_hostname, If `ns_hostname` is '' then it will not be converted to unicode. I'm not sure if it can cause an issue. Patch 123: ACK Patch 124: ACK Patch 125: ACK -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0116] Refactoring of service autobind
On 25/09/14 14:47, Jan Cholasta wrote: Dne 25.9.2014 v 10:51 Martin Basti napsal(a): On 19/09/14 14:30, Jan Cholasta wrote: Dne 19.9.2014 v 13:32 Martin Basti napsal(a): On 01/09/14 16:26, Martin Basti wrote: On 28/08/14 14:01, Jan Cholasta wrote: Hi, Dne 27.8.2014 v 15:22 Martin Basti napsal(a): Patch attached. 1) Please rename object_exists to entry_exists. 2) Use empty attribute list in get_entry() in object_exists/entry_exists. 3) Please update LDAPObject.get_dn_if_exists() to use object_exists/entry_exists. 4) I'm not a fan of how do_bind() is laid out, IMHO something like this would be better (untested): +def do_bind(self, dm_password=None, autobind=AUTOBIND_AUTO, timeout=DEFAULT_TIMEOUT): +if dm_password: +self.do_simple_bind(bindpw=dm_password, timeout=timeout) +return + +if autobind != AUTOBIND_DISABLED and os.getegid() == 0 and self.ldapi: +try: +# autobind +pw_name = pwd.getpwuid(os.geteuid()).pw_name +self.do_external_bind(pw_name, timeout=timeout) +return +except errors.NotFound: +if autobind == AUTOBIND_ENABLED: +# autobind was required and failed, raise +# exception that it failed +raise + +# Fall back +self.do_sasl_gssapi_bind(timeout=timeout) Honza 3) skipped as we discuss on IRC Updated patch attached ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Please review, this should be in 4.1 1) The patch need a rebase on top of current ipa-4-1. I can apply it (Am I doing something wrong?) 2) You can remove import pwd from service.py, it is no longer used there. 3) Are named constants for the autobind argument the right thing to do? It is a tri-state which can be expressed with None/True/False. (I'm just asking, I don't have a strong opinion on this.) As we discussed on IRC, using None/True/False, is not good approach. Updated patch attached ACK, but the patch still does not apply cleanly on ipa-4-1: $ git apply freeipa-mbasti-0116.3-Refactoring-of-autobind-object_exists.patch error: patch failed: ipaserver/install/service.py:20 error: ipaserver/install/service.py: patch does not apply Rebased patches attached -- Martin Basti From 851ab5beecfccfcc61323ef9127c61098c3d54be Mon Sep 17 00:00:00 2001 From: Martin Basti mba...@redhat.com Date: Wed, 27 Aug 2014 15:06:42 +0200 Subject: [PATCH] Refactoring of autobind, object_exists Required to prevent code duplications ipaldap.IPAdmin now has method do_bind, which tries several bind methods ipaldap.IPAClient now has method object_exists(dn) --- install/tools/ipa-adtrust-install | 4 ++-- ipapython/ipaldap.py | 37 + ipaserver/install/bindinstance.py | 25 + ipaserver/install/cainstance.py | 2 +- ipaserver/install/dsinstance.py | 2 +- ipaserver/install/service.py | 30 -- 6 files changed, 50 insertions(+), 50 deletions(-) diff --git a/install/tools/ipa-adtrust-install b/install/tools/ipa-adtrust-install index 7b616c1b65c60945a2e5dc19c4afc39dad285978..6e55bbe3e57f1c609398dc571e90cb8677d91a33 100755 --- a/install/tools/ipa-adtrust-install +++ b/install/tools/ipa-adtrust-install @@ -25,7 +25,7 @@ from ipaserver.install import adtrustinstance from ipaserver.install.installutils import * from ipaserver.install import service from ipapython import version -from ipapython import ipautil, sysrestore +from ipapython import ipautil, sysrestore, ipaldap from ipalib import api, errors, util from ipapython.config import IPAOptionParser import krbV @@ -405,7 +405,7 @@ def main(): smb = adtrustinstance.ADTRUSTInstance(fstore) smb.realm = api.env.realm -smb.autobind = service.ENABLED +smb.autobind = ipaldap.AUTOBIND_ENABLED smb.setup(api.env.host, ip_address, api.env.realm, api.env.domain, netbios_name, reset_netbios_name, options.rid_base, options.secondary_rid_base, diff --git a/ipapython/ipaldap.py b/ipapython/ipaldap.py index 2818f787bd8a78b358e27f88de6ccdb214011986..1702daa253d7eb568c27f66fda1810b4661656ad 100644 --- a/ipapython/ipaldap.py +++ b/ipapython/ipaldap.py @@ -27,6 +27,8 @@ from decimal import Decimal from copy import deepcopy import contextlib import collections +import os +import pwd import ldap import ldap.sasl @@ -53,6 +55,10 @@ _debug_log_ldap = False _missing = object() +# Autobind modes +AUTOBIND_AUTO = 1 +AUTOBIND_ENABLED = 2 +AUTOBIND_DISABLED = 3 def unicode_from_utf8(val): ''' @@ -1633,6 +1639,18 @@ class LDAPClient(object): with self.error_handler(): self.conn.delete_s(dn) +def entry_exists(self, dn): + +Test whether the given
Re: [Freeipa-devel] [PATCH 0068] Move OTP synchronization step to after counter writeback
On 09/19/2014 07:53 PM, Nathaniel McCallum wrote: This prevents synchronization when an authentication collision occurs. https://fedorahosted.org/freeipa/ticket/4493 NOTE: this patch is related to the above ticket, but does not solve it. For the solution, please see patch 0064. This behavior fix is from patch 0062 (rescinded) and is worth keeping. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Hello Nathaniel, . My understanding is that during a pre_bind, the plugins validates token codes (for example HOTP) checking that step ranges [-25..+25]. As soon as the token is valid, the new HOTPcounter is written in the entry. But in case of negative steps,I believe the otp-decrement plugin will reject this update. If TOTPwatermark is updated and there is a second token code, then clockOffset is also updated. This update is done during a pre_bind, so if there are parallel binds on the server, there is a possibility that TOTPwatermark is updated from a bind and 'clockOffset' updated from an other bind. An option is to do a single internal modify to update both. thanks thierry ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES 0114-0115] DNS: allow to add root zone '.'
On 25/09/14 14:56, Petr Vobornik wrote: Ticket: https://fedorahosted.org/freeipa/ticket/4149 5. You've removed 'idnssoamname' and 'force' from Web UI but dnszone-add precallback still uses these params. What is the intended purpose? User should use modify dialog in webUI for zones. Precallback fills default value for idnsmname from LDAP. with --force there will be no validation of user specified soa mname Issue with web ui is that it can't call dnszone-mod with --force option. Should be fixed separately. Unrelated to this patch set: b. Web UI doesn't have means to call dnszone-mod with --force option I'm not sure what you mean, it didn't do that before my patches. See #5 Updated patches attached Review of new version: All new issues are nitpicks. If somebody else thinks they are OK and if pspacek's functional tests pass then ACK. Patch: 114: ACK Patch: 115: ACK Patch: 120 1) Why is there: `default=None, # value will be added in precallback from ldap` ? Static 'default' is by default `None` You're right. I'll leave there only comment. 2) Wonder if RequirementError would be a better fit: +raise errors.ValidationError( +name='name_server', +error=_(uis required)) I tried it and ipa currently returns RequirementError, I will change it Patch 121: 3) +if ns_hostname: +ns_hostname = normalize_zone(ns_hostname) +ns_hostname = unicode(ns_hostname) try: api.Command.dnszone_add(unicode(name), -idnssoamname=unicode(ns_main), +idnssoamname=ns_hostname, If `ns_hostname` is '' then it will not be converted to unicode. I'm not sure if it can cause an issue. ns_hostname values can be only None or DNSName instance, DNSName instance is always non_zero. Patch 123: ACK Patch 124: ACK Patch 125: ACK -- Martin Basti ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0645 ipa-replica-prepare: Wait for the DNS entry to be resolvable
On 25.9.2014 14:56, Petr Viktorin wrote:
On 09/24/2014 02:07 PM, Petr Viktorin wrote:
On 09/24/2014 01:54 PM, Petr Spacek wrote:
On 24.9.2014 13:47, Petr Viktorin wrote:
On 09/23/2014 06:00 PM, Petr Spacek wrote:
On 22.9.2014 14:09, Petr Viktorin wrote:
On 09/22/2014 01:48 PM, Petr Spacek wrote:
On 22.9.2014 10:38, Martin Kosek wrote:
On 09/22/2014 10:31 AM, Petr Spacek wrote:
On 22.9.2014 10:14, Martin Kosek wrote:
On 09/19/2014 07:29 PM, Petr Viktorin wrote:
https://fedorahosted.org/freeipa/ticket/4551
See ticket commit message for details.
Shouldn't we add a 1 sec sleep between tries? Wouldn't current
version just
hammer DNS server with as many DNS queries as it can send?
[...]
LGTM except one nitpick I didn't see before:
+if not options.wait_for_dns or self.check_dns(replica_fqdn):
+self.log.debug('%s A/ record resolvable', replica_fqdn)
+return
This will print message
'%s A/ record resolvable', replica_fqdn
even if I use switch --no-wait-for-dns
This is sooo minor detail that it can be deferred, please open a ticket
if you don't plan to send new version of the patch.
You're right.
Let's do it correctly now; this isn't worth the overhead of a ticket.
Based on discussion more with Petr, I extended the list of handled exceptions.
ACK, it works even in corner cases like YXDOMAIN* answer and the like.
* Some name that ought not to exist, does exist. [RFC2136]
--
Petr^2 Spacek
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0645 ipa-replica-prepare: Wait for the DNS entry to be resolvable
On 09/25/2014 03:23 PM, Petr Spacek wrote:
On 25.9.2014 14:56, Petr Viktorin wrote:
On 09/24/2014 02:07 PM, Petr Viktorin wrote:
On 09/24/2014 01:54 PM, Petr Spacek wrote:
On 24.9.2014 13:47, Petr Viktorin wrote:
On 09/23/2014 06:00 PM, Petr Spacek wrote:
On 22.9.2014 14:09, Petr Viktorin wrote:
On 09/22/2014 01:48 PM, Petr Spacek wrote:
On 22.9.2014 10:38, Martin Kosek wrote:
On 09/22/2014 10:31 AM, Petr Spacek wrote:
On 22.9.2014 10:14, Martin Kosek wrote:
On 09/19/2014 07:29 PM, Petr Viktorin wrote:
https://fedorahosted.org/freeipa/ticket/4551
See ticket commit message for details.
Shouldn't we add a 1 sec sleep between tries? Wouldn't current
version just
hammer DNS server with as many DNS queries as it can send?
[...]
LGTM except one nitpick I didn't see before:
+if not options.wait_for_dns or self.check_dns(replica_fqdn):
+self.log.debug('%s A/ record resolvable',
replica_fqdn)
+return
This will print message
'%s A/ record resolvable', replica_fqdn
even if I use switch --no-wait-for-dns
This is sooo minor detail that it can be deferred, please open a ticket
if you don't plan to send new version of the patch.
You're right.
Let's do it correctly now; this isn't worth the overhead of a ticket.
Based on discussion more with Petr, I extended the list of handled
exceptions.
ACK, it works even in corner cases like YXDOMAIN* answer and the like.
* Some name that ought not to exist, does exist. [RFC2136]
Thanks for the review!
Pushed to:
master: ffe4417c630537b1fd51292c86877fbea66862fb
ipa-4-1: ee4a023cf1d2cf5f3d10386979d00d96410e9e80
ipa-4-0: 179423761eb297dd62f0fa9bc33a4aa849d8bb34
--
Petr³
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0118] Allow to disable service (in LDAP)
On 22/09/14 19:30, Martin Basti wrote:
On 19/09/14 14:47, Jan Cholasta wrote:
Dne 19.9.2014 v 13:33 Martin Basti napsal(a):
On 02/09/14 11:59, Martin Basti wrote:
On 02/09/14 09:10, Jan Cholasta wrote:
Hi,
Dne 1.9.2014 v 16:57 Martin Basti napsal(a):
This patch allows to disable service in LDAP to prevents service
to be
started by ipactl restart
Required by DNSSEC
Patch attached
I don't think the extra argument in ldap_enable is necessary. It
should enable the service no matter if the entry existed before or
not.
Similarly, in ldap_disable you should not raise an error when the
entry is not found, because that already makes the service disabled.
Honza
Updated patch attached
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Please review, this should be in 4.1
--
Martin Basti
1) ipaConfigString is case-insensitive, so you need to look for the
string enabledService in a case-insensitive way.
2) Don't say failed to enable ... when the service is already
enabled. It is not a failure. Same for disabled.
3) Missing ipaConfigString is nothing to warn about.
4) You should log success in both ldap_enable/ldap_disable.
5) The except below update_entry should say except
errors.EmptyModlist.
Thank you,
updated patch attached
Updated patch attached. I modified ldap_disable to use search function.
--
Martin Basti
From 045137a10a7b003f92132c96fa8b341eeaf2d95c Mon Sep 17 00:00:00 2001
From: Martin Basti mba...@redhat.com
Date: Thu, 28 Aug 2014 19:27:44 +0200
Subject: [PATCH] LDAP disable service
This patch allows to disable service in LDAP (ipactl will not start it)
---
ipaserver/install/service.py | 67 +++-
1 file changed, 66 insertions(+), 1 deletion(-)
diff --git a/ipaserver/install/service.py b/ipaserver/install/service.py
index 8fb802099d7e58a10fd8fb17ff5dc7511eb98c7f..d095fe041bec8098937bf5b99b301a41842ce53e 100644
--- a/ipaserver/install/service.py
+++ b/ipaserver/install/service.py
@@ -392,6 +392,32 @@ class Service(object):
self.ldap_connect()
entry_name = DN(('cn', name), ('cn', fqdn), ('cn', 'masters'), ('cn', 'ipa'), ('cn', 'etc'), ldap_suffix)
+
+# enable disabled service
+try:
+entry = self.admin_conn.get_entry(entry_name, ['ipaConfigString'])
+except errors.NotFound:
+pass
+else:
+if any(u'enabledservice' == val.lower()
+ for val in entry.get('ipaConfigString', [])):
+root_logger.debug(service %s startup entry already enabled, name)
+return
+
+entry.setdefault('ipaConfigString', []).append(u'enabledService')
+
+try:
+self.admin_conn.update_entry(entry)
+except errors.EmptyModlist:
+root_logger.debug(service %s startup entry already enabled, name)
+return
+except:
+root_logger.debug(failed to enable service %s startup entry, name)
+raise
+
+root_logger.debug(service %s startup entry enabled, name)
+return
+
order = SERVICE_LIST[name][1]
entry = self.admin_conn.make_entry(
entry_name,
@@ -404,9 +430,48 @@ class Service(object):
try:
self.admin_conn.add_entry(entry)
except (errors.DuplicateEntry), e:
-root_logger.debug(failed to add %s Service startup entry % name)
+root_logger.debug(failed to add service %s startup entry, name)
raise e
+def ldap_disable(self, name, fqdn, ldap_suffix):
+assert isinstance(ldap_suffix, DN)
+if not self.admin_conn:
+self.ldap_connect()
+
+entry_dn = DN(('cn', name), ('cn', fqdn), ('cn', 'masters'),
+('cn', 'ipa'), ('cn', 'etc'), ldap_suffix)
+search_kw = {'ipaConfigString': u'enabledService'}
+filter = self.admin_conn.make_filter(search_kw)
+try:
+entries, truncated = self.admin_conn.find_entries(
+filter=filter,
+attrs_list=['ipaConfigString'],
+base_dn=entry_dn,
+scope=self.admin_conn.SCOPE_BASE)
+except errors.NotFound:
+root_logger.debug(service %s startup entry already disabled, name)
+return
+
+assert len(entries) == 1 # only one entry is expected
+entry = entries[0]
+
+# case insensitive
+for value in entry.get('ipaConfigString', []):
+if value.lower() == u'enabledservice':
+entry['ipaConfigString'].remove(value)
+break
+
+try:
+self.admin_conn.update_entry(entry)
+except errors.EmptyModlist:
+pass
+except:
+root_logger.debug(failed to disable service %s startup
Re: [Freeipa-devel] [PATCH] 0009 Detect and configure all usable IP addresses.
On 09/24/2014 08:54 PM, Martin Basti wrote: On 24/09/14 15:44, David Kupka wrote: On 09/23/2014 08:25 PM, Martin Basti wrote: On 23/09/14 13:23, David Kupka wrote: On 09/18/2014 06:34 PM, Martin Basti wrote: ... 1) +if options.unattended: +for ip in ip_addresses: +if search_reverse_zones and find_reverse_zone(str(ip)): +# reverse zone is already in LDAP +continue +for rz in ret_reverse_zones: +if verify_reverse_zone(rz, ip): +# reverse zone was entered by user +break +else: +rz = get_reverse_zone_default(str(ip)) +ret_reverse_zones.append(rz) +elif options.reverse_zones or create_reverse(): +for ip in ip_addresses: +if search_reverse_zones and find_reverse_zone(str(ip)): +# reverse zone is already in LDAP +continue +for rz in ret_reverse_zones: +if verify_reverse_zone(rz, ip): +# reverse zone was entered by user +break +else: +rz = get_reverse_zone_default(str(ip)) +rz = read_reverse_zone(rz, str(ip)) +ret_reverse_zones.append(rz) +else: +options.no_reverse = True +ret_reverse_zones = [] You can make it shorter without duplications: # this ifs can be in one line if not options.unatended: if not options.reverse_zones if not create_reverse(): options.no_reverse=True return [] for ip in ip_addresses: if search_reverse_zones and find_reverse_zone(str(ip)): # reverse zone is already in LDAP continue for rz in ret_reverse_zones: if verify_reverse_zone(rz, ip): # reverse zone was entered by user break else: rz = get_reverse_zone_default(str(ip)) if not options.unattended: rz = read_reverse_zone(rz, str(ip)) ret_reverse_zones.append(rz) Thanks, I modified it bit different way to alse address recommendation 3). 2) Typo? There is no IP address matching reverze_zone %s. -^^ Thanks, fixed. 3) Would be nice to ask user to create new zones only if new zones are required. (If all required zones exist in LDAP, you ask user anyway) I added one more variable and ask only once. 4) Ask framework gurus, if installutils module is better place for function above Petr^3 said that it's ok to have it in bindinstance.py. NACK, most important point is 7 1) I'm not sure if this is bug, but interactively is allowed to add only one ip address Unable to resolve IP address for host name Please provide the IP address to be used for this host name: 2001:db8::2 The kerberos protocol requires a Realm name to be defined. For the sake of infinite usability and UX I rewrote it to ask for multiple addresses the same way as for DNS forwarders. Also I really simplified IP address checking code when I was in it. I tested it but please look at it carefully. Also I found that ipa-dns-install and ipa-adtrust-install also accept --ip-address param. So I modified ipa-dns-install in the same way as ipa-server-install and ipa-replica-install. After discussion with tbabej I decided to dont touch ipa-adtrust-install now as it do not use specified value at all. I will remove the processing code and mark the param as deprecated in separate patch. 2) I'm getting error message Invalid reverse zone 10.in-addr.arpa. for IP address 2001:db8::dead:beef Invalid reverse zone 10.in-addr.arpa. for IP address fed0:babe:baab:0:21a:4aff:fe10:4e18 - or - Do you want to configure the reverse zone? [yes]: Please specify the reverse zone name [0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa.]: Invalid reverse zone 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa. for IP address fed0:babe:baab:0:21a:4aff:fe10:4e18 Please specify the reverse zone name [0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa.]: Using reverse zone(s) 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa., 0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa. This shouldn't be there Moved the message to function used when installation is attended by user. Could be better to ask user to specific zone for ip address a.b.c.d. Probably, but lets leave some work for future. 4) just nitpick The IPA Master Server will be configured with: ... IP address(es): 2001:db8::dead:beef, fed0:babe:baab:0:21a:4aff:fe10:4e18 ... Reverse zone: 0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.ip6.arpa., 0.0.0.0.b.a.a.b.e.b.a.b.0.d.e.f.ip6.arpa. You have label IP address(es), so you should use label Reverse zone(s) Fixed. 5) ipa-server-install --ip-address=10.16.78.105 --reverse-zone=10.in-addr.arpa. --reverse-zone=16.10.in-addr.arpa.
Re: [Freeipa-devel] [PATCHES 0114-0115] DNS: allow to add root zone '.'
On 25.9.2014 10:31, Martin Basti wrote: On 24/09/14 16:24, Martin Basti wrote: On 24/09/14 16:05, Martin Basti wrote: On 23/09/14 17:45, Petr Vobornik wrote: On 25.8.2014 14:52, Martin Basti wrote: Patches attached. Ticket: https://fedorahosted.org/freeipa/ticket/4149 There is a bug in bind-dyndb-ldap (or worse in dirsrv), which cause the named service is stopped after deleting zone. Bug ticket: https://fedorahosted.org/bind-dyndb-ldap/ticket/138 Review of: http://www.redhat.com/archives/freeipa-devel/2014-September/msg00484.html 1. Please follow pep8 for the new code. # git diff HEAD~7 -U0 | pep8 --diff --ignore=E501 Produces 25 erros. Only E124 and E128 could be ignored if they are part of old code. I left there some pep8 errors. They don't decrease readability Patch 120: 3. This patch uses term 'deprecated' in a different meaning than a DeprecatedParam. It creates inconsistency - future confusion. IMHO this usage is correct since the usual understanding of deprecation is that the param is still usable but user should be prepared that it will be removed in a future. IMHO DeprecatedParam is badly designed but that's not an issue of this patch. I think we can leave this as is and create a ticket to rename DeprecatedParam e.g. to RemovedParam. What do you think? https://fedorahosted.org/freeipa/ticket/4566 5. You've removed 'idnssoamname' and 'force' from Web UI but dnszone-add precallback still uses these params. What is the intended purpose? User should use modify dialog in webUI for zones. Precallback fills default value for idnsmname from LDAP. with --force there will be no validation of user specified soa mname Purpose is a user should let IPA to fill mname with safe value. Patch 123: 10. In `normalize_zonemgr(zonemgr)`, if zonemgr contains '@' shouldn't it be normalized to contain '.' at the end? Or is it handled by bind-dyndb-ldap? Zone manager (SOA RNAME) can eb relative name, BIND will append zone name. Currently we cant validate if email address is reachable, it doestn matter if it is filled with nonexistent absolute name, or nonexistent relative name. Unrelated to this patch set: a. One is able to run: # ipa dnszone-remove-permission $zone multiple times and it always returns success Is it intentional? No, it isn't. I will inspect it and I will send additional patch b. Web UI doesn't have means to call dnszone-mod with --force option I'm not sure what you mean, it didn't do that before my patches. Updated patches attached I accidentally removed one line in previous patchset. Updated patches attached Sorry my IDE was too smart, and somehow added its configuration file to commit and I didn't notice it. Patches attached. ACK, it works for me. Replica installation and deletion properly adds and deletes records as necessary. I would defer further improvements to https://fedorahosted.org/freeipa/ticket/3343 -- Petr^2 Spacek ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES 0114-0115] DNS: allow to add root zone '.'
On 09/25/2014 04:32 PM, Petr Spacek wrote: On 25.9.2014 10:31, Martin Basti wrote: On 24/09/14 16:24, Martin Basti wrote: On 24/09/14 16:05, Martin Basti wrote: On 23/09/14 17:45, Petr Vobornik wrote: On 25.8.2014 14:52, Martin Basti wrote: Patches attached. Ticket: https://fedorahosted.org/freeipa/ticket/4149 There is a bug in bind-dyndb-ldap (or worse in dirsrv), which cause the named service is stopped after deleting zone. Bug ticket: https://fedorahosted.org/bind-dyndb-ldap/ticket/138 Review of: http://www.redhat.com/archives/freeipa-devel/2014-September/msg00484.html 1. Please follow pep8 for the new code. # git diff HEAD~7 -U0 | pep8 --diff --ignore=E501 Produces 25 erros. Only E124 and E128 could be ignored if they are part of old code. I left there some pep8 errors. They don't decrease readability Patch 120: 3. This patch uses term 'deprecated' in a different meaning than a DeprecatedParam. It creates inconsistency - future confusion. IMHO this usage is correct since the usual understanding of deprecation is that the param is still usable but user should be prepared that it will be removed in a future. IMHO DeprecatedParam is badly designed but that's not an issue of this patch. I think we can leave this as is and create a ticket to rename DeprecatedParam e.g. to RemovedParam. What do you think? https://fedorahosted.org/freeipa/ticket/4566 5. You've removed 'idnssoamname' and 'force' from Web UI but dnszone-add precallback still uses these params. What is the intended purpose? User should use modify dialog in webUI for zones. Precallback fills default value for idnsmname from LDAP. with --force there will be no validation of user specified soa mname Purpose is a user should let IPA to fill mname with safe value. Patch 123: 10. In `normalize_zonemgr(zonemgr)`, if zonemgr contains '@' shouldn't it be normalized to contain '.' at the end? Or is it handled by bind-dyndb-ldap? Zone manager (SOA RNAME) can eb relative name, BIND will append zone name. Currently we cant validate if email address is reachable, it doestn matter if it is filled with nonexistent absolute name, or nonexistent relative name. Unrelated to this patch set: a. One is able to run: # ipa dnszone-remove-permission $zone multiple times and it always returns success Is it intentional? No, it isn't. I will inspect it and I will send additional patch b. Web UI doesn't have means to call dnszone-mod with --force option I'm not sure what you mean, it didn't do that before my patches. Updated patches attached I accidentally removed one line in previous patchset. Updated patches attached Sorry my IDE was too smart, and somehow added its configuration file to commit and I didn't notice it. Patches attached. ACK, it works for me. Replica installation and deletion properly adds and deletes records as necessary. I would defer further improvements to https://fedorahosted.org/freeipa/ticket/3343 Pushed to: ipa-4-1: b7e3a990369d85dfd12165891cf9142d669a0259 master: bc2eaa145637e1947449ee53548243ab22059805 -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 749-754 webui: new ID views section
On 9/25/2014 2:25 AM, Alexander Bokovoy wrote: On Wed, 24 Sep 2014, Endi Sukma Dewata wrote: 4. If I understand correctly the description field for the User ID Overrides and Group ID Overrides should be optional too because it's also used to optionally override the description attribute of the original entry. No, this is description of the override itself. We don't want to override original description field, if any, we want to provide a way to document why this override was done. In that case the 'description' probably should have been a MUST. objectClasses: (2.16.840.1.113730.3.8.12.30 NAME 'ipaOverrideAnchor' SUP top STRUCTURAL MUST ( ipaAnchorUUID ) MAY ( description ) X-ORIGIN 'IPA v4' ) BTW, the LDAP schema in the wiki page is outdated: http://www.freeipa.org/page/V4/Migrating_existing_environments_to_Trust 6. Can multiple ID views be applied to the same host? Does the order matter? If so, how would the UI manage the order? No. Single ID view per host. The scheme is actually a bit more complex: - IPA users: data from main tree is overridden with a data from a host-specific ID view - AD users: data from AD is overridden by a data from a default trust view which is then overridden by a data from a host-specific ID view OK, right now if I apply an ID view to a host that already uses another ID view, the host will be removed silently from the other ID view. Should the operation fail/provide a warning if the host already uses another ID view? 7. Related to #6, there probably should be a tab in the Host details page showing which ID views apply to it. There is only a single view and yes, it would be good to add a property there, linking it to the ID view tab, if possible. I think that field should be editable as well so you can select the ID view from Host details page. 9. This probably requires server support. In the Apply to hosts association dialog, if a host is already added it will still appear in the dialog box. As a comparison, a User that has been added into a User Group will not appear in the association dialog anymore. Could be trivially filtered out on Web UI side. It may not be possible if the list of hosts is paged. The UI will not get the full list of hosts, so it won't be able to filter out hosts that are already added but not currently displayed. I'm not sure how important is this, but we did this for some other pages. Since #4 is not a UI issue, patch #754 is ACKed. Other issues can be addressed later. -- Endi S. Dewata ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0118] Allow to disable service (in LDAP)
Dne 25.9.2014 v 16:15 Martin Basti napsal(a): On 22/09/14 19:30, Martin Basti wrote: On 19/09/14 14:47, Jan Cholasta wrote: Dne 19.9.2014 v 13:33 Martin Basti napsal(a): On 02/09/14 11:59, Martin Basti wrote: On 02/09/14 09:10, Jan Cholasta wrote: Hi, Dne 1.9.2014 v 16:57 Martin Basti napsal(a): This patch allows to disable service in LDAP to prevents service to be started by ipactl restart Required by DNSSEC Patch attached I don't think the extra argument in ldap_enable is necessary. It should enable the service no matter if the entry existed before or not. Similarly, in ldap_disable you should not raise an error when the entry is not found, because that already makes the service disabled. Honza Updated patch attached ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Please review, this should be in 4.1 -- Martin Basti 1) ipaConfigString is case-insensitive, so you need to look for the string enabledService in a case-insensitive way. 2) Don't say failed to enable ... when the service is already enabled. It is not a failure. Same for disabled. 3) Missing ipaConfigString is nothing to warn about. 4) You should log success in both ldap_enable/ldap_disable. 5) The except below update_entry should say except errors.EmptyModlist. Thank you, updated patch attached Updated patch attached. I modified ldap_disable to use search function. ACK. -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES 0114-0115] DNS: allow to add root zone '.'
On 09/25/2014 04:39 PM, Petr Viktorin wrote: On 09/25/2014 04:32 PM, Petr Spacek wrote: On 25.9.2014 10:31, Martin Basti wrote: On 24/09/14 16:24, Martin Basti wrote: On 24/09/14 16:05, Martin Basti wrote: On 23/09/14 17:45, Petr Vobornik wrote: On 25.8.2014 14:52, Martin Basti wrote: Patches attached. Ticket: https://fedorahosted.org/freeipa/ticket/4149 There is a bug in bind-dyndb-ldap (or worse in dirsrv), which cause the named service is stopped after deleting zone. Bug ticket: https://fedorahosted.org/bind-dyndb-ldap/ticket/138 Review of: http://www.redhat.com/archives/freeipa-devel/2014-September/msg00484.html 1. Please follow pep8 for the new code. # git diff HEAD~7 -U0 | pep8 --diff --ignore=E501 Produces 25 erros. Only E124 and E128 could be ignored if they are part of old code. I left there some pep8 errors. They don't decrease readability Patch 120: 3. This patch uses term 'deprecated' in a different meaning than a DeprecatedParam. It creates inconsistency - future confusion. IMHO this usage is correct since the usual understanding of deprecation is that the param is still usable but user should be prepared that it will be removed in a future. IMHO DeprecatedParam is badly designed but that's not an issue of this patch. I think we can leave this as is and create a ticket to rename DeprecatedParam e.g. to RemovedParam. What do you think? https://fedorahosted.org/freeipa/ticket/4566 5. You've removed 'idnssoamname' and 'force' from Web UI but dnszone-add precallback still uses these params. What is the intended purpose? User should use modify dialog in webUI for zones. Precallback fills default value for idnsmname from LDAP. with --force there will be no validation of user specified soa mname Purpose is a user should let IPA to fill mname with safe value. Patch 123: 10. In `normalize_zonemgr(zonemgr)`, if zonemgr contains '@' shouldn't it be normalized to contain '.' at the end? Or is it handled by bind-dyndb-ldap? Zone manager (SOA RNAME) can eb relative name, BIND will append zone name. Currently we cant validate if email address is reachable, it doestn matter if it is filled with nonexistent absolute name, or nonexistent relative name. Unrelated to this patch set: a. One is able to run: # ipa dnszone-remove-permission $zone multiple times and it always returns success Is it intentional? No, it isn't. I will inspect it and I will send additional patch b. Web UI doesn't have means to call dnszone-mod with --force option I'm not sure what you mean, it didn't do that before my patches. Updated patches attached I accidentally removed one line in previous patchset. Updated patches attached Sorry my IDE was too smart, and somehow added its configuration file to commit and I didn't notice it. Patches attached. ACK, it works for me. Replica installation and deletion properly adds and deletes records as necessary. I would defer further improvements to https://fedorahosted.org/freeipa/ticket/3343 Pushed to: ipa-4-1: b7e3a990369d85dfd12165891cf9142d669a0259 master: bc2eaa145637e1947449ee53548243ab22059805 I reopened the ticket, we missed update to DNS help page (ipa help dns): https://fedorahosted.org/freeipa/ticket/4149#comment:18 Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 749-754 webui: new ID views section
Note: I'll send response also to previous mail. On 25.9.2014 16:40, Endi Sukma Dewata wrote: On 9/25/2014 2:25 AM, Alexander Bokovoy wrote: On Wed, 24 Sep 2014, Endi Sukma Dewata wrote: 4. If I understand correctly the description field for the User ID Overrides and Group ID Overrides should be optional too because it's also used to optionally override the description attribute of the original entry. No, this is description of the override itself. We don't want to override original description field, if any, we want to provide a way to document why this override was done. In that case the 'description' probably should have been a MUST. objectClasses: (2.16.840.1.113730.3.8.12.30 NAME 'ipaOverrideAnchor' SUP top STRUCTURAL MUST ( ipaAnchorUUID ) MAY ( description ) X-ORIGIN 'IPA v4' ) BTW, the LDAP schema in the wiki page is outdated: http://www.freeipa.org/page/V4/Migrating_existing_environments_to_Trust New server version which is being developed by Tomas will not have description required. 6. Can multiple ID views be applied to the same host? Does the order matter? If so, how would the UI manage the order? No. Single ID view per host. The scheme is actually a bit more complex: - IPA users: data from main tree is overridden with a data from a host-specific ID view - AD users: data from AD is overridden by a data from a default trust view which is then overridden by a data from a host-specific ID view OK, right now if I apply an ID view to a host that already uses another ID view, the host will be removed silently from the other ID view. Should the operation fail/provide a warning if the host already uses another ID view? If something then IMHO warning is better. 7. Related to #6, there probably should be a tab in the Host details page showing which ID views apply to it. There is only a single view and yes, it would be good to add a property there, linking it to the ID view tab, if possible. I think that field should be editable as well so you can select the ID view from Host details page. I'll add readonly field yet. Editable is a bigger task which we don't have time for atm. But should done in later versions. 9. This probably requires server support. In the Apply to hosts association dialog, if a host is already added it will still appear in the dialog box. As a comparison, a User that has been added into a User Group will not appear in the association dialog anymore. Could be trivially filtered out on Web UI side. It may not be possible if the list of hosts is paged. The UI will not get the full list of hosts, so it won't be able to filter out hosts that are already added but not currently displayed. I'm not sure how important is this, but we did this for some other pages. UI gets all hosts the view is applied on, so it can be filtered. Since #4 is not a UI issue, patch #754 is ACKed. Other issues can be addressed later. OK I will addressed(am addressing) other issues separately so this patch doesn't have to be reviewed multiple times. -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 749-754 webui: new ID views section
All issues will be done separately as already stated in other sub-thread. I've removed issues which are discussed in the other sub-thread. On 25.9.2014 09:25, Alexander Bokovoy wrote: On Wed, 24 Sep 2014, Endi Sukma Dewata wrote: OK, some comments/questions: 1. For consistency, the ID view should be capitalized into ID View in the navigation tab, page title, and dialog title. See ID Ranges as an example. Will be fixed in a new iteration of the server plugin. UI will use it automatically. 2. The tab titles in the ID view details page are quite long, and the User ID overrides and Group ID overrides labels aren't quite appropriate because the ID view can override other attributes too. How about using facet groups like in User Groups? For example: - ID view applies to: - Hosts - ID view overrides: - Users - Groups - Settings Will add. 3. Since the tab already says Applied to hosts, the current button labels is kind of redundant. How about renaming and reorder the buttons like this? - Refresh - Remove - Add - Add hosts in host group - Remove hosts in host group I agree that it's a little bit redundant. But I think that they describe the operation better. In other association facets the buttons have 'add' and 'remove' titles but they corresponds to 'add-*', 'remove-*' commands. I'm afraid that users would not associate these buttons with idview-(un)apply commands. 4. If I understand correctly the description field for the User ID... Discussed in other thread... In any way, UI reflects API. 5. Not sure if this is a problem. The search field in User/Group ID Overrides can be used to find the overriding attributes, but not the User/Group to override. I already reported it to Tomas. The issue is that the override is saved in LDAP in a UUID form (more or less) and so it doesn't contain related user login or group name. Might be fixed in other iteration of server part. 7. Related to #6, there probably should be a tab in the Host details page showing which ID views apply to it. There is only a single view and yes, it would be good to add a property there, linking it to the ID view tab, if possible. Will add simple readonly field (link to view). It will be improved later (based on ipa-4-1 priorities) 9. This probably requires server support. In the Apply to hosts association dialog, if a host is already added it will still appear in the dialog box. As a comparison, a User that has been added into a User Group will not appear in the association dialog anymore. Could be trivially filtered out on Web UI side. Will be implemented. -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 755 webui-ci: case-insensitive record check
On Thu, Sep 25, 2014 at 09:44:03AM +0200, Petr Viktorin wrote: On 09/25/2014 03:30 AM, Fraser Tweedale wrote: On Wed, Sep 24, 2014 at 09:16:52AM -0500, Endi Sukma Dewata wrote: On 9/24/2014 8:26 AM, Petr Vobornik wrote: On 24.9.2014 04:43, Endi Sukma Dewata wrote: On 9/22/2014 9:49 AM, Petr Vobornik wrote: [PATCH] webui-ci: case-insensitive record check Indirect association are no longer lower cased, which caused a issue in CI. Is the use of |= operator intentional? I don't see the has variable defined anywhere else in this method. has |= self.has_record(pkey.lower(), parent, table_name) If this has been tested to work, then ACK. it's defined on the previous line: has = self.has_record(pkey, parent, table_name) has |= self.has_record(pkey.lower(), parent, table_name) Ah, I see. I thought the old line was being replaced. ACK. IMO the following reads better:: has = self.has_record(pkey, parent, table_name) \ || self.has_record(pkey.lower(), parent, table_name) (Just a comment - no reason to block the patch.) Feel free to push the patch as it is, but since we're debating style... Write this: has_record = (self.has_record(pkey, parent, table_name) or self.has_record(pkey.lower(), parent, table_name)) Hah, yes. What I wrote is not even Python! I have been writing too much Java it seems -_- For boolean values, you should prefer `or` and `and` over the bitwise operators, since truthy/falsy values might not be just booleans. It's better illustrated with `and`: `3 and 8` is true, but `3 8` is false. And from PEP8: - The preferred way of wrapping long lines is by using Python's implied line continuation inside parentheses, brackets and braces. Long lines can be broken over multiple lines by wrapping expressions in parentheses. These should be used in preference to using a backslash for line continuation. - The preferred place to break around a binary operator is after the operator, not before it. -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
