Re: [Freeipa-devel] [PATCH] 0005 Add list of domains associated to our realm to cn=etc
On Wed, 13 Feb 2013, Alexander Bokovoy wrote: On Tue, 12 Feb 2013, Ana Krivokapic wrote: Add new LDAP container to store the list of domains associated with IPA realm. Add two new ipa commands (ipa realmdomains-show and ipa realmdomains-mod) to allow manipulation of the list of realm domains. Unit test file covering these new commands was added. https://fedorahosted.org/freeipa/ticket/2945 ACK, works perfectly. We need to decide on the questions still open in the http://www.freeipa.org/page/V3/Realm_Domains but the decision should not prevent this work from being committed. Committed to master along with a one-liner that limits ipasam to look only at $SUFFIX with base scope when searching for its own domain name as now we use domainRelatedObject in two different places and previous subtree search now gives too wide answer. -- / Alexander Bokovoy ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 255 Added Web UI support for service PAC type option: NONE
On Thu, Feb 14, 2013 at 09:56:44AM -0600, Endi Sukma Dewata wrote: > On 2/14/2013 6:30 AM, Petr Vobornik wrote: > >>If they are mutually exclusive, they probably should be separated using > >>radio buttons like this: > >> > >> PAC: ( ) None > >>(o) Type: > >>[x] MS-PAC > >>[ ] PAD > > > >You missed one option: nothing selected. It can be solved by adding '( > >) Inherited' radio. > > I wouldn't have guessed that :) I agree we should add the > 'Inherited' option. > Would it be possible to print the inherited values here, i.e. the default values given in the ipakrbauthzdata attribute of ipaGuiConfig object? I think this would improve the user experience because you do not have to remember/look up the default values. bye, Sumit ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 1085 cert-find command
On 02/18/2013 08:39 PM, Rob Crittenden wrote: Rob Crittenden wrote: Petr Viktorin wrote: On 02/15/2013 10:43 PM, Rob Crittenden wrote: Petr Viktorin wrote: On 02/06/2013 07:23 PM, Rob Crittenden wrote: Petr Viktorin wrote: On 02/06/2013 12:44 AM, Rob Crittenden wrote: This adds a cert-find command for the dogtag backend. Searches can be done by serial number, by subject, revocation reason, issue date, notbefore, notafter and revocation dates. I added some basic tests for this. I made it a separate test file because the cert plugin tests do not use the declarative format and rely on the selfsign backend by default. rob Thanks! The code works well, but I found a few issues. These tests don't work when the full test suite is run: test_cert adds and revokes additional certs that throw the code off. Perhaps have the tests only query valid certs? I don't see that option but I think it would be helpful to support. I added some rather nasty hacks to the test to make things pass. I limit the search to 10 certificates, which is the number start with by default. There is an open dogtag ticket to return only VALID certificates so we should be able to drop this eventually. I had to go further on one of the revocation tests, limiting it to a sizelimit of 1. The count changes every time the suite runs so this is the safest for now. It also means that one test will fail if this is the only part of the suite executed. This gets rid of most of the failures, but it still fails the "certs for this IPA server/short name" tests if the cert from ./make-cert is present (creating it is part of `make test`). If make-cert is run more times, it'll revoke the previous cert, so the test for revocation reason 4 fails as well. It looks like when using sizelimit Dogtag will always discard *newer* certs, ones with higher serials. Is it documented behavior or does Dogtag just happen to do that? It isn't documented anywhere I could find, it is just what dogtag returns I wonder how other people run their tests. This solution looks like it could break easily if people do something differently :( I'm not sure how to solve this properly. Perhaps not using Declarative, and checking "by hand" that the wanted certs are in the response and the unwanted ones are not, would work better. I ended up switching the test class. It is not a very fine-grained set of tests, mostly searching with limits and verifying that we fall within a reasonable range, but it is better than nothing. rob This works much better, thanks! Just two nitpicks now. The patch doesn't apply well, there's a conflict in VERSION and some added trailing whitespace, AFAIK this would be the only (first?) test that relies on Nose's ordering of test modules -- tests 0011 and 0030 rely on the other cert tests running first. Please at least mention that in a comment. Or better, move class test_cert_find to test_cert.py Rebased the patch and removed whitespace. I went ahead and combined this with the existing test_cert file. Originally test_cert was only tested against lite-server but since it works against a live dogtag server too it makes sense to combine them. I improved the set up documentation a little bit and tried to handle all the different configurations that one might see so that this should be runnable against either a live server or the lite-server for both the selfsign and dogtag backends. This relies on the user configuring ~/.ipa/default.conf to match the remote server. There is no way from a client to know what kind of CA backend a server is running. rob And the patch. rob ACK, thank you! -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 0177-0179 Add missing dict methods to CIDict
Hi,
On 5.2.2013 18:02, Petr Viktorin wrote:
CIDict, our case-insensitive dictionary, inherits from dict but did not
reimplement the full dict interface. Calling the missing methods
silently invoked case-sensitive behavior. Our code seems to avoid that,
but it's a bit of a minefield for new development.
Patch 119 adds the missing dict methods (except
view{items,keys,values}(), which now raise errors), and adds tests.
Can you please also add the (obj, **kwargs) and (**kwargs) variants of
__init__ and update?
Patches 117-118 modernize the testsuite a bit (these have been sitting
in my queue for a while, I think now is a good time to submit them):
The first one moves some old tests from the main code tree to tests/.
(The adtrust_install test wasn't run before, this move makes nose notice
it).
The second converts CIDict's unittest-based suite to nose.
Honza
--
Jan Cholasta
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 90 Run interactive_prompt callbacks after CSV values are split
On 14.2.2013 10:45, Petr Viktorin wrote:
This needs a test; here one I used to check it.
Otherwise it works well, ACK if the test is added.
Thank you, test added.
Honza
--
Jan Cholasta
>From d845724362507c662e45f21396b46ce520f25a45 Mon Sep 17 00:00:00 2001
From: Jan Cholasta
Date: Wed, 9 Jan 2013 18:09:10 +0100
Subject: [PATCH] Run interactive_prompt callbacks after CSV values are split.
https://fedorahosted.org/freeipa/ticket/3334
---
ipalib/cli.py | 16
tests/test_cmdline/test_cli.py | 28
2 files changed, 36 insertions(+), 8 deletions(-)
diff --git a/ipalib/cli.py b/ipalib/cli.py
index ca186c7..3d59e4a 100644
--- a/ipalib/cli.py
+++ b/ipalib/cli.py
@@ -1045,6 +1045,14 @@ class cli(backend.Executioner):
if self.env.interactive:
self.prompt_interactively(cmd, kw)
kw = cmd.split_csv(**kw)
+if self.env.interactive:
+try:
+callbacks = cmd.get_callbacks('interactive_prompt')
+except AttributeError:
+pass
+else:
+for callback in callbacks:
+callback(cmd, kw)
kw['version'] = API_VERSION
self.load_files(cmd, kw)
return kw
@@ -1207,14 +1215,6 @@ class cli(backend.Executioner):
param.label, param.confirm
)
-try:
-callbacks = cmd.get_callbacks('interactive_prompt')
-except AttributeError:
-pass
-else:
-for callback in callbacks:
-callback(cmd, kw)
-
def load_files(self, cmd, kw):
"""
Load files from File parameters.
diff --git a/tests/test_cmdline/test_cli.py b/tests/test_cmdline/test_cli.py
index 06c6124..4d730d5 100644
--- a/tests/test_cmdline/test_cli.py
+++ b/tests/test_cmdline/test_cli.py
@@ -237,3 +237,31 @@ class TestCLIParsing(object):
all=False,
force=False,
version=API_VERSION)
+
+def test_dnsrecord_del_comma(self):
+try:
+self.run_command(
+'dnszone_add', idnsname=u'test-example.com',
+idnssoamname=u'ns.test-example.com', force=True)
+except errors.NotFound:
+raise nose.SkipTest('DNS is not configured')
+try:
+self.run_command(
+'dnsrecord_add',
+dnszoneidnsname=u'test-example.com',
+idnsname=u'test',
+txtrecord=u'"A pretty little problem," said Holmes.')
+with self.fake_stdin('no\nyes\n'):
+self.check_command(
+'dnsrecord_del test-example.com test',
+'dnsrecord_del',
+dnszoneidnsname=u'test-example.com',
+idnsname=u'test',
+del_all=False,
+txtrecord=[u'"A pretty little problem," said Holmes.'],
+structured=False,
+raw=False,
+all=False,
+version=API_VERSION)
+finally:
+self.run_command('dnszone_del', idnsname=u'test-example.com')
--
1.8.1
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 255 Added Web UI support for service PAC type option: NONE
On 02/19/2013 01:40 PM, Sumit Bose wrote: On Thu, Feb 14, 2013 at 09:56:44AM -0600, Endi Sukma Dewata wrote: On 2/14/2013 6:30 AM, Petr Vobornik wrote: If they are mutually exclusive, they probably should be separated using radio buttons like this: PAC: ( ) None (o) Type: [x] MS-PAC [ ] PAD You missed one option: nothing selected. It can be solved by adding '( ) Inherited' radio. I wouldn't have guessed that :) I agree we should add the 'Inherited' option. Would it be possible to print the inherited values here, i.e. the default values given in the ipakrbauthzdata attribute of ipaGuiConfig object? I think this would improve the user experience because you do not have to remember/look up the default values. bye, Sumit It is possible, but not straightforward. I see two options: 1) Currently Web UI loads config at start so we can use it. The disadvantage is that the value might be changed and so the displayed value might be incorrect. 2) Other option is to run config-show along with service-show in a batch to get the up-to-date value. I'm not a big fan of #2 but it is probably better solution. -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 255 Added Web UI support for service PAC type option: NONE
On Tue, Feb 19, 2013 at 02:01:24PM +0100, Petr Vobornik wrote: > On 02/19/2013 01:40 PM, Sumit Bose wrote: > >On Thu, Feb 14, 2013 at 09:56:44AM -0600, Endi Sukma Dewata wrote: > >>On 2/14/2013 6:30 AM, Petr Vobornik wrote: > If they are mutually exclusive, they probably should be separated using > radio buttons like this: > > PAC: ( ) None > (o) Type: > [x] MS-PAC > [ ] PAD > >>> > >>>You missed one option: nothing selected. It can be solved by adding '( > >>>) Inherited' radio. > >> > >>I wouldn't have guessed that :) I agree we should add the > >>'Inherited' option. > >> > > > >Would it be possible to print the inherited values here, i.e. the > >default values given in the ipakrbauthzdata attribute of ipaGuiConfig > >object? I think this would improve the user experience because you do > >not have to remember/look up the default values. > > > >bye, > >Sumit > > > > It is possible, but not straightforward. > > I see two options: > 1) Currently Web UI loads config at start so we can use it. The > disadvantage is that the value might be changed and so the displayed > value might be incorrect. > 2) Other option is to run config-show along with service-show in a > batch to get the up-to-date value. > > I'm not a big fan of #2 but it is probably better solution. I agree with both :-). Since it is not straightforward I guess it would be better not to add this with this ticket but open a new one. Do you agree? bye, Sumit > -- > Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 255 Added Web UI support for service PAC type option: NONE
On 02/19/2013 02:08 PM, Sumit Bose wrote: On Tue, Feb 19, 2013 at 02:01:24PM +0100, Petr Vobornik wrote: On 02/19/2013 01:40 PM, Sumit Bose wrote: On Thu, Feb 14, 2013 at 09:56:44AM -0600, Endi Sukma Dewata wrote: On 2/14/2013 6:30 AM, Petr Vobornik wrote: If they are mutually exclusive, they probably should be separated using radio buttons like this: PAC: ( ) None (o) Type: [x] MS-PAC [ ] PAD You missed one option: nothing selected. It can be solved by adding '( ) Inherited' radio. I wouldn't have guessed that :) I agree we should add the 'Inherited' option. Would it be possible to print the inherited values here, i.e. the default values given in the ipakrbauthzdata attribute of ipaGuiConfig object? I think this would improve the user experience because you do not have to remember/look up the default values. bye, Sumit It is possible, but not straightforward. I see two options: 1) Currently Web UI loads config at start so we can use it. The disadvantage is that the value might be changed and so the displayed value might be incorrect. 2) Other option is to run config-show along with service-show in a batch to get the up-to-date value. I'm not a big fan of #2 but it is probably better solution. I agree with both :-). Since it is not straightforward I guess it would be better not to add this with this ticket but open a new one. Do you agree? Yes bye, Sumit -- Petr Vobornik -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 137-144 LDAP code refactoring (Part 3)
On 29.1.2013 10:21, Jan Cholasta wrote: A patch from this patchset (part 3) causes some of the dns plugin tests to fail (idnsallowdynupdate is missing in dnszone_add output). Honza Patch 143: +assert isinstance(entry_or_dn, DN) +if normalize is None or normalize: +entry_or_dn = self.normalize_dn(entry_or_dn) +entry_attrs = dict(entry_attrs) Can you please return LDAPEntry here as well, i.e. replace dict(entry_attrs) with self.make_entry(entry_or_dn, entry_attrs)? +def delete_entry(self, entry, normalize=None): +"""Delete entry. + +The `normalize` argument does nothing when called with a LDAPEntry. + +The legacy variant is: +delete_entry(dn, normalize=True) +""" I don't think this is right. We don't need to know any of the attributes of an entry to delete it, just its DN. I think we should keep the DN variant of delete_entry as the primary one. Honza -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 255 Added Web UI support for service PAC type option: NONE
On Tue, Feb 19, 2013 at 02:12:24PM +0100, Petr Vobornik wrote: > On 02/19/2013 02:08 PM, Sumit Bose wrote: > >On Tue, Feb 19, 2013 at 02:01:24PM +0100, Petr Vobornik wrote: > >>On 02/19/2013 01:40 PM, Sumit Bose wrote: > >>>On Thu, Feb 14, 2013 at 09:56:44AM -0600, Endi Sukma Dewata wrote: > On 2/14/2013 6:30 AM, Petr Vobornik wrote: > >>If they are mutually exclusive, they probably should be separated using > >>radio buttons like this: > >> > >> PAC: ( ) None > >>(o) Type: > >>[x] MS-PAC > >>[ ] PAD > > > >You missed one option: nothing selected. It can be solved by adding '( > >) Inherited' radio. > > I wouldn't have guessed that :) I agree we should add the > 'Inherited' option. > > >>> > >>>Would it be possible to print the inherited values here, i.e. the > >>>default values given in the ipakrbauthzdata attribute of ipaGuiConfig > >>>object? I think this would improve the user experience because you do > >>>not have to remember/look up the default values. > >>> > >>>bye, > >>>Sumit > >>> > >> > >>It is possible, but not straightforward. > >> > >>I see two options: > >> 1) Currently Web UI loads config at start so we can use it. The > >>disadvantage is that the value might be changed and so the displayed > >>value might be incorrect. > >> 2) Other option is to run config-show along with service-show in a > >>batch to get the up-to-date value. > >> > >>I'm not a big fan of #2 but it is probably better solution. > >I agree with both :-). Since it is not straightforward I guess it would > >be better not to add this with this ticket but open a new one. Do you > >agree? > > Yes Thanks, I've opened https://fedorahosted.org/freeipa/ticket/3436 . bye, Sumit > > > > >bye, > >Sumit > > > >>-- > >>Petr Vobornik > > > -- > Petr Vobornik ___ 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 15:38, Petr Viktorin wrote: Alright, I renamed get_single to single_value(). I also rebased to current master. Patch 152: +def single_value(self, name, default=_missing): +values = self.get(name, [default]) +if len(values) != 1: +raise ValueError( +'%s has %s values, one expected' % (name, len(values))) +if values[0] is _missing: +raise KeyError(name) +return values[0] I would prefer if you used __getitem__() instead of get() and re-raised KeyError if default is _missing. Also, until we disallow non-lists, we need to check if values actually is list. I.e., do something like this: +def single_value(self, name, default=_missing): +try: +values = super(LDAPEntry, self).__getitem__(self._get_attr_name(name)) +except KeyError: +if default is _missing: +raise +return default +if not isinstance(values, list): +return values +if len(values) != 1: +raise ValueError( +'%s has %s values, one expected' % (name, len(values))) +return values[0] Patch 159: Like I said in my review of your patch 143, I think we should use DNs instead of entries in delete_entry, so I think it would make sense to do delete_entry(entry.dn) in this patch. Patch 160: I think you should do this (replace % with ,): +root_logger.debug( +"failed to find mapping tree entry for %s", self.suffix) Honza -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 90 Run interactive_prompt callbacks after CSV values are split
On 02/19/2013 01:57 PM, Jan Cholasta wrote: On 14.2.2013 10:45, Petr Viktorin wrote: This needs a test; here one I used to check it. Otherwise it works well, ACK if the test is added. Thank you, test added. Honza ACK -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 137-144 LDAP code refactoring (Part 3)
On 02/19/2013 02:17 PM, Jan Cholasta wrote:
On 29.1.2013 10:21, Jan Cholasta wrote:
A patch from this patchset (part 3) causes some of the dns plugin tests
to fail (idnsallowdynupdate is missing in dnszone_add output).
Honza
Patch 143:
+assert isinstance(entry_or_dn, DN)
+if normalize is None or normalize:
+entry_or_dn = self.normalize_dn(entry_or_dn)
+entry_attrs = dict(entry_attrs)
Can you please return LDAPEntry here as well, i.e. replace
dict(entry_attrs) with self.make_entry(entry_or_dn, entry_attrs)?
Sure. I tried to keep the old behavior with old usage, but you're right,
using LDAPEntry here will be better.
+def delete_entry(self, entry, normalize=None):
+"""Delete entry.
+
+The `normalize` argument does nothing when called with a
LDAPEntry.
+
+The legacy variant is:
+delete_entry(dn, normalize=True)
+"""
I don't think this is right. We don't need to know any of the attributes
of an entry to delete it, just its DN. I think we should keep the DN
variant of delete_entry as the primary one.
Makes sense. I made them both primary.
I didn't bother documenting normalize since your patch will remove that.
Updated patch attached; I'll update my repo when I respond to your
comments on part 4.
--
Petr³
From e0ea08a85d211198a7d745e944bea6d999f2317d Mon Sep 17 00:00:00 2001
From: Petr Viktorin
Date: Wed, 23 Jan 2013 06:38:32 -0500
Subject: [PATCH] Change {add,update,delete}_entry to take LDAPEntries
These methods currently take (dn, entry_attrs, normalize=True)
(or (dn, normalize=True) for delete).
Change them to also accept just an LDAPEntry.
For add and update, document the old style as deprecated.
Part of the work for: https://fedorahosted.org/freeipa/ticket/2660
---
ipaserver/ipaldap.py | 79 +++--
1 files changed, 50 insertions(+), 29 deletions(-)
diff --git a/ipaserver/ipaldap.py b/ipaserver/ipaldap.py
index b496bfbb480f2cc2c7b22397d475d725f7f75eea..95e466f6e8ff6007f7e18ac071cfde5ff3bfa718 100644
--- a/ipaserver/ipaldap.py
+++ b/ipaserver/ipaldap.py
@@ -1364,21 +1364,40 @@ class LDAPConnection(object):
self.log.debug("get_members: result=%s", entries)
return entries
-def add_entry(self, dn, entry_attrs, normalize=True):
-"""Create a new entry."""
-
-assert isinstance(dn, DN)
-
-if normalize:
-dn = self.normalize_dn(dn)
-# remove all None or [] values, python-ldap hates'em
-entry_attrs = dict(
-# FIXME, shouldn't these values be an error?
-(k, v) for (k, v) in entry_attrs.iteritems()
-if v is not None and v != []
-)
+def _get_dn_and_attrs(self, entry_or_dn, entry_attrs, normalize):
+"""Helper for legacy calling style for {add,update}_entry
+"""
+if entry_attrs is None:
+assert normalize is None
+return entry_or_dn.dn, entry_or_dn
+else:
+assert isinstance(entry_or_dn, DN)
+if normalize is None or normalize:
+entry_or_dn = self.normalize_dn(entry_or_dn)
+entry_attrs = self.make_entry(entry_or_dn, entry_attrs)
+for key, value in entry_attrs.items():
+if value is None:
+entry_attrs[key] = []
+return entry_or_dn, entry_attrs
+
+def add_entry(self, entry, entry_attrs=None, normalize=None):
+"""Create a new entry.
+
+This should be called as add_entry(entry).
+
+The legacy two/three-argument variant is:
+add_entry(dn, entry_attrs, normalize=True)
+"""
+dn, attrs = self._get_dn_and_attrs(entry, entry_attrs, normalize)
+
+# remove all [] values (python-ldap hates 'em)
+attrs = dict((k, v) for k, v in attrs.iteritems()
+# FIXME: Once entry values are always lists, this condition can
+# be just "if v":
+if v is not None and v != [])
+
try:
-self.conn.add_s(dn, list(entry_attrs.iteritems()))
+self.conn.add_s(dn, list(attrs.iteritems()))
except _ldap.LDAPError, e:
self.handle_errors(e)
@@ -1465,34 +1484,36 @@ class LDAPConnection(object):
return modlist
-def update_entry(self, dn, entry_attrs, normalize=True):
-"""
-Update entry's attributes.
+def update_entry(self, entry, entry_attrs=None, normalize=None):
+"""Update entry's attributes.
-An attribute value set to None deletes all current values.
-"""
+This should be called as update_entry(entry).
-assert isinstance(dn, DN)
-if normalize:
-dn = self.normalize_dn(dn)
+The legacy two/three-argument variant is:
+update_entry(dn, entry_attrs, normalize=True)
+"""
+dn, attrs = self._get_dn_and_attrs(entry, entry_attrs,
Re: [Freeipa-devel] [PATCH] 90 Run interactive_prompt callbacks after CSV values are split
Petr Viktorin wrote: On 02/19/2013 01:57 PM, Jan Cholasta wrote: On 14.2.2013 10:45, Petr Viktorin wrote: This needs a test; here one I used to check it. Otherwise it works well, ACK if the test is added. Thank you, test added. Honza ACK Pushed to master and ipa-3-1 rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 360 Add autodiscovery section in ipa-client-install man pages
Martin Kosek wrote: On 01/31/2013 04:41 PM, Martin Kosek wrote: On 01/31/2013 02:44 PM, Petr Spacek wrote: On 31.1.2013 13:18, Martin Kosek wrote: Explain how autodiscovery and failover works and which options are important for these elements. https://fedorahosted.org/freeipa/ticket/3383 Could you add some note about "how ipa-client installer will be confused by AD"? One paragraph with some explanation could help. Sure, makes sense. Updated patch attached. Martin Petr noticed a typo in the updated section. Fixed version attached. Martin ACK, pushed to master and ipa-3-1 rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 1087 Some missing v3 schema on upgrades
Hi, On 18.2.2013 22:00, Rob Crittenden wrote: An objectclass and attribute are not being added on upgrades. Missing these causes the UI to not work. I also noticed a typo in the ordering of a number of the trust attributes so fix those as well. rob The patch looks good, but I think errors like this will pop up from time to time, because we have to maintain the same thing in two places - the installation LDIFs and update files. Maybe we should start thinking about merging these two somehow, e.g. using the LDIFs for both installation and updates, with directives for the updater in specially formatted comments. 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
Petr Viktorin wrote: On 02/06/2013 10:55 AM, Jan Cholasta wrote: On 5.2.2013 15:45, Petr Viktorin wrote: On 02/05/2013 01:38 PM, Jan Cholasta wrote: On 4.2.2013 15:49, Petr Viktorin wrote: [...] I see one of the changes is using has_key instead of `in` for a CIDict. Given that dict.has_key() is deprecated, I think a better solution would be to add __contains__ to CIDict. Is there a reason against that? I think a separate patch with this and other changes to make CIDict more like dict would be better. OK, I'll make a patch. [...] Updated patches attached. The changes look good but I can no longer apply the patches. Can you please rebase them? Sure. ACK I think dropping raw=True in patch 99.3 is going to break a check later where we look at the managedby attribute. Without raw this will be managedby_host. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 1085 cert-find command
Petr Viktorin wrote: On 02/18/2013 08:39 PM, Rob Crittenden wrote: Rob Crittenden wrote: Petr Viktorin wrote: On 02/15/2013 10:43 PM, Rob Crittenden wrote: Petr Viktorin wrote: On 02/06/2013 07:23 PM, Rob Crittenden wrote: Petr Viktorin wrote: On 02/06/2013 12:44 AM, Rob Crittenden wrote: This adds a cert-find command for the dogtag backend. Searches can be done by serial number, by subject, revocation reason, issue date, notbefore, notafter and revocation dates. I added some basic tests for this. I made it a separate test file because the cert plugin tests do not use the declarative format and rely on the selfsign backend by default. rob Thanks! The code works well, but I found a few issues. These tests don't work when the full test suite is run: test_cert adds and revokes additional certs that throw the code off. Perhaps have the tests only query valid certs? I don't see that option but I think it would be helpful to support. I added some rather nasty hacks to the test to make things pass. I limit the search to 10 certificates, which is the number start with by default. There is an open dogtag ticket to return only VALID certificates so we should be able to drop this eventually. I had to go further on one of the revocation tests, limiting it to a sizelimit of 1. The count changes every time the suite runs so this is the safest for now. It also means that one test will fail if this is the only part of the suite executed. This gets rid of most of the failures, but it still fails the "certs for this IPA server/short name" tests if the cert from ./make-cert is present (creating it is part of `make test`). If make-cert is run more times, it'll revoke the previous cert, so the test for revocation reason 4 fails as well. It looks like when using sizelimit Dogtag will always discard *newer* certs, ones with higher serials. Is it documented behavior or does Dogtag just happen to do that? It isn't documented anywhere I could find, it is just what dogtag returns I wonder how other people run their tests. This solution looks like it could break easily if people do something differently :( I'm not sure how to solve this properly. Perhaps not using Declarative, and checking "by hand" that the wanted certs are in the response and the unwanted ones are not, would work better. I ended up switching the test class. It is not a very fine-grained set of tests, mostly searching with limits and verifying that we fall within a reasonable range, but it is better than nothing. rob This works much better, thanks! Just two nitpicks now. The patch doesn't apply well, there's a conflict in VERSION and some added trailing whitespace, AFAIK this would be the only (first?) test that relies on Nose's ordering of test modules -- tests 0011 and 0030 rely on the other cert tests running first. Please at least mention that in a comment. Or better, move class test_cert_find to test_cert.py Rebased the patch and removed whitespace. I went ahead and combined this with the existing test_cert file. Originally test_cert was only tested against lite-server but since it works against a live dogtag server too it makes sense to combine them. I improved the set up documentation a little bit and tried to handle all the different configurations that one might see so that this should be runnable against either a live server or the lite-server for both the selfsign and dogtag backends. This relies on the user configuring ~/.ipa/default.conf to match the remote server. There is no way from a client to know what kind of CA backend a server is running. rob And the patch. rob ACK, thank you! pushed to master and ipa-3-1 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0005 Add list of domains associated to our realm to cn=etc
Alexander Bokovoy wrote: On Wed, 13 Feb 2013, Alexander Bokovoy wrote: On Tue, 12 Feb 2013, Ana Krivokapic wrote: Add new LDAP container to store the list of domains associated with IPA realm. Add two new ipa commands (ipa realmdomains-show and ipa realmdomains-mod) to allow manipulation of the list of realm domains. Unit test file covering these new commands was added. https://fedorahosted.org/freeipa/ticket/2945 ACK, works perfectly. We need to decide on the questions still open in the http://www.freeipa.org/page/V3/Realm_Domains but the decision should not prevent this work from being committed. Committed to master along with a one-liner that limits ipasam to look only at $SUFFIX with base scope when searching for its own domain name as now we use domainRelatedObject in two different places and previous subtree search now gives too wide answer. Pushed patch (and 1-liner to ipa-3-1) rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 1087 Some missing v3 schema on upgrades
Jan Cholasta wrote: Hi, On 18.2.2013 22:00, Rob Crittenden wrote: An objectclass and attribute are not being added on upgrades. Missing these causes the UI to not work. I also noticed a typo in the ordering of a number of the trust attributes so fix those as well. rob The patch looks good, but I think errors like this will pop up from time to time, because we have to maintain the same thing in two places - the installation LDIFs and update files. Maybe we should start thinking about merging these two somehow, e.g. using the LDIFs for both installation and updates, with directives for the updater in specially formatted comments. Honza This idea came up long, long ago when we first added the updater very early in v2. The problem, as I recall, is that some schema is needed during the install so we need to ship it in ldif format, and the idea of splitting it didn't appeal to us. So perhaps what we should endeavor to do is add all new schema via updates and only update the schema files themselves if the schema is needed for a fresh install (since updates are done last). This also puts more schema into 99user.ldif which may or may not be desirable. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 1087 Some missing v3 schema on upgrades
On Tue, 2013-02-19 at 13:32 -0500, Rob Crittenden wrote: > Jan Cholasta wrote: > > Hi, > > > > On 18.2.2013 22:00, Rob Crittenden wrote: > >> An objectclass and attribute are not being added on upgrades. Missing > >> these causes the UI to not work. > >> > >> I also noticed a typo in the ordering of a number of the trust > >> attributes so fix those as well. > >> > >> rob > >> > > > > The patch looks good, but I think errors like this will pop up from time > > to time, because we have to maintain the same thing in two places - the > > installation LDIFs and update files. Maybe we should start thinking > > about merging these two somehow, e.g. using the LDIFs for both > > installation and updates, with directives for the updater in specially > > formatted comments. > > > > Honza > > > > This idea came up long, long ago when we first added the updater very > early in v2. The problem, as I recall, is that some schema is needed > during the install so we need to ship it in ldif format, and the idea of > splitting it didn't appeal to us. > > So perhaps what we should endeavor to do is add all new schema via > updates and only update the schema files themselves if the schema is > needed for a fresh install (since updates are done last). > > This also puts more schema into 99user.ldif which may or may not be > desirable. Ron another option is to keep putting all updates only in schema files, and then have the updater "validate" the schema files. Validation would be: 1. Download schema from server (we already do this in the framework so it comes for free) 2. parse the schema files and check if each attribute and objectclass is present and in the correct form. 3. if any attribute is missing, we add it 4. if any attribute has been changed, we change it 5. same for object classes. This would allow us to keep everything just in schema files, and for now only updates would end up in 99.ldif I know there is also work in 389ds to improve schema validation and handling, so there is a chance in future we will have online interfaces to put data in multiple files w/o lumping everything in 99.ldif So by keeping stuff in schema files rather than arbitrary update files we are also sort of future proof. Finally keeping data in schema files instead of spreading it in updates should make it easier to keep an eye on the whole schema. The main issue I see is that this approach needs new code to analyze and compare schema files, however that shouldn't be overly hard. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0027] Add checks for SELinux in install scripts
Tomas Babej wrote: On 02/04/2013 04:21 PM, Rob Crittenden wrote: Tomas Babej wrote: On 01/30/2013 05:12 PM, Tomas Babej wrote: Hi, The checks make sure that SELinux is: - installed and enabled (on server install) - installed and enabled OR not installed (on client install) Please note that client installs with SELinux not installed are allowed since freeipa-client package has no dependency on SELinux. (any objections to this approach?) The (unsupported) option --allow-no-selinux has been added. It can used to bypass the checks. Parts of platform-dependant code were refactored to use newly added is_selinux_enabled() function. https://fedorahosted.org/freeipa/ticket/3359 Tomas I forgot to edit the man pages. Thanks Rob! Updated patch attached. Tomas After a bit of off-line discussion I don't think we're quite ready yet to require SELinux by default on client installations (even with a flag to work around it). The feeling is this would be disruptive to existing automation. Can you still do the check but not enforce it, simply display a big warning if SELinux is disabled? rob Sure, here is the updated patch. I edited the commit message, RFE description and man pages according to the new behaviour. Tomas The patch looks good, I'm just wondering about one thing. The default value for is_selinux_enabled() is True in ipapython/services.py.in. So this means that any non-Red Hat/non-Fedora system, by default, is going to assume that SELinux is enabled. My hesitation has to when we call check_selinux_status(). It may incorrectly error out. I suspect that the user would have to work around this using --allow-selinux-disabled but this wouldn't make a lot of sense since they actually do have SELinux disabled. What do you think? rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0002 Add missing error message when adding duplicate external member to group
Ana Krivokapic wrote: When adding a duplicate member to a group, an error message is issued, informing the user that the entry is already a member of the group. This message was missing in case of an external member. Ticket: https://fedorahosted.org/freeipa/ticket/3254 This works ok but the sister command, group-remove-member, has the same problem. Can you add a fix there as well? I don't know if there is a way to add a unit test for this since the external member is validated meaning we'd need to set up trusts as well. It might be nice to have an optional test that can be run when a trust is configured to avoid regressions. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 355 Avoid internal error when user is not Trust admin
Martin Kosek wrote:
On 01/24/2013 12:01 PM, Martin Kosek wrote:
When user tries to perform any action requiring communication with
trusted domain, IPA server tries to retrieve a trust secret on his
behalf to be able to establish the connection. This happens for
example during group-add-member command when external user is
being resolved in the AD.
When user is not member of Trust admins group, the retrieval crashes
and reports internal error. Catch this exception and rather report
properly formatted ACIError.
I hit this error after updating to the latest FreeIPA version with the AD CVE
fixed.
Martin
I filed a ticket to not loose this fix and patch. Attaching an updated patch
with ticket URL in description.
Martin
The patch fixes the problem but the error is untranslated:
member group: AD\Domain Admins: Insufficient access:
Gettext('communication with trusted domains is allowed for Trusts
administrator group members only', domain='ipa', localedir=None)
rob
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES 0031-0032] Improve HBAC rule handling in selinuxusermap-add/mod/find
Tomas Babej wrote:
On 02/06/2013 07:57 PM, Rob Crittenden wrote:
Tomas Babej wrote:
Hi,
this pair of patches improves HBAC rule handling in selinuxusermap
commands.
Patch 0031 deals with:
https://fedorahosted.org/freeipa/ticket/3349
Patch 0032 takes care of:
https://fedorahosted.org/freeipa/ticket/3348
and is to be applied on top of Patch 0031.
See commit messages for detailed info.
Tomas
ACK for patch 0032.
For patch 0031 we can't change the data type of an existing attribute.
It will break backwards compatibility. Can you test with an older
client to see if it cares (it may not care about the name of the
type). If older clients will work then this is probably ok.
I gather that seealso detected as a DN attribute and converted into a
DN class and this is blowing up the Str validator?
Yes, that was exactly the case.
rob
I added a workaround for older client versions, tested it with
freeipa-client/admintools 2.2, works as expeceted.
However, this only should be issue if there is older admintools package
on the client than on the server.
Outline is such as follows: I added a new flag for DNParam seelalso
attribute, called 'allow_malformed' that allows any string to be passed
to DNParam. Its value gets wrapped in 'malformed=yes,value='.
This allows to parse out the string in selinuxusermap-add/mod
pre_callback out of the DN and search for the rule with such name so
that it's DN gets in LDAP instead.
Updated patch attached.
Tomas
I like where you're going with this, just a couple of comments:
1. Should we come up with a more universal name for allow_malformed? Is
this something that we should allow at a higher level? I was thinking
allow_raw, or allow_non_dn, or something like that.
2. I think that if a bad dn is passed in as a Str the conversion into a
DN won't be handled:
+if 'allow_malformed' in self.flags:
+dn = DN(('malformed','yes'),('value',value))
Should this be wrapped in a try/except to raise a ConversionError if it
fails?
rob
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0033] Prevent changing protected group's name using --setattr
Petr Viktorin wrote: On 02/11/2013 11:17 AM, Tomas Babej wrote: Hi, The name of any protected group now cannot be changed by modifing the cn attribute using --setattr. Unit tests have been added to make sure there is no regression. https://fedorahosted.org/freeipa/ticket/3354 Tomas We need a better general way to validate getattr/setattr on known atributes, but works well for now. ACK Pushed to master and ipa-3-1 rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0180 Check SSH connection in ipa-replica-conncheck
Petr Viktorin wrote: On 02/15/2013 08:18 PM, Rob Crittenden wrote: Petr Viktorin wrote: On 02/15/2013 04:38 PM, Rob Crittenden wrote: Petr Viktorin wrote: ipa-replica-conncheck ran SSH in quiet mode, probably to suppress a message about connecting to an unknown host. This made it hard to debug connection errors. I didn't find a way to separate SSH output from the output of the called command, I decided to try an additional SSH connection before calling conncheck. SSH is set to verbose and if it fails the errors get printed out. Also, the host is added to a temporary known_hosts file. The second SSH is called without the -q flag so errors/warnings are not lost even if the command fails. The temporary known_hosts file is used so the unknown host warning doesn't appear here. https://fedorahosted.org/freeipa/ticket/3402 The general procedure looks good, I don't think we should hardcode the path to ssh. ipautil.run() overrides the current environment so we should be able to safely run just 'ssh'. We eventually need a cross-platform way of locating system binaries. The hardcoded path to ipa-replica-conncheck is probably ok since we provide that binary ourselves. rob Changed, thanks. Looks and works well. I just have one final question. Should remote_addr and temp_known_hosts be passed in as args? They are basically globals but it is obvious where they came from, so not really a NAK, just a question. rob Well, it's a style issue so I only have a bunch of rule-of-thumb handwaving to justify the decision... Here goes: Closing over the variables in main() doesn't have drawbacks of globals: nothing outside main() can see/modify them and it doesn't keep main() from (indirectly) calling itself (this particular main() probably won't need to do that, but in general it's good to avoid this kind of surprises). Passing the variables in as arguments would only make sense if run_ssh was a top-level function, otherwise you have to worry about the right order/names of arguments and gain nothing. The nested function makes it clear that this is just a quick way to keep things DRY, not a full-featured generic SSH caller. A top-level "run_ssh" helper shouldn't unconditionally disable StrictHostKeychecking for example. The code is tied to the caller functionally, so I kept it tied syntactically as well. On the other hand, nested functions are "magic" that you need a good reason for. I think the above is just reason enough, but I don't feel that strongly about it. Here's a version with top-level run_ssh, push it if it seems better to you. Works for me. I pushed the second patch to master and ipa-3-1. It is clear enough where the variables come from. We can revisit in the future if this gets messy. rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] Backup and Restore design
I've looked into some basic backup and restore procedures for IPA. My findings are here: http://freeipa.org/page/V3/Backup_and_Restore This is in support of ticket https://fedorahosted.org/freeipa/ticket/3128 rob ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Backup and Restore design
On 02/19/2013 10:43 PM, Rob Crittenden wrote: I've looked into some basic backup and restore procedures for IPA. My findings are here: http://freeipa.org/page/V3/Backup_and_Restore Good write up Rob! It seems to me there are two critical sub-issues to solve first that could benefit us in the long run anyway. 1) Replacing certs. This has been a pain point for many admins who for various reasons now have invalid certs and need to redeploy all certs to bring back up our integrated web of services. Can this be a separate work item? It would be generally useful in contexts other than backups. 2) Tracking every file we modify. Aren't we already a good way there with the sysrestore functionality already in use in the install tools? It is just a matter of enhancing it a bit and being diligent about making sure it's used in every modification we make? Actually I'm not thinking we track every modification, rather we keep a manifest of everything we've touched and then just snapshot those files during backup, this would capture any modifications not made by us but perhaps are critical to restoring a working system (i.e. puppet modifications). The sysrestore module could provide the manifest, right? John -- John Dennis Looking to carve out IT costs? www.redhat.com/carveoutcosts/ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
