Re: [Freeipa-devel] [PATCH 0067] Use stack allocation when writing values during otp auth
On 09/29/2014 06:01 PM, thierry bordaz wrote: On 09/29/2014 05:45 PM, Nathaniel McCallum wrote: On Thu, 2014-09-25 at 13:45 +0200, thierry bordaz wrote: On 09/19/2014 07:49 PM, Nathaniel McCallum wrote: This is an optimization from patch 0062 (rescinded) which I think is worth keeping. There is no ticket for this. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Hello, That is exact that slapi* are doing a intensive usage of the of alloc/free. Using a stack allocated mods would reduce the number of alloc/free but I afraid it will not have a significant impact. For example further slapi_modify_internal is doing tons of alloc/free. Also I think it is not possible to use stack allocated mods. In fact mods will be modified by internal_mod (for example to add 'modifytimestamp', 'modifiersname') and mods will be reallocated. This could also occurs from plugins. Finally if a modify retry occurs, the original mods are freeed. See ldap/servers/slapd/task.c. In this file, everything is stack allocated except for the value itself. Hi Nathaniel, Yes you are correct. This is even a common method :-[ In fact the mods are duplicated/replaced by normalized one at the begining of modify_internal_pb. This is that duplicated ones that can later be extended/freed. Sorry for the noise. The fix is valid. Ack. thanks thierry Pushed to: master: 35ec0f7e3d9832c9b687bb01561a10e0304dfa74 ipa-4-1: ada187f66f3e51467aaf9bd21f74198b73805c3e Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 130] extdom: add support for new version
On 09/29/2014 07:01 PM, Jakub Hrozek wrote: On Mon, Sep 29, 2014 at 06:16:30PM +0200, Sumit Bose wrote: Hi, Jakub found another issue which is fixed with this new version. bye, Sumit and now with patch ... Thank you, ACK Pushed to: master: 3c75b9171e5721097f6ba2855e41f0e4129b907b ipa-4-1: 2006d8759b767364031052480a3fc8947dea0998 Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 336-339 Installer certificate options usability fixes
On 09/29/2014 11:02 PM, Petr Viktorin wrote: On 09/29/2014 04:32 PM, Jan Cholasta wrote: Dne 26.9.2014 v 19:40 Jan Cholasta napsal(a): Dne 26.9.2014 v 17:37 Rob Crittenden napsal(a): Petr Viktorin wrote: On 09/24/2014 06:13 PM, Jan Cholasta wrote: Hi, the attached patches fix https://fedorahosted.org/freeipa/ticket/4480 and https://fedorahosted.org/freeipa/ticket/4489. (Note that design page for this is TBD.) IMO it's OK to just remove them, as they were added during 4.1 development and are not available in any released version of IPA. Ah, OK Added patch 341 for stricter CA certificate validation which fixes https://fedorahosted.org/freeipa/ticket/4477. Updated patches attached. I didn't find any more issues I understood that as an ACK, so I pushed to master, ipa-4-1 (to expedite preparations for 4.1 Alpha). I just had to do a minor conflict resolution for master branch. except of course the missing design page and tests. Any ETA on those? Right, this is still a work to do. I saw Honza created http://www.freeipa.org/page/V4/Installer_CA_options_usability_improvements but it still misses the content. If there are missing tests, please file a ticket so that we have the deficiency tracked. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0033] Remove trivial path constants
Hello Gabe, Thanks for the patch! And thank you for being patient, most people are focusing on wrapping up FreeIPA 4.1 release, so the review forces are limited. Martin On 09/30/2014 05:13 AM, Gabe Alford wrote: Updated patch to fix merge conflicts from recent changes. On Wed, Sep 24, 2014 at 8:34 PM, Gabe Alford redhatri...@gmail.com wrote: Hello, Patch for https://fedorahosted.org/freeipa/ticket/4399. Let me know if I missed any. Thanks, Gabe ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 336-339 Installer certificate options usability fixes
On 09/30/2014 09:00 AM, Martin Kosek wrote: On 09/29/2014 11:02 PM, Petr Viktorin wrote: On 09/29/2014 04:32 PM, Jan Cholasta wrote: Dne 26.9.2014 v 19:40 Jan Cholasta napsal(a): Dne 26.9.2014 v 17:37 Rob Crittenden napsal(a): Petr Viktorin wrote: On 09/24/2014 06:13 PM, Jan Cholasta wrote: Hi, the attached patches fix https://fedorahosted.org/freeipa/ticket/4480 and https://fedorahosted.org/freeipa/ticket/4489. (Note that design page for this is TBD.) IMO it's OK to just remove them, as they were added during 4.1 development and are not available in any released version of IPA. Ah, OK Added patch 341 for stricter CA certificate validation which fixes https://fedorahosted.org/freeipa/ticket/4477. Updated patches attached. I didn't find any more issues I understood that as an ACK, so I pushed to master, ipa-4-1 (to expedite preparations for 4.1 Alpha). I just had to do a minor conflict resolution for master branch. Umh, and also had to fix a subsequent error caused by the merge - pushed as a one liner (attached). Martin From 2421b13a9b8bd79084e9cfe488690057445d7aa7 Mon Sep 17 00:00:00 2001 From: Martin Kosek mko...@redhat.com Date: Tue, 30 Sep 2014 09:35:28 +0200 Subject: [PATCH] Fix ImportError in ipa-ca-install Patch 3aa0731f was not merged correctly and import for a function that no longer exists. This patch fixes the import. https://fedorahosted.org/freeipa/ticket/4480 --- install/tools/ipa-ca-install | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/install/tools/ipa-ca-install b/install/tools/ipa-ca-install index e54af2f59d805a2b8387c6d7fb64bb6b8e7cbd93..c984bf4778403f1a152afed2df567c8399e65809 100755 --- a/install/tools/ipa-ca-install +++ b/install/tools/ipa-ca-install @@ -29,7 +29,7 @@ from ipaserver.install import certs from ipaserver.install.installutils import (HostnameLocalhost, ReplicaConfig, expand_replica_info, read_replica_info, get_host_name, BadHostError, private_ccache, read_replica_info_dogtag_port, load_external_cert, -create_replica_config, validate_external_cert) +create_replica_config) from ipaserver.install import dsinstance, cainstance, bindinstance from ipaserver.install.replication import replica_conn_check from ipapython import version -- 1.9.3 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 319, 324-335 CA management and renewal fixes
Dne 29.9.2014 v 18:09 Rob Crittenden napsal(a): Jan Cholasta wrote: Dne 29.9.2014 v 15:05 Rob Crittenden napsal(a): Jan Cholasta wrote: Dne 26.9.2014 v 17:13 Rob Crittenden napsal(a): Jan Cholasta wrote: Dne 23.9.2014 v 20:39 Rob Crittenden napsal(a): Jan Cholasta wrote: Hi, the attached patches fix various bugs and shortcomings in the CA management and renewal code. Related tickets: https://fedorahosted.org/freeipa/ticket/4416, https://fedorahosted.org/freeipa/ticket/4460. (Patch 319 was originally posted at http://www.redhat.com/archives/freeipa-devel/2014-September/msg00132.html.) Only two of the patches includes what ticket(s) they address. Most have the tersest of commit messages. If got more and more difficult to see why the changes were needed at all, as you'll see. Sorry, fixed (hopefully). Note that most of these patches fix stuff I didn't have time to fix before I posted the original CA management patches, hence the missing tickets. Well, the policy is that every commit should have a ticket. So I guess re-open the old tickets or open new ones. This will help someone in the future know the why of a patch. I've certainly been guilty OK, I will reopen the related tickets. Here is a new set of reviews as trying to intermingle was making my eyes cross: 319: I guess I still don't understand why you can't pull the certs out of LDAP when creating this database. When this code runs, we know that the client is configured, so we have access to authentication. Why can't create_ipa_nssdb pull the certs directly? It seems more robust to me, and the code is already written in ipa-client-install to do this. Well, I don't understand why do you want them to be updated so much, as nothing will break if they are not. Also try to imagine what would happen if, say, 10k clients were updated at the same moment... What's the point of a database missing certificates? It won't be missing any certificates if /etc/pki/nssdb was not missing any certificates before the update. As I said, the update will not break anything. It will not fix anything either, but I don't think this kind of fixing should be done during client RPM upgrade. It is not consistent with anything else we do during client RPM upgrade, it does not scale well and it just does not feel right to me in overall. Ok, I'll concede the point that there is no difference post-update, but it doesn't do what ipa-certupdate does. You can potentially end up with a completely diffferent set of certificates. So why the difference? If you end up with a *completely* different set of certificates, it's because the admins screwed up, so it's theirs responsibility to fix it, not ours IMHO. Post install of new client bits: # certutil -L -d /etc/ipa/nssdb/ IPA CA CT,C,C After running ipa-certupdate: # certutil -L -d /etc/ipa/nssdb/ CN=Primary CA,O=example.com,C=US ,, CN=Secondary CA,O=example.com,C=US ,, GREYOAK.COM IPA CA CT,C,C Quite the difference. Not much of a difference when you realize that IPA CA and GREYOAK.COM IPA CA are the same certificate and the rest are there just for completeness. I'll ACK for now since this doesn't materially change anything and shouldn't break any installs but it begs the question of why it is acceptable now but not acceptable to make ipa-certupdate do the same. I have changed the NSS database used by the RPC code from /etc/pki/nssdb to /etc/ipa/nssdb. What the RPM update does is a necessary configuration update to keep RPC working. ipa-certupdate on the other hand fetches new policy from the server, which is quite a different thing IMO. So ACK for 319, 324-331, 340. Thanks! The LDAP update happens in renew_ca_cert. Are there any relevant errors in /var/log/messages? Is there caRenewalMaster in ipaConfigString of the master entry of the master where you run ipa-cacert-manage renew? Sep 25 16:06:44 grindle renew_ca_cert: Traceback (most recent call last): File /usr/lib64/ipa/certmonger/renew_ca_cert, line 214, in module main() File /usr/lib64/ipa/certmonger/renew_ca_cert, line 79, in main ca = cainstance.CAInstance(host_name=api.env.host, ldapi=False) File /usr/lib/python2.7/site-packages/ipaserver/install/cainstance.py, line 357, in __init__ self.ra_agent_pwd = self.ra_agent_db + /pwdfile.txt TypeError: unsupported operand type(s) for +: 'NoneType' and 'str' Sep 25 16:06:44 grindle certmonger: Certificate named caSigningCert cert-pki-ca in token NSS Certificate DB in database /etc/pki/pki-tomcat/alias issued by CA and saved. One of the KRA patches broke this. I assumed you'd be testing on top of ipa-4-1, so I didn't fix it, sorry. See the attached patch 342 for a fix. Martin, can you please review it? rob The patches needed a rebase, attached. -- Jan Cholasta From
Re: [Freeipa-devel] [PATCHES] 336-339 Installer certificate options usability fixes
On 09/30/2014 09:00 AM, Martin Kosek wrote: On 09/29/2014 11:02 PM, Petr Viktorin wrote: On 09/29/2014 04:32 PM, Jan Cholasta wrote: Dne 26.9.2014 v 19:40 Jan Cholasta napsal(a): Dne 26.9.2014 v 17:37 Rob Crittenden napsal(a): Petr Viktorin wrote: On 09/24/2014 06:13 PM, Jan Cholasta wrote: Hi, the attached patches fix https://fedorahosted.org/freeipa/ticket/4480 and https://fedorahosted.org/freeipa/ticket/4489. (Note that design page for this is TBD.) IMO it's OK to just remove them, as they were added during 4.1 development and are not available in any released version of IPA. Ah, OK Added patch 341 for stricter CA certificate validation which fixes https://fedorahosted.org/freeipa/ticket/4477. Updated patches attached. I didn't find any more issues I understood that as an ACK, so I pushed to master, ipa-4-1 (to expedite preparations for 4.1 Alpha). I just had to do a minor conflict resolution for master branch. except of course the missing design page and tests. Any ETA on those? Right, this is still a work to do. I saw Honza created http://www.freeipa.org/page/V4/Installer_CA_options_usability_improvements but it still misses the content. If there are missing tests, please file a ticket so that we have the deficiency tracked. https://fedorahosted.org/freeipa/ticket/4588 -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES] 319, 324-335 CA management and renewal fixes
On 09/30/2014 09:59 AM, Jan Cholasta wrote: Dne 29.9.2014 v 18:09 Rob Crittenden napsal(a): Jan Cholasta wrote: Dne 29.9.2014 v 15:05 Rob Crittenden napsal(a): Jan Cholasta wrote: Dne 26.9.2014 v 17:13 Rob Crittenden napsal(a): Jan Cholasta wrote: Dne 23.9.2014 v 20:39 Rob Crittenden napsal(a): Jan Cholasta wrote: Hi, the attached patches fix various bugs and shortcomings in the CA management and renewal code. Related tickets: https://fedorahosted.org/freeipa/ticket/4416, https://fedorahosted.org/freeipa/ticket/4460. (Patch 319 was originally posted at http://www.redhat.com/archives/freeipa-devel/2014-September/msg00132.html.) Only two of the patches includes what ticket(s) they address. Most have the tersest of commit messages. If got more and more difficult to see why the changes were needed at all, as you'll see. Sorry, fixed (hopefully). Note that most of these patches fix stuff I didn't have time to fix before I posted the original CA management patches, hence the missing tickets. Well, the policy is that every commit should have a ticket. So I guess re-open the old tickets or open new ones. This will help someone in the future know the why of a patch. I've certainly been guilty OK, I will reopen the related tickets. Here is a new set of reviews as trying to intermingle was making my eyes cross: 319: I guess I still don't understand why you can't pull the certs out of LDAP when creating this database. When this code runs, we know that the client is configured, so we have access to authentication. Why can't create_ipa_nssdb pull the certs directly? It seems more robust to me, and the code is already written in ipa-client-install to do this. Well, I don't understand why do you want them to be updated so much, as nothing will break if they are not. Also try to imagine what would happen if, say, 10k clients were updated at the same moment... What's the point of a database missing certificates? It won't be missing any certificates if /etc/pki/nssdb was not missing any certificates before the update. As I said, the update will not break anything. It will not fix anything either, but I don't think this kind of fixing should be done during client RPM upgrade. It is not consistent with anything else we do during client RPM upgrade, it does not scale well and it just does not feel right to me in overall. Ok, I'll concede the point that there is no difference post-update, but it doesn't do what ipa-certupdate does. You can potentially end up with a completely diffferent set of certificates. So why the difference? If you end up with a *completely* different set of certificates, it's because the admins screwed up, so it's theirs responsibility to fix it, not ours IMHO. Post install of new client bits: # certutil -L -d /etc/ipa/nssdb/ IPA CA CT,C,C After running ipa-certupdate: # certutil -L -d /etc/ipa/nssdb/ CN=Primary CA,O=example.com,C=US ,, CN=Secondary CA,O=example.com,C=US ,, GREYOAK.COM IPA CA CT,C,C Quite the difference. Not much of a difference when you realize that IPA CA and GREYOAK.COM IPA CA are the same certificate and the rest are there just for completeness. I'll ACK for now since this doesn't materially change anything and shouldn't break any installs but it begs the question of why it is acceptable now but not acceptable to make ipa-certupdate do the same. I have changed the NSS database used by the RPC code from /etc/pki/nssdb to /etc/ipa/nssdb. What the RPM update does is a necessary configuration update to keep RPC working. ipa-certupdate on the other hand fetches new policy from the server, which is quite a different thing IMO. So ACK for 319, 324-331, 340. Thanks! The LDAP update happens in renew_ca_cert. Are there any relevant errors in /var/log/messages? Is there caRenewalMaster in ipaConfigString of the master entry of the master where you run ipa-cacert-manage renew? Sep 25 16:06:44 grindle renew_ca_cert: Traceback (most recent call last): File /usr/lib64/ipa/certmonger/renew_ca_cert, line 214, in module main() File /usr/lib64/ipa/certmonger/renew_ca_cert, line 79, in main ca = cainstance.CAInstance(host_name=api.env.host, ldapi=False) File /usr/lib/python2.7/site-packages/ipaserver/install/cainstance.py, line 357, in __init__ self.ra_agent_pwd = self.ra_agent_db + /pwdfile.txt TypeError: unsupported operand type(s) for +: 'NoneType' and 'str' Sep 25 16:06:44 grindle certmonger: Certificate named caSigningCert cert-pki-ca in token NSS Certificate DB in database /etc/pki/pki-tomcat/alias issued by CA and saved. One of the KRA patches broke this. I assumed you'd be testing on top of ipa-4-1, so I didn't fix it, sorry. See the attached patch 342 for a fix. Martin, can you please
Re: [Freeipa-devel] [PATCHES] 319, 324-335 CA management and renewal fixes
Dne 30.9.2014 v 10:09 Martin Kosek napsal(a): On 09/30/2014 09:59 AM, Jan Cholasta wrote: Dne 29.9.2014 v 18:09 Rob Crittenden napsal(a): Jan Cholasta wrote: Dne 29.9.2014 v 15:05 Rob Crittenden napsal(a): Jan Cholasta wrote: Dne 26.9.2014 v 17:13 Rob Crittenden napsal(a): Jan Cholasta wrote: Dne 23.9.2014 v 20:39 Rob Crittenden napsal(a): Jan Cholasta wrote: Hi, the attached patches fix various bugs and shortcomings in the CA management and renewal code. Related tickets: https://fedorahosted.org/freeipa/ticket/4416, https://fedorahosted.org/freeipa/ticket/4460. (Patch 319 was originally posted at http://www.redhat.com/archives/freeipa-devel/2014-September/msg00132.html.) Only two of the patches includes what ticket(s) they address. Most have the tersest of commit messages. If got more and more difficult to see why the changes were needed at all, as you'll see. Sorry, fixed (hopefully). Note that most of these patches fix stuff I didn't have time to fix before I posted the original CA management patches, hence the missing tickets. Well, the policy is that every commit should have a ticket. So I guess re-open the old tickets or open new ones. This will help someone in the future know the why of a patch. I've certainly been guilty OK, I will reopen the related tickets. Here is a new set of reviews as trying to intermingle was making my eyes cross: 319: I guess I still don't understand why you can't pull the certs out of LDAP when creating this database. When this code runs, we know that the client is configured, so we have access to authentication. Why can't create_ipa_nssdb pull the certs directly? It seems more robust to me, and the code is already written in ipa-client-install to do this. Well, I don't understand why do you want them to be updated so much, as nothing will break if they are not. Also try to imagine what would happen if, say, 10k clients were updated at the same moment... What's the point of a database missing certificates? It won't be missing any certificates if /etc/pki/nssdb was not missing any certificates before the update. As I said, the update will not break anything. It will not fix anything either, but I don't think this kind of fixing should be done during client RPM upgrade. It is not consistent with anything else we do during client RPM upgrade, it does not scale well and it just does not feel right to me in overall. Ok, I'll concede the point that there is no difference post-update, but it doesn't do what ipa-certupdate does. You can potentially end up with a completely diffferent set of certificates. So why the difference? If you end up with a *completely* different set of certificates, it's because the admins screwed up, so it's theirs responsibility to fix it, not ours IMHO. Post install of new client bits: # certutil -L -d /etc/ipa/nssdb/ IPA CA CT,C,C After running ipa-certupdate: # certutil -L -d /etc/ipa/nssdb/ CN=Primary CA,O=example.com,C=US ,, CN=Secondary CA,O=example.com,C=US ,, GREYOAK.COM IPA CA CT,C,C Quite the difference. Not much of a difference when you realize that IPA CA and GREYOAK.COM IPA CA are the same certificate and the rest are there just for completeness. I'll ACK for now since this doesn't materially change anything and shouldn't break any installs but it begs the question of why it is acceptable now but not acceptable to make ipa-certupdate do the same. I have changed the NSS database used by the RPC code from /etc/pki/nssdb to /etc/ipa/nssdb. What the RPM update does is a necessary configuration update to keep RPC working. ipa-certupdate on the other hand fetches new policy from the server, which is quite a different thing IMO. So ACK for 319, 324-331, 340. Thanks! The LDAP update happens in renew_ca_cert. Are there any relevant errors in /var/log/messages? Is there caRenewalMaster in ipaConfigString of the master entry of the master where you run ipa-cacert-manage renew? Sep 25 16:06:44 grindle renew_ca_cert: Traceback (most recent call last): File /usr/lib64/ipa/certmonger/renew_ca_cert, line 214, in module main() File /usr/lib64/ipa/certmonger/renew_ca_cert, line 79, in main ca = cainstance.CAInstance(host_name=api.env.host, ldapi=False) File /usr/lib/python2.7/site-packages/ipaserver/install/cainstance.py, line 357, in __init__ self.ra_agent_pwd = self.ra_agent_db + /pwdfile.txt TypeError: unsupported operand type(s) for +: 'NoneType' and 'str' Sep 25 16:06:44 grindle certmonger: Certificate named caSigningCert cert-pki-ca in token NSS Certificate DB in database /etc/pki/pki-tomcat/alias issued by CA and saved. One of the KRA patches broke this. I assumed you'd be testing on top of ipa-4-1, so I didn't fix it, sorry. See the attached patch 342 for a fix. Martin, can you please review it? rob
Re: [Freeipa-devel] [PATCHES] 319, 324-335 CA management and renewal fixes
On 09/30/2014 10:19 AM, Jan Cholasta wrote: Dne 30.9.2014 v 10:09 Martin Kosek napsal(a): On 09/30/2014 09:59 AM, Jan Cholasta wrote: Dne 29.9.2014 v 18:09 Rob Crittenden napsal(a): Jan Cholasta wrote: Dne 29.9.2014 v 15:05 Rob Crittenden napsal(a): Jan Cholasta wrote: Dne 26.9.2014 v 17:13 Rob Crittenden napsal(a): Jan Cholasta wrote: Dne 23.9.2014 v 20:39 Rob Crittenden napsal(a): Jan Cholasta wrote: Hi, the attached patches fix various bugs and shortcomings in the CA management and renewal code. Related tickets: https://fedorahosted.org/freeipa/ticket/4416, https://fedorahosted.org/freeipa/ticket/4460. (Patch 319 was originally posted at http://www.redhat.com/archives/freeipa-devel/2014-September/msg00132.html.) Only two of the patches includes what ticket(s) they address. Most have the tersest of commit messages. If got more and more difficult to see why the changes were needed at all, as you'll see. Sorry, fixed (hopefully). Note that most of these patches fix stuff I didn't have time to fix before I posted the original CA management patches, hence the missing tickets. Well, the policy is that every commit should have a ticket. So I guess re-open the old tickets or open new ones. This will help someone in the future know the why of a patch. I've certainly been guilty OK, I will reopen the related tickets. Here is a new set of reviews as trying to intermingle was making my eyes cross: 319: I guess I still don't understand why you can't pull the certs out of LDAP when creating this database. When this code runs, we know that the client is configured, so we have access to authentication. Why can't create_ipa_nssdb pull the certs directly? It seems more robust to me, and the code is already written in ipa-client-install to do this. Well, I don't understand why do you want them to be updated so much, as nothing will break if they are not. Also try to imagine what would happen if, say, 10k clients were updated at the same moment... What's the point of a database missing certificates? It won't be missing any certificates if /etc/pki/nssdb was not missing any certificates before the update. As I said, the update will not break anything. It will not fix anything either, but I don't think this kind of fixing should be done during client RPM upgrade. It is not consistent with anything else we do during client RPM upgrade, it does not scale well and it just does not feel right to me in overall. Ok, I'll concede the point that there is no difference post-update, but it doesn't do what ipa-certupdate does. You can potentially end up with a completely diffferent set of certificates. So why the difference? If you end up with a *completely* different set of certificates, it's because the admins screwed up, so it's theirs responsibility to fix it, not ours IMHO. Post install of new client bits: # certutil -L -d /etc/ipa/nssdb/ IPA CA CT,C,C After running ipa-certupdate: # certutil -L -d /etc/ipa/nssdb/ CN=Primary CA,O=example.com,C=US ,, CN=Secondary CA,O=example.com,C=US ,, GREYOAK.COM IPA CA CT,C,C Quite the difference. Not much of a difference when you realize that IPA CA and GREYOAK.COM IPA CA are the same certificate and the rest are there just for completeness. I'll ACK for now since this doesn't materially change anything and shouldn't break any installs but it begs the question of why it is acceptable now but not acceptable to make ipa-certupdate do the same. I have changed the NSS database used by the RPC code from /etc/pki/nssdb to /etc/ipa/nssdb. What the RPM update does is a necessary configuration update to keep RPC working. ipa-certupdate on the other hand fetches new policy from the server, which is quite a different thing IMO. So ACK for 319, 324-331, 340. Thanks! The LDAP update happens in renew_ca_cert. Are there any relevant errors in /var/log/messages? Is there caRenewalMaster in ipaConfigString of the master entry of the master where you run ipa-cacert-manage renew? Sep 25 16:06:44 grindle renew_ca_cert: Traceback (most recent call last): File /usr/lib64/ipa/certmonger/renew_ca_cert, line 214, in module main() File /usr/lib64/ipa/certmonger/renew_ca_cert, line 79, in main ca = cainstance.CAInstance(host_name=api.env.host, ldapi=False) File /usr/lib/python2.7/site-packages/ipaserver/install/cainstance.py, line 357, in __init__ self.ra_agent_pwd = self.ra_agent_db + /pwdfile.txt TypeError: unsupported operand type(s) for +: 'NoneType' and 'str' Sep 25 16:06:44 grindle certmonger: Certificate named caSigningCert cert-pki-ca in token NSS Certificate DB in database /etc/pki/pki-tomcat/alias issued by CA and saved. One of the KRA patches broke this. I assumed you'd be testing on top of
Re: [Freeipa-devel] [PATCHES 247-281] ID views - management part
On 09/30/2014 09:56 AM, Tomas Babej wrote: Attaching updated patchset with resolved objections from Petr^1 and Petr^3. (three more patches attached) LGTM. (I never did functional testing for this though.) -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES 247-281] ID views - management part
On 30.9.2014 10:22, Petr Viktorin wrote: On 09/30/2014 09:56 AM, Tomas Babej wrote: Attaching updated patchset with resolved objections from Petr^1 and Petr^3. (three more patches attached) LGTM. (I never did functional testing for this though.) ACK 280-4, 281-4 LGTM 282-4 (not tested) -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCHES 247-281] ID views - management part
On 09/30/2014 10:37 AM, Petr Vobornik wrote: On 30.9.2014 10:22, Petr Viktorin wrote: On 09/30/2014 09:56 AM, Tomas Babej wrote: Attaching updated patchset with resolved objections from Petr^1 and Petr^3. (three more patches attached) LGTM. (I never did functional testing for this though.) ACK 280-4, 281-4 LGTM 282-4 (not tested) Thanks for all for this heroic effort! :-) Pushed to: master: 2a230b6cc16037fbf56d79bfde2fb4d1ab386ef6 ipa-4-1: f0b6254106f98875e2c94af81bcb836d3ad46681 Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 749-754 webui: new ID views section
All pushed. master: * 15b6ed67056ce918e11f7ea5c2d193534b5ce6b5 webui: improve breadcrumb navigation * 26bd309c96446b9eda26a08e6924d6e1b4c621fc webui: treat value as pkey in link widget * 27196b92c60917d8488dad8721d2087e9fee716c webui: do not show internal facet name to user * 8b0e2ed991e9a1a49ef92e314d3d4855beb93b46 webui: allow to skip link widget link validation * 749101db74219681735226664c1f83ebb4dc4aa7 webui: add simple link column support * ae5a34cbbc0cd3841647a2ad166bdfc65399da19 webui: new ID views section * 2cc78acf9b45b5f8a2d12e232d53267a31732df6 webui: facet group labels for idview's facets * 0e76bc1cb65b3eb81b37b4b45ccb71bf91fe5fbc webui: list only not-applied hosts in apply to host dialog * 00d598bab043e277d3f57eab5092c04cf5d6f5f8 webui: add link from host to idview ipa-4-1: * f3c8c4c00f42692f4484dd7875a991a8a0443208 webui: improve breadcrumb navigation * 1050ec887782d1ebf906d239e6aab98aecfc9db4 webui: treat value as pkey in link widget * 86fc8ec0c8d22bd32abe157a047148f0fabf0ff9 webui: do not show internal facet name to user * e0c33446799a2f199b181660dd2b03a4ca6636da webui: allow to skip link widget link validation * cd4c337002fa5c67d0dcad271790fc7130af47d1 webui: add simple link column support * 8a4730ce3c971a23d3d3e2ce55d9bb5a0c46124a webui: new ID views section * bdf1e6c2262b09e6d515d09a37e8a33c4a4e85df webui: facet group labels for idview's facets * 7b7b98db185efba17225c2029d5728bd794e4650 webui: list only not-applied hosts in apply to host dialog * 6388aaad80fe5ab18ad4100fb28e3257f55dbca5 webui: add link from host to idview -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] [PATCH 0283] idviews: Fix typo in upgrade handling of the Default Trust
Hi, Fixed missing comma. Also removes leading spaces from the ldif, since this is not stripped by the updater. Part of: https://fedorahosted.org/freeipa/ticket/3979 -- Tomas Babej Associate Software Engineer | Red Hat | Identity Management RHCE | Brno Site | IRC: tbabej | freeipa.org From 2a10a5fa3789a7f2b68b3343696dc3125a931f99 Mon Sep 17 00:00:00 2001 From: Tomas Babej tba...@redhat.com Date: Tue, 30 Sep 2014 11:31:08 +0200 Subject: [PATCH] idviews: Fix typo in upgrade handling of the Default Trust View Fixed missing comma. Also removes leading spaces from the ldif, since this is not stripped by the updater. Part of: https://fedorahosted.org/freeipa/ticket/3979 --- ipaserver/install/plugins/adtrust.py | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/ipaserver/install/plugins/adtrust.py b/ipaserver/install/plugins/adtrust.py index e5082fe04cf4b42c786dfd94f0ac3f9a0e8902e0..1290278cf5e5a8d25638e20c95690360d5837eac 100644 --- a/ipaserver/install/plugins/adtrust.py +++ b/ipaserver/install/plugins/adtrust.py @@ -132,11 +132,11 @@ class update_default_trust_view(PostUpdate): api.env.basedn) default_trust_view_entry = [ -'objectclass: top', -'objectclass: ipaIDView' -'cn: Default Trust View', -'description: Default Trust View for AD users. ' - 'Should not be deleted.' +'objectclass:top', +'objectclass:ipaIDView', +'cn:Default Trust View', +'description:Default Trust View for AD users. ' +'Should not be deleted.', ] # First, see if trusts are enabled on the server -- 1.9.3 ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0283] idviews: Fix typo in upgrade handling of the Default Trust
Dne 30.9.2014 v 11:40 Tomas Babej napsal(a): Hi, Fixed missing comma. Also removes leading spaces from the ldif, since this is not stripped by the updater. Part of: https://fedorahosted.org/freeipa/ticket/3979 ACK. -- Jan Cholasta ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0283] idviews: Fix typo in upgrade handling of the Default Trust
On 09/30/2014 11:48 AM, Jan Cholasta wrote: Dne 30.9.2014 v 11:40 Tomas Babej napsal(a): Hi, Fixed missing comma. Also removes leading spaces from the ldif, since this is not stripped by the updater. Part of: https://fedorahosted.org/freeipa/ticket/3979 ACK. Pushed to: master: 00457a9c109c1df0788a979f07c7fb5c0cc3bc8b ipa-4-1: 7ddebb613d1d14ebe11491651eb7bbfe21c64f5b Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] Use alpha instead of pre for alpha releases
Last time (2.1) we used Preview/Testing for the pre-beta release,
but the Git tags were still named alpha_*.
I think alpha is a better name, let's use that.
--
Petr³
From 50c941ce35f1a715fc3b8e7dc154639760498c05 Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Tue, 30 Sep 2014 11:48:20 +0200
Subject: [PATCH] VERSION,Makefile: Rename pre to alpha
Last time (2.1) we used Preview/Testing for the pre-beta release,
but the Git tags were still named alpha_*.
Use alpha, remove pre.
---
Makefile | 6 +++---
VERSION | 17 +
2 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/Makefile b/Makefile
index 3bdec716d57e61849f689812267c62dca82f0f45..eca282a2390002dcefcc7b544b69a47b81418e0d 100644
--- a/Makefile
+++ b/Makefile
@@ -23,8 +23,8 @@ endif # in a git tree and git returned a version
endif # git
ifndef IPA_VERSION
-ifdef IPA_VERSION_PRE_RELEASE
-IPA_VERSION=$(IPA_VERSION_MAJOR).$(IPA_VERSION_MINOR).$(IPA_VERSION_RELEASE).pre$(IPA_VERSION_PRE_RELEASE)
+ifdef IPA_VERSION_ALPHA_RELEASE
+IPA_VERSION=$(IPA_VERSION_MAJOR).$(IPA_VERSION_MINOR).$(IPA_VERSION_RELEASE).alpha$(IPA_VERSION_ALPHA_RELEASE)
else
ifdef IPA_VERSION_BETA_RELEASE
IPA_VERSION=$(IPA_VERSION_MAJOR).$(IPA_VERSION_MINOR).$(IPA_VERSION_RELEASE).beta$(IPA_VERSION_BETA_RELEASE)
@@ -35,7 +35,7 @@ else
IPA_VERSION=$(IPA_VERSION_MAJOR).$(IPA_VERSION_MINOR).$(IPA_VERSION_RELEASE)
endif # rc
endif # beta
-endif # pre
+endif # alpha
endif # ipa_version
IPA_VENDOR_VERSION=$(IPA_VERSION)$(IPA_VENDOR_VERSION_SUFFIX)
diff --git a/VERSION b/VERSION
index 60c62a3e237be5e4f15525cc35d51479d056834e..78ef7d8ecd49654bf960fda8dedc1a178dd97827 100644
--- a/VERSION
+++ b/VERSION
@@ -2,9 +2,10 @@
# freeIPA Version #
# #
# freeIPA versions are as follows #
-# 1.0.xNew production series #
-# 1.0.x{pre,beta,rc}y Preview/Testing, Beta RC #
-# 1.0.0GITabcdefg Build from GIT #
+# 1.0.x New production series #
+# 1.0.x{alpha,beta,rc}y Alpha/Preview/Testing, Beta, #
+# Release Candidate #
+# 1.0.0GITabcdefgBuild from GIT#
# #
@@ -23,14 +24,14 @@ IPA_VERSION_MINOR=0
IPA_VERSION_RELEASE=0
-# For 'pre' releases the version will be #
+# For 'alpha' releases the version will be #
# #
-# MAJOR.MINOR.RELEASEprePRE_RELEASE#
+# MAJOR.MINOR.RELEASEalphaALPHA_RELEASE#
# #
-# e.g. IPA_VERSION_PRE_RELEASE=1 #
-# - 1.0.0pre1 #
+# e.g. IPA_VERSION_ALPHA_RELEASE=1 #
+# - 1.0.0alpha1 #
-IPA_VERSION_PRE_RELEASE=
+IPA_VERSION_ALPHA_RELEASE=
# For 'beta' releases the version will be #
--
1.9.3
From 41ccfe8d6f026eb71efc22f512535f64bd863fa2 Mon Sep 17 00:00:00 2001
From: Petr Viktorin pvikt...@redhat.com
Date: Tue, 30 Sep 2014 11:52:07 +0200
Subject: [PATCH] Become IPA 4.1.0 Alpha 1
---
VERSION | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/VERSION b/VERSION
index 78ef7d8ecd49654bf960fda8dedc1a178dd97827..efd4f615d190ee2e0ed9ca1b310c4fef0cf87adc 100644
--- a/VERSION
+++ b/VERSION
@@ -20,7 +20,7 @@
# - 1.0.0 #
IPA_VERSION_MAJOR=4
-IPA_VERSION_MINOR=0
+IPA_VERSION_MINOR=1
IPA_VERSION_RELEASE=0
@@ -31,7 +31,7 @@ IPA_VERSION_RELEASE=0
# e.g. IPA_VERSION_ALPHA_RELEASE=1 #
# - 1.0.0alpha1 #
-IPA_VERSION_ALPHA_RELEASE=
+IPA_VERSION_ALPHA_RELEASE=1
# For 'beta' releases the version will be #
--
1.9.3
___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Use alpha instead of pre for alpha releases
On 09/30/2014 12:55 PM, Petr Viktorin wrote: Last time (2.1) we used Preview/Testing for the pre-beta release, but the Git tags were still named alpha_*. I think alpha is a better name, let's use that. +1, I would also prefer to use Alpha, I this it is more understandable than pre-release. Actually all Alpha and Beta versions are pre-releases... Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Use alpha instead of pre for alpha releases
On Tue, 30 Sep 2014, Martin Kosek wrote: On 09/30/2014 12:55 PM, Petr Viktorin wrote: Last time (2.1) we used Preview/Testing for the pre-beta release, but the Git tags were still named alpha_*. I think alpha is a better name, let's use that. +1, I would also prefer to use Alpha, I this it is more understandable than pre-release. Actually all Alpha and Beta versions are pre-releases... I'm fine with that. -- / Alexander Bokovoy ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Use alpha instead of pre for alpha releases
On 30.9.2014 12:55, Petr Viktorin wrote: Last time (2.1) we used Preview/Testing for the pre-beta release, but the Git tags were still named alpha_*. I think alpha is a better name, let's use that. +1 -- Petr Vobornik ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0068] Move OTP synchronization step to after counter writeback
On 09/29/2014 08:38 PM, Nathaniel McCallum wrote: On Thu, 2014-09-25 at 15:15 +0200, thierry bordaz wrote: On 09/19/2014 07:53 PM, Nathaniel McCallum wrote: This prevents synchronization when an authentication collision occurs. https://fedorahosted.org/freeipa/ticket/4493 NOTE: this patch is related to the above ticket, but does not solve it. For the solution, please see patch 0064. This behavior fix is from patch 0062 (rescinded) and is worth keeping. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Hello Nathaniel, . My understanding is that during a pre_bind, the plugins validates token codes (for example HOTP) checking that step ranges [-25..+25]. As soon as the token is valid, the new HOTPcounter is written in the entry. But in case of negative steps,I believe the otp-decrement plugin will reject this update. HOTP never goes backwards. See line 188 in libotp.c. Even if this check weren't present, we would *want* the decrement plugin to reject the update. Ok. If TOTPwatermark is updated and there is a second token code, then clockOffset is also updated. This update is done during a pre_bind, so if there are parallel binds on the server, there is a possibility that TOTPwatermark is updated from a bind and 'clockOffset' updated from an other bind. An option is to do a single internal modify to update both. We don't care about atomicity in this case. If two TOTP synchronizations occur at almost the same time, the value of clockOffset will be written twice with the same value. Since the values are the same, we don't care which value gets written first. I was just wondering if parallel binds with different 'step' values could occur. Because different 'step' values can lead to different 'clockOffset'. thanks thierry Nathaniel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
[Freeipa-devel] FreeIPA 4.1 Alpha 1
Hello, It is time to produce an Alpha 1 release of FreeIPA 4.1, as a technology preview for other developers or testers. The main reason is that the Views feature is slowly getting complete and we want other people take a look. The FreeIPA framework and UI part are done, SSSD and slapi-nis are in progress. Given the nature of the build, I would only announce it on freeipa-devel and wait with a more baked release for freeipa-users and freeipa-interest. I prepared a first version of the release notes here: http://www.freeipa.org/page/Releases/4.1.0.alpha1 Please feel free to fix or extend, we can then use it as a base for our next 4.1 release notes. -- Martin Kosek mko...@redhat.com Supervisor, Software Engineering - Identity Management Team Red Hat Inc. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] Use alpha instead of pre for alpha releases
On 09/30/2014 01:15 PM, Alexander Bokovoy wrote: On Tue, 30 Sep 2014, Martin Kosek wrote: On 09/30/2014 12:55 PM, Petr Viktorin wrote: Last time (2.1) we used Preview/Testing for the pre-beta release, but the Git tags were still named alpha_*. I think alpha is a better name, let's use that. +1, I would also prefer to use Alpha, I this it is more understandable than pre-release. Actually all Alpha and Beta versions are pre-releases... I'm fine with that. Pushed tagged ipa-4-1: 946291c0db2b2445e5519553d5f707d8b10a09f1 [new tag] alpha_1-4-1-0 And I the first patch went to master as well: master: 9ba33971fad5ec3b6fb5445669228b0ea9a89ec5 -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0068] Move OTP synchronization step to after counter writeback
On Tue, 2014-09-30 at 13:42 +0200, thierry bordaz wrote: On 09/29/2014 08:38 PM, Nathaniel McCallum wrote: On Thu, 2014-09-25 at 15:15 +0200, thierry bordaz wrote: On 09/19/2014 07:53 PM, Nathaniel McCallum wrote: This prevents synchronization when an authentication collision occurs. https://fedorahosted.org/freeipa/ticket/4493 NOTE: this patch is related to the above ticket, but does not solve it. For the solution, please see patch 0064. This behavior fix is from patch 0062 (rescinded) and is worth keeping. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Hello Nathaniel, . My understanding is that during a pre_bind, the plugins validates token codes (for example HOTP) checking that step ranges [-25..+25]. As soon as the token is valid, the new HOTPcounter is written in the entry. But in case of negative steps,I believe the otp-decrement plugin will reject this update. HOTP never goes backwards. See line 188 in libotp.c. Even if this check weren't present, we would *want* the decrement plugin to reject the update. Ok. If TOTPwatermark is updated and there is a second token code, then clockOffset is also updated. This update is done during a pre_bind, so if there are parallel binds on the server, there is a possibility that TOTPwatermark is updated from a bind and 'clockOffset' updated from an other bind. An option is to do a single internal modify to update both. We don't care about atomicity in this case. If two TOTP synchronizations occur at almost the same time, the value of clockOffset will be written twice with the same value. Since the values are the same, we don't care which value gets written first. I was just wondering if parallel binds with different 'step' values could occur. Because different 'step' values can lead to different 'clockOffset'. It is possible, but this isn't a concern. This code path is reached only when synchronization is being performed (not regular binds). This means that the clockOffset value is currently wildly incorrect. We are trying to move this value to something roughly correct. For various reasons, we can't get this value exactly. So if parallel authentication has succeeded, either of the resulting step values is correct. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0068] Move OTP synchronization step to after counter writeback
On 09/30/2014 02:41 PM, Nathaniel McCallum wrote: On Tue, 2014-09-30 at 13:42 +0200, thierry bordaz wrote: On 09/29/2014 08:38 PM, Nathaniel McCallum wrote: On Thu, 2014-09-25 at 15:15 +0200, thierry bordaz wrote: On 09/19/2014 07:53 PM, Nathaniel McCallum wrote: This prevents synchronization when an authentication collision occurs. https://fedorahosted.org/freeipa/ticket/4493 NOTE: this patch is related to the above ticket, but does not solve it. For the solution, please see patch 0064. This behavior fix is from patch 0062 (rescinded) and is worth keeping. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Hello Nathaniel, . My understanding is that during a pre_bind, the plugins validates token codes (for example HOTP) checking that step ranges [-25..+25]. As soon as the token is valid, the new HOTPcounter is written in the entry. But in case of negative steps,I believe the otp-decrement plugin will reject this update. HOTP never goes backwards. See line 188 in libotp.c. Even if this check weren't present, we would *want* the decrement plugin to reject the update. Ok. If TOTPwatermark is updated and there is a second token code, then clockOffset is also updated. This update is done during a pre_bind, so if there are parallel binds on the server, there is a possibility that TOTPwatermark is updated from a bind and 'clockOffset' updated from an other bind. An option is to do a single internal modify to update both. We don't care about atomicity in this case. If two TOTP synchronizations occur at almost the same time, the value of clockOffset will be written twice with the same value. Since the values are the same, we don't care which value gets written first. I was just wondering if parallel binds with different 'step' values could occur. Because different 'step' values can lead to different 'clockOffset'. It is possible, but this isn't a concern. This code path is reached only when synchronization is being performed (not regular binds). This means that the clockOffset value is currently wildly incorrect. We are trying to move this value to something roughly correct. For various reasons, we can't get this value exactly. So if parallel authentication has succeeded, either of the resulting step values is correct. Thanks Nathaniel for the explanations. The fix is fine for me and for me it is a ACK. thanks thierry ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0068] Move OTP synchronization step to after counter writeback
On 09/30/2014 02:53 PM, thierry bordaz wrote: On 09/30/2014 02:41 PM, Nathaniel McCallum wrote: On Tue, 2014-09-30 at 13:42 +0200, thierry bordaz wrote: On 09/29/2014 08:38 PM, Nathaniel McCallum wrote: On Thu, 2014-09-25 at 15:15 +0200, thierry bordaz wrote: On 09/19/2014 07:53 PM, Nathaniel McCallum wrote: This prevents synchronization when an authentication collision occurs. https://fedorahosted.org/freeipa/ticket/4493 NOTE: this patch is related to the above ticket, but does not solve it. For the solution, please see patch 0064. This behavior fix is from patch 0062 (rescinded) and is worth keeping. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Hello Nathaniel, . My understanding is that during a pre_bind, the plugins validates token codes (for example HOTP) checking that step ranges [-25..+25]. As soon as the token is valid, the new HOTPcounter is written in the entry. But in case of negative steps,I believe the otp-decrement plugin will reject this update. HOTP never goes backwards. See line 188 in libotp.c. Even if this check weren't present, we would *want* the decrement plugin to reject the update. Ok. If TOTPwatermark is updated and there is a second token code, then clockOffset is also updated. This update is done during a pre_bind, so if there are parallel binds on the server, there is a possibility that TOTPwatermark is updated from a bind and 'clockOffset' updated from an other bind. An option is to do a single internal modify to update both. We don't care about atomicity in this case. If two TOTP synchronizations occur at almost the same time, the value of clockOffset will be written twice with the same value. Since the values are the same, we don't care which value gets written first. I was just wondering if parallel binds with different 'step' values could occur. Because different 'step' values can lead to different 'clockOffset'. It is possible, but this isn't a concern. This code path is reached only when synchronization is being performed (not regular binds). This means that the clockOffset value is currently wildly incorrect. We are trying to move this value to something roughly correct. For various reasons, we can't get this value exactly. So if parallel authentication has succeeded, either of the resulting step values is correct. Thanks Nathaniel for the explanations. The fix is fine for me and for me it is a ACK. thanks thierry Pushed to: master: 915837c14af5f0839d1d08683ea8332334e284ba ipa-4-1: 98debb7fb1b1e998d48806702ad4d950b6dd9f23 -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On 09/29/2014 08:30 PM, Nathaniel McCallum wrote:
On Mon, 2014-09-22 at 09:32 -0400, Simo Sorce wrote:
On Sun, 21 Sep 2014 22:33:47 -0400
Nathaniel McCallum npmccal...@redhat.com wrote:
Comments inline.
+
+#define ch_malloc(type) \
+(type*) slapi_ch_malloc(sizeof(type))
+#define ch_calloc(count, type) \
+(type*) slapi_ch_calloc(count, sizeof(type))
+#define ch_free(p) \
+slapi_ch_free((void**) (p))
please do not redefine slapi functions, it just makes it harder to know
what you used.
+typedef struct {
+bool exists;
+long long value;
+} counter;
please no typedefs of structures, use struct counter { ... }; and
reference it as struct counter in the code.
Btw, FWIW you could use a value of -1 to indicate non-existence of the
counter value, given counters can only be positive, this would avoid
the need for a struct.
+static int
+send_error(Slapi_PBlock *pb, int rc, char *template, ...)
+{
+va_list ap;
+int res;
+
+va_start(ap, template);
+res = vsnprintf(NULL, 0, template, ap);
+va_end(ap);
+
+if (res 0) {
+char str[res + 1];
+
+va_start(ap, template);
+res = vsnprintf(str, sizeof(str), template, ap);
+va_end(ap);
+
+if (res 0)
+slapi_send_ldap_result(pb, rc, NULL, str, 0, NULL);
+}
+
+if (res = 0)
+slapi_send_ldap_result(pb, rc, NULL, NULL, 0, NULL);
+
+slapi_pblock_set(pb, SLAPI_RESULT_CODE, rc);
+return rc;
+}
This function seem not really useful, you use send_error() only at the
end of one single function where you could have the classic scheme of
using a done: label and just use directly slapi_ch_smprintf. This would
remove the need to use vsnprintf and all the va_ machinery which is
more than 50% of this function.
+static long long
+get_value(const LDAPMod *mod, long long def)
+{
+const struct berval *bv;
+long long val;
+
+if (mod == NULL)
+return def;
+
+if (mod-mod_vals.modv_bvals == NULL)
+return def;
+
+bv = mod-mod_vals.modv_bvals[0];
+if (bv == NULL)
+return def;
In general (here and elsewhere) I prefer to always use {} in if clauses.
I have been bitten enough time by people adding an instruction that
should be in the braces but forgot to add braces (defensive programming
style). But up to you.
+char buf[bv-bv_len + 1];
+memcpy(buf, bv-bv_val, bv-bv_len);
+buf[sizeof(buf)-1] = '\0';
+
+val = strtoll(buf, NULL, 10);
+if (val == LLONG_MIN || val == LLONG_MAX)
+return def;
+
+return val;
+}
+
+static struct berval **
+make_berval_array(long long value)
+{
+struct berval **bvs;
+
+bvs = ch_calloc(2, struct berval*);
+bvs[0] = ch_malloc(struct berval);
+bvs[0]-bv_val = slapi_ch_smprintf(%lld, value);
+bvs[0]-bv_len = strlen(bvs[0]-bv_val);
+
+return bvs;
+}
+
+static LDAPMod *
+make_ldapmod(int op, const char *attr, long long value)
+{
+LDAPMod *mod;
+
+mod = (LDAPMod*) slapi_ch_calloc(1, sizeof(LDAPMod));
+mod-mod_op = op | LDAP_MOD_BVALUES;
+mod-mod_type = slapi_ch_strdup(attr);
+mod-mod_bvalues = make_berval_array(value);
+
+return mod;
+}
+
+static void
+convert_ldapmod_to_bval(LDAPMod *mod)
+{
+if (mod == NULL || (mod-mod_op LDAP_MOD_BVALUES))
+return;
+
+mod-mod_op |= LDAP_MOD_BVALUES;
+
+if (mod-mod_values == NULL) {
+mod-mod_bvalues = NULL;
+return;
+}
+
+for (size_t i = 0; mod-mod_values[i] != NULL; i++) {
+struct berval *bv = ch_malloc(struct berval);
+bv-bv_val = mod-mod_values[i];
+bv-bv_len = strlen(bv-bv_val);
+mod-mod_bvalues[i] = bv;
+}
+}
+
+static counter
+get_counter(Slapi_Entry *entry, const char *attr)
+{
+Slapi_Attr *sattr = NULL;
+
+return (counter) {
+slapi_entry_attr_find(entry, attr, sattr) == 0,
+slapi_entry_attr_get_longlong(entry, attr)
+};
+}
+
+static void
+berval_free_array(struct berval***bvals)
+{
+for (size_t i = 0; (*bvals)[i] != NULL; i++) {
+ch_free((*bvals)[i]-bv_val);
+ch_free((*bvals)[i]);
+}
+
+slapi_ch_free((void**) bvals);
+}
+
+static bool
+is_replication(Slapi_PBlock *pb)
+{
+int repl = 0;
+slapi_pblock_get(pb, SLAPI_IS_REPLICATED_OPERATION, repl);
+return repl != 0;
+}
+
+static const char *
+get_attribute(Slapi_Entry *entry)
+{
+static struct {
+const char *clss;
+const char *attr;
+} table[] = {
+{ ipatokenHOTP, ipatokenHOTPcounter },
+{ ipatokenTOTP, ipatokenTOTPwatermark },
+{ NULL, NULL }
+};
+
+const char *attr = NULL;
+char **clsses = NULL;
+
+clsses = slapi_entry_attr_get_charray(entry, objectClass);
+if (clsses == NULL)
+return NULL;
+
+for (size_t i = 0; attr == NULL clsses[i] != NULL; i++) {
+for (size_t j = 0; attr == NULL table[j].clss != NULL;
j++) {
+if (PL_strcasecmp(table[j].clss, clsses[i]) == 0)
+attr =
Re: [Freeipa-devel] [PATCH 0033] Remove trivial path constants
On 09/30/2014 05:13 AM, Gabe Alford wrote: Updated patch to fix merge conflicts from recent changes. On Wed, Sep 24, 2014 at 8:34 PM, Gabe Alford redhatri...@gmail.com mailto:redhatri...@gmail.com wrote: Hello, Patch for https://fedorahosted.org/freeipa/ticket/4399. Let me know if I missed any. Thanks, Gabe Thanks for the patch, and sorry for the delay! ipaserver/tools/ipa-upgradeconfig: The `filename` and `ca_file` aren't module-level constants; I think in this case they improve readability. The ticket calls for removing module-level lines like: NSSWITCH_CONF = paths.NSSWITCH_CONF which are just silly, but assigning a name locally to a global constant is a valid thing to do -- even if the local name just says the file we're working on now. -ca_file = paths.ALIAS_CACERT_ASC -if os.path.exists(ca_file): +if os.path.exists(paths.SYSCONFIG_HTTPD): Whoops! install/wsgi/plugins.py: -PLUGINS_DIR = paths.IPA_JS_PLUGINS_DIR - [...] -if not os.path.isdir(PLUGINS_DIR): +if not os.path.isdir(paths.IPA_CA_CSR): Whoops too! ipaplatform/fedora/tasks.py: ipa-client/ipa-install/ipa-client-install: ipaserver/install/dsinstance.py: ipaserver/install/httpinstance.py: Again, I'd not change the target_fname, filepath. ipapython/sysrestore.py: Again, `SYSRESTORE_PATH` describes better what we do with `paths.TMP`, so I'd prefer keeping it. ipaserver/install/adtrustinstance.py: I don't think we want to convert the self.* to constants. ipaserver/install/certs.py: I'd leave NSS_DIR as it is, rather than lose the comment. ipapython/ipautil.py: ipaserver/install/ldapupdate.py: ipalib/session.py: ipaserver/install/bindinstance.py: SHARE_DIR, UPDATES_DIR, and krbccache_dir, NAMED_CONF (respectively) need to stay, unless you also replace them in everything that uses them. Be sure to run make-lint after doing these changes. I've rebased, and I made some of the changes as I went along the review. You can base another revision on the attached patch. -- Petr³ From a04e19cfd767fa5a994030a6ecf0b3f74fcaa903 Mon Sep 17 00:00:00 2001 From: Gabe redhatri...@gmail.com Date: Mon, 29 Sep 2014 17:23:25 -0600 Subject: [PATCH] Remove trivial path constants from modules https://fedorahosted.org/freeipa/ticket/4399 --- .../certmonger/dogtag-ipa-ca-renew-agent-submit| 8 ++- install/tools/ipa-adtrust-install | 8 ++- install/tools/ipa-ca-install | 5 +- install/tools/ipa-dns-install | 8 ++- install/tools/ipa-replica-conncheck| 9 ++-- install/tools/ipa-replica-install | 6 +-- install/tools/ipa-server-install | 39 ++ install/tools/ipa-upgradeconfig| 30 +-- install/wsgi/plugins.py| 6 +-- ipa-client/ipa-install/ipa-client-automount| 62 ++ ipa-client/ipa-install/ipa-client-install | 37 ++--- ipa-client/ipaclient/ntpconf.py| 28 +- ipalib/session.py | 4 +- ipaplatform/fedora/tasks.py| 6 +-- ipapython/certmonger.py| 8 +-- ipapython/ipautil.py | 1 - ipaserver/dcerpc.py| 3 +- ipaserver/install/adtrustinstance.py | 3 +- ipaserver/install/bindinstance.py | 23 ipaserver/install/dsinstance.py| 9 ++-- ipaserver/install/ipa_backup.py| 7 +-- ipaserver/install/ipa_replica_prepare.py | 15 +++--- ipaserver/install/ipa_restore.py | 3 +- ipaserver/install/sysupgrade.py| 5 +- ipaserver/install/upgradeinstance.py | 5 +- ipaserver/rpcserver.py | 5 +- 26 files changed, 140 insertions(+), 203 deletions(-) diff --git a/install/certmonger/dogtag-ipa-ca-renew-agent-submit b/install/certmonger/dogtag-ipa-ca-renew-agent-submit index 4f0b78accac6840471f8b2e9f17288b3b4e82105..942ffec65d7b041fc6f9d3b2c19d3596fae79d31 100755 --- a/install/certmonger/dogtag-ipa-ca-renew-agent-submit +++ b/install/certmonger/dogtag-ipa-ca-renew-agent-submit @@ -71,8 +71,7 @@ def request_cert(): syslog.syslog(syslog.LOG_NOTICE, Forwarding request to dogtag-ipa-renew-agent) -path = paths.DOGTAG_IPA_RENEW_AGENT_SUBMIT -args = [path] + sys.argv[1:] +args = [paths.DOGTAG_IPA_RENEW_AGENT_SUBMIT] + sys.argv[1:] stdout, stderr, rc = ipautil.run(args, raiseonerr=False, env=os.environ) sys.stderr.write(stderr) sys.stderr.flush() @@ -282,12 +281,11 @@ def export_csr(): if not cert: return (REJECTED, New certificate requests not supported) -csr_file = paths.IPA_CA_CSR try: -with open(csr_file, 'wb') as f: +with
Re: [Freeipa-devel] [PATCH] 0001 Refactor selinuxenabled check
On 09/26/2014 06:34 PM, thierry bordaz wrote: On 09/26/2014 05:53 PM, Francesco Marella wrote: On 26/09/2014 17:43, thierry bordaz wrote: Hello, When called from set_selinux_booleans, if not selinux_enabled, you may want to 'return False' rather than 'return'. Now it looks like callers of set_selinux_booleans do not check the returned value :-) thanks thierry On 09/26/2014 05:26 PM, Francesco Marella wrote: This should be the final one. fm On 26/09/2014 16:30, Francesco Marella wrote: On 26/09/2014 15:41, Petr Viktorin wrote: Hello! Thanks for the patch! The new function is not one of the platform-independent tasks, and doesn't even use `self`, so you can define it as a module-level helper function. But more importantly, this won't work: the blocks you are replacing return from their functions. You'd need to use something like: if not selinux_enabled(): return instead of: self.check_enabled_selinux() ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel Thanks Francesco ! For me it is a ACK. Thanks indeed! Pushed to master: f5b302be47eb94fb064af6b9a1855da4d318898e -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0064] Create ipa-otp-decrement 389DS plugin
On Tue, 2014-09-30 at 18:30 +0200, thierry bordaz wrote:
On 09/29/2014 08:30 PM, Nathaniel McCallum wrote:
On Mon, 2014-09-22 at 09:32 -0400, Simo Sorce wrote:
On Sun, 21 Sep 2014 22:33:47 -0400
Nathaniel McCallum npmccal...@redhat.com wrote:
Comments inline.
+
+#define ch_malloc(type) \
+(type*) slapi_ch_malloc(sizeof(type))
+#define ch_calloc(count, type) \
+(type*) slapi_ch_calloc(count, sizeof(type))
+#define ch_free(p) \
+slapi_ch_free((void**) (p))
please do not redefine slapi functions, it just makes it harder to know
what you used.
+typedef struct {
+bool exists;
+long long value;
+} counter;
please no typedefs of structures, use struct counter { ... }; and
reference it as struct counter in the code.
Btw, FWIW you could use a value of -1 to indicate non-existence of the
counter value, given counters can only be positive, this would avoid
the need for a struct.
+static int
+send_error(Slapi_PBlock *pb, int rc, char *template, ...)
+{
+va_list ap;
+int res;
+
+va_start(ap, template);
+res = vsnprintf(NULL, 0, template, ap);
+va_end(ap);
+
+if (res 0) {
+char str[res + 1];
+
+va_start(ap, template);
+res = vsnprintf(str, sizeof(str), template, ap);
+va_end(ap);
+
+if (res 0)
+slapi_send_ldap_result(pb, rc, NULL, str, 0, NULL);
+}
+
+if (res = 0)
+slapi_send_ldap_result(pb, rc, NULL, NULL, 0, NULL);
+
+slapi_pblock_set(pb, SLAPI_RESULT_CODE, rc);
+return rc;
+}
This function seem not really useful, you use send_error() only at the
end of one single function where you could have the classic scheme of
using a done: label and just use directly slapi_ch_smprintf. This would
remove the need to use vsnprintf and all the va_ machinery which is
more than 50% of this function.
+static long long
+get_value(const LDAPMod *mod, long long def)
+{
+const struct berval *bv;
+long long val;
+
+if (mod == NULL)
+return def;
+
+if (mod-mod_vals.modv_bvals == NULL)
+return def;
+
+bv = mod-mod_vals.modv_bvals[0];
+if (bv == NULL)
+return def;
In general (here and elsewhere) I prefer to always use {} in if clauses.
I have been bitten enough time by people adding an instruction that
should be in the braces but forgot to add braces (defensive programming
style). But up to you.
+char buf[bv-bv_len + 1];
+memcpy(buf, bv-bv_val, bv-bv_len);
+buf[sizeof(buf)-1] = '\0';
+
+val = strtoll(buf, NULL, 10);
+if (val == LLONG_MIN || val == LLONG_MAX)
+return def;
+
+return val;
+}
+
+static struct berval **
+make_berval_array(long long value)
+{
+struct berval **bvs;
+
+bvs = ch_calloc(2, struct berval*);
+bvs[0] = ch_malloc(struct berval);
+bvs[0]-bv_val = slapi_ch_smprintf(%lld, value);
+bvs[0]-bv_len = strlen(bvs[0]-bv_val);
+
+return bvs;
+}
+
+static LDAPMod *
+make_ldapmod(int op, const char *attr, long long value)
+{
+LDAPMod *mod;
+
+mod = (LDAPMod*) slapi_ch_calloc(1, sizeof(LDAPMod));
+mod-mod_op = op | LDAP_MOD_BVALUES;
+mod-mod_type = slapi_ch_strdup(attr);
+mod-mod_bvalues = make_berval_array(value);
+
+return mod;
+}
+
+static void
+convert_ldapmod_to_bval(LDAPMod *mod)
+{
+if (mod == NULL || (mod-mod_op LDAP_MOD_BVALUES))
+return;
+
+mod-mod_op |= LDAP_MOD_BVALUES;
+
+if (mod-mod_values == NULL) {
+mod-mod_bvalues = NULL;
+return;
+}
+
+for (size_t i = 0; mod-mod_values[i] != NULL; i++) {
+struct berval *bv = ch_malloc(struct berval);
+bv-bv_val = mod-mod_values[i];
+bv-bv_len = strlen(bv-bv_val);
+mod-mod_bvalues[i] = bv;
+}
+}
+
+static counter
+get_counter(Slapi_Entry *entry, const char *attr)
+{
+Slapi_Attr *sattr = NULL;
+
+return (counter) {
+slapi_entry_attr_find(entry, attr, sattr) == 0,
+slapi_entry_attr_get_longlong(entry, attr)
+};
+}
+
+static void
+berval_free_array(struct berval***bvals)
+{
+for (size_t i = 0; (*bvals)[i] != NULL; i++) {
+ch_free((*bvals)[i]-bv_val);
+ch_free((*bvals)[i]);
+}
+
+slapi_ch_free((void**) bvals);
+}
+
+static bool
+is_replication(Slapi_PBlock *pb)
+{
+int repl = 0;
+slapi_pblock_get(pb, SLAPI_IS_REPLICATED_OPERATION, repl);
+return
