Re: [Freeipa-devel] [PATCHES 0001-0002] ipa-client-install NTP fixes

[Martin Kosek]

On 03/12/2015 09:43 PM, Nathan Kinder wrote:



On 03/04/2015 11:25 AM, Nathan Kinder wrote:



On 03/04/2015 10:58 AM, Martin Basti wrote:

On 04/03/15 19:56, Nathan Kinder wrote:


On 03/04/2015 10:41 AM, Rob Crittenden wrote:

Nathan Kinder wrote:


On 02/28/2015 01:13 PM, Nathan Kinder wrote:


On 02/28/2015 01:07 PM, Rob Crittenden wrote:

Nathan Kinder wrote:


On 02/27/2015 01:18 PM, Nathan Kinder wrote:


On 02/27/2015 01:08 PM, Rob Crittenden wrote:

Nathan Kinder wrote:


On 02/27/2015 12:20 PM, Rob Crittenden wrote:

Nathan Kinder wrote:


On 02/26/2015 12:55 AM, Martin Kosek wrote:

On 02/26/2015 03:28 AM, Nathan Kinder wrote:

Hi,

The two attached patches address some issues that affect
ipa-client-install when syncing time from the NTP server.
Now that we
use ntpd to perform the time sync, the client install can
end up hanging
forever when the server is not reachable (firewall issues,
etc.).  These
patches address the issues in two different ways:

1 - Don't attempt to sync time when --no-ntp is specified.

2 - Implement a timeout capability that is used when we
run ntpd to
perform the time sync to prevent indefinite hanging.

The one potentially contentious issue is that this
introduces a new
dependency on python-subprocess32 to allow us to have
timeout support
when using Python 2.x.  This is packaged for Fedora, but I
don't see it
on RHEL or CentOS currently.  It would need to be packaged
there.

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

Thanks,
-NGK

Thanks for Patches. For the second patch, I would really
prefer to avoid new
dependency, especially if it's not packaged in RHEL/CentOS.
Maybe we could use
some workaround instead, as in:

http://stackoverflow.com/questions/3733270/python-subprocess-timeout


I don't like having to add an additional dependency either,
but the
alternative seems more risky.  Utilizing the subprocess32
module (which
is really just a backport of the normal subprocess module
from Python
3.x) is not invasive for our code in ipautil.run().  Adding
some sort of
a thread that has to kill the spawned subprocess seems more
risky (see
the discussion about a race condition in the stackoverflow
thread
above).  That said, I'm sure the thread/poll method can be
made to work
if you and others feel strongly that this is a better
approach than
adding a new dependency.

Why not use /usr/bin/timeout from coreutils?

That sounds like a perfectly good idea.  I wasn't aware of it's
existence (or it's possible that I forgot about it).  Thanks
for the
suggestion!  I'll test out a reworked version of the patch.

Do you think that there is value in leaving the timeout
capability
centrally in ipautil.run()?  We only need it for the call to
'ntpd'
right now, but there might be a need for using a timeout for
other
commands in the future.  The alternative is to just modify
synconce_ntp() to use /usr/bin/timeout and leave ipautil.run()
alone.

I think it would require a lot of research. One of the programs
spawned
by this is pkicreate which could take quite some time, and
spawning a
clone in particular.

It is definitely an interesting idea but I think it is safest
for now to
limit it to just NTP for now.

What I meant was that we would have an optional keyword "timeout"
parameter to ipautil.run() that defaults to None, just like my
subprocess32 approach.  If a timeout is not passed in, we would use
subprocess.Popen() to run the specified command just like we do
today.
We would only actually pass the timeout parameter to
ipautil.run() in
synconce_ntp() for now, so no other commands would have a
timeout in
effect.  The capability would be available for other commands
this way
though.

Let me propose a patch with this implementation, and if you
don't like
it, we can leave ipautil.run() alone and restrict the changes to
synconce_ntp().

An updated patch 0002 is attached that uses the approach
mentioned above.

Looks good. Not to nitpick to death but...

Can you add timeout to ipaplatform/base/paths.py as BIN_TIMEOUT =
"/usr/bin/timeout" and reference that instead? It's for portability.

Sure.  I was wondering if we should do something around a full path.


And a question. I'm impatient. Should there be a notice that it will
timeout after n seconds somewhere so people like me don't ^C after 2
seconds? Or is that just overkill and I need to learn patience?

Probably both. :)  There's always going to be someone out there who
will
do ctrl-C, so I think printing out a notice is a good idea.  I'll
add this.


Stylistically, should we prefer p.returncode is 15 or p.returncode
== 15?

After some reading, it seems that '==' should be used.  Small integers
work with 'is', but '==' is the consistent way that equality of
integers
should be checked.  I'll modify this.

Another updated patch 0002 is attached that addresses Rob's review
comments.

Thanks,
-NGK


LGTM. Does someone else have time to test this?

I also don't know if there is a policy on placement of new items in
paths.py. Things are all ove

Re: [Freeipa-devel] [PATCH 0208] Respect --test option in upgrade plugins

[Martin Kosek]

On 03/12/2015 05:10 PM, Rob Crittenden wrote:

Petr Spacek wrote:

On 12.3.2015 16:23, Rob Crittenden wrote:

David Kupka wrote:

On 03/06/2015 06:00 PM, Martin Basti wrote:

Upgrade plugins which modify LDAP data directly should not be executed
in --test mode.

This patch is a workaround, to ensure update with --test option will not
modify any LDAP data.

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

Patch attached.





Ideally we want to fix all plugins to dry-run the upgrade not just skip
when there is '--test' option but it is a good first step.
Works for me, ACK.



I agree that this breaks the spirit of --test and think it should be
fixed before committing.


Considering how often is the option is used ... I do not think that this
requires 'proper' fix now. It was broken for *years* so this patch is a huge
improvement and IMHO should be commited in current form. We can re-visit it
later on, open a ticket :-)



No. There is no rush for this, at least not for the promise of a future
fix that will never come.


I checked the code and to me, the proper fix looks like instrumenting 
ldap.update_entry calls in upgrade plugins with


if options.test:
   log message
else
   do the update

right? I see just couple places that would need to be updated:

$ git grep -E "(ldap|conn).update_entry" ipaserver/install/plugins
ipaserver/install/plugins/dns.py:ldap.update_entry(container_entry)
ipaserver/install/plugins/fix_replica_agreements.py: 
repl.conn.update_entry(replica)
ipaserver/install/plugins/fix_replica_agreements.py: 
repl.conn.update_entry(replica)
ipaserver/install/plugins/update_idranges.py: 
ldap.update_entry(entry)
ipaserver/install/plugins/update_idranges.py: 
ldap.update_entry(entry)
ipaserver/install/plugins/update_managed_permissions.py: 
ldap.update_entry(base_entry)
ipaserver/install/plugins/update_managed_permissions.py: 
ldap.update_entry(entry)

ipaserver/install/plugins/update_pacs.py:ldap.update_entry(entry)
ipaserver/install/plugins/update_referint.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_services.py: 
ldap.update_entry(entry)
ipaserver/install/plugins/update_uniqueness.py: 
ldap.update_entry(uid_uniqueness_plugin)



So from my POV, very quick fix. In that case, I would also prefer a fix now 
than a ticket that would never be done.


Martin

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0208] Respect --test option in upgrade plugins

[Petr Spacek]

On 13.3.2015 10:18, Martin Kosek wrote:
> On 03/12/2015 05:10 PM, Rob Crittenden wrote:
>> Petr Spacek wrote:
>>> On 12.3.2015 16:23, Rob Crittenden wrote:
 David Kupka wrote:
> On 03/06/2015 06:00 PM, Martin Basti wrote:
>> Upgrade plugins which modify LDAP data directly should not be executed
>> in --test mode.
>>
>> This patch is a workaround, to ensure update with --test option will not
>> modify any LDAP data.
>>
>> https://fedorahosted.org/freeipa/ticket/3448
>>
>> Patch attached.
>>
>>
>>
>
> Ideally we want to fix all plugins to dry-run the upgrade not just skip
> when there is '--test' option but it is a good first step.
> Works for me, ACK.
>

 I agree that this breaks the spirit of --test and think it should be
 fixed before committing.
>>>
>>> Considering how often is the option is used ... I do not think that this
>>> requires 'proper' fix now. It was broken for *years* so this patch is a huge
>>> improvement and IMHO should be commited in current form. We can re-visit it
>>> later on, open a ticket :-)
>>>
>>
>> No. There is no rush for this, at least not for the promise of a future
>> fix that will never come.
> 
> I checked the code and to me, the proper fix looks like instrumenting
> ldap.update_entry calls in upgrade plugins with
> 
> if options.test:
>log message
> else
>do the update
> 
> right? I see just couple places that would need to be updated:
> 
> $ git grep -E "(ldap|conn).update_entry" ipaserver/install/plugins
> ipaserver/install/plugins/dns.py:ldap.update_entry(container_entry)
> ipaserver/install/plugins/fix_replica_agreements.py:
> repl.conn.update_entry(replica)
> ipaserver/install/plugins/fix_replica_agreements.py:
> repl.conn.update_entry(replica)
> ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
> ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
> ipaserver/install/plugins/update_managed_permissions.py:
> ldap.update_entry(base_entry)
> ipaserver/install/plugins/update_managed_permissions.py: 
> ldap.update_entry(entry)
> ipaserver/install/plugins/update_pacs.py:ldap.update_entry(entry)
> ipaserver/install/plugins/update_referint.py:
> ldap.update_entry(entry)
> ipaserver/install/plugins/update_services.py: ldap.update_entry(entry)
> ipaserver/install/plugins/update_uniqueness.py:
> ldap.update_entry(uid_uniqueness_plugin)
> 
> 
> So from my POV, very quick fix. In that case, I would also prefer a fix now
> than a ticket that would never be done.

I really dislike this approach because I consider it flawed by design. Plugin
author has to think about it all the time and do not forget to add if
otherwise ... too bad.

I can see two 'safer' ways to do that:
- LDAP transactions :-)
- 'mock_writes=True' option in LDAP backend which would print modlists instead
of applying them (and return success to the caller).

Both cases eliminate the need to scatter 'ifs' all over update plugins and do
not add risk of forgetting about one of them when adding/changing plugin code.

-- 
Petr^2 Spacek

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0208] Respect --test option in upgrade plugins

[Alexander Bokovoy]

On Fri, 13 Mar 2015, Petr Spacek wrote:

On 13.3.2015 10:18, Martin Kosek wrote:

On 03/12/2015 05:10 PM, Rob Crittenden wrote:

Petr Spacek wrote:

On 12.3.2015 16:23, Rob Crittenden wrote:

David Kupka wrote:

On 03/06/2015 06:00 PM, Martin Basti wrote:

Upgrade plugins which modify LDAP data directly should not be executed
in --test mode.

This patch is a workaround, to ensure update with --test option will not
modify any LDAP data.

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

Patch attached.





Ideally we want to fix all plugins to dry-run the upgrade not just skip
when there is '--test' option but it is a good first step.
Works for me, ACK.



I agree that this breaks the spirit of --test and think it should be
fixed before committing.


Considering how often is the option is used ... I do not think that this
requires 'proper' fix now. It was broken for *years* so this patch is a huge
improvement and IMHO should be commited in current form. We can re-visit it
later on, open a ticket :-)



No. There is no rush for this, at least not for the promise of a future
fix that will never come.


I checked the code and to me, the proper fix looks like instrumenting
ldap.update_entry calls in upgrade plugins with

if options.test:
   log message
else
   do the update

right? I see just couple places that would need to be updated:

$ git grep -E "(ldap|conn).update_entry" ipaserver/install/plugins
ipaserver/install/plugins/dns.py:ldap.update_entry(container_entry)
ipaserver/install/plugins/fix_replica_agreements.py:
repl.conn.update_entry(replica)
ipaserver/install/plugins/fix_replica_agreements.py:
repl.conn.update_entry(replica)
ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_managed_permissions.py:
ldap.update_entry(base_entry)
ipaserver/install/plugins/update_managed_permissions.py: 
ldap.update_entry(entry)
ipaserver/install/plugins/update_pacs.py:ldap.update_entry(entry)
ipaserver/install/plugins/update_referint.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_services.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_uniqueness.py:
ldap.update_entry(uid_uniqueness_plugin)


