Re: [Freeipa-devel] [PATCHES] 267-294 Support multiple CA certificates in LDAP

2014-06-13 Thread Martin Kosek
On 06/12/2014 07:45 PM, Jan Cholasta wrote:
...
 Note that automatic distribution of CA certificates to IPA systems is not
 implemented yet (it's planned for IPA 4.2, see
 https://fedorahosted.org/freeipa/ticket/4322), so /etc/ipa/ca.crt,
 /etc/pki/nssdb, /etc/dirsrv/slapd-REALM and /etc/httpd/alias are updated 
 *only*
 during client/server install.
 
 Honza

For 4.0, we will need to come up with manual procedure how to renew the
certificates *without* reinstalling the client or server.

I think the best way would be to prepare a simple script to renew
client/server, something like

/usr/share/ipa/ipa-renew-client-certificate
/usr/share/ipa/ipa-renew-server-certificate

and refer to it in the ipa-cacert-manage man page. People could then pretty
easily run those after a cert change, using whatever means their infrastructure
uses - puppet, ssh, ...

Martin

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] [PATCH 0224] cainstance: Read CS.cfg for preop.pin in a loop

2014-06-13 Thread Martin Kosek
On 06/12/2014 06:22 PM, Nathaniel McCallum wrote:
 On Thu, 2014-06-12 at 17:07 +0200, Tomas Babej wrote:
 On 06/12/2014 04:45 PM, Nathaniel McCallum wrote:
 On Thu, 2014-06-12 at 16:36 +0200, Tomas Babej wrote:
 On 06/12/2014 04:27 PM, Nathaniel McCallum wrote:
 On Thu, 2014-06-12 at 16:20 +0200, Martin Kosek wrote:
 On 06/12/2014 03:15 PM, Tomas Babej wrote:
 On 06/12/2014 02:37 PM, Nathaniel McCallum wrote:
 On Thu, 2014-06-12 at 13:29 +0200, Tomas Babej wrote:
 On 06/12/2014 10:45 AM, Martin Kosek wrote:
 On 06/11/2014 06:49 PM, Nathaniel McCallum wrote:
 On Wed, 2014-06-11 at 11:08 +0200, Tomas Babej wrote:
 Hi,

 As due to possible race conditions, the preop.pin might not be
 written in the CS.cfg at the time installer tries to read it.

 In case no value for preop.pin was found, retry until timeout
 was reached.

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

 (applies on ipa-3-0 branch)
 There is inconsistent spacing around '='. For instance:
 +f=open(filename, 'r')
 +data = f.read()

 Also, I'm fairly certain this is incorrect:
 +total_timeout =+ 1

 Nathaniel
 +1, this is certainly is not a deterministic, constant measuring of 
 a timeout.
 Fixed.

 I would rather see something like

 op_timeout = time.time() + api.env.startup_timeout

 while preop_pin is None and time.time()  op_timeout:


 Also, I do not think this will work, IOError is risen when a file is 
 not found,
 so this code would not solve the problem if waiting on file to 
 appear.
 Yes, but the problem was not in being unable to open the file, but 
 that
 certain file content
 is not in the file yet. It seems that pkicreate creates the file in
 time, but the content written
 later and that causes the race condition.

 If you inspect the original code, and bugzilla, you can see that
 installation fails with:

 2013-01-25T02:58:44Z INFO The ipa-server-install command failed, 
 exception: 
 RuntimeError: Unable to find preop.pin in 
 /var/lib/pki-ca/conf/CS.cfg. Is your CA already configured?

 While the original code

 # read the config file and get the preop pin

 try:  
  


 f=open(filename)  
  

 except IOError,
 e:
  

 root_logger.error(Cannot open configuration file. +
 str(e))  
 raise
 e 


 data =
 f.read()  
   

 data =
 data.split('\n')  
   

 pattern = re.compile(preop.pin=(.*)
 )
 for line in
 data: 
  

 match = re.search(pattern,
 line) 
   
 if
 (match):  
   


 preop_pin=match.group(1)  
  

 break 
 if preop_pin is None:
 raise RuntimeError(Unable to find preop.pin in %s. Is your CA
 already configured? % filename)
  
 does raise the IOError in case the file was not able to be opened. 
 Since
 we get the Runtime error,
 file must exist, only the desired content is missing.

 That said, I have no objections to amending the patch so that it
 properly handles this race condition
 we did not encounter. Better safe than sorry, it's a simple change and
 already included in the
 current iteration of the patch.

 Updated patch attached.
 I think I would prefer something like inotify watching for
 IN_CLOSE_WRITE rather than this polling.

 Nathaniel

 Wouldn't that be a little bit of an overkill for that purpose?
 If would. Nathaniel, this is a fix for ipa-3-0 branch only, we do not 
 have this
 problem in current master as this issue only affects Dogtag 9 
 installation.

 The target is to have small, contained and the non-intrusive patch. Thus 
 the
 proposed solution to just implement a little wait loop.

 - we'll need to introduce an additional dependency for python-inotify
 - watching for IN_CLOSE_WRITE is not equivalent with the preop_pin
 written to CS.cfg (and we don't know that unless we inspect the code for
 pkicreate, which in turn would make our code dependant on code not
 located in our codebase), so we will still need to check if preop_pin is
 there, and loop over if not
 Sorry, I forgot this was for 3.0 branch.

 Nitpick: '=' spacing is still not fixed.
 I fixed 

[Freeipa-devel] [PATCHES 0066-0067] Upgrade procedure for forwardzones

2014-06-13 Thread Martin Basti
Patches attached, require patches mbasti 0052-0055.
-- 
Martin^2 Basti
From 4d7025f5bd5f3d069dda2da6d4795d3796778c5f Mon Sep 17 00:00:00 2001
From: Martin Basti mba...@redhat.com
Date: Fri, 13 Jun 2014 10:15:23 +0200
Subject: [PATCH 1/2] Added upgrade step executed before schmema is upgraded

Class PreSchemaUpdate is executed before ldap schema update

This is required by ticket: https://fedorahosted.org/freeipa/ticket/3210
---
 ipaserver/install/ipa_ldap_updater.py   | 14 --
 ipaserver/install/ldapupdate.py | 15 ++-
 ipaserver/install/plugins/__init__.py   |  5 +++--
 ipaserver/install/plugins/baseupdate.py | 13 -
 ipaserver/install/upgradeinstance.py| 17 +
 5 files changed, 54 insertions(+), 10 deletions(-)

diff --git a/ipaserver/install/ipa_ldap_updater.py b/ipaserver/install/ipa_ldap_updater.py
index d894b302407a7f431102ce5cff2fed3cbed5ed47..cecc4ab308c4ab4046fcd8e728648c913baf364f 100644
--- a/ipaserver/install/ipa_ldap_updater.py
+++ b/ipaserver/install/ipa_ldap_updater.py
@@ -190,12 +190,6 @@ class LDAPUpdater_NonUpgrade(LDAPUpdater):
 
 modified = False
 
-if options.update_schema:
-modified = schemaupdate.update_schema(
-options.schema_files,
-dm_password=self.dirman_password,
-live_run=not options.test) or modified
-
 ld = LDAPUpdate(
 dm_password=self.dirman_password,
 sub_dict={},
@@ -203,6 +197,14 @@ class LDAPUpdater_NonUpgrade(LDAPUpdater):
 ldapi=options.ldapi,
 plugins=options.plugins or self.run_plugins)
 
+modified = ld.pre_schema_update(ordered=True)
+
+if options.update_schema:
+modified = schemaupdate.update_schema(
+options.schema_files,
+dm_password=self.dirman_password,
+live_run=not options.test) or modified
+
 if not self.files:
 self.files = ld.get_all_files(UPDATES_DIR)
 
diff --git a/ipaserver/install/ldapupdate.py b/ipaserver/install/ldapupdate.py
index a9167aeeed562732e66fd76e2410dfc62a4b145e..cdfcda9ee387423c36e263bc1d72d3c20f0282b7 100644
--- a/ipaserver/install/ldapupdate.py
+++ b/ipaserver/install/ldapupdate.py
@@ -43,7 +43,8 @@ from ipalib import errors
 from ipalib import api
 from ipapython.dn import DN
 from ipapython.ipa_log_manager import *
-from ipaserver.install.plugins import PRE_UPDATE, POST_UPDATE
+from ipaserver.install.plugins import (PRE_UPDATE, POST_UPDATE,
+   PRE_SCHEMA_UPDATE)
 from ipaserver.plugins import ldap2
 
 
@@ -793,6 +794,18 @@ class LDAPUpdate:
 for dn, update in sorted_updates:
 self._delete_record(update)
 
