Re: [Freeipa-devel] [PATCH 0405] idviews: Add user certificate attribute to user ID overrides

2016-04-13 Thread Tomas Babej
On 04/07/2016 01:53 PM, Sumit Bose wrote:
> On Mon, Apr 04, 2016 at 04:27:02PM +0200, Jan Cholasta wrote:
>> Hi,
>>
>> On 1.4.2016 16:53, Tomas Babej wrote:
>>> Hi,
>>>
>>> this extends the user ID overrides with capability to store the user
>>> certificate.
>>>
>>> https://fedorahosted.org/freeipa/ticket/4955
>>
>> The preferred way of managing certificates nowadays is using $OBJ-add-cert
>> and $OBJ-remove-cert commands, you should add them here as well.
>>
>> I would even go as far as not allowing to modify certificates using
>> idoverrideuser-mod - in user-mod and host-mod, it's there just for backward
>> compatibility, which is not the case here. But I don't have a strong opinion
>> on that.
>>
>> For consistency with user-find and host-find, the full certificate blob
>> should not be shown in idoverrideuser-find. You can do that by setting
>> search_display_attributes attribute on the idoverrideuser class
>> appropriately.
> 
> I tested the current patch with my related patches for SSSD and all is
> working as expected.
> 
> bye,
> Sumit
> 
>>
>> 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
> 

Thanks for the reviews,

attaching a updated patch that addresses Honza's comments.

Tomas
From bc7a20b942931e43b4d7e4e79b88cae8a113385d Mon Sep 17 00:00:00 2001
From: Tomas Babej 
Date: Thu, 3 Mar 2016 15:14:10 +0100
Subject: [PATCH] idviews: Add user certificate attribute to user ID overrides

---
 ACI.txt  |  2 +-
 API.txt  | 30 +++--
 install/share/71idviews.ldif |  2 +-
 ipalib/plugins/idviews.py| 80 ++--
 4 files changed, 108 insertions(+), 6 deletions(-)

diff --git a/ACI.txt b/ACI.txt
index 24cb332ce6e10c82a5bfab76d084fb6c0277800d..ae00cf7a1b8e2ea0e33798993bb24dc5f06127e3 100644
--- a/ACI.txt
+++ b/ACI.txt
@@ -149,7 +149,7 @@ aci: (targetfilter = "(objectclass=ipahostgroup)")(version 3.0;acl "permission:S
 dn: cn=views,cn=accounts,dc=ipa,dc=example
 aci: (targetattr = "cn || createtimestamp || description || entryusn || gidnumber || ipaanchoruuid || modifytimestamp || objectclass")(targetfilter = "(objectclass=ipaGroupOverride)")(version 3.0;acl "permission:System: Read Group ID Overrides";allow (compare,read,search) userdn = "ldap:///all";;)
 dn: cn=views,cn=accounts,dc=ipa,dc=example
-aci: (targetattr = "createtimestamp || description || entryusn || gecos || gidnumber || homedirectory || ipaanchoruuid || ipaoriginaluid || ipasshpubkey || loginshell || modifytimestamp || objectclass || uid || uidnumber")(targetfilter = "(objectclass=ipaUserOverride)")(version 3.0;acl "permission:System: Read User ID Overrides";allow (compare,read,search) userdn = "ldap:///all";;)
+aci: (targetattr = "createtimestamp || description || entryusn || gecos || gidnumber || homedirectory || ipaanchoruuid || ipaoriginaluid || ipasshpubkey || loginshell || modifytimestamp || objectclass || uid || uidnumber || usercertificate")(targetfilter = "(objectclass=ipaUserOverride)")(version 3.0;acl "permission:System: Read User ID Overrides";allow (compare,read,search) userdn = "ldap:///all";;)
 dn: cn=ranges,cn=etc,dc=ipa,dc=example
 aci: (targetattr = "cn || createtimestamp || entryusn || ipabaseid || ipabaserid || ipaidrangesize || ipanttrusteddomainsid || iparangetype || ipasecondarybaserid || modifytimestamp || objectclass")(targetfilter = "(objectclass=ipaidrange)")(version 3.0;acl "permission:System: Read ID Ranges";allow (compare,read,search) userdn = "ldap:///all";;)
 dn: cn=views,cn=accounts,dc=ipa,dc=example
diff --git a/API.txt b/API.txt
index 5b75413f930d0e9caaffc68023bed8106d786653..76b260da72533ee88027f72d56a591c7566c72b7 100644
--- a/API.txt
+++ b/API.txt
@@ -2429,7 +2429,7 @@ output: Entry('result', , Gettext('A dictionary representing an LDA
 output: Output('summary', (, ), None)
 output: PrimaryKey('value', None, None)
 command: idoverrideuser_add
-args: 2,15,3
+args: 2,16,3
 arg: Str('idviewcn', cli_name='idview', multivalue=False, primary_key=True, query=True, required=True)
 arg: Str('ipaanchoruuid', attribute=True, cli_name='anchor', multivalue=False, primary_key=True, required=True)
 option: Str('addattr*', cli_name='addattr', exclude='webui')
@@ -2446,6 +2446,19 @@ option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui
 option: Str('setattr*', cli_name='setattr', exclude='webui')
 option: Str('uid', attribute=True, cli_name='login', maxlength=255, multivalue=False, pattern='^[a-zA-Z0-9_.][a-zA-Z0-9_.-]{0,252}[a-zA-Z0-9_.$-]?$', required=False)
 option: Int('uidnumber', attribute=True, cli_name='uid', minvalue=1, multivalue=False, required=False)
+option: Bytes('usercertificate', attribute=True, cli_name='certificate', multivalue=True, required=False)
+option: Str('version?', exclude='webui')
+output: Entry(

Re: [Freeipa-devel] [TEST][patch-0035] Test replica installed under domain level 0 is functional after domain upgrade

2016-04-13 Thread Oleg Fayans
Hi Martin,

I've updated the patch with regard to your review. Thank you!

On 04/12/2016 03:35 PM, Martin Babinsky wrote:
> On 04/07/2016 12:35 PM, Oleg Fayans wrote:
>>
>>
>>
> Hi Oleg,
> 
> since this is a part of replica promotion test suite please add the link
> to https://fedorahosted.org/freeipa/ticket/5723 to the commit message.

Done

> 
> The patch cannot be applied cleanly, even 3-way merge fails with:
> 
> """
> git am
> ../review/ofayans/freeipa-ofayans-0035-Add-test-if-replica-is-working-after-domain-upgrade.patch
> -3
> Applying: Add test if replica is working after domain upgrade
> error: invalid object 100644 acae5c924594cc73bf262eeab5f843f252723207
> for 'ipatests/test_integration/test_replica_promotion.py'
> fatal: git-write-tree: error building trees
> Repository lacks necessary blobs to fall back on 3-way merge.
> Cannot fall back to three-way merge.
> Patch failed at 0001 Add test if replica is working after domain upgrade
> """
> 
> I had to fall back to plain 'patch -p1'
> 
> I have your previous patches 0033-0034 applied. The patch probably needs
> a rebase.

Fixed

> 
> Also I would be more happy if the username for 'testuser' was not
> hardcoded in the code. You can factor it out as a class member.
> 

Done

-- 
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
From a0a0fd1ca1a06704e5e692f53385f4430e453160 Mon Sep 17 00:00:00 2001
From: Oleg Fayans 
Date: Wed, 13 Apr 2016 10:02:32 +0200
Subject: [PATCH] Add test if replica is working after domain upgrade

Corresponds to the testcase described in
http://www.freeipa.org/page/V4/Replica_Promotion/Test_plan#Test_case:
_Replica_created_using_old_workflow_is_functional_after_domain_upgrade

https://fedorahosted.org/freeipa/ticket/5723
---
 .../test_integration/test_replica_promotion.py | 26 ++
 1 file changed, 26 insertions(+)

diff --git a/ipatests/test_integration/test_replica_promotion.py b/ipatests/test_integration/test_replica_promotion.py
index 85beaec4530848b6005712a1ae1dead5f5d69cd4..1f683b6d5c067ec526b307eea1460cafbadb80cb 100644
--- a/ipatests/test_integration/test_replica_promotion.py
+++ b/ipatests/test_integration/test_replica_promotion.py
@@ -351,3 +351,29 @@ class TestProhibitReplicaUninstallation(IntegrationTest):
in result.stdout_text), ("Expected error message was not found")
 self.replicas[0].run_command(['ipa-server-install', '--uninstall',
   '-U', '--ignore-topology-disconnect'])
+
+
+class TestOldReplicaWorksAfterDomainUpgrade(IntegrationTest):
+topology = 'star'
+num_replicas = 1
+domain_level = DOMAIN_LEVEL_0
+username = 'testuser'
+
+def test_replica_after_domain_upgrade(self):
+tasks.kinit_admin(self.master)
+tasks.kinit_admin(self.replicas[0])
+self.master.run_command(['ipa', 'user-add', self.username,
+ '--first', 'test',
+ '--last', 'user'])
+tasks.wait_for_replication(self.replicas[0].ldap_connect())
+self.master.run_command(['ipa', 'domainlevel-set',
+ str(DOMAIN_LEVEL_1)])
+result = self.replicas[0].run_command(['ipa', 'user-show',
+   self.username])
+assert("User login: %s" % self.username in result.stdout_text), (
+"A testuser was not found on replica after domain upgrade")
+self.replicas[0].run_command(['ipa', 'user-del', self.username])
+tasks.wait_for_replication(self.master.ldap_connect())
+result1 = self.master.run_command(['ipa', 'user-show', self.username],
+  raiseonerr=False)
+assert_error(result1, "%s: user not found" % self.username, 2)
-- 
1.8.3.1

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

Re: [Freeipa-devel] [python-pytest-multihost]-Ticket-6 run_command produces exit code 1 on windows

2016-04-13 Thread Niranjan
Niranjan wrote:
> Petr Viktorin wrote:
> > On 04/06/2016 10:55 AM, Niranjan wrote:
> > > Greetings,
> > > 
> > > For https://fedorahosted.org/python-pytest-multihost/ticket/6 , i have 
> > > proposed
> > > a patch, I think this patch is more of a workaround , than a solution. I 
> > > would
> > > like to get more inputs on how to use pytest-multihost to execute 
> > > commands 
> > > on Windows. The method i am using requires cygwin with openssh/bash to be
> > > installed. 
> > 
> > Wow. I'm surprised the only problem on Windows is the "set -e".
> > 
> > Regarding the patch:
> > 
> > - Why is get_winhost_class needed? I don't see it called anywhere.
> > - Similarly, why is host_type needed? Your patch only sets it.
> > 
> > If run_command of WinHost and Unix hosts are this similar, I would put
> > the 'set -e\n' for Unix (and an empty string for Windows) in a class
> > attribute named e.g. "command_prelude", use it in run_command, and move
> > run_command from Host to BaseHost.
> > Would that work, or am I missing something?
> How do we detect the host is windows/unix then , we still need host_type 
> to know what the type of host is (unix/windows)?
> > 
> > 
> > -- 
> > Petr Viktorin
Please review the attached patch.



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

From 5b01d7eb1bc3723201b6083a814467f87d865367 Mon Sep 17 00:00:00 2001
From: Niranjan MR 
Date: Tue, 12 Apr 2016 17:18:10 +0530
Subject: [PATCH] modify run_command to execute on Windows

Add global parameter windows_test_dir to specify test directory
for Windows
Add parameter host_type(per host) to specify the type of host
(host_type:windows)
move run_command from Host class to BaseHost class

Signed-off-by: Niranjan MR 
---
 pytest_multihost/config.py |  3 ++
 pytest_multihost/host.py   | 87 +-
 2 files changed, 50 insertions(+), 40 deletions(-)

diff --git a/pytest_multihost/config.py b/pytest_multihost/config.py
index 
31045c21e8ee67c1793f154f841375f8df7b365f..58a3a5fbc8dbcdaac35e3fd305f23999d5f5b09f
 100644
--- a/pytest_multihost/config.py
+++ b/pytest_multihost/config.py
@@ -45,6 +45,7 @@ class Config(object):
 self.ssh_password = kwargs.get('ssh_password')
 self.ssh_username = kwargs.get('ssh_username', 'root')
 self.ipv6 = bool(kwargs.get('ipv6', False))
+self.windows_test_dir = kwargs.get('windows_test_dir', 
'/home/Administrator')
 
 if not self.ssh_password and not self.ssh_key_filename:
 self.ssh_key_filename = '~/.ssh/id_rsa'
@@ -80,6 +81,8 @@ class Config(object):
 dct['ssh_key_filename'] = dct.pop('root_ssh_key_filename')
 if 'root_password' in dct:
 dct['ssh_password'] = dct.pop('root_password')
+if 'windows_test_dir' in dct:
+dct['windows_test_dir'] = dct.pop('windows_test_dir')
 
 all_init_args = set(init_args) | set(cls.extra_init_args)
 extra_args = set(dct) - all_init_args
diff --git a/pytest_multihost/host.py b/pytest_multihost/host.py
index 
e6c0db50799b2042e699d66ed9ec5f04f6592d31..828ba81279b563e4c5d2735bc6396480f4152304
 100644
--- a/pytest_multihost/host.py
+++ b/pytest_multihost/host.py
@@ -27,9 +27,13 @@ class BaseHost(object):
 transport_class = None
 
 def __init__(self, domain, hostname, role, ip=None,
- external_hostname=None, username=None, password=None):
+ external_hostname=None, username=None, 
+ password=None, windows_test_dir=None, 
+ host_type=None):
 self.domain = domain
 self.role = str(role)
+self.host_type = host_type
+self.command_prelude = 'set -e\n'
 if username is None:
 self.ssh_username = self.config.ssh_username
 else:
@@ -40,6 +44,8 @@ class BaseHost(object):
 else:
 self.ssh_key_filename = None
 self.ssh_password = password
+if windows_test_dir is None:
+self.windows_test_dir = self.config.windows_test_dir
 
 shortname, dot, ext_domain = hostname.partition('.')
 self.shortname = shortname
@@ -118,11 +124,13 @@ class BaseHost(object):
 
 username = dct.pop('username', None)
 password = dct.pop('password', None)
+windows_test_dir = dct.pop('windows_test_dir', None)
+host_type = dct.pop('host_type', None)
 
 check_config_dict_empty(dct, 'host %s' % hostname)
 
 return cls(domain, hostname, role, ip, external_hostname,
-   username, password)
+   username, password, windows_test_dir, host_type)
 
 def to_dict(self):
 """Export info about this Host to a dict"""
@@ -131,6 +139,8 @@ class BaseHost(object):
 'ip': self.ip,
 'role': self.role,
 'external_hostname': self.ext

Re: [Freeipa-devel] [PATCH] Added fix for notifying user about account expiration in Web UI

2016-04-13 Thread Abhijeet Kasurde

Ping

On 03/22/2016 04:32 PM, Abhijeet Kasurde wrote:

Hi All,

Please find the updated patches as per review comments.

On 03/18/2016 07:39 PM, Petr Vobornik wrote:

On 03/18/2016 02:21 PM, Abhijeet Kasurde wrote:

Hi All,

Please review these patches.

Fixes : https://fedorahosted.org/freeipa/ticket/5077

Thanks,
Abhijeet Kasurde



'invalid' is a default and right now is meant for invalid 
password(not correct, see below). So by reading the patch, it will 
break the case when user sets invalid password.


Better would be to process kinit output in 
rpcserver.py:login_password and set e.g: 'krbprincipal-expired' reason.


Then add it to a list of known errors in ipa.js:login_password:498. 
We should probaly add also 'invalid-password' to the list.


Then do the change as in this patch but only with: 
'krbprincipal-expired'.


If 'invalid-password' is added to the list of know errors then we 
should change the default error from "The password or username you 
entered is incorrect. " to e.g.: 'Login failed from unknown reason"



Thanks Petr for suggestions.

Thanks,
Abhijeet Kasurde




-- 
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] ca-less tests updated

2016-04-13 Thread Oleg Fayans
Please. disregard my previous patch-0014: it doesn't pass pylint. The
newer one does.

On 04/08/2016 04:46 PM, Oleg Fayans wrote:
> Hi all,
> 
> It's been a while, and now the patch seems to be stable. It does hit one
> known issue with replica installation occationally [1], but other than
> that works fine on both domain levels.
> 
> [1] https://fedorahosted.org/freeipa/ticket/5758
> 
> 
> 

-- 
Oleg Fayans
Quality Engineer
FreeIPA team
RedHat.
From 1d750eb5097c8cffc7b6ceb7c3bfd719ca7078cb Mon Sep 17 00:00:00 2001
From: Oleg Fayans 
Date: Wed, 13 Apr 2016 11:58:33 +0200
Subject: [PATCH] Actualized ca-less tests

https://fedorahosted.org/freeipa/ticket/4589
---
 ipatests/test_integration/test_caless.py | 452 +--
 1 file changed, 248 insertions(+), 204 deletions(-)

diff --git a/ipatests/test_integration/test_caless.py b/ipatests/test_integration/test_caless.py
index fdc4fc8efe73631e9ab03f3b9019444f7d7e09ec..6b05112af120cbba35b0fea593e68291665bfd52 100644
--- a/ipatests/test_integration/test_caless.py
+++ b/ipatests/test_integration/test_caless.py
@@ -32,20 +32,19 @@ from ipaplatform.paths import paths
 from ipapython.dn import DN
 from ipatests.test_integration.base import IntegrationTest
 from ipatests.test_integration import tasks
+from env_config import get_global_config
+from ipalib.constants import DOMAIN_LEVEL_0, DOMAIN_LEVEL_1
 
 _DEFAULT = object()
+config = get_global_config()
 
 
 def get_install_stdin(cert_passwords=()):
 lines = [
-'yes',  # Existing BIND configuration detected, overwrite? [no]
 '',  # Server host name (has default)
-'',  # Confirm domain name (has default)
 ]
 lines.extend(cert_passwords)  # Enter foo.p12 unlock password
 lines += [
-'',  # Do you want to configure the reverse zone? [yes]
-'',  # Please specify the reverse zone name [47.34.10.in-addr.arpa.]
 'yes',  # Continue with these values?
 ]
 return '\n'.join(lines + [''])
@@ -86,22 +85,23 @@ class CALessBase(IntegrationTest):
 client_hostname = cls.clients[0].hostname
 else:
 client_hostname = 'unused-client.test'
-env = {
+cls.env = {
 'domain': cls.master.domain.name,
 'server1': cls.master.hostname,
 'server2': replica_hostname,
 'client': client_hostname,
 'dbdir': 'nssdb',
-'dbpassword': cls.cert_password,
 'crl_path': cls.crl_path,
+'dirman_password': cls.master.config.dirman_password,
 }
-ipautil.run(['bash', '-ex', scriptfile], cwd=cls.cert_dir, env=env)
+ipautil.run(['bash', '-ex', scriptfile], cwd=cls.cert_dir, env=cls.env)
 
 for host in cls.get_all_hosts():
 tasks.apply_common_fixes(host)
 
 # Copy CRLs over
 base = os.path.join(cls.cert_dir, 'nssdb')
+host.transport.mkdir_recursive(host.config.test_dir)
 host.transport.mkdir_recursive(cls.crl_path)
 for source in glob.glob(os.path.join(base, '*.crl')):
 dest = os.path.join(cls.crl_path, os.path.basename(source))
@@ -112,6 +112,10 @@ class CALessBase(IntegrationTest):
 # Remove the NSS database
 shutil.rmtree(cls.cert_dir)
 
+# Remove CA cert in /etc/pki/nssdb, in case of failed (un)install
+for host in cls.get_all_hosts():
+cls.uninstall_server(host)
+
 super(CALessBase, cls).uninstall(mh)
 
 @classmethod
@@ -140,6 +144,11 @@ class CALessBase(IntegrationTest):
 for filename in set(files_to_copy):
 cls.copy_cert(host, filename)
 
+# Remove existing ca certs from default database to avoid conflicts
+args = ["certutil", "-D", "-d", "/etc/httpd/alias", "-n"]
+host.run_command(args + ["ca1"], raiseonerr=False)
+host.run_command(args + ["ca1/server"], raiseonerr=False)
+
 host.collect_log(paths.IPASERVER_INSTALL_LOG)
 host.collect_log(paths.IPACLIENT_INSTALL_LOG)
 inst = host.domain.realm.replace('.', '-')
@@ -152,11 +161,14 @@ class CALessBase(IntegrationTest):
 '--dirsrv-cert-file', dirsrv_pkcs12,
 '--ca-cert-file', root_ca_file,
 '--ip-address', host.ip,
-'-r', host.domain.name,
+'-n', host.domain.name,
+'-r', host.domain.realm,
 '-p', host.config.dirman_password,
 '-a', host.config.admin_password,
 '--setup-dns',
 '--forwarder', host.config.dns_forwarder,
+'--auto-reverse',
+'--domain-level', str(config.domain_level)
 ]
 
 if http_pin is not None:
@@ -165,7 +177,6 @@ class CALessBase(IntegrationTest):
 args.extend(['--dirsrv-pin', dirsrv_pin])
 if unattended:
 args.extend(['-U'])
-
 return host.run_command(args, raiseonerr=False, stdin_text=stdin_text)
 
 @classmethod
@@ 

Re: [Freeipa-devel] [PATCH 0024] ipa-replica-manage: added --suffix option for certain commands

2016-04-13 Thread Martin Basti



On 06.04.2016 14:04, Stanislav Laznicka wrote:

On 03/30/2016 04:52 PM, Martin Basti wrote:

On 24.03.2016 19:10, Stanislav Laznicka wrote:

On 03/23/2016 08:13 PM, Martin Basti wrote:

[...]
Can you please update design
http://www.freeipa.org/page/V4/Manage_replication_topology_4_4 (mainly
the --suffix option)? Also there are missing clean-ruv and list-ruv
commands in design, and fix usage at the bottom.

1)
I don't understand this expression
+if dirman_passwd is None or (
+   not dirman_passwd and args[0] in cs_enabled_commands):

You already tested if subcommand belongs to cs_enabled_commands few
lines above, IMO the 'dirman_password is None' expression is enough.
If I understand it well, when empty password is entered, the program 
continues and uses Kerberos credentials for some operations. E.g. 
for list-ruv, if empty password is entered, the command would only 
display RUVs for domain tree but not for the CA tree no matter if CA 
is set up or not (in the current state of the patch, after get_ruv 
refactoring). This here is one possible way around this, although 
the check for non-empty password might probably just as well be in 
get_ruv_both_suffixes.


ok

2)
+# tuple of commands that work with ca tree and need Directory Manager
password
+cs_enabled_commands = ("list-ruv", "clean-ruv", "abort-clean-ruv")

this variable is used only toi detect if dirman passwd is needed, I
suggest to rename it to commands_req_dirman_passwd, or something 
better.


3)
Q: Do we need is_cs_set() function?
A: Yes!

I wanted to give you ultimate NACK, but then I checked how get_ruv 
code

works and I changed my mind.

Please write a comment where is_cs_set function is used, why we need
extra function instead of catching an exception, possibly you can 
open a

refactoring ticket.
After the refactoring changes, is_cs_set should not be needed 
anymore, removed it.


Shame:
1)
+if not test_connection(realm, host, options.nolookup) or\
Please use parentheses instead of backslash

2)
+   args[0] in cs_enabled_commands:

+   not dirman_passwd and args[0] in cs_enabled_commands):

Indentation is not multiplication of 4

Shame on me indeed, fixed it.


Nitpicks (I don't insist on fixing these):
1)
+if servers.get('ca', None):

None is default

2)
+for (netloc, rid) in servers['ca']:
parentheses are not needed

3)
+ print("\t%s: %s" % (netloc, rid))
Would be nice to use .format() instead of %

Martin^2



I changed my mind, ultimate NACK.
Please fix get_ruv function, is_cs_set will not help. In case there 
are no RUVs but CA is installed, sys.exit there prevents us from 
removing RUVs (or any else operation) on domain suffix, and vice 
versa.
I propose to move ticket to 4.4 milestone because I would like to 
avoid breaking something in 4.3, as this will hit many places in 
ipa-replica-manage.


Please provide the refactoring of get_ruv as separate patch a put 
these patches on top of it.


Martin2
Did the refactoring. There also seemed to be duplicit code in 
abort_clean_ruv for some reason, removed it (I hope it does not 
break anything, but it seemed rather useless). Also had to change 
the numbers of the patches so that they would apply.


NACK

* ipa-replica-manage refactoring *

1)
Instead of:
-print("Failed to connect to server %s: %s" % (host, e))
-sys.exit(1)
+root_logger.debug("Failed to connect to server {host}: {err}"
+  .format(host=host, err=e))
+raise RuntimeError(e)

I expected

-print("Failed to connect to server %s: %s" % (host, e))
-sys.exit(1)
+root_logger.debug(traceback.format_exc())
+raise RuntimeError("Failed to connect to server {host}: {err}"
+  .format(host=host, err=e)))

Should be fixed now.


2)
-print("No RUV records found.")
-sys.exit(0)

Here is exit state 0, so we should not raise error.

I think that we should create new nonfatal exception.
Made a new nonfatal error exception, then. This step was a bit 
controversial when it comes to get_ruv_both_suffixes because it needs 
to catch both this new exception and a RuntimeError exception for both 
trees. As the main idea here was not to stop if either tree is missing 
(resulting in RuntimeError in get_ruv) or contains no records 
(NonFatalError), it is only printed that something bad may have 
happened (or it's debug-logged in case of nonfatal errors). In the 
end, only NonFatalError exception is raised if no records were found 
for whatever reason resulting in the script always returning 0.


3)
-print("unable to decode: %s" % ruv)
+root_logger.debug("unable to decode: %s" % ruv)
This may break tests, because the logger logs to stderr, leave it as 
print for now


4)
-servers = get_ruv(realm, host, dirman_passwd, nolookup)
+try:
+servers = get_ruv(realm, host, dirman_passwd, nolookup)
+except Run

Re: [Freeipa-devel] [PATCH 0405] idviews: Add user certificate attribute to user ID overrides

2016-04-13 Thread Tomas Babej
On 04/13/2016 09:55 AM, Tomas Babej wrote:
> On 04/07/2016 01:53 PM, Sumit Bose wrote:
>> On Mon, Apr 04, 2016 at 04:27:02PM +0200, Jan Cholasta wrote:
>>> Hi,
>>>
>>> On 1.4.2016 16:53, Tomas Babej wrote:
 Hi,

 this extends the user ID overrides with capability to store the user
 certificate.

 https://fedorahosted.org/freeipa/ticket/4955
>>>
>>> The preferred way of managing certificates nowadays is using $OBJ-add-cert
>>> and $OBJ-remove-cert commands, you should add them here as well.
>>>
>>> I would even go as far as not allowing to modify certificates using
>>> idoverrideuser-mod - in user-mod and host-mod, it's there just for backward
>>> compatibility, which is not the case here. But I don't have a strong opinion
>>> on that.
>>>
>>> For consistency with user-find and host-find, the full certificate blob
>>> should not be shown in idoverrideuser-find. You can do that by setting
>>> search_display_attributes attribute on the idoverrideuser class
>>> appropriately.
>>
>> I tested the current patch with my related patches for SSSD and all is
>> working as expected.
>>
>> bye,
>> Sumit
>>
>>>
>>> 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
>>
> 
> Thanks for the reviews,
> 
> attaching a updated patch that addresses Honza's comments.
> 
> Tomas
> 

Sending an improved version addressing a couple of additional issues.

Tomas
From f56129024fecfe1522cd6bd85f7daddfd3bf5129 Mon Sep 17 00:00:00 2001
From: Tomas Babej 
Date: Thu, 3 Mar 2016 15:14:10 +0100
Subject: [PATCH] idviews: Add user certificate attribute to user ID overrides

---
 ACI.txt  |  2 +-
 API.txt  | 30 ++--
 install/share/71idviews.ldif |  2 +-
 ipalib/plugins/idviews.py| 82 ++--
 4 files changed, 110 insertions(+), 6 deletions(-)

diff --git a/ACI.txt b/ACI.txt
index 24cb332ce6e10c82a5bfab76d084fb6c0277800d..ae00cf7a1b8e2ea0e33798993bb24dc5f06127e3 100644
--- a/ACI.txt
+++ b/ACI.txt
@@ -149,7 +149,7 @@ aci: (targetfilter = "(objectclass=ipahostgroup)")(version 3.0;acl "permission:S
 dn: cn=views,cn=accounts,dc=ipa,dc=example
 aci: (targetattr = "cn || createtimestamp || description || entryusn || gidnumber || ipaanchoruuid || modifytimestamp || objectclass")(targetfilter = "(objectclass=ipaGroupOverride)")(version 3.0;acl "permission:System: Read Group ID Overrides";allow (compare,read,search) userdn = "ldap:///all";;)
 dn: cn=views,cn=accounts,dc=ipa,dc=example
-aci: (targetattr = "createtimestamp || description || entryusn || gecos || gidnumber || homedirectory || ipaanchoruuid || ipaoriginaluid || ipasshpubkey || loginshell || modifytimestamp || objectclass || uid || uidnumber")(targetfilter = "(objectclass=ipaUserOverride)")(version 3.0;acl "permission:System: Read User ID Overrides";allow (compare,read,search) userdn = "ldap:///all";;)
+aci: (targetattr = "createtimestamp || description || entryusn || gecos || gidnumber || homedirectory || ipaanchoruuid || ipaoriginaluid || ipasshpubkey || loginshell || modifytimestamp || objectclass || uid || uidnumber || usercertificate")(targetfilter = "(objectclass=ipaUserOverride)")(version 3.0;acl "permission:System: Read User ID Overrides";allow (compare,read,search) userdn = "ldap:///all";;)
 dn: cn=ranges,cn=etc,dc=ipa,dc=example
 aci: (targetattr = "cn || createtimestamp || entryusn || ipabaseid || ipabaserid || ipaidrangesize || ipanttrusteddomainsid || iparangetype || ipasecondarybaserid || modifytimestamp || objectclass")(targetfilter = "(objectclass=ipaidrange)")(version 3.0;acl "permission:System: Read ID Ranges";allow (compare,read,search) userdn = "ldap:///all";;)
 dn: cn=views,cn=accounts,dc=ipa,dc=example
diff --git a/API.txt b/API.txt
index 5b75413f930d0e9caaffc68023bed8106d786653..76b260da72533ee88027f72d56a591c7566c72b7 100644
--- a/API.txt
+++ b/API.txt
@@ -2429,7 +2429,7 @@ output: Entry('result', , Gettext('A dictionary representing an LDA
 output: Output('summary', (, ), None)
 output: PrimaryKey('value', None, None)
 command: idoverrideuser_add
-args: 2,15,3
+args: 2,16,3
 arg: Str('idviewcn', cli_name='idview', multivalue=False, primary_key=True, query=True, required=True)
 arg: Str('ipaanchoruuid', attribute=True, cli_name='anchor', multivalue=False, primary_key=True, required=True)
 option: Str('addattr*', cli_name='addattr', exclude='webui')
@@ -2446,6 +2446,19 @@ option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui
 option: Str('setattr*', cli_name='setattr', exclude='webui')
 option: Str('uid', attribute=True, cli_name='login', maxlength=255, multivalue=False, pattern='^[a-zA-Z0-9_.][a-zA-Z0-9_.-]{0,252}[a-zA-Z0-9_.$-]?$', required=False)
 option: Int('uidnumber', attribute=True, cli_name='uid', minvalue=1, multivalue=False, requir

[Freeipa-devel] [PATCH] 0013 webui: Add ability to convert users from preserved to staged state

2016-04-13 Thread Pavel Vomacka

Hello,

This patch adds ability to convert users from preserved to staged state.

Fixes this ticket: https://fedorahosted.org/freeipa/ticket/5371

--
Pavel^3 Vomacka
>From ac0396b8644fe9cf421a491c35c7eed017dc5cca Mon Sep 17 00:00:00 2001
From: Pavel Vomacka 
Date: Wed, 13 Apr 2016 09:09:17 +0200
Subject: [PATCH] Add ability to convert users from preserved to staged state

Add 'stage' button to the page with list of preserved users and 'stage'
option into the actions dropdown menu on preserved user details page.
Stage action converts users from preserved state to staged state.

https://fedorahosted.org/freeipa/ticket/5371
---
 install/ui/ipa.css  |  4 +++
 install/ui/src/freeipa/stageuser.js | 54 +
 install/ui/src/freeipa/user.js  | 20 --
 install/ui/test/data/ipa_init.json  |  4 +++
 ipalib/plugins/internal.py  |  4 +++
 5 files changed, 84 insertions(+), 2 deletions(-)

diff --git a/install/ui/ipa.css b/install/ui/ipa.css
index f419eb224252aa03eaf4b25bb03435f4c9a6de9b..7997a71d76e1f8ed7811d68b53a5f12892c49eb4 100644
--- a/install/ui/ipa.css
+++ b/install/ui/ipa.css
@@ -174,6 +174,10 @@ div[name=settings].facet-group li a {
 float: right;
 }
 
+.facet-controls-right .fa.fa-user {
+opacity: 0.5;
+}
+
 .control-buttons {
 display: inline-block;
 }
diff --git a/install/ui/src/freeipa/stageuser.js b/install/ui/src/freeipa/stageuser.js
index ca4e30025a14853a287e4a29478f80a843ab1c27..1ca692b6a34e0897c10188998a8ee64fb39aa237 100644
--- a/install/ui/src/freeipa/stageuser.js
+++ b/install/ui/src/freeipa/stageuser.js
@@ -291,6 +291,9 @@ stageuser.search_preserved_facet_spec = {
 actions: [
 {
 $type: 'batch_undel'
+},
+{
+$type: 'batch_stage'
 }
 ],
 control_buttons: [
@@ -298,6 +301,11 @@ stageuser.search_preserved_facet_spec = {
 name: 'undel',
 label: '@i18n:buttons.restore',
 icon: 'fa-heart'
+},
+{
+name: 'batch_stage',
+label: '@i18n:buttons.stage',
+icon: 'fa-user'
 }
 ],
 policies: [
@@ -316,6 +324,12 @@ mod_user.entity_spec.policies.push(
 {
 $factory: IPA.facet_update_policy,
 source_facet: 'search_preserved',
+dest_entity: 'stageuser',
+dest_facet: 'search'
+},
+{
+$factory: IPA.facet_update_policy,
+source_facet: 'search_preserved',
 dest_entity: 'user',
 dest_facet: 'search'
 },
@@ -358,6 +372,44 @@ stageuser.batch_undel_action = function(spec) {
 return IPA.batch_items_action(spec);
 };
 
+stageuser.batch_stage_action = function(spec) {
+
+spec = spec || {};
+
+spec.name = spec.name || 'batch_stage';
+spec.method = spec.method || 'stage';
+spec.needs_confirm = spec.needs_confirm === undefined ? true : spec.needs_confirm;
+spec.enable_cond = spec.enable_cond || ['item-selected'];
+spec.success_msg = spec.success_msg || '@i18n:objects.stageuser.stage_success';
+spec.confirm_msg = spec.confirm_msg || '@i18n:objects.stageuser.stage_confirm';
+
+return IPA.batch_items_action(spec);
+};
+
+stageuser.stage_action = function(spec) {
+
+spec = spec || {};
+
+spec.name = spec.name || 'stage';
+spec.method = spec.method || 'stage';
+spec.show_cond = spec.show_cond || ['preserved_user'];
+spec.needs_confirm = spec.needs_confirm !== undefined ? spec.needs_confirm : true;
+spec.confirm_msg = spec.confirm_msg || '@i18n:objects.stageuser.stage_one_confirm';
+spec.label = spec.label || '@i18n:buttons.stage';
+
+var that = IPA.object_action(spec);
+
+that.on_success = function(facet, data, text_status, xhr) {
+
+IPA.notify_success(data.result.summary);
+facet.on_update.notify();
+facet.redirect();
+};
+
+return that;
+};
+
+
 /**
  * Stage user entity specification object
  * @member stageuser
@@ -374,6 +426,8 @@ stageuser.register = function() {
 var f = reg.facet;
 a.register('batch_activate', stageuser.batch_activate_action);
 a.register('batch_undel', stageuser.batch_undel_action);
+a.register('batch_stage', stageuser.batch_stage_action);
+a.register('stage', stageuser.stage_action);
 e.register({type: 'stageuser', spec: stageuser.stageuser_spec});
 f.register_from_spec('user_search_preserved', stageuser.search_preserved_facet_spec);
 };
diff --git a/install/ui/src/freeipa/user.js b/install/ui/src/freeipa/user.js
index a9727f57d69e7126d87707f541786ffab4d0c999..a871dd509125bc64f51bad7fc5a04fff94eed9c6 100644
--- a/install/ui/src/freeipa/user.js
+++ b/install/ui/src/freeipa/user.js
@@ -61,7 +61,19 @@ return {
 name: 'user',
 policies: [
 IPA.search_facet_update_policy,
-IPA.details_facet_update_policy
+IPA.details_facet_update_policy,
+{
+$factory: IPA.facet_update_policy,
+source_facet: 'de

Re: [Freeipa-devel] [PATCH 0453 - 0458] host-del: fix updatedns option

2016-04-13 Thread Petr Spacek
On 1.4.2016 18:30, Martin Basti wrote:
> https://fedorahosted.org/freeipa/ticket/5675
> 
> Patches attached.

NACK, it breaks if the client does not have any corresponding DNS record.

[root@vm-033 git]# ipa host-add host.test. --force
--
Added host "host.test"
--
  Host name: host.test
  Principal name: host/host.t...@dom-033.abc.idm.lab.eng.brq.redhat.com
  Password: False
  Keytab: False
  Managed by: host.test

[root@vm-033 git]# ipa host-del host.test. --updatedns
ipa: ERROR: host.test: host not found

I think we already had a ticket to prevent this kind of error, no?

-- 
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] [TEST][patch-0035] Fixed failing legacy client tests

2016-04-13 Thread Martin Basti



On 12.04.2016 17:23, Martin Babinsky wrote:

On 04/12/2016 11:19 AM, Oleg Fayans wrote:





ACK.


Pushed to:
master: 280f1ed85f40a0d123fc891f9ad02a4fff4a363b
ipa-4-3: 488dcd3e94a5d6c791baff8cb2f07530072b1d31

--
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] [DESIGN] Kerberos principal alias handling

2016-04-13 Thread Martin Babinsky

On 04/13/2016 07:50 AM, David Kupka wrote:

On 08/04/16 17:10, Martin Babinsky wrote:

Hi list,

I have put together a draft [1] outlining the effort to reimplement the
handling of Kerberos principals in both backend and frontend layers of
FreeIPA so that we may have multiple aliases per user, host or service
and thus implement stuff like
https://fedorahosted.org/freeipa/ticket/3961 and
https://fedorahosted.org/freeipa/ticket/5413 .

Since much of the plumbing was already implemented,[2] the document
mainly describes what the patches do. Some parts required by other use
cases may be missing so please point these out.

I would also be happy if you could correct all factual inacurracies, I
did research on this issue a long time ago and my knowledge turned a bit
rusty.

[1] http://www.freeipa.org/page/V4/Kerberos_principal_aliases
[2]
https://www.redhat.com/archives/freeipa-devel/2015-October/msg00048.html



Hi,
after reading the designs following thoughts comes to my mind.

1) Just to be sure that I understand the new ticket obtaining process
correctly I'd like to summarize.
We need to always search all krbPrincipalName values, krbCanonicalName
and ipaKrbPrincipalAlias (for backward compatibility).
For TGT request case sensitivity of the search and principal in returned
ticket depends on canonicalization. When canonicalization is requested
the search is case-insensitive and krbCanonicalName is used otherwise
case-sensitive search is performed and principal from request is used.
When requesting TGS search is always case-sensitive and principal from
request is used.

In pseudo-code:

get_tgt(principal, secret, canonicalization)
 if canonicalization
 if principal case-insensitive-in {krbPrincipalName +
ipaKrbPrincipalAlias + krbCanonicalName}
 # verify secret, perform various other checks...
 return TGT(krbCanonicalName)
 else
 if principal case-sensitive-in {krbPrincipalName +
ipaKrbPrincipalAlias + krbCanonicalName}
 # verify secret, perform various other checks...
 return TGT(principal)

get_tgs(service_principal, TGT)
 if service_principal case-sensitive-in {krbPrincipalName +
ipaKrbPrincipalAlias + krbCanonicalName}
 # verify TGT, perform various other checks...
 return TGS(service_principal)

Do I understand it right?

2) I would like to add following constrains for
krb{Canonical,Principal}Name attributes:

when user/host/service is created krbCanonicalName is set to the same
value as krbPrincipalName
krbCanonicalName cannot be modified
krbPrincipalName with the same value as krbCanonicalName cannot be
removed/modified
krbPrincipalName must be case-insensitively unique in whole DB
krbPrincipalName attributes can be added and/or removed


I will definitwly add this constraints into the design.


This will allow us to keep the first krbPrincipalName as RDN for
services/hosts and give the flexibility of adding/removing aliases.

'Change of username' use case is also solvable with this approach. When
username is changed we add krbPrincipalName with the new username. That
will allow user to login with either old or new name.

3) ad CLI:
{user,host,service}-add - Can canonicalname be specified? Or will it
take principal argument/option value?
Can we add {user,host,service}-{add,remove}-principal set of commands
for principal manipulation? I really don't want to use
--{add,set,del}-attr unless necessary.

I have added the commands to the design.


Will {user,host,service}-{show,find} display krbCanonicalName by default
or only with --all option?


IIRC it should be printed out only when '--all' is requested.

4) ad Upgrade:
I think it would be worth to check and document what happens during
upgrade of multiple replicas. There may be confusing behavior when
obtaining tickets. KDC behavior will differ among servers and since
autodiscovery is in use we don't know if we are talking to the old or
new server. I'm not sure what exactly will happen but I suspect it won't
be nice.

I will test this but I expected that 'kinit -C' will get back TGT with 
different principal for different replica (old vs. new) for newly added 
entries (the old entries will behave the same as before). That may not 
be what we want, but I can not think of any way to amend this.


What we forgot to discuss is the handling of krbCanonicalName during 
MODRDN operation, e.g. when username is changed.


Currently the design assumes that when uid changes the MODRDN plugin 
will change krbCanonicalName attribute to reflect this change, e.g.


uid: jsmith->johns

krbCanonicalName: jsmith@REALM -> krbCanoncialName: johns@REALM

I remember talking with Simo (CC'ed) that this is the desired behavior 
but I am not sure if this still holds.


--
Martin^3 Babinsky

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


Re: [Freeipa-devel] [PATCH 0453 - 0458, 0460] host-del: fix updatedns option

2016-04-13 Thread Martin Basti



On 13.04.2016 16:01, Petr Spacek wrote:

On 1.4.2016 18:30, Martin Basti wrote:

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

Patches attached.

NACK, it breaks if the client does not have any corresponding DNS record.

[root@vm-033 git]# ipa host-add host.test. --force
--
Added host "host.test"
--
   Host name: host.test
   Principal name: host/host.t...@dom-033.abc.idm.lab.eng.brq.redhat.com
   Password: False
   Keytab: False
   Managed by: host.test

[root@vm-033 git]# ipa host-del host.test. --updatedns
ipa: ERROR: host.test: host not found

I think we already had a ticket to prevent this kind of error, no?



This will be resolved in https://fedorahosted.org/freeipa/ticket/5627

Patch 460 attached, feel free to review both tickets :)
(Patch requires my previous DNS patches)

Martin^2
From c533b677a6fe6e303c28004ef321460be7622c19 Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Wed, 2 Mar 2016 13:29:27 +0100
Subject: [PATCH] host-del --updatedns: print warnings instead of error

When DNS records do not exist, print warnings instead of hard error

https://fedorahosted.org/freeipa/ticket/5627
---
 ipalib/messages.py | 11 +++
 ipalib/plugins/host.py | 26 --
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/ipalib/messages.py b/ipalib/messages.py
index 5cd0ea1769920c076c62729ed5fe359cd1680723..872b455c1a1b65fb9e75c3d03293422ce70972c9 100644
--- a/ipalib/messages.py
+++ b/ipalib/messages.py
@@ -360,6 +360,17 @@ class ResultFormattingError(PublicMessage):
 type = "warning"
 
 
+class FailedToRemoveHostDNSRecords(PublicMessage):
+"""
+**13020** Failed to remove host DNS records
+"""
+
+errno = 13020
+type = "warning"
+format = _("DNS record(s) of host %(host)s could not be removed. "
+   "(%(reason)s)")
+
+
 def iter_messages(variables, base):
 """Return a tuple with all subclasses
 """
diff --git a/ipalib/plugins/host.py b/ipalib/plugins/host.py
index 20b5776dd9b7fba231155237231d9f5f505e1297..04bb2991a1d463e17b1ed11e06b35dc3a9829073 100644
--- a/ipalib/plugins/host.py
+++ b/ipalib/plugins/host.py
@@ -23,6 +23,7 @@ import string
 import six
 
 from ipalib import api, errors, util
+from ipalib import messages
 from ipalib import Str, Flag, Bytes
 from ipalib.plugable import Registry
 from ipalib.plugins.baseldap import (LDAPQuery, LDAPObject, LDAPCreate,
@@ -122,6 +123,10 @@ host_pwd_chars = string.digits + string.ascii_letters + '_,.@+-='
 
 
 def remove_ptr_rec(ipaddr, host, domain):
+"""
+Remove PTR record of IP address (ipaddr)
+:return: True if PTR record was removed, False if record was not found
+"""
 api.log.debug('deleting PTR record of ipaddr %s', ipaddr)
 try:
 revzone, revname = get_reverse_zone(ipaddr)
@@ -134,6 +139,9 @@ def remove_ptr_rec(ipaddr, host, domain):
 api.Command['dnsrecord_del'](revzone, revname, **delkw)
 except errors.NotFound:
 api.log.debug('PTR record of ipaddr %s not found', ipaddr)
+return False
+
+return True
 
 
 def update_sshfp_record(zone, record, entry_attrs):
@@ -760,16 +768,20 @@ class host_del(LDAPDelete):
 parts = fqdn.split('.')
 domain = unicode('.'.join(parts[1:]))
 # Get all resources for this host
+rec_removed = False
 try:
 record = api.Command['dnsrecord_show'](
 domain, parts[0])['result']
 except errors.NotFound:
-self.obj.handle_not_found(*keys)
+pass
 else:
 # remove PTR records first
 for attr in ('arecord', 'record'):
 for val in record.get(attr, []):
-remove_ptr_rec(val, parts[0], domain)
+rec_removed = (
+remove_ptr_rec(val, parts[0], domain) or
+rec_removed
+)
 try:
 # remove all A, , SSHFP records of the host
 api.Command['dnsrecord_mod'](
@@ -781,6 +793,16 @@ class host_del(LDAPDelete):
 )
 except errors.EmptyModlist:
 pass
+else:
+rec_removed = True
+
+if not rec_removed:
+self.add_message(
+messages.FailedToRemoveHostDNSRecords(
+host=fqdn,
+reason=_("No A, , SSHFP or PTR records found.")
+)
+)
 
 if self.api.Command.ca_is_enabled()['result']:
 try:
-- 
2.5.5

-- 
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 0143-0144] different errors/warnings for different LDAP limit type exceeded

2016-04-13 Thread Petr Spacek
On 8.4.2016 17:13, Martin Babinsky wrote:
> On 04/01/2016 05:37 PM, Martin Babinsky wrote:
>> On 03/24/2016 04:14 PM, Martin Babinsky wrote:
>>> On 03/22/2016 04:28 PM, Rob Crittenden wrote:
 Martin Babinsky wrote:
> On 03/21/2016 12:25 PM, Jan Cholasta wrote:
>> On 21.3.2016 10:17, Petr Spacek wrote:
>>> On 18.3.2016 13:49, Rob Crittenden wrote:
 Martin Babinsky wrote:
> These patches implement behavior agreed upon during discussion of
> https://fedorahosted.org/freeipa/ticket/5677
>
> However I'm not sure if we want to push them into 4-3 branch (the
> ticket
> is triaged into 4.3.2 milestone) since they modify the framework
> behavior quite a bit.
>
> If there is no need to have it there (CC'ing Milan since he is the
> reporter), I would retriage it into 4.4 milestone.


 + desc="while getting entries (search base: '{}',"
 + "filter: {})".format(base_dn, filter))

 This is going to expose parts of the DIT in an error message to
 users. We have
 tried in the past to hide the implementation. I'd propose logging
 the
 error
 and making the exception less verbose.
>>
>> I agree with Rob here, we shouldn't expose internal stuff in error
>> messages for users.
>>
>> In this particular case, even if we included internal stuff in the
>> error
>> message, it should be the error message returned by the server rather
>> than this ad-hoc message.
>>
>>>
>>> IMHO it actually helps to print the DN. At very least the user can
>>> see
>>> if the
>>> error is happening always with the same DN or if it keeps changing.
>>>
>>> In other words, for user it is not that important to understand
>>> meaning of the
>>> DN but it might be important to see if it is the same or not.
>>
>> I can't imagine a situation where it would actually be useful for the
>> user (as opposed to the admin, who has access to logs) to know the
>> base
>> DN of some arbitrary LDAP search operation. Could you give an example?
>>
> Right, attaching updated patches.

 I may have suggested debug logging the detailed error. I was wrong. This
 should log at the error level so it always appears in the logs. This may
 be a spurious error and having the user turn on debug logging to capture
 the reasons would be asking a lot.

 rob
>>> That's right, the user would then have to enable debug mode and re-run
>>> the command. I have changed the log level to error as you suggested.
>>>
>>>
>>>
>> Bump for review.
>>
> Moar bumps.

ACK, sorry for the delay!

-- 
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] [WIP PATCH] server-del: perform full master removal in managed topology

2016-04-13 Thread Martin Babinsky
This is a WIP patch which moves the `ipa-replica-manage del` subcommand 
to the 'server-del' API method and exposes it as CLI command[1]. A CI 
test suite is also included.


There are some issues with the patch I would like to discuss in more 
detail on the list:


1.) In the original subcommand there was a lot of output (mostly print 
statements) during all stages of master removal. I have tried to port 
these as messages to the command which results in quite voluminous 
response sent back to the frontend. Should we try to reduce the output?


2.) In the original discussion[2] we assumed that the cleanup part would 
me a separate API method called during server_del postcallback. However 
since the two objects ended up sharing a lot of state (e.g. topology 
state from pre-callback, messages) i have merged it to server-del. That 
makes the code rather unwieldy but I found it difficult to keep the two 
entities separate without some hacking around framework capabilities


3.) since actions in post-callback require a knowledge about topology 
state gathered in pre-callback, I had to store some information in the 
command's context. Sorry about that, if you know about some way to 
circumvent me, let me know.


4.) The master can not remove itself. I have implemented an ad-hoc 
forwarding of the request to other master that can do the job. Is this okay?


5.) Since the original behavior of 'chekc_deleted_segments' was kept, 
the code can sometimes hang waiting for removal of some segments, 
especially during forced removal in wonky topologies. This can cause 
gateway timeout in JSON-RPC call and report false positive error back to 
the user. This makes a large part of 'TestServerDel' suite to fail. How 
should we handle this? Should we keep the original behavior?


6.) There are some in-place imports of server-side code in some places. 
I'm not very happy about it and would be glad if we can agree on a way 
to fix this.



[1] https://fedorahosted.org/freeipa/ticket/5588
[2] https://www.redhat.com/archives/freeipa-devel/2016-March/msg00335.html

--
Martin^3 Babinsky
From e807b266a126e9708329871362aa52a7e15f4183 Mon Sep 17 00:00:00 2001
From: Martin Babinsky 
Date: Fri, 8 Apr 2016 16:29:04 +0200
Subject: [PATCH] integration test suite for server-del

https://fedorahosted.org/freeipa/ticket/5588
---
 ipatests/test_integration/tasks.py   |  32 ++-
 ipatests/test_integration/test_caless.py |  11 +-
 ipatests/test_integration/test_server_del.py | 307 +++
 3 files changed, 335 insertions(+), 15 deletions(-)
 create mode 100644 ipatests/test_integration/test_server_del.py

diff --git a/ipatests/test_integration/tasks.py b/ipatests/test_integration/tasks.py
index 70f4fa7fd96f9d993332996f087577d1c88f828e..f3be216ee75a5d25e0722cb21ac64ad43318b623 100644
--- a/ipatests/test_integration/tasks.py
+++ b/ipatests/test_integration/tasks.py
@@ -57,10 +57,10 @@ def check_arguments_are(slice, instanceof):
 and third arguments are integers
 """
 def wrapper(func):
-def wrapped(*args):
+def wrapped(*args, **kwargs):
 for i in args[slice[0]:slice[1]]:
 assert isinstance(i, instanceof), "Wrong type: %s: %s" % (i, type(i))
-return func(*args)
+return func(*args, **kwargs)
 return wrapped
 return wrapper
 
@@ -721,7 +721,7 @@ def clean_replication_agreement(master, replica):
 
 
 @check_arguments_are((0, 3), Host)
-def create_segment(master, leftnode, rightnode):
+def create_segment(master, leftnode, rightnode, suffix=DOMAIN_SUFFIX_NAME):
 """
 creates a topology segment. The first argument is a node to run the command
 :returns: a hash object containing segment's name, leftnode, rightnode
@@ -731,7 +731,7 @@ def create_segment(master, leftnode, rightnode):
 lefthost = leftnode.hostname
 righthost = rightnode.hostname
 segment_name = "%s-to-%s" % (lefthost, righthost)
-result = master.run_command(["ipa", "topologysegment-add", DOMAIN_SUFFIX_NAME,
+result = master.run_command(["ipa", "topologysegment-add", suffix,
  segment_name,
  "--leftnode=%s" % lefthost,
  "--rightnode=%s" % righthost], raiseonerr=False)
@@ -743,7 +743,7 @@ def create_segment(master, leftnode, rightnode):
 return {}, result.stderr_text
 
 
-def destroy_segment(master, segment_name):
+def destroy_segment(master, segment_name, suffix=DOMAIN_SUFFIX_NAME):
 """
 Destroys topology segment.
 :param master: reference to master object of class Host
@@ -753,7 +753,7 @@ def destroy_segment(master, segment_name):
 kinit_admin(master)
 command = ["ipa",
"topologysegment-del",
-   DOMAIN_SUFFIX_NAME,
+   suffix,
segment_name]
 result = master.run_command(command, raiseonerr=False)
 return result.returncode, result.stderr_text
@@ -1181,

Re: [Freeipa-devel] [PATCH 0453 - 0458, 0460] host-del: fix updatedns option

2016-04-13 Thread Petr Spacek
On 13.4.2016 16:31, Martin Basti wrote:
> 
> 
> On 13.04.2016 16:01, Petr Spacek wrote:
>> On 1.4.2016 18:30, Martin Basti wrote:
>>> https://fedorahosted.org/freeipa/ticket/5675
>>>
>>> Patches attached.
>> NACK, it breaks if the client does not have any corresponding DNS record.
>>
>> [root@vm-033 git]# ipa host-add host.test. --force
>> --
>> Added host "host.test"
>> --
>>Host name: host.test
>>Principal name: host/host.t...@dom-033.abc.idm.lab.eng.brq.redhat.com
>>Password: False
>>Keytab: False
>>Managed by: host.test
>>
>> [root@vm-033 git]# ipa host-del host.test. --updatedns
>> ipa: ERROR: host.test: host not found
>>
>> I think we already had a ticket to prevent this kind of error, no?
>>
> 
> This will be resolved in https://fedorahosted.org/freeipa/ticket/5627
> 
> Patch 460 attached, feel free to review both tickets :)
> (Patch requires my previous DNS patches)

ACK

-- 
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 0453 - 0458] host-del: fix updatedns option

2016-04-13 Thread Petr Spacek
On 13.4.2016 16:01, Petr Spacek wrote:
> On 1.4.2016 18:30, Martin Basti wrote:
>> https://fedorahosted.org/freeipa/ticket/5675
>>
>> Patches attached.
> 
> NACK, it breaks if the client does not have any corresponding DNS record.
> 
> [root@vm-033 git]# ipa host-add host.test. --force
> --
> Added host "host.test"
> --
>   Host name: host.test
>   Principal name: host/host.t...@dom-033.abc.idm.lab.eng.brq.redhat.com
>   Password: False
>   Keytab: False
>   Managed by: host.test
> 
> [root@vm-033 git]# ipa host-del host.test. --updatedns
> ipa: ERROR: host.test: host not found
> 
> I think we already had a ticket to prevent this kind of error, no?

ACK as this is solved by separate patch.

-- 
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] [WIP PATCH] server-del: perform full master removal in managed topology

2016-04-13 Thread Rob Crittenden

Martin Babinsky wrote:

This is a WIP patch which moves the `ipa-replica-manage del` subcommand
to the 'server-del' API method and exposes it as CLI command[1]. A CI
test suite is also included.

There are some issues with the patch I would like to discuss in more
detail on the list:

1.) In the original subcommand there was a lot of output (mostly print
statements) during all stages of master removal. I have tried to port
these as messages to the command which results in quite voluminous
response sent back to the frontend. Should we try to reduce the output?


I don't think it applies anymore. These messages were there so the user 
would know something was happening. If it is an API command there isn't 
much we can do other than add something to the cli to print "This could 
take a bit" before making the call.



2.) In the original discussion[2] we assumed that the cleanup part would
me a separate API method called during server_del postcallback. However
since the two objects ended up sharing a lot of state (e.g. topology
state from pre-callback, messages) i have merged it to server-del. That
makes the code rather unwieldy but I found it difficult to keep the two
entities separate without some hacking around framework capabilities


I haven't looked at the code but as a general principal having separate 
operations has saved our bacon on more than one occasion.



3.) since actions in post-callback require a knowledge about topology
state gathered in pre-callback, I had to store some information in the
command's context. Sorry about that, if you know about some way to
circumvent me, let me know.


Looks like it is the only way since you are extending server_del. 
Another option would be to drop pre/post and add all this topology stuff 
directly to server_del execute.



4.) The master can not remove itself. I have implemented an ad-hoc
forwarding of the request to other master that can do the job. Is this
okay?


Assisted suicide?

What does this effectively do? Potentially disconnect it from the 
topology, perhaps to run as standalone for upgrade, integration or other 
testing (e.g. there might be valid reasons to do this)? If so that seems 
ok to me, or if too hacky you could just spit out a message directing 
them to disconnect from another host, but that would be strange in the 
UI I think.



5.) Since the original behavior of 'chekc_deleted_segments' was kept,
the code can sometimes hang waiting for removal of some segments,
especially during forced removal in wonky topologies. This can cause
gateway timeout in JSON-RPC call and report false positive error back to
the user. This makes a large part of 'TestServerDel' suite to fail. How
should we handle this? Should we keep the original behavior?


Oh to have async calls...

I wonder if "I'm still here" messages could be sent. I'm not entirely 
sure this is possible with the framework but it might be one way to keep 
long-lived connections alive.



6.) There are some in-place imports of server-side code in some places.
I'm not very happy about it and would be glad if we can agree on a way
to fix this.


Is this to prevent imports on non-servers? You might look at trust for 
inspiration, it does this.


rob



[1] https://fedorahosted.org/freeipa/ticket/5588
[2] https://www.redhat.com/archives/freeipa-devel/2016-March/msg00335.html





--
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 0459] use python-netifaces for detection of the local ip addresses

2016-04-13 Thread Martin Basti

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

Patch attached.
From 77d1ead350cd17d2c56706c9f9aa5d2d7d5da73d Mon Sep 17 00:00:00 2001
From: Martin Basti 
Date: Wed, 13 Apr 2016 16:14:42 +0200
Subject: [PATCH] Use netifaces module instead of 'ip' command

Netifaces allows to get addresses from local interfaces of the host in
safer way than parsing output of the ip command.

https://fedorahosted.org/freeipa/ticket/5591
---
 client/ipa-client-install | 45 +++--
 freeipa.spec.in   |  2 ++
 ipaplatform/base/paths.py |  1 -
 ipapython/ipautil.py  | 33 +
 4 files changed, 38 insertions(+), 43 deletions(-)

diff --git a/client/ipa-client-install b/client/ipa-client-install
index be203586a150e8fc22a42616052b95545918efb8..c38843f85639a9118cd1a471709992690643d79a 100755
--- a/client/ipa-client-install
+++ b/client/ipa-client-install
@@ -33,6 +33,7 @@ try:
 from optparse import SUPPRESS_HELP, OptionGroup, OptionValueError
 import dns
 import gssapi
+import netifaces
 
 import nss.nss as nss
 import SSSDConfig
@@ -1526,39 +1527,31 @@ def unconfigure_nisdomain():
 
 
 def get_iface_from_ip(ip_addr):
-result = ipautil.run([paths.IP, '-oneline', 'address', 'show'],
- capture_output=True)
-for line in result.output.split('\n'):
-fields = line.split()
-if len(fields) < 6:
-continue
-if fields[2] not in ['inet', 'inet6']:
-continue
-(ip, mask) = fields[3].rsplit('/', 1)
-if ip == ip_addr:
-return fields[1]
+for interface in netifaces.interfaces():
+if_addrs = netifaces.ifaddresses(interface)
+for family in [netifaces.AF_INET, netifaces.AF_INET6]:
+for ip in if_addrs.get(family, []):
+if ip['addr'] == ip_addr:
+return interface
 else:
 raise RuntimeError("IP %s not assigned to any interface." % ip_addr)
 
 
 def get_local_ipaddresses(iface=None):
-args = [paths.IP, '-oneline', 'address', 'show']
 if iface:
-args += ['dev', iface]
-result = ipautil.run(args, capture_output=True)
-lines = result.output.split('\n')
+interfaces = [iface]
+else:
+interfaces = netifaces.interfaces()
+
 ips = []
-for line in lines:
-fields = line.split()
-if len(fields) < 6:
-continue
-if fields[2] not in ['inet', 'inet6']:
-continue
-(ip, mask) = fields[3].rsplit('/', 1)
-try:
-ips.append(ipautil.CheckedIPAddress(ip))
-except ValueError:
-continue
+for interface in interfaces:
+if_addrs = netifaces.ifaddresses(interface)
+for family in [netifaces.AF_INET, netifaces.AF_INET6]:
+for ip in if_addrs.get(family, []):
+try:
+ips.append(ipautil.CheckedIPAddress(ip['addr']))
+except ValueError:
+continue
 return ips
 
 
diff --git a/freeipa.spec.in b/freeipa.spec.in
index 8582622ccc0f6d85a680b69269b045bd7c6be8c7..72ff5f63b1b3dea2678d34f787c8b2790329f3c6 100644
--- a/freeipa.spec.in
+++ b/freeipa.spec.in
@@ -500,6 +500,7 @@ Requires: python-ldap >= 2.4.15
 Requires: python-requests
 Requires: python-custodia
 Requires: python-dns >= 1.11.1
+Requires: python-netifaces >= 0.10.4
 
 Conflicts: %{alt_name}-python < %{version}
 
@@ -548,6 +549,7 @@ Requires: python3-pyldap >= 2.4.15
 Requires: python3-custodia
 Requires: python3-requests
 Requires: python3-dns >= 1.11.1
+Requires: python3-netifaces >= 0.10.4
 
 %description -n python3-ipalib
 IPA is an integrated solution to provide centrally managed Identity (users,
diff --git a/ipaplatform/base/paths.py b/ipaplatform/base/paths.py
index 375a06442ee00301712264318fece30be2b45fd3..18f717aebe4c432d8739006689672c3e6de42268 100644
--- a/ipaplatform/base/paths.py
+++ b/ipaplatform/base/paths.py
@@ -142,7 +142,6 @@ class BasePathNamespace(object):
 CACERT_P12 = "/root/cacert.p12"
 ROOT_IPA_CSR = "/root/ipa.csr"
 NAMED_PID = "/run/named/named.pid"
-IP = "/sbin/ip"
 NOLOGIN = "/sbin/nologin"
 SBIN_REBOOT = "/sbin/reboot"
 SBIN_RESTORECON = "/sbin/restorecon"
diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
index d705c51f8d1937ffe57e52cce4b590951653c37b..e595d80ca3b971ad2f0031972e9e58b5adff8e54 100644
--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -32,6 +32,7 @@ import socket
 import re
 import datetime
 import netaddr
+import netifaces
 import time
 import gssapi
 import pwd
@@ -151,24 +152,24 @@ class CheckedIPAddress(netaddr.IPAddress):
 
 if match_local:
 if addr.version == 4:
-family = 'inet'
+family = netifaces.AF_INET
 elif addr.version == 6:
-family = 'inet6'
+family = netifaces.AF_INET6
+else:
+raise Val

Re: [Freeipa-devel] [PATCH 0453 - 0458] host-del: fix updatedns option

2016-04-13 Thread Martin Basti



On 13.04.2016 17:05, Petr Spacek wrote:

On 13.4.2016 16:01, Petr Spacek wrote:

On 1.4.2016 18:30, Martin Basti wrote:

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

Patches attached.

NACK, it breaks if the client does not have any corresponding DNS record.

[root@vm-033 git]# ipa host-add host.test. --force
--
Added host "host.test"
--
   Host name: host.test
   Principal name: host/host.t...@dom-033.abc.idm.lab.eng.brq.redhat.com
   Password: False
   Keytab: False
   Managed by: host.test

[root@vm-033 git]# ipa host-del host.test. --updatedns
ipa: ERROR: host.test: host not found

I think we already had a ticket to prevent this kind of error, no?

ACK as this is solved by separate patch.


pushed to master:
* 40e3a0bf63c766fc281517c9d192907376c2d353 host_del: fix removal of host 
records
* 9a0f92be0dc1dc22827c918b5808b1ccb4e4b409 host_del: replace dns-record 
find command with show
* bea066c33647c16a7b18deb1392838acb831ac88 host_del: remove unneeded 
dnszone-show command call
* 1e70d6b914656d670f9afed26ccd5f93e3aa54d5 host_del: split removing 
A/ and PTR records to separate functions
* e8c8134eee159fa6eb7c8f2156c328798abdda80 host_del: remove only A, 
, SSHFP, PTR records
* 54e3859595e1f5f2e669b8af20afdac1187d8cd7 host_del: update help for 
--updatedns option


--
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 0453 - 0458, 0460] host-del: fix updatedns option

2016-04-13 Thread Martin Basti



On 13.04.2016 17:05, Petr Spacek wrote:

On 13.4.2016 16:31, Martin Basti wrote:


On 13.04.2016 16:01, Petr Spacek wrote:

On 1.4.2016 18:30, Martin Basti wrote:

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

Patches attached.

NACK, it breaks if the client does not have any corresponding DNS record.

[root@vm-033 git]# ipa host-add host.test. --force
--
Added host "host.test"
--
Host name: host.test
Principal name: host/host.t...@dom-033.abc.idm.lab.eng.brq.redhat.com
Password: False
Keytab: False
Managed by: host.test

[root@vm-033 git]# ipa host-del host.test. --updatedns
ipa: ERROR: host.test: host not found

I think we already had a ticket to prevent this kind of error, no?


This will be resolved in https://fedorahosted.org/freeipa/ticket/5627

Patch 460 attached, feel free to review both tickets :)
(Patch requires my previous DNS patches)

ACK


Pushed to master: b23ad42269c606f234f4f8c545e3c763e648f551

--
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 0143-0144] different errors/warnings for different LDAP limit type exceeded

2016-04-13 Thread Martin Basti



On 13.04.2016 16:48, Petr Spacek wrote:

On 8.4.2016 17:13, Martin Babinsky wrote:

On 04/01/2016 05:37 PM, Martin Babinsky wrote:

On 03/24/2016 04:14 PM, Martin Babinsky wrote:

On 03/22/2016 04:28 PM, Rob Crittenden wrote:

Martin Babinsky wrote:

On 03/21/2016 12:25 PM, Jan Cholasta wrote:

On 21.3.2016 10:17, Petr Spacek wrote:

On 18.3.2016 13:49, Rob Crittenden wrote:

Martin Babinsky wrote:

These patches implement behavior agreed upon during discussion of
https://fedorahosted.org/freeipa/ticket/5677

However I'm not sure if we want to push them into 4-3 branch (the
ticket
is triaged into 4.3.2 milestone) since they modify the framework
behavior quite a bit.

If there is no need to have it there (CC'ing Milan since he is the
reporter), I would retriage it into 4.4 milestone.


+ desc="while getting entries (search base: '{}',"
+ "filter: {})".format(base_dn, filter))

This is going to expose parts of the DIT in an error message to
users. We have
tried in the past to hide the implementation. I'd propose logging
the
error
and making the exception less verbose.

I agree with Rob here, we shouldn't expose internal stuff in error
messages for users.

In this particular case, even if we included internal stuff in the
error
message, it should be the error message returned by the server rather
than this ad-hoc message.


IMHO it actually helps to print the DN. At very least the user can
see
if the
error is happening always with the same DN or if it keeps changing.

In other words, for user it is not that important to understand
meaning of the
DN but it might be important to see if it is the same or not.

I can't imagine a situation where it would actually be useful for the
user (as opposed to the admin, who has access to logs) to know the
base
DN of some arbitrary LDAP search operation. Could you give an example?


Right, attaching updated patches.

I may have suggested debug logging the detailed error. I was wrong. This
should log at the error level so it always appears in the logs. This may
be a spurious error and having the user turn on debug logging to capture
the reasons would be asking a lot.

rob

That's right, the user would then have to enable debug mode and re-run
the command. I have changed the log level to error as you suggested.




Bump for review.


Moar bumps.

ACK, sorry for the delay!


pushed to master:
* 1f0959735f9828a09439f17f1468dcd3dfb914db differentiate between limit 
types when LDAP search exceeds configured limits
* 62bb478e112cd4677e681f4750c5f5e5c9221607 specify type of exceeded 
limit when warning about truncated search results


--
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] Locations design v2: LDAP schema & user interface

2016-04-13 Thread Jan Cholasta

On 6.4.2016 10:57, Petr Spacek wrote:

On 6.4.2016 10:50, Jan Cholasta wrote:

On 4.4.2016 13:51, Petr Spacek wrote:

On 4.4.2016 13:39, Martin Basti wrote:



On 31.03.2016 09:58, Petr Spacek wrote:

On 26.2.2016 15:37, Petr Spacek wrote:

On 25.2.2016 16:46, Simo Sorce wrote:

On Thu, 2016-02-25 at 15:54 +0100, Petr Spacek wrote:

On 25.2.2016 15:28, Simo Sorce wrote:

On Thu, 2016-02-25 at 14:45 +0100, Petr Spacek wrote:

Variant C
-
An alternative is to be lazy and dumb. Maybe it would be enough for
the first
round ...

We would retain
[first step - no change from variant A]
* create locations
* assign 'main' (aka 'primary' aka 'home') servers to locations
++ specify weights for the 'main' servers in given location, i.e.
manually
input (server, weight) tuples

Then, backups would be auto-generated set of all remaining servers
from all
other locations.

Additional storage complexity: 0

This covers the scenario "always prefer local servers and use remote
only as
fallback" easily. It does not cover any other scenario.

This might be sufficient for the first run and would allow us to
gather some
feedback from the field.

Now I'm inclined to this variant :-)

To be honest, this is all I always had in mind, for the first step.

To recap:
- define a location with the list of servers (perhaps location is a
property of server objects so you can have only one location per server,
and if you remove the server it is automatically removed from the
location w/o additional work or referential integrity necessary), if
weight is not defined (default) then they all have the same weight.

Agreed.



- Allow to specify backup locations in the location object, priorities
are calculated automatically and all backup locations have same weight.

Hmm, weights have to be inherited form the original location in all
cases. Did
you mean that all backup locations have the same *priority*?

Yes, sorry.


Anyway, explicit configuration of backup locations is introducing API and
schema for variant A and that is what I'm questioning above. It is hard to
make it extensible so we do not have headache in future when somebody
decides
that more flexibility is needed OR that link-based approach is better.

I think no matter we do we'll need to allow admins to override backup
locations, in future if we can calculate them automatically admins will
simply not set any backup location explicitly (or set some special value
like "autogenerate" and the system will do it for them.

Forcing admins to mentally calculate weights to force the system to
autogenerate the configuration they want would be a bad experience, I
personally would find it very annoying.


In other words, for doing what you propose above we would have to design
complete schema and API for variant A anyway to make sure we do not lock
ourselves, so we are not getting any saving by doing so.

A seemed much more complicated to me, as you wanted to define a ful
matrix for weights of servers when they are served as backups and all
that.


- Define a *default* location, which is the backup for any other
location but always with lower priority to any other explicitly defined
backup locations.

I would rather *always* use the default location as backup for all other
locations. It does not require any API or schema (as it equals to "all
servers" except "servers in this location" which can be easily calculated
on fly).

We can start with this, but it works well only in a stellar topology
where you have a central location all other location connect to.
As soon as you have a super-stellar topology where you have hub location
to which regional locations connect to, then this is wasteful.


This can be later on extended in whatever direction we want without any
upgrade/migration problem.

More importantly, all the schema and API will be common for all other
variants
anyway so we can start doing so and see how much time is left when it is
done.

I am ok with this for the first step.
After all location is mostly about the "normal" case where clients want
to reach the local servers, the backup part is only an additional
feature we can keep simple for now. It's a degraded mode of operation
anyway so it is probably ok to have just one default backup location as
a starting point.

Okay, now we are in agreement. I will think about minimal schema and API
over
the weekend.

Well, it took longer than one weekend.

There was couple of changes in the design document:
* ‎Feature Management: CLI proposal
* ‎Feature Management: web UI - idea with topology graph replaced original
complicated table
* Feature Management: described necessary configuration outside of IPA DNS
* Version 1 parts which were moved into separate document:
V4/DNS_Location_Mechanism_with_per_client_override
* ‎Assumptions: removed misleading reference to DHCP, clarified role of DNS
views
* Assumptions: removed misleading mention of 'different networks' and added
summary explaining how Location is defined
* Implementation: high-lev

Re: [Freeipa-devel] [WIP PATCH] server-del: perform full master removal in managed topology

2016-04-13 Thread Jan Cholasta

On 13.4.2016 17:10, Rob Crittenden wrote:

Martin Babinsky wrote:

This is a WIP patch which moves the `ipa-replica-manage del` subcommand
to the 'server-del' API method and exposes it as CLI command[1]. A CI
test suite is also included.

There are some issues with the patch I would like to discuss in more
detail on the list:

1.) In the original subcommand there was a lot of output (mostly print
statements) during all stages of master removal. I have tried to port
these as messages to the command which results in quite voluminous
response sent back to the frontend. Should we try to reduce the output?


I don't think it applies anymore. These messages were there so the user
would know something was happening. If it is an API command there isn't
much we can do other than add something to the cli to print "This could
take a bit" before making the call.


+1




2.) In the original discussion[2] we assumed that the cleanup part would
me a separate API method called during server_del postcallback. However
since the two objects ended up sharing a lot of state (e.g. topology
state from pre-callback, messages) i have merged it to server-del. That
makes the code rather unwieldy but I found it difficult to keep the two
entities separate without some hacking around framework capabilities


I haven't looked at the code but as a general principal having separate
operations has saved our bacon on more than one occasion.


The patch adds a force option, which allows you to re-run server-del 
even if the master entry does not exist anymore, so I think we are safe.





3.) since actions in post-callback require a knowledge about topology
state gathered in pre-callback, I had to store some information in the
command's context. Sorry about that, if you know about some way to
circumvent me, let me know.


Looks like it is the only way since you are extending server_del.
Another option would be to drop pre/post and add all this topology stuff
directly to server_del execute.


4.) The master can not remove itself. I have implemented an ad-hoc
forwarding of the request to other master that can do the job. Is this
okay?


Why can't the master remove itself?



Assisted suicide?

What does this effectively do? Potentially disconnect it from the
topology, perhaps to run as standalone for upgrade, integration or other
testing (e.g. there might be valid reasons to do this)? If so that seems
ok to me, or if too hacky you could just spit out a message directing
them to disconnect from another host, but that would be strange in the
UI I think.


5.) Since the original behavior of 'chekc_deleted_segments' was kept,
the code can sometimes hang waiting for removal of some segments,
especially during forced removal in wonky topologies. This can cause
gateway timeout in JSON-RPC call and report false positive error back to
the user. This makes a large part of 'TestServerDel' suite to fail. How
should we handle this? Should we keep the original behavior?


Oh to have async calls...

I wonder if "I'm still here" messages could be sent. I'm not entirely
sure this is possible with the framework but it might be one way to keep
long-lived connections alive.


Impossible, everything is synchronous.




6.) There are some in-place imports of server-side code in some places.
I'm not very happy about it and would be glad if we can agree on a way
to fix this.


Is this to prevent imports on non-servers? You might look at trust for
inspiration, it does this.


I wouldn't worry about this, I'm going to move most plugin code to 
ipaserver as part of the thin client feature anyway ;-)


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] 0051 Allow CustodiaClient to be used by arbitrary principals

2016-04-13 Thread Fraser Tweedale
On Wed, Apr 13, 2016 at 11:15:50AM +1000, Fraser Tweedale wrote:
> On Tue, Apr 12, 2016 at 09:31:30AM -0400, Simo Sorce wrote:
> > On Sat, 2016-04-09 at 10:11 +1000, Fraser Tweedale wrote:
> > > On Fri, Apr 08, 2016 at 10:47:19AM -0400, Simo Sorce wrote:
> > > > On Sat, 2016-04-09 at 00:23 +1000, Fraser Tweedale wrote:
> > > > > -name = gssapi.Name('host@%s' % (self.client,),
> > > > > 
> > > > > -   gssapi.NameType.hostbased_service)
> > > > 
> > > > If you remove this then on a serve that has nfs keys in the keytab you
> > > > may end up acquiring the wrong credentials.
> > > > You need to pass down what credentials you want to use to initialize the
> > > > cred store, we canot rely on ordering in the system keytab case.
> > > > 
> > > > Simo.
> > > > 
> > > Thanks Simo; updated patch attached.
> > 
> > Except the ACI the rest looks good to me.
> > For ACI please add a separate patch that follows the naming scheme for
> > subCA keys.
> > 
> The ACI here targets the Custodia server public keys, so the client
> can search and read them.  It should just read:
> 
> add:aci: (target = "ldap:///cn=*,cn=custodia,cn=ipa,cn=etc,$SUFFIX";)
> (targetattr = "ipaPublicKey || ipaKeyUsage || memberPrincipal")
> (version 3.0; acl "Anyone can search Custodia public keys";
> allow(read, search, compare) userdn = "ldap:///all";;)
> 
> I don't mind putting the ACI in a separate patch, but it is
> necessary to restrict read access on the public keys to only the
> dogtag-ipa-custodia service principals.
> 
Updated patches attached.  ACI was split into new patch and
simplified (removed ($dn) macro).

Cheers,
Fraser
From 1f1193b3a4e786c63bde4fa0abe3640c16481633 Mon Sep 17 00:00:00 2001
From: Fraser Tweedale 
Date: Fri, 8 Apr 2016 15:21:19 +1000
Subject: [PATCH] Allow CustodiaClient to be used by arbitrary principals

Currently CustodiaClient assumes that the client is the host
principal, and it is hard-coded to read the host keytab and server
keys.

For the Lightweight CAs feature, Dogtag on CA replicas will use
CustodiaClient to retrieve signing keys from the originating
replica.  Because this process runs as 'pkiuser', the host keys
cannot be used; instead, each Dogtag replica will have a service
principal to use for Custodia authentication.

Update CustodiaClient to allow specifying the keytab and Custodia
keyfile to use.  Avoid hard-coding the service name to find in the
keytab.

Part of: https://fedorahosted.org/freeipa/ticket/4559
---
 ipapython/secrets/client.py | 17 ++---
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/ipapython/secrets/client.py b/ipapython/secrets/client.py
index 
5b671988ddc66eedd9ae1cd4ddec0e1308bc5a93..a15057ae67c377a782db3642d14384e0bf11b5a2
 100644
--- a/ipapython/secrets/client.py
+++ b/ipapython/secrets/client.py
@@ -41,16 +41,19 @@ class CustodiaClient(object):
 
 return iSecStore(config)
 
-def __init__(self, client, server, realm, ldap_uri=None, auth_type=None):
-self.client = client
-self.creds = None
+def __init__(self, client, server, realm, ldap_uri=None, auth_type=None,
+client_servicename='host', keyfile=None, keytab=None):
+self.client_service = '%s@%s' % (client_servicename, client)
+self.keytab = keytab or paths.KRB5_KEYTAB
+self.creds = self.init_creds()
 
 self.service_name = gssapi.Name('HTTP@%s' % (server,),
 gssapi.NameType.hostbased_service)
 self.server = server
 
-keyfile = os.path.join(paths.IPA_CUSTODIA_CONF_DIR, 'server.keys')
-self.ikk = IPAKEMKeys({'server_keys': keyfile})
+if keyfile is None:
+keyfile = os.path.join(paths.IPA_CUSTODIA_CONF_DIR, 'server.keys')
+self.ikk = IPAKEMKeys({'server_keys': keyfile, 'ldap_uri': ldap_uri})
 
 self.kemcli = KEMClient(self._server_keys(server, realm),
 self._client_keys())
@@ -61,9 +64,9 @@ class CustodiaClient(object):
 requests.packages.urllib3.disable_warnings()
 
 def init_creds(self):
-name = gssapi.Name('host@%s' % (self.client,),
+name = gssapi.Name(self.client_service,
gssapi.NameType.hostbased_service)
-store = {'client_keytab': paths.KRB5_KEYTAB,
+store = {'client_keytab': self.keytab,
  'ccache': 'MEMORY:Custodia_%s' % b64encode(os.urandom(8))}
 return gssapi.Credentials(name=name, store=store, usage='initiate')
 
-- 
2.5.5

From 234e6334dafb27224b89fed86968df3016b27474 Mon Sep 17 00:00:00 2001
From: Fraser Tweedale 
Date: Wed, 13 Apr 2016 14:51:16 +1000
Subject: [PATCH] Allow all principals to read Custodia keys

Add an ACI to allow all authenticated principals to read and search
for Custodia server public keys.

Part of: https://fedorahosted.org/freeipa/ticket/4559
---
 install/updates/20-aci.update | 3 +++
 1 file changed, 3 insertions(+)

diff 

Re: [Freeipa-devel] [WIP PATCH] server-del: perform full master removal in managed topology

2016-04-13 Thread Jan Cholasta

Hi,

On 13.4.2016 16:49, Martin Babinsky wrote:

This is a WIP patch which moves the `ipa-replica-manage del` subcommand
to the 'server-del' API method and exposes it as CLI command[1]. A CI
test suite is also included.



`server-del` now accepts the following options:
* `--cleanup`: perform a cleanup after an already deleted master


I would prefer if this was actually called --force, for reasons 
explained in the design thread: 
.



* `--force-removal`: force master removal, i.e. ignore topology errors


So, this is actually the all-powerful --force option we always try to 
avoid, but with a different name (and not very good one - if you are 
removing something, what other than removal would you need to force?).


Could you split this into separate options?

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


[Freeipa-devel] [PATCH] 0053..0054 Configure lightweight CA key replication

2016-04-13 Thread Fraser Tweedale
Hi all,

The attached patches configure lightweight CA key replication on IPA
CAs, on upgrade and installation.

Patches 0051..0052 from my other mail are also needed for the system
to work, but this patchset does not depend on them and can be
reviewed independently.

There is also no hard dependency on the (unreleased) Dogtag 10.3.0b1
- it just puts the necessary principals/keys/configuration in place.

Cheers,
Fraser
From aa91bd3c6773d42c864a8f34eabad8b90bb01f8b Mon Sep 17 00:00:00 2001
From: Fraser Tweedale 
Date: Mon, 11 Apr 2016 12:42:35 +1000
Subject: [PATCH 53/54] Optionally add service name to Custodia key DNs

Lightweight CAs support introduces new service principals for
Dogtag, with Custodia keys.  The current Custodia key creation uses
a DN that contains only they key type and the hostname, so keys for
multiple services on the same host cannot be created.

Add the 'generate_keys' method to generate keys for a host or an
arbitrary service.  When a service name is given, include the
service name in the DN.

This change does not affect searching because all searching is done
using the ipaKeyUsage and memberPrincipal attributes.

Part of: https://fedorahosted.org/freeipa/ticket/4559
---
 ipapython/secrets/kem.py | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/ipapython/secrets/kem.py b/ipapython/secrets/kem.py
index 
1025ed7980f055c82c602634e8845fa490cf0514..533121779241d30e19fef4c050bb69c55d29ec22
 100644
--- a/ipapython/secrets/kem.py
+++ b/ipapython/secrets/kem.py
@@ -105,10 +105,11 @@ class KEMLdap(iSecLdap):
 encoding=serialization.Encoding.DER,
 format=serialization.PublicFormat.SubjectPublicKeyInfo)
 
-def set_key(self, usage, host, principal, key):
+def set_key(self, usage, servicename, host, principal, key):
 public_key = self._format_public_key(key)
 conn = self.connect()
-name = '%s/%s' % (KEY_USAGE_MAP[usage], host)
+service_segment = '~' + servicename if servicename else ''
+name = '%s%s/%s' % (KEY_USAGE_MAP[usage], service_segment, host)
 dn = 'cn=%s,%s' % (name, self.keysbase)
 try:
 mods = [('objectClass', ['nsContainer',
@@ -170,15 +171,18 @@ class IPAKEMKeys(KEMKeysStore):
 return conn.get_key(usage, kid)
 
 def generate_server_keys(self):
-principal = 'host/%s@%s' % (self.host, self.realm)
+self.generate_keys()
+
+def generate_keys(self, servicename=None):
+principal = '%s/%s@%s' % (servicename or 'host', self.host, self.realm)
 # Neutralize the key with read if any
 self._server_keys = None
 # Generate private key and store it
 pubkeys = newServerKeys(self.config['server_keys'], principal)
 # Store public key in LDAP
 ldapconn = KEMLdap(self.ldap_uri)
-ldapconn.set_key(KEY_USAGE_SIG, self.host, principal, pubkeys[0])
-ldapconn.set_key(KEY_USAGE_ENC, self.host, principal, pubkeys[1])
+ldapconn.set_key(KEY_USAGE_SIG, servicename, self.host, principal, 
pubkeys[0])
+ldapconn.set_key(KEY_USAGE_ENC, servicename, self.host, principal, 
pubkeys[1])
 
 @property
 def server_keys(self):
-- 
2.5.5

From cf452abcf309ce3cded3eeef70b07f108d9eb92d Mon Sep 17 00:00:00 2001
From: Fraser Tweedale 
Date: Mon, 11 Apr 2016 16:47:33 +1000
Subject: [PATCH 54/54] Setup lightweight CA key retrieval on install/upgrade

To configure Dogtag lightweight CA key replication on installation
and upgrade:

- add the 'dogtag-ipa-custodia/$HOSTNAME' service principal
- create Custodia keys
- retrieve keytab
- configure the IPACustodiaKeyRetriever in CS.cfg

Part of: https://fedorahosted.org/freeipa/ticket/4559
---
 install/tools/ipa-ca-install|  4 
 ipaserver/install/cainstance.py | 43 +
 ipaserver/install/server/install.py |  8 ++-
 ipaserver/install/server/upgrade.py |  6 +-
 4 files changed, 59 insertions(+), 2 deletions(-)

diff --git a/install/tools/ipa-ca-install b/install/tools/ipa-ca-install
index 
1bc5def03bf687a1e4f9fb38a54363b5429c8fc4..0af5b39116b4649423ed2a51579e2adc767d802b
 100755
--- a/install/tools/ipa-ca-install
+++ b/install/tools/ipa-ca-install
@@ -226,6 +226,10 @@ def install_master(safe_options, options):
 ca.install_check(True, None, options)
 ca.install(True, None, options)
 
+CA = cainstance.CAInstance(
+api.env.realm, certs.NSS_DIR, host_name=api.env.host)
+CA.setup_lightweight_ca_key_retrieval()
+
 
 def install(safe_options, options, filename):
 options.promote = False
diff --git a/ipaserver/install/cainstance.py b/ipaserver/install/cainstance.py
index 
a21f7d2671461dfb99797d39fc7ee5706317241f..08d0ec661a5b0a95cfc4f8ac8ccc1476d86e6ea0
 100644
--- a/ipaserver/install/cainstance.py
+++ b/ipaserver/install/cainstance.py
@@ -59,6 +59,7 @@ from ipapython.certdb import get_ca_nickname
 from ipapython.dn import DN
 from ipapython.ipa_log_manager

Re: [Freeipa-devel] [DESIGN] Sub-CAs; authenticating to Custodia

2016-04-13 Thread Jan Cholasta

On 7.4.2016 16:17, Petr Spacek wrote:

On 7.4.2016 15:20, Fraser Tweedale wrote:

On Thu, Apr 07, 2016 at 12:29:00PM +0200, Jan Cholasta wrote:

On 7.4.2016 12:13, Christian Heimes wrote:

On 2016-04-07 11:09, Petr Spacek wrote:

On 7.4.2016 08:43, Fraser Tweedale wrote:

Hi team,

I updated the Sub-CAs design page with more detail for the key
replication[1].  This part of the design is nearly complete (a large
patchset is in review over at pki-devel@) but there are various
options about how to authenticate to Custodia.

[1] http://www.freeipa.org/page/V4/Sub-CAs#Key_replication

In brief, the options are:

1) authenticate as host principal; install binary setuid
root:pkiuser to read host keytab and custodia keys.


Huh, I really do not like this. Host keytab on IPA master is one of the most
sensitive keys we have.

Maybe gssproxy can be used somehow, but I think it would be better to use
separate key.



2) authenticate as host principal; copy host keytab and custodia
keys to location readable by pkiuser.


No, really, do not copy host keytab anywhere.



3) create new principal for pkiuser to use, along with custodia keys
and keytab in location readable by pkiuser.

I prefer option (1) for reasons outlined in the design page.  The
design page goes into quite a bit more detail so please review the
section linked above and get back to me with your thoughts.


The only downside of (3) using new keys is:
... This approach requires the creation of new principals, and Kerberos
keytabs and Custodia keys for those principals, as part of the
installation/upgrade process.

Compared with additional SUID binary this seems as safer and easier way to go.
FreeIPA installers already create quite a lot of principals and keytabs so
this is well understood task.

I would do (3).


+1 for (3)

A SUID binary feels like a dangerous hack.


+1


OK, (3) it is.  Thanks all for your input.

Now for next question: what should service principal name be?  I
think `dogtag/example@example.com' but am open to other
suggestions, e.g. `pki/...'.


Do you plan to attempt to standardize this name in future? I do not expect that.

Considering private nature of it, it should be as specific as possible to
avoid any potential conflicts with future standards. "dogtag-key-replication"
sounds like a good candidate.


IMO it shouldn't be *that* specific, considering we want to switch 
Dogtag from SSL to GSSAPI authentication to DS, which will probably use 
the same principal name. I think "ipa-pki/..." or "dogtag/..." should be 
fine.




Before you set the name in stone make sure it does not conflict with anything
listed on
http://www.iana.org/assignments/service-names-port-numbers/service-names-port-numbers.xhtml

These names have potential to be used by someone else.


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