Re: [Freeipa-devel] [PATCH 0165] --zonemgr options must be unicode
On 13/11/14 18:28, Martin Basti wrote: To allow IDNA zonemgr email, value must be unicode not ASCII Ticket: https://fedorahosted.org/freeipa/ticket/4724 Patch attached. Patch for ipa-4.0 added. -- Martin Basti From 033b2ab8b8d92e92a852eedbd7f7d4a5aea1e92b Mon Sep 17 00:00:00 2001 From: Martin Basti mba...@redhat.com Date: Thu, 13 Nov 2014 18:22:22 +0100 Subject: [PATCH] Fix: zonemgr must be unicode value To support IDNA --zonemgr option must be unicode not ascii https://fedorahosted.org/freeipa/ticket/4724 --- ipaserver/install/bindinstance.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ipaserver/install/bindinstance.py b/ipaserver/install/bindinstance.py index ecaf4e0f92b33b6933e747280986dee7ad44a86d..ae517843695068e5598a4e845f1ba421b11faa08 100644 --- a/ipaserver/install/bindinstance.py +++ b/ipaserver/install/bindinstance.py @@ -22,6 +22,7 @@ import os import pwd import netaddr import re +import sys import time import ldap @@ -403,6 +404,8 @@ def zonemgr_callback(option, opt_str, value, parser): # validate the value first try: +# IDNA support requires unicode +value = value.decode(sys.stdin.encoding) validate_zonemgr_str(value) except ValueError, e: parser.error(invalid zonemgr: + unicode(e)) -- 1.8.3.1 From a90379c83e3e10373e54868402f55e965713236e Mon Sep 17 00:00:00 2001 From: Martin Basti mba...@redhat.com Date: Thu, 13 Nov 2014 18:22:22 +0100 Subject: [PATCH] Fix: zonemgr must be unicode value To support IDNA --zonemgr option must be unicode not ascii https://fedorahosted.org/freeipa/ticket/4724 --- ipaserver/install/bindinstance.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ipaserver/install/bindinstance.py b/ipaserver/install/bindinstance.py index 6cf018e9cda3734a99a8ac5ac1df134e9e4c2293..70e987aa1eb48466c36cae5aadda1aa56f9c34f9 100644 --- a/ipaserver/install/bindinstance.py +++ b/ipaserver/install/bindinstance.py @@ -403,6 +403,8 @@ def zonemgr_callback(option, opt_str, value, parser): # validate the value first try: +# IDNA support requires unicode +value = value.decode(sys.stdin.encoding) validate_zonemgr_str(value) except ValueError, e: parser.error(invalid zonemgr: + unicode(e)) -- 1.8.3.1 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 0673 Do not restore SELinux settings that were not backed up
This fixes https://fedorahosted.org/freeipa/ticket/4678 -- Petr³ From bdeb005e8660868dc3faa0b15d04cb4954aef3b6 Mon Sep 17 00:00:00 2001 From: Petr Viktorin pvikt...@redhat.com Date: Tue, 18 Nov 2014 10:40:31 +0100 Subject: [PATCH] Do not restore SELinux settings that were not backed up https://fedorahosted.org/freeipa/ticket/4678 --- ipaplatform/base/tasks.py | 3 ++- ipaplatform/redhat/tasks.py | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/ipaplatform/base/tasks.py b/ipaplatform/base/tasks.py index 9b15119c4a1a1cc63e0e09ae436d8cec7c603fb9..ff71c2bd12be3b775a0ed43c8038ee3924d2c9f6 100644 --- a/ipaplatform/base/tasks.py +++ b/ipaplatform/base/tasks.py @@ -152,7 +152,8 @@ def set_selinux_booleans(self, required_settings, backup_func=None): :param required_settings: A dictionary mapping the boolean names to desired_values. - The desired value can be 'on' or 'off'. + The desired value can be 'on' or 'off', + or None to leave the setting unchanged. :param backup_func: A function called for each boolean with two arguments: the name and the previous value diff --git a/ipaplatform/redhat/tasks.py b/ipaplatform/redhat/tasks.py index cb0a27f73eb3269c32bb5f06aaf2b5bf23c4168e..b26604aa736eb472c88bc0dcbc3a4b515712ce9d 100644 --- a/ipaplatform/redhat/tasks.py +++ b/ipaplatform/redhat/tasks.py @@ -363,6 +363,8 @@ def get_setsebool_args(changes): updated_vars = {} failed_vars = {} for setting, state in required_settings.iteritems(): +if state is None: +continue try: (stdout, stderr, rc) = ipautil.run([paths.GETSEBOOL, setting]) original_state = stdout.split()[2] -- 2.1.0 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0078] Enable QR code display by default in otptoken-add
On 11/14/2014 08:29 PM, Simo Sorce wrote: On Fri, 14 Nov 2014 20:05:35 +0100 Petr Viktorin pvikt...@redhat.com wrote: On 11/14/2014 08:03 PM, Petr Viktorin wrote: On 11/14/2014 07:26 PM, Simo Sorce wrote: On Fri, 14 Nov 2014 14:08:24 +0100 Petr Viktorin pvikt...@redhat.com wrote: On 11/14/2014 01:18 PM, Petr Vobornik wrote: [...] Nope, defaults are filled in by the client. (And also on the server if they're still missing; it's part of the common validation.) IMHO this is quite unfortunate behavior which may also fail horribly if there is a newer client and an older server - backwards compatibility is on API level, not CLI level. Defaults should be filled by server, not a client. We should seriously reconsider the design of our CLI. But that's for different, future discussion. You can't use a newer client with an older server, you get a VersionError in that case. Does it break only for this command ? Or in general. In general. It's been built into the framework since IPA 2.0 [0]. There have been four years of development assuming this compatibility scheme. I should clarify – this is only for the API, i.e. the `ipa` command. Clients of the ipa-client-install sort don't use the API. I know they don't or it would be a disaster. However it is unreasonable to keep changing the API without any 2 way compatibility going forward. I expect it to be extremely common for an admin to have a much more recent desktop then the server being used. Having to jump through hoops to use the admin cli is not friendly. And we do not change the actual CLI that much, so it would be unexpected. We need to take seriously in consideration compatibility going both ways (of course new commands should just get NotSupported errors when used against old servers. But old commands should work, there is no good reason to break this kind of compatibility, it is just an artefact of botched versioning we did a few years ago and it is about time we seriously address this, also because once we make one of these APIs public and supported we cannot willy nilly break it, and we cannot force people to change their software if all works well except a version number being sent in and out. Individual interfaces need to be versioned, and one an interface is release it must no change (a new version need to be created if changes are needed). Well, it is what it is. This paradigm (forward compatibility only) was there since the beginning (read - IPA 2.0) and we cannot change it that simply - it is big effort on it's own that needs to be planned, designed, implemented. To solve this, you would need to introduce some kind of version/metadata handshake between new client and old server so that the clients knows what API does the old server expects. It would need to know which attributes were changed/added in incompatible way between it's and server's version. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0165] --zonemgr options must be unicode
On 11/18/2014 12:07 PM, Martin Basti wrote: On 13/11/14 18:28, Martin Basti wrote: To allow IDNA zonemgr email, value must be unicode not ASCII Ticket: https://fedorahosted.org/freeipa/ticket/4724 Patch attached. Patch for ipa-4.0 added. Thanks, works for me, ACK. -- David Kupka ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0078] Enable QR code display by default in otptoken-add
On Tue, 18 Nov 2014 12:27:28 +0100 Martin Kosek mko...@redhat.com wrote: On 11/14/2014 08:29 PM, Simo Sorce wrote: On Fri, 14 Nov 2014 20:05:35 +0100 Petr Viktorin pvikt...@redhat.com wrote: On 11/14/2014 08:03 PM, Petr Viktorin wrote: On 11/14/2014 07:26 PM, Simo Sorce wrote: On Fri, 14 Nov 2014 14:08:24 +0100 Petr Viktorin pvikt...@redhat.com wrote: On 11/14/2014 01:18 PM, Petr Vobornik wrote: [...] Nope, defaults are filled in by the client. (And also on the server if they're still missing; it's part of the common validation.) IMHO this is quite unfortunate behavior which may also fail horribly if there is a newer client and an older server - backwards compatibility is on API level, not CLI level. Defaults should be filled by server, not a client. We should seriously reconsider the design of our CLI. But that's for different, future discussion. You can't use a newer client with an older server, you get a VersionError in that case. Does it break only for this command ? Or in general. In general. It's been built into the framework since IPA 2.0 [0]. There have been four years of development assuming this compatibility scheme. I should clarify – this is only for the API, i.e. the `ipa` command. Clients of the ipa-client-install sort don't use the API. I know they don't or it would be a disaster. However it is unreasonable to keep changing the API without any 2 way compatibility going forward. I expect it to be extremely common for an admin to have a much more recent desktop then the server being used. Having to jump through hoops to use the admin cli is not friendly. And we do not change the actual CLI that much, so it would be unexpected. We need to take seriously in consideration compatibility going both ways (of course new commands should just get NotSupported errors when used against old servers. But old commands should work, there is no good reason to break this kind of compatibility, it is just an artefact of botched versioning we did a few years ago and it is about time we seriously address this, also because once we make one of these APIs public and supported we cannot willy nilly break it, and we cannot force people to change their software if all works well except a version number being sent in and out. Individual interfaces need to be versioned, and one an interface is release it must no change (a new version need to be created if changes are needed). Well, it is what it is. This paradigm (forward compatibility only) was there since the beginning (read - IPA 2.0) and we cannot change it that simply - it is big effort on it's own that needs to be planned, designed, implemented. And needs to be changed, it always was supposed to be temporary, and it is time to change it. now that we have various distributions and not just Fedora with FreeIPA we cannot make the ipa command useless unless you happen to have the same exact version that you have on the server, normally clients are always a few versions ahead of servers which move more slowly. To solve this, you would need to introduce some kind of version/metadata handshake between new client and old server so that the clients knows what API does the old server expects. It would need to know which attributes were changed/added in incompatible way between it's and server's version. No, this would be the wrong way to go. The solution is the same Linux adopted for ABI compatibility in libraries. We version the single API command, all we need to do is add a version field in the json structure (or even just in the command name). Any time people want to change a command a new command is created instead and the old one stays around for older clients. This is how all successful and backwards compatible RPC APIs are built, and we need to follow suite asap. 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 0161] Fix dyndb-ldap working dir permission
Hi, Dne 13.11.2014 v 14:50 Martin Basti napsal(a): On 13/11/14 13:59, Jan Cholasta wrote: Dne 12.11.2014 v 13:33 Martin Basti napsal(a): On 11/11/14 16:58, Jan Cholasta wrote: Hi, Dne 11.11.2014 v 16:22 Martin Basti napsal(a): Using specfile to create file doesn't work if named user is not on system. Appropriate permission have to be set during ipa-dns installation. Patch attached Why is the directory set up in dnskeysyncinstance instead of bindinstance? Because, dnskeysyncinstance is the daemon which requires permission change. (dir is created by dyndb-ldap plugin) OK. But please rename the method to something more suitable (fix_dyndb_ldap_workdir_permissions?) and add a docstring/comment. Also please change the ticket link to https://fedorahosted.org/freeipa/ticket/4716 (cloned from BZ). The original patch was released with 4.1.1, shouldn't there be update in ipa-upgradeconfig? Cases: 1) fresh RPM install, no named user during RPM install - named doesn't start, user had to fix it immediately, can't wait until next release. 2) fresh RPM install, named user - no impact 3) upgrade IPA with DNS - no impact 4) upgrade IPA without DNS - after DNS installation, same as 1) 5) IPA 4.1.0 with installed DNS, upgrade to 4.1.2 - DNSSEC will not work (If user doesnt use DNSSEC) Only 5) looks serious for me, so here is updated patch. Could you do the update without the code duplication? In similar code an appropriate *instance method is usually called. The uid/gid resolution in ipa-upgradeconfig still looks like duplicated code to me. I would suggest doing something along these lines in ipa-upgradeconfig: dnskeysync = dnskeysyncinstance.DNSKeySyncInstance() dnskeysync.set_dyndb_ldap_workdir_permissions() and have DNSKeySyncInstance.set_dyndb_ldap_workdir_permissions() do all the real work. Martin^2 Honza Honza Thanks. updated patch attached. Martin^2 Honza -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 0674 Raise an error when a critical DS task fails
Backup and restore depend on DS tasks succeeding, but they only waited for the task to end and didn't check the result. This patch adds a new helper, run_task, that runs a task, waits for it, and checks the result. Backup and restore are changed to use this helper. -- Petr³ From a557d1e816280719404f7e2dbabad4f425b21e5e Mon Sep 17 00:00:00 2001 From: Petr Viktorin pvikt...@redhat.com Date: Tue, 18 Nov 2014 14:05:59 +0100 Subject: [PATCH] Raise an error when a critical DS task fails The backup/restore ignored results of backup/restore task, because the helper used does not check for errors. Add an error-checking helper to installutils, and have backup/restore use it. The wait_for_task helper is moved to installutils as well. Its return value is changed. (The return value was always ignored in current code.) --- ipaserver/install/installutils.py | 55 +++ ipaserver/install/ipa_backup.py | 19 ++ ipaserver/install/ipa_restore.py | 20 +++--- ipaserver/install/replication.py | 21 +-- 4 files changed, 61 insertions(+), 54 deletions(-) diff --git a/ipaserver/install/installutils.py b/ipaserver/install/installutils.py index d3f09eccb161cc9371fa7c1109816f47194d9cb6..955fa348c6f519b5145288dd58cf8b8ffe997903 100644 --- a/ipaserver/install/installutils.py +++ b/ipaserver/install/installutils.py @@ -29,6 +29,7 @@ import traceback import textwrap from contextlib import contextmanager +import time from dns import resolver, rdatatype from dns.exception import DNSException @@ -46,6 +47,8 @@ from ipaplatform import services from ipaplatform.paths import paths +log = log_mgr.get_logger(__name__) + # Used to determine install status IPA_MODULES = [ 'httpd', 'kadmin', 'dirsrv', 'pki-cad', 'pki-tomcatd', 'install', @@ -1035,3 +1038,55 @@ def load_external_cert(files, subject_base): ca_file.flush() return cert_file, ca_file + + +def wait_for_task(conn, dn): +Check task status + +Task is complete when the nsTaskExitCode attr is set. + +:return: the task's entry + +Usually you will want to call run_task, which checks the task's exit code + +assert isinstance(dn, DN) +attrlist = [ +'nsTaskLog', 'nsTaskStatus', 'nsTaskExitCode', 'nsTaskCurrentItem', +'nsTaskTotalItems'] +while True: +entry = conn.get_entry(dn, attrlist) +if entry.single_value.get('nsTaskExitCode'): +break +time.sleep(1) +for log_item in entry.get('nsTaskLog', []): +log.debug('DS task log: %s', log_item) +return entry + + +def run_task(conn, entry, task_name, raiseonerr=True): +Run a DS task, wait for it to finish, check it completed successfully + +:param conn: connected LDAPClient +:param entry: task to run, as an LDAP entry +:param task_name: name to use in messages +:param raiseonerr: if true (default), raise RuntimeError if the task failed + +dn = entry.dn +try: +conn.add_entry(entry) +except Exception as e: +raise admintool.ScriptError( +'Unable to add %s task: %s' % (task_name, e)) + +log.info(Waiting for %s task to finish, task_name) +entry = wait_for_task(conn, dn) +exit_code = int(entry.single_value['nsTaskExitCode']) +if exit_code: +result = entry.single_value.get('nsTaskStatus', 'no status') +if raiseonerr: +raise RuntimeError('%s task failed: %s: %s' % ( +task_name, exit_code, result)) +else: +log.warn('%s task failed: %s: %s', + task_name, exit_code, result) +log.info(%s task finished, task_name) diff --git a/ipaserver/install/ipa_backup.py b/ipaserver/install/ipa_backup.py index 682b626e55b9ead5424369e1bab85109ff6b8207..4dc715cf880ee97bc0c27760e5ce3f082b0cef4f 100644 --- a/ipaserver/install/ipa_backup.py +++ b/ipaserver/install/ipa_backup.py @@ -35,7 +35,6 @@ from ipapython.config import IPAOptionParser from ipapython.dn import DN from ipaserver.install.dsinstance import realm_to_serverid, DS_USER -from ipaserver.install.replication import wait_for_task from ipaserver.install import installutils from ipapython import ipaldap from ipalib.session import ISO8601_DATETIME_FMT @@ -403,14 +402,7 @@ def db2ldif(self, instance, backend, online=True): } ) -try: -conn.add_entry(ent) -except Exception, e: -raise admintool.ScriptError('Unable to add LDIF task: %s' -% e) - -self.log.info(Waiting for LDIF to finish) -wait_for_task(conn, dn) +installutils.run_task(conn, ent, 'LDIF export') else: args = ['%s/db2ldif' % self.__find_scripts_dir(instance), '-r', @@ -450,14 +442,7 @@ def db2bak(self, instance, online=True): } ) -try: -
Re: [Freeipa-devel] [PATCHES] Fix getkeytab operation
On Tue, 2014-11-18 at 00:01 -0500, Simo Sorce wrote: Hello team, Recently Alexander opened the following bug: https://fedorahosted.org/freeipa/ticket/4718 The problem with this code is that manual ASN.1 encoding is fragile and too prone to error, and I found various issues while investigating the bug. So I decided to give a shot at replacing the fragile manual code with more robust code autogenerated by the asn1c compiler. While working on replacing the code with the autogenerated one I discovered additional encoding issues of which the following ticket represent only the most evident: https://fedorahosted.org/freeipa/ticket/4728 I found numerous encoding errors which basically made the getkeytab control we implemented in both the client and the server not respect the encoding we actually defined with ASN.1 notation here: http://www.freeipa.org/page/V4/Keytab_Retrieval While digging and testing replacement functions is also became evident that the getkeytab feature was only partially working and that the bug in 4718 was not just a minor error, but cannot ever work on existing servers as there are compounding bugs that would prevent using the getkeytab protocol to ever work if the user specified enctypes via ipa-getkeytab instead of just requesting the server's defaults. Because of this and because it was just too hard *and* useless to try to be compatible with existing broken clients and servers the new code breaks compatibility for correctness of implementation. The break in compatibility is mitigated by the fact that ipa-getkeytab always falls back to the old setkeytab control in case of error, so normal operations will not be disrupted. The only feature that will not work if you have a buggy client or a buggy server is the keytab retrieval option, as that feature is only available with the new control. Given we have only recently introduced CLI and UI to actually enable the use of the retrieval option and given the fact 4.x has not yet hit major distribution stable releases I think that this patchset is acceptable as long as we can land it in 4.1.2 and/or in an immediately following patch release (also in 4.0.x possibly) so that it can land as a zero day upgrade for Fedora (and an upgrade for Debian). If you have any questions, please shoot. This code is fully tested by me on top of master. I think it should apply directly on 4.1.2 and 4.0.x with none or minor changes. Patch 0001: Typo in the commit message. Otherwise LGTM. Patch 0002: I would strongly prefer that generated code live in its own directory and static library. Then the wrapper around that code should make its own library and link to the library for the generated code. Something like: * asn1/asn1c/libasn1c.a * asn1/libipaasn1.a Also, shouldn't the functions in ipa_asn1.[ch] get a prefix? Maybe ipa_asn1_? I'd love to see some function documentation in ipa_asn1.h. At the least, this should cover allocation semantics. Are there any changes in KeytabModule itself from the previous implementation? It looks like yes. NewKeys.enctypes for instance used to be SEQUENCE OF Int16, but is now SEQUENCE OF INTEGER. Isn't the Kerberos enctype Int32? Why not use this? Same with GKReply.newkvno (new_kvno), KrbKey.key, KrbKey.salt, etc. It seems to me like you're trying to resist pulling in the Kerberos ASN.1 module. If this is the only reason, it seems to me like we'll probably need it eventually anyway and we can just configure the compiler to drop the unused symbols. But maybe I'm missing something. Why does lber.h get added to ipa_krb5.h? Aren't we trying to get rid of lber with this patch? Nathaniel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] Improve developers RPM builds
On Mon, 2014-11-17 at 23:42 -0500, Simo Sorce wrote: This small patch will add a date component to the ipa version when building straight from git. This way successive builds will clearly install on top of a previous one even if alphabetically the git commit id has gone backwards. It also gives a chance of remembering what/when you had installed on older VMs at a simply rpm -qi glance. ACK And thank you! This has been bugging me. Nathaniel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0161] Fix dyndb-ldap working dir permission
On 18/11/14 15:01, Jan Cholasta wrote: Hi, Dne 13.11.2014 v 14:50 Martin Basti napsal(a): On 13/11/14 13:59, Jan Cholasta wrote: Dne 12.11.2014 v 13:33 Martin Basti napsal(a): On 11/11/14 16:58, Jan Cholasta wrote: Hi, Dne 11.11.2014 v 16:22 Martin Basti napsal(a): Using specfile to create file doesn't work if named user is not on system. Appropriate permission have to be set during ipa-dns installation. Patch attached Why is the directory set up in dnskeysyncinstance instead of bindinstance? Because, dnskeysyncinstance is the daemon which requires permission change. (dir is created by dyndb-ldap plugin) OK. But please rename the method to something more suitable (fix_dyndb_ldap_workdir_permissions?) and add a docstring/comment. Also please change the ticket link to https://fedorahosted.org/freeipa/ticket/4716 (cloned from BZ). The original patch was released with 4.1.1, shouldn't there be update in ipa-upgradeconfig? Cases: 1) fresh RPM install, no named user during RPM install - named doesn't start, user had to fix it immediately, can't wait until next release. 2) fresh RPM install, named user - no impact 3) upgrade IPA with DNS - no impact 4) upgrade IPA without DNS - after DNS installation, same as 1) 5) IPA 4.1.0 with installed DNS, upgrade to 4.1.2 - DNSSEC will not work (If user doesnt use DNSSEC) Only 5) looks serious for me, so here is updated patch. Could you do the update without the code duplication? In similar code an appropriate *instance method is usually called. The uid/gid resolution in ipa-upgradeconfig still looks like duplicated code to me. I would suggest doing something along these lines in ipa-upgradeconfig: dnskeysync = dnskeysyncinstance.DNSKeySyncInstance() dnskeysync.set_dyndb_ldap_workdir_permissions() and have DNSKeySyncInstance.set_dyndb_ldap_workdir_permissions() do all the real work. Updated patch attached. Martin^2 Martin^2 Honza Honza Thanks. updated patch attached. Martin^2 Honza -- Martin Basti From 59b6e540f03898ffc93621a3eab74b7e07974728 Mon Sep 17 00:00:00 2001 From: Martin Basti mba...@redhat.com Date: Tue, 11 Nov 2014 13:00:18 +0100 Subject: [PATCH] Fix named working directory permissions Just adding dir to specfile doesnt work, because is not guarantee the named is installed, during RPM installation. Ticket: https://fedorahosted.org/freeipa/ticket/4716 --- freeipa.spec.in | 3 +-- install/tools/ipa-upgradeconfig | 14 + ipaplatform/base/paths.py | 1 + ipaserver/install/dnskeysyncinstance.py | 36 +++-- 4 files changed, 46 insertions(+), 8 deletions(-) diff --git a/freeipa.spec.in b/freeipa.spec.in index 36c2a35e7a0c60d4f68e2d945688ee30506e47c6..d0e9f910e2247ce1620e9b62f412d43ff663652d 100644 --- a/freeipa.spec.in +++ b/freeipa.spec.in @@ -420,7 +420,6 @@ mkdir -p %{buildroot}%{_usr}/share/ipa/html/ /bin/touch %{buildroot}%{_usr}/share/ipa/html/preferences.html mkdir -p %{buildroot}%{_initrddir} mkdir %{buildroot}%{_sysconfdir}/sysconfig/ -mkdir -p %{buildroot}%{_localstatedir}/named/dyndb-ldap/ipa/ install -m 644 init/ipa_memcached.conf %{buildroot}%{_sysconfdir}/sysconfig/ipa_memcached install -m 644 init/ipa-dnskeysyncd.conf %{buildroot}%{_sysconfdir}/sysconfig/ipa-dnskeysyncd install -m 644 init/ipa-ods-exporter.conf %{buildroot}%{_sysconfdir}/sysconfig/ipa-ods-exporter @@ -660,7 +659,6 @@ fi %config(noreplace) %{_sysconfdir}/sysconfig/ipa-ods-exporter %dir %attr(0700,apache,apache) %{_localstatedir}/run/ipa_memcached/ %dir %attr(0700,root,root) %{_localstatedir}/run/ipa/ -%dir %attr(0770,named,named) %{_localstatedir}/named/dyndb-ldap/ipa/ # NOTE: systemd specific section %{_tmpfilesdir}/%{name}.conf %attr(644,root,root) %{_unitdir}/ipa.service @@ -774,6 +772,7 @@ fi %attr(700,root,root) %dir %{_localstatedir}/lib/ipa/sysupgrade %attr(755,root,root) %dir %{_localstatedir}/lib/ipa/pki-ca %ghost %{_localstatedir}/lib/ipa/pki-ca/publish +%ghost %{_localstatedir}/named/dyndb-ldap/ipa %attr(755,root,root) %{_libdir}/krb5/plugins/kdb/ipadb.so %{_mandir}/man1/ipa-replica-conncheck.1.gz %{_mandir}/man1/ipa-replica-install.1.gz diff --git a/install/tools/ipa-upgradeconfig b/install/tools/ipa-upgradeconfig index 6556d8f313d3a9efeb32d4cba97cb82796459652..b0b574476ffc5ce6f075cf46177cc059483551ab 100644 --- a/install/tools/ipa-upgradeconfig +++ b/install/tools/ipa-upgradeconfig @@ -30,6 +30,7 @@ import shutil import pwd import fileinput import ConfigParser +import grp from ipalib import api import SSSDConfig @@ -1161,6 +1162,18 @@ def mask_named_regular(): return False +def fix_dyndb_ldap_workdir_permissions(): +Fix dyndb-ldap working dir permissions. DNSSEC daemons requires it +if sysupgrade.get_upgrade_state('dns', 'dyndb_ipa_workdir_perm'): +return + +if bindinstance.named_conf_exists(): +root_logger.info('[Fix bind-dyndb-ldap IPA working
Re: [Freeipa-devel] [PATCH 0078] Enable QR code display by default in otptoken-add
On Tue, 2014-11-18 at 07:45 -0500, Simo Sorce wrote: On Tue, 18 Nov 2014 12:27:28 +0100 Martin Kosek mko...@redhat.com wrote: On 11/14/2014 08:29 PM, Simo Sorce wrote: On Fri, 14 Nov 2014 20:05:35 +0100 Petr Viktorin pvikt...@redhat.com wrote: On 11/14/2014 08:03 PM, Petr Viktorin wrote: On 11/14/2014 07:26 PM, Simo Sorce wrote: On Fri, 14 Nov 2014 14:08:24 +0100 Petr Viktorin pvikt...@redhat.com wrote: On 11/14/2014 01:18 PM, Petr Vobornik wrote: [...] Nope, defaults are filled in by the client. (And also on the server if they're still missing; it's part of the common validation.) IMHO this is quite unfortunate behavior which may also fail horribly if there is a newer client and an older server - backwards compatibility is on API level, not CLI level. Defaults should be filled by server, not a client. We should seriously reconsider the design of our CLI. But that's for different, future discussion. You can't use a newer client with an older server, you get a VersionError in that case. Does it break only for this command ? Or in general. In general. It's been built into the framework since IPA 2.0 [0]. There have been four years of development assuming this compatibility scheme. I should clarify – this is only for the API, i.e. the `ipa` command. Clients of the ipa-client-install sort don't use the API. I know they don't or it would be a disaster. However it is unreasonable to keep changing the API without any 2 way compatibility going forward. I expect it to be extremely common for an admin to have a much more recent desktop then the server being used. Having to jump through hoops to use the admin cli is not friendly. And we do not change the actual CLI that much, so it would be unexpected. We need to take seriously in consideration compatibility going both ways (of course new commands should just get NotSupported errors when used against old servers. But old commands should work, there is no good reason to break this kind of compatibility, it is just an artefact of botched versioning we did a few years ago and it is about time we seriously address this, also because once we make one of these APIs public and supported we cannot willy nilly break it, and we cannot force people to change their software if all works well except a version number being sent in and out. Individual interfaces need to be versioned, and one an interface is release it must no change (a new version need to be created if changes are needed). Well, it is what it is. This paradigm (forward compatibility only) was there since the beginning (read - IPA 2.0) and we cannot change it that simply - it is big effort on it's own that needs to be planned, designed, implemented. And needs to be changed, it always was supposed to be temporary, and it is time to change it. now that we have various distributions and not just Fedora with FreeIPA we cannot make the ipa command useless unless you happen to have the same exact version that you have on the server, normally clients are always a few versions ahead of servers which move more slowly. To solve this, you would need to introduce some kind of version/metadata handshake between new client and old server so that the clients knows what API does the old server expects. It would need to know which attributes were changed/added in incompatible way between it's and server's version. No, this would be the wrong way to go. The solution is the same Linux adopted for ABI compatibility in libraries. We version the single API command, all we need to do is add a version field in the json structure (or even just in the command name). Any time people want to change a command a new command is created instead and the old one stays around for older clients. This is how all successful and backwards compatible RPC APIs are built, and we need to follow suite asap. +1 However, this is probably 4.2 material, no? If so, let's file a bug and schedule it. This patch still needs to land in 4.1.2, so is it okay as it is? Nathaniel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0674 Raise an error when a critical DS task fails
On 18.11.2014 16:28, Petr Viktorin wrote: Backup and restore depend on DS tasks succeeding, but they only waited for the task to end and didn't check the result. This patch adds a new helper, run_task, that runs a task, waits for it, and checks the result. Backup and restore are changed to use this helper. Does not fix, but helps with debugging of issues such as: * https://fedorahosted.org/freeipa/ticket/4712 * https://fedorahosted.org/freeipa/ticket/4736 (duplicate of 4712) If ACKed I would also assign it to the ticket 4712. -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0078] Enable QR code display by default in otptoken-add
On 18.11.2014 17:27, Nathaniel McCallum wrote: On Tue, 2014-11-18 at 07:45 -0500, Simo Sorce wrote: On Tue, 18 Nov 2014 12:27:28 +0100 Martin Kosek mko...@redhat.com wrote: On 11/14/2014 08:29 PM, Simo Sorce wrote: On Fri, 14 Nov 2014 20:05:35 +0100 Petr Viktorin pvikt...@redhat.com wrote: On 11/14/2014 08:03 PM, Petr Viktorin wrote: On 11/14/2014 07:26 PM, Simo Sorce wrote: On Fri, 14 Nov 2014 14:08:24 +0100 Petr Viktorin pvikt...@redhat.com wrote: On 11/14/2014 01:18 PM, Petr Vobornik wrote: [...] Nope, defaults are filled in by the client. (And also on the server if they're still missing; it's part of the common validation.) IMHO this is quite unfortunate behavior which may also fail horribly if there is a newer client and an older server - backwards compatibility is on API level, not CLI level. Defaults should be filled by server, not a client. We should seriously reconsider the design of our CLI. But that's for different, future discussion. You can't use a newer client with an older server, you get a VersionError in that case. Does it break only for this command ? Or in general. In general. It's been built into the framework since IPA 2.0 [0]. There have been four years of development assuming this compatibility scheme. I should clarify – this is only for the API, i.e. the `ipa` command. Clients of the ipa-client-install sort don't use the API. I know they don't or it would be a disaster. However it is unreasonable to keep changing the API without any 2 way compatibility going forward. I expect it to be extremely common for an admin to have a much more recent desktop then the server being used. Having to jump through hoops to use the admin cli is not friendly. And we do not change the actual CLI that much, so it would be unexpected. We need to take seriously in consideration compatibility going both ways (of course new commands should just get NotSupported errors when used against old servers. But old commands should work, there is no good reason to break this kind of compatibility, it is just an artefact of botched versioning we did a few years ago and it is about time we seriously address this, also because once we make one of these APIs public and supported we cannot willy nilly break it, and we cannot force people to change their software if all works well except a version number being sent in and out. Individual interfaces need to be versioned, and one an interface is release it must no change (a new version need to be created if changes are needed). Well, it is what it is. This paradigm (forward compatibility only) was there since the beginning (read - IPA 2.0) and we cannot change it that simply - it is big effort on it's own that needs to be planned, designed, implemented. And needs to be changed, it always was supposed to be temporary, and it is time to change it. now that we have various distributions and not just Fedora with FreeIPA we cannot make the ipa command useless unless you happen to have the same exact version that you have on the server, normally clients are always a few versions ahead of servers which move more slowly. To solve this, you would need to introduce some kind of version/metadata handshake between new client and old server so that the clients knows what API does the old server expects. It would need to know which attributes were changed/added in incompatible way between it's and server's version. No, this would be the wrong way to go. The solution is the same Linux adopted for ABI compatibility in libraries. We version the single API command, all we need to do is add a version field in the json structure (or even just in the command name). Any time people want to change a command a new command is created instead and the old one stays around for older clients. This is how all successful and backwards compatible RPC APIs are built, and we need to follow suite asap. +1 However, this is probably 4.2 material, no? If so, let's file a bug and schedule it. Yes, https://fedorahosted.org/freeipa/ticket/4739 This patch still needs to land in 4.1.2, so is it okay as it is? I don't think the label is necessary but it doesn't hurt either, at least it's clear, so ACK. Nathaniel -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] Fix getkeytab operation
On Tue, 18 Nov 2014 11:23:42 -0500 Nathaniel McCallum npmccal...@redhat.com wrote: On Tue, 2014-11-18 at 00:01 -0500, Simo Sorce wrote: Hello team, Recently Alexander opened the following bug: https://fedorahosted.org/freeipa/ticket/4718 The problem with this code is that manual ASN.1 encoding is fragile and too prone to error, and I found various issues while investigating the bug. So I decided to give a shot at replacing the fragile manual code with more robust code autogenerated by the asn1c compiler. While working on replacing the code with the autogenerated one I discovered additional encoding issues of which the following ticket represent only the most evident: https://fedorahosted.org/freeipa/ticket/4728 I found numerous encoding errors which basically made the getkeytab control we implemented in both the client and the server not respect the encoding we actually defined with ASN.1 notation here: http://www.freeipa.org/page/V4/Keytab_Retrieval While digging and testing replacement functions is also became evident that the getkeytab feature was only partially working and that the bug in 4718 was not just a minor error, but cannot ever work on existing servers as there are compounding bugs that would prevent using the getkeytab protocol to ever work if the user specified enctypes via ipa-getkeytab instead of just requesting the server's defaults. Because of this and because it was just too hard *and* useless to try to be compatible with existing broken clients and servers the new code breaks compatibility for correctness of implementation. The break in compatibility is mitigated by the fact that ipa-getkeytab always falls back to the old setkeytab control in case of error, so normal operations will not be disrupted. The only feature that will not work if you have a buggy client or a buggy server is the keytab retrieval option, as that feature is only available with the new control. Given we have only recently introduced CLI and UI to actually enable the use of the retrieval option and given the fact 4.x has not yet hit major distribution stable releases I think that this patchset is acceptable as long as we can land it in 4.1.2 and/or in an immediately following patch release (also in 4.0.x possibly) so that it can land as a zero day upgrade for Fedora (and an upgrade for Debian). If you have any questions, please shoot. This code is fully tested by me on top of master. I think it should apply directly on 4.1.2 and 4.0.x with none or minor changes. Patch 0001: Typo in the commit message. Otherwise LGTM. Patch 0002: I would strongly prefer that generated code live in its own directory and static library. Then the wrapper around that code should make its own library and link to the library for the generated code. Something like: * asn1/asn1c/libasn1c.a * asn1/libipaasn1.a Although I can see the logic, it sounds a little bit extreme to build a convenience library for a convenience library ... what's the gain ? The ipa_asn1.c code is intimately tied to the autogenerated code anyway. Also, shouldn't the functions in ipa_asn1.[ch] get a prefix? Maybe ipa_asn1_? uhmm maybe we should indeed. any other opinion ? I'd love to see some function documentation in ipa_asn1.h. At the least, this should cover allocation semantics. Yeah, I added a comment or two in the .c file but did not make it into .h file, I'll fix that. Are there any changes in KeytabModule itself from the previous implementation? It looks like yes. NewKeys.enctypes for instance used to be SEQUENCE OF Int16, but is now SEQUENCE OF INTEGER. Isn't the Kerberos enctype Int32? Why not use this? INTEGER is really all we need. Same with GKReply.newkvno (new_kvno), KrbKey.key, KrbKey.salt, etc. It seems to me like you're trying to resist pulling in the Kerberos ASN.1 module. If this is the only reason, it seems to me like we'll probably need it eventually anyway and we can just configure the compiler to drop the unused symbols. It would be a lot of code we do not need. I could import just the Int32 definition, but why ? INTEGER works fine for our purposes (we use system defined integers so it is limited to a 'long'). But maybe I'm missing something. Why does lber.h get added to ipa_krb5.h? Aren't we trying to get rid of lber with this patch? Because the header file uses struct berval in a function I am not touching, so it need the include to avoid compile warnings, eventually we may change things around to improve minor details, but this patch is blocking for Fedora 21 so I would like to avoid anything that is not a hard blocker. 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 0161] Fix dyndb-ldap working dir permission
Dne 18.11.2014 v 16:53 Martin Basti napsal(a): On 18/11/14 15:01, Jan Cholasta wrote: Hi, Dne 13.11.2014 v 14:50 Martin Basti napsal(a): On 13/11/14 13:59, Jan Cholasta wrote: Dne 12.11.2014 v 13:33 Martin Basti napsal(a): On 11/11/14 16:58, Jan Cholasta wrote: Hi, Dne 11.11.2014 v 16:22 Martin Basti napsal(a): Using specfile to create file doesn't work if named user is not on system. Appropriate permission have to be set during ipa-dns installation. Patch attached Why is the directory set up in dnskeysyncinstance instead of bindinstance? Because, dnskeysyncinstance is the daemon which requires permission change. (dir is created by dyndb-ldap plugin) OK. But please rename the method to something more suitable (fix_dyndb_ldap_workdir_permissions?) and add a docstring/comment. Also please change the ticket link to https://fedorahosted.org/freeipa/ticket/4716 (cloned from BZ). The original patch was released with 4.1.1, shouldn't there be update in ipa-upgradeconfig? Cases: 1) fresh RPM install, no named user during RPM install - named doesn't start, user had to fix it immediately, can't wait until next release. 2) fresh RPM install, named user - no impact 3) upgrade IPA with DNS - no impact 4) upgrade IPA without DNS - after DNS installation, same as 1) 5) IPA 4.1.0 with installed DNS, upgrade to 4.1.2 - DNSSEC will not work (If user doesnt use DNSSEC) Only 5) looks serious for me, so here is updated patch. Could you do the update without the code duplication? In similar code an appropriate *instance method is usually called. The uid/gid resolution in ipa-upgradeconfig still looks like duplicated code to me. I would suggest doing something along these lines in ipa-upgradeconfig: dnskeysync = dnskeysyncinstance.DNSKeySyncInstance() dnskeysync.set_dyndb_ldap_workdir_permissions() and have DNSKeySyncInstance.set_dyndb_ldap_workdir_permissions() do all the real work. Updated patch attached. Martin^2 Thanks, ACK. Pushed to: master: 7c176b708eb855ea8774ad36ba72fd31952a8895 ipa-4-1: ba124045b9f39f8264a974c977beba6f15b1b1fb -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 373 Update Requires on pki-ca to 10.2.1-0.1
Hi, the attached patch fixes https://fedorahosted.org/freeipa/ticket/4645. Honza -- Jan Cholasta From d022389ef15101fca108ec2be9b88b417f369dc3 Mon Sep 17 00:00:00 2001 From: Jan Cholasta jchol...@redhat.com Date: Tue, 18 Nov 2014 14:57:17 + Subject: [PATCH] Update Requires on pki-ca to 10.2.1-0.1 https://fedorahosted.org/freeipa/ticket/4645 --- freeipa.spec.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/freeipa.spec.in b/freeipa.spec.in index b2ff97a..52e6436 100644 --- a/freeipa.spec.in +++ b/freeipa.spec.in @@ -130,7 +130,7 @@ Requires(post): systemd-units Requires: selinux-policy = %{selinux_policy_version} Requires(post): selinux-policy-base Requires: slapi-nis = 0.54.1-1 -Requires: pki-ca = 10.2.0-3 +Requires: pki-ca = 10.2.1-0.1 %if 0%{?rhel} Requires: subscription-manager %endif -- 2.1.0 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH 0165] Workaround: unable to get CA status during update
Ticket: https://fedorahosted.org/freeipa/ticket/4676 Attached patches: * Version A: uses wget to get status of CA * Version B: write warning instead of raising exception (error is false positive, CA is running) I'm open to suggestions which approach is better Martin^2 -- Martin Basti From 6ca8f768beacc3328a7911f23afc433404ba871e Mon Sep 17 00:00:00 2001 From: Martin Basti mba...@redhat.com Date: Tue, 18 Nov 2014 19:49:15 +0100 Subject: [PATCH] Using wget to get status of CA This is just workaround Ticket: https://fedorahosted.org/freeipa/ticket/4676 --- ipaplatform/redhat/services.py | 21 - 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/ipaplatform/redhat/services.py b/ipaplatform/redhat/services.py index e032b57778cce5e3169910d1e0ebd9902aff1838..10a082e34f5cf9c7e3ba1a2fa96d498c4fcbd386 100644 --- a/ipaplatform/redhat/services.py +++ b/ipaplatform/redhat/services.py @@ -24,6 +24,7 @@ Contains Red Hat OS family-specific service class implementations. import os import time +import xml.dom.minidom from ipaplatform.tasks import tasks from ipaplatform.base import services as base_services @@ -185,7 +186,25 @@ class RedHatCAService(RedHatService): op_timeout = time.time() + timeout while time.time() op_timeout: try: -status = dogtag.ca_status(use_proxy=use_proxy) +# FIXME +# workaround https://fedorahosted.org/freeipa/ticket/4716 +# status = dogtag.ca_status(use_proxy=use_proxy) +url = https://; + api.env.ca_host + /ca/admin/ca/getStatus +args = [ +paths.BIN_WGET, +'-S', '-O', '-', +'--timeout=30', +url +] + +stdout, stderr, returncode = ipautil.run(args) + +#parse body +doc = xml.dom.minidom.parseString(stdout) +item_node = doc.getElementsByTagName(XMLResponse)[0] +item_node = item_node.getElementsByTagName(Status)[0] +status = item_node.childNodes[0].data +# end of workaround except Exception: status = 'check interrupted' import traceback -- 1.8.3.1 From 61508160f5ce2947c78a4e1fd1319ddee538b7bc Mon Sep 17 00:00:00 2001 From: Martin Basti mba...@redhat.com Date: Tue, 18 Nov 2014 18:30:59 +0100 Subject: [PATCH] Show warning instead of error if CA did not start This is just workaround, checking if CA is working raises false positive exception during upgrade Ticket: https://fedorahosted.org/freeipa/ticket/4676 --- install/tools/ipa-upgradeconfig | 4 1 file changed, 4 insertions(+) diff --git a/install/tools/ipa-upgradeconfig b/install/tools/ipa-upgradeconfig index b81a474b2bb14f1582dabd649400c13f7ce6d369..02bfe3a79f83e65f428fe2220d940eb39fdbd928 100644 --- a/install/tools/ipa-upgradeconfig +++ b/install/tools/ipa-upgradeconfig @@ -1473,6 +1473,10 @@ def main(): ca.restart(dogtag.configured_constants().PKI_INSTANCE_NAME) except ipautil.CalledProcessError, e: root_logger.error(Failed to restart %s: %s, ca.service_name, e) +# FIXME https://fedorahosted.org/freeipa/ticket/4676 +# workaround +except RuntimeError as e: +root_logger.warning(str(e)) set_sssd_domain_option('ipa_server_mode', 'True') -- 1.8.3.1 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] Fix getkeytab operation
On Tue, 2014-11-18 at 12:40 -0500, Simo Sorce wrote: On Tue, 18 Nov 2014 11:23:42 -0500 Nathaniel McCallum npmccal...@redhat.com wrote: On Tue, 2014-11-18 at 00:01 -0500, Simo Sorce wrote: Hello team, Recently Alexander opened the following bug: https://fedorahosted.org/freeipa/ticket/4718 The problem with this code is that manual ASN.1 encoding is fragile and too prone to error, and I found various issues while investigating the bug. So I decided to give a shot at replacing the fragile manual code with more robust code autogenerated by the asn1c compiler. While working on replacing the code with the autogenerated one I discovered additional encoding issues of which the following ticket represent only the most evident: https://fedorahosted.org/freeipa/ticket/4728 I found numerous encoding errors which basically made the getkeytab control we implemented in both the client and the server not respect the encoding we actually defined with ASN.1 notation here: http://www.freeipa.org/page/V4/Keytab_Retrieval While digging and testing replacement functions is also became evident that the getkeytab feature was only partially working and that the bug in 4718 was not just a minor error, but cannot ever work on existing servers as there are compounding bugs that would prevent using the getkeytab protocol to ever work if the user specified enctypes via ipa-getkeytab instead of just requesting the server's defaults. Because of this and because it was just too hard *and* useless to try to be compatible with existing broken clients and servers the new code breaks compatibility for correctness of implementation. The break in compatibility is mitigated by the fact that ipa-getkeytab always falls back to the old setkeytab control in case of error, so normal operations will not be disrupted. The only feature that will not work if you have a buggy client or a buggy server is the keytab retrieval option, as that feature is only available with the new control. Given we have only recently introduced CLI and UI to actually enable the use of the retrieval option and given the fact 4.x has not yet hit major distribution stable releases I think that this patchset is acceptable as long as we can land it in 4.1.2 and/or in an immediately following patch release (also in 4.0.x possibly) so that it can land as a zero day upgrade for Fedora (and an upgrade for Debian). If you have any questions, please shoot. This code is fully tested by me on top of master. I think it should apply directly on 4.1.2 and 4.0.x with none or minor changes. Patch 0001: Typo in the commit message. Otherwise LGTM. Patch 0002: I would strongly prefer that generated code live in its own directory and static library. Then the wrapper around that code should make its own library and link to the library for the generated code. Something like: * asn1/asn1c/libasn1c.a * asn1/libipaasn1.a Although I can see the logic, it sounds a little bit extreme to build a convenience library for a convenience library ... what's the gain ? The ipa_asn1.c code is intimately tied to the autogenerated code anyway. Moving the files to a new directory and creating a new libtool library can hardly be called extreme. It would probably take 5 minutes of work. It probably took you longer to respond to this email. :) I think it adds a great deal of readability to a new programmer approaching this code. And that, for me, is worth it. Also, shouldn't the functions in ipa_asn1.[ch] get a prefix? Maybe ipa_asn1_? uhmm maybe we should indeed. any other opinion ? I'd love to see some function documentation in ipa_asn1.h. At the least, this should cover allocation semantics. Yeah, I added a comment or two in the .c file but did not make it into .h file, I'll fix that. Are there any changes in KeytabModule itself from the previous implementation? It looks like yes. NewKeys.enctypes for instance used to be SEQUENCE OF Int16, but is now SEQUENCE OF INTEGER. Isn't the Kerberos enctype Int32? Why not use this? INTEGER is really all we need. Same with GKReply.newkvno (new_kvno), KrbKey.key, KrbKey.salt, etc. It seems to me like you're trying to resist pulling in the Kerberos ASN.1 module. If this is the only reason, it seems to me like we'll probably need it eventually anyway and we can just configure the compiler to drop the unused symbols. It would be a lot of code we do not need. That would be automatically generated once and probably never touched again. I could import just the Int32 definition, but why ? INTEGER works fine for our purposes (we use system defined integers so it is limited to a 'long'). It works and is easier, at the expense of readability (and perhaps some subtle bugs later for the other types). It really just seems odd to me to
Re: [Freeipa-devel] [PATCH 0074] Make token window sizes configurable
On 13.11.2014 08:53, Martin Kosek wrote: On 11/13/2014 08:51 AM, Nathaniel McCallum wrote: On Thu, 2014-11-13 at 08:48 +0100, Martin Kosek wrote: On 11/12/2014 11:37 PM, Nathaniel McCallum wrote: On Mon, 2014-11-10 at 08:28 +0100, Martin Kosek wrote: On 11/07/2014 04:44 PM, Petr Vobornik wrote: On 7.11.2014 08:58, Martin Kosek wrote: On 11/04/2014 05:17 PM, Nathaniel McCallum wrote: On Wed, 2014-10-29 at 09:34 -0400, Nathaniel McCallum wrote: On Wed, 2014-10-29 at 12:21 +0100, Petr Viktorin wrote: On 10/29/2014 10:37 AM, Martin Kosek wrote: On 10/28/2014 09:59 PM, Nathaniel McCallum wrote: On Thu, 2014-10-23 at 18:07 -0400, Nathaniel McCallum wrote: This patch gives the administrator variables to control the size of the authentication and synchronization windows for OTP tokens. https://fedorahosted.org/freeipa/ticket/4511 NOTE: There is one known issue with this patch which I don't know how to solve. This patch changes the schema in install/share/60ipaconfig.ldif. On an upgrade, all of the new attributeTypes appear correctly. However, the modifications to the pre-existing objectClass do not show up on the server. What am I doing wrong? After modifying ipaGuiConfig manually, everything in this patch works just fine. This new version takes into account the new (proper) OIDs and attribute names. Thanks Nathaniel! The above known issue still remains. Petr3, any idea what could have gone wrong? ObjectClass MAY list extension should work just fine, AFAIK. You added a blank line to the LDIF file. This is an entry separator, so the objectClasses after the blank line don't belong to cn=schema, so they aren't considered in the update. Without the blank line it works fine. Thanks for the catch! Here is a version without the blank line. I forgot to remove the old steps defines. This patch performs this cleanup. I am now wondering, is the global config object really the nest place to add these OTP specific settings? I would prefer not to overload the object and instead: - create new ipaOTPConfig objectclass - add it to cn=otp,$SUFFIX - create otpconfig-mod and otpconfig-show commands to follow an example of dnsconfig-* and trustconfig-* commands IMO, this would allow more flexibility for the OTP settings and would also scale better for the future updates. +1 I will comment the patch as if ^^ would not exist because it will still be needed in the new plugin. Because of ^^ I did not test, just read. 1. Got: install/ui/src/freeipa/serverconfig.js(135): lint warning: extra comma is not recommended in array initializers Please run: jsl -nofilelisting -nosummary -nologo -conf jsl.conf in install/ui directory The goal is no have no warnings and errors. 2. new attrs should be added to 'System: Read Global Configuration' managed permission +1. Though if we go with OTP config, it should be called System: Read OTP Configuration Martin Attached is a new set of patches that replaces this single patch. This now fixes multiple issues. I now create two new entries: * cn=TOTP,cn=Token Config,cn=etc,$SUFFIX * cn=HOTP,cn=Token Config,cn=etc,$SUFFIX There are two corresponding CLI commands: * totpconfig-(show|mod) * hotpconfig-(show|mod) There is no UI support for this yet (pointers welcome). This is designed so that eventually tokens can grow a per-token override, but I have not yet implemented this feature (it should be easy in the future). Additionally, I had to do some shared refactoring to address issues in ipa-otp-lasttoken, which is why all of these are now merged into a single patch set. Nathaniel I'm little confused with a state of reviews. Thierry were some of the patches ACKed in different threads or are they under review (I'm not reviewing DS plugin parts)? Ah, I meant adding the token config to cn=otp,SUFFIX directly, but if we want to make TOTP/HOTP token config as separate entries (to enable future per-token overrides), your approach should make sense. Rather adding Rob to CC for sanity. That would work too. I'm open to that. I am just not sure we should create them as separate plugins, I think the new commands should be rather added to otp plugin directly so that they show in ipa help otptoken instead of adding 2 new topics just for OTP config. I can play with that. Do you plan to change it? I like the idea of a single point of help for OTP but I'm also unsure about the length of the commands. Current solution is also more consistent with a rest of the framework. Would it be something like: otptoken-totpconfig-(show|mod) otptoken-hotpconfig-(show|mod) Maybe it would be better to introduce more help topics for otp. This concept is used for HBAC already: $ ipa help hbac hbacsvcgroup HBAC Service Groups hbacsvc HBAC Services hbacrule Host-based access control $ ipa help hbacrule Host-based access control ... a lot of text So we could introduce otp umbrella topic: $ ipa help otp opttoken
Re: [Freeipa-devel] [PATCHES] Fix getkeytab operation
On Tue, 18 Nov 2014 14:28:08 -0500 Nathaniel McCallum npmccal...@redhat.com wrote: On Tue, 2014-11-18 at 12:40 -0500, Simo Sorce wrote: On Tue, 18 Nov 2014 11:23:42 -0500 Nathaniel McCallum npmccal...@redhat.com wrote: On Tue, 2014-11-18 at 00:01 -0500, Simo Sorce wrote: Hello team, Recently Alexander opened the following bug: https://fedorahosted.org/freeipa/ticket/4718 The problem with this code is that manual ASN.1 encoding is fragile and too prone to error, and I found various issues while investigating the bug. So I decided to give a shot at replacing the fragile manual code with more robust code autogenerated by the asn1c compiler. While working on replacing the code with the autogenerated one I discovered additional encoding issues of which the following ticket represent only the most evident: https://fedorahosted.org/freeipa/ticket/4728 I found numerous encoding errors which basically made the getkeytab control we implemented in both the client and the server not respect the encoding we actually defined with ASN.1 notation here: http://www.freeipa.org/page/V4/Keytab_Retrieval While digging and testing replacement functions is also became evident that the getkeytab feature was only partially working and that the bug in 4718 was not just a minor error, but cannot ever work on existing servers as there are compounding bugs that would prevent using the getkeytab protocol to ever work if the user specified enctypes via ipa-getkeytab instead of just requesting the server's defaults. Because of this and because it was just too hard *and* useless to try to be compatible with existing broken clients and servers the new code breaks compatibility for correctness of implementation. The break in compatibility is mitigated by the fact that ipa-getkeytab always falls back to the old setkeytab control in case of error, so normal operations will not be disrupted. The only feature that will not work if you have a buggy client or a buggy server is the keytab retrieval option, as that feature is only available with the new control. Given we have only recently introduced CLI and UI to actually enable the use of the retrieval option and given the fact 4.x has not yet hit major distribution stable releases I think that this patchset is acceptable as long as we can land it in 4.1.2 and/or in an immediately following patch release (also in 4.0.x possibly) so that it can land as a zero day upgrade for Fedora (and an upgrade for Debian). If you have any questions, please shoot. This code is fully tested by me on top of master. I think it should apply directly on 4.1.2 and 4.0.x with none or minor changes. Patch 0001: Typo in the commit message. Otherwise LGTM. Patch 0002: I would strongly prefer that generated code live in its own directory and static library. Then the wrapper around that code should make its own library and link to the library for the generated code. Something like: * asn1/asn1c/libasn1c.a * asn1/libipaasn1.a Although I can see the logic, it sounds a little bit extreme to build a convenience library for a convenience library ... what's the gain ? The ipa_asn1.c code is intimately tied to the autogenerated code anyway. Moving the files to a new directory and creating a new libtool library can hardly be called extreme. It would probably take 5 minutes of work. It probably took you longer to respond to this email. :) I think it adds a great deal of readability to a new programmer approaching this code. And that, for me, is worth it. Ok, it's not the work that is extreme, it is just the additional layering that is a bit silly imo, as the asn1/libipaasn1.a would just be a very thin wrapper, the generated Makefile will probably be bigger then the wrapper it compiles :) but ok. Also, shouldn't the functions in ipa_asn1.[ch] get a prefix? Maybe ipa_asn1_? uhmm maybe we should indeed. any other opinion ? I'd love to see some function documentation in ipa_asn1.h. At the least, this should cover allocation semantics. Yeah, I added a comment or two in the .c file but did not make it into .h file, I'll fix that. Are there any changes in KeytabModule itself from the previous implementation? It looks like yes. NewKeys.enctypes for instance used to be SEQUENCE OF Int16, but is now SEQUENCE OF INTEGER. Isn't the Kerberos enctype Int32? Why not use this? INTEGER is really all we need. Same with GKReply.newkvno (new_kvno), KrbKey.key, KrbKey.salt, etc. It seems to me like you're trying to resist pulling in the Kerberos ASN.1 module. If this is the only reason, it seems to me like we'll probably need it eventually anyway and we can just configure the compiler to
Re: [Freeipa-devel] [PATCHES] Fix getkeytab operation
On Tue, 2014-11-18 at 14:45 -0500, Simo Sorce wrote: On Tue, 18 Nov 2014 14:28:08 -0500 Nathaniel McCallum npmccal...@redhat.com wrote: On Tue, 2014-11-18 at 12:40 -0500, Simo Sorce wrote: On Tue, 18 Nov 2014 11:23:42 -0500 Nathaniel McCallum npmccal...@redhat.com wrote: On Tue, 2014-11-18 at 00:01 -0500, Simo Sorce wrote: Hello team, Recently Alexander opened the following bug: https://fedorahosted.org/freeipa/ticket/4718 The problem with this code is that manual ASN.1 encoding is fragile and too prone to error, and I found various issues while investigating the bug. So I decided to give a shot at replacing the fragile manual code with more robust code autogenerated by the asn1c compiler. While working on replacing the code with the autogenerated one I discovered additional encoding issues of which the following ticket represent only the most evident: https://fedorahosted.org/freeipa/ticket/4728 I found numerous encoding errors which basically made the getkeytab control we implemented in both the client and the server not respect the encoding we actually defined with ASN.1 notation here: http://www.freeipa.org/page/V4/Keytab_Retrieval While digging and testing replacement functions is also became evident that the getkeytab feature was only partially working and that the bug in 4718 was not just a minor error, but cannot ever work on existing servers as there are compounding bugs that would prevent using the getkeytab protocol to ever work if the user specified enctypes via ipa-getkeytab instead of just requesting the server's defaults. Because of this and because it was just too hard *and* useless to try to be compatible with existing broken clients and servers the new code breaks compatibility for correctness of implementation. The break in compatibility is mitigated by the fact that ipa-getkeytab always falls back to the old setkeytab control in case of error, so normal operations will not be disrupted. The only feature that will not work if you have a buggy client or a buggy server is the keytab retrieval option, as that feature is only available with the new control. Given we have only recently introduced CLI and UI to actually enable the use of the retrieval option and given the fact 4.x has not yet hit major distribution stable releases I think that this patchset is acceptable as long as we can land it in 4.1.2 and/or in an immediately following patch release (also in 4.0.x possibly) so that it can land as a zero day upgrade for Fedora (and an upgrade for Debian). If you have any questions, please shoot. This code is fully tested by me on top of master. I think it should apply directly on 4.1.2 and 4.0.x with none or minor changes. Patch 0001: Typo in the commit message. Otherwise LGTM. Patch 0002: I would strongly prefer that generated code live in its own directory and static library. Then the wrapper around that code should make its own library and link to the library for the generated code. Something like: * asn1/asn1c/libasn1c.a * asn1/libipaasn1.a Although I can see the logic, it sounds a little bit extreme to build a convenience library for a convenience library ... what's the gain ? The ipa_asn1.c code is intimately tied to the autogenerated code anyway. Moving the files to a new directory and creating a new libtool library can hardly be called extreme. It would probably take 5 minutes of work. It probably took you longer to respond to this email. :) I think it adds a great deal of readability to a new programmer approaching this code. And that, for me, is worth it. Ok, it's not the work that is extreme, it is just the additional layering that is a bit silly imo, as the asn1/libipaasn1.a would just be a very thin wrapper, the generated Makefile will probably be bigger then the wrapper it compiles :) but ok. As I see it, we're setting out a new precedent. All new ASN.1 code will take this route (which is, indeed, better). So while it is small now, it won't stay small forever. Being that we are in the business of routinely handling ASN.1 stuff, this seems to me like a sensible architecture for the future. Also, shouldn't the functions in ipa_asn1.[ch] get a prefix? Maybe ipa_asn1_? uhmm maybe we should indeed. any other opinion ? I'd love to see some function documentation in ipa_asn1.h. At the least, this should cover allocation semantics. Yeah, I added a comment or two in the .c file but did not make it into .h file, I'll fix that. Are there any changes in KeytabModule itself from the previous implementation? It looks like yes. NewKeys.enctypes for instance
Re: [Freeipa-devel] [PATCH] Improve developers RPM builds
On Tue, 18 Nov 2014 11:25:19 -0500 Nathaniel McCallum npmccal...@redhat.com wrote: On Mon, 2014-11-17 at 23:42 -0500, Simo Sorce wrote: This small patch will add a date component to the ipa version when building straight from git. This way successive builds will clearly install on top of a previous one even if alphabetically the git commit id has gone backwards. It also gives a chance of remembering what/when you had installed on older VMs at a simply rpm -qi glance. ACK And thank you! This has been bugging me. Nathaniel I added your reviewed-by and pushed it to master. Do people want to push it to other branches ? 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] Improve developers RPM builds
On 11/18/2014 09:25 PM, Simo Sorce wrote: On Tue, 18 Nov 2014 11:25:19 -0500 Nathaniel McCallum npmccal...@redhat.com wrote: On Mon, 2014-11-17 at 23:42 -0500, Simo Sorce wrote: This small patch will add a date component to the ipa version when building straight from git. This way successive builds will clearly install on top of a previous one even if alphabetically the git commit id has gone backwards. It also gives a chance of remembering what/when you had installed on older VMs at a simply rpm -qi glance. ACK And thank you! This has been bugging me. Nathaniel I added your reviewed-by and pushed it to master. Do people want to push it to other branches ? Simo. It should be harmless for release builds and should help development builds. So +1 for adding to all actively developed branches, i.e. ipa-4-1 and ipa-4-0. Thanks, Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0165] Workaround: unable to get CA status during update
On 11/18/2014 08:20 PM, Martin Basti wrote: Ticket: https://fedorahosted.org/freeipa/ticket/4676 Attached patches: * Version A: uses wget to get status of CA * Version B: write warning instead of raising exception (error is false positive, CA is running) I'm open to suggestions which approach is better Martin^2 I like A, but I am concerned why you suddenly ignore the use_proxy option. I added it for a reason as it affects to which port we need to connect, regardless the transport library. See https://fedorahosted.org/freeipa/ticket/3973 where I added this option. Second, I am not happy by you duplicating the XML parsing code, I would rather see it splited in dogtag.py in separate _ca_status_parse or similar function call. Given the obstacles, I am inclining for - pushing B as a safe fix for Fedora 21 Final - fixing issues in A and pushing it for minor release after that to avoid the nasty warning and have some reasonable medium-term fix until the framework migrates to something better than httpslib, line python-requests maybe. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 373 Update Requires on pki-ca to 10.2.1-0.1
On Tue, 2014-11-18 at 19:56 +0100, Jan Cholasta wrote: Hi, the attached patch fixes https://fedorahosted.org/freeipa/ticket/4645. ACK ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] Fix getkeytab operation
On Tue, 2014-11-18 at 17:58 -0500, Simo Sorce wrote: On Tue, 18 Nov 2014 15:01:15 -0500 Nathaniel McCallum npmccal...@redhat.com wrote: As I see it, we're setting out a new precedent. All new ASN.1 code will take this route (which is, indeed, better). So while it is small now, it won't stay small forever. Being that we are in the business of routinely handling ASN.1 stuff, this seems to me like a sensible architecture for the future. Ok, I think I should have fixed all the issues you brought up. Still have a typo (wuld) in the commit message of the first patch. :) And my tests still work fine :) Simo. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] Improve developers RPM builds
On Tue, 18 Nov 2014 21:46:00 +0100 Martin Kosek mko...@redhat.com wrote: On 11/18/2014 09:25 PM, Simo Sorce wrote: On Tue, 18 Nov 2014 11:25:19 -0500 Nathaniel McCallum npmccal...@redhat.com wrote: On Mon, 2014-11-17 at 23:42 -0500, Simo Sorce wrote: This small patch will add a date component to the ipa version when building straight from git. This way successive builds will clearly install on top of a previous one even if alphabetically the git commit id has gone backwards. It also gives a chance of remembering what/when you had installed on older VMs at a simply rpm -qi glance. ACK And thank you! This has been bugging me. Nathaniel I added your reviewed-by and pushed it to master. Do people want to push it to other branches ? Simo. It should be harmless for release builds and should help development builds. So +1 for adding to all actively developed branches, i.e. ipa-4-1 and ipa-4-0. Ok, pushed there too. 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] [PATCHES] Fix getkeytab operation
On Tue, 18 Nov 2014 18:00:49 -0500 Nathaniel McCallum npmccal...@redhat.com wrote: On Tue, 2014-11-18 at 17:58 -0500, Simo Sorce wrote: On Tue, 18 Nov 2014 15:01:15 -0500 Nathaniel McCallum npmccal...@redhat.com wrote: As I see it, we're setting out a new precedent. All new ASN.1 code will take this route (which is, indeed, better). So while it is small now, it won't stay small forever. Being that we are in the business of routinely handling ASN.1 stuff, this seems to me like a sensible architecture for the future. Ok, I think I should have fixed all the issues you brought up. Still have a typo (wuld) in the commit message of the first patch. :) I think I can fix it before pushing if that's the only issue ? 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] [PATCHES] Fix getkeytab operation
On Tue, 2014-11-18 at 20:39 -0500, Simo Sorce wrote: On Tue, 18 Nov 2014 18:00:49 -0500 Nathaniel McCallum npmccal...@redhat.com wrote: On Tue, 2014-11-18 at 17:58 -0500, Simo Sorce wrote: On Tue, 18 Nov 2014 15:01:15 -0500 Nathaniel McCallum npmccal...@redhat.com wrote: As I see it, we're setting out a new precedent. All new ASN.1 code will take this route (which is, indeed, better). So while it is small now, it won't stay small forever. Being that we are in the business of routinely handling ASN.1 stuff, this seems to me like a sensible architecture for the future. Ok, I think I should have fixed all the issues you brought up. Still have a typo (wuld) in the commit message of the first patch. :) I think I can fix it before pushing if that's the only issue ? I'll do a more thorough review tomorrow. I haven't even looked at the third patch yet. Nathaniel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH] 374 Fix wrong expiration date on renewed IPA CA certificates
Hi, the attached patch fixes https://fedorahosted.org/freeipa/ticket/4717. Honza -- Jan Cholasta From 871217e002b8a2ee4f58c42977ac680a5305de1a Mon Sep 17 00:00:00 2001 From: Jan Cholasta jchol...@redhat.com Date: Tue, 18 Nov 2014 14:01:59 + Subject: [PATCH] Fix wrong expiration date on renewed IPA CA certificates The expiration date was always set to the expiration date of the original certificate. https://fedorahosted.org/freeipa/ticket/4717 --- freeipa.spec.in | 4 ++-- install/certmonger/dogtag-ipa-ca-renew-agent-submit | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/freeipa.spec.in b/freeipa.spec.in index af36703..b464189 100644 --- a/freeipa.spec.in +++ b/freeipa.spec.in @@ -140,7 +140,7 @@ Requires: python-dns = 1.11.1 Requires: zip Requires: policycoreutils = 2.1.12-5 Requires: tar -Requires(pre): certmonger = 0.75.13 +Requires(pre): certmonger = 0.76.8 Requires(pre): 389-ds-base = 1.3.3.5 Requires: fontawesome-fonts Requires: open-sans-fonts @@ -227,7 +227,7 @@ Requires: wget Requires: libcurl = 7.21.7-2 Requires: xmlrpc-c = 1.27.4 Requires: sssd = 1.12.2 -Requires: certmonger = 0.75.6 +Requires: certmonger = 0.76.8 Requires: nss-tools Requires: bind-utils Requires: oddjob-mkhomedir diff --git a/install/certmonger/dogtag-ipa-ca-renew-agent-submit b/install/certmonger/dogtag-ipa-ca-renew-agent-submit index e5ad963..0a2cff1 100755 --- a/install/certmonger/dogtag-ipa-ca-renew-agent-submit +++ b/install/certmonger/dogtag-ipa-ca-renew-agent-submit @@ -146,6 +146,8 @@ def request_cert(): path = paths.DOGTAG_IPA_RENEW_AGENT_SUBMIT args = [path] + sys.argv[1:] +if os.environ.get('CERTMONGER_CA_PROFILE') == 'caCACert': +args += ['-O', 'bypassCAnotafter=true'] stdout, stderr, rc = ipautil.run(args, raiseonerr=False, env=os.environ) sys.stderr.write(stderr) sys.stderr.flush() -- 2.1.0 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel