Re: [Freeipa-devel] [PATCH 0013-0021] Coverity patches

2016-02-01 Thread Jan Cholasta

Hi,

On 29.1.2016 15:49, Martin Basti wrote:



On 29.01.2016 15:49, Stanislav Laznicka wrote:

Reworded the commits so that they better reflect what's going on in those.

On 01/29/2016 02:49 PM, Stanislav Laznicka wrote:

Hello,

I made some patches based on the Coverity report from 18.1.2016.

Cheers,
Standa







NACK, see my previous email


I don't think this deserves 9 patches, 1 would be sufficient enough.


Patch 0013:

1) I think this unreachable return is intentional, as indicated by the 
comment:


-#we shouldn't get here
-return [UNKNOWN_ERROR]

2) How is this dead code?

-if options.mode == 'validate_pot' or options.mode == 'validate_po':
+if options.mode in ('validate_pot', 'validate_po'):

-elif options.mode == 'create_test' or 'test_gettext':
+elif options.mode in ('create_test', 'test_gettext'):



Patch 0014-0015: LGTM


Patch 0016: The original code is in fact correct.


Patch 0017: This will break Python 3. The two branches are performing 
the same action, but with different data types.



Patch 0018: LGTM


Patch 0019: IMO the original code is better (None has a __class__ too, 
you know).



Patch 0020: LGTM


Patch 0021: Please use the original error messages (there are no 
requests being added to D-Bus, but to certmonger).



Honza

--
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 0411] upgrade: log to ipaupgrade.log if ipa is not installed

2016-02-01 Thread Martin Basti



On 29.01.2016 12:12, Martin Kosek wrote:

On 01/29/2016 10:48 AM, Martin Basti wrote:

Missing record in ipaupgrade.log that upgrade failed because IPA is not
installed, causes harder time to debugging upgrade from log.

Patch attached.

I am thinking that in these general catch-all clauses, it could be also useful
to print the stack trace in the DEBUG log, especially for the cases when the
exception is re-raised to some general one, like here:

 try:
 data_upgrade.create_instance()
...
 except RuntimeError:
 raise RuntimeError('IPA upgrade failed.', 1)


It is already logged to DEBUG log in data_upgrade instance.

--
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] [TEST][Patch 0021] Fixed recent replica installation issues in the lab

2016-02-01 Thread Oleg Fayans
Hi Petr,

Please find the new version of the patch attached. Comments are inline

On 01/29/2016 11:58 AM, Petr Spacek wrote:
> On 27.1.2016 11:16, Oleg Fayans wrote:
>> Sorry, trailing whitespace detected. This version passes lint
>>
>> On 01/27/2016 09:23 AM, Oleg Fayans wrote:
 Hi,

 On 01/21/2016 04:41 PM, Petr Spacek wrote:
>> Hello,
>>
>> On 21.1.2016 13:42, Oleg Fayans wrote:
 freeipa-ofayans-0021-Removed-ip-address-option-from-replica-installation.patch


 From d7ab06a4dcddb919fda351b983d478f1b6968578 Mon Sep 17 00:00:00 2001
 From: Oleg Fayans 
 Date: Thu, 21 Jan 2016 13:30:02 +0100
 Subject: [PATCH] Removed --ip-address option from replica installation

 Explicitly specifying ip-address of the replica messes up with the 
 current
 bind-dyndb-ldap logic, causing reverse zone not to be created.

 Enabled reverse-zone creation for the clients residing in different 
 subnet from
 master
 ---
  ipatests/test_integration/tasks.py | 19 ---
  1 file changed, 12 insertions(+), 7 deletions(-)

 diff --git a/ipatests/test_integration/tasks.py 
 b/ipatests/test_integration/tasks.py
 index 
 6eb55501389c72b4c7aaa599fd4852d7e8f1f3c2..43ef78b0c55deed24a0444f0ac6c38ddb2517481
  100644
 --- a/ipatests/test_integration/tasks.py
 +++ b/ipatests/test_integration/tasks.py
 @@ -69,6 +69,8 @@ def prepare_reverse_zone(host, ip):
  host.run_command(["ipa",
"dnszone-add",
zone], raiseonerr=False)
 +return zone
 +
  
  def prepare_host(host):
  if isinstance(host, Host):
 @@ -319,11 +321,8 @@ def domainlevel(host):
  def replica_prepare(master, replica):
  apply_common_fixes(replica)
  fix_apache_semaphores(replica)
 -prepare_reverse_zone(master, replica.ip)
 -master.run_command(['ipa-replica-prepare',
 -'-p', replica.config.dirman_password,
 -'--ip-address', replica.ip,
 -replica.hostname])
 +master.run_command(['ipa-replica-prepare', '-p', 
 replica.config.dirman_password,
 +'--auto-reverse', replica.hostname])
>>
>> I guess that you will need --ip-address option in cases where master's 
>> reverse
>> record does not exist (yet).
 And yo were right. Fixed

>>
>> I would recommend you to test this in libvirt or somewhere without revere
>> records, I suspect that it might blow up.
>>
  replica_bundle = master.get_file_contents(
  paths.REPLICA_INFO_GPG_TEMPLATE % replica.hostname)
  replica_filename = get_replica_filename(replica)
 @@ -339,8 +338,7 @@ def install_replica(master, replica, 
 setup_ca=True, setup_dns=False,
  # and replica installation would fail
  args = ['ipa-replica-install', '-U',
  '-p', replica.config.dirman_password,
 -'-w', replica.config.admin_password,
 -'--ip-address', replica.ip]
 +'-w', replica.config.admin_password]
  if setup_ca:
  args.append('--setup-ca')
  if setup_dns:
 @@ -380,6 +378,13 @@ def install_client(master, client, extra_args=()):
  client.collect_log(paths.IPACLIENT_INSTALL_LOG)
  
  apply_common_fixes(client)
 +# Now, for the situations where a client resides in a different 
 subnet from
 +# master, we need to explicitly tell master to create a reverse 
 zone for
 +# the client and enable dynamic updates for this zone.
 +allow_sync_ptr(master)
 +zone = prepare_reverse_zone(master, client.ip)
 +master.run_command(["ipa", "dnszone-mod", zone,
 +"--dynamic-update=TRUE"], raiseonerr=False)
>>
>> I'm not a big fan of ignoring exceptions here, it might be better to
>> encapsulate the first command with try: except: and run the zone-mod 
>> only if
>> the add worked as expected.
>>
>> Also, logging an message that reverse zone was not added might be a good 
>> idea.
 Agreed. Done.

>>
>> HTH
>>
>> Petr^2 Spacek
>>
>>
  
  client.run_command(['ipa-client-install', '-U',
  '--domain', client.domain.name,
>>



>> -- Oleg Fayans Quality Engineer FreeIPA team RedHat.
>>
>>
>> freeipa-ofayans-0021.2-Removed-ip-address-option-from-replica-installation.patch
>>
>>
>> 

[Freeipa-devel] [PATCH 0132] always start certmonger during IPA server configuration upgrade

2016-02-01 Thread Martin Babinsky

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

--
Martin^3 Babinsky
From bfa239bd1ac71f89a54f8be548432b89b2f527dd Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Mon, 1 Feb 2016 12:59:04 +0100
Subject: [PATCH] always start certmonger during IPA server configuration
 upgrade

This patch fixes a regression introduced by commit
bef0f4c5c38e7ff6415e8f8c96dc306ef7f0ce56. Instead of checking whether
there is CA installed in the topology, we should always start certmonger
service during upgrade regardless when CA was configured.

https://fedorahosted.org/freeipa/ticket/5655
---
 ipaserver/install/server/upgrade.py | 33 +
 1 file changed, 5 insertions(+), 28 deletions(-)

diff --git a/ipaserver/install/server/upgrade.py b/ipaserver/install/server/upgrade.py
index 20379f19c652cb0b5911a4c2f1c67eae7f763379..48f8579a4bf49c9e225733facb06fcc001dbdb32 100644
--- a/ipaserver/install/server/upgrade.py
+++ b/ipaserver/install/server/upgrade.py
@@ -291,24 +291,6 @@ def setup_firefox_extension(fstore):
 http.setup_firefox_extension(realm, domain)
 
 
-def is_ca_enabled():
-"""
-check whether there is an active CA master
-:return: True if there is an active CA in topology, False otherwise
-"""
-ldap2 = api.Backend.ldap2
-was_connected = ldap2.isconnected()
-
-if not was_connected:
-ldap2.connect()
-
-try:
-return api.Command.ca_is_enabled()['result']
-finally:
-if not was_connected:
-ldap2.disconnect()
-
-
 def ca_configure_profiles_acl(ca):
 root_logger.info('[Authorizing RA Agent to modify profiles]')
 
@@ -1481,6 +1463,10 @@ def upgrade_configuration():
 )
 upgrade_pki(ca, fstore)
 
+certmonger_service = services.knownservices.certmonger
+if ca.is_configured() and not certmonger_service.is_running():
+certmonger_service.start()
+
 ca.configure_certmonger_renewal_guard()
 
 update_dbmodules(api.env.realm)
@@ -1496,8 +1482,7 @@ def upgrade_configuration():
 http.configure_selinux_for_httpd()
 http.change_mod_nss_port_from_http()
 
-if is_ca_enabled():
-http.configure_certmonger_renewal_guard()
+http.configure_certmonger_renewal_guard()
 
 http.enable_and_start_oddjobd()
 
@@ -1650,14 +1635,6 @@ def upgrade_check(options):
 print(unicode(e))
 sys.exit(1)
 
-try:
-ca_is_enabled = is_ca_enabled()
-except Exception as e:
-raise RuntimeError("Cannot connect to LDAP server: {0}".format(e))
-
-if not services.knownservices.certmonger.is_running() and ca_is_enabled:
-raise RuntimeError('Certmonger is not running. Start certmonger and run upgrade again.')
-
 if not options.skip_version_check:
 # check IPA version and data version
 try:
-- 
2.5.0

From d10dbd7c5bbcc7987c66e6ea0c15a658bf9d609c Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Mon, 1 Feb 2016 12:59:04 +0100
Subject: [PATCH] always start certmonger during IPA server configuration
 upgrade

This patch fixes a regression introduced by commit
bef0f4c5c38e7ff6415e8f8c96dc306ef7f0ce56. Instead of checking whether
there is CA installed in the topology, we should always start certmonger
service during upgrade regardless when CA was configured.

https://fedorahosted.org/freeipa/ticket/5655
---
 ipaserver/install/server/upgrade.py | 33 +
 1 file changed, 5 insertions(+), 28 deletions(-)

diff --git a/ipaserver/install/server/upgrade.py b/ipaserver/install/server/upgrade.py
index 2d971964480e2f7829591f194a21ed6d23eccdd2..0a46635979497f8028465c2295b22485fd9c0279 100644
--- a/ipaserver/install/server/upgrade.py
+++ b/ipaserver/install/server/upgrade.py
@@ -292,24 +292,6 @@ def setup_firefox_extension(fstore):
 http.setup_firefox_extension(realm, domain)
 
 
-def is_ca_enabled():
-"""
-check whether there is an active CA master
-:return: True if there is an active CA in topology, False otherwise
-"""
-ldap2 = api.Backend.ldap2
-was_connected = ldap2.isconnected()
-
-if not was_connected:
-ldap2.connect()
-
-try:
-return api.Command.ca_is_enabled()['result']
-finally:
-if not was_connected:
-ldap2.disconnect()
-
-
 def ca_configure_profiles_acl(ca):
 root_logger.info('[Authorizing RA Agent to modify profiles]')
 
@@ -1420,6 +1402,10 @@ def upgrade_configuration():
 )
 upgrade_pki(ca, fstore)
 
+certmonger_service = services.knownservices.certmonger
+if ca.is_configured() and not certmonger_service.is_running():
+certmonger_service.start()
+
 ca.configure_certmonger_renewal_guard()
 
 update_dbmodules(api.env.realm)
@@ -1435,8 +1421,7 @@ def upgrade_configuration():
 http.configure_selinux_for_httpd()
 http.change_mod_nss_port_from_http()
 
-if is_ca_enabled():
-http.configure_certmonger_renewal_guard()
+

Re: [Freeipa-devel] [PATCH 0132] always start certmonger during IPA server configuration upgrade

2016-02-01 Thread Martin Basti



On 01.02.2016 13:55, Martin Babinsky wrote:

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





LGTM, works for me, tested on both ca-less server and CA-full server.

Because patch is touching certmonger I would like to get final ACK from 
Honza.
-- 
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 0013-0021] Coverity patches

2016-02-01 Thread Jan Cholasta

On 1.2.2016 12:11, Petr Spacek wrote:

On 1.2.2016 09:03, Jan Cholasta wrote:

Hi,

On 29.1.2016 15:49, Martin Basti wrote:



On 29.01.2016 15:49, Stanislav Laznicka wrote:

Reworded the commits so that they better reflect what's going on in those.

On 01/29/2016 02:49 PM, Stanislav Laznicka wrote:

Hello,

I made some patches based on the Coverity report from 18.1.2016.

Cheers,
Standa







NACK, see my previous email


I don't think this deserves 9 patches, 1 would be sufficient enough.


I would rather have it is separate patches as these fixes are largely not
related. It will make bisecting easier.


Most of the fixes are cosmetic, which makes them related, and the rest 
are small isolated changes, so in reality it would hardly make bisecting 
easier and only increase the overhead. In the past we usually had put 
such fixes into a single patch and AFAIK nobody complained so far.






Patch 0013:

1) I think this unreachable return is intentional, as indicated by the comment:

-#we shouldn't get here
-return [UNKNOWN_ERROR]


I would use
assert False, "we shouldn't get here"
neither we nor Coverity are confused when we hit the code path one day.

UNKNOWN_ERROR would pop up somewhere else and it will be harder to find out
why the hell the code behaves as mad. Traceback will clearly indicate that
there is a problem with the 'switch'.


Sure, my point is that returning None is no better than returning 
UNKNOWN_ERROR.




Petr^2 Spacek


2) How is this dead code?

-if options.mode == 'validate_pot' or options.mode == 'validate_po':
+if options.mode in ('validate_pot', 'validate_po'):

-elif options.mode == 'create_test' or 'test_gettext':
+elif options.mode in ('create_test', 'test_gettext'):



Patch 0014-0015: LGTM


Actually scratch that, patch 0015 breaks correct code.




Patch 0016: The original code is in fact correct.


Patch 0017: This will break Python 3. The two branches are performing the same
action, but with different data types.


Patch 0018: LGTM


Patch 0019: IMO the original code is better (None has a __class__ too, you 
know).


Patch 0020: LGTM


Patch 0021: Please use the original error messages (there are no requests
being added to D-Bus, but to certmonger).


Honza





--
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 542] replica install: validate DS and HTTP server certificates

2016-02-01 Thread Martin Basti



On 27.01.2016 09:10, Martin Babinsky wrote:

On 01/25/2016 08:29 AM, Jan Cholasta wrote:

Hi,

the attached patch fixes .

Honza




You may need to rebase the patch on top of ipa-4-2, otherwise ACK.


Pushed to:
master: 465ce82a4d098c4c419913f30a1a028afc7ae445
ipa-4-3: 15357aea39eb9e496439e4ef711b97616ef7ee9a
ipa-4-2: c2ade68df88e440cd969bede298f0c1feae59fcc

--
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 0126-0127] reset openldap client config to point to freshly promote replica

2016-02-01 Thread Martin Basti



On 29.01.2016 18:06, Martin Basti wrote:



On 29.01.2016 09:01, Martin Babinsky wrote:

On 01/20/2016 09:40 AM, Martin Babinsky wrote:

On 01/14/2016 05:29 PM, Martin Babinsky wrote:

On 01/13/2016 05:59 PM, Rob Crittenden wrote:

Martin Babinsky wrote:

fixes https://fedorahosted.org/freeipa/ticket/5584

In order to ensure consistent behavior with ipa-client-install, I 
opted

to reuse the configure_openldap_conf() function and restoring the
config
from client sysrestore before modifying it.

If you think this approach is not optimal please propose an 
alternative

solution.


You could also just do an action set on URI to change the value, 
right?

It would need a new function but it would be very small.

If you do end up keeping this I'd want a new commit message for 
moving
the code to include why you're moving it (to avoid the need to 
deference

the ticket).

rob



Here's the patch that implements the change in URI directive. Please
keep in mind that we not only have to change the URI to point to
ourselves, we also have to do it in a way consistent with
ipa-client-install, i.e. leave a comment with new URI if it was 
already

set by third party.

Plain 'addifnotset' directive will not do, however, because then we 
end
up with two comments, one original, and one pointing to ourselves. 
Plain
'set' may rewrite the URI set by user and thus we would have to 
test its

value anyway.

The correct handling of these cases coupled with a way 
IPAChangeConf is

written results in a solution presented here.

The fact that it is not much shorter than configure_openldap_conf 
and is

additionally pretty ugly (a fact at least partially caused by me not
being very fluent in IPAChangeConf usage) led me to the conclusion 
that

restoring original ldap.conf and reusing already wirrten code for
reediting it anew with replica as URI is actually not that bad idea.





Bump for review/discussion.


Another bump.


Works for me, ACK


Pushed to:
master: 23f5edb4be08b359c6acd8a18a5e23c3dd784136
ipa-4-3: c61bc48de6a75a948adad2032bd69d96007be444

--
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 0407] make lint: migration to config file and pylint plugin due pylint 1.5.2

2016-02-01 Thread Martin Basti



On 26.01.2016 14:16, Martin Basti wrote:



On 20.01.2016 14:38, Jan Cholasta wrote:

Hi,

On 19.1.2016 13:43, Martin Basti wrote:

New pylint version will broke our custom make-lint script again,
attached patch migrates make-lint to:
* config file
* pylint plugin
which are supported by pylint and should not have regular compatibility
issues

to test new approach run ./make-lint2

Advantages:
* compatibility with pylint
* works on both pylint-1.4.3-3.fc23.noarch and 
pylint-1.5.2-1.fc24.noarch
* pylint plugin works in different way than the previous custom 
checker.
Missing ("dynamic") attributes are added to abstract syntax tree 
instead

of ignoring them and all their sub-members. This makes check better,
pylint can detect more typos in tests configurations, api, env, etc..

Disadvantages:
* any new attribute in api, test config, etc.. must be added to
definition of missing members (pylint plugin) - this should not happen
too often


1) Please "mv pylint_plugins/fix_ipa_members.py pylint_plugins.py" 
and "rm -rf pylint_plugins/", no need for this redundant directory 
structure.


2) Rename pylintrc to freeipa.pylintrc so you have to always specify 
it explicitly with --rcfile.


3) Use the load-plugins directive in freeipa.pylintrc to load the 
plugins rather than --load-plugins.


4) Instead of running pylint twice, run it only once with both normal 
and Python 3 checks enabled:


[MESSAGE CONTROL]
enable=all,python3
disable=...,no-absolute-import




Q:
* make-lint: should it be just bash script or rather python script?


IMO neither, it should be a make target (make lint).


* add dynamic detection of python files to be checked


You can use "find . -type f -executable ! -path \*/.\* ! -name 
\*.py\* -exec grep -lsm1 '^#!.*\bpython' \{\} \;".



* should I keep the current options from original make-lint?


No, but allow pylint options to be overridable (make lint 
PYLINTFLAGS="--disable=python3")



* several false positive errors I haven't been able to fix in plugin
yet, in worst case they can be locally disabled:


Disable them locally.

Honza


Updated patch attached.

Please note that make-lint script has been removed, to execute lint 
check use 'make lint'





Updated patch attached:
* fixes recently added error
* fixes PEP8
* cleanup of pylint config file

Patch is only for master branch, for 4.3 and 4.2 I will send different 
patches when this will be acked
From 02464278b9dbd93ad9bc69ed63ebf26b4e662d3e Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Fri, 15 Jan 2016 16:58:38 +0100
Subject: [PATCH] make lint: use config file and plugin for pylint

Our custom implementation of pylint checker is often broken by
incompatible change on pylint side. Using supported solutions (config
file, pylint plugins) should avoid this issue.

The plugin adds missing (dynamic) member to classes in abstract syntax
tree generated for pylint, instead of just ignoring missing members and
all sub-members. This should improve pylint detection of typos and
missing members in api. env and test config.

make-lint python script has been removed, to run pylint execute 'make
lint'

https://fedorahosted.org/freeipa/ticket/5615
---
 Makefile|  13 +-
 ipalib/plugins/vault.py |   4 +
 ipatests/test_ipapython/test_ipautil.py |   2 +
 make-lint   | 351 
 pylint_plugins.py   | 210 +++
 pylintrc|  85 
 6 files changed, 311 insertions(+), 354 deletions(-)
 delete mode 100755 make-lint
 create mode 100644 pylint_plugins.py
 create mode 100644 pylintrc

diff --git a/Makefile b/Makefile
index d2857ae4744188532e360a5f2375a17d3acb7d4a..44a50176ad215b6c81e42ad72e6cca4da76cb78e 100644
--- a/Makefile
+++ b/Makefile
@@ -54,7 +54,9 @@ LIBDIR ?= /usr/lib
 
 DEVELOPER_MODE ?= 0
 ifneq ($(DEVELOPER_MODE),0)
-LINT_OPTIONS=--no-fail
+LINT_IGNORE_FAIL=true
+else
+LINT_IGNORE_FAIL=false
 endif
 
 PYTHON ?= $(shell rpm -E %__python || echo /usr/bin/python2)
@@ -127,8 +129,13 @@ client-dirs:
 	fi
 
 lint: bootstrap-autogen
-	./make-lint $(LINT_OPTIONS)
-	$(MAKE) -C install/po validate-src-strings
+	FILES=`find . \
+		-type d -exec test -e '{}/__init__.py' \; -print -prune -o \
+		-name \*.py -print -o \
+		-type f \! -path '*/.*' \! -name '*~' -exec grep -qsm1 '^#!.*\bpython' '{}' \; -print`; \
+	echo "Pylint is running, please wait ..."; \
+	PYTHONPATH=. pylint --rcfile=pylintrc $(PYLINTFLAGS) $$FILES || $(LINT_IGNORE_FAIL)
+	$(MAKE) -C install/po validate-src-strings || $(LINT_IGNORE_FAIL)
 
 
 test:
diff --git a/ipalib/plugins/vault.py b/ipalib/plugins/vault.py
index 4d8419e75770dc4c8b856560cf6c1613a132f8c0..f495aa1cbe80c07c34db64a61695ca0cafa88263 100644
--- a/ipalib/plugins/vault.py
+++ b/ipalib/plugins/vault.py
@@ -1651,7 +1651,9 @@ class vault_archive(PKQuery, Local):
 session_key = slot.key_gen(mechanism, None, 

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

2016-02-01 Thread Martin Basti



On 14.01.2016 10:01, Alexander Bokovoy wrote:

On Thu, 14 Jan 2016, Martin Basti wrote:



On 14.01.2016 08:24, 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

This patchset seems to fall through cracks -- it was ACKed but not
committed.
IIRC all simo's ACKed patches which haven't been pushed depend on 
simo's patch 560, which has no ACK


If not then, patches need rebase, they have missing blobs

no, 560 is unrelated.


Pushed to:
master: f9ed0b6ff8bf7e59de5450200d9fc5ad6e05299c
ipa-4-3: 7e09456d8b80eabb87bb2cf595904b5cc740af8e

--
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] [TEST][Patch 0023] Updated connect/disconnect replica to work on both domain levels

2016-02-01 Thread Oleg Fayans

-- 
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
From 6842d2c1d068853f16f42df893112cb1e8d716ff Mon Sep 17 00:00:00 2001
From: Oleg Fayans 
Date: Mon, 1 Feb 2016 11:34:11 +0100
Subject: [PATCH] Updated connect/disconnect replica to work with both
 domainlevels

---
 ipatests/test_integration/tasks.py | 29 +++--
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index 27d2449ec71e0bf55a576cc495175a5ae41da1d6..318c8c880fe0df22c9b5089cbd88e017936e4df5 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -604,14 +604,31 @@ def sync_time(host, server):
 host.run_command(['ntpdate', server.hostname])
 
 
-def connect_replica(master, replica):
-kinit_admin(replica)
-replica.run_command(['ipa-replica-manage', 'connect', master.hostname])
+def connect_replica(master, replica, domain_level=None):
+if domain_level is None:
+domain_level = master.config.domain_level
+if domain_level == DOMAIN_LEVEL_0:
+replica.run_command(['ipa-replica-manage', 'connect', master.hostname])
+else:
+kinit_admin(master)
+master.run_command(["ipa", "topologysegment-add", DOMAIN_SUFFIX_NAME,
+"%s-to-%s" % (master.hostname, replica.hostname),
+"--leftnode=%s" % master.hostname,
+"--rightnode=%s" % replica.hostname
+])
 
 
-def disconnect_replica(master, replica):
-kinit_admin(replica)
-replica.run_command(['ipa-replica-manage', 'disconnect', master.hostname])
+def disconnect_replica(master, replica, domain_level=None):
+if domain_level is None:
+domain_level = master.config.domain_level
+if domain_level == DOMAIN_LEVEL_0:
+replica.run_command(['ipa-replica-manage', 'disconnect', master.hostname])
+else:
+kinit_admin(master)
+master.run_command(["ipa", "topologysegment-del", DOMAIN_SUFFIX_NAME,
+"%s-to-%s" % (master.hostname, replica.hostname),
+"--continue"
+])
 
 
 def kinit_admin(host):
-- 
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 0013-0021] Coverity patches

2016-02-01 Thread Petr Spacek
On 1.2.2016 09:03, Jan Cholasta wrote:
> Hi,
> 
> On 29.1.2016 15:49, Martin Basti wrote:
>>
>>
>> On 29.01.2016 15:49, Stanislav Laznicka wrote:
>>> Reworded the commits so that they better reflect what's going on in those.
>>>
>>> On 01/29/2016 02:49 PM, Stanislav Laznicka wrote:
 Hello,

 I made some patches based on the Coverity report from 18.1.2016.

 Cheers,
 Standa


>>>
>>>
>>>
>> NACK, see my previous email
> 
> I don't think this deserves 9 patches, 1 would be sufficient enough.

I would rather have it is separate patches as these fixes are largely not
related. It will make bisecting easier.


> Patch 0013:
> 
> 1) I think this unreachable return is intentional, as indicated by the 
> comment:
> 
> -#we shouldn't get here
> -return [UNKNOWN_ERROR]

I would use
assert False, "we shouldn't get here"
neither we nor Coverity are confused when we hit the code path one day.

UNKNOWN_ERROR would pop up somewhere else and it will be harder to find out
why the hell the code behaves as mad. Traceback will clearly indicate that
there is a problem with the 'switch'.

Petr^2 Spacek

> 2) How is this dead code?
> 
> -if options.mode == 'validate_pot' or options.mode == 'validate_po':
> +if options.mode in ('validate_pot', 'validate_po'):
> 
> -elif options.mode == 'create_test' or 'test_gettext':
> +elif options.mode in ('create_test', 'test_gettext'):
> 
> 
> 
> Patch 0014-0015: LGTM
> 
> 
> Patch 0016: The original code is in fact correct.
> 
> 
> Patch 0017: This will break Python 3. The two branches are performing the same
> action, but with different data types.
> 
> 
> Patch 0018: LGTM
> 
> 
> Patch 0019: IMO the original code is better (None has a __class__ too, you 
> know).
> 
> 
> Patch 0020: LGTM
> 
> 
> Patch 0021: Please use the original error messages (there are no requests
> being added to D-Bus, but to certmonger).
> 
> 
> Honza

-- 
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] [TEST][Patch 0021] Fixed recent replica installation issues in the lab

2016-02-01 Thread Petr Spacek
On 1.2.2016 11:52, Oleg Fayans wrote:
> Hi Petr,
> 
> Please find the new version of the patch attached. Comments are inline
> 
> On 01/29/2016 11:58 AM, Petr Spacek wrote:
>> On 27.1.2016 11:16, Oleg Fayans wrote:
>>> Sorry, trailing whitespace detected. This version passes lint
>>>
>>> On 01/27/2016 09:23 AM, Oleg Fayans wrote:
> Hi,
>
> On 01/21/2016 04:41 PM, Petr Spacek wrote:
>>> Hello,
>>>
>>> On 21.1.2016 13:42, Oleg Fayans wrote:
> freeipa-ofayans-0021-Removed-ip-address-option-from-replica-installation.patch
>
>
> From d7ab06a4dcddb919fda351b983d478f1b6968578 Mon Sep 17 00:00:00 2001
> From: Oleg Fayans 
> Date: Thu, 21 Jan 2016 13:30:02 +0100
> Subject: [PATCH] Removed --ip-address option from replica installation
>
> Explicitly specifying ip-address of the replica messes up with the 
> current
> bind-dyndb-ldap logic, causing reverse zone not to be created.
>
> Enabled reverse-zone creation for the clients residing in different 
> subnet from
> master
> ---
>  ipatests/test_integration/tasks.py | 19 ---
>  1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/ipatests/test_integration/tasks.py 
> b/ipatests/test_integration/tasks.py
> index 
> 6eb55501389c72b4c7aaa599fd4852d7e8f1f3c2..43ef78b0c55deed24a0444f0ac6c38ddb2517481
>  100644
> --- a/ipatests/test_integration/tasks.py
> +++ b/ipatests/test_integration/tasks.py
> @@ -69,6 +69,8 @@ def prepare_reverse_zone(host, ip):
>  host.run_command(["ipa",
>"dnszone-add",
>zone], raiseonerr=False)
> +return zone
> +
>  
>  def prepare_host(host):
>  if isinstance(host, Host):
> @@ -319,11 +321,8 @@ def domainlevel(host):
>  def replica_prepare(master, replica):
>  apply_common_fixes(replica)
>  fix_apache_semaphores(replica)
> -prepare_reverse_zone(master, replica.ip)
> -master.run_command(['ipa-replica-prepare',
> -'-p', replica.config.dirman_password,
> -'--ip-address', replica.ip,
> -replica.hostname])
> +master.run_command(['ipa-replica-prepare', '-p', 
> replica.config.dirman_password,
> +'--auto-reverse', replica.hostname])
>>>
>>> I guess that you will need --ip-address option in cases where master's 
>>> reverse
>>> record does not exist (yet).
> And yo were right. Fixed
>
>>>
>>> I would recommend you to test this in libvirt or somewhere without 
>>> revere
>>> records, I suspect that it might blow up.
>>>
>  replica_bundle = master.get_file_contents(
>  paths.REPLICA_INFO_GPG_TEMPLATE % replica.hostname)
>  replica_filename = get_replica_filename(replica)
> @@ -339,8 +338,7 @@ def install_replica(master, replica, 
> setup_ca=True, setup_dns=False,
>  # and replica installation would fail
>  args = ['ipa-replica-install', '-U',
>  '-p', replica.config.dirman_password,
> -'-w', replica.config.admin_password,
> -'--ip-address', replica.ip]
> +'-w', replica.config.admin_password]
>  if setup_ca:
>  args.append('--setup-ca')
>  if setup_dns:
> @@ -380,6 +378,13 @@ def install_client(master, client, 
> extra_args=()):
>  client.collect_log(paths.IPACLIENT_INSTALL_LOG)
>  
>  apply_common_fixes(client)
> +# Now, for the situations where a client resides in a different 
> subnet from
> +# master, we need to explicitly tell master to create a reverse 
> zone for
> +# the client and enable dynamic updates for this zone.
> +allow_sync_ptr(master)
> +zone = prepare_reverse_zone(master, client.ip)
> +master.run_command(["ipa", "dnszone-mod", zone,
> +"--dynamic-update=TRUE"], raiseonerr=False)
>>>
>>> I'm not a big fan of ignoring exceptions here, it might be better to
>>> encapsulate the first command with try: except: and run the zone-mod 
>>> only if
>>> the add worked as expected.
>>>
>>> Also, logging an message that reverse zone was not added might be a 
>>> good idea.
> Agreed. Done.
>
>>>
>>> HTH
>>>
>>> Petr^2 Spacek
>>>
>>>
>  
>  client.run_command(['ipa-client-install', '-U',
>  '--domain', client.domain.name,

Re: [Freeipa-devel] [TEST][Patch 0023] Updated connect/disconnect replica to work on both domain levels

2016-02-01 Thread Martin Basti



On 01.02.2016 12:21, Oleg Fayans wrote:




ACK

Pushed to:
master: aa30199e0b6002aeb8c01e2561a3d55fe3f1ceb5
ipa-4-3: a8775de8aaca56a2f589d5aebe957f78ffc66c16

-- 
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 154] ipa-kdb: map_groups() consider all results

2016-02-01 Thread Jakub Hrozek
On Tue, Jan 05, 2016 at 07:55:33PM +0100, Sumit Bose wrote:
> Hi,
> 
> to find out to which local group a external user is mapped we do a
> dereference search over the external groups with the SIDs related to the
> external user. If a SID is mapped to more than one external group we
> currently consider only the first returned match. With this patch all
> results are taken into account. This makes sure all expected local group
> memberships are added to the PAC which resolves
> https://fedorahosted.org/freeipa/ticket/5573.

I tested with an AD user who was a member of several IPA external groups. All
groups were displayed.  We also have positive feedback from several users
who applied this patch.

The code looks good to me as well, Sumit explained some parts I didn't
understand on IRC.

ACK from me..

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