So from my POV, very quick fix. In that case, I would also prefer a fix now
than a ticket that would never be done.


I really dislike this approach because I consider it flawed by design. Plugin
author has to think about it all the time and do not forget to add if
otherwise ... too bad.

I can see two 'safer' ways to do that:
- LDAP transactions :-)
- 'mock_writes=True' option in LDAP backend which would print modlists instead
of applying them (and return success to the caller).

Both cases eliminate the need to scatter 'ifs' all over update plugins and do
not add risk of forgetting about one of them when adding/changing plugin code.

I like idea about mock_writes=True. However, I think we still need to make
sure plugin writers rely on options.test value to see that we aren't
going to write the data. The reason for it is that we might get into
configurations where plugins would be doing updates based on earlier
performed tasks.  If task configuration is not going to be written, its
status will never be correct and plugin would get an error.

--
/ Alexander Bokovoy

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCHES 0015-0017] consolidation of various Kerberos auth methods in FreeIPA code

[Martin Babinsky]

On 03/11/2015 12:42 PM, Petr Spacek wrote:

diff --git a/ipaserver/rpcserver.py b/ipaserver/rpcserver.py
index 
d6bc955b9d9910a24eec5df1def579310eb54786..36f16908ac8477d9982bfee613b77576853054eb
 100644
--- a/ipaserver/rpcserver.py
+++ b/ipaserver/rpcserver.py
@@ -958,8 +958,8 @@ class login_password(Backend, KerberosSession, HTTP_Status):

  def kinit(self, user, realm, password, ccache_name):
  # get http service ccache as an armor for FAST to enable OTP 
authentication
-armor_principal = krb5_format_service_principal_name(
-'HTTP', self.api.env.host, realm)
+armor_principal = str(krb5_format_service_principal_name(
+'HTTP', self.api.env.host, realm))
  keytab = paths.IPA_KEYTAB
  armor_name = "%sA_%s" % (krbccache_prefix, user)
  armor_path = os.path.join(krbccache_dir, armor_name)
@@ -967,34 +967,33 @@ class login_password(Backend, KerberosSession, 
HTTP_Status):
  self.debug('Obtaining armor ccache: principal=%s keytab=%s ccache=%s',
 armor_principal, keytab, armor_path)

-(stdout, stderr, returncode) = ipautil.run(
-[paths.KINIT, '-kt', keytab, armor_principal],
-env={'KRB5CCNAME': armor_path}, raiseonerr=False)
-
-if returncode != 0:
-raise CCacheError()
+try:
+ipautil.kinit_keytab(paths.IPA_KEYTAB, armor_path,
+ armor_principal)
+except StandardError, e:
+raise CCacheError(str(e))

  # Format the user as a kerberos principal
  principal = krb5_format_principal_name(user, realm)

-(stdout, stderr, returncode) = ipautil.run(
-[paths.KINIT, principal, '-T', armor_path],
-env={'KRB5CCNAME': ccache_name, 'LC_ALL': 'C'},
-stdin=password, raiseonerr=False)
+try:
+ipautil.kinit_password(principal, password,
+   env={'KRB5CCNAME': ccache_name,
+'LC_ALL': 'C'},
+   armor_ccache=armor_path)

-self.debug('kinit: principal=%s returncode=%s, stderr="%s"',
-   principal, returncode, stderr)
-
-self.debug('Cleanup the armor ccache')
-ipautil.run(
-[paths.KDESTROY, '-A', '-c', armor_path],
-env={'KRB5CCNAME': armor_path},
-raiseonerr=False)
-
-if returncode != 0:
-if stderr.strip() == 'kinit: Cannot read password while getting 
initial credentials':
-raise PasswordExpired(principal=principal, 
message=unicode(stderr))
-raise InvalidSessionPassword(principal=principal, 
message=unicode(stderr))
+self.debug('Cleanup the armor ccache')
+ipautil.run(
+[paths.KDESTROY, '-A', '-c', armor_path],
+env={'KRB5CCNAME': armor_path},
+raiseonerr=False)
+except ipautil.CalledProcessError, e:
+if ('kinit: Cannot read password while '
+'getting initial credentials') in e.output:

I know it is not your code but please make sure it will work with non-English
LANG or LC_MESSAGE.


I have done some research about the way the environmental variables line 
LC_MESSAGE, LC_ALL, etc. work, 
(https://www.gnu.org/software/gettext/manual/gettext.html#Locale-Names 
and the following section).


It turns out that the CalledProcessError handling code will work also in 
non-english environment, because in the following code snippet, kinit is 
actually run using LC_ALL=C environment variable effectively disabling 
any localization and forcing the program to print out default (i.e. 
english) error messages.


>> +try:
>> +ipautil.kinit_password(principal, password,
>> +   env={'KRB5CCNAME': ccache_name,
>> +'LC_ALL': 'C'},
>> +   armor_ccache=armor_path)

Thus when handling the exception, we may be sure that any error message 
returned by above is in default locale. This greatly simplifies the 
logic deciding what exception to raise further based on the error message.


>> +except ipautil.CalledProcessError, e:
>> +if ('kinit: Cannot read password while '
>> +'getting initial credentials') in e.output:

A very clever move by Nathaniel (according to git blame) to circumvent 
the locale-specific behavior when it's actualy not needed.


--
Martin^3 Babinsky

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0208] Respect --test option in upgrade plugins

[Petr Spacek]

On 13.3.2015 10:42, Alexander Bokovoy wrote:
> On Fri, 13 Mar 2015, Petr Spacek wrote:
>> On 13.3.2015 10:18, Martin Kosek wrote:
>>> On 03/12/2015 05:10 PM, Rob Crittenden wrote:
 Petr Spacek wrote:
> On 12.3.2015 16:23, Rob Crittenden wrote:
>> David Kupka wrote:
>>> On 03/06/2015 06:00 PM, Martin Basti wrote:
 Upgrade plugins which modify LDAP data directly should not be executed
 in --test mode.

 This patch is a workaround, to ensure update with --test option will 
 not
 modify any LDAP data.

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

 Patch attached.



>>>
>>> Ideally we want to fix all plugins to dry-run the upgrade not just skip
>>> when there is '--test' option but it is a good first step.
>>> Works for me, ACK.
>>>
>>
>> I agree that this breaks the spirit of --test and think it should be
>> fixed before committing.
>
> Considering how often is the option is used ... I do not think that this
> requires 'proper' fix now. It was broken for *years* so this patch is a 
> huge
> improvement and IMHO should be commited in current form. We can re-visit 
> it
> later on, open a ticket :-)
>

 No. There is no rush for this, at least not for the promise of a future
 fix that will never come.
>>>
>>> I checked the code and to me, the proper fix looks like instrumenting
>>> ldap.update_entry calls in upgrade plugins with
>>>
>>> if options.test:
>>>log message
>>> else
>>>do the update
>>>
>>> right? I see just couple places that would need to be updated:
>>>
>>> $ git grep -E "(ldap|conn).update_entry" ipaserver/install/plugins
>>> ipaserver/install/plugins/dns.py:ldap.update_entry(container_entry)
>>> ipaserver/install/plugins/fix_replica_agreements.py:
>>> repl.conn.update_entry(replica)
>>> ipaserver/install/plugins/fix_replica_agreements.py:
>>> repl.conn.update_entry(replica)
>>> ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
>>> ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
>>> ipaserver/install/plugins/update_managed_permissions.py:
>>> ldap.update_entry(base_entry)
>>> ipaserver/install/plugins/update_managed_permissions.py:
>>> ldap.update_entry(entry)
>>> ipaserver/install/plugins/update_pacs.py:
>>> ldap.update_entry(entry)
>>> ipaserver/install/plugins/update_referint.py:   
>>> ldap.update_entry(entry)
>>> ipaserver/install/plugins/update_services.py: ldap.update_entry(entry)
>>> ipaserver/install/plugins/update_uniqueness.py:
>>> ldap.update_entry(uid_uniqueness_plugin)
>>>
>>>
>>> So from my POV, very quick fix. In that case, I would also prefer a fix now
>>> than a ticket that would never be done.
>>
>> I really dislike this approach because I consider it flawed by design. Plugin
>> author has to think about it all the time and do not forget to add if
>> otherwise ... too bad.
>>
>> I can see two 'safer' ways to do that:
>> - LDAP transactions :-)
>> - 'mock_writes=True' option in LDAP backend which would print modlists 
>> instead
>> of applying them (and return success to the caller).
>>
>> Both cases eliminate the need to scatter 'ifs' all over update plugins and do
>> not add risk of forgetting about one of them when adding/changing plugin 
>> code.
> I like idea about mock_writes=True. However, I think we still need to make
> sure plugin writers rely on options.test value to see that we aren't
> going to write the data. The reason for it is that we might get into
> configurations where plugins would be doing updates based on earlier
> performed tasks.  If task configuration is not going to be written, its
> status will never be correct and plugin would get an error.

That is exactly why I mentioned LDAP transactions. There is no other way how
to test complex plugins which actually read own writes (except mocking the
whole LDAP interface somewhere).

-- 
Petr^2 Spacek

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0208] Respect --test option in upgrade plugins

[Martin Kosek]

On 03/13/2015 11:00 AM, Petr Spacek wrote:

On 13.3.2015 10:42, Alexander Bokovoy wrote:

On Fri, 13 Mar 2015, Petr Spacek wrote:

On 13.3.2015 10:18, Martin Kosek wrote:

On 03/12/2015 05:10 PM, Rob Crittenden wrote:

Petr Spacek wrote:

On 12.3.2015 16:23, Rob Crittenden wrote:

David Kupka wrote:

On 03/06/2015 06:00 PM, Martin Basti wrote:

Upgrade plugins which modify LDAP data directly should not be executed
in --test mode.

This patch is a workaround, to ensure update with --test option will not
modify any LDAP data.

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

Patch attached.





Ideally we want to fix all plugins to dry-run the upgrade not just skip
when there is '--test' option but it is a good first step.
Works for me, ACK.



I agree that this breaks the spirit of --test and think it should be
fixed before committing.


Considering how often is the option is used ... I do not think that this
requires 'proper' fix now. It was broken for *years* so this patch is a huge
improvement and IMHO should be commited in current form. We can re-visit it
later on, open a ticket :-)



No. There is no rush for this, at least not for the promise of a future
fix that will never come.


I checked the code and to me, the proper fix looks like instrumenting
ldap.update_entry calls in upgrade plugins with

if options.test:
log message
else
do the update

right? I see just couple places that would need to be updated:

$ git grep -E "(ldap|conn).update_entry" ipaserver/install/plugins
ipaserver/install/plugins/dns.py:ldap.update_entry(container_entry)
ipaserver/install/plugins/fix_replica_agreements.py:
repl.conn.update_entry(replica)
ipaserver/install/plugins/fix_replica_agreements.py:
repl.conn.update_entry(replica)
ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_managed_permissions.py:
ldap.update_entry(base_entry)
ipaserver/install/plugins/update_managed_permissions.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_pacs.py:ldap.update_entry(entry)
ipaserver/install/plugins/update_referint.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_services.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_uniqueness.py:
ldap.update_entry(uid_uniqueness_plugin)


So from my POV, very quick fix. In that case, I would also prefer a fix now
than a ticket that would never be done.


I really dislike this approach because I consider it flawed by design. Plugin
author has to think about it all the time and do not forget to add if
otherwise ... too bad.

I can see two 'safer' ways to do that:
- LDAP transactions :-)
- 'mock_writes=True' option in LDAP backend which would print modlists instead
of applying them (and return success to the caller).

Both cases eliminate the need to scatter 'ifs' all over update plugins and do
not add risk of forgetting about one of them when adding/changing plugin code.

I like idea about mock_writes=True. However, I think we still need to make
sure plugin writers rely on options.test value to see that we aren't
going to write the data. The reason for it is that we might get into
configurations where plugins would be doing updates based on earlier
performed tasks.  If task configuration is not going to be written, its
status will never be correct and plugin would get an error.


That is exactly why I mentioned LDAP transactions. There is no other way how
to test complex plugins which actually read own writes (except mocking the
whole LDAP interface somewhere).


While this may be a good idea long term, I do not think any of us is 
considering implementing the LDAP transaction support within work on this 
refactoring.


So in this thread, let us focus on how to fix options.test mid-term. I 
currently see 2 proposed ways:

- Making the plugins aware of options.test
- Make ldap2 write operations only print the update and not do it. Although 
thinking of this approach, I think it may make some plugins like DNS loop 
forever. IIRC, at least DNS upgrade plugin have loop

  - search for all unfixed DNS zones
  - fix them with ldap update
  - was the search truncated, i.e.  are there more zones to update?
if yes, go back to start

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0208] Respect --test option in upgrade plugins

[Jan Cholasta]

Dne 13.3.2015 v 11:17 Martin Kosek napsal(a):

On 03/13/2015 11:00 AM, Petr Spacek wrote:

On 13.3.2015 10:42, Alexander Bokovoy wrote:

On Fri, 13 Mar 2015, Petr Spacek wrote:

On 13.3.2015 10:18, Martin Kosek wrote:

On 03/12/2015 05:10 PM, Rob Crittenden wrote:

Petr Spacek wrote:

On 12.3.2015 16:23, Rob Crittenden wrote:

David Kupka wrote:

On 03/06/2015 06:00 PM, Martin Basti wrote:

Upgrade plugins which modify LDAP data directly should not be
executed
in --test mode.

This patch is a workaround, to ensure update with --test
option will not
modify any LDAP data.

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

Patch attached.





Ideally we want to fix all plugins to dry-run the upgrade not
just skip
when there is '--test' option but it is a good first step.
Works for me, ACK.



I agree that this breaks the spirit of --test and think it
should be
fixed before committing.


Considering how often is the option is used ... I do not think
that this
requires 'proper' fix now. It was broken for *years* so this
patch is a huge
improvement and IMHO should be commited in current form. We can
re-visit it
later on, open a ticket :-)



No. There is no rush for this, at least not for the promise of a
future
fix that will never come.


I checked the code and to me, the proper fix looks like instrumenting
ldap.update_entry calls in upgrade plugins with

if options.test:
log message
else
do the update

right? I see just couple places that would need to be updated:

$ git grep -E "(ldap|conn).update_entry" ipaserver/install/plugins
ipaserver/install/plugins/dns.py:
ldap.update_entry(container_entry)
ipaserver/install/plugins/fix_replica_agreements.py:
repl.conn.update_entry(replica)
ipaserver/install/plugins/fix_replica_agreements.py:
repl.conn.update_entry(replica)
ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_managed_permissions.py:
ldap.update_entry(base_entry)
ipaserver/install/plugins/update_managed_permissions.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_pacs.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_referint.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_services.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_uniqueness.py:
ldap.update_entry(uid_uniqueness_plugin)


So from my POV, very quick fix. In that case, I would also prefer a
fix now
than a ticket that would never be done.


I really dislike this approach because I consider it flawed by
design. Plugin
author has to think about it all the time and do not forget to add if
otherwise ... too bad.

I can see two 'safer' ways to do that:
- LDAP transactions :-)
- 'mock_writes=True' option in LDAP backend which would print
modlists instead
of applying them (and return success to the caller).

Both cases eliminate the need to scatter 'ifs' all over update
plugins and do
not add risk of forgetting about one of them when adding/changing
plugin code.

I like idea about mock_writes=True. However, I think we still need to
make
sure plugin writers rely on options.test value to see that we aren't
going to write the data. The reason for it is that we might get into
configurations where plugins would be doing updates based on earlier
performed tasks.  If task configuration is not going to be written, its
status will never be correct and plugin would get an error.


That is exactly why I mentioned LDAP transactions. There is no other
way how
to test complex plugins which actually read own writes (except mocking
the
whole LDAP interface somewhere).


While this may be a good idea long term, I do not think any of us is
considering implementing the LDAP transaction support within work on
this refactoring.

So in this thread, let us focus on how to fix options.test mid-term. I
currently see 2 proposed ways:
- Making the plugins aware of options.test
- Make ldap2 write operations only print the update and not do it.
Although thinking of this approach, I think it may make some plugins
like DNS loop forever. IIRC, at least DNS upgrade plugin have loop
   - search for all unfixed DNS zones
   - fix them with ldap update
   - was the search truncated, i.e.  are there more zones to update?
 if yes, go back to start


 - Make the plugins not call {add,update,delete}_entry themselves but 
rather return the updates like they should. This is what the ticket 
() requests and what 
should be done to make --test work for them.


--
Jan Cholasta

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0208] Respect --test option in upgrade plugins

[Petr Spacek]

On 13.3.2015 11:34, Jan Cholasta wrote:
> Dne 13.3.2015 v 11:17 Martin Kosek napsal(a):
>> On 03/13/2015 11:00 AM, Petr Spacek wrote:
>>> On 13.3.2015 10:42, Alexander Bokovoy wrote:
 On Fri, 13 Mar 2015, Petr Spacek wrote:
> On 13.3.2015 10:18, Martin Kosek wrote:
>> On 03/12/2015 05:10 PM, Rob Crittenden wrote:
>>> Petr Spacek wrote:
 On 12.3.2015 16:23, Rob Crittenden wrote:
> David Kupka wrote:
>> On 03/06/2015 06:00 PM, Martin Basti wrote:
>>> Upgrade plugins which modify LDAP data directly should not be
>>> executed
>>> in --test mode.
>>>
>>> This patch is a workaround, to ensure update with --test
>>> option will not
>>> modify any LDAP data.
>>>
>>> https://fedorahosted.org/freeipa/ticket/3448
>>>
>>> Patch attached.
>>>
>>>
>>>
>>
>> Ideally we want to fix all plugins to dry-run the upgrade not
>> just skip
>> when there is '--test' option but it is a good first step.
>> Works for me, ACK.
>>
>
> I agree that this breaks the spirit of --test and think it
> should be
> fixed before committing.

 Considering how often is the option is used ... I do not think
 that this
 requires 'proper' fix now. It was broken for *years* so this
 patch is a huge
 improvement and IMHO should be commited in current form. We can
 re-visit it
 later on, open a ticket :-)

>>>
>>> No. There is no rush for this, at least not for the promise of a
>>> future
>>> fix that will never come.
>>
>> I checked the code and to me, the proper fix looks like instrumenting
>> ldap.update_entry calls in upgrade plugins with
>>
>> if options.test:
>> log message
>> else
>> do the update
>>
>> right? I see just couple places that would need to be updated:
>>
>> $ git grep -E "(ldap|conn).update_entry" ipaserver/install/plugins
>> ipaserver/install/plugins/dns.py:
>> ldap.update_entry(container_entry)
>> ipaserver/install/plugins/fix_replica_agreements.py:
>> repl.conn.update_entry(replica)
>> ipaserver/install/plugins/fix_replica_agreements.py:
>> repl.conn.update_entry(replica)
>> ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
>> ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
>> ipaserver/install/plugins/update_managed_permissions.py:
>> ldap.update_entry(base_entry)
>> ipaserver/install/plugins/update_managed_permissions.py:
>> ldap.update_entry(entry)
>> ipaserver/install/plugins/update_pacs.py:
>> ldap.update_entry(entry)
>> ipaserver/install/plugins/update_referint.py:
>> ldap.update_entry(entry)
>> ipaserver/install/plugins/update_services.py: ldap.update_entry(entry)
>> ipaserver/install/plugins/update_uniqueness.py:
>> ldap.update_entry(uid_uniqueness_plugin)
>>
>>
>> So from my POV, very quick fix. In that case, I would also prefer a
>> fix now
>> than a ticket that would never be done.
>
> I really dislike this approach because I consider it flawed by
> design. Plugin
> author has to think about it all the time and do not forget to add if
> otherwise ... too bad.
>
> I can see two 'safer' ways to do that:
> - LDAP transactions :-)
> - 'mock_writes=True' option in LDAP backend which would print
> modlists instead
> of applying them (and return success to the caller).
>
> Both cases eliminate the need to scatter 'ifs' all over update
> plugins and do
> not add risk of forgetting about one of them when adding/changing
> plugin code.
 I like idea about mock_writes=True. However, I think we still need to
 make
 sure plugin writers rely on options.test value to see that we aren't
 going to write the data. The reason for it is that we might get into
 configurations where plugins would be doing updates based on earlier
 performed tasks.  If task configuration is not going to be written, its
 status will never be correct and plugin would get an error.
>>>
>>> That is exactly why I mentioned LDAP transactions. There is no other
>>> way how
>>> to test complex plugins which actually read own writes (except mocking
>>> the
>>> whole LDAP interface somewhere).
>>
>> While this may be a good idea long term, I do not think any of us is
>> considering implementing the LDAP transaction support within work on
>> this refactoring.
>>
>> So in this thread, let us focus on how to fix options.test mid-term. I
>> currently see 2 proposed ways:
>> - Making the plugins aware of options.test
>> - Make ldap2 write operations only print the update and not do it.
>> Although thinking of this approach, I think it may make some plugins
>> like DNS loop for

Re: [Freeipa-devel] [PATCHES 137-139] extdom: add err_msg member to request context

[Sumit Bose]

On Wed, Mar 04, 2015 at 06:35:22PM +0100, Sumit Bose wrote:
> Hi,
> 
> this patch series improves error reporting of the extdom plugin
> especially on the client side. Currently there is only SSSD ticket
> https://fedorahosted.org/sssd/ticket/2463 . Shall I create a
> corresponding FreeIPA ticket as well?
> 
> In the third patch I already added a handful of new error messages.
> Suggestions for more messages are welcome.
> 
> bye,
> Sumit

Rebased versions attached.

bye,
Sumit
From e402a733322c68db0f808d3386a27e5fd9bc177b Mon Sep 17 00:00:00 2001
From: Sumit Bose 
Date: Mon, 2 Feb 2015 00:52:10 +0100
Subject: [PATCH 137/139] extdom: add err_msg member to request context

---
 daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h| 1 +
 daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c | 1 +
 daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_extop.c  | 5 -
 3 files changed, 6 insertions(+), 1 deletion(-)

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 
d4c851169ddadc869a59c53075f9fc7f33321085..421f6c6ea625aba2db7e9ffc84115b3647673699
 100644
--- a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h
+++ b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h
@@ -116,6 +116,7 @@ struct extdom_req {
 gid_t gid;
 } posix_gid;
 } data;
+char *err_msg;
 };
 
 struct extdom_res {
diff --git a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c 
b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c
index 
47bcb179f04e08c64d92f55809b84f2d59622344..c2fd42f13fca97587ddc4c12b560e590462f121b
 100644
--- a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c
+++ b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c
@@ -356,6 +356,7 @@ void free_req_data(struct extdom_req *req)
 break;
 }
 
+free(req->err_msg);
 free(req);
 }
 
diff --git a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_extop.c 
b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_extop.c
index 
dc7785877fc321ddaa5b6967d1c1b06cb454bbbf..708d0e4a2fc9da4f87a24a49c945587049f7280f
 100644
--- a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_extop.c
+++ b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_extop.c
@@ -149,12 +149,15 @@ static int ipa_extdom_extop(Slapi_PBlock *pb)
 rc = LDAP_SUCCESS;
 
 done:
-free_req_data(req);
+if (req->err_msg != NULL) {
+err_msg = req->err_msg;
+}
 if (err_msg != NULL) {
 LOG("%s", err_msg);
 }
 slapi_send_ldap_result(pb, rc, NULL, err_msg, 0, NULL);
 ber_bvfree(ret_val);
+free_req_data(req);
 return SLAPI_PLUGIN_EXTENDED_SENT_RESULT;
 }
 
-- 
2.1.0

From 6a6a18313745e5e50b629009a74f7b0ad1975fe2 Mon Sep 17 00:00:00 2001
From: Sumit Bose 
Date: Mon, 2 Feb 2015 00:53:06 +0100
Subject: [PATCH 138/139] extdom: add add_err_msg() with test

---
 .../ipa-extdom-extop/ipa_extdom.h  |  1 +
 .../ipa-extdom-extop/ipa_extdom_cmocka_tests.c | 43 ++
 .../ipa-extdom-extop/ipa_extdom_common.c   | 23 
 3 files changed, 67 insertions(+)

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 
421f6c6ea625aba2db7e9ffc84115b3647673699..0d5d55d2fb0ece95466b0225b145a4edeef18efa
 100644
--- a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h
+++ b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h
@@ -185,4 +185,5 @@ int getgrnam_r_wrapper(size_t buf_max, const char *name,
struct group *grp, char **_buf, size_t *_buf_len);
 int getgrgid_r_wrapper(size_t buf_max, gid_t gid,
struct group *grp, char **_buf, size_t *_buf_len);
+void set_err_msg(struct extdom_req *req, const char *format, ...);
 #endif /* _IPA_EXTDOM_H_ */
diff --git 
a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_cmocka_tests.c 
b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_cmocka_tests.c
index 
d5bacd7e8c9dc0a71eea70162406c7e5b67384ad..586b58b0fd4c7610e9cb4643b6dae04f9d22b8ab
 100644
--- a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_cmocka_tests.c
+++ b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_cmocka_tests.c
@@ -213,6 +213,47 @@ void test_getgrgid_r_wrapper(void **state)
 free(buf);
 }
 
+void extdom_req_setup(void **state)
+{
+struct extdom_req *req;
+
+req = calloc(sizeof(struct extdom_req), 1);
+assert_non_null(req);
+
+*state = req;
+}
+
+void extdom_req_teardown(void **state)
+{
+struct extdom_req *req;
+
+req = (struct extdom_req *) *state;
+
+free_req_data(req);
+}
+
+void test_set_err_msg(void **state)
+{
+struct extdom_req *req;
+
+req = (struct extdom_req *) *state;
+assert_null(req->err_msg);
+
+set_err_msg(NULL, NULL);
+assert_null(req->err_msg);
+
+set_err_msg(req, NULL);
+assert_null(req-

Re: [Freeipa-devel] [PATCH 140] extdom: migrate check-based test to cmocka

[Sumit Bose]

On Wed, Mar 04, 2015 at 06:42:05PM +0100, Sumit Bose wrote:
> Hi,
> 
> this is the first patch for https://fedorahosted.org/freeipa/ticket/4922
> which converts the check-based tests of the extdom plugin to cmocka.
> 
> bye,
> Sumit

Rebased version attached.

bye,
Sumit
From c801df101baa41146a06a70ff4075e308905577b Mon Sep 17 00:00:00 2001
From: Sumit Bose 
Date: Mon, 9 Feb 2015 18:12:01 +0100
Subject: [PATCH] extdom: migrate check-based test to cmocka

Besides moving the existing tests to cmocka two new tests are added
which were missing from the old tests.

Related to https://fedorahosted.org/freeipa/ticket/4922
---
 .../ipa-slapi-plugins/ipa-extdom-extop/Makefile.am |  20 --
 .../ipa-extdom-extop/ipa_extdom.h  |  14 ++
 .../ipa-extdom-extop/ipa_extdom_cmocka_tests.c | 156 +++-
 .../ipa-extdom-extop/ipa_extdom_common.c   |  28 +--
 .../ipa-extdom-extop/ipa_extdom_tests.c| 203 -
 5 files changed, 176 insertions(+), 245 deletions(-)
 delete mode 100644 
daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_tests.c

diff --git a/daemons/ipa-slapi-plugins/ipa-extdom-extop/Makefile.am 
b/daemons/ipa-slapi-plugins/ipa-extdom-extop/Makefile.am
index 
a1679812ef3c5de8c6e18433cbb991a99ad0b6c8..9c2fa1c6a5f95ba06b33c0a5b560939863a88f0e
 100644
--- a/daemons/ipa-slapi-plugins/ipa-extdom-extop/Makefile.am
+++ b/daemons/ipa-slapi-plugins/ipa-extdom-extop/Makefile.am
@@ -38,11 +38,6 @@ libipa_extdom_extop_la_LIBADD =  \
 TESTS =
 check_PROGRAMS =
 
-if HAVE_CHECK
-TESTS += extdom_tests
-check_PROGRAMS += extdom_tests
-endif
-
 if HAVE_CMOCKA
 if HAVE_NSS_WRAPPER
 TESTS_ENVIRONMENT = . ./test_data/test_setup.sh;
@@ -51,21 +46,6 @@ check_PROGRAMS += extdom_cmocka_tests
 endif
 endif
 
-extdom_tests_SOURCES = \
-   ipa_extdom_tests.c  \
-   ipa_extdom_common.c \
-   $(NULL)
-extdom_tests_CFLAGS = $(CHECK_CFLAGS)
-extdom_tests_LDFLAGS = \
-   -rpath $(shell pkg-config --libs-only-L dirsrv | sed -e 's/-L//') \
-   $(NULL)
-extdom_tests_LDADD =   \
-   $(CHECK_LIBS)   \
-   $(LDAP_LIBS)\
-   $(DIRSRV_LIBS)  \
-   $(SSSNSSIDMAP_LIBS) \
-   $(NULL)
-
 extdom_cmocka_tests_SOURCES =  \
ipa_extdom_cmocka_tests.c   \
ipa_extdom_common.c \
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 
0d5d55d2fb0ece95466b0225b145a4edeef18efa..65dd43ea35726db6231386a0fcbba9be1bd71412
 100644
--- a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h
+++ b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom.h
@@ -185,5 +185,19 @@ int getgrnam_r_wrapper(size_t buf_max, const char *name,
struct group *grp, char **_buf, size_t *_buf_len);
 int getgrgid_r_wrapper(size_t buf_max, gid_t gid,
struct group *grp, char **_buf, size_t *_buf_len);
+int pack_ber_sid(const char *sid, struct berval **berval);
+int pack_ber_name(const char *domain_name, const char *name,
+  struct berval **berval);
+int pack_ber_user(struct ipa_extdom_ctx *ctx,
+  enum response_types response_type,
+  const char *domain_name, const char *user_name,
+  uid_t uid, gid_t gid,
+  const char *gecos, const char *homedir,
+  const char *shell, struct sss_nss_kv *kv_list,
+  struct berval **berval);
+int pack_ber_group(enum response_types response_type,
+   const char *domain_name, const char *group_name,
+   gid_t gid, char **members, struct sss_nss_kv *kv_list,
+   struct berval **berval);
 void set_err_msg(struct extdom_req *req, const char *format, ...);
 #endif /* _IPA_EXTDOM_H_ */
diff --git 
a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_cmocka_tests.c 
b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_cmocka_tests.c
index 
586b58b0fd4c7610e9cb4643b6dae04f9d22b8ab..42d588d08a96f8a26345f85aade9523e05f6f56e
 100644
--- a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_cmocka_tests.c
+++ b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_cmocka_tests.c
@@ -213,30 +213,46 @@ void test_getgrgid_r_wrapper(void **state)
 free(buf);
 }
 
+struct test_data {
+struct extdom_req *req;
+struct ipa_extdom_ctx *ctx;
+};
+
 void extdom_req_setup(void **state)
 {
-struct extdom_req *req;
+struct test_data *test_data;
 
-req = calloc(sizeof(struct extdom_req), 1);
-assert_non_null(req);
+test_data = calloc(sizeof(struct test_data), 1);
+assert_non_null(test_data);
 
-*state = req;
+test_data->req = calloc(sizeof(struct extdom_req), 1);
+assert_non_null(test_data->req);
+
+test_data->ctx = calloc(sizeof(struct ipa_extdom_ctx), 1);
+assert_non_null(test_data->req);
+
+*state = test_data;
 }
 
 void extdom_req_tea

Re: [Freeipa-devel] [PATCH 0208] Respect --test option in upgrade plugins

[Martin Basti]

On 13/03/15 11:55, Petr Spacek wrote:

On 13.3.2015 11:34, Jan Cholasta wrote:

Dne 13.3.2015 v 11:17 Martin Kosek napsal(a):

On 03/13/2015 11:00 AM, Petr Spacek wrote:

On 13.3.2015 10:42, Alexander Bokovoy wrote:

On Fri, 13 Mar 2015, Petr Spacek wrote:

On 13.3.2015 10:18, Martin Kosek wrote:

On 03/12/2015 05:10 PM, Rob Crittenden wrote:

Petr Spacek wrote:

On 12.3.2015 16:23, Rob Crittenden wrote:

David Kupka wrote:

On 03/06/2015 06:00 PM, Martin Basti wrote:

Upgrade plugins which modify LDAP data directly should not be
executed
in --test mode.

This patch is a workaround, to ensure update with --test
option will not
modify any LDAP data.

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

Patch attached.




Ideally we want to fix all plugins to dry-run the upgrade not
just skip
when there is '--test' option but it is a good first step.
Works for me, ACK.


I agree that this breaks the spirit of --test and think it
should be
fixed before committing.

Considering how often is the option is used ... I do not think
that this
requires 'proper' fix now. It was broken for *years* so this
patch is a huge
improvement and IMHO should be commited in current form. We can
re-visit it
later on, open a ticket :-)


No. There is no rush for this, at least not for the promise of a
future
fix that will never come.

I checked the code and to me, the proper fix looks like instrumenting
ldap.update_entry calls in upgrade plugins with

if options.test:
 log message
else
 do the update

right? I see just couple places that would need to be updated:

$ git grep -E "(ldap|conn).update_entry" ipaserver/install/plugins
ipaserver/install/plugins/dns.py:
ldap.update_entry(container_entry)
ipaserver/install/plugins/fix_replica_agreements.py:
repl.conn.update_entry(replica)
ipaserver/install/plugins/fix_replica_agreements.py:
repl.conn.update_entry(replica)
ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_managed_permissions.py:
ldap.update_entry(base_entry)
ipaserver/install/plugins/update_managed_permissions.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_pacs.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_referint.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_services.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_uniqueness.py:
ldap.update_entry(uid_uniqueness_plugin)


So from my POV, very quick fix. In that case, I would also prefer a
fix now
than a ticket that would never be done.

I really dislike this approach because I consider it flawed by
design. Plugin
author has to think about it all the time and do not forget to add if
otherwise ... too bad.

I can see two 'safer' ways to do that:
- LDAP transactions :-)
- 'mock_writes=True' option in LDAP backend which would print
modlists instead
of applying them (and return success to the caller).

Both cases eliminate the need to scatter 'ifs' all over update
plugins and do
not add risk of forgetting about one of them when adding/changing
plugin code.

I like idea about mock_writes=True. However, I think we still need to
make
sure plugin writers rely on options.test value to see that we aren't
going to write the data. The reason for it is that we might get into
configurations where plugins would be doing updates based on earlier
performed tasks.  If task configuration is not going to be written, its
status will never be correct and plugin would get an error.

That is exactly why I mentioned LDAP transactions. There is no other
way how
to test complex plugins which actually read own writes (except mocking
the
whole LDAP interface somewhere).

While this may be a good idea long term, I do not think any of us is
considering implementing the LDAP transaction support within work on
this refactoring.

So in this thread, let us focus on how to fix options.test mid-term. I
currently see 2 proposed ways:
- Making the plugins aware of options.test
- Make ldap2 write operations only print the update and not do it.
Although thinking of this approach, I think it may make some plugins
like DNS loop forever. IIRC, at least DNS upgrade plugin have loop
- search for all unfixed DNS zones
- fix them with ldap update
- was the search truncated, i.e.  are there more zones to update?
  if yes, go back to start

  - Make the plugins not call {add,update,delete}_entry themselves but rather
return the updates like they should. This is what the ticket
() requests and what should be
done to make --test work for them.

How do you propose to handle iterative updates like the DNS upgrade mentioned
by Martin^1? Return set of updates along with boolean 'call me again'?
Something else?

So instead of DNS commands logic, which can be used in plugin, we should 
reimplement the dnzone commands in upgrade plugin, to get modlist? And 
keep watching it and do sam

Re: [Freeipa-devel] [PATCH 0208] Respect --test option in upgrade plugins

[Petr Spacek]

On 13.3.2015 12:01, Martin Basti wrote:
> On 13/03/15 11:55, Petr Spacek wrote:
>> On 13.3.2015 11:34, Jan Cholasta wrote:
>>> Dne 13.3.2015 v 11:17 Martin Kosek napsal(a):
 On 03/13/2015 11:00 AM, Petr Spacek wrote:
> On 13.3.2015 10:42, Alexander Bokovoy wrote:
>> On Fri, 13 Mar 2015, Petr Spacek wrote:
>>> On 13.3.2015 10:18, Martin Kosek wrote:
 On 03/12/2015 05:10 PM, Rob Crittenden wrote:
> Petr Spacek wrote:
>> On 12.3.2015 16:23, Rob Crittenden wrote:
>>> David Kupka wrote:
 On 03/06/2015 06:00 PM, Martin Basti wrote:
> Upgrade plugins which modify LDAP data directly should not be
> executed
> in --test mode.
>
> This patch is a workaround, to ensure update with --test
> option will not
> modify any LDAP data.
>
> https://fedorahosted.org/freeipa/ticket/3448
>
> Patch attached.
>
>
>
 Ideally we want to fix all plugins to dry-run the upgrade not
 just skip
 when there is '--test' option but it is a good first step.
 Works for me, ACK.

>>> I agree that this breaks the spirit of --test and think it
>>> should be
>>> fixed before committing.
>> Considering how often is the option is used ... I do not think
>> that this
>> requires 'proper' fix now. It was broken for *years* so this
>> patch is a huge
>> improvement and IMHO should be commited in current form. We can
>> re-visit it
>> later on, open a ticket :-)
>>
> No. There is no rush for this, at least not for the promise of a
> future
> fix that will never come.
 I checked the code and to me, the proper fix looks like instrumenting
 ldap.update_entry calls in upgrade plugins with

 if options.test:
  log message
 else
  do the update

 right? I see just couple places that would need to be updated:

 $ git grep -E "(ldap|conn).update_entry" ipaserver/install/plugins
 ipaserver/install/plugins/dns.py:
 ldap.update_entry(container_entry)
 ipaserver/install/plugins/fix_replica_agreements.py:
 repl.conn.update_entry(replica)
 ipaserver/install/plugins/fix_replica_agreements.py:
 repl.conn.update_entry(replica)
 ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
 ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
 ipaserver/install/plugins/update_managed_permissions.py:
 ldap.update_entry(base_entry)
 ipaserver/install/plugins/update_managed_permissions.py:
 ldap.update_entry(entry)
 ipaserver/install/plugins/update_pacs.py:
 ldap.update_entry(entry)
 ipaserver/install/plugins/update_referint.py:
 ldap.update_entry(entry)
 ipaserver/install/plugins/update_services.py: ldap.update_entry(entry)
 ipaserver/install/plugins/update_uniqueness.py:
 ldap.update_entry(uid_uniqueness_plugin)


 So from my POV, very quick fix. In that case, I would also prefer a
 fix now
 than a ticket that would never be done.
>>> I really dislike this approach because I consider it flawed by
>>> design. Plugin
>>> author has to think about it all the time and do not forget to add if
>>> otherwise ... too bad.
>>>
>>> I can see two 'safer' ways to do that:
>>> - LDAP transactions :-)
>>> - 'mock_writes=True' option in LDAP backend which would print
>>> modlists instead
>>> of applying them (and return success to the caller).
>>>
>>> Both cases eliminate the need to scatter 'ifs' all over update
>>> plugins and do
>>> not add risk of forgetting about one of them when adding/changing
>>> plugin code.
>> I like idea about mock_writes=True. However, I think we still need to
>> make
>> sure plugin writers rely on options.test value to see that we aren't
>> going to write the data. The reason for it is that we might get into
>> configurations where plugins would be doing updates based on earlier
>> performed tasks.  If task configuration is not going to be written, its
>> status will never be correct and plugin would get an error.
> That is exactly why I mentioned LDAP transactions. There is no other
> way how
> to test complex plugins which actually read own writes (except mocking
> the
> whole LDAP interface somewhere).
 While this may be a good idea long term, I do not think any of us is
 considering implementing the LDAP transaction support within work on
 this refactoring.

 So in this thread, let us focus on how to fix options.test mid-ter

Re: [Freeipa-devel] [PATCH 0208] Respect --test option in upgrade plugins

[Jan Cholasta]

Dne 13.3.2015 v 12:08 Petr Spacek napsal(a):

On 13.3.2015 12:01, Martin Basti wrote:

On 13/03/15 11:55, Petr Spacek wrote:

On 13.3.2015 11:34, Jan Cholasta wrote:

Dne 13.3.2015 v 11:17 Martin Kosek napsal(a):

On 03/13/2015 11:00 AM, Petr Spacek wrote:

On 13.3.2015 10:42, Alexander Bokovoy wrote:

On Fri, 13 Mar 2015, Petr Spacek wrote:

On 13.3.2015 10:18, Martin Kosek wrote:

On 03/12/2015 05:10 PM, Rob Crittenden wrote:

Petr Spacek wrote:

On 12.3.2015 16:23, Rob Crittenden wrote:

David Kupka wrote:

On 03/06/2015 06:00 PM, Martin Basti wrote:

Upgrade plugins which modify LDAP data directly should not be
executed
in --test mode.

This patch is a workaround, to ensure update with --test
option will not
modify any LDAP data.

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

Patch attached.




Ideally we want to fix all plugins to dry-run the upgrade not
just skip
when there is '--test' option but it is a good first step.
Works for me, ACK.


I agree that this breaks the spirit of --test and think it
should be
fixed before committing.

Considering how often is the option is used ... I do not think
that this
requires 'proper' fix now. It was broken for *years* so this
patch is a huge
improvement and IMHO should be commited in current form. We can
re-visit it
later on, open a ticket :-)


No. There is no rush for this, at least not for the promise of a
future
fix that will never come.

I checked the code and to me, the proper fix looks like instrumenting
ldap.update_entry calls in upgrade plugins with

if options.test:
  log message
else
  do the update

right? I see just couple places that would need to be updated:

$ git grep -E "(ldap|conn).update_entry" ipaserver/install/plugins
ipaserver/install/plugins/dns.py:
ldap.update_entry(container_entry)
ipaserver/install/plugins/fix_replica_agreements.py:
repl.conn.update_entry(replica)
ipaserver/install/plugins/fix_replica_agreements.py:
repl.conn.update_entry(replica)
ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_managed_permissions.py:
ldap.update_entry(base_entry)
ipaserver/install/plugins/update_managed_permissions.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_pacs.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_referint.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_services.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_uniqueness.py:
ldap.update_entry(uid_uniqueness_plugin)


So from my POV, very quick fix. In that case, I would also prefer a
fix now
than a ticket that would never be done.

I really dislike this approach because I consider it flawed by
design. Plugin
author has to think about it all the time and do not forget to add if
otherwise ... too bad.

I can see two 'safer' ways to do that:
- LDAP transactions :-)
- 'mock_writes=True' option in LDAP backend which would print
modlists instead
of applying them (and return success to the caller).

Both cases eliminate the need to scatter 'ifs' all over update
plugins and do
not add risk of forgetting about one of them when adding/changing
plugin code.

I like idea about mock_writes=True. However, I think we still need to
make
sure plugin writers rely on options.test value to see that we aren't
going to write the data. The reason for it is that we might get into
configurations where plugins would be doing updates based on earlier
performed tasks.  If task configuration is not going to be written, its
status will never be correct and plugin would get an error.

That is exactly why I mentioned LDAP transactions. There is no other
way how
to test complex plugins which actually read own writes (except mocking
the
whole LDAP interface somewhere).

While this may be a good idea long term, I do not think any of us is
considering implementing the LDAP transaction support within work on
this refactoring.

So in this thread, let us focus on how to fix options.test mid-term. I
currently see 2 proposed ways:
- Making the plugins aware of options.test
- Make ldap2 write operations only print the update and not do it.
Although thinking of this approach, I think it may make some plugins
like DNS loop forever. IIRC, at least DNS upgrade plugin have loop
 - search for all unfixed DNS zones
 - fix them with ldap update
 - was the search truncated, i.e.  are there more zones to update?
   if yes, go back to start

   - Make the plugins not call {add,update,delete}_entry themselves but rather
return the updates like they should. This is what the ticket
() requests and what should be
done to make --test work for them.

How do you propose to handle iterative updates like the DNS upgrade mentioned
by Martin^1? Return set of updates along with boolean 'call me again'?
Something else?


So instead of DNS commands logic, which can be used in plugin, we should
reimp

Re: [Freeipa-devel] [PATCH 0208] Respect --test option in upgrade plugins

[Martin Basti]

On 13/03/15 10:18, Martin Kosek wrote:

On 03/12/2015 05:10 PM, Rob Crittenden wrote:

Petr Spacek wrote:

On 12.3.2015 16:23, Rob Crittenden wrote:

David Kupka wrote:

On 03/06/2015 06:00 PM, Martin Basti wrote:
Upgrade plugins which modify LDAP data directly should not be 
executed

in --test mode.

This patch is a workaround, to ensure update with --test option 
will not

modify any LDAP data.

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

Patch attached.





Ideally we want to fix all plugins to dry-run the upgrade not just 
skip

when there is '--test' option but it is a good first step.
Works for me, ACK.



I agree that this breaks the spirit of --test and think it should be
fixed before committing.


Considering how often is the option is used ... I do not think that 
this
requires 'proper' fix now. It was broken for *years* so this patch 
is a huge
improvement and IMHO should be commited in current form. We can 
re-visit it

later on, open a ticket :-)



No. There is no rush for this, at least not for the promise of a future
fix that will never come.


I checked the code and to me, the proper fix looks like instrumenting 
ldap.update_entry calls in upgrade plugins with


if options.test:
   log message
else
   do the update

right? I see just couple places that would need to be updated:

$ git grep -E "(ldap|conn).update_entry" ipaserver/install/plugins
ipaserver/install/plugins/dns.py: ldap.update_entry(container_entry)
ipaserver/install/plugins/fix_replica_agreements.py: 
repl.conn.update_entry(replica)
ipaserver/install/plugins/fix_replica_agreements.py: 
repl.conn.update_entry(replica)

ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_managed_permissions.py: 
ldap.update_entry(base_entry)
ipaserver/install/plugins/update_managed_permissions.py: 
ldap.update_entry(entry)

ipaserver/install/plugins/update_pacs.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_referint.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_services.py: ldap.update_entry(entry)
ipaserver/install/plugins/update_uniqueness.py: 
ldap.update_entry(uid_uniqueness_plugin)



So from my POV, very quick fix. In that case, I would also prefer a 
fix now than a ticket that would never be done.


Martin

And ldap.add_entry, del_entry, ipa add/mod/del commands

--
Martin Basti

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0208] Respect --test option in upgrade plugins

[Martin Basti]

On 13/03/15 12:50, Jan Cholasta wrote:

Dne 13.3.2015 v 12:08 Petr Spacek napsal(a):

On 13.3.2015 12:01, Martin Basti wrote:

On 13/03/15 11:55, Petr Spacek wrote:

On 13.3.2015 11:34, Jan Cholasta wrote:

Dne 13.3.2015 v 11:17 Martin Kosek napsal(a):

On 03/13/2015 11:00 AM, Petr Spacek wrote:

On 13.3.2015 10:42, Alexander Bokovoy wrote:

On Fri, 13 Mar 2015, Petr Spacek wrote:

On 13.3.2015 10:18, Martin Kosek wrote:

On 03/12/2015 05:10 PM, Rob Crittenden wrote:

Petr Spacek wrote:

On 12.3.2015 16:23, Rob Crittenden wrote:

David Kupka wrote:

On 03/06/2015 06:00 PM, Martin Basti wrote:
Upgrade plugins which modify LDAP data directly should 
not be

executed
in --test mode.

This patch is a workaround, to ensure update with --test
option will not
modify any LDAP data.

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

Patch attached.



Ideally we want to fix all plugins to dry-run the upgrade 
not

just skip
when there is '--test' option but it is a good first step.
Works for me, ACK.


I agree that this breaks the spirit of --test and think it
should be
fixed before committing.

Considering how often is the option is used ... I do not think
that this
requires 'proper' fix now. It was broken for *years* so this
patch is a huge
improvement and IMHO should be commited in current form. We 
can

re-visit it
later on, open a ticket :-)

No. There is no rush for this, at least not for the promise 
of a

future
fix that will never come.
I checked the code and to me, the proper fix looks like 
instrumenting

ldap.update_entry calls in upgrade plugins with

