Re: [Freeipa-devel] [PATCH] 0748 Handle encoding for ipautil.run

[Jan Cholasta]

On 3.12.2015 15:42, Petr Viktorin wrote:

On 12/03/2015 10:55 AM, Jan Cholasta wrote:
[...]

If we don't care about output somewhere, we should not capture it
there
at all.


Then people need to remember to put "capture_output=True" everywhere
(except that also disables logging of the output, so we'd need an
additional option).


capture_output=True is the default and capture_output=False should be
set where output is not needed.


"capture_output=True" being the default would mean that it's still
possible to leave "capture_output=False" out, but ignore the output. It
would make the wrong usage easier to type than the right one, which is
something to avoid.


I'm not sure I understand what are you trying to say here.

My point is that capture_output=True is currently the default (see for
yourself) and it is indeed possible to leave capture_output=False but
ignore the output. I believe this is wrong and that you should always
have to explicitly request the output to be captured.


OK, looks like I will need to refactor ipautil.run even for Python 2,
then.


If this is just about changing the default and fixing all run()
invocations, I can provide the patch myself, if that would work better
for you.


I seem to have have problems coming up with a solution that would match
your expectations, so let me write the semantics as I understand you'd
like hem:

Would you be happy with the following signature?

def run(args, stdin=None, raiseonerr=True,
  nolog=(), env=None,
  capture_output=False, skip_output=False,
  cwd=None, runas=None, timeout=None, suplementary_groups=[],
  capture_stderr=False,
  encode_stdout=True,
  encode_stderr=True,
 ):

Output and error would be always logged, except if skip_output is set.
But they would be only returned if capture_output/capture_stderr were
set.


I would personally also rename capture_output to capture_stdout. If we
change the default, we have to update its every use, so we might as well
rename it. (Again, I can provide the patch.)


If we agree on the semantics, writing the patch isn't that big a deal.


Right.




If encode_* is False, bytes are returned, by default
locale.getpreferredencoding() is used. (If the caller wants any other
encoding, it can do that by itself.)


I thought bytes should be the default after all? See my comment in the
previous mail.


If the default is no capturing, I'd rather go with text by default.
With capturing by deafult, "run(args)" could raise encoding errors *if
the output is then ignored*. If it's "run(args, capture_stdout=True)", I
think it's fine to encode.


OK, makes sense.




stdin it encoded using locale.getpreferredencoding() if it's unicode.


If we do encode/decode in run(), I think there should be a way to
override the default encoding. My idea was to either accept/return only
bytes and let the caller handle encoding themselves, or to handle
encoding in run(), but be able to override the defaults.

Given we handle encoding in run(), I would imagine it like this:

 def run(args, stdin=None, raiseonerr=True, nolog=(), env=None,
 capture_stdout=False, skip_output=False, cwd=None,
 runas=None, timeout=None, suplementary_groups=[],
 capture_stderr=False, encode_stdout=False,
 encode_stderr=False, encoding=None):

i.e. nothing is captured by default, when stdout or stderr are captured
they are returned as bytes by default, when stdout or stderr are
returned as text they are decoded using the locale encoding by default,
or the encoding can be explicitly specified.

Do you think this is feasible?


Feasible, yes.
In the majority of cases where the output is needed, we want text
encoded with locale.getpreferredencoding(), so I'd rather make that the
default rather than bytes.

Or we could make "encode_stdout" imply "capture_stdout", so you wouldn't
have to always specify both. (It would be better named "encoded_stdout"
in that case.)


I think it should be OK to decode by default. (IMO it would be best to 
reverse the logic and name it "raw_stdout", with False default. Do we 
need "raw_stderr" at all? I don't think we do.)


So the signature will be this:

  def run(args, stdin=None, raiseonerr=True, nolog=(), env=None,
  capture_stdout=False, skip_output=False, cwd=None,
  runas=None, timeout=None, suplementary_groups=[],
  capture_stderr=False, raw_stdout=False, encoding=None):

Then, when you want to capture just error messages on stderr:

rc, _out, err = run(args,
capture_stderr=True)

When you want to capture text on both stdout and stderr:

rc, out, err = run(args,
   capture_stdout=True,
   capture_stderr=True)

When you want to capture bytes on stdout and text on stderr (think 
"certutil -L -r"):


rc, out, err = run(args,
   capture_stdout=True,
   

Re: [Freeipa-devel] [PATCH 556-557] Add option to disable setkeytab extended operations

[Alexander Bokovoy]

On Wed, 02 Dec 2015, Simo Sorce wrote:

 We also have to fix the permission to change keytab, so that it uses
 the new
 style (https://fedorahosted.org/freeipa/ticket/5487). I would
 actually make
 this ticket and the ipa-sam ticket a blocker for this patch set.

 Otherwise you are actually introducing a switch that breaks FreeIPA
 as some of
 it's core server functions still use the old method.
>>>
>>> The point of this patchset is to introduce the switch early so that
>>> current server will support the off switch when newer servers down the
>>> road are ready to flip it. The defaults are still to allow these
>>> operations for services/hosts.
>>
>> I still do not get the logic about an early switch. Now, if switch is
>> turned
>> on, FreeIPA server breaks on several places. I would really rather
>> expect the
>> switch to be introduced when the server itself supports it. Then,
>> admin would
>> enable it when the clients are sufficiently updated and can use the
>> new method.
>>
>> Why would admin want to enable the switch early if it breaks FreeIPA
>> some of
>> the FreeIPA servers? Permission can be upgraded in newer version and get
>> replicated, that's fine. But ipa-sam would be still broken on this old
>> FreeIPA
>> server.
> Old server is not a problem here: ipa-sam only talks to the
> localhost-based server as we always use ldapi protocol. So if server is
> running old-behavior FreeIPA, ipa-sam on the same server will work
> against it.
>
> What I don't want to have is a situation where setkeytab is disabled and
> a new server obeys it but ipa-sam on this new server is not updated and
> will expectedly fail. We are not that forced to do the change right now
> in 4.3, given that the default will still be to keep setkeytab, thus we
> can wait with this patch until ipa-sam is fixed and then push both
> patches into the closest release, be it 4.3.x (x>=0) or whatever is the
> next one.
>

+1, fixing ipa-sam would be then a release blocker for 4.3 which is not
desired.

Also what about adding support for "ipaProtectedoperation check" for
user principals?

I'm afraid that forbidding getting user principal might be regarded as a
regression which might cause that admins won't set DisableSetKeytab.


One thing at a time.
We have bugs open for all these things, but I see no reason not to add
an *optional* setting just because some things break when it is turned
on.

The problem is that current getkeytab extended operation has not enough
information to be a replacement for ipa-sam's use of setkeytab, as I've
found with your current patches. So adding an optional setting in the
hope that it will not be used in real life is a bit naive, but if people
activate it, whole trust setup will break. I still prefer to have the
patchset first be completed and then merged. There is no problem in
keeping it aside while functionality is being implemented, we can
trivially rebase.
--
/ 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] [PATCH 25] Improve error logging for Dogtag subsystem installation

[Christian Heimes]

On 2015-12-03 11:04, Jan Cholasta wrote:
> On 2.12.2015 13:44, Petr Spacek wrote:
>> On 2.12.2015 13:23, Jan Cholasta wrote:
>>> On 2.12.2015 12:54, Petr Spacek wrote:
 On 2.12.2015 12:51, Christian Heimes wrote:
> On 2015-12-02 08:37, Petr Spacek wrote:
>> On 1.12.2015 18:42, Christian Heimes wrote:
>>>   From 33be1f56a64e53d261a1058c4606a7e48c0aac52 Mon Sep 17
>>> 00:00:00 2001
>>> From: Christian Heimes 
>>> Date: Tue, 1 Dec 2015 15:49:53 +0100
>>> Subject: [PATCH 25] Improve error logging for Dogtag subsystem
>>> installation
>>>
>>> In the case of a failed installation or uninstallation of a Dogtag
>>> subsystem, the error output of pkispawn / pkidestroyed are now
>>> shown to
>>> the user. It makes it more obvious what went wrong and makes it
>>> easier
>>> to debug a problem.
>>>
>>> The error handler also attempts to get the full name of the
>>> installation
>>> / uninstallation log file from stdout. pkispawn and pkidestroy
>>> print the
>>> absolute name as 'Log file: /path/to/file.log'. The user no
>>> longer has
>>> to guess the right log file.
>>>
>>> Example:
>>> [1/8]: configuring KRA instance
>>> Failed to configure KRA instance: Command ''/usr/sbin/pkispawn' '-s'
>>> 'KRA' '-f' '/tmp/tmp1UpbwF'' returned non-zero exit status 1
>>> pkispawn: ERROR... PKI subsystem 'KRA' for instance
>>> 'pki-tomcat' already exists!
>>> See the installation logs and the following files/directories for
>>> more
>>> information:
>>> /var/log/pki/pki-tomcat
>>> /var/log/pki/pki-kra-spawn.20151201151735.log
>>> [error] RuntimeError: KRA configuration failed.
>>>
>>> The patch also changes a couple of modules that were using
>>> the CalledProcessError exception object from subprocess instead of
>>> ipautil.
>>
>> I'm wondering if ipautil.run() can log stdout and stderr on log
>> level ERROR
>> when return code is non-zero (and log on level DEBUG as usual when
>> return
>> code
>> is zero).
>>
>> IMHO it would be nicer, universal, and does not require any
>> changes in places
>> calling ipautil.run().
>
> I think it's a bit confusing to print out stdout and stderr, because
> both streams are captured separately. The output is missing its
> chronological order. subprocess can capture stdout and stderr in the
> same stream, but then we can't distinguish between output and error
> output...

 I do not think it is a problem if these two are clearly marked as such:
 standard output: %s (if non-empty)
 stanrard error output: %s (if non-empty)
>>>
>>> We do not want to log with level ERROR by default when rc != 0,
>>> because some
>>> commands generate a *lot* of output.
>>
>> I do not agree, but whatever. Somebody needs to review the original
>> Christian's patch.
> 
> We had a short discussion about this with Petr offline and we agreed
> that a reasonable compromise would be to log the last few lines of
> stderr with ERROR level when a command fails.
> 
> This would mean adding custom __str__() to CalledProcessError, so that
> the stderr tail is logged when the CalledProcessError is not handled,
> and also logging it from ipautil.run() if raiseonerr == False.

Yes, that sounds like a reasonable idea.

In the default case (raiseonerr == True) ipautil.run() returns a custom
CalledProcessError exception that prints the command and the last two or
three non-empty lines from stderr. Callers can either log the exception
directly or format the out as they see fit.

With raiseonerr == False and exit code != 0 the same information is
logged with log level ERROR. I can just create the exception object and
log its string representation without raising the exception.

Christian



signature.asc
Description: OpenPGP digital signature
-- 
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] one-direction segments in ipaca suffix do not merge

[Oleg Fayans]

Hi all,

Should not these two one-directional segments in ipaca suffix be merged 
automatically?


$ ipa topologysegment-find ipaca
--
2 segments matched
--
  Segment name: master.justfor.test-to-replica1.justfor.test
  Left node: master.justfor.test
  Right node: replica1.justfor.test
  Connectivity: left-right

  Segment name: replica1.justfor.test-to-master.justfor.test
  Left node: replica1.justfor.test
  Right node: master.justfor.test
  Connectivity: left-right

Number of entries returned 2



--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.

--
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 0388] tests: Add hostmask detection for sudo rules validating

[Lukas Slebodnik]

On (03/12/15 11:35), Tomas Babej wrote:
>
>
>On 12/02/2015 05:25 PM, Lukas Slebodnik wrote:
>> On (02/12/15 15:41), Tomas Babej wrote:
>>>
>>>
>>> On 12/02/2015 09:24 AM, Tomas Babej wrote:


 On 12/01/2015 06:27 PM, Tomas Babej wrote:
>
>
> On 11/30/2015 05:32 PM, Lukas Slebodnik wrote:
>> On (30/11/15 13:09), Tomas Babej wrote:
>>> Hi,
>>>
>>> IPA sudo tests worked under the assumption that the clients that
>>> are executing the sudo commands have their IPs assigned within
>>> 255.255.255.0 hostmask.
>>>
>>> Removes this (invalid) assumption and adds a dynamic detection of
>>> the hostmask of the IPA client.
>>>
>>> https://fedorahosted.org/freeipa/ticket/5501
>>
>> >From e6f1846f0d7d17303e5b30b1643651ba739b2b6c Mon Sep 17 00:00:00 2001
>>> From: Tomas Babej 
>>> Date: Mon, 30 Nov 2015 12:53:39 +0100
>>> Subject: [PATCH] tests: Add hostmask detection for sudo rules 
>>> validating on
>>> hostmask
>>>
>>> IPA sudo tests worked under the assumption that the clients that
>>> are executing the sudo commands have their IPs assigned within
>>> 255.255.255.0 hostmask.
>>>
>>> Removes this (invalid) assumption and adds a dynamic detection of
>>> the hostmask of the IPA client.
>>>
>
> Thanks, updated patch attached.
>
> Tomas
>

 Actually, a small improvement is necessary.

 Updated patch attached.

 Tomas



>>>
>>> Thanks to Lukas, we found another problem with the test.
>>>
>>> Updated patch attached.
>>>
>> Thank you for 4th revision of patch
>> but there is still one issue.
>> 
>> === FAILURES 
>> ===
>> _ TestSudo.test_sudo_rule_restricted_to_one_hostmask_negative 
>> __
>> 
>> self = > 0x7ff695f56890>
>> 
>> def test_sudo_rule_restricted_to_one_hostmask_negative(self):
>> result1 = self.list_sudo_commands("testuser1")
>>>   assert result1.returncode != 0
>> E   assert 0 != 0
>> E+  where 0 = > 0x7ff695f56bd0>.returncode
>> 
>> test_integration/test_sudo.py:323: AssertionError
>>  1 failed, 74 passed in 807.35 seconds 
>> =
>> 
>> LS
>> 
>
>Here the test affected the negative test setup, which is fixed in the
>latest revision.
>
>Thanks,
>Tomas

>From 6242dfda205f6b2749627023384878448fd9e60c Mon Sep 17 00:00:00 2001
>From: Tomas Babej 
>Date: Wed, 2 Dec 2015 15:25:49 +0100
>Subject: [PATCH] tests: Add hostmask detection for sudo rules validating on
> hostmask
>
>IPA sudo tests worked under the assumption that the clients
>that are executing the sudo commands have their IPs assigned
>within 255.255.255.0 hostmask.
>
>Removes this (invalid) assumption and adds a
>dynamic detection of the hostmask of the IPA client.
>
>https://fedorahosted.org/freeipa/ticket/5501

