[PATCH] 36 - Fix for track ticket 493 in Dogtag 10.0.2 - No interpolation for password fields.
Please review the patch attached for fixing the ticket 493 in Dogtag 10.0.2. Tested in both in both interactive and file-passing (-f option) of pkispawn. --Abhishek
On 3/4/2013 10:03 PM, Abhishek Koneru wrote:
Please review the patch attached for fixing the ticket 493 in Dogtag 10.0.2.
Tested in both in both interactive and file-passing (-f option) of pkispawn.
--Abhishek
Some comments: 1. In set_property() the new code checks whether the new property is sensitive. If it is the value will be added directly into the dict, otherwise it will use the ConfigParser.set(). This code might not be necessary. To my understanding the interpolation will be evaluated during retrieval only (in get() and items()), so changing how the value is stored would not affect the interpolation. 2. In the new code the sensitive_parameters will be parsed each time flatten_master_dict() is called. It might be better to parse the list once right after the file is loaded in read_pki_configuration_file(), and store the parsed value as an attribute so it can be used by other methods without parsing it again. 3. The list_items() could be simplified. It's not necessary to merge the section with the default section because interpolation only works in the same section. I think you could call ConfigParser.items(section, True) to get all (param, raw_value) pairs. If the parameter is not sensitive then call ConfigParser.get(section, param) to get the interpolated value and use it to replace the raw value. Then either return all tuples as a list like the original items() or put them in a dict like in your patch. 4. What does the 'list' in list_items() mean? It might be better to use the same name as the original method it's trying to replace (e.g. items()). -- Endi S. Dewata
On discussion with Endi and Ade, it has been agreed to use an escape('%') character to allow usage of % in the values of the configuration file. If a configuration file is passed to the pkispawn script, any occurrence of '%' must have a '%' used with it as an escape character. Eg. key=abc%def, must be entered as key=abc%%def => which is read as abc %def. Similarly, key=abc%%def must be key=abc%%%%def I the interactive mode, the escape character is added by the code. User can enter the actual value without the escape character. Please review the attached patch attached for with the required changes. --Abhishek On Tue, 2013-03-05 at 14:59 -0600, Endi Sukma Dewata wrote:
On 3/4/2013 10:03 PM, Abhishek Koneru wrote:
Please review the patch attached for fixing the ticket 493 in Dogtag 10.0.2.
Tested in both in both interactive and file-passing (-f option) of pkispawn.
--Abhishek
Some comments:
1. In set_property() the new code checks whether the new property is sensitive. If it is the value will be added directly into the dict, otherwise it will use the ConfigParser.set(). This code might not be necessary. To my understanding the interpolation will be evaluated during retrieval only (in get() and items()), so changing how the value is stored would not affect the interpolation.
2. In the new code the sensitive_parameters will be parsed each time flatten_master_dict() is called. It might be better to parse the list once right after the file is loaded in read_pki_configuration_file(), and store the parsed value as an attribute so it can be used by other methods without parsing it again.
3. The list_items() could be simplified. It's not necessary to merge the section with the default section because interpolation only works in the same section. I think you could call ConfigParser.items(section, True) to get all (param, raw_value) pairs. If the parameter is not sensitive then call ConfigParser.get(section, param) to get the interpolated value and use it to replace the raw value. Then either return all tuples as a list like the original items() or put them in a dict like in your patch.
4. What does the 'list' in list_items() mean? It might be better to use the same name as the original method it's trying to replace (e.g. items()).
On 3/7/2013 10:33 AM, Abhishek Koneru wrote:
On discussion with Endi and Ade, it has been agreed to use an escape('%') character to allow usage of % in the values of the configuration file.
If a configuration file is passed to the pkispawn script, any occurrence of '%' must have a '%' used with it as an escape character. Eg. key=abc%def, must be entered as key=abc%%def => which is read as abc %def. Similarly, key=abc%%def must be key=abc%%%%def
I the interactive mode, the escape character is added by the code. User can enter the actual value without the escape character.
Please review the attached patch attached for with the required changes.
ACK. Just a few things: 1. Please clean up the whitespace before push: git am --whitespace=fix <patch> 2. Please add a one-line title/summary in the commit log. See previous commit logs for example. The first line of the log message is used in many things such as patch filename, patch title, trac link, release notes, etc. so it would be nice to have a short but descriptive title. -- Endi S. Dewata
Followed the steps. Pushed to master. --Abhishek ----- Original Message ----- From: "Endi Sukma Dewata" <edewata@redhat.com> To: "Abhishek Koneru" <akoneru@redhat.com> Cc: "pki-devel" <pki-devel@redhat.com> Sent: Thursday, March 7, 2013 1:22:18 PM Subject: Re: [PATCH] 36-1 Revised - [PATCH] 36 - Fix for track ticket 493 in Dogtag 10.0.2 - No interpolation for password fields. On 3/7/2013 10:33 AM, Abhishek Koneru wrote:
On discussion with Endi and Ade, it has been agreed to use an escape('%') character to allow usage of % in the values of the configuration file.
If a configuration file is passed to the pkispawn script, any occurrence of '%' must have a '%' used with it as an escape character. Eg. key=abc%def, must be entered as key=abc%%def => which is read as abc %def. Similarly, key=abc%%def must be key=abc%%%%def
I the interactive mode, the escape character is added by the code. User can enter the actual value without the escape character.
Please review the attached patch attached for with the required changes.
ACK. Just a few things: 1. Please clean up the whitespace before push: git am --whitespace=fix <patch> 2. Please add a one-line title/summary in the commit log. See previous commit logs for example. The first line of the log message is used in many things such as patch filename, patch title, trac link, release notes, etc. so it would be nice to have a short but descriptive title. -- Endi S. Dewata
participants (2)
-
Abhishek Koneru -
Endi Sukma Dewata