if options.test:
  log message
else
  do the update

right? I see just couple places that would need to be updated:

$ git grep -E "(ldap|conn).update_entry" 
ipaserver/install/plugins

ipaserver/install/plugins/dns.py:
ldap.update_entry(container_entry)
ipaserver/install/plugins/fix_replica_agreements.py:
repl.conn.update_entry(replica)
ipaserver/install/plugins/fix_replica_agreements.py:
repl.conn.update_entry(replica)
ipaserver/install/plugins/update_idranges.py: 
ldap.update_entry(entry)
ipaserver/install/plugins/update_idranges.py: 
ldap.update_entry(entry)

ipaserver/install/plugins/update_managed_permissions.py:
ldap.update_entry(base_entry)
ipaserver/install/plugins/update_managed_permissions.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_pacs.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_referint.py:
ldap.update_entry(entry)
ipaserver/install/plugins/update_services.py: 
ldap.update_entry(entry)

ipaserver/install/plugins/update_uniqueness.py:
ldap.update_entry(uid_uniqueness_plugin)


So from my POV, very quick fix. In that case, I would also 
prefer a

fix now
than a ticket that would never be done.

I really dislike this approach because I consider it flawed by
design. Plugin
author has to think about it all the time and do not forget to 
add if

otherwise ... too bad.

I can see two 'safer' ways to do that:
- LDAP transactions :-)
- 'mock_writes=True' option in LDAP backend which would print
modlists instead
of applying them (and return success to the caller).

Both cases eliminate the need to scatter 'ifs' all over update
plugins and do
not add risk of forgetting about one of them when adding/changing
plugin code.
I like idea about mock_writes=True. However, I think we still 
need to

make
sure plugin writers rely on options.test value to see that we 
aren't
going to write the data. The reason for it is that we might get 
into
configurations where plugins would be doing updates based on 
earlier
performed tasks.  If task configuration is not going to be 
written, its

status will never be correct and plugin would get an error.
That is exactly why I mentioned LDAP transactions. There is no 
other

way how
to test complex plugins which actually read own writes (except 
mocking

the
whole LDAP interface somewhere).

While this may be a good idea long term, I do not think any of us is
considering implementing the LDAP transaction support within work on
this refactoring.

So in this thread, let us focus on how to fix options.test 
mid-term. I

currently see 2 proposed ways:
- Making the plugins aware of options.test
- Make ldap2 write operations only print the update and not do it.
Although thinking of this approach, I think it may make some plugins
like DNS loop forever. IIRC, at least DNS upgrade plugin have loop
 - search for all unfixed DNS zones
 - fix them with ldap update
 - was the search truncated, i.e.  are there more zones to 
update?

   if yes, go back to start
   - Make the plugins not call {add,update,delete}_entry 
themselves but rather

return the updates like they should. This is what the ticket
() requests and what 
should be

done to make --test work for them.
How do you propose to handle iterative updates like the DNS upgrade 
mentioned

by Martin^1? Return set of updates along with boolean 'call me again'?
Something else?

So in

Re: [Freeipa-devel] [PATCH 0044] Man pages: ipa-replica-prepare can only be created on first master

[Gabe Alford]

On Thu, Mar 12, 2015 at 8:26 AM, Martin Kosek  wrote:

> On 03/12/2015 02:37 PM, Gabe Alford wrote:
> > Hello,
> >
> > Fix for https://fedorahosted.org/freeipa/ticket/4944. Since there seems
> to
> > be plenty of time, I added it to the freeipa-4-1 branch.
>
> Thanks Gabe! I would still suggest against moving the tickets to milestones
> yourself, all new tickets should still undergo the weekly triage so that
> all
> core developers see it and we can decide the target milestone.
>

Sorry about that.


> With this one, it would likely indeed end in 4.1.x, especially given you
> contributed a patch, but still...
>
> For the patch itself, I still think the wording is not as should be:
>
> - following line is not entirely trie, you can install can create replica
> also
> on servers installed with ipa-replica-install :-)
> +A replica can be created on any IPA master server installed with
> ipa\-server\-install.
>
> - Following line may also use some rewording:
> However if you want to create a replica as a redundant CA with an existing
> replica or master, ipa\-replica\-prepare should be run on a replica or
> master
> that contains the CA.
>
> Maybe we should add subsection to DESCRIPTION section, with following
> lines:
>

What should the .SS be called? Replica Info? PKI INFO? Preparation
Requirements?


> - A replica should only be installed on the same or higher version of IPA
> on
> the remote system.
>
- A replica with PKI can only be installed from replica file prepared on a
> master with PKI

Makes sense?
>

We will see if the coffee is working today. :)


> Martin
>
-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0044] Man pages: ipa-replica-prepare can only be created on first master

[Martin Kosek]

On 03/13/2015 02:13 PM, Gabe Alford wrote:

On Thu, Mar 12, 2015 at 8:26 AM, Martin Kosek mailto:mko...@redhat.com>> wrote:

On 03/12/2015 02:37 PM, Gabe Alford wrote:
 > Hello,
 >
 > Fix for https://fedorahosted.org/freeipa/ticket/4944. Since there seems 
to
 > be plenty of time, I added it to the freeipa-4-1 branch.

Thanks Gabe! I would still suggest against moving the tickets to milestones
yourself, all new tickets should still undergo the weekly triage so that all
core developers see it and we can decide the target milestone.


Sorry about that.

With this one, it would likely indeed end in 4.1.x, especially given you
contributed a patch, but still...

For the patch itself, I still think the wording is not as should be:

- following line is not entirely trie, you can install can create replica 
also
on servers installed with ipa-replica-install :-)
+A replica can be created on any IPA master server installed with
ipa\-server\-install.

- Following line may also use some rewording:
However if you want to create a replica as a redundant CA with an existing
replica or master, ipa\-replica\-prepare should be run on a replica or 
master
that contains the CA.

Maybe we should add subsection to DESCRIPTION section, with following lines:


What should the .SS be called? Replica Info? PKI INFO? Preparation Requirements?


"Limitations"?




- A replica should only be installed on the same or higher version of IPA on
the remote system.

- A replica with PKI can only be installed from replica file prepared on a
master with PKI

Makes sense?


We will see if the coffee is working today. :)

Martin




--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] Purpose of default user group

[Petr Vobornik]

Thanks all for the answers.

On 03/10/2015 03:27 PM, Rob Crittenden wrote:

Petr Vobornik wrote:

In ipa migrate-ds we also set the group to all users who are not member
of anything. Why is it important for a user to be a member of a group?


Every POSIX user needs a default GID. We don't create user-private
groups for migrated users.



How should default GID be set during migration? IMHO there are two issues:

1. ipausers group is not a POSIX group. Which, btw, also creates this 
nice issue:

  $ ipa user-add fbar --noprivate
  First name: Foo
  Last name: Bar
  ipa: ERROR: Default group for new users is not POSIX

2. migrated users have to be POSIX therefore they have gidnumber and 
migrate-ds checks for its presence. But the command doesn't do anything 
with the GID number later even if the group doesn't exist nor in a step 
where default group is set. Therefore, default group, even if POSIX, 
would not work for this use case(set default GID number).


Q: Is it expected that user private groups will be migrated? (e.g. for 
migration from other FreeIPA instance). If not, then there would be a 
lot of users without a private group with the same GID number as UID number.


Q: Why don't we allow to create user private group? What would be better 
if migrating from FreeIPA instance: migrate private groups or create new 
private groups using Managed Entries plugin?

--
Petr Vobornik

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0208] Respect --test option in upgrade plugins

[Rob Crittenden]

Jan Cholasta wrote:
> Dne 13.3.2015 v 12:08 Petr Spacek napsal(a):
>> On 13.3.2015 12:01, Martin Basti wrote:
>>> On 13/03/15 11:55, Petr Spacek wrote:
 On 13.3.2015 11:34, Jan Cholasta wrote:
> Dne 13.3.2015 v 11:17 Martin Kosek napsal(a):
>> On 03/13/2015 11:00 AM, Petr Spacek wrote:
>>> On 13.3.2015 10:42, Alexander Bokovoy wrote:
 On Fri, 13 Mar 2015, Petr Spacek wrote:
> On 13.3.2015 10:18, Martin Kosek wrote:
>> On 03/12/2015 05:10 PM, Rob Crittenden wrote:
>>> Petr Spacek wrote:
 On 12.3.2015 16:23, Rob Crittenden wrote:
> David Kupka wrote:
>> On 03/06/2015 06:00 PM, Martin Basti wrote:
>>> Upgrade plugins which modify LDAP data directly should
>>> not be
>>> executed
>>> in --test mode.
>>>
>>> This patch is a workaround, to ensure update with --test
>>> option will not
>>> modify any LDAP data.
>>>
>>> https://fedorahosted.org/freeipa/ticket/3448
>>>
>>> Patch attached.
>>>
>>>
>>>
>> Ideally we want to fix all plugins to dry-run the upgrade not
>> just skip
>> when there is '--test' option but it is a good first step.
>> Works for me, ACK.
>>
> I agree that this breaks the spirit of --test and think it
> should be
> fixed before committing.
 Considering how often is the option is used ... I do not think
 that this
 requires 'proper' fix now. It was broken for *years* so this
 patch is a huge
 improvement and IMHO should be commited in current form. We can
 re-visit it
 later on, open a ticket :-)

>>> No. There is no rush for this, at least not for the promise of a
>>> future
>>> fix that will never come.
>> I checked the code and to me, the proper fix looks like
>> instrumenting
>> ldap.update_entry calls in upgrade plugins with
>>
>> if options.test:
>>   log message
>> else
>>   do the update
>>
>> right? I see just couple places that would need to be updated:
>>
>> $ git grep -E "(ldap|conn).update_entry"
>> ipaserver/install/plugins
>> ipaserver/install/plugins/dns.py:
>> ldap.update_entry(container_entry)
>> ipaserver/install/plugins/fix_replica_agreements.py:
>> repl.conn.update_entry(replica)
>> ipaserver/install/plugins/fix_replica_agreements.py:
>> repl.conn.update_entry(replica)
>> ipaserver/install/plugins/update_idranges.py:
>> ldap.update_entry(entry)
>> ipaserver/install/plugins/update_idranges.py:
>> ldap.update_entry(entry)
>> ipaserver/install/plugins/update_managed_permissions.py:
>> ldap.update_entry(base_entry)
>> ipaserver/install/plugins/update_managed_permissions.py:
>> ldap.update_entry(entry)
>> ipaserver/install/plugins/update_pacs.py:
>> ldap.update_entry(entry)
>> ipaserver/install/plugins/update_referint.py:
>> ldap.update_entry(entry)
>> ipaserver/install/plugins/update_services.py:
>> ldap.update_entry(entry)
>> ipaserver/install/plugins/update_uniqueness.py:
>> ldap.update_entry(uid_uniqueness_plugin)
>>
>>
>> So from my POV, very quick fix. In that case, I would also
>> prefer a
>> fix now
>> than a ticket that would never be done.
> I really dislike this approach because I consider it flawed by
> design. Plugin
> author has to think about it all the time and do not forget to
> add if
> otherwise ... too bad.
>
> I can see two 'safer' ways to do that:
> - LDAP transactions :-)
> - 'mock_writes=True' option in LDAP backend which would print
> modlists instead
> of applying them (and return success to the caller).
>
> Both cases eliminate the need to scatter 'ifs' all over update
> plugins and do
> not add risk of forgetting about one of them when adding/changing
> plugin code.
 I like idea about mock_writes=True. However, I think we still
 need to
 make
 sure plugin writers rely on options.test value to see that we
 aren't
 going to write the data. The reason for it is that we might get
 into
 configurations where plugins would be doing updates based on
 earlier
 performed tasks.  If task configuration is not going to be
 written, its
 status will never be correct and plugin would get an error.
>>> That is exactly why I mentioned LDAP tran

Re: [Freeipa-devel] [PATCHES 0015-0017] consolidation of various Kerberos auth methods in FreeIPA code

[Simo Sorce]

On Wed, 2015-03-11 at 12:42 +0100, Petr Spacek wrote:
> I would like to see new code compatible with Python 3. Here I'm not
> sure what is the generic solution for xrange but in this particular
> case I would recommend you to use just range. Attempts variable should
> have small values so the x/range differences do not matter here.