The sudo test passed with this patch.
Thank you very much.

Functional ACK.


LS

-- 
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 0390] man: Update the ipa-replica-install manpage with promotion

[Tomas Babej]

Hi,

this patch updates the man page for the ipa-replica-install given the
latest changes (including the Jan's OTP patch).

Tomas
From 454e091bc29a536452094ecbf6fe72c100d46f30 Mon Sep 17 00:00:00 2001
From: Tomas Babej 
Date: Thu, 3 Dec 2015 11:42:03 +0100
Subject: [PATCH] man: Update the ipa-replica-install manpage with promotion
 related info

---
 install/tools/man/ipa-replica-install.1 | 45 -
 1 file changed, 38 insertions(+), 7 deletions(-)

diff --git a/install/tools/man/ipa-replica-install.1 b/install/tools/man/ipa-replica-install.1
index df01c616c89abeb5c1f421c6e29b1034b2ec9ea7..05753fac51af29024450033c025a733f8fe6a59f 100644
--- a/install/tools/man/ipa-replica-install.1
+++ b/install/tools/man/ipa-replica-install.1
@@ -20,13 +20,24 @@
 .SH "NAME"
 ipa\-replica\-install \- Create an IPA replica
 .SH "SYNOPSIS"
+.SS "DOMAIN LEVEL 0"
+.TP
 ipa\-replica\-install [\fIOPTION\fR]... replica_file
+.SS "DOMAIN LEVEL 1"
+.TP
+ipa\-replica\-install [\fIOPTION\fR]...
 .SH "DESCRIPTION"
-Configures a new IPA server that is a replica of the server that generated it. Once it has been created it is an exact copy of the original IPA server and is an equal master. Changes made to any master are automatically replicated to other masters.
+Configures a new IPA server that is a replica of the server. Once it has been created it is an exact copy of the original IPA server and is an equal master. Changes made to any master are automatically replicated to other masters.
 
-The replica_file is created using the ipa\-replica\-prepare utility.
+To create a replica in a domain at domain level 0, you need to provide an replica file. The replica_file is created using the ipa\-replica\-prepare utility.
 
-If the installation fails you may need to run ipa\-server\-install \-\-uninstall before running ipa\-replica\-install again.
+To create a replica in a domain at domain level 1, you cannot provide an replica file, the machine only needs to be enrolled in the FreeIPA domain first. This process of turning the IPA client into a replica is also referred to as replica promotion.
+
+If you're starting with an existing IPA client, simply run ipa\-replica\-install to have it promoted into a replica.
+
+To promote a blank machine into a replica, you have two options, you can either run ipa\-client\-install in a separate step, or pass the enrollment related options to the ipa\-replica\-install (see ENROLLMENT OPTIONS). In the latter case, ipa\-replica\-install will join the machine to the IPA realm automatically and will proceed with the promotion step.
+
+If the installation fails you may need to run ipa\-server\-install \-\-uninstall and ipa\-client\-install before running ipa\-replica\-install again.
 
 The installation will fail if the host you are installing the replica on exists as a host in IPA or an existing replication agreement exists (for example, from a previously failed installation).
 
@@ -42,11 +53,11 @@ certificate operations will be forwarded to a master with a CA installed.
 The IP address of this server. If this address does not match the address the host resolves to and \-\-setup\-dns is not selected the installation will fail. If the server hostname is not resolvable, a record for the hostname and IP_ADDRESS is added to /etc/hosts.
 This this option can be used multiple times to specify more IP addresses of the server (e.g. multihomed and/or dualstacked server).
 .TP
-\fB\-p\fR \fIDM_PASSWORD\fR, \fB\-\-password\fR=\fIDM_PASSWORD\fR
-Directory Manager (existing master) password
+\fB\-p\fR \fIPASSWORD\fR, \fB\-\-password\fR=\fIPASSWORD\fR
+For domain level 0, this enter the existing Directory Manager password. Under domain level 1, use this option to specify the password for joining a machine to the IPA realm. Assumes bulk password unless principal is also set.
 .TP
-\fB\-w\fR \fIADMIN_PASSWORD\fR, \fB\-\-admin\-password\fR=\fIADMIN_PASSWORD\fR
-Admin user Kerberos password used for connection check
+\fB\-w\fR, \fB\-\-admin\-password\fR
+The Kerberos password for the given principal.
 .TP
 \fB\-\-mkhomedir\fR
 Create home directories for users on their first login
@@ -78,6 +89,26 @@ An unattended installation that will never prompt for user input
 \fB\-\-dirsrv\-config\-file\fR
 The path to LDIF file that will be used to modify configuration of dse.ldif during installation of the directory server instance
 
+.SS "ENROLLMENT OPTIONS"
+.TP
+\fB\-\-server\fR
+The fully qualified domain name of the IPA server to enroll to.
+.TP
+\fB\-d\fR, \fB\-\-realm\fR=\fIREALM_NAME\fR
+Set the IPA realm name to REALM_NAME. Under normal circumstances, this option is not needed as the realm name is retrieved from the IPA server.
+.TP
+\fB\-n\fR, \fB\-\-domain\fR=\fIDOMAIN\fR
+Set the domain name to DOMAIN. When no \-\-server option is specified, the installer will try to discover all available servers via DNS SRV record autodiscovery (see DNS Autodiscovery section in ipa-client-install 

Re: [Freeipa-devel] [PATCH] 939 topologysuffix: change iparepltopoconfroot API properties

[Jan Cholasta]

On 2.12.2015 16:57, Petr Vobornik wrote:

Change CLI option, label and type to reflect that it is a only a DN
of the suffix.


Works for me, ACK.

Pushed to master: 581f5432bff7df909c1d7d7b8a55c5c81282afc0

--
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 0390] man: Update the ipa-replica-install manpage with promotion

[Petr Vobornik]

On 12/03/2015 12:06 PM, Tomas Babej wrote:

Hi,

this patch updates the man page for the ipa-replica-install given the
latest changes (including the Jan's OTP patch).

Tomas




Questions/suggestions:

1. "you cannot provide an replica file"

It's true, but wouldn't it be better to say that "you don't need to". Or 
communicate that it is a good thing. Cannot sounds too negative to me.


2. Why options: --password, --admin-password are in 'BASIC OPTIONS' 
section and not also in new 'ENROLLMENT OPTIONS' section together with 
--principal, --keytab, etc..? Maybe 'ENROLLMENT OPTIONS' should be 
before "BASIC OPTIONS" and "BASIC OPTIONS" renamed.



General ideas, not related to this patch:
"""
   If the installation fails you may need to run ipa-server-install 
--uninstall and ipa-client-install before running ipa-replica-install again.


   The installation will fail if the host you are installing the 
replica on exists as a host in IPA or an existing replication agreement 
exists (for example, from a previously failed installation)

"""

I believe validators check this situation, right? So do we need to write 
it in the man page.

--
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] one-direction segments in ipaca suffix do not merge

[Ludwig Krispenz]

On 12/03/2015 01:50 PM, Oleg Fayans wrote:

Hi all,

Should not these two one-directional segments in ipaca suffix be 
merged automatically?
yes they should, and normally do. What is your scenario when you get 
this, can you reproduce ?


$ ipa topologysegment-find ipaca
--
2 segments matched
--
  Segment name: master.justfor.test-to-replica1.justfor.test
  Left node: master.justfor.test
  Right node: replica1.justfor.test
  Connectivity: left-right

  Segment name: replica1.justfor.test-to-master.justfor.test
  Left node: replica1.justfor.test
  Right node: master.justfor.test
  Connectivity: left-right

Number of entries returned 2





--
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 556-557] Add option to disable setkeytab extended operations

[Petr Vobornik]

On 12/02/2015 07:16 PM, Simo Sorce wrote:

On Tue, 2015-12-01 at 16:44 +0100, Petr Vobornik wrote:

On 12/01/2015 04:20 PM, Alexander Bokovoy wrote:

On Tue, 01 Dec 2015, Martin Kosek wrote:

On 12/01/2015 02:59 PM, Simo Sorce wrote:

On Tue, 2015-12-01 at 14:42 +0100, Martin Kosek wrote:

On 12/01/2015 02:38 PM, Simo Sorce wrote:

On Tue, 2015-12-01 at 10:11 +0200, Alexander Bokovoy wrote:

On Mon, 30 Nov 2015, Simo Sorce wrote:

On Wed, 2015-11-25 at 09:47 -0500, Simo Sorce wrote:

On Wed, 2015-11-25 at 09:02 -0500, Rob Crittenden wrote:

Jan Cholasta wrote:

On 24.11.2015 22:17, Simo Sorce wrote:

On Tue, 2015-11-24 at 14:57 -0500, Simo Sorce wrote:

On Tue, 2015-11-24 at 14:42 -0500, Simo Sorce wrote:

Since some time we use the getkeytab operation to fetch
keytabs on
newer
clients. According to bug #232 setkeytab can be used to
circumvent
password quality controls so it needs to be slowly retired.

The attached patches implement #5485 in 2 parts.

The first introduces the option DisableSetKeytab which
globally
disables
the setkeytab extended operation. This is set to false by
default for
backwards compatibility.

The second introduces an option called
DisableUserSetKeytab, which is
active by default in new installs (but not in upgraded
ones), and only
disables the use of setkeytab for ipa suers, but not for
hosts/services.
This is because user's are the ones that may abuse the
interface to
escape password policies and users also normally do not
acquire
keytabs,
so it is a safe bet to disable just them by default in new
installs.

(Testing in progress)


Tested and working as expected.


I realized that adding options to ipaConfig require to add
them in the
UI as well, attached patches add options in API.txt and
config.py
Make now complain I should change API Major or Minor, but it
is not
clear to me why given this are additional values and no real
change or
new function is introduced. What's the recommendation ?


When does make complain? It is supposed to complain only when
API.txt
does not match code.

Anyway, we usually bump minor version even for backward
compatible
changes, see e.g. commit 9549a59.



The point of API.txt (and the heavy client) was to save a
round-trip.
Being able to pass in an invalid option would void that rule hence
having to update the API when new values are added.

rob


Ok added version change to the second patch (so we bump it only
once
given these are basically related changes.


Bump, is this ok ?

This patch is fine but please fix setkeytab use in ipa-sam before
committing this patch.


This patch does not disable setkeytab yet, so it can go in right away
(it blocks other patches already). We have a bug to change ipa-sam,
should we mark it blocker for 4.4 ?


We also have to fix the permission to change keytab, so that it uses
the new
style (https://fedorahosted.org/freeipa/ticket/5487). I would
actually make
this ticket and the ipa-sam ticket a blocker for this patch set.

Otherwise you are actually introducing a switch that breaks FreeIPA
as some of
it's core server functions still use the old method.


The point of this patchset is to introduce the switch early so that
current server will support the off switch when newer servers down the
road are ready to flip it. The defaults are still to allow these
operations for services/hosts.


I still do not get the logic about an early switch. Now, if switch is
turned
on, FreeIPA server breaks on several places. I would really rather
expect the
switch to be introduced when the server itself supports it. Then,
admin would
enable it when the clients are sufficiently updated and can use the
new method.

Why would admin want to enable the switch early if it breaks FreeIPA
some of
the FreeIPA servers? Permission can be upgraded in newer version and get
replicated, that's fine. But ipa-sam would be still broken on this old
FreeIPA
server.

Old server is not a problem here: ipa-sam only talks to the
localhost-based server as we always use ldapi protocol. So if server is
running old-behavior FreeIPA, ipa-sam on the same server will work
against it.

What I don't want to have is a situation where setkeytab is disabled and
a new server obeys it but ipa-sam on this new server is not updated and
will expectedly fail. We are not that forced to do the change right now
in 4.3, given that the default will still be to keep setkeytab, thus we
can wait with this patch until ipa-sam is fixed and then push both
patches into the closest release, be it 4.3.x (x>=0) or whatever is the
next one.



+1, fixing ipa-sam would be then a release blocker for 4.3 which is not
desired.

Also what about adding support for "ipaProtectedoperation check" for
user principals?

I'm afraid that forbidding getting user principal might be regarded as a
regression which might cause that admins won't set DisableSetKeytab.


One thing at a time.
We have bugs open for all these things, but I see no reason not to add
an *optional* setting just because some things 

Re: [Freeipa-devel] [PATCH 0388] tests: Add hostmask detection for sudo rules validating

[Tomas Babej]

On 12/02/2015 05:25 PM, Lukas Slebodnik wrote:
> On (02/12/15 15:41), Tomas Babej wrote:
>>
>>
>> On 12/02/2015 09:24 AM, Tomas Babej wrote:
>>>
>>>
>>> On 12/01/2015 06:27 PM, Tomas Babej wrote:


 On 11/30/2015 05:32 PM, Lukas Slebodnik wrote:
> On (30/11/15 13:09), Tomas Babej wrote:
>> Hi,
>>
>> IPA sudo tests worked under the assumption that the clients that
>> are executing the sudo commands have their IPs assigned within
>> 255.255.255.0 hostmask.
>>
>> Removes this (invalid) assumption and adds a dynamic detection of
>> the hostmask of the IPA client.
>>
>> https://fedorahosted.org/freeipa/ticket/5501
>
> >From e6f1846f0d7d17303e5b30b1643651ba739b2b6c Mon Sep 17 00:00:00 2001
>> From: Tomas Babej 
>> Date: Mon, 30 Nov 2015 12:53:39 +0100
>> Subject: [PATCH] tests: Add hostmask detection for sudo rules validating 
>> on
>> hostmask
>>
>> IPA sudo tests worked under the assumption that the clients that
>> are executing the sudo commands have their IPs assigned within
>> 255.255.255.0 hostmask.
>>
>> Removes this (invalid) assumption and adds a dynamic detection of
>> the hostmask of the IPA client.
>>

 Thanks, updated patch attached.

 Tomas

>>>
>>> Actually, a small improvement is necessary.
>>>
>>> Updated patch attached.
>>>
>>> Tomas
>>>
>>>
>>>
>>
>> Thanks to Lukas, we found another problem with the test.
>>
>> Updated patch attached.
>>
> Thank you for 4th revision of patch
> but there is still one issue.
> 
> === FAILURES 
> ===
> _ TestSudo.test_sudo_rule_restricted_to_one_hostmask_negative 
> __
> 
> self = 
> 
> def test_sudo_rule_restricted_to_one_hostmask_negative(self):
> result1 = self.list_sudo_commands("testuser1")
>>   assert result1.returncode != 0
> E   assert 0 != 0
> E+  where 0 =  0x7ff695f56bd0>.returncode
> 
> test_integration/test_sudo.py:323: AssertionError
>  1 failed, 74 passed in 807.35 seconds 
> =
> 
> LS
> 

Here the test affected the negative test setup, which is fixed in the
latest revision.

Thanks,
Tomas
From 6242dfda205f6b2749627023384878448fd9e60c Mon Sep 17 00:00:00 2001
From: Tomas Babej 
Date: Wed, 2 Dec 2015 15:25:49 +0100
Subject: [PATCH] tests: Add hostmask detection for sudo rules validating on
 hostmask

IPA sudo tests worked under the assumption that the clients
that are executing the sudo commands have their IPs assigned
within 255.255.255.0 hostmask.

Removes this (invalid) assumption and adds a
dynamic detection of the hostmask of the IPA client.

https://fedorahosted.org/freeipa/ticket/5501
---
 ipatests/test_integration/test_sudo.py | 46 +-
 1 file changed, 40 insertions(+), 6 deletions(-)

diff --git a/ipatests/test_integration/test_sudo.py b/ipatests/test_integration/test_sudo.py
index 1dd4c5d73c9fa4288af4fc2708aa3abd51407217..a1b7aa6cfc257e0eeb7def3c0e5b3c7fa98ee250 100644
--- a/ipatests/test_integration/test_sudo.py
+++ b/ipatests/test_integration/test_sudo.py
@@ -17,6 +17,9 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see .
 
+import pytest
+import re
+
 from ipatests.test_integration.base import IntegrationTest
 from ipatests.test_integration.tasks import clear_sssd_cache
 
@@ -104,6 +107,20 @@ class TestSudo(IntegrationTest):
 
 return result
 
+def get_client_ip_with_hostmask(self):
+"""
+Detects the IP of the client including the hostmask.
+"""
+
+ip = self.client.ip
+result = self.client.run_command(['ip', 'addr'])
+full_ip_regex = r'(?P%s/\d{1,2}) ' % re.escape(ip)
+match = re.search(full_ip_regex, result.stdout_text)
+
+if match:
+return match.group('full_ip')
+
+
 def test_nisdomainname(self):
 result = self.client.run_command('nisdomainname')
 assert self.client.domain.name in result.stdout_text
@@ -269,13 +286,25 @@ class TestSudo(IntegrationTest):
  '--hostgroups', 'testhostgroup'])
 
 def test_sudo_rule_restricted_to_one_hostmask_setup(self):
-# Add the client's /24 hostmask to the rule
-ip = self.client.ip
+# We need to detect the hostmask first
+full_ip = self.get_client_ip_with_hostmask()
+
+# Make a note for the next test, which needs to be skipped
+# if hostmask detection failed
+self.__class__.skip_hostmask_based = False
+
+if not full_ip:
+self.__class__.skip_hostmask_based = True
+raise pytest.skip("Hostmask could not be detected")
+
 self.master.run_command(['ipa', '-n', 'sudorule-add-host',
  'testrule',
-  

Re: [Freeipa-devel] [PATCH] 494 Update Contributors.txt

[Petr Spacek]

On 2.12.2015 12:37, Martin Kosek wrote:
> On 12/02/2015 12:35 PM, Martin Basti wrote:
>>
>>
>> On 02.12.2015 12:24, Martin Kosek wrote:
>>> +Thierry Bordaz
>>> 
>>> +Thierry Bordaz
>>> 
>>
>> Do we want this there?
> 
> I do not want it there, but I had to add it since Author field in git was 
> wrong
> when the patch was pushed.
> 
> So I blame the original author and the reviewer for making me add this mail
> alias :-)

+1

@Martin^2: Again, DNS names are not secret. Really.

If they were we would have a bigger problem: Your e-mail headers contain DNS
names and IP addresses of internal mail servers and of course the machine you
used to send the e-mail.

Hello, dhcp129-150.brq.redhat.com aka 10.34.129.150! :-)

-- 
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 acceptance criteria

[Lukas Slebodnik]

On (02/12/15 13:14), Rob Crittenden wrote:
>Is it still mandatory that tests pass the unit tests before acceptance?
Unit test could be executed as part of "%check" phase in spec files.
I recently added C-base unit tests there.

I was not bale to run "make tests" there because many tests failed.
If they require real IPA server for execution ( or lite server)
then they are not unit test but integration tests.
One solution would be to skip them or to usw cwrap[1] + lite server.
So it can be run also in mock/koji which has many restrictions.

Also lint should be also part of "%check" phase and not part of
ordinary build.
BTW I could not see a lint[2] in fedora build at all. So I'm not sure
if it is executed with upstream spec file.

If someone wants to comply that it would take long time to build rpms
then you might take a look how long glibc is built.
http://koji.fedoraproject.org/koji/buildinfo?buildID=697271
(half an hour on x86_64)

>I've seen a number of cases over the past couple of months where a
>change goes through then shortly afterward a patch to fix the tests.
>IMHO this should be caught in advance.
>
>Things slip through and goodness knows I've acked more than a few
>patches without running the full suite. I just have a feeling it has
>become more frequent lately.
>
>rob

LS

[1] https://cwrap.org/
[2] 
https://kojipkgs.fedoraproject.org//packages/freeipa/4.2.3/1.1.fc23/data/logs/x86_64/build.log

-- 
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] 494 Update Contributors.txt

[Martin Basti]

On 03.12.2015 09:00, Petr Spacek wrote:

On 2.12.2015 12:37, Martin Kosek wrote:

On 12/02/2015 12:35 PM, Martin Basti wrote:


On 02.12.2015 12:24, Martin Kosek wrote:

+Thierry Bordaz
+Thierry Bordaz

Do we want this there?

I do not want it there, but I had to add it since Author field in git was wrong
when the patch was pushed.

So I blame the original author and the reviewer for making me add this mail
alias :-)

+1

@Martin^2: Again, DNS names are not secret. Really.

If they were we would have a bigger problem: Your e-mail headers contain DNS
names and IP addresses of internal mail servers and of course the machine you
used to send the e-mail.

Hello, dhcp129-150.brq.redhat.com aka 10.34.129.150! :-)


I know, I'm not afraid of security in this case, it is just mess.

--
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 acceptance criteria

[Petr Spacek]

On 2.12.2015 19:14, Rob Crittenden wrote:
> Is it still mandatory that tests pass the unit tests before acceptance?
> I've seen a number of cases over the past couple of months where a
> change goes through then shortly afterward a patch to fix the tests.
> IMHO this should be caught in advance.
> 
> Things slip through and goodness knows I've acked more than a few
> patches without running the full suite. I just have a feeling it has
> become more frequent lately.

When we are at it... An automated thingy which accepts URL to a Git repo, does
all the test magic, and spits out test results without user interaction would
be an awesome Christmas present!

Bonus points if we can get Github integration so I can just push and have it
tested automatically so I cannot forget to do that before sending the patch
for review.

-- 
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 25] Improve error logging for Dogtag subsystem installation

[Jan Cholasta]

On 2.12.2015 13:44, Petr Spacek wrote:

On 2.12.2015 13:23, Jan Cholasta wrote:

On 2.12.2015 12:54, Petr Spacek wrote:

On 2.12.2015 12:51, Christian Heimes wrote:

On 2015-12-02 08:37, Petr Spacek wrote:

On 1.12.2015 18:42, Christian Heimes wrote:

  From 33be1f56a64e53d261a1058c4606a7e48c0aac52 Mon Sep 17 00:00:00 2001
From: Christian Heimes 
Date: Tue, 1 Dec 2015 15:49:53 +0100
Subject: [PATCH 25] Improve error logging for Dogtag subsystem installation

In the case of a failed installation or uninstallation of a Dogtag
subsystem, the error output of pkispawn / pkidestroyed are now shown to
the user. It makes it more obvious what went wrong and makes it easier
to debug a problem.

The error handler also attempts to get the full name of the installation
/ uninstallation log file from stdout. pkispawn and pkidestroy print the
absolute name as 'Log file: /path/to/file.log'. The user no longer has
to guess the right log file.

Example:
[1/8]: configuring KRA instance
Failed to configure KRA instance: Command ''/usr/sbin/pkispawn' '-s'
'KRA' '-f' '/tmp/tmp1UpbwF'' returned non-zero exit status 1
pkispawn: ERROR... PKI subsystem 'KRA' for instance
'pki-tomcat' already exists!
See the installation logs and the following files/directories for more
information:
/var/log/pki/pki-tomcat
/var/log/pki/pki-kra-spawn.20151201151735.log
[error] RuntimeError: KRA configuration failed.

The patch also changes a couple of modules that were using
the CalledProcessError exception object from subprocess instead of
ipautil.


I'm wondering if ipautil.run() can log stdout and stderr on log level ERROR
when return code is non-zero (and log on level DEBUG as usual when return
code
is zero).

IMHO it would be nicer, universal, and does not require any changes in places
calling ipautil.run().


I think it's a bit confusing to print out stdout and stderr, because
both streams are captured separately. The output is missing its
chronological order. subprocess can capture stdout and stderr in the
same stream, but then we can't distinguish between output and error
output...


I do not think it is a problem if these two are clearly marked as such:
standard output: %s (if non-empty)
stanrard error output: %s (if non-empty)


We do not want to log with level ERROR by default when rc != 0, because some
commands generate a *lot* of output.


I do not agree, but whatever. Somebody needs to review the original
Christian's patch.


We had a short discussion about this with Petr offline and we agreed 
that a reasonable compromise would be to log the last few lines of 
stderr with ERROR level when a command fails.


This would mean adding custom __str__() to CalledProcessError, so that 
the stderr tail is logged when the CalledProcessError is not handled, 
and also logging it from ipautil.run() if raiseonerr == False.


--
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 556-557] Add option to disable setkeytab extended operations

[Martin Kosek]

On 12/02/2015 07:16 PM, Simo Sorce wrote:
> On Tue, 2015-12-01 at 16:44 +0100, Petr Vobornik wrote:
>> On 12/01/2015 04:20 PM, Alexander Bokovoy wrote:
>>> On Tue, 01 Dec 2015, Martin Kosek wrote:
 On 12/01/2015 02:59 PM, Simo Sorce wrote:
> On Tue, 2015-12-01 at 14:42 +0100, Martin Kosek wrote:
>> On 12/01/2015 02:38 PM, Simo Sorce wrote:
>>> On Tue, 2015-12-01 at 10:11 +0200, Alexander Bokovoy wrote:
 On Mon, 30 Nov 2015, Simo Sorce wrote:
> On Wed, 2015-11-25 at 09:47 -0500, Simo Sorce wrote:
>> On Wed, 2015-11-25 at 09:02 -0500, Rob Crittenden wrote:
>>> Jan Cholasta wrote:
 On 24.11.2015 22:17, Simo Sorce wrote:
> On Tue, 2015-11-24 at 14:57 -0500, Simo Sorce wrote:
>> On Tue, 2015-11-24 at 14:42 -0500, Simo Sorce wrote:
>>> Since some time we use the getkeytab operation to fetch
>>> keytabs on
>>> newer
>>> clients. According to bug #232 setkeytab can be used to
>>> circumvent
>>> password quality controls so it needs to be slowly retired.
>>>
>>> The attached patches implement #5485 in 2 parts.
>>>
>>> The first introduces the option DisableSetKeytab which
>>> globally
>>> disables
>>> the setkeytab extended operation. This is set to false by
>>> default for
>>> backwards compatibility.
>>>
>>> The second introduces an option called
>>> DisableUserSetKeytab, which is
>>> active by default in new installs (but not in upgraded
>>> ones), and only
>>> disables the use of setkeytab for ipa suers, but not for
>>> hosts/services.
>>> This is because user's are the ones that may abuse the
>>> interface to
>>> escape password policies and users also normally do not
>>> acquire
>>> keytabs,
>>> so it is a safe bet to disable just them by default in new
>>> installs.
>>>
>>> (Testing in progress)
>>
>> Tested and working as expected.
>
> I realized that adding options to ipaConfig require to add
> them in the
> UI as well, attached patches add options in API.txt and
> config.py
> Make now complain I should change API Major or Minor, but it
> is not
> clear to me why given this are additional values and no real
> change or
> new function is introduced. What's the recommendation ?

 When does make complain? It is supposed to complain only when
 API.txt
 does not match code.

 Anyway, we usually bump minor version even for backward
 compatible
 changes, see e.g. commit 9549a59.

>>>
>>> The point of API.txt (and the heavy client) was to save a
>>> round-trip.
>>> Being able to pass in an invalid option would void that rule hence
>>> having to update the API when new values are added.
>>>
>>> rob
>>
>> Ok added version change to the second patch (so we bump it only
>> once
>> given these are basically related changes.
>
> Bump, is this ok ?
 This patch is fine but please fix setkeytab use in ipa-sam before
 committing this patch.
>>>
>>> This patch does not disable setkeytab yet, so it can go in right away
>>> (it blocks other patches already). We have a bug to change ipa-sam,
>>> should we mark it blocker for 4.4 ?
>>
>> We also have to fix the permission to change keytab, so that it uses
>> the new
>> style (https://fedorahosted.org/freeipa/ticket/5487). I would
>> actually make
>> this ticket and the ipa-sam ticket a blocker for this patch set.
>>
>> Otherwise you are actually introducing a switch that breaks FreeIPA
>> as some of
>> it's core server functions still use the old method.
>
> The point of this patchset is to introduce the switch early so that
> current server will support the off switch when newer servers down the
> road are ready to flip it. The defaults are still to allow these
> operations for services/hosts.

 I still do not get the logic about an early switch. Now, if switch is
 turned
 on, FreeIPA server breaks on several places. I would really rather
 expect the
 switch to be introduced when the server itself supports it. Then,
 admin would
 enable it when the clients are sufficiently updated and can use the
 new method.

 Why would admin want to enable the switch early if it breaks FreeIPA
 some of
 the FreeIPA servers? Permission can be upgraded in 

Re: [Freeipa-devel] patch acceptance criteria

[Martin Kosek]

On 12/03/2015 09:08 AM, Petr Spacek wrote:
> On 2.12.2015 19:14, Rob Crittenden wrote:
>> Is it still mandatory that tests pass the unit tests before acceptance?
>> I've seen a number of cases over the past couple of months where a
>> change goes through then shortly afterward a patch to fix the tests.
>> IMHO this should be caught in advance.
>>
>> Things slip through and goodness knows I've acked more than a few
>> patches without running the full suite. I just have a feeling it has
>> become more frequent lately.
> 
> When we are at it... An automated thingy which accepts URL to a Git repo, does
> all the test magic, and spits out test results without user interaction would
> be an awesome Christmas present!
> 
> Bonus points if we can get Github integration so I can just push and have it
> tested automatically so I cannot forget to do that before sending the patch
> for review.

+1. Having basic CI test suite run on top of a Pull Request would be awesome.

-- 
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 509-514] replica promotion: use host credentials when setting up replication

[Martin Basti]

On 01.12.2015 12:19, Jan Cholasta wrote:

On 23.11.2015 15:47, Simo Sorce wrote:

On Mon, 2015-11-23 at 15:37 +0100, Jan Cholasta wrote:


Ad alternative is to add the host to ipaservers before the checks are
done and remove it again if any of them fail.


Too error prone, I am ok with the current way in your patches
until/unless I can think of a fail safe way. :-)


