Re: [Freeipa-devel] [PATCH 83] Cookie Expires date should be locale insensitive
On Wed, Dec 19, 2012 at 01:36:40PM -0500, John Dennis wrote: -- John Dennis jden...@redhat.com Looking to carve out IT costs? www.redhat.com/carveoutcosts/ Patch is working as expected and he code looks good to me. I just have a minor comment. I think 'import time' can be removed from both files. Although it looks like it wasn't used even before your patch I wonder if you can add the removal here? bye, Sumit ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 83] Cookie Expires date should be locale insensitive
On 12/19/2012 07:36 PM, John Dennis wrote: I tested the patch on RHEL platform and it works fine and removes the annoying error. My comments on the patch: 1) I do not think its necessary to write target branches to commit message. Also there is a typo: ipapython/cooke.py 2) As for the tests - could we for example try setting non-US locale in the test to verify that cookie lib is locale independent? Python has means to do that, (import locale; locale.setlocale(locale.LC_ALL, 'cs_CZ')). But this is not a blocker for this patch. I am sure that Petr^3 will have more comments on the code as he is reviewing it too :-) Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 83] Cookie Expires date should be locale insensitive
On 12/19/2012 07:36 PM, John Dennis wrote: The Expires attribute in a cookie is supposed to follow the RFC 822 (superseded by RFC 1123) date format. That format includes a weekday abbreviation (e.g. Tue) which must be in English according to the RFC's. ipapython/cooke.py has methods to parse and format the Expires attribute but they were based on strptime() and strftime() which respects the locale. If a non-English locale is in effect the wrong date string will be produced and/or it won't be able to parse the date string. The fix is to use the date parsing and formatting functions from email.utils which specifically follow the RFC's and are not locale sensitive. This patch also updates the unit test to use email.utils as well. The patch should be applied to the following branches: master, 3.0, 3.1 Ticket:https://fedorahosted.org/freeipa/ticket/3313 This solves the issue for me. It's better than what's there now, so It's OK as a hotfix. However, I did find something to discuss. Your comment references RFC 1123, which doesn't seem relevant at all. The cookie Expires header is defined in RFC 6265 (section 5.1.1), but email.utils.parsedate uses syntax defined in RFC 2822. Apparently the two are equivalent for common usage, but RFC 6265 specifies a more complicated algorithm. Shouldn't we follow it? To nitpick, I'm not a fan of including target branches in the commit message (they should be in the accompanying e-mail), or of documenting past bugs as comments in the code (git log/blame works better for checking history). -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0043 Allow-PKI-CA-Replica-Installs-when-CRL-exceeds-default
On Wed, 2012-12-19 at 21:35 -0500, Simo Sorce wrote: On Wed, 2012-12-19 at 22:41 +, JR Aquino wrote: On Dec 19, 2012, at 2:32 PM, Simo Sorce wrote: On Wed, 2012-12-19 at 20:52 +, JR Aquino wrote: Due to a limitation with 389 DS, the nsslapd-maxbersize cannot be set dynamically. This causes an issue during IPA PKI-CA Replica installs, when the master has a CRL that exceeds the default limit. The cainstance.py code attempts to set this value prior to performing the initial PKI-CA replication, however, since the value cannot be set dynamically, the installation fails. This patch works around the issue by adding the ldif to the original initialization values bootstrapped by the call to setup-ds.pl Why are we not simply restarting the instance after setting the value ? What's in database.ldif ? What produces it ? /usr/share/pki/ca/conf/database.ldif is part of the dogtag installation and it contains the following entry: dn: cn=config changetype: modify replace: nsslapd-maxbersize nsslapd-maxbersize: 209715200 It's purpose is to increase the limit for maxbersize from 2097152 to 209715200. The ldif is inserted via the jars that are wrapped by pkisilent... So this leaves 3 options: #1 Add code to perform the ldap insert followed by a dirsrv restart into the cainstance.py code #2 Add recode the jar files from DogTag to require a dirsrv restart after the insert, but prior to the replication #3 Just initialize the dirsrv database with the correct value to begin with. 1 line fix #4 Ask 389 to allow maxbersize to be a dynamically initialized variable #3 Seemed the path of least resistance. I did take the time to code #1 and verify that it worked as well. I have a ticket open for #4 Alee hinted that the jar modifications for #2 might not be trivial... Method #3 is ok, but for master, where we have unified ds instances, you should look at doing ti as we do change other similar attributes in install/updates/10-config.update so that older installations are updated as well. If you do it only at install and the CRL grows later you'd get older server start choking because they have not been updated. Are you referring to masters which have been converted from non-unified DS to a single DS using an as-yet-to-be-written script? The ldif change mentioned above is already performed as part of the dogtag install. For a freshly installed master, there is no large CRL to break the installation. In the replica scenario, this change is needed before we attempt replication because the large CRL breaks replication. In fact, if that value had not been set on the master, there would be no large CRL to cause replication problems. Simo. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0043 Allow-PKI-CA-Replica-Installs-when-CRL-exceeds-default
On Thu, 2012-12-20 at 09:39 -0500, Ade Lee wrote: On Wed, 2012-12-19 at 21:35 -0500, Simo Sorce wrote: On Wed, 2012-12-19 at 22:41 +, JR Aquino wrote: On Dec 19, 2012, at 2:32 PM, Simo Sorce wrote: On Wed, 2012-12-19 at 20:52 +, JR Aquino wrote: Due to a limitation with 389 DS, the nsslapd-maxbersize cannot be set dynamically. This causes an issue during IPA PKI-CA Replica installs, when the master has a CRL that exceeds the default limit. The cainstance.py code attempts to set this value prior to performing the initial PKI-CA replication, however, since the value cannot be set dynamically, the installation fails. This patch works around the issue by adding the ldif to the original initialization values bootstrapped by the call to setup-ds.pl Why are we not simply restarting the instance after setting the value ? What's in database.ldif ? What produces it ? /usr/share/pki/ca/conf/database.ldif is part of the dogtag installation and it contains the following entry: dn: cn=config changetype: modify replace: nsslapd-maxbersize nsslapd-maxbersize: 209715200 It's purpose is to increase the limit for maxbersize from 2097152 to 209715200. The ldif is inserted via the jars that are wrapped by pkisilent... So this leaves 3 options: #1 Add code to perform the ldap insert followed by a dirsrv restart into the cainstance.py code #2 Add recode the jar files from DogTag to require a dirsrv restart after the insert, but prior to the replication #3 Just initialize the dirsrv database with the correct value to begin with. 1 line fix #4 Ask 389 to allow maxbersize to be a dynamically initialized variable #3 Seemed the path of least resistance. I did take the time to code #1 and verify that it worked as well. I have a ticket open for #4 Alee hinted that the jar modifications for #2 might not be trivial... Method #3 is ok, but for master, where we have unified ds instances, you should look at doing ti as we do change other similar attributes in install/updates/10-config.update so that older installations are updated as well. If you do it only at install and the CRL grows later you'd get older server start choking because they have not been updated. Are you referring to masters which have been converted from non-unified DS to a single DS using an as-yet-to-be-written script? I was thinking of a current 3.1 setup with multiple replicas installed before this patch lands in Fedora. Old master (3.0) with split instances, new replicas (3.1) with unified instances. After a while CRL in master grows past limit. All replicas break because no update fixed them. The ldif change mentioned above is already performed as part of the dogtag install. For a freshly installed master, there is no large CRL to break the installation. In the replica scenario, this change is needed before we attempt replication because the large CRL breaks replication. In fact, if that value had not been set on the master, there would be no large CRL to cause replication problems. Understood, I am not asking for a huge change, just that the change is done in an update file and not just on a fresh install. 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 83] Cookie Expires date should be locale insensitive
On 12/20/2012 12:35 PM, Petr Viktorin wrote: On 12/19/2012 07:36 PM, John Dennis wrote: The Expires attribute in a cookie is supposed to follow the RFC 822 (superseded by RFC 1123) date format. That format includes a weekday abbreviation (e.g. Tue) which must be in English according to the RFC's. ipapython/cooke.py has methods to parse and format the Expires attribute but they were based on strptime() and strftime() which respects the locale. If a non-English locale is in effect the wrong date string will be produced and/or it won't be able to parse the date string. The fix is to use the date parsing and formatting functions from email.utils which specifically follow the RFC's and are not locale sensitive. This patch also updates the unit test to use email.utils as well. The patch should be applied to the following branches: master, 3.0, 3.1 Ticket:https://fedorahosted.org/freeipa/ticket/3313 This solves the issue for me. It's better than what's there now, so It's OK as a hotfix. However, I did find something to discuss. Your comment references RFC 1123, which doesn't seem relevant at all. The cookie Expires header is defined in RFC 6265 (section 5.1.1), but email.utils.parsedate uses syntax defined in RFC 2822. Apparently the two are equivalent for common usage, but RFC 6265 specifies a more complicated algorithm. Shouldn't we follow it? To nitpick, I'm not a fan of including target branches in the commit message (they should be in the accompanying e-mail), or of documenting past bugs as comments in the code (git log/blame works better for checking history). Pushed to master, ipa-3-1, ipa-3-0 (since we need this hotfix now). I just fixed the commit message as written above. You can open another upstream ticket to fix these comment discrepancies. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0043 Allow-PKI-CA-Replica-Installs-when-CRL-exceeds-default
On 12/20/2012 06:49 AM, Simo Sorce wrote: On Thu, 2012-12-20 at 09:39 -0500, Ade Lee wrote: On Wed, 2012-12-19 at 21:35 -0500, Simo Sorce wrote: On Wed, 2012-12-19 at 22:41 +, JR Aquino wrote: On Dec 19, 2012, at 2:32 PM, Simo Sorce wrote: On Wed, 2012-12-19 at 20:52 +, JR Aquino wrote: Due to a limitation with 389 DS, the nsslapd-maxbersize cannot be set dynamically. This causes an issue during IPA PKI-CA Replica installs, when the master has a CRL that exceeds the default limit. The cainstance.py code attempts to set this value prior to performing the initial PKI-CA replication, however, since the value cannot be set dynamically, the installation fails. This patch works around the issue by adding the ldif to the original initialization values bootstrapped by the call to setup-ds.pl Why are we not simply restarting the instance after setting the value ? What's in database.ldif ? What produces it ? /usr/share/pki/ca/conf/database.ldif is part of the dogtag installation and it contains the following entry: dn: cn=config changetype: modify replace: nsslapd-maxbersize nsslapd-maxbersize: 209715200 It's purpose is to increase the limit for maxbersize from 2097152 to 209715200. If your CA is relatively recent, 209715200 should give you enough room to generate CRLs v1 with up to 9.4 millions entries. If you plan on having bigger CRLs, consider further increase of nsslapd-maxbersize. The ldif is inserted via the jars that are wrapped by pkisilent... So this leaves 3 options: #1 Add code to perform the ldap insert followed by a dirsrv restart into the cainstance.py code #2 Add recode the jar files from DogTag to require a dirsrv restart after the insert, but prior to the replication #3 Just initialize the dirsrv database with the correct value to begin with. 1 line fix #4 Ask 389 to allow maxbersize to be a dynamically initialized variable #3 Seemed the path of least resistance. I did take the time to code #1 and verify that it worked as well. I have a ticket open for #4 Alee hinted that the jar modifications for #2 might not be trivial... Method #3 is ok, but for master, where we have unified ds instances, you should look at doing ti as we do change other similar attributes in install/updates/10-config.update so that older installations are updated as well. If you do it only at install and the CRL grows later you'd get older server start choking because they have not been updated. Are you referring to masters which have been converted from non-unified DS to a single DS using an as-yet-to-be-written script? I was thinking of a current 3.1 setup with multiple replicas installed before this patch lands in Fedora. Old master (3.0) with split instances, new replicas (3.1) with unified instances. After a while CRL in master grows past limit. All replicas break because no update fixed them. The ldif change mentioned above is already performed as part of the dogtag install. For a freshly installed master, there is no large CRL to break the installation. In the replica scenario, this change is needed before we attempt replication because the large CRL breaks replication. In fact, if that value had not been set on the master, there would be no large CRL to cause replication problems. Understood, I am not asking for a huge change, just that the change is done in an update file and not just on a fresh install. Simo. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