To be honest, I do not think we should care for this code specifically.
The move to Python3 will require python-krbV code to be completely
removed, and we should use python-gssapi instead. I will help with the
"how do I kinit" part once we can do that.

Now, I do not know if it is worth NACKing this patch in that sense, or
letting it through, it depends on whether we want to be able to backport
this specific fix or whether it is ok to have it only available on
python-gssapi capable machines.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0044] Man pages: ipa-replica-prepare can only be created on first master

[Gabe Alford]

"Limitations" is fine with me. Updated patch attached.

On Fri, Mar 13, 2015 at 7:17 AM, Martin Kosek  wrote:

> On 03/13/2015 02:13 PM, Gabe Alford wrote:
>
>> On Thu, Mar 12, 2015 at 8:26 AM, Martin Kosek > > wrote:
>>
>> On 03/12/2015 02:37 PM, Gabe Alford wrote:
>>  > Hello,
>>  >
>>  > Fix for https://fedorahosted.org/freeipa/ticket/4944. Since there
>> seems to
>>  > be plenty of time, I added it to the freeipa-4-1 branch.
>>
>> Thanks Gabe! I would still suggest against moving the tickets to
>> milestones
>> yourself, all new tickets should still undergo the weekly triage so
>> that all
>> core developers see it and we can decide the target milestone.
>>
>>
>> Sorry about that.
>>
>> With this one, it would likely indeed end in 4.1.x, especially given
>> you
>> contributed a patch, but still...
>>
>> For the patch itself, I still think the wording is not as should be:
>>
>> - following line is not entirely trie, you can install can create
>> replica also
>> on servers installed with ipa-replica-install :-)
>> +A replica can be created on any IPA master server installed with
>> ipa\-server\-install.
>>
>> - Following line may also use some rewording:
>> However if you want to create a replica as a redundant CA with an
>> existing
>> replica or master, ipa\-replica\-prepare should be run on a replica
>> or master
>> that contains the CA.
>>
>> Maybe we should add subsection to DESCRIPTION section, with following
>> lines:
>>
>>
>> What should the .SS be called? Replica Info? PKI INFO? Preparation
>> Requirements?
>>
>
> "Limitations"?
>
>
>
>>
>> - A replica should only be installed on the same or higher version of
>> IPA on
>> the remote system.
>>
>> - A replica with PKI can only be installed from replica file prepared
>> on a
>> master with PKI
>>
>> Makes sense?
>>
>>
>> We will see if the coffee is working today. :)
>>
>> Martin
>>
>>
>>
>
From 1a679b80db8b577b531a3bc825340f06e56b9886 Mon Sep 17 00:00:00 2001
From: Gabe 
Date: Fri, 13 Mar 2015 07:34:49 -0600
Subject: [PATCH] ipa-replica-prepare can only be created on the first master

- https://fedorahosted.org/freeipa/ticket/4944
---
 install/tools/man/ipa-replica-prepare.1 | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/install/tools/man/ipa-replica-prepare.1 b/install/tools/man/ipa-replica-prepare.1
index 1879d2ee88fc78fb755a702a2b2fe9a93e153b45..4c5ad3e8e49798eb33667903f2de1f35d83596c0 100644
--- a/install/tools/man/ipa-replica-prepare.1
+++ b/install/tools/man/ipa-replica-prepare.1
@@ -24,15 +24,17 @@ ipa\-replica\-prepare [\fIOPTION\fR]... hostname
 .SH "DESCRIPTION"
 Generates a replica file that may be used with ipa\-replica\-install to create a replica of an IPA server.
 
-A replica can only be created on an IPA server installed with ipa\-server\-install (the first server).
+A replica can be created on any IPA master or replica server.
 
 You must provide the fully\-qualified hostname of the machine you want to install the replica on and a host\-specific replica_file will be created. It is host\-specific because SSL server certificates are generated as part of the process and they are specific to a particular hostname.
 
 If IPA manages the DNS for your domain, you should either use the \fB\-\-ip\-address\fR option or add the forward and reverse records manually using IPA plugins.
 
 Once the file has been created it will be named replica\-hostname. This file can then be moved across the network to the target machine and a new IPA replica setup by running ipa\-replica\-install replica\-hostname.
-
+.SS "LIMITATIONS"
 A replica should only be installed on the same or higher version of IPA on the remote system.
+
+A replica with PKI can only be installed from a replica file prepared on a master with PKI.
 .SH "OPTIONS"
 .TP
 \fB\-\-dirsrv\-cert\-file\fR=\fIFILE\fR
-- 
1.8.3.1

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 0044] Man pages: ipa-replica-prepare can only be created on first master

[Martin Kosek]

On 03/13/2015 02:38 PM, Gabe Alford wrote:

"Limitations" is fine with me. Updated patch attached.



Works for me. I just changed the case of the subsection name to be consistent 
with man pages like ipa-client-install.


Thanks Gabe! ACK.

Pushed to:
master: fbf192f0e255c5f48e93f8838fc530b26f357deb
ipa-4-1: 169a37d1a8585528c88985e19255c40f63bc831f

Martin

--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

[Freeipa-devel] [PATCH] ipatests: port of p11helper test from github

[Milan Kubik]

Hi,

this is a patch with port of [1] to pytest.

[1]: https://github.com/spacekpe/freeipa-pkcs11/blob/master/python/run.py

Cheers,
Milan

>From 0bbd56eb04e9494ed008d212dabdf32cf6f36e17 Mon Sep 17 00:00:00 2001
From: Milan Kubik 
Date: Thu, 12 Mar 2015 16:52:33 +0100
Subject: [PATCH] ipatests: port of p11helper test from github

Ported the github hosted [1] script to use pytest's abilities
and included it in ipatests/test_ipapython directory.

[1]: https://github.com/spacekpe/freeipa-pkcs11/blob/master/python/run.py

https://fedorahosted.org/freeipa/ticket/4829
---
 ipatests/test_ipapython/test_ipap11helper.py | 222 +++
 make-lint|   2 +-
 2 files changed, 223 insertions(+), 1 deletion(-)
 create mode 100644 ipatests/test_ipapython/test_ipap11helper.py

diff --git a/ipatests/test_ipapython/test_ipap11helper.py b/ipatests/test_ipapython/test_ipap11helper.py
new file mode 100644
index ..b10cf9a27594aaa0041d54dcb3805d753e6adeb4
--- /dev/null
+++ b/ipatests/test_ipapython/test_ipap11helper.py
@@ -0,0 +1,222 @@
+# -*- coding: utf-8 -*-
+# Authors:
+#   Milan Kubik 
+#
+# Copyright (C) 2015  Red Hat
+# see file 'COPYING' for use and warranty information
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+"""
+Test the `ipapython/ipap11helper/p11helper.c` module.
+"""
+
+
+from binascii import hexlify
+import os
+import os.path
+import logging
+import sys
+import subprocess
+import tempfile
+
+import pytest
+
+import _ipap11helper
+
+config_data = """
+# SoftHSM v2 configuration file
+directories.tokendir = %s/tokens
+objectstore.backend = file
+"""
+
+libsofthsm = "/usr/lib64/pkcs11/libsofthsm2.so"
+
+logging.basicConfig(level=logging.INFO)
+log = logging.getLogger('t')
+
+@pytest.fixture(scope="module")
+def p11(request):
+token_path = tempfile.mkdtemp(prefix='pytest_', suffix='_pkcs11')
+os.chdir(token_path)
+os.mkdir('tokens')
+
+with open('softhsm2.conf', 'w') as cfg:
+cfg.write(config_data % token_path)
+
+os.environ['SOFTHSM2_CONF'] = os.path.join(token_path, 'softhsm2.conf')
+
+subprocess.check_call(['softhsm2-util', '--init-token', '--slot', '0',
+  '--label', 'test', '--pin', '1234', '--so-pin', '1234'])
+
+try:
+p11 = _ipap11helper.P11_Helper(0, "1234", libsofthsm)
+except _ipap11helper.Error:
+pytest.fail('Failed to initialize the helper object.', pytrace=False)
+
+def fin():
+try:
+p11.finalize()
+except _ipap11helper.Error:
+pytest.fail('Failed to finalize the helper object.', pytrace=False)
+finally:
+del os.environ['SOFTHSM2_CONF']
+
+request.addfinalizer(fin)
+
+return p11
+
+def str_to_hex(s):
+return ''.join("{:02x}".format(ord(c)) for c in s)
+
+class test_p11helper(object):
+def test_generate_master_key(self, p11):
+r = p11.generate_master_key(u"žžž-aest", "m", key_length=16, cka_wrap=True,
+cka_unwrap=True)
+assert r
+
+# replica 1
+def test_generate_replica_key_pair(self, p11):
+r = p11.generate_replica_key_pair(u"replica1", "id1", pub_cka_wrap=True,
+  priv_cka_unwrap=True)
+assert r
+
+def test_find_key(self, p11):
+rep1_pub = p11.find_keys(_ipap11helper.KEY_CLASS_PUBLIC_KEY, label=u"replica1", cka_wrap=True)
+assert len(rep1_pub) == 1, "replica key pair has to contain 1 pub key instead of %s" % len(rep1_pub)
+
+rep1_priv = p11.find_keys(_ipap11helper.KEY_CLASS_PRIVATE_KEY, label=u"replica1", cka_unwrap=True)
+assert len(rep1_priv) == 1, "replica key pair has to contain 1 private key instead of %s" % len(rep1_priv)
+
+def test_find_key_by_uri(self, p11):
+rep1_pub = p11.find_keys(uri="pkcs11:object=replica1;objecttype=public")
+assert len(rep1_pub) == 1, "replica key pair has to contain 1 pub key instead of %s" % len(rep1_pub)
+
+def test_get_attribute_from_object(self, p11):
+rep1_pub = p11.find_keys(_ipap11helper.KEY_CLASS_PUBLIC_KEY, label=u"replica1", cka_wrap=True)[0]
+
+iswrap = p11.get_attribute(rep1_pub, _ipap11helper.CKA_WRAP)
+assert iswrap is True, "replica public key has to have CKA_WRAP = TRUE"
+
+
+# replica 2
+def test_gene

Re: [Freeipa-devel] Purpose of default user group

[Rob Crittenden]

Petr Vobornik wrote:
> Thanks all for the answers.
> 
> On 03/10/2015 03:27 PM, Rob Crittenden wrote:
>> Petr Vobornik wrote:
>>> In ipa migrate-ds we also set the group to all users who are not member
>>> of anything. Why is it important for a user to be a member of a group?
>>
>> Every POSIX user needs a default GID. We don't create user-private
>> groups for migrated users.
>>
> 

IPA to IPA migration is a bit of a special case, and not something we
really planned on (though we've tended to keep it basically working).

Migration was expected to be from an existing LDAP server providing
POSIX users and groups.

> How should default GID be set during migration? IMHO there are two issues:
> 
> 1. ipausers group is not a POSIX group. Which, btw, also creates this
> nice issue:
>   $ ipa user-add fbar --noprivate
>   First name: Foo
>   Last name: Bar
>   ipa: ERROR: Default group for new users is not POSIX

Right, we assumed that incoming user would already have valid groups.

> 2. migrated users have to be POSIX therefore they have gidnumber and
> migrate-ds checks for its presence. But the command doesn't do anything
> with the GID number later even if the group doesn't exist nor in a step
> where default group is set. Therefore, default group, even if POSIX,
> would not work for this use case(set default GID number).

It does verify that the GID points to an existing group. If not you'll
get a warning like:

GID number %s of migrated user %s does not point to a known group.

> Q: Is it expected that user private groups will be migrated? (e.g. for
> migration from other FreeIPA instance). If not, then there would be a
> lot of users without a private group with the same GID number as UID
> number.

IPA to IPA migration wasn't really planned out, so no.

It is slightly complex because it will add another remote LDAP call for
each user to see if they have an existing group in their name and ensure
that the group contains no members (or only this user). And then later
when groups are migrated skip over the existing private group silently.

> Q: Why don't we allow to create user private group? What would be better
> if migrating from FreeIPA instance: migrate private groups or create new
> private groups using Managed Entries plugin?

Because of the additional logic in evaluating what the current state of
groups is on the remote server. It's doable but it would be slower.

Worth an RFE I think.

rob

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCHES 0204-0207, 0211] Server upgrade: Make LDAP data upgrade deterministic