Updated patches attached. Note that 520 should be applied between 509 
and 510.





Functional ACK
-- 
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] 0748 Handle encoding for ipautil.run

[Petr Viktorin]

On 12/03/2015 10:55 AM, Jan Cholasta wrote:
[...]
>>> If we don't care about output somewhere, we should not capture it
>>> there
>>> at all.
>>
>> Then people need to remember to put "capture_output=True" everywhere
>> (except that also disables logging of the output, so we'd need an
>> additional option).
>
> capture_output=True is the default and capture_output=False should be
> set where output is not needed.

 "capture_output=True" being the default would mean that it's still
 possible to leave "capture_output=False" out, but ignore the output. It
 would make the wrong usage easier to type than the right one, which is
 something to avoid.
>>>
>>> I'm not sure I understand what are you trying to say here.
>>>
>>> My point is that capture_output=True is currently the default (see for
>>> yourself) and it is indeed possible to leave capture_output=False but
>>> ignore the output. I believe this is wrong and that you should always
>>> have to explicitly request the output to be captured.
>>
>> OK, looks like I will need to refactor ipautil.run even for Python 2,
>> then.
> 
> If this is just about changing the default and fixing all run()
> invocations, I can provide the patch myself, if that would work better
> for you.
> 
>> I seem to have have problems coming up with a solution that would match
>> your expectations, so let me write the semantics as I understand you'd
>> like hem:
>>
>> Would you be happy with the following signature?
>>
>> def run(args, stdin=None, raiseonerr=True,
>>  nolog=(), env=None,
>>  capture_output=False, skip_output=False,
>>  cwd=None, runas=None, timeout=None, suplementary_groups=[],
>>  capture_stderr=False,
>>  encode_stdout=True,
>>  encode_stderr=True,
>> ):
>>
>> Output and error would be always logged, except if skip_output is set.
>> But they would be only returned if capture_output/capture_stderr were
>> set.
> 
> I would personally also rename capture_output to capture_stdout. If we
> change the default, we have to update its every use, so we might as well
> rename it. (Again, I can provide the patch.)

If we agree on the semantics, writing the patch isn't that big a deal.

>> If encode_* is False, bytes are returned, by default
>> locale.getpreferredencoding() is used. (If the caller wants any other
>> encoding, it can do that by itself.)
> 
> I thought bytes should be the default after all? See my comment in the
> previous mail.

If the default is no capturing, I'd rather go with text by default.
With capturing by deafult, "run(args)" could raise encoding errors *if
the output is then ignored*. If it's "run(args, capture_stdout=True)", I
think it's fine to encode.

>> stdin it encoded using locale.getpreferredencoding() if it's unicode.
> 
> If we do encode/decode in run(), I think there should be a way to
> override the default encoding. My idea was to either accept/return only
> bytes and let the caller handle encoding themselves, or to handle
> encoding in run(), but be able to override the defaults.
> 
> Given we handle encoding in run(), I would imagine it like this:
> 
> def run(args, stdin=None, raiseonerr=True, nolog=(), env=None,
> capture_stdout=False, skip_output=False, cwd=None,
> runas=None, timeout=None, suplementary_groups=[],
> capture_stderr=False, encode_stdout=False,
> encode_stderr=False, encoding=None):
> 
> i.e. nothing is captured by default, when stdout or stderr are captured
> they are returned as bytes by default, when stdout or stderr are
> returned as text they are decoded using the locale encoding by default,
> or the encoding can be explicitly specified.
> 
> Do you think this is feasible?

Feasible, yes.
In the majority of cases where the output is needed, we want text
encoded with locale.getpreferredencoding(), so I'd rather make that the
default rather than bytes.

Or we could make "encode_stdout" imply "capture_stdout", so you wouldn't
have to always specify both. (It would be better named "encoded_stdout"
in that case.)

-- 
Petr Viktorin

-- 
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] one-direction segments in ipaca suffix do not merge

[Ludwig Krispenz]

On 12/03/2015 02:31 PM, Petr Vobornik wrote:

On 12/03/2015 02:00 PM, Ludwig Krispenz wrote:


On 12/03/2015 01:50 PM, Oleg Fayans wrote:

Hi all,

Should not these two one-directional segments in ipaca suffix be
merged automatically?

yes they should, and normally do. What is your scenario when you get
this, can you reproduce ?


I encountered it yesterday bu:
* on A. `ipa-server-install --setup-dns`
* on: B `ipa-replica-install --setup-ca`
* on: C `ipa-replica-install --setup-ca`

It created: A <-> B <-> C

Agreements were not joined. Don't remember if in both suffixes.

Btw,
After I created A <-> C and removed B using `ipa-replica-manage del 
B`. I've encountered DS on A on 100%CPU for quite some time ~ 30s or 
more.

please take a pstack next time it occurs


I can provide logs/access for all 3 machines.

yes, I will have a look




$ ipa topologysegment-find ipaca
--
2 segments matched
--
  Segment name: master.justfor.test-to-replica1.justfor.test
  Left node: master.justfor.test
  Right node: replica1.justfor.test
  Connectivity: left-right

  Segment name: replica1.justfor.test-to-master.justfor.test
  Left node: replica1.justfor.test
  Right node: master.justfor.test
  Connectivity: left-right

Number of entries returned 2










--
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 0388] tests: Add hostmask detection for sudo rules validating

[Oleg Fayans]

Hi Tomas,

I would prefer get_client_ip_with_hostmask function to go to 
ipatests/test_integration/tasks.py, and to be more generic: we probably 
will need to run it against an arbitrary host.

The rest looks fine

On 12/03/2015 11:35 AM, Tomas Babej wrote:



On 12/02/2015 05:25 PM, Lukas Slebodnik wrote:

On (02/12/15 15:41), Tomas Babej wrote:



On 12/02/2015 09:24 AM, Tomas Babej wrote:



On 12/01/2015 06:27 PM, Tomas Babej wrote:



On 11/30/2015 05:32 PM, Lukas Slebodnik wrote:

On (30/11/15 13:09), Tomas Babej wrote:

Hi,

IPA sudo tests worked under the assumption that the clients that
are executing the sudo commands have their IPs assigned within
255.255.255.0 hostmask.

Removes this (invalid) assumption and adds a dynamic detection of
the hostmask of the IPA client.

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


>From e6f1846f0d7d17303e5b30b1643651ba739b2b6c Mon Sep 17 00:00:00 2001

From: Tomas Babej 
Date: Mon, 30 Nov 2015 12:53:39 +0100
Subject: [PATCH] tests: Add hostmask detection for sudo rules validating on
hostmask

IPA sudo tests worked under the assumption that the clients that
are executing the sudo commands have their IPs assigned within
255.255.255.0 hostmask.

Removes this (invalid) assumption and adds a dynamic detection of
the hostmask of the IPA client.



Thanks, updated patch attached.

Tomas



Actually, a small improvement is necessary.

Updated patch attached.

Tomas





Thanks to Lukas, we found another problem with the test.

Updated patch attached.


Thank you for 4th revision of patch
but there is still one issue.

=== FAILURES ===
_ TestSudo.test_sudo_rule_restricted_to_one_hostmask_negative __

self = 

 def test_sudo_rule_restricted_to_one_hostmask_negative(self):
 result1 = self.list_sudo_commands("testuser1")

   assert result1.returncode != 0

E   assert 0 != 0
E+  where 0 = .returncode

test_integration/test_sudo.py:323: AssertionError
 1 failed, 74 passed in 807.35 seconds =

LS



Here the test affected the negative test setup, which is fixed in the
latest revision.

Thanks,
Tomas





--
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.

--
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] one-direction segments in ipaca suffix do not merge

[Petr Vobornik]

On 12/03/2015 02:00 PM, Ludwig Krispenz wrote:


On 12/03/2015 01:50 PM, Oleg Fayans wrote:

Hi all,

Should not these two one-directional segments in ipaca suffix be
merged automatically?

yes they should, and normally do. What is your scenario when you get
this, can you reproduce ?


I encountered it yesterday bu:
* on A. `ipa-server-install --setup-dns`
* on: B `ipa-replica-install --setup-ca`
* on: C `ipa-replica-install --setup-ca`

It created: A <-> B <-> C

Agreements were not joined. Don't remember if in both suffixes.

Btw,
After I created A <-> C and removed B using `ipa-replica-manage del B`. 
I've encountered DS on A on 100%CPU for quite some time ~ 30s or more.


I can provide logs/access for all 3 machines.



$ ipa topologysegment-find ipaca
--
2 segments matched
--
  Segment name: master.justfor.test-to-replica1.justfor.test
  Left node: master.justfor.test
  Right node: replica1.justfor.test
  Connectivity: left-right

  Segment name: replica1.justfor.test-to-master.justfor.test
  Left node: replica1.justfor.test
  Right node: master.justfor.test
  Connectivity: left-right

Number of entries returned 2








--
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 acceptance criteria

[Rob Crittenden]

Martin Kosek wrote:
> On 12/03/2015 09:08 AM, Petr Spacek wrote:
>> On 2.12.2015 19:14, Rob Crittenden wrote:
>>> Is it still mandatory that tests pass the unit tests before acceptance?
>>> I've seen a number of cases over the past couple of months where a
>>> change goes through then shortly afterward a patch to fix the tests.
>>> IMHO this should be caught in advance.
>>>
>>> Things slip through and goodness knows I've acked more than a few
>>> patches without running the full suite. I just have a feeling it has
>>> become more frequent lately.
>>
>> When we are at it... An automated thingy which accepts URL to a Git repo, 
>> does
>> all the test magic, and spits out test results without user interaction would
>> be an awesome Christmas present!
>>
>> Bonus points if we can get Github integration so I can just push and have it
>> tested automatically so I cannot forget to do that before sending the patch
>> for review.
> 
> +1. Having basic CI test suite run on top of a Pull Request would be awesome.
> 

I'd be happy with just the ipatests being run manually with each review.

And it's then reviewer that I'm focusing on here. A developer _should_
also run the tests but part of the reviewer's responsibility is to
ensure the patch does what it says it does without breaking things.

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

[Freeipa-devel] [PATCH 562-563] Fix ipa-sam to use the getkeytab control instead of the setkeytab control

[Simo Sorce]

The first patch is preparatory and is needed in general now that we want
top allow alias and use krbCanonicalName as the canonical name when
multiple values are avilable in krbPrincipalName.

The second patch changes slightly how the interdomain trust account is
created so that the getkeytab control can generate the proper key (with
the right salt) for interop reasons with AD. The change should be
upgrade safe because keys are generate at account creation so older
accounts lacking the alias won't be a problem.

Fixes ##5495

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York
From e13bb47a9e3673bb7af627bfb2bc59476552947e Mon Sep 17 00:00:00 2001
From: Simo Sorce 
Date: Wed, 2 Dec 2015 15:20:42 -0500
Subject: [PATCH 1/2] Improve keytab code to select the right principal.

Whe requesting a keytab the salt used is the NORMAL type (for backwards and AD
compatibility), however since we added alias support we need to search for the
krbCanonicalName in preference, hen nothing is specified, and for the requested
principal name when a getkeytab operation is performed. This is so that the
correct salt can be applied. (Windows AD uses some peculiar aliases for some
special accounts to generate the salt).

Signed-off-by: Simo Sorce 
---
 daemons/ipa-slapi-plugins/ipa-pwd-extop/encoding.c | 23 +++---
 .../ipa-pwd-extop/ipa_pwd_extop.c  |  3 ++-
 daemons/ipa-slapi-plugins/ipa-pwd-extop/ipapwd.h   |  1 +
 daemons/ipa-slapi-plugins/ipa-pwd-extop/prepost.c  |  2 +-
 4 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/encoding.c b/daemons/ipa-slapi-plugins/ipa-pwd-extop/encoding.c
index 5ca155dcf49923b06bdd1ea56c5fba8fff5e410e..9c62f0560aa999b2179a7767040047dfa89288e0 100644
--- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/encoding.c
+++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/encoding.c
@@ -104,6 +104,7 @@ void ipapwd_keyset_free(struct ipapwd_keyset **pkset)
 
 Slapi_Value **ipapwd_encrypt_encode_key(struct ipapwd_krbcfg *krbcfg,
 struct ipapwd_data *data,
+char *preferred_principal,
 int num_encsalts,
 krb5_key_salt_tuple *encsalts,
 char **errMesg)