+def pre_schema_update(self, ordered=False):
+Execute the update before the LDPA schema is updated.
+
+if self.plugins:
+self.info('PRE_SCHEMA_UPDATE')
+all_updates = {}
+updates = api.Backend.updateclient.update(PRE_SCHEMA_UPDATE, self.dm_password, self.ldapi, self.live_run)
+self.merge_updates(all_updates, updates)
+self._run_updates(all_updates)
+
+return self.modified
+
 def update(self, files, ordered=False):
 Execute the update. files is a list of the update files to use.
 
diff --git a/ipaserver/install/plugins/__init__.py b/ipaserver/install/plugins/__init__.py
index 49bef4df80d9b8ea2f5861dcb69c7ae2fb882472..296831db754b45121b6c7260777c151827a95467 100644
--- a/ipaserver/install/plugins/__init__.py
+++ b/ipaserver/install/plugins/__init__.py
@@ -20,8 +20,9 @@
 
 Provide a separate api for updates.
 
-PRE_UPDATE = 1
-POST_UPDATE = 2
+PRE_SCHEMA_UPDATE = 1
+PRE_UPDATE = 2
+POST_UPDATE = 3
 
 FIRST = 1
 MIDDLE = 2
diff --git a/ipaserver/install/plugins/baseupdate.py b/ipaserver/install/plugins/baseupdate.py
index a480a8ee289646374af33f98dcf1f6978a969aa5..b0f7b7b600231314b7e3c008a86e20cf8fed4288 100644
--- a/ipaserver/install/plugins/baseupdate.py
+++ b/ipaserver/install/plugins/baseupdate.py
@@ -20,7 +20,8 @@
 from ipalib import api
 from ipalib import Updater, Object
 from ipaserver.install import service
-from ipaserver.install.plugins import PRE_UPDATE, POST_UPDATE, MIDDLE
+from ipaserver.install.plugins import (PRE_UPDATE, POST_UPDATE,
+   PRE_SCHEMA_UPDATE, MIDDLE)
 
 class DSRestart(service.Service):
 
@@ -55,6 +56,16 @@ class update(Object):
 
 api.register(update)
 
+class PreSchemaUpdate(Updater):
+
+Base class for updates that run after file processing.
+
+updatetype = PRE_SCHEMA_UPDATE
+order = MIDDLE
+
+def __init__(self):
+super(PreSchemaUpdate, self).__init__()
+
 class PreUpdate(Updater):
 
 Base class for updates that run prior to file processing.
diff --git a/ipaserver/install/upgradeinstance.py b/ipaserver/install/upgradeinstance.py
index 

Re: [Freeipa-devel] [PATCH] 0571 ipalib.frontend: Do API version check before converting arguments

2014-06-13 Thread Martin Kosek
On 06/09/2014 01:21 PM, Petr Viktorin wrote:
 On 06/09/2014 12:22 PM, Petr Viktorin wrote:
 Hello,
 This fixes https://fedorahosted.org/freeipa/ticket/3963 for master. I'll
 make a slightly less invasive patch for 3.x.

 Basically, the version argument is now expected for all commands
 (including e.g. ping  env), and also when calling the commands in_server.
 If it's not given, a message is returned, so this should should only
 really affect tests that check the output strictly.

 
 For the 3.x maintenance branches, here's a much smaller fix. This only checks
 the version if it was given.

Worked fine - tested on RHEL-6.5.

Pushed to:
ipa-3-0: 220539a3653b15e4f5679b53cab8e601abaf8990
ipa-3-1: 98f5abe37461844b42989766caee525c0d8864f8
ipa-3-2: b4d2637fc43798669b8ea1bc6fe0f851fd30401a
ipa-3-3: 7486140e00c2f1e119250fb69040864fa902290d

Martin

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


[Freeipa-devel] [PATCHES] 0581-0582 ipalib.config: Only convert numeric values to float

2014-06-13 Thread Petr Viktorin

First patch: minor fix in env loading

Second patch:

When api.env is loaded, strings that look like floats get 
auto-converted to floats. This is wrong, as the conversion can lose 
precision, which matters for the new api_version.


Here's a patch that disables this conversion, and fixes places that the 
disabling might break.


--
Petr³
From 47b6db3e91a16b6598c655edf17d1533b5e3dcc8 Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Fri, 13 Jun 2014 12:40:32 +0200
Subject: [PATCH] ipalib.config: Only convert basedn to DN

The current code would convert values to DN if the key was
a substring of 'basedn', e.g. 'base' or 'sed'.

Only convert if we're actually dealing with 'basedn'.
---
 ipalib/config.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ipalib/config.py b/ipalib/config.py
index f86c0a5ea3885d2bf89712f91b0c0705dceedd32..709e067416046b2c5174554043be87fa27042bf9 100644
--- a/ipalib/config.py
+++ b/ipalib/config.py
@@ -257,7 +257,7 @@ def __setitem__(self, key, value):
 value = m[value]
 elif value.isdigit():
 value = int(value)
-elif key in ('basedn'):
+elif key == 'basedn':
 value = DN(value)
 else:
 try:
-- 
1.9.0

From bb3d1e766140b0ed1cbf37562080070cf46cab1e Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Fri, 13 Jun 2014 12:47:48 +0200
Subject: [PATCH] ipalib.config: Don't autoconvert values to float

When api.env is loaded, strings that look like floats got
auto-converted to floats.
This is wrong, as the conversion to float can lose precision.
Case in point: the api_version (e.g. '2.88') should never be
interpreted as float.

Do not automatically convert to float.

We have two numeric options: startup_timeout and wait_for_dns.
wait_for_dns is already converted to int when used in the code.
Convert startup_timeout to float explicitly when used, so
configuration that specified it with a decimal point continues
to work.
---
 ipalib/config.py   | 5 -
 ipapython/ipautil.py   | 2 ++
 ipapython/platform/fedora16/service.py | 2 +-
 ipatests/test_ipalib/test_config.py| 5 ++---
 4 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/ipalib/config.py b/ipalib/config.py
index 709e067416046b2c5174554043be87fa27042bf9..b12cfd32184edf964353fda9304dbb5149eb525f 100644
--- a/ipalib/config.py
+++ b/ipalib/config.py
@@ -259,11 +259,6 @@ def __setitem__(self, key, value):
 value = int(value)
 elif key == 'basedn':
 value = DN(value)
-else:
-try:
-value = float(value)
-except (TypeError, ValueError):
-pass
 assert type(value) in (unicode, int, float, bool, NoneType, DN)
 object.__setattr__(self, key, value)
 self.__d[key] = value
diff --git a/ipapython/ipautil.py b/ipapython/ipautil.py
index 844dbb68792c483552149be7ae442a1e40eb9626..d95983b208f47ff42dfc254248e2f4f03d372bff 100644
--- a/ipapython/ipautil.py
+++ b/ipapython/ipautil.py
@@ -1135,6 +1135,7 @@ def wait_for_open_ports(host, ports, timeout=0):
 in seconds may be specified to limit the wait. If the timeout is
 exceeded, socket.timeout exception is raised.
 
+timeout = float(timeout)
 if not isinstance(ports, (tuple, list)):
 ports = [ports]
 
@@ -1156,6 +1157,7 @@ def wait_for_open_socket(socket_name, timeout=0):
 Wait until the specified socket on the local host is open. Timeout
 in seconds may be specified to limit the wait.
 
+timeout = float(timeout)
 op_timeout = time.time() + timeout
 
 while True:
diff --git a/ipapython/platform/fedora16/service.py b/ipapython/platform/fedora16/service.py
index 41c241ae5c31df56544b5b2bebd71c5ef109dd6e..86403d82583ed1e70044ce788d7ead7a5f3544a1 100644
--- a/ipapython/platform/fedora16/service.py
+++ b/ipapython/platform/fedora16/service.py
@@ -152,7 +152,7 @@ def __wait_until_running(self):
 'The httpd proxy is not installed, wait on local port')
 use_proxy = False
 root_logger.debug('Waiting until the CA is running')
-timeout = api.env.startup_timeout
+timeout = float(api.env.startup_timeout)
 op_timeout = time.time() + timeout
 while time.time()  op_timeout:
 try:
diff --git a/ipatests/test_ipalib/test_config.py b/ipatests/test_ipalib/test_config.py
index f896b893601c3ce85213cac39338095d7ac946f7..e04dd953074342f09b0ca4ca6dbea37eb37aaf48 100644
--- a/ipatests/test_ipalib/test_config.py
+++ b/ipatests/test_ipalib/test_config.py
@@ -43,8 +43,7 @@
 ('trailing_whitespace', u' value  ', u'value'),
 ('an_int', 42, 42),
 ('int_repr', ' 42 ', 42),