[Martin Basti]

On 12/03/15 16:21, Rob Crittenden wrote:

Martin Basti wrote:

The patchset ensure, the upgrade order will respect ordering of entries
in *.update files.

Required for: https://fedorahosted.org/freeipa/ticket/4904

Patch 205 also fixes https://fedorahosted.org/freeipa/ticket/3560

Required patch mbasti-0203

Patches attached.




Just reading the patches, untested.

I think ordered should default to True in the update() method of
ldapupdater to keep in spirit with the design.

Otherwise LGTM that it implements what was designed.

rob



New patch that switch default value for ordered to True attached.

--
Martin Basti

From 653d50779cc0e66ac942d5f72d986156fdeabc2d Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Fri, 13 Mar 2015 14:59:26 +0100
Subject: [PATCH] Server Upgrade: order update files by default

https://fedorahosted.org/freeipa/ticket/4904
---
 ipaserver/install/dsinstance.py   | 2 +-
 ipaserver/install/ipa_ldap_updater.py | 2 +-
 ipaserver/install/ldapupdate.py   | 2 +-
 ipaserver/install/upgradeinstance.py  | 3 +--
 4 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/ipaserver/install/dsinstance.py b/ipaserver/install/dsinstance.py
index 6bf31da99f36aaa76bc5cd6b2c80f92f51f86698..52382873527502b28943f32b2e14990dee69f424 100644
--- a/ipaserver/install/dsinstance.py
+++ b/ipaserver/install/dsinstance.py
@@ -509,7 +509,7 @@ class DsInstance(service.Service):
 def apply_updates(self):
 ld = ldapupdate.LDAPUpdate(dm_password=self.dm_password, sub_dict=self.sub_dict, plugins=True)
 files = ld.get_all_files(ldapupdate.UPDATES_DIR)
-ld.update(files, ordered=True)
+ld.update(files)
 
 def __add_referint_module(self):
 self._ldap_mod("referint-conf.ldif")
diff --git a/ipaserver/install/ipa_ldap_updater.py b/ipaserver/install/ipa_ldap_updater.py
index 4ad7b972795e3aaacd1f5b43a2ad1e26b34a1367..5df7cdf4275b75cc63045e29b92902698dc011a9 100644
--- a/ipaserver/install/ipa_ldap_updater.py
+++ b/ipaserver/install/ipa_ldap_updater.py
@@ -208,7 +208,7 @@ class LDAPUpdater_NonUpgrade(LDAPUpdater):
 if not self.files:
 self.files = ld.get_all_files(UPDATES_DIR)
 
-modified = ld.update(self.files, ordered=True) or modified
+modified = ld.update(self.files) or modified
 
 if modified and options.test:
 self.log.info('Update complete, changes to be made, test mode')
diff --git a/ipaserver/install/ldapupdate.py b/ipaserver/install/ldapupdate.py
index 3e4fc3f7a1de52f7019e674bb04000a082e53ea7..3e59a91153e01223a114365568c4a97b7f66efba 100644
--- a/ipaserver/install/ldapupdate.py
+++ b/ipaserver/install/ldapupdate.py
@@ -745,7 +745,7 @@ class LDAPUpdate:
 for update in all_updates:
 self._delete_record(update)
 
-def update(self, files, ordered=False):
+def update(self, files, ordered=True):
 """Execute the update. files is a list of the update files to use.
 :param ordered: Update files are executed in alphabetical order
 
diff --git a/ipaserver/install/upgradeinstance.py b/ipaserver/install/upgradeinstance.py
index 95306fc3c1c13d9b37438d5caf75da43bde06361..018db87a33c5f29aa65836de3f649141fc163acd 100644
--- a/ipaserver/install/upgradeinstance.py
+++ b/ipaserver/install/upgradeinstance.py
@@ -131,8 +131,7 @@ class IPAUpgrade(service.Service):
 ld = ldapupdate.LDAPUpdate(dm_password='', ldapi=True, live_run=self.live_run, plugins=True)
 if len(self.files) == 0:
 self.files = ld.get_all_files(ldapupdate.UPDATES_DIR)
-self.modified = (ld.update(self.files, ordered=True) or
- self.modified)
+self.modified = (ld.update(self.files) or self.modified)
 except ldapupdate.BadSyntax, e:
 root_logger.error('Bad syntax in upgrade %s' % str(e))
 self.modified = False
-- 
2.1.0

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCH 140] extdom: migrate check-based test to cmocka

[Jakub Hrozek]

On Fri, Mar 13, 2015 at 11:56:46AM +0100, Sumit Bose wrote:
> On Wed, Mar 04, 2015 at 06:42:05PM +0100, Sumit Bose wrote:
> > Hi,
> > 
> > this is the first patch for https://fedorahosted.org/freeipa/ticket/4922
> > which converts the check-based tests of the extdom plugin to cmocka.
> > 
> > bye,
> > Sumit
> 
> Rebased version attached.
> 
> bye,
> Sumit

The test itself is fine, but did freeipa consider moving to cmocka-1.0+
to avoid warnings like:
ipa_extdom_cmocka_tests.c: In function ‘main’:
ipa_extdom_cmocka_tests.c:408:5: warning: ‘_run_tests’ is deprecated
(declared at /usr/include/cmocka.h:2001) [-Wdeprecated-declarations]
 return run_tests(tests);

But I'm fine with ACKing this patch, the conversion should be done
separately.

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCHES 137-139] extdom: add err_msg member to request context

[Jakub Hrozek]

On Fri, Mar 13, 2015 at 11:55:09AM +0100, Sumit Bose wrote:
> On Wed, Mar 04, 2015 at 06:35:22PM +0100, Sumit Bose wrote:
> > Hi,
> > 
> > this patch series improves error reporting of the extdom plugin
> > especially on the client side. Currently there is only SSSD ticket
> > https://fedorahosted.org/sssd/ticket/2463 . Shall I create a
> > corresponding FreeIPA ticket as well?
> > 
> > In the third patch I already added a handful of new error messages.
> > Suggestions for more messages are welcome.
> > 
> > bye,
> > Sumit
> 
> Rebased versions attached.
> 
> bye,
> Sumit

The patches look good and work fine. I admit I cheated a bit and
modified the code to return a failure. Then I saw on the client:

[sssd[be[ipa.example.com]]] [ipa_s2n_exop_send] (0x0400): Executing extended 
operation
[sssd[be[ipa.example.com]]] [ipa_s2n_exop_done] (0x0040): 
ldap_extended_operation result: Operations
error(1), Failed to create fully qualified name.
[sssd[be[ipa.example.com]]] [ipa_s2n_exop_done] (0x0040): 
ldap_extended_operation failed, server logs might contain more details.
[sssd[be[ipa.example.com]]] [ipa_s2n_get_user_done] (0x0040): s2n exop request 
failed.

I just saw one typo:

> @@ -918,6 +934,7 @@ static int handle_sid_request(struct ipa_extdom_ctx *ctx,
>  ret = sss_nss_getorigbyname(pwd.pw_name, &kv_list, &id_type);
>  if (ret != 0 || !(id_type == SSS_ID_TYPE_UID
>  || id_type == SSS_ID_TYPE_BOTH)) {
> +set_err_msg(req, "Failed ot read original data");
 ~
>  if (ret == ENOENT) {
>  ret = LDAP_NO_SUCH_OBJECT;
>  } else {

And a compilation warning caused by previous patches.

So ACK provided the typo is fixed prior to pushing the patch.

-- 
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Re: [Freeipa-devel] [PATCHES 0015-0017] consolidation of various Kerberos auth methods in FreeIPA code

[Martin Babinsky]

Attaching the next iteration of patches.

I have tried my best to reword the ipa-client-install man page bit about 
the new option. Any suggestions to further improve it are welcome.


I have also slightly modified the 'kinit_keytab' function so that in 
Kerberos errors are reported for each attempt and the text of the last 
error is retained when finally raising exception.


--
Martin^3 Babinsky
From b6e0b91e27f041eedca4995e2ee09311d48e7168 Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Fri, 13 Mar 2015 16:38:02 +0100
Subject: [PATCH 1/3] ipautil: new functions kinit_keytab and kinit_password

kinit_keytab replaces kinit_hostprincipal and performs Kerberos auth using
keytab file. Function is also able to repeat authentication multiple times
before giving up and raising StandardError.

kinit_password wraps kinit auth using password and also supports FAST
authentication using httpd armor ccache.
---
 ipapython/ipautil.py | 65 +---
 1 file changed, 51 insertions(+), 14 deletions(-)

diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
index 4116d974e620341119b56fad3cff1bda48af3bab..7f2ca6f341907642e7acab87c89d7d46214a5646 100644
--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -1175,27 +1175,64 @@ def wait_for_open_socket(socket_name, timeout=0):
 else:
 raise e
 
-def kinit_hostprincipal(keytab, ccachedir, principal):
+
+def kinit_keytab(keytab, ccache_path, principal, attempts=1):
 """
-Given a ccache directory and a principal kinit as that user.
+Given a ccache_path , keytab file and a principal kinit as that user.
+
+The optional parameter 'attempts' specifies how many times the credential
+initialization should be attempted before giving up and raising
+StandardError.
 
 This blindly overwrites the current CCNAME so if you need to save
 it do so before calling this function.
 
+This function is also not thread-safe since it modifies environment
+variables.
+
 Thus far this is used to kinit as the local host.
 """
-try:
-ccache_file = 'FILE:%s/ccache' % ccachedir
-krbcontext = krbV.default_context()
-ktab = krbV.Keytab(name=keytab, context=krbcontext)
-princ = krbV.Principal(name=principal, context=krbcontext)
-os.environ['KRB5CCNAME'] = ccache_file
-ccache = krbV.CCache(name=ccache_file, context=krbcontext, primary_principal=princ)
-ccache.init(princ)
-ccache.init_creds_keytab(keytab=ktab, principal=princ)
-return ccache_file
-except krbV.Krb5Error, e:
-raise StandardError('Error initializing principal %s in %s: %s' % (principal, keytab, str(e)))
+root_logger.debug("Initializing principal %s using keytab %s"
+  % (principal, keytab))
+last_exc = ""
+for attempt in range(1, attempts + 1):
+try:
+krbcontext = krbV.default_context()
+ktab = krbV.Keytab(name=keytab, context=krbcontext)
+princ = krbV.Principal(name=principal, context=krbcontext)
+os.environ['KRB5CCNAME'] = ccache_path
+ccache = krbV.CCache(name=ccache_path, context=krbcontext,
+ primary_principal=princ)
+ccache.init(princ)
+ccache.init_creds_keytab(keytab=ktab, principal=princ)
+root_logger.debug("Attempt %d/%d: success"
+  % (attempt, attempts))
+return
+except krbV.Krb5Error as e:
+last_exc = str(e)
+root_logger.debug("Attempt %d/%d: failed: %s"
+  % (attempt, attempts, last_exc))
+time.sleep(1)
+
+root_logger.debug("Maximum number of attempts (%d) reached"
+  % attempts)
+raise StandardError("Error initializing principal %s: %s"
+% (principal, last_exc))
+
+
+def kinit_password(principal, password, env={}, armor_ccache=""):
+"""perform interactive kinit as principal using password. Additional
+enviroment variables can be specified using env. If using FAST for
+web-based authentication, use armor_ccache to specify http service ccache.
+"""
+root_logger.debug("Initializing principal %s using password" % principal)
+args = [paths.KINIT, principal]
+if armor_ccache:
+root_logger.debug("Using armor ccache %s for FAST webauth"
+  % armor_ccache)
+args.extend(['-T', armor_ccache])
+run(args, env=env, stdin=password)
+
 
 def dn_attribute_property(private_name):
 '''
-- 
2.1.0

From 74fd74fdeeb746e4d6bebdc459d4401a63279b78 Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Fri, 13 Mar 2015 16:39:05 +0100
Subject: [PATCH 2/3] ipa-client-install: try to get host TGT several times
 before giving up

New option '--kinit-attempts' enables the host to make multiple attempts to
obtain TGT from KDC before giving up and abor