@@ -128,12 +129,20 @@ Slapi_Value **ipapwd_encrypt_encode_key(struct ipapwd_krbcfg *krbcfg,
 
 kvno = ipapwd_get_cur_kvno(data->target);
 
-krbPrincipalName = slapi_entry_attr_get_charptr(data->target,
-"krbPrincipalName");
-if (!krbPrincipalName) {
-*errMesg = "no krbPrincipalName present in this entry\n";
-LOG_FATAL("%s", *errMesg);
-goto enc_error;
+if (preferred_principal) {
+krbPrincipalName = slapi_ch_strdup(preferred_principal);
+} else {
+krbPrincipalName = slapi_entry_attr_get_charptr(data->target,
+"krbCanonicalName");
+if (!krbPrincipalName) {
+krbPrincipalName = slapi_entry_attr_get_charptr(data->target,
+"krbPrincipalName");
+}
+if (!krbPrincipalName) {
+*errMesg = "no krbPrincipalName present in this entry\n";
+LOG_FATAL("%s", *errMesg);
+goto enc_error;
+}
 }
 
 krberr = krb5_parse_name(krbctx, krbPrincipalName, );
@@ -215,7 +224,7 @@ int ipapwd_gen_hashes(struct ipapwd_krbcfg *krbcfg,
 
 if (is_krb) {
 
-*svals = ipapwd_encrypt_encode_key(krbcfg, data,
+*svals = ipapwd_encrypt_encode_key(krbcfg, data, NULL,
krbcfg->num_pref_encsalts,
krbcfg->pref_encsalts,
errMesg);
diff --git a/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c b/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c
index a910625cea711ca8c564f3c547a1d61788b071be..527238b1b710dfe63d90338ccf806c86c8d9cf6c 100644
--- a/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c
+++ b/daemons/ipa-slapi-plugins/ipa-pwd-extop/ipa_pwd_extop.c
@@ -661,6 +661,7 @@ static Slapi_Entry *get_entry_by_principal(const char *principal)
 Slapi_PBlock *pb = NULL;
 char *attrlist[] = { "krbPrincipalKey", "krbLastPwdChange",
  "userPassword", "krbPrincipalName",
+ "krbCanonicalName",
  "enrolledBy", "objectClass", NULL };
 Slapi_Entry **es = NULL;
 int res, ret, i;
@@ -1664,7 +1665,7 @@ static int ipapwd_getkeytab(Slapi_PBlock *pb, struct ipapwd_krbcfg *krbcfg)
 data.target = target_entry;
 data.password = password;
 
-svals = ipapwd_encrypt_encode_key(krbcfg, ,
+svals = ipapwd_encrypt_encode_key(krbcfg, , service_name,
 

Re: [Freeipa-devel] [PATCH 556-557] Add option to disable setkeytab extended operations

[Simo Sorce]

On Thu, 2015-12-03 at 12:21 +0200, Alexander Bokovoy wrote:
> On Wed, 02 Dec 2015, Simo Sorce wrote:
> >>  We also have to fix the permission to change keytab, so that it uses
> >>  the new
> >>  style (https://fedorahosted.org/freeipa/ticket/5487). I would
> >>  actually make
> >>  this ticket and the ipa-sam ticket a blocker for this patch set.
> >> 
> >>  Otherwise you are actually introducing a switch that breaks FreeIPA
> >>  as some of
> >>  it's core server functions still use the old method.
> >> >>>
> >> >>> The point of this patchset is to introduce the switch early so that
> >> >>> current server will support the off switch when newer servers down the
> >> >>> road are ready to flip it. The defaults are still to allow these
> >> >>> operations for services/hosts.
> >> >>
> >> >> I still do not get the logic about an early switch. Now, if switch is
> >> >> turned
> >> >> on, FreeIPA server breaks on several places. I would really rather
> >> >> expect the
> >> >> switch to be introduced when the server itself supports it. Then,
> >> >> admin would
> >> >> enable it when the clients are sufficiently updated and can use the
> >> >> new method.
> >> >>
> >> >> Why would admin want to enable the switch early if it breaks FreeIPA
> >> >> some of
> >> >> the FreeIPA servers? Permission can be upgraded in newer version and get
> >> >> replicated, that's fine. But ipa-sam would be still broken on this old
> >> >> FreeIPA
> >> >> server.
> >> > Old server is not a problem here: ipa-sam only talks to the
> >> > localhost-based server as we always use ldapi protocol. So if server is
> >> > running old-behavior FreeIPA, ipa-sam on the same server will work
> >> > against it.
> >> >
> >> > What I don't want to have is a situation where setkeytab is disabled and
> >> > a new server obeys it but ipa-sam on this new server is not updated and
> >> > will expectedly fail. We are not that forced to do the change right now
> >> > in 4.3, given that the default will still be to keep setkeytab, thus we
> >> > can wait with this patch until ipa-sam is fixed and then push both
> >> > patches into the closest release, be it 4.3.x (x>=0) or whatever is the
> >> > next one.
> >> >
> >>
> >> +1, fixing ipa-sam would be then a release blocker for 4.3 which is not
> >> desired.
> >>
> >> Also what about adding support for "ipaProtectedoperation check" for
> >> user principals?
> >>
> >> I'm afraid that forbidding getting user principal might be regarded as a
> >> regression which might cause that admins won't set DisableSetKeytab.
> >
> >One thing at a time.
> >We have bugs open for all these things, but I see no reason not to add
> >an *optional* setting just because some things break when it is turned
> >on.
> The problem is that current getkeytab extended operation has not enough
> information to be a replacement for ipa-sam's use of setkeytab, as I've
> found with your current patches. So adding an optional setting in the
> hope that it will not be used in real life is a bit naive, but if people
> activate it, whole trust setup will break. I still prefer to have the
> patchset first be completed and then merged. There is no problem in
> keeping it aside while functionality is being implemented, we can
> trivially rebase.

I just sent patches 562 and 563 which will address the ipa-sam issue
(tested by Alexander already).

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] 0001 Refactor test_user_plugin

[Filip Škola]

Hi,

sending corrected version.

F.

On Thu, 12 Nov 2015 14:03:19 +0100
Milan Kubík  wrote:

> On 11/10/2015 12:13 PM, Filip Škola wrote:
> > Hi,
> >
> > fixed.
> >
> > F.
> >
> > On Tue, 10 Nov 2015 10:52:45 +0100
> > Milan Kubík  wrote:
> >
> >> On 11/09/2015 04:35 PM, Filip Škola wrote:
> >>> Another patch was applied in the meantime.
> >>>
> >>> Attaching an updated version.
> >>>
> >>> F.
> >>>
> >>> On Mon, 9 Nov 2015 13:35:02 +0100
> >>> Milan Kubík  wrote:
> >>>
>  On 11/06/2015 11:32 AM, Filip Škola wrote:
>  Hi,
>  the patch doesn't apply.
> 
> >> Please fix this.
> >>
> >>   ipatests/test_xmlrpc/test_user_plugin.py:1419:
> >> [E0602(undefined-variable),
> >> TestDeniedBindWithExpiredPrincipal.teardown_class] Undefined
> >> variable 'user1')
> >>
> >> Also, use the version numbers for your changed patches.
> >>
> >
> >
> Thanks for the patch. Several issues:
> 
> 1. Use dict.items instead of dict.iteritems, for python3 compatibility
> 
> 2. What is the purpose of TestPrepare class? The 'purge' methods do
> not call any ipa commands.
> Tracker.make_fixture should be used to make the Tracked resources
> clean themselves up when they're out of scope.
> 
> 3. Why reference the resources by hardcoded name if they have a
> fixture representation?
> 
> 4. Rewrite {create,delete}_test_group to a fixture. You may want to
> use different scope (or not).
> 
> 5. In `def atest_rename_to_invalid_login(self, user):` - use 
> pytest.skipif decorator and provide a reason if you must,
> do not obfuscate method name in order not to run it.
> 
> 

>From e2411ccfbabbb2a9e747c547141466ed38331528 Mon Sep 17 00:00:00 2001
From: Filip Skola 
Date: Fri, 6 Nov 2015 10:57:37 +0100
Subject: [PATCH] Refactor test_user_plugin, use UserTracker for tests

---
 ipatests/test_xmlrpc/test_user_plugin.py| 2387 ++-
 ipatests/test_xmlrpc/tracker/user_plugin.py |  162 +-
 2 files changed, 1033 insertions(+), 1516 deletions(-)

diff --git a/ipatests/test_xmlrpc/test_user_plugin.py b/ipatests/test_xmlrpc/test_user_plugin.py
index 084fb83c42d362204ff4547357226c8f56d217fb..7d299d228aef4881efce5b3f575c95d21a0d41d3 100644
--- a/ipatests/test_xmlrpc/test_user_plugin.py
+++ b/ipatests/test_xmlrpc/test_user_plugin.py
@@ -2,6 +2,7 @@
 #   Rob Crittenden 
 #   Pavel Zuna 
 #   Jason Gerard DeRose 
+#   Filip Skola 
 #
 # Copyright (C) 2008, 2009  Red Hat
 # see file 'COPYING' for use and warranty information
@@ -23,6 +24,7 @@
 Test the `ipalib/plugins/user.py` module.
 """
 
+import pytest
 import datetime
 import ldap
 import re
@@ -30,42 +32,42 @@ import re
 from ipalib import api, errors
 from ipatests.test_xmlrpc import objectclasses
 from ipatests.util import (
-assert_equal, assert_not_equal, raises)
+assert_deepequal, assert_equal, assert_not_equal, raises)
 from xmlrpc_test import (
-XMLRPC_test, Declarative, fuzzy_digits, fuzzy_uuid, fuzzy_password,
-fuzzy_string, fuzzy_dergeneralizedtime, add_sid, add_oc)
+XMLRPC_test, fuzzy_digits, fuzzy_uuid, fuzzy_password,
+fuzzy_string, fuzzy_dergeneralizedtime, add_sid, add_oc, raises_exact)
 from ipapython.dn import DN
-import pytest
+from ipapython.version import API_VERSION
+
+from ipatests.test_xmlrpc.ldaptracker import Tracker
+from ipatests.test_xmlrpc.tracker.group_plugin import GroupTracker
+from ipatests.test_xmlrpc.tracker.user_plugin import UserTracker
 
-user1 = u'tuser1'
-user2 = u'tuser2'
 admin1 = u'admin'
-admin2 = u'admin2'
-renameduser1 = u'tuser'
-group1 = u'group1'
-admins_group = u'admins'
+admin_group = u'admins'
 
 invaliduser1 = u'+tuser1'
 invaliduser2 = u'tuser1234567890123456789012345678901234567890'
 
 sshpubkey = (u'ssh-rsa B3NzaC1yc2EDAQABAAABAQDGAX3xAeLeaJggwTqMjxNwa6X'
-  'HBUAikXPGMzEpVrlLDCZtv00djsFTBi38PkgxBJVkgRWMrcBsr/35lq7P6w8KGI'
-  'wA8GI48Z0qBS2NBMJ2u9WQ2hjLN6GdMlo77O0uJY3251p12pCVIS/bHRSq8kHO2'
-  'No8g7KA9fGGcagPfQH+ee3t7HUkpbQkFTmbPPN++r3V8oVUk5LxbryB3UIIVzNm'
-  'cSIn3JrXynlvui4MixvrtX6zx+O/bBo68o8/eZD26QrahVbA09fivrn/4h3TM01'
-  '9Eu/c2jOdckfU3cHUV/3Tno5d6JicibyaoDDK7S/yjdn5jhaz8MSEayQvFkZkiF'
-  '0L public key test')
+ 'HBUAikXPGMzEpVrlLDCZtv00djsFTBi38PkgxBJVkgRWMrcBsr/35lq7P6w8KGI'
+ 'wA8GI48Z0qBS2NBMJ2u9WQ2hjLN6GdMlo77O0uJY3251p12pCVIS/bHRSq8kHO2'
+ 'No8g7KA9fGGcagPfQH+ee3t7HUkpbQkFTmbPPN++r3V8oVUk5LxbryB3UIIVzNm'
+ 'cSIn3JrXynlvui4MixvrtX6zx+O/bBo68o8/eZD26QrahVbA09fivrn/4h3TM01'
+ '9Eu/c2jOdckfU3cHUV/3Tno5d6JicibyaoDDK7S/yjdn5jhaz8MSEayQvFkZkiF'
+ '0L public key test')
 sshpubkeyfp = (u'13:67:6B:BF:4E:A2:05:8E:AE:25:8B:A1:31:DE:6F:1B '
-'public key test (ssh-rsa)')
+   'public key test (ssh-rsa)')
 
-validlanguage1 = u'en-US;q=0.987 , en, 

[Freeipa-devel] [PATCH] 0751 spec: Split out python-ipap11helper and, python-default_encoding_utf8

[Petr Viktorin]

Hello,
This specfile patch makes python-ipalib noarch, by splitting out the
arch-dependent stuff.

It depends on Honza Cholasta's patches 516 and 517.

Part of https://fedorahosted.org/freeipa/ticket/3197

-- 
Petr Viktorin
From 64bd17e9e95987c1c841c78aa6a70c7acd296e38 Mon Sep 17 00:00:00 2001
From: Petr Viktorin 
Date: Thu, 3 Dec 2015 16:42:37 +0100
Subject: [PATCH] spec: Split out python-ipap11helper and
 python-default_encoding_utf8

Splitting out the packages with compiled code allows the python-ipalib
package to be noarch.

https://fedorahosted.org/freeipa/ticket/3197
---
 freeipa.spec.in | 44 +---
 1 file changed, 41 insertions(+), 3 deletions(-)

diff --git a/freeipa.spec.in b/freeipa.spec.in
index 0724794ecd81e7b007f5ca35afe2dd6924a0a789..5d4da5e64ada0f7834b50d0b87efdebea95d3781 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -315,6 +315,7 @@ This package provides command-line tools for IPA administrators.
 %package -n python-ipalib
 Summary: Python libraries used by IPA
 Group: System Environment/Libraries
+BuildArch: noarch
 Requires: python-gssapi >= 1.1.2
 Requires: gnupg
 Requires: iproute
@@ -335,6 +336,8 @@ Requires: dbus-python
 Requires: python-setuptools
 Requires: python-six
 Requires: python-jwcrypto
+Requires: python-ipap11helper
+Requires: python-default_encoding_utf8
 
 Provides: %{name}-python = %{version}-%{release}
 Obsoletes: %{name}-python < 4.3
@@ -350,6 +353,31 @@ features for further integration with Linux based clients (SUDO, automount)
 and integration with Active Directory based infrastructures (Trusts).
 If you are using IPA, you need to install this package.
 
+%package -n python-ipap11helper
+Summary: IPA pkcs11 helper
+Group: System Environment/Libraries
+
+Obsoletes: %{name}-python < 4.3
+
+%description -n python-ipap11helper
+IPA is an integrated solution to provide centrally managed Identity (users,
+hosts, services), Authentication (SSO, 2FA), and Authorization
+(host access control, SELinux user roles, services). The solution provides
+features for further integration with Linux based clients (SUDO, automount)
+and integration with Active Directory based infrastructures (Trusts).
+If you are using IPA, you need to install this package.
+
+%package -n python-default_encoding_utf8
+Summary: Set the default string encoding for the Python Unicode implementation
+Group: System Environment/Libraries
+
+Obsoletes: %{name}-python < 4.3
+
+%description -n python-default_encoding_utf8
+Exposes the PyUnicode_SetDefaultEncoding function to Python code,
+allowing programs to modify the internal default string encoding
+for Python 2.
+
 %if ! %{ONLY_CLIENT}
 %package tests
 Summary: IPA tests and test tools
@@ -979,14 +1007,24 @@ fi
 %{python_sitelib}/ipalib/*
 %dir %{python_sitelib}/ipaplatform
 %{python_sitelib}/ipaplatform/*
-%attr(0644,root,root) %{python_sitearch}/default_encoding_utf8.so
-%attr(0644,root,root) %{python_sitearch}/_ipap11helper.so
 %{python_sitelib}/ipapython-*.egg-info
 %{python_sitelib}/freeipa-*.egg-info
 %{python_sitelib}/ipaplatform-*.egg-info
-%{python_sitearch}/python_default_encoding-*.egg-info
+
+%files -n python-ipap11helper
+%defattr(-,root,root,-)
+%doc README Contributors.txt
+%license COPYING
+%attr(0644,root,root) %{python_sitearch}/_ipap11helper.so
 %{python_sitearch}/_ipap11helper-*.egg-info
 
+%files -n python-default_encoding_utf8
+%defattr(-,root,root,-)
+%doc README Contributors.txt
+%license COPYING
+%attr(0644,root,root) %{python_sitearch}/default_encoding_utf8.so
+%{python_sitearch}/python_default_encoding-*.egg-info
+
 %if ! %{ONLY_CLIENT}
 %files tests -f tests-python.list
 %defattr(-,root,root,-)
-- 
2.5.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 556-557] Add option to disable setkeytab extended operations

[Simo Sorce]

On Thu, 2015-12-03 at 09:31 +0100, Petr Vobornik wrote:
> On 12/02/2015 07:16 PM, Simo Sorce wrote:
> > On Tue, 2015-12-01 at 16:44 +0100, Petr Vobornik wrote:
> >> On 12/01/2015 04:20 PM, Alexander Bokovoy wrote:
> >>> On Tue, 01 Dec 2015, Martin Kosek wrote:
>  On 12/01/2015 02:59 PM, Simo Sorce wrote:
> > On Tue, 2015-12-01 at 14:42 +0100, Martin Kosek wrote:
> >> On 12/01/2015 02:38 PM, Simo Sorce wrote:
> >>> On Tue, 2015-12-01 at 10:11 +0200, Alexander Bokovoy wrote:
>  On Mon, 30 Nov 2015, Simo Sorce wrote:
> > On Wed, 2015-11-25 at 09:47 -0500, Simo Sorce wrote:
> >> On Wed, 2015-11-25 at 09:02 -0500, Rob Crittenden wrote:
> >>> Jan Cholasta wrote:
>  On 24.11.2015 22:17, Simo Sorce wrote:
> > On Tue, 2015-11-24 at 14:57 -0500, Simo Sorce wrote:
> >> On Tue, 2015-11-24 at 14:42 -0500, Simo Sorce wrote:
> >>> Since some time we use the getkeytab operation to fetch
> >>> keytabs on
> >>> newer
> >>> clients. According to bug #232 setkeytab can be used to
> >>> circumvent
> >>> password quality controls so it needs to be slowly retired.
> >>>
> >>> The attached patches implement #5485 in 2 parts.
> >>>
> >>> The first introduces the option DisableSetKeytab which
> >>> globally
> >>> disables
> >>> the setkeytab extended operation. This is set to false by
> >>> default for
> >>> backwards compatibility.
> >>>
> >>> The second introduces an option called
> >>> DisableUserSetKeytab, which is
> >>> active by default in new installs (but not in upgraded
> >>> ones), and only
> >>> disables the use of setkeytab for ipa suers, but not for
> >>> hosts/services.
> >>> This is because user's are the ones that may abuse the
> >>> interface to
> >>> escape password policies and users also normally do not
> >>> acquire
> >>> keytabs,
> >>> so it is a safe bet to disable just them by default in new
> >>> installs.
> >>>
> >>> (Testing in progress)
> >>
> >> Tested and working as expected.
> >
> > I realized that adding options to ipaConfig require to add
> > them in the
> > UI as well, attached patches add options in API.txt and
> > config.py
> > Make now complain I should change API Major or Minor, but it
> > is not
> > clear to me why given this are additional values and no real
> > change or
> > new function is introduced. What's the recommendation ?
> 
>  When does make complain? It is supposed to complain only when
>  API.txt
>  does not match code.
> 
>  Anyway, we usually bump minor version even for backward
>  compatible
>  changes, see e.g. commit 9549a59.
> 
> >>>
> >>> The point of API.txt (and the heavy client) was to save a
> >>> round-trip.
> >>> Being able to pass in an invalid option would void that rule hence
> >>> having to update the API when new values are added.
> >>>
> >>> rob
> >>
> >> Ok added version change to the second patch (so we bump it only
> >> once
> >> given these are basically related changes.
> >
> > Bump, is this ok ?
>  This patch is fine but please fix setkeytab use in ipa-sam before
>  committing this patch.
> >>>
> >>> This patch does not disable setkeytab yet, so it can go in right away
> >>> (it blocks other patches already). We have a bug to change ipa-sam,
> >>> should we mark it blocker for 4.4 ?
> >>
> >> We also have to fix the permission to change keytab, so that it uses
> >> the new
> >> style (https://fedorahosted.org/freeipa/ticket/5487). I would
> >> actually make
> >> this ticket and the ipa-sam ticket a blocker for this patch set.
> >>
> >> Otherwise you are actually introducing a switch that breaks FreeIPA
> >> as some of
> >> it's core server functions still use the old method.
> >
> > The point of this patchset is to introduce the switch early so that
> > current server will support the off switch when newer servers down the
> > road are ready to flip it. The defaults are still to allow these
> > operations for services/hosts.
> 
>  I still do not get the logic about an early switch. Now, if switch is
>  turned
>  on, FreeIPA server breaks on several places. I would really rather
>  expect the
>  switch to be introduced when the server itself 

Re: [Freeipa-devel] [PATCH 562-563] Fix ipa-sam to use the getkeytab control instead of the setkeytab control

[Alexander Bokovoy]

On Thu, 03 Dec 2015, Simo Sorce wrote:

The first patch is preparatory and is needed in general now that we want
top allow alias and use krbCanonicalName as the canonical name when
multiple values are avilable in krbPrincipalName.

The second patch changes slightly how the interdomain trust account is
created so that the getkeytab control can generate the proper key (with
the right salt) for interop reasons with AD. The change should be
upgrade safe because keys are generate at account creation so older
accounts lacking the alias won't be a problem.

Fixes ##5495

Thanks. ACK to both. They work for me against Windows Server 2012R2.

Now we need to fix Samba AD salt generation so that it is compatible
with both Windows and FreeIPA for AES/DES keys and not only RC4... ;)

--
/ 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

[Freeipa-devel] [PATCH] 495 Update Build instructions

[Martin Kosek]

Original dnf builddep command does not work, unless --spec option is
added.

-- 
Martin Kosek 
Associate Manager, Software Engineering - Identity Management Team
Red Hat, Inc.
From 12971ffdd4f3c16536457113c0b3b0c1ee12ac2a Mon Sep 17 00:00:00 2001
From: Martin Kosek 
Date: Thu, 3 Dec 2015 16:10:34 +0100
Subject: [PATCH] Update Build instructions

Original dnf builddep command does not work, unless --spec option is
added.
---
 BUILD.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/BUILD.txt b/BUILD.txt
index 4507fa357375edc09377368ab3d1eb1c48994a61..fb1a7996b6b0c69c5431b180eec65c2549448d91 100644
--- a/BUILD.txt
+++ b/BUILD.txt
@@ -7,7 +7,7 @@ For more information, see http://www.freeipa.org/page/Build
 
 The quickest way to get the dependencies needed for building is:
 
-# dnf builddep -b freeipa.spec.in
+# dnf builddep -b --spec freeipa.spec.in
 
 or
 
-- 
2.5.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 acceptance criteria

[Petr Spacek]

On 3.12.2015 16:07, Rob Crittenden wrote:
> Petr Spacek wrote:
>> On 3.12.2015 15:34, Rob Crittenden wrote:
>>> Martin Kosek wrote:
 On 12/03/2015 09:08 AM, Petr Spacek wrote:
> On 2.12.2015 19:14, Rob Crittenden wrote:
>> Is it still mandatory that tests pass the unit tests before acceptance?
>> I've seen a number of cases over the past couple of months where a
>> change goes through then shortly afterward a patch to fix the tests.
>> IMHO this should be caught in advance.
>>
>> Things slip through and goodness knows I've acked more than a few
>> patches without running the full suite. I just have a feeling it has
>> become more frequent lately.
>
> When we are at it... An automated thingy which accepts URL to a Git repo, 
> does
> all the test magic, and spits out test results without user interaction 
> would
> be an awesome Christmas present!
>
> Bonus points if we can get Github integration so I can just push and have 
> it
> tested automatically so I cannot forget to do that before sending the 
> patch
> for review.

 +1. Having basic CI test suite run on top of a Pull Request would be 
 awesome.

>>>
>>> I'd be happy with just the ipatests being run manually with each review.
>>>
>>> And it's then reviewer that I'm focusing on here. A developer _should_
>>> also run the tests but part of the reviewer's responsibility is to
>>> ensure the patch does what it says it does without breaking things.
>>
>> Sure. I'm just saying that people are notoriously bad automation tool, so I
>> would like to off-load this kind of checks to a tool which will not forget or
>> be lazy :-)
>>
> 
> Sure, but it's the _job_ of the reviewer to do these things. My previous
> statement is better read as: A developer _shall_ also run the tests.
> 
> And even beyond that reviewers need to ensure that the patch works, does
> it properly in the context of IPA, doesn't break anything, works in
> SELinux enforcing mode, updates man pages, etc.
> 
> In other words, when a patch breaks something, don't blame the
> developer, blame the reviewer.

I would blame both ;-)

Again, I'm not against proper reviews, just saying that it simply does not
scale, so it needs automation.

-- 
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

[Freeipa-devel] [PATCH] 940 Update ipa-(cs)replica-manage man pages

[Petr Vobornik]

SSIA
--
Petr Vobornik
From a47fd60f49f1a87bb86913463df7c7813d1cdad3 Mon Sep 17 00:00:00 2001
From: Petr Vobornik 
Date: Thu, 3 Dec 2015 16:28:51 +0100
Subject: [PATCH] Update ipa-(cs)replica-manage man pages

---
 install/tools/man/ipa-csreplica-manage.1 | 17 -
 install/tools/man/ipa-replica-manage.1   | 13 +
 2 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/install/tools/man/ipa-csreplica-manage.1 b/install/tools/man/ipa-csreplica-manage.1
index 3164ea60d67db99445dac168fad967cb48be428e..ab5bfddd884f65b6806c4e69f212324dc5b67c5f 100644
--- a/install/tools/man/ipa-csreplica-manage.1
+++ b/install/tools/man/ipa-csreplica-manage.1
@@ -22,16 +22,20 @@ ipa\-csreplica\-manage \- Manage an IPA CS replica
 .SH "SYNOPSIS"
 ipa\-csreplica\-manage [\fIOPTION\fR]...  [connect|disconnect|del|list|re\-initialize|force\-sync]
 .SH "DESCRIPTION"
-Manages the CA replication agreements of an IPA server.
+Manages the CA replication agreements of an IPA server for domain at domain level 0.
+
+To manage CA replication agreements in a domain at domain level 1, use IPA CLI or Web UI, see `ipa help topology` for additional information.
+
 .TP
 \fBconnect\fR [SERVER_A] 
-\- Adds a new replication agreement between SERVER_A/localhost and SERVER_B
+\- Adds a new replication agreement between SERVER_A/localhost and SERVER_B. Applicable only at domain level 0.
 .TP
 \fBdisconnect\fR [SERVER_A] 
-\- Removes a replication agreement between SERVER_A/localhost and SERVER_B
+\- Removes a replication agreement between SERVER_A/localhost and SERVER_B. Applicable only at domain level 0.
 .TP
 \fBdel\fR 
-\- Removes all replication agreements and data about SERVER
+\- Removes all replication agreements and data about SERVER. Applicable only at domain level 0.
+
 .TP
 \fBlist\fR [SERVER]
 \- Lists all the servers or the list of agreements of SERVER
@@ -86,9 +90,12 @@ Add a new replication agreement:
 Remove an existing replication agreement:
  # ipa\-csreplica\-manage disconnect srv1.example.com srv3.example.com
 .TP
-Completely remove a replica:
+Completely remove a replica at domain level 0:
  # ipa\-csreplica\-manage del srv4.example.com
 .TP
+Completely remove a replica at domain level 1:
+ # ipa\-replica\-manage del srv4.example.com
+.TP
 Using connect/disconnect you can manage the replication topology.
 .SH "EXIT STATUS"
 0 if the command was successful
diff --git a/install/tools/man/ipa-replica-manage.1 b/install/tools/man/ipa-replica-manage.1
index c09ed362f3143e6e38716e1b3a96e90001a64674..46facf31affb3fbcba63deaedd171379da15bf8b 100644
--- a/install/tools/man/ipa-replica-manage.1
+++ b/install/tools/man/ipa-replica-manage.1
@@ -22,16 +22,21 @@ ipa\-replica\-manage \- Manage an IPA replica
 .SH "SYNOPSIS"
 ipa\-replica\-manage [\fIOPTION\fR]... [COMMAND]
 .SH "DESCRIPTION"
-Manages the replication agreements of an IPA server. The available commands are:
+Manages the replication agreements of an IPA server.
+
+To manage IPA replication agreements in a domain at domain level 1, use IPA CLI
+or Web UI, see `ipa help topology` for additional information.
+
+The available commands are:
 .TP
 \fBconnect\fR [SERVER_A] 
-\- Adds a new replication agreement between SERVER_A/localhost and SERVER_B
+\- Adds a new replication agreement between SERVER_A/localhost and SERVER_B. Applicable only for winsync agreements at domain level 1.
 .TP
 \fBdisconnect\fR [SERVER_A] 
-\- Removes a replication agreement between SERVER_A/localhost and SERVER_B
+\- Removes a replication agreement between SERVER_A/localhost and SERVER_B. Applicable only for winsync agreements at domain level 1.
 .TP
 \fBdel\fR 
-\- Removes all replication agreements and data about SERVER
+\- Removes all replication agreements and data about SERVER. At domain level 1 it removes data and agreements for both suffixes - domain and ca.
 .TP
 \fBlist\fR [SERVER]
 \- Lists all the servers or the list of agreements of SERVER
-- 
2.4.3

-- 
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] 941 Extend topology help

[Petr Vobornik]

`ipa help topology` is improved.
--
Petr Vobornik
From 7fcaa87aab86d816ee6bc63a4bbaf5c65f4961d9 Mon Sep 17 00:00:00 2001
From: Petr Vobornik 
Date: Thu, 3 Dec 2015 16:29:27 +0100
Subject: [PATCH] Extend topology help

`ipa help topology` is improved.
---
 ipalib/plugins/topology.py | 52 --
 1 file changed, 50 insertions(+), 2 deletions(-)

diff --git a/ipalib/plugins/topology.py b/ipalib/plugins/topology.py
index 40f9fa8038d4bcb6c0eeb82a37ac796d732b82fd..36330dee45738434266fd15d5e28167bdedb5a12 100644
--- a/ipalib/plugins/topology.py
+++ b/ipalib/plugins/topology.py
@@ -22,9 +22,57 @@ if six.PY3:
 __doc__ = _("""
 Topology
 
-Management of a replication topology.
+Management of a replication topology at domain level 1.
+""") + _("""
+IPA server's data is stored in LDAP server in two suffixes:
+* domain suffix, e.g., 'dc=example,dc=com', contains all domain related data
+* ca suffix, 'o=ipaca', is present only on server with CA installed. It
+  contains data for Certificate Server component
+""") + _("""
+Data stored on IPA servers is replicated to other IPA servers. The way it is
+replicated is defined by replication agreements. Replication agreements needs
+to be set for both suffixes separately. On domain level 0 they are managed
+using ipa-replica-manage and ipa-csreplica-manage tools. With domain level 1
+they are managed centrally using `ipa topology*` commands.
+""") + _("""
+Agreements are represented by topology segments. By default topology segment
+represents 2 replication agreements - one for each direction, e.g., A to B and
+B to A. Creation of unidirectional segments is not allowed.
+""") + _("""
+To verify if all servers are replicated with other servers use command:
+  ipa topologysuffix-verify $suffix
+""") + _("""
 
-Requires minimum domain level 1.
+Examples:
+  Find all IPA servers:
+ipa server-find
+""") + _("""
+  Find all suffixes:
+ipa topologysuffix-find
+""") + _("""
+  Add topology segment to 'domain' suffix:
+ipa topologysegment-add domain --left IPA_SERVER_A --right IPA_SERVER_B
+""") + _("""
+  Add topology segment to 'ca' suffix:
+ipa topologysegment-add ca --left IPA_SERVER_A --right IPA_SERVER_B
+""") + _("""
+  List all topology segments in 'domain' suffix:
+ipa topologysegment-find domain
+""") + _("""
+  List all topology segments in 'ca' suffix:
+ipa topologysegment-find ca
+""") + _("""
+  Delete topology segment in 'domain' suffix:
+ipa topologysegment-del domain segment_name
+""") + _("""
+  Delete topology segment in 'ca' suffix:
+ipa topologysegment-del ca segment_name
+""") + _("""
+  Verify topology of 'domain' suffix:
+ipa topologysuffix-verify domain
+""") + _("""
+  Verify topology of 'ca' suffix:
+ipa topologysuffix-verify ca
 """)
 
 register = Registry()
-- 
2.4.3

-- 
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 acceptance criteria

[Lukas Slebodnik]

On (03/12/15 15:53), Petr Spacek wrote:
>On 3.12.2015 15:34, Rob Crittenden wrote:
>> Martin Kosek wrote:
>>> On 12/03/2015 09:08 AM, Petr Spacek wrote:
 On 2.12.2015 19:14, Rob Crittenden wrote:
> Is it still mandatory that tests pass the unit tests before acceptance?
> I've seen a number of cases over the past couple of months where a
> change goes through then shortly afterward a patch to fix the tests.
> IMHO this should be caught in advance.
>
> Things slip through and goodness knows I've acked more than a few
> patches without running the full suite. I just have a feeling it has
> become more frequent lately.

 When we are at it... An automated thingy which accepts URL to a Git repo, 
 does
 all the test magic, and spits out test results without user interaction 
 would
 be an awesome Christmas present!

 Bonus points if we can get Github integration so I can just push and have 
 it
 tested automatically so I cannot forget to do that before sending the patch
 for review.
>>>
>>> +1. Having basic CI test suite run on top of a Pull Request would be 
>>> awesome.
>>>
>> 
>> I'd be happy with just the ipatests being run manually with each review.
>> 
>> And it's then reviewer that I'm focusing on here. A developer _should_
>> also run the tests but part of the reviewer's responsibility is to
>> ensure the patch does what it says it does without breaking things.
>
>Sure. I'm just saying that people are notoriously bad automation tool, so I
>would like to off-load this kind of checks to a tool which will not forget or
>be lazy :-)
>
Petr, patches are always welcomed.

LS

-- 
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 acceptance criteria

[Rob Crittenden]

Lukas Slebodnik wrote:
> On (02/12/15 13:14), Rob Crittenden wrote:
>> Is it still mandatory that tests pass the unit tests before acceptance?
> Unit test could be executed as part of "%check" phase in spec files.
> I recently added C-base unit tests there.
> 
> I was not bale to run "make tests" there because many tests failed.
> If they require real IPA server for execution ( or lite server)
> then they are not unit test but integration tests.
> One solution would be to skip them or to usw cwrap[1] + lite server.
> So it can be run also in mock/koji which has many restrictions.

It would represent quite a lot of work but it may be a good idea to
investigate. Ipsilon uses cwrap for its tests so some of the
configuration can be gleaned from that.

I would definitely be opposed to this as part of the freeipa.spec in the
git tree. In Fedora it might help find problems when rawhide becomes
Fedora.next so it would provide some value there.

> 
> Also lint should be also part of "%check" phase and not part of
> ordinary build.
> BTW I could not see a lint[2] in fedora build at all. So I'm not sure
> if it is executed with upstream spec file.

It isn't there because the expectation is that lint already passes as
part of the release process. I don't see the value on running lint on
release tarballs.

I also want to keep the focus on the reviewer's responsibility to ensure
that patches do what they are supposed to and don't break things.

rob

> If someone wants to comply that it would take long time to build rpms
> then you might take a look how long glibc is built.
> http://koji.fedoraproject.org/koji/buildinfo?buildID=697271
> (half an hour on x86_64)
> 
>> I've seen a number of cases over the past couple of months where a
>> change goes through then shortly afterward a patch to fix the tests.
>> IMHO this should be caught in advance.
>>
>> Things slip through and goodness knows I've acked more than a few
>> patches without running the full suite. I just have a feeling it has
>> become more frequent lately.
>>
>> rob
> 
> LS
> 
> [1] https://cwrap.org/
> [2] 
> https://kojipkgs.fedoraproject.org//packages/freeipa/4.2.3/1.1.fc23/data/logs/x86_64/build.log
> 

-- 
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 561] Catch up with upstream kerberos.ldif schema

[Martin Basti]

On 25.11.2015 00:41, Simo Sorce wrote:

Not much action here, but it will close a ticket (#2086) and get us on
par with upstream.

Simo.




ACK

Pushed to master: 5ed1b844dcb7d4a57a7067eef63b644df47bd740

-- 
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 0108] replica install: improvements in the handling of CA-related IPA config entries

[Martin Babinsky]

This patch fixes https://fedorahosted.org/freeipa/ticket/5506 and also 
reorganizes the way CA installer updates IPA default.conf.


Maybe a simpler patch would suffice, but I had a need to improve things 
a bit.


This one is for master branch only. IIRC the situation described in 
#5506 does not occur in domain level 0, however the root cause 
(incorrect forwarding of certmonger requests by CA-less replicas) 
manifests when enrolling a client against CA-less replica and requesting 
host certificate.


Should I open a separate ticket for that?

--
Martin^3 Babinsky
From c67a6c03a4f2ed82aea7e0da03c9e2270eea2d42 Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Wed, 2 Dec 2015 12:22:45 +0100
Subject: [PATCH] replica install: improvements in the handling of CA-related
 IPA config entries

When a CA-less replica is installed, its IPA config file should be updated so
that ca_host points to nearest CA master and all certificate requests are
forwarded to it. A subsequent installation of CA subsystem on the replica
should clear this entry from the config so that all certificate requests are
handled by freshly installed local CA.

https://fedorahosted.org/freeipa/ticket/5506
---
 ipaserver/install/ca.py| 16 
 ipaserver/install/cainstance.py| 19 ++-
 ipaserver/install/server/replicainstall.py |  7 +++
 3 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/ipaserver/install/ca.py b/ipaserver/install/ca.py
index fcead1891583c2e495951fbeb733e6eec3b07ccf..1a51ebc8cf994eae70323ee0642bebd746080de2 100644
--- a/ipaserver/install/ca.py
+++ b/ipaserver/install/ca.py
@@ -7,8 +7,6 @@ from __future__ import print_function
 import sys
 import os.path
 
-from six.moves.configparser import RawConfigParser
-
 from ipaserver.install import cainstance, dsinstance, bindinstance
 from ipapython import ipautil, certdb
 from ipaplatform import services
@@ -236,20 +234,6 @@ def install_step_1(standalone, replica_config, options):
 if standalone:
 ca.start('pki-tomcat')
 
-# Update config file
-try:
-parser = RawConfigParser()
-parser.read(paths.IPA_DEFAULT_CONF)
-parser.set('global', 'enable_ra', 'True')
-parser.set('global', 'ra_plugin', 'dogtag')
-parser.set('global', 'dogtag_version', '10')
-with open(paths.IPA_DEFAULT_CONF, 'w') as f:
-parser.write(f)
-except IOError as e:
-print("Failed to update /etc/ipa/default.conf")
-root_logger.error(str(e))
-sys.exit(1)
-
 # We need to restart apache as we drop a new config file in there
 services.knownservices.httpd.restart(capture_output=True)
 
diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py
index 65f9e463d39ca1ecf4c42ca22620cf1f2de06880..2ca718a7b6799b7daf825918517a54852746a84f 100644
--- a/ipaserver/install/cainstance.py
+++ b/ipaserver/install/cainstance.py
@@ -41,7 +41,7 @@ import shlex
 import pipes
 
 from six.moves import urllib
-from six.moves.configparser import ConfigParser
+from six.moves.configparser import ConfigParser, RawConfigParser
 
 from ipalib import api
 from ipalib import pkcs10, x509
@@ -429,6 +429,7 @@ class CAInstance(DogtagInstance):
 self.step("importing IPA certificate profiles",
   import_included_profiles)
 self.step("adding default CA ACL", ensure_default_caacl)
+self.step("updating IPA configuration", update_ipa_conf)
 
 self.start_creation(runtime=210)
 
@@ -1343,6 +1344,7 @@ class CAInstance(DogtagInstance):
   self.track_servercert)
 self.step("Configure HTTP to proxy connections",
   self.http_proxy)
+self.step("updating IPA configuration", update_ipa_conf)
 self.step("Restart HTTP server to pick up changes",
   self.__restart_http_instance)
 
@@ -1768,6 +1770,21 @@ def ensure_default_caacl():
 api.Backend.ldap2.disconnect()
 
 
+def update_ipa_conf():
+"""
+Update IPA configuration file to ensure that RA plugins are enabled and
+that CA host points to localhost
+"""
+parser = RawConfigParser()
+parser.read(paths.IPA_DEFAULT_CONF)
+parser.set('global', 'enable_ra', 'True')
+parser.set('global', 'ra_plugin', 'dogtag')
+parser.set('global', 'dogtag_version', '10')
+parser.remove_option('global', 'ca_host')
+with open(paths.IPA_DEFAULT_CONF, 'w') as f:
+parser.write(f)
+
+
 if __name__ == "__main__":
 standard_logging_setup("install.log")
 ds = dsinstance.DsInstance()
diff --git a/ipaserver/install/server/replicainstall.py b/ipaserver/install/server/replicainstall.py
index ec77ab21b1e4969bdcd8d9e588eed7b97e3a9079..d2b03431ee68c41b750fac33c3cf954d4bb5892e 100644
--- a/ipaserver/install/server/replicainstall.py
+++ 

Re: [Freeipa-devel] patch acceptance criteria

[Rob Crittenden]

Petr Spacek wrote:
> On 3.12.2015 15:34, Rob Crittenden wrote:
>> Martin Kosek wrote:
>>> On 12/03/2015 09:08 AM, Petr Spacek wrote:
 On 2.12.2015 19:14, Rob Crittenden wrote:
> Is it still mandatory that tests pass the unit tests before acceptance?
> I've seen a number of cases over the past couple of months where a
> change goes through then shortly afterward a patch to fix the tests.
> IMHO this should be caught in advance.
>
> Things slip through and goodness knows I've acked more than a few
> patches without running the full suite. I just have a feeling it has
> become more frequent lately.

 When we are at it... An automated thingy which accepts URL to a Git repo, 
 does
 all the test magic, and spits out test results without user interaction 
 would
 be an awesome Christmas present!

 Bonus points if we can get Github integration so I can just push and have 
 it
 tested automatically so I cannot forget to do that before sending the patch
 for review.
>>>
>>> +1. Having basic CI test suite run on top of a Pull Request would be 
>>> awesome.
>>>
>>
>> I'd be happy with just the ipatests being run manually with each review.
>>
>> And it's then reviewer that I'm focusing on here. A developer _should_
>> also run the tests but part of the reviewer's responsibility is to
>> ensure the patch does what it says it does without breaking things.
> 
> Sure. I'm just saying that people are notoriously bad automation tool, so I
> would like to off-load this kind of checks to a tool which will not forget or
> be lazy :-)
> 

Sure, but it's the _job_ of the reviewer to do these things. My previous
statement is better read as: A developer _shall_ also run the tests.

And even beyond that reviewers need to ensure that the patch works, does
it properly in the context of IPA, doesn't break anything, works in
SELinux enforcing mode, updates man pages, etc.

In other words, when a patch breaks something, don't blame the
developer, blame the reviewer.

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] [PATCH] 495 Update Build instructions

[Petr Vobornik]

On 12/03/2015 04:13 PM, Martin Kosek wrote:

Original dnf builddep command does not work, unless --spec option is
added.



ACK

Pushed to master: 03c7d63c52615cefef260e169ec4dadb85d54842
--
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 0388] tests: Add hostmask detection for sudo rules validating

[Tomas Babej]

On 12/03/2015 04:26 PM, Aleš Mareček wrote:
> Hello,
> 
> ACK for code
> NACK for the placing "get_client_ip_with_hostmask" function to test_sudo.py 
> (this function should be in some more general file)
> 

What place would you propose? The task.py is not a good place, as this
is not really a task.

Nevertheless, I'd rather have it moved when an use case outside
test_sudo.py actually arises. Right now it would lead to unnecessary
cluttering.

Tomas

-- 
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 acceptance criteria

[Jakub Hrozek]

On Thu, Dec 03, 2015 at 09:59:46AM -0500, Rob Crittenden wrote:
> Lukas Slebodnik wrote:
> > On (02/12/15 13:14), Rob Crittenden wrote:
> >> Is it still mandatory that tests pass the unit tests before acceptance?
> > Unit test could be executed as part of "%check" phase in spec files.
> > I recently added C-base unit tests there.
> > 
> > I was not bale to run "make tests" there because many tests failed.
> > If they require real IPA server for execution ( or lite server)
> > then they are not unit test but integration tests.
> > One solution would be to skip them or to usw cwrap[1] + lite server.
> > So it can be run also in mock/koji which has many restrictions.
> 
> It would represent quite a lot of work but it may be a good idea to
> investigate. Ipsilon uses cwrap for its tests so some of the
> configuration can be gleaned from that.

SSSD uses cwrap tests as well, Nikolai wrote tests that spin up an
openldap server and there are even tests that test against a Samba DC on
review..so IPA developers can also look into the SSSD tree.

-- 
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 0388] tests: Add hostmask detection for sudo rules validating

[Aleš Mareček]

Hello,

ACK for code
NACK for the placing "get_client_ip_with_hostmask" function to test_sudo.py 
(this function should be in some more general file)

- Original Message -
> From: "Oleg Fayans" 
> To: freeipa-devel@redhat.com
> Sent: Thursday, December 3, 2015 3:32:46 PM
> Subject: Re: [Freeipa-devel] [PATCH 0388] tests: Add hostmask detection for 
> sudo rules validating
> 
> Hi Tomas,
> 
> I would prefer get_client_ip_with_hostmask function to go to
> ipatests/test_integration/tasks.py, and to be more generic: we probably
> will need to run it against an arbitrary host.

tasks.py is not the proper place as well

> The rest looks fine
> 
> On 12/03/2015 11:35 AM, Tomas Babej wrote:
> >
> >
> > On 12/02/2015 05:25 PM, Lukas Slebodnik wrote:
> >> On (02/12/15 15:41), Tomas Babej wrote:
> >>>
> >>>
> >>> On 12/02/2015 09:24 AM, Tomas Babej wrote:
> 
> 
>  On 12/01/2015 06:27 PM, Tomas Babej wrote:
> >
> >
> > On 11/30/2015 05:32 PM, Lukas Slebodnik wrote:
> >> On (30/11/15 13:09), Tomas Babej wrote:
> >>> Hi,
> >>>
> >>> IPA sudo tests worked under the assumption that the clients that
> >>> are executing the sudo commands have their IPs assigned within
> >>> 255.255.255.0 hostmask.
> >>>
> >>> Removes this (invalid) assumption and adds a dynamic detection of
> >>> the hostmask of the IPA client.
> >>>
> >>> https://fedorahosted.org/freeipa/ticket/5501
> >>
> >> >From e6f1846f0d7d17303e5b30b1643651ba739b2b6c Mon Sep 17 00:00:00
> >> >2001
> >>> From: Tomas Babej 
> >>> Date: Mon, 30 Nov 2015 12:53:39 +0100
> >>> Subject: [PATCH] tests: Add hostmask detection for sudo rules
> >>> validating on
> >>> hostmask
> >>>
> >>> IPA sudo tests worked under the assumption that the clients that
> >>> are executing the sudo commands have their IPs assigned within
> >>> 255.255.255.0 hostmask.
> >>>
> >>> Removes this (invalid) assumption and adds a dynamic detection of
> >>> the hostmask of the IPA client.
> >>>
> >
> > Thanks, updated patch attached.
> >
> > Tomas
> >
> 
>  Actually, a small improvement is necessary.
> 
>  Updated patch attached.
> 
>  Tomas
> 
> 
> 
> >>>
> >>> Thanks to Lukas, we found another problem with the test.
> >>>
> >>> Updated patch attached.
> >>>
> >> Thank you for 4th revision of patch
> >> but there is still one issue.
> >>
> >> === FAILURES
> >> ===
> >> _ TestSudo.test_sudo_rule_restricted_to_one_hostmask_negative
> >> __
> >>
> >> self =  >> 0x7ff695f56890>
> >>
> >>  def test_sudo_rule_restricted_to_one_hostmask_negative(self):
> >>  result1 = self.list_sudo_commands("testuser1")
> >>>assert result1.returncode != 0
> >> E   assert 0 != 0
> >> E+  where 0 =  >> 0x7ff695f56bd0>.returncode
> >>
> >> test_integration/test_sudo.py:323: AssertionError
> >>  1 failed, 74 passed in 807.35 seconds
> >> =
> >>
> >> LS
> >>
> >
> > Here the test affected the negative test setup, which is fixed in the
> > latest revision.
> >
> > Thanks,
> > Tomas
> >
> >
> >
> 
> --
> Oleg Fayans
> Quality Engineer
> FreeIPA team
> RedHat.
> 
> --
> 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
> 

-- 
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 acceptance criteria

[Rob Crittenden]

Petr Spacek wrote:
> On 3.12.2015 16:07, Rob Crittenden wrote:
>> Petr Spacek wrote:
>>> On 3.12.2015 15:34, Rob Crittenden wrote:
 Martin Kosek wrote:
> On 12/03/2015 09:08 AM, Petr Spacek wrote:
>> On 2.12.2015 19:14, Rob Crittenden wrote:
>>> Is it still mandatory that tests pass the unit tests before acceptance?
>>> I've seen a number of cases over the past couple of months where a
>>> change goes through then shortly afterward a patch to fix the tests.
>>> IMHO this should be caught in advance.
>>>
>>> Things slip through and goodness knows I've acked more than a few
>>> patches without running the full suite. I just have a feeling it has
>>> become more frequent lately.
>>
>> When we are at it... An automated thingy which accepts URL to a Git 
>> repo, does
>> all the test magic, and spits out test results without user interaction 
>> would
>> be an awesome Christmas present!
>>
>> Bonus points if we can get Github integration so I can just push and 
>> have it
>> tested automatically so I cannot forget to do that before sending the 
>> patch
>> for review.
>
> +1. Having basic CI test suite run on top of a Pull Request would be 
> awesome.
>

 I'd be happy with just the ipatests being run manually with each review.

 And it's then reviewer that I'm focusing on here. A developer _should_
 also run the tests but part of the reviewer's responsibility is to
 ensure the patch does what it says it does without breaking things.
>>>
>>> Sure. I'm just saying that people are notoriously bad automation tool, so I
>>> would like to off-load this kind of checks to a tool which will not forget 
>>> or
>>> be lazy :-)
>>>
>>
>> Sure, but it's the _job_ of the reviewer to do these things. My previous
>> statement is better read as: A developer _shall_ also run the tests.
>>
>> And even beyond that reviewers need to ensure that the patch works, does
>> it properly in the context of IPA, doesn't break anything, works in
>> SELinux enforcing mode, updates man pages, etc.
>>
>> In other words, when a patch breaks something, don't blame the
>> developer, blame the reviewer.
> 
> I would blame both ;-)
> 
> Again, I'm not against proper reviews, just saying that it simply does not
> scale, so it needs automation.
> 

I haven't worked on IPA for a while so won't comment on the scaling
other than to say it's always been a busy project and reviews are always
a trade-off.

I don't want to devolve this into automation as a panacea. There are
steps for review that aren't being followed either closely enough or at
all. That's what I'm trying to address.

There should not be follow-up patches to fix tests that are failing.
There should not be follow-up patches to add missing manpage
information. And that's on the reviewer. That's all I'm saying.

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] [PATCH 562-563] Fix ipa-sam to use the getkeytab control instead of the setkeytab control

[Alexander Bokovoy]

On Thu, 03 Dec 2015, Simo Sorce wrote:

On Thu, 2015-12-03 at 19:33 +0200, Alexander Bokovoy wrote:

On Thu, 03 Dec 2015, Simo Sorce wrote:
>The first patch is preparatory and is needed in general now that we want
>top allow alias and use krbCanonicalName as the canonical name when
>multiple values are avilable in krbPrincipalName.
>
>The second patch changes slightly how the interdomain trust account is
>created so that the getkeytab control can generate the proper key (with
>the right salt) for interop reasons with AD. The change should be
>upgrade safe because keys are generate at account creation so older
>accounts lacking the alias won't be a problem.
>
>Fixes ##5495
Thanks. ACK to both. They work for me against Windows Server 2012R2.

Now we need to fix Samba AD salt generation so that it is compatible
with both Windows and FreeIPA for AES/DES keys and not only RC4... ;)


And so we did:
https://git.samba.org/?p=idra/samba.git;a=commitdiff;h=8e87601a998b43f58589ff88341946ca4d9ab5ee;hp=412cefc7c8222ccc77e15099a162f9fb7bb01c57
and:
https://twitter.com/abbrasuo/status/672480716928716800

:-)

Yep, thanks Simo!

I've posted few screenshots of the current status of Samba AD with MIT
Kerberos running on Fedora 23 and establishing cross-forest trust to
FreeIPA on my Google+ page:
https://plus.google.com/+AlexanderBokovoy/posts/NgozL7Rgw64

The patches to Samba are in Andreas' git tree, plus few changes Simo did
for proper generation of the salt for interdomain trust object keys.
Currently Samba generates the salt principal wrongly for TDO keys and it
works in Heimdal only because Heimdal users RC4 keys for cross-realm
trust which does not use the salt.

Once Simo fixed the salt in password_hash ldb module, we were able to
complete trust to FreeIPA in such way that MIT KDC was able to respond
on AS request for the interdomain TDO principal and SSSD on FreeIPA side
was able to use the resulting Kerberos session to authenticate with SASL
GSSAPI to Samba AD's LDAP to look up users and groups. The POSIX
attributes are managed by FreeIPA (UID/GIDs are autogenerated in this
deployment) but they can also be picked up from Samba AD.

We plan to work on remaining fixes to eventually get the full Samba AD
support in Fedora 24, but this represents a huge milestone in our four
year quest to make it a reality.

Thanks to everyone!

--
/ 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] [PATCH 0002] Refactor test_group_plugin

[Filip Škola]

On Mon, 30 Nov 2015 17:18:30 +0100
Milan Kubík  wrote:

> On 11/23/2015 04:42 PM, Filip Škola wrote:
> > Sending updated patch.
> >
> > F.
> >
> > On Mon, 23 Nov 2015 14:59:34 +0100
> > Filip Škola  wrote:
> >
> >> Found couple of issues (broke some dependencies).
> >>
> >> NACK
> >>
> >> F.
> >>
> >> On Fri, 20 Nov 2015 13:56:36 +0100
> >> Filip Škola  wrote:
> >>
> >>> Another one.
> >>>
> >>> F.
> >>
> >
> >
> 
> Hi, the tests look good. Few remarks, though.
> 
> 1. Please, use the shortes copyright notice in new modules.
> 
>  #
>  # Copyright (C) 2015  FreeIPA Contributors see COPYING for
> license #
> 
> 2. The tests `test_group_remove_group_from_protected_group` and 
> `test_group_full_set_of_objectclass_not_available_post_detach`
> were not ported. Please, include them in the patch.
> 
> Also, for less hassle, please rebase your patches on top of 
> freeipa-mkubik-0025-3-Separated-Tracker-implementations-into-standalone-pa.patch
> Which changes the location of tracker implementations and prevents 
> circular imports.
> 
> Thanks.
> 



Hi, 

these cases are there, in corresponding classes. They are marked with
the original comments. (However I can move them to separate class if
desirable.)

The copyright notice is changed. Also included a few changes in the
test with user without private group.

Filip
>From ff0b1fd07f15a076d5b370ff5299784b85e40af8 Mon Sep 17 00:00:00 2001
From: Filip Skola 
Date: Mon, 9 Nov 2015 16:48:55 +0100
Subject: [PATCH] Refactor test_group_plugin, use GroupTracker for tests

---
 ipatests/test_xmlrpc/test_group_plugin.py| 1728 +-
 ipatests/test_xmlrpc/tracker/group_plugin.py |  153 ++-
 2 files changed, 739 insertions(+), 1142 deletions(-)

diff --git a/ipatests/test_xmlrpc/test_group_plugin.py b/ipatests/test_xmlrpc/test_group_plugin.py
index f2bd0f4b9c8d517500b63cf49a8a7bc7c29aab6e..1ac278c7e224b8dd8793dc56c299dcae88aa78a3 100644
--- a/ipatests/test_xmlrpc/test_group_plugin.py
+++ b/ipatests/test_xmlrpc/test_group_plugin.py
@@ -1,6 +1,7 @@
 # Authors:
 #   Rob Crittenden 
 #   Pavel Zuna 
+#   Filip Skola 
 #
 # Copyright (C) 2008  Red Hat
 # see file 'COPYING' for use and warranty information
@@ -26,1137 +27,648 @@ import pytest
 
 from ipalib import api, errors
 from ipatests.test_xmlrpc import objectclasses
-from ipatests.test_xmlrpc.xmlrpc_test import (Declarative, fuzzy_digits, fuzzy_uuid, fuzzy_set_ci,
- add_sid, add_oc, XMLRPC_test, raises_exact)
+from ipatests.test_xmlrpc.xmlrpc_test import (
+fuzzy_digits, fuzzy_uuid, fuzzy_set_ci, add_sid, add_oc,
+XMLRPC_test, raises_exact
+)
 from ipapython.dn import DN
-from ipatests.test_xmlrpc.test_user_plugin import get_user_result
 
+from ipatests.test_xmlrpc.tracker.group_plugin import GroupTracker
 from ipatests.test_xmlrpc.tracker.user_plugin import UserTracker
-from ipatests.util import assert_deepequal
+from ipatests.test_xmlrpc.test_user_plugin import user, user_npg2
+from ipatests.util import assert_deepequal, get_group_dn
 
 
-group1 = u'testgroup1'
-group2 = u'testgroup2'
-group3 = u'testgroup3'
-renamedgroup1 = u'testgroup'
-user1 = u'tuser1'
+notagroup = u'notagroup'
+renamedgroup1 = u'renamedgroup'
+invalidgroup1 = u'+tgroup1'
+external_sid1 = u'S-1-1-123456-789-1'
 
-invalidgroup1=u'+tgroup1'
 
-# When adding external SID member to a group we can't test
-# it fully due to possibly missing Samba 4 python bindings
-# and/or not configured AD trusts. Thus, we'll use incorrect
-# SID value to merely test that proper exceptions are raised
-external_sid1=u'S-1-1-123456-789-1'
+@pytest.fixture(scope='class')
+def group(request):
+tracker = GroupTracker(name=u'testgroup1', description=u'Test desc1')
+return tracker.make_fixture(request)
 
-def get_group_dn(cn):
-return DN(('cn', cn), api.env.container_group, api.env.basedn)
+
+@pytest.fixture(scope='class')
+def group2(request):
+tracker = GroupTracker(name=u'testgroup2', description=u'Test desc2')
+return tracker.make_fixture(request)
+
+
+@pytest.fixture(scope='class')
+def managed_group(request, user):
+user.ensure_exists()
+tracker = GroupTracker(
+name=user.uid, description=u'User private group for %s' % user.uid
+)
+tracker.exists = True
+# Managed group gets created when user is created
+tracker.track_create()
+return tracker
+
+
+@pytest.fixture(scope='class')
+def admins(request):
+# Track the admins group
+tracker = GroupTracker(
+name=u'admins', description=u'Account administrators group'
+)
+tracker.exists = True
+tracker.track_create()
+tracker.attrs.update(member_user=[u'admin'])
+return tracker
+
+
+@pytest.fixture(scope='class')
+def trustadmins(request):
+# Track the 'trust admins' group
+tracker = GroupTracker(
+name=u'trust admins',