-('a_float', 3.14, 3.14),
-('float_repr', ' 3.14 ', 3.14),
+('not_a_float', '3.14', u'3.14'),
 ('true', True, True),
 ('true_repr', ' True ', 

Re: [Freeipa-devel] [PATCH] 0571 ipalib.frontend: Do API version check before converting arguments

2014-06-13 Thread Petr Viktorin

On 06/13/2014 10:41 AM, Martin Kosek wrote:

On 06/09/2014 12:22 PM, Petr Viktorin wrote:

Hello,
This fixes https://fedorahosted.org/freeipa/ticket/3963 for master. I'll make a
slightly less invasive patch for 3.x.

Basically, the version argument is now expected for all commands (including
e.g. ping  env), and also when calling the commands in_server.
If it's not given, a message is returned, so this should should only really
affect tests that check the output strictly.



Functionally works fine, tested with curl. I see one issue in the test that may
be related:

==
FAIL: Test the `ipalib.config.Env._finalize_core` method.
--
Traceback (most recent call last):
   File /usr/lib/python2.7/site-packages/nose/case.py, line 197, in runTest
 self.test(*self.arg)
   File /root/freeipa-master/ipatests/test_ipalib/test_config.py, line 572, in
test_finalize_core
 assert o[key] == value, '%r is %r; should be %r' % (key, o[key], value)
AssertionError: 'api_version' is 2.88; should be u'2.88'


That's a different issue; the test's been broken since 2a8c509. I've 
been meaning to get to it for a while.

Addressed in my patch 0582.


--
Petr³

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] [PATCHES] 267-294 Support multiple CA certificates in LDAP

2014-06-13 Thread Simo Sorce
On Fri, 2014-06-13 at 09:05 +0200, Martin Kosek wrote:
 On 06/12/2014 07:45 PM, Jan Cholasta wrote:
 ...
  Note that automatic distribution of CA certificates to IPA systems is not
  implemented yet (it's planned for IPA 4.2, see
  https://fedorahosted.org/freeipa/ticket/4322), so /etc/ipa/ca.crt,
  /etc/pki/nssdb, /etc/dirsrv/slapd-REALM and /etc/httpd/alias are updated 
  *only*
  during client/server install.
  
  Honza
 
 For 4.0, we will need to come up with manual procedure how to renew the
 certificates *without* reinstalling the client or server.
 
 I think the best way would be to prepare a simple script to renew
 client/server, something like
 
 /usr/share/ipa/ipa-renew-client-certificate
 /usr/share/ipa/ipa-renew-server-certificate

I assume you mean /usr/bin or /usr/libexec/ipa ?

 and refer to it in the ipa-cacert-manage man page. People could then pretty
 easily run those after a cert change, using whatever means their 
 infrastructure
 uses - puppet, ssh, ...


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

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] [PATCHES] 267-294 Support multiple CA certificates in LDAP

2014-06-13 Thread Martin Kosek
On 06/13/2014 02:55 PM, Simo Sorce wrote:
 On Fri, 2014-06-13 at 09:05 +0200, Martin Kosek wrote:
 On 06/12/2014 07:45 PM, Jan Cholasta wrote:
 ...
 Note that automatic distribution of CA certificates to IPA systems is not
 implemented yet (it's planned for IPA 4.2, see
 https://fedorahosted.org/freeipa/ticket/4322), so /etc/ipa/ca.crt,
 /etc/pki/nssdb, /etc/dirsrv/slapd-REALM and /etc/httpd/alias are updated 
 *only*
 during client/server install.

 Honza

 For 4.0, we will need to come up with manual procedure how to renew the
 certificates *without* reinstalling the client or server.

 I think the best way would be to prepare a simple script to renew
 client/server, something like

 /usr/share/ipa/ipa-renew-client-certificate
 /usr/share/ipa/ipa-renew-server-certificate
 
 I assume you mean /usr/bin or /usr/libexec/ipa ?

Right, that's better. I think we do not want to store it in /usr/bin as fully
supported scripts as I would feel obliged to keep that scripts supported and
around even when automatic renewal is available in FreeIPA 4.2.

So maybe /usr/libexec/ipa would be better.

 
 and refer to it in the ipa-cacert-manage man page. People could then pretty
 easily run those after a cert change, using whatever means their 
 infrastructure
 uses - puppet, ssh, ...

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] [PATCHES] 267-294 Support multiple CA certificates in LDAP

2014-06-13 Thread Rob Crittenden
Martin Kosek wrote:
 On 06/13/2014 02:55 PM, Simo Sorce wrote:
 On Fri, 2014-06-13 at 09:05 +0200, Martin Kosek wrote:
 On 06/12/2014 07:45 PM, Jan Cholasta wrote:
 ...
 Note that automatic distribution of CA certificates to IPA systems is not
 implemented yet (it's planned for IPA 4.2, see
 https://fedorahosted.org/freeipa/ticket/4322), so /etc/ipa/ca.crt,
 /etc/pki/nssdb, /etc/dirsrv/slapd-REALM and /etc/httpd/alias are updated 
 *only*
 during client/server install.

 Honza

 For 4.0, we will need to come up with manual procedure how to renew the
 certificates *without* reinstalling the client or server.

 I think the best way would be to prepare a simple script to renew
 client/server, something like

 /usr/share/ipa/ipa-renew-client-certificate
 /usr/share/ipa/ipa-renew-server-certificate

 I assume you mean /usr/bin or /usr/libexec/ipa ?
 
 Right, that's better. I think we do not want to store it in /usr/bin as fully
 supported scripts as I would feel obliged to keep that scripts supported and
 around even when automatic renewal is available in FreeIPA 4.2.
 
 So maybe /usr/libexec/ipa would be better.

I guess it depends on what our expectations of user's running this are.

If it is basically sample code, then yeah, /usr/share may be ok. If it's
something supported we expect some people to run, /usr/[s]bin is
probably the place. /usr/libexec is for binaries run by other programs IIRC.

rob

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


[Freeipa-devel] [PATCHES] 0583-0584 Convert DNS default permissions to managed

2014-06-13 Thread Petr Viktorin


With the first patch, old SYSTEM permissions can be replaced. The Read 
DNS Entries did not have an associated ACI, but was rather rolled into 
a single ACI with the managedBy rule used for per-zone access.

(and before that it was part of a deny rule.)
We can't remove this permission in an update file, because we need to 
check that it is indeed an old SYSTEM perm and not a new one with the 
same name.



The second patch converts DNS permissions to managed.

The ACIs are put directly in $SUFFIX, because the cn=dns subtree does 
not exist in all installations.


I hope to change this for https://fedorahosted.org/freeipa/ticket/4058, 
when I've thought more about relationships between plugins, packages, 
install options, and the updater.


--
Petr³
From 8c7b9bcf64bc3762ce8060bd88fd0be6764c8cca Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Fri, 13 Jun 2014 15:58:24 +0200
Subject: [PATCH] managed permission updater: Add mechanism to replace SYSTEM
 permissions

The Read DNS Entries permission, which was marked SYSTEM (no associated
ACI), can now be converted to a regular managed permission.

Add a mechanism for the updater to replace old SYSTEM permissions.

This cannot be done in an update file because we do not want to replace
V2 permissions with the same name.

Part of the work for: https://fedorahosted.org/freeipa/ticket/4346
---
 .../install/plugins/update_managed_permissions.py  | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/ipaserver/install/plugins/update_managed_permissions.py b/ipaserver/install/plugins/update_managed_permissions.py
index 7b1405a1974826fd90acd0d5082f51d8b25034cd..2ca054d50d11eec9527e0ef1e5d53d2f8e479ed0 100644
--- a/ipaserver/install/plugins/update_managed_permissions.py
+++ b/ipaserver/install/plugins/update_managed_permissions.py
@@ -67,6 +67,8 @@
 * replaces
   - A list of ACIs corresponding to legacy default permissions replaced
 by this permission.
+* replaces_system
+  - A list of names of old SYSTEM permissions this replaces.
 * fixup_function
   - A callable that may modify the template in-place before it is applied.
   - Called with the permission name, template dict, and keyword arguments:
@@ -410,6 +412,21 @@ def update_permission(self, ldap, obj, name, template, anonymous_read_aci):
 self.log.info(Removing legacy permission '%s', legacy_name)
 self.api.Command[permission_del](unicode(legacy_name))
 
+for name in template.get('replaces_system', ()):
+name = unicode(name)
+try:
+entry = ldap.get_entry(permission_plugin.get_dn(name),
+   ['ipapermissiontype'])
+except errors.NotFound:
+self.log.info(Legacy permission '%s' not found, name)
+else:
+flags = entry.get('ipapermissiontype', [])
+if list(flags) == ['SYSTEM']:
+self.log.info(Removing legacy permission '%s', name)
+self.api.Command[permission_del](name, force=True)
+else:
+self.log.info(Ignoring V2 permission '%s', name)
+
 def get_upgrade_attr_lists(self, current_acistring, default_acistrings):
 Compute included and excluded attributes for a new permission
 
@@ -497,6 +514,7 @@ def update_entry(self, obj, entry, template,
 
 template = dict(template)
 template.pop('replaces', None)
+template.pop('replaces_system', None)
 
 fixup_function = template.pop('fixup_function', None)
 if fixup_function:
-- 
1.9.0

From 46d49e16b2cfa3a7fb8d947e6ae33e6b3050f87a Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Mon, 9 Jun 2014 15:06:35 +0200
Subject: [PATCH] Convert DNS default permissions to managed

Convewrt the existing default permissions.

The Read permission is split between Read DNS Entries and Read
DNS Configuration.

Part of the work for: https://fedorahosted.org/freeipa/ticket/4346
---
 ACI.txt  |  12 +
 install/share/dns.ldif   |  59 
 install/updates/40-delegation.update |   6 +--
 install/updates/40-dns.update|  29 ++
 ipalib/plugins/dns.py| 101 +++
 5 files changed, 119 insertions(+), 88 deletions(-)

diff --git a/ACI.txt b/ACI.txt
index 2ceaacc077467b6ef54e09d0aa7d3d5695c8fd40..6b75e79c3d771d33558750958f61ada82fd1e5eb 100644
--- a/ACI.txt
+++ b/ACI.txt
@@ -10,6 +10,18 @@ dn: cn=System: Read Global Configuration,cn=permissions,cn=pbac,dc=ipa,dc=exampl
 aci: (targetattr = cn || ipacertificatesubjectbase || ipaconfigstring || ipacustomfields || ipadefaultemaildomain || ipadefaultloginshell || ipadefaultprimarygroup || ipagroupobjectclasses || ipagroupsearchfields || ipahomesrootdir || ipakrbauthzdata || ipamaxusernamelength || ipamigrationenabled || ipapwdexpadvnotify || ipasearchrecordslimit || 

[Freeipa-devel] [PATCHES] 0585-0587 Convert Password Policy COSTemplate default permissions to managed

2014-06-13 Thread Petr Viktorin

The first patch is preparation.

As for the second two, this is how the bulk of the transition will look.

--
Petr³
From 979ef3e1a8e37b8ad6ad60027993f3c8de6a3d98 Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Thu, 12 Jun 2014 09:46:36 +0200
Subject: [PATCH] Add $REALM to variables supported by the managed permission
 updater

This will allow converting password policy permissions

Part of the work for: https://fedorahosted.org/freeipa/ticket/4346
---
 ipaserver/install/plugins/update_managed_permissions.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ipaserver/install/plugins/update_managed_permissions.py b/ipaserver/install/plugins/update_managed_permissions.py
index 7b1405a1974826fd90acd0d5082f51d8b25034cd..f68faf262da5bcfbd4167213dff33db4676f7b2e 100644
--- a/ipaserver/install/plugins/update_managed_permissions.py
+++ b/ipaserver/install/plugins/update_managed_permissions.py
@@ -343,6 +343,7 @@ def update_permission(self, ldap, obj, name, template, anonymous_read_aci):
 if 'replaces' in template:
 sub_dict = {
 'SUFFIX': str(self.api.env.basedn),
+'REALM': str(self.api.env.realm),
 }
 legacy_acistrs = [ipautil.template_str(r, sub_dict)
   for r in template['replaces']]
-- 
1.9.0

From 0b9cd3c194a8d80405cb754e5cbbf39c9d6f0579 Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Wed, 4 Jun 2014 17:39:10 +0200
Subject: [PATCH] Convert COSTemplate default permissions to managed

Part of the work for: https://fedorahosted.org/freeipa/ticket/4346
---
 ACI.txt  |  6 ++
 install/updates/40-delegation.update | 24 
 ipalib/plugins/pwpolicy.py   | 22 ++
 3 files changed, 28 insertions(+), 24 deletions(-)

diff --git a/ACI.txt b/ACI.txt
index 2ceaacc077467b6ef54e09d0aa7d3d5695c8fd40..5573da2fa733955789377be8c3fcbfb2f821ed9c 100644
--- a/ACI.txt
+++ b/ACI.txt
@@ -8,6 +8,12 @@ dn: cn=System: Read Automount Configuration,cn=permissions,cn=pbac,dc=ipa,dc=exa
 aci: (targetattr = automountinformation || automountkey || automountmapname || cn || description || objectclass)(version 3.0;acl permission:System: Read Automount Configuration;allow (compare,read,search) userdn = ldap:///anyone;;)
 dn: cn=System: Read Global Configuration,cn=permissions,cn=pbac,dc=ipa,dc=example
 aci: (targetattr = cn || ipacertificatesubjectbase || ipaconfigstring || ipacustomfields || ipadefaultemaildomain || ipadefaultloginshell || ipadefaultprimarygroup || ipagroupobjectclasses || ipagroupsearchfields || ipahomesrootdir || ipakrbauthzdata || ipamaxusernamelength || ipamigrationenabled || ipapwdexpadvnotify || ipasearchrecordslimit || ipasearchtimelimit || ipaselinuxusermapdefault || ipaselinuxusermaporder || ipauserauthtype || ipauserobjectclasses || ipausersearchfields || objectclass)(targetfilter = (objectclass=ipaguiconfig))(version 3.0;acl permission:System: Read Global Configuration;allow (compare,read,search) userdn = ldap:///all;;)
+dn: cn=System: Add Group Password Policy costemplate,cn=permissions,cn=pbac,dc=ipa,dc=example
+aci: (targetfilter = (objectclass=costemplate))(version 3.0;acl permission:System: Add Group Password Policy costemplate;allow (add) groupdn = ldap:///cn=System: Add Group Password Policy costemplate,cn=permissions,cn=pbac,dc=ipa,dc=example;)
+dn: cn=System: Delete Group Password Policy costemplate,cn=permissions,cn=pbac,dc=ipa,dc=example
+aci: (targetfilter = (objectclass=costemplate))(version 3.0;acl permission:System: Delete Group Password Policy costemplate;allow (delete) groupdn = ldap:///cn=System: Delete Group Password Policy costemplate,cn=permissions,cn=pbac,dc=ipa,dc=example;)
+dn: cn=System: Modify Group Password Policy costemplate,cn=permissions,cn=pbac,dc=ipa,dc=example
+aci: (targetattr = cospriority)(targetfilter = (objectclass=costemplate))(version 3.0;acl permission:System: Modify Group Password Policy costemplate;allow (write) groupdn = ldap:///cn=System: Modify Group Password Policy costemplate,cn=permissions,cn=pbac,dc=ipa,dc=example;)
 dn: cn=System: Read Group Password Policy costemplate,cn=permissions,cn=pbac,dc=ipa,dc=example
 aci: (targetattr = cn || cospriority || krbpwdpolicyreference || objectclass)(targetfilter = (objectclass=costemplate))(version 3.0;acl permission:System: Read Group Password Policy costemplate;allow (compare,read,search) groupdn = ldap:///cn=System: Read Group Password Policy costemplate,cn=permissions,cn=pbac,dc=ipa,dc=example;)
 dn: cn=System: Read Group Membership,cn=permissions,cn=pbac,dc=ipa,dc=example
diff --git a/install/updates/40-delegation.update b/install/updates/40-delegation.update
index 7c3a284b8d2a0592240e56d8118c821a25fc7798..36a0ad020699f6391251d03bd664f55af90500f4 100644
--- a/install/updates/40-delegation.update
+++ b/install/updates/40-delegation.update
@@ -170,27 +170,6 @@ dn: cn=Password Policy 

Re: [Freeipa-devel] DNSSEC key metadata handling

2014-06-13 Thread Petr Spacek

On 12.6.2014 17:49, Petr Spacek wrote:

On 12.6.2014 17:19, Simo Sorce wrote:

On Thu, 2014-06-12 at 17:08 +0200, Petr Spacek wrote:

Hello list,

I have realized that we need to store certain DNSSEC metadata for every
(zone,key,replica) triplet. It is necessary to handle splits in replication
topology.

DNSSEC key can be in one of following states:
- key created
- published but not used for signing
- published and used for signing
- published and not used for signing but old signatures exist
- unpublished

Every state transition has to be postponed until relevant TTL expires, and of
course, we need to consider TTL on all replicas.


Example of a problem

DNS TTL=10 units
Key life time=100 units

Replica=1 Key=1 Time=0   Published
Replica=2 Key=1 Time=0   Published
Replica=1 Key=1 Time=10  Published, signing
Replica=2 Key=1 Time=10  Published, signing
Replica=1 Key=2 Time=90  Generated, published, not signing yet
Replica=2 Key=2 Time=90  replication is broken: key=2 is not on replica=2
Replica=1 Key=1 Time=100
^^^ From time=100, all new signatures should be created with key=2 but that
can break DNSSEC validation because key=2 is not available on replica=2.


Can you explain how this break validation ?
Aren't signatures regenerated on each replica ?

They are.


And so isn't each replica self-consistent ?

Ah, sorry, I didn't mention one important detail. Keys published in the zone
'example.com.' have to match keys published in parent zone. There has to be a
mechanism for synchronizing this.

Validation will break if (keys published by parent) are not subset of (keys on
replicas).

Next logical step in the example above is to remove key1 from replica 1 so you
will end replica1 having key2 and replica2 having only key1.

How can we guarantee that synchronization mechanism will not wipe key1 from
parent? Or the other way around? How can we guarantee that key2 was uploaded?

Also, things will break is number of keys in parent exceeds reasonable number
(because DNS replies will be to big etc.).


Proposal 1
==
- Store state and timestamps for (zone,key,replica) triplet
- Do state transition only if all triplets (zone,key,?) indicate that all
replicas reached desired state so the transition is safe.
- This implicitly means that no transition will happen if one or more replicas
is down. This is necessary otherwise DNSSEC validation can break mysteriously
when keys got out of sync.

dn: cn=some-replica-id,ipk11Label=zone1_keyid123_private, cn=keys, cn=sec,
cn=dns, dc=example
idnssecKeyCreated: timestamp
idnssecKeyPublished: timestamp
idnssecKeyActivated: timestamp
idnssecKeyInactivated: timestamp
idnssecKeyDeleted: timestamp


Why do you care for all 5 states ?

In short, to follow RFC 6781 and all it's requirements.


Simo and I have discussed this off-line. The final decision is to rely on 
replication. The assumption is that if replication is broken, everything will 
break soon anyway, so the original proposal is overkill.


We have to store one set of timestamps somewhere to be able to follow RFC 
6781, so we decided to store it in the key-metadata object.


I added other attributes to object class definition so it contains all 
necessary metadata. The new object class idnsSecKey is now complete.


Please note that DN in the previous example was incorrect. It is necessary to 
store the metadata separately for pairs (zone, key) to cover the case where 
key is shared between zones. This also nicely splits metadata from actual key 
material.


All attributes are single-valued.
MUST attributes are:
 idnsSecKeyRef
 idnsSecKeyCreated
 idnsSecAlgorithm

dn: cn=z/ksk+keytag, cn=keys, idnsname=example.com, cn=dns, dc=example
objectClass: idnsSecKey
idnsSecKeyRef: DN of the PKCS#11 key object under cn=keys, cn=sec, dn=dns
idnsSecKeyCreated: timestamp
idnsSecKeyPublish: timestamp
idnsSecKeyActivate: timestamp
idnsSecKeyInactive: timestamp
idnsSecKeyDelete: timestamp
idnsSecKeyZone: boolean equivalent to bit 7 (ZONE) in [1]
idnsSecKeyRevoke: boolean equivalent to bit 8 (REVOKE) in [1]
idnsSecKeySep: boolean equivalent to bit 15 (SEP) in [1]
idnsSecAlgorithm: string used as mnemonic in [2]

[1] 
http://www.iana.org/assignments/dnskey-flags/dnskey-flags.xhtml#dnskey-flags-1
[2] 
http://www.iana.org/assignments/dns-sec-alg-numbers/dns-sec-alg-numbers.xhtml#dns-sec-alg-numbers-1



It looks to me the only relevant states are Activated and (perhaps)
Deactivated.

But then again if replication is broken *many* other things are, so
should we *really* care to handle broken replication by adding all this
information ?

We need to keep track of timestamps anyway (to follow RFC 6781) so we need
some other way how to *make sure* that timestamps never go backwards, even if
replication was broken and later restarted.

What do you propose?


Effectively, state machine will be controlled by max(attribute) over all
replicas (for given key).

Replication traffic estimation
--
Number 

Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs

2014-06-13 Thread Rob Crittenden
Simo Sorce wrote:
 On Wed, 2014-06-11 at 17:03 -0400, Rob Crittenden wrote:
 0001

 When is_allowed_to_access_attr() fails it should include the value of
 access in the error log for debugging.
 
 Ok added more detailed logging
 
 Nit: Coluld not fetch REALM backend
 
 Fixed
 
 There are still a ton of ber_scanf failed duplicated fatal errors. I'm
 fine keeping a common err_msg but the fatal error should be unique.
 
 Yeah thanks to this comment, I had a small change of heart.
 Instead of sending such detailed information to clients I reverted to
 send a little less information to the clients and instead LOG_FATAL in a
 more detailed way. HTH
 
 This breaks normal host delegation. If you add a host to another host's
 managedby, getting the keytab will fail. This is due to:

 [11/Jun/2014:16:56:45 -0400] NSACLPlugin - conn=4 op=3 (main): Deny
 write on
 entry(fqdn=client2.example.com,cn=computers,cn=accounts,dc=example,dc=com).attr(ipaProtectedOperation;write_keys)
 to fqdn=client1.example.com,cn=computers,cn=accounts,dc=example,dc=com:
 no aci matched the subject by aci(97): aciname= Groups allowed to
 create keytab keys, acidn=cn=accounts,dc=example,dc=com
 
 Ok this should be working now, I added a new ACI to allow also
 managedby#USERDN to operate on keytabs.
 
 New patches attached.

Functionally these seem to work ok. I think there should be some
documented way to enable the -r in ipa-getkeytab. Right now I'm not even
entirely sure how one would add a permission to do so.

rob

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs

2014-06-13 Thread Simo Sorce
On Fri, 2014-06-13 at 12:54 -0400, Rob Crittenden wrote:
 Simo Sorce wrote:
  On Wed, 2014-06-11 at 17:03 -0400, Rob Crittenden wrote:
  0001
 
  When is_allowed_to_access_attr() fails it should include the value of
  access in the error log for debugging.
  
  Ok added more detailed logging
  
  Nit: Coluld not fetch REALM backend
  
  Fixed
  
  There are still a ton of ber_scanf failed duplicated fatal errors. I'm
  fine keeping a common err_msg but the fatal error should be unique.
  
  Yeah thanks to this comment, I had a small change of heart.
  Instead of sending such detailed information to clients I reverted to
  send a little less information to the clients and instead LOG_FATAL in a
  more detailed way. HTH
  
  This breaks normal host delegation. If you add a host to another host's
  managedby, getting the keytab will fail. This is due to:
 
  [11/Jun/2014:16:56:45 -0400] NSACLPlugin - conn=4 op=3 (main): Deny
  write on
  entry(fqdn=client2.example.com,cn=computers,cn=accounts,dc=example,dc=com).attr(ipaProtectedOperation;write_keys)
  to fqdn=client1.example.com,cn=computers,cn=accounts,dc=example,dc=com:
  no aci matched the subject by aci(97): aciname= Groups allowed to
  create keytab keys, acidn=cn=accounts,dc=example,dc=com
  
  Ok this should be working now, I added a new ACI to allow also
  managedby#USERDN to operate on keytabs.
  
  New patches attached.
 
 Functionally these seem to work ok. I think there should be some
 documented way to enable the -r in ipa-getkeytab. Right now I'm not even
 entirely sure how one would add a permission to do so.
 
 rob
 

ATM the only way is to add the ipaAllowedOperations objectclass to the
object you want to allow retrieving a keyt from and the
ipaAllowedToPerform;reasd_key attribute

Example:
dn: test/foo.example@example.com,cn=services,cn=accounts,dc=example,dc=com
changetype: modify
add: objectclass
objectclass: ipaAllowedOperations
-
add: ipaAllowedToPerform;read_key
ipaAllowedToPerform;reasd_key: 
uid=cluster-admin,cn=users,cn=accounts,dc=example,dc=com

Once you do this the user called cluster-admin will be allowed to
retrieve the keytab w/o changing it.

Of course you can list there a group or another host/service DN.

HTH,
Simo.

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

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs

2014-06-13 Thread Simo Sorce
On Fri, 2014-06-13 at 14:04 -0400, Simo Sorce wrote:
 On Fri, 2014-06-13 at 12:54 -0400, Rob Crittenden wrote:
  Simo Sorce wrote:
   On Wed, 2014-06-11 at 17:03 -0400, Rob Crittenden wrote:
   0001
  
   When is_allowed_to_access_attr() fails it should include the value of
   access in the error log for debugging.
   
   Ok added more detailed logging
   
   Nit: Coluld not fetch REALM backend
   
   Fixed
   
   There are still a ton of ber_scanf failed duplicated fatal errors. I'm
   fine keeping a common err_msg but the fatal error should be unique.
   
   Yeah thanks to this comment, I had a small change of heart.
   Instead of sending such detailed information to clients I reverted to
   send a little less information to the clients and instead LOG_FATAL in a
   more detailed way. HTH
   
   This breaks normal host delegation. If you add a host to another host's
   managedby, getting the keytab will fail. This is due to:
  
   [11/Jun/2014:16:56:45 -0400] NSACLPlugin - conn=4 op=3 (main): Deny
   write on
   entry(fqdn=client2.example.com,cn=computers,cn=accounts,dc=example,dc=com).attr(ipaProtectedOperation;write_keys)
   to fqdn=client1.example.com,cn=computers,cn=accounts,dc=example,dc=com:
   no aci matched the subject by aci(97): aciname= Groups allowed to
   create keytab keys, acidn=cn=accounts,dc=example,dc=com
   
   Ok this should be working now, I added a new ACI to allow also
   managedby#USERDN to operate on keytabs.
   
   New patches attached.
  
  Functionally these seem to work ok. I think there should be some
  documented way to enable the -r in ipa-getkeytab. Right now I'm not even
  entirely sure how one would add a permission to do so.
  
  rob
  
 
 ATM the only way is to add the ipaAllowedOperations objectclass to the
 object you want to allow retrieving a keyt from and the
 ipaAllowedToPerform;reasd_key attribute
 
 Example:
 dn: test/foo.example@example.com,cn=services,cn=accounts,dc=example,dc=com
 changetype: modify
 add: objectclass
 objectclass: ipaAllowedOperations
 -
 add: ipaAllowedToPerform;read_key
 ipaAllowedToPerform;reasd_key: 
 uid=cluster-admin,cn=users,cn=accounts,dc=example,dc=com
 
 Once you do this the user called cluster-admin will be allowed to
 retrieve the keytab w/o changing it.
 
 Of course you can list there a group or another host/service DN.

Doh, I realized we haven't created a feature page for this, I am going
to create one now, so that the UI work we'll need in future can look it
up and information like the above is registered.

Will be available here:
http://www.freeipa.org/page/V4/Keytab_Retrieval

Simo.

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

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs

2014-06-13 Thread Rob Crittenden
Rob Crittenden wrote:
 Simo Sorce wrote:
 On Wed, 2014-06-11 at 17:03 -0400, Rob Crittenden wrote:
 0001

 When is_allowed_to_access_attr() fails it should include the value of
 access in the error log for debugging.

 Ok added more detailed logging

 Nit: Coluld not fetch REALM backend

 Fixed

 There are still a ton of ber_scanf failed duplicated fatal errors. I'm
 fine keeping a common err_msg but the fatal error should be unique.

 Yeah thanks to this comment, I had a small change of heart.
 Instead of sending such detailed information to clients I reverted to
 send a little less information to the clients and instead LOG_FATAL in a
 more detailed way. HTH

 This breaks normal host delegation. If you add a host to another host's
 managedby, getting the keytab will fail. This is due to:

 [11/Jun/2014:16:56:45 -0400] NSACLPlugin - conn=4 op=3 (main): Deny
 write on
 entry(fqdn=client2.example.com,cn=computers,cn=accounts,dc=example,dc=com).attr(ipaProtectedOperation;write_keys)
 to fqdn=client1.example.com,cn=computers,cn=accounts,dc=example,dc=com:
 no aci matched the subject by aci(97): aciname= Groups allowed to
 create keytab keys, acidn=cn=accounts,dc=example,dc=com

 Ok this should be working now, I added a new ACI to allow also
 managedby#USERDN to operate on keytabs.

 New patches attached.
 
 Functionally these seem to work ok. I think there should be some
 documented way to enable the -r in ipa-getkeytab. Right now I'm not even
 entirely sure how one would add a permission to do so.

NACK

Simo noticed that the ACIs are in cn=accounts. On the one hand this is a
reasonable place to put these, on the other it is a bit broad.

I think we'll need type-specific ACIs in a number of subtrees: users,
computers and services.

rob

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


Re: [Freeipa-devel] [PATCH 0053] Implement OTP token importing

2014-06-13 Thread Nathaniel McCallum
I am CC'ing Simo because he wants to review my PBKDF2 implementation.

On Wed, 2014-06-11 at 17:41 -0400, Nathaniel McCallum wrote:
 On Wed, 2014-06-11 at 14:24 +0200, Jan Cholasta wrote:
  Hi,
  
  On 13.5.2014 18:40, Nathaniel McCallum wrote:
   On Tue, 2014-05-13 at 12:38 -0400, Nathaniel McCallum wrote:
   This patch adds support for importing tokens using RFC 6030 key
   container files. This includes decryption support. For sysadmin sanity,
   any tokens which fail to add will be written to the output file for
   examination. The main use case here is where a small subset of a large
   set of tokens fails to validate or add. Using the output file, the
   sysadmin can attempt to recover these specific tokens.
  
   This code is implemented as a server-side script. However, it doesn't
   actually need to run on the server. This was done because importing is
   an odd fit for the IPA command framework:
   1. We need to write an output file.
   2. The operation may be long-running (thousands of tokens).
   3. Only admins need to perform this task and it only happens
   infrequently.
  
   I forgot to put the link to the ticket in the commit message. Fixed.
  
  1) I think you should initialize NSS in ipa_otptoken_import.py, not in 
  the ipa-otptoken-import script.
 
 Fixed.
 
  2) The pep8 tool reports a lot of errors in ipa_otptoken_import.py.
 
 Fixed (mostly). The remaining output from pep8 is, I think, entirely
 justifiable.
 
  3) Other error messages are in the form message: %s, I think this one 
  should use that form as well:
  
  +if encoding != 'DECIMAL':
  +raise ValidationError('Unsupported encoding (%s)!' % encoding)
 
 Fixed.
 
  4) This is not right:
  
  +except:
  +self.log.warn(Error adding token:  + 
  str(sys.exc_info()[1]))
  
  I think it should be something like this instead:
  
   except ValidationError, e:
   self.log.warn(Error adding token: %s, e)
 
 Fixed.
 
  5) There is no man page for ipa-otptoken-import.
 
 TODO (tomorrow).

Fixed.

  6) Output file is created even when ipa-otptoken-import fails with 
  Unable to connect to LDAP! Did you kinit? and other initialization 
  errors, which makes subsequent ipa-otptoken-import fail with Output 
  file already exists!.
 
 Fixed.
 
  7) When a key is specified by reference in Key/KeyReference instead of 
  directly in Key/Data/Secret like in 
  http://git.savannah.gnu.org/cgit/oath-toolkit.git/tree/pskctool/tests/pskc-figure4.xml,
   
  import fails with Key not found in token!. I would expect a different 
  error message.
 
 This error is now: Referenced keys are not supported!
 
  8) Importing 
  http://git.savannah.gnu.org/cgit/oath-toolkit.git/tree/pskctool/tests/pskc-figure5.xml
   
  produces this output:
  
  /usr/lib/python2.7/site-packages/ipaserver/install/ipa_otptoken_import.py:307:
   
  FutureWarning: The behavior of this method will change in future 
  versions. Use specific 'len(elem)' or 'elem is not None' test instead.
 if data.get('pinpolicy', None):
  Error adding token: 'NoneType' object has no attribute 'strip'
 
 This now states:
 Error adding token: PINPolicy policy not supported!
 Error adding token: Unsupported token type!
 
  9) Using an arbitrary file in -k produces this output:
  
  (SEC_ERROR_INVALID_KEY) The key does not support the requested operation.
  Traceback (most recent call last):
 File /usr/sbin/ipa-otptoken-import, line 29, in module
   nss.nss_shutdown()
  nss.error.NSPRError: (SEC_ERROR_BUSY) NSS could not shutdown. Objects 
  are still in use.
 
 What do you mean by arbitrary file? A file that is not the key?
 Like /dev/null? I'm not able to reproduce this.
 
  10) Importing 
  http://git.savannah.gnu.org/cgit/oath-toolkit.git/tree/pskctool/tests/pskc-figure7.xml
   
  and 
  http://git.savannah.gnu.org/cgit/oath-toolkit.git/tree/pskctool/tests/pskc-figure8.xml
   
  produces this output:
  
  Error adding token: object of type 'NoneType' has no len()
 
 Import fails with:
 Derived keys are not currently supported!
  or
 X.509 keys are not currently supported!
 
 It would be nice to support these in the future.

I added support for derived keys. However, pskc-figure7.xml appears to
have a problem. This problem is actually in RFC 6030 itself (where
pskc-figure7.xml comes from). According to the xenc11 standard, all of
these tags should be in the xenc11 namespace (not pkcs5 or undefined as
given in RFC 6030):
pkcs5:PBKDF2-params
  Salt
SpecifiedEj7/PEpyEpw=/Specified
  /Salt
  IterationCount1000/IterationCount
  KeyLength16/KeyLength
  PRF/
/pkcs5:PBKDF2-params

When fixing this error in pskc-figure7.xml, key derivation works in this
latest patch.

  11) Importing 
  http://git.savannah.gnu.org/cgit/oath-toolkit.git/tree/pskctool/tests/pskc-all.xml
   
  or 
  

Re: [Freeipa-devel] [PATCH 0049] Add support for protected tokens

2014-06-13 Thread Nathaniel McCallum
On Wed, 2014-06-11 at 12:43 -0400, Nathaniel McCallum wrote:
 On Wed, 2014-06-11 at 12:12 +0200, Ludwig Krispenz wrote:
  On 05/13/2014 04:33 PM, Jan Cholasta wrote:
   On 12.5.2014 21:02, Nathaniel McCallum wrote:
   On Thu, 2014-05-08 at 13:51 -0400, Simo Sorce wrote:
   On Thu, 2014-05-08 at 12:26 -0400, Nathaniel McCallum wrote:
   On Wed, 2014-05-07 at 11:17 -0400, Simo Sorce wrote:
   On Wed, 2014-05-07 at 09:54 -0400, Dmitri Pal wrote:
   On 05/07/2014 09:05 AM, Nathaniel McCallum wrote:
   On Wed, 2014-05-07 at 11:42 +0200, Jan Cholasta wrote:
   Hi,
  
   On 6.5.2014 17:08, Nathaniel McCallum wrote:
   On Tue, 2014-05-06 at 09:49 -0400, Nathaniel McCallum wrote:
   On Mon, 2014-05-05 at 12:42 -0400, Nathaniel McCallum wrote:
   This also constitutes a rethinking of the token ACIs after the
   introduction of SELFDN support.
  
   Admins, as before, have full access to all token permissions.
  
   Normal users have read/search/compare access to all of the 
   non-secret
   data for tokens assigned to them, whether protected or 
   non-protected.
   Users can add or delete non-protected tokens and modify most 
   of their
   metadata. However they cannot create, delete or modify 
   protected tokens.
   Regardless of whether the token is protected or not, users 
   cannot change
   a token's ownership or unique identity.
  
   In contrast, admins can create protected tokens. This 
   protects the token
   from deletion or modification when assigned to users. 
   Additionally, when
   a user account is deleted, the assigned non-protected tokens 
   are deleted
   but the protected tokens are merely orphaned. This permits 
   the token to
   be reassigned without having to recreate it. This last point is
   particularly useful in the case of hardware tokens.
  
   https://fedorahosted.org/freeipa/ticket/4228
  
   NOTE: This patch depends on my patch 0048.
   This new version makes ipatokenDisabled visible for token 
   owners. It is
   also writable if the token is non-protected. This 
   additionally fixes:
  
   https://fedorahosted.org/freeipa/ticket/4259
   This new version changes the way the default value of 
   protected is setup
   in accordance with the changes made for the review of my patch 
   0048.2.
  
   Nathaniel
   Is using the ipatokenprotected attribute the final design?
   No. Alternate designs are welcome. The code is easy enough to 
   modify.
  
   I did not dig too deep into this, but I think it might be 
   better to
   instead use the managedby attribute on a token to limit who can 
   delete
   (or administer in other way) the token. On otptoken-add, 
   managedby would
   be set to the whoami user DN, unless run with --protected, in 
   which
   case managedby would be left empty. Then, when deleting a user, 
   the
   token would be deleted only if the user manages the token.
   It seems to me that the mechanics of this are roughly the same as
   protected, just with a different syntax. The cost of this is more
   complex ACIs. In particular, we'd have to use two userdn clauses 
   (is
   this possible?) instead of a simple filter. If there is a clear 
   benefit,
   we can justify the more obtuse syntax.
  
   We usually try not to create new attributes until it is fully 
   justified.
   I would like Simo to chime in on this.
  
   I would also prefer to reuse existing attributes and mechanism if
   possible and if it will not preclude future work.
  
   In this case, it sounds like managed-by has the appropriate 
   meaning:
   who manages the token ? - myself, admin, other, none ?
  
   Nathaniel can you send 2 lines showing the difference in ACIs between
   using managed-by vs a new attribute ?
  
   These are the ACIs using the protected mechanism:
  
   aci: (targetfilter = (objectClass=ipaToken))(targetattrs =
   objectclass || description || ipatokenUniqueID || ipatokenDisabled ||
   ipatokenNotBefore || ipatokenNotAfter || ipatokenVendor || 
   ipatokenModel
   || ipatokenSerial || ipatokenOwner || ipatokenProtected)(version 3.0;
   acl Users can read basic token info; allow (read, search, compare)
   userattr = ipatokenOwner#USERDN;)
  
   aci: (targetfilter = (objectClass=ipatokenTOTP))(targetattrs =
   ipatokenOTPalgorithm || ipatokenOTPdigits ||
   ipatokenTOTPtimeStep)(version 3.0; acl Users can see TOTP details;
   allow (read, search, compare) userattr = ipatokenOwner#USERDN;)
  
   aci: (targetfilter = (objectClass=ipatokenHOTP))(targetattrs =
   ipatokenOTPalgorithm || ipatokenOTPdigits)(version 3.0; acl 
   Users can
   see HOTP details; allow (read, search, compare) userattr =
   ipatokenOwner#USERDN;)
  
   aci: (targetfilter =
   ((objectClass=ipaToken)(!(ipatokenProtected=TRUE(targetattrs =
   description || ipatokenDisabled || ipatokenNotBefore ||
   ipatokenNotAfter || ipatokenVendor || ipatokenModel ||
   ipatokenSerial)(version 3.0; acl Users can write basic token info;
   allow (write) userattr = ipatokenOwner#USERDN;)
  
   aci: (target 

Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs

2014-06-13 Thread Simo Sorce
On Fri, 2014-06-13 at 14:29 -0400, Rob Crittenden wrote:
 Rob Crittenden wrote:
  Simo Sorce wrote:
  On Wed, 2014-06-11 at 17:03 -0400, Rob Crittenden wrote:
  0001
 
  When is_allowed_to_access_attr() fails it should include the value of
  access in the error log for debugging.
 
  Ok added more detailed logging
 
  Nit: Coluld not fetch REALM backend
 
  Fixed
 
  There are still a ton of ber_scanf failed duplicated fatal errors. I'm
  fine keeping a common err_msg but the fatal error should be unique.
 
  Yeah thanks to this comment, I had a small change of heart.
  Instead of sending such detailed information to clients I reverted to
  send a little less information to the clients and instead LOG_FATAL in a
  more detailed way. HTH
 
  This breaks normal host delegation. If you add a host to another host's
  managedby, getting the keytab will fail. This is due to:
 
  [11/Jun/2014:16:56:45 -0400] NSACLPlugin - conn=4 op=3 (main): Deny
  write on
  entry(fqdn=client2.example.com,cn=computers,cn=accounts,dc=example,dc=com).attr(ipaProtectedOperation;write_keys)
  to fqdn=client1.example.com,cn=computers,cn=accounts,dc=example,dc=com:
  no aci matched the subject by aci(97): aciname= Groups allowed to
  create keytab keys, acidn=cn=accounts,dc=example,dc=com
 
  Ok this should be working now, I added a new ACI to allow also
  managedby#USERDN to operate on keytabs.
 
  New patches attached.
  
  Functionally these seem to work ok. I think there should be some
  documented way to enable the -r in ipa-getkeytab. Right now I'm not even
  entirely sure how one would add a permission to do so.
 
 NACK
 
 Simo noticed that the ACIs are in cn=accounts. On the one hand this is a
 reasonable place to put these, on the other it is a bit broad.
 
 I think we'll need type-specific ACIs in a number of subtrees: users,
 computers and services.

[Only patch 3 attached, as none of the others changed, and addiotional
discussion is needed, see below.]

Ok after looking carefully into this problem I see that there are really
2 issues.
1) managedby for users, we do not want someone adding a managedby
attribute to inadvertently allow the manager to set a user's password.

So I changed that ACI and restricted it only to ipaHost and ipaService
objects (tested).

I haven't touched any other ACI because in order to use them you need to
have intentionally added an ipaAllowedToPerform attribute to the user
entry.

2) and I think this is a MUCH bigger issue, the Admin users are
unbounded and pass any Access Control Check and this means they can now
retrieve any key for users or machines.
It is already bad enough that admins can unconditionally set any key,
but this at least leaves back a pretty big trail (the original client
password/key fails to work), and is a necessary evil (password resets,
hosts creation/recovery).
But I am not very comfortable with the idea an admin can retrieve any
key without actually ending up changing it. Petr do we have any short
term plan to address the Admin's super ACI ?

Otherwise we can add ipaProtectedOperation in the exclude list for the
superACI (Admins can manage any entry) and add the following ACI in
cn=accounts, to restore admin ability to set keys (but not retrieve
them):

aci: (targetattr=ipaProtectedOperation;write_keys)(version 3.0; acl
 Admins are allowed to rekey any entity; allow(write) groupdn=ldap
 :///cn=admins,cn=groups,cn=accounts,$SUFFIX;)

I tested this combination and it effectively stops admin from retrieving
all keys unless explicitly authorize by positive
ACIs/ipaAllowedToPerform attributes.


Thoughts ?

-- 
Simo Sorce * Red Hat, Inc * New York
From e3a19bf910f2e2bbaabb870471045597469e790e Mon Sep 17 00:00:00 2001
From: Simo Sorce s...@redhat.com
Date: Tue, 17 Sep 2013 00:30:14 -0400
Subject: [PATCH 3/6] keytab: Add new extended operation to get a keytab.

This new extended operation allow to create new keys or retrieve
existing ones. The new set of keys is returned as a ASN.1 structure
similar to the one that is passed in by the 'set keytab' extended
operation.

Access to the operation is regulated through a new special ACI that
allows 'retrieval' only if the user has access to an attribute named
ipaProtectedOperation postfixed by the subtypes 'read_keys' and
'write_keys' to distinguish between creation and retrieval operation.

For example for allowing retrieval by a specific user the following ACI
is set on cn=accounts:

(targetattr=ipaProtectedOperation;read_keys) ...
 ... userattr=ipaAllowedToPerform;read_keys#USERDN)

This ACI matches only if the service object hosts a new attribute named
ipaAllowedToPerform that holds the DN of the user attempting the
operation.

Resolves:
https://fedorahosted.org/freeipa/ticket/3859
---
 .../ipa-pwd-extop/ipa_pwd_extop.c  | 571 +
 install/share/60basev3.ldif|   3 +
 install/share/default-aci.ldif |   8 +-
 install/updates/20-aci.update   

Re: [Freeipa-devel] [PATCH] #3859: Better mechanism to retrieve keytabs

2014-06-13 Thread Tomas Babej

On 06/13/2014 10:20 PM, Simo Sorce wrote:
 On Fri, 2014-06-13 at 14:29 -0400, Rob Crittenden wrote:
 Rob Crittenden wrote:
 Simo Sorce wrote:
 On Wed, 2014-06-11 at 17:03 -0400, Rob Crittenden wrote:
 0001

 When is_allowed_to_access_attr() fails it should include the value of
 access in the error log for debugging.
 Ok added more detailed logging

 Nit: Coluld not fetch REALM backend
 Fixed

 There are still a ton of ber_scanf failed duplicated fatal errors. I'm
 fine keeping a common err_msg but the fatal error should be unique.
 Yeah thanks to this comment, I had a small change of heart.
 Instead of sending such detailed information to clients I reverted to
 send a little less information to the clients and instead LOG_FATAL in a
 more detailed way. HTH

 This breaks normal host delegation. If you add a host to another host's
 managedby, getting the keytab will fail. This is due to:

 [11/Jun/2014:16:56:45 -0400] NSACLPlugin - conn=4 op=3 (main): Deny
 write on
 entry(fqdn=client2.example.com,cn=computers,cn=accounts,dc=example,dc=com).attr(ipaProtectedOperation;write_keys)
 to fqdn=client1.example.com,cn=computers,cn=accounts,dc=example,dc=com:
 no aci matched the subject by aci(97): aciname= Groups allowed to
 create keytab keys, acidn=cn=accounts,dc=example,dc=com
 Ok this should be working now, I added a new ACI to allow also
 managedby#USERDN to operate on keytabs.

 New patches attached.
 Functionally these seem to work ok. I think there should be some
 documented way to enable the -r in ipa-getkeytab. Right now I'm not even
 entirely sure how one would add a permission to do so.
 NACK

 Simo noticed that the ACIs are in cn=accounts. On the one hand this is a
 reasonable place to put these, on the other it is a bit broad.

 I think we'll need type-specific ACIs in a number of subtrees: users,
 computers and services.
 [Only patch 3 attached, as none of the others changed, and addiotional
 discussion is needed, see below.]

 Ok after looking carefully into this problem I see that there are really
 2 issues.
 1) managedby for users, we do not want someone adding a managedby
 attribute to inadvertently allow the manager to set a user's password.

 So I changed that ACI and restricted it only to ipaHost and ipaService
 objects (tested).

 I haven't touched any other ACI because in order to use them you need to
 have intentionally added an ipaAllowedToPerform attribute to the user
 entry.

 2) and I think this is a MUCH bigger issue, the Admin users are
 unbounded and pass any Access Control Check and this means they can now
 retrieve any key for users or machines.
 It is already bad enough that admins can unconditionally set any key,
 but this at least leaves back a pretty big trail (the original client
 password/key fails to work), and is a necessary evil (password resets,
 hosts creation/recovery).
 But I am not very comfortable with the idea an admin can retrieve any
 key without actually ending up changing it. Petr do we have any short
 term plan to address the Admin's super ACI ?

 Otherwise we can add ipaProtectedOperation in the exclude list for the
 superACI (Admins can manage any entry) and add the following ACI in
 cn=accounts, to restore admin ability to set keys (but not retrieve
 them):

 aci: (targetattr=ipaProtectedOperation;write_keys)(version 3.0; acl
  Admins are allowed to rekey any entity; allow(write) groupdn=ldap
  :///cn=admins,cn=groups,cn=accounts,$SUFFIX;)

 I tested this combination and it effectively stops admin from retrieving
 all keys unless explicitly authorize by positive
 ACIs/ipaAllowedToPerform attributes.


 Thoughts ?

Just a heads up, I managed to catch a typo with my sleepy eyes:

--- a/install/share/default-aci.ldif
+++ b/install/share/default-aci.ldif
@@ -21,11 +21,17 @@ changetype: modify
 add: aci
 aci: (targetfilter =
(|(objectClass=ipaConfigObject)(dnahostname=*)))(version 3.0;acl
Admins can change GUI config; allow (delete) groupdn =
ldap:///cn=admins,cn=groups,cn=accounts,$SUFFIX;;)
 
-dn: cn=accounts,$SUFFIX
+dn: cngaccounts,$SUFFIX




 ___
 Freeipa-devel mailing list
 Freeipa-devel@redhat.com
 https://www.redhat.com/mailman/listinfo/freeipa-devel

-- 
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org 

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [Freeipa-devel] [PATCH 0053] Implement OTP token importing

2014-06-13 Thread Simo Sorce
On Fri, 2014-06-13 at 15:39 -0400, Nathaniel McCallum wrote:
 I am CC'ing Simo because he wants to review my PBKDF2 implementation.

Thank you.
I am a bit concerned you are using etree.parse(self.args[0]) directly
with the default parser configuration.

I think we should, at least, do something like this:
parser = etree.XMLParser(resolve_entities=False)
parser.parse(self.args[0])

We wouldn't want to inadvertently hit the network when reading this xml
file, would we ?


Reading the PBKDF2KeyDerivation code I see no particular issue, although
I found it a bit hard to decod what it was doing as there are no
comments, nor a direct reference to the algorithm you are implementing.

It would be nice to have comments either before the function or within
the function that gives an explanation of the algorithm being
implemented so that whoever needs to read this in the future can readily
understand what is going on.
I've used this as reference to refresh my memory on the algorithm:
http://en.wikipedia.org/wiki/PBKDF2

Btw for the final join, wouldn't it be more compact to use:
dk += ''.join(map(chr,u)) ?

Actually this creates a new string at each iteration...
if you declare dk = [] before the loop and dk.append(''.join(map(chr,
u))) in the loop.
Then you can return ''.join(dk)[:self.klen] and build the final string
only once.

Or given you already use lambdas in there perhaps simplify even to:

dk = []
for i ...
...
dk.append(u)

return ''.join(map(lambda x: ''.join(map(chr, x)), dk))[:self.klen]


In general the flow is a bit hard to follow due to the clever use of
map(), maybe a few comments sprinkled before functions about who/how is
meant to use them would help the casual observer.

Other than the first point though, anything else looks good to me.

Simo.

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel