Improvements in {get|set}_directive functions #347

martbab · 2016-12-16T15:25:05Z

These should fix https://fedorahosted.org/freeipa/ticket/6460 and future-proof
the code a bit

pspacek

Please see my in-line comments.

pspacek · 2016-12-19T07:20:52Z

ipapython/ipautil.py

@@ -1294,15 +1294,17 @@ def unescape_seq(seq, *args):

 def escape_seq(seq, *args):
    """
-    escape (prepend '\\') all occurences of sequence in input strings
+    escape (prepend '\\') all occurences of sequence in input strings unless it
+    is already escaped.


I think that the function called escape_seq should have no say in what is being escaped. Given that next commit modified get_directive to be inverse of set_directive, why is this change necessary at all?

Probably not, I was just thinking about future proofing the code. You are probably right that the function should be as dumb as possible.

pspacek · 2016-12-19T07:22:03Z

ipaserver/install/installutils.py

+        file to quote values. This character will be stripped and unescaped
+        from the raw value.
+
+    :returns: The (unquoted) value if the directive was found, None otherwise


I know this is not originally your code but KeyError exception souds more appropriate to me (for non-existing directives)...

I would rather not alter the function's contract in this way, who know how many callers are relying on this particular behavior. If you open a ticket for this I can do it in separate PR.

No problem, I do not insist on this change.

pspacek · 2016-12-19T07:25:30Z

ipaserver/install/installutils.py

    """
    fd = open(filename, "r")
    for line in fd:
        if line.lstrip().startswith(directive):
            line = line.strip()
            result = line.split(separator, 1)[1]
-            result = result.strip('"')
+            result = result.strip(quote_char)


I think that this usage of strip will cause problems if the text in config file is like \"abc\". It will remove only the last " (which is incorrect anyway because " are part of the string and not quotation).

BTW this usage of strip does not remove any whitespace (unless quote_char is a whitespace ...) so the commit message needs correction as well.

Line 456: line = line.strip() so yes it does. The code masks this fact rather well.

Regarding stripping the quote_char, I realized that the correct thing to do is actually to first unescape all quote chars and then do the strip as this is the inverse operation of set_directive. Do you agree with such a change?.

Hmm, that depends. How is quote_char supposed to behave? Can it be used just once in each value like "this is my value" ? Or is it possible to use it as in C strings like "first" " part" "of " "string" producing string value first part of string?

Speaking of whitespace, the functions should be really inversible. Is there a reason to mock with the whitespace when reading values? (Just wondering out loud.)

Thinking about it a bit more, I would argue that first use of strip() on line #347 (diff) is incorrect anyway. The code first does strip() and then split(separator, 1)[1].

What if result of split(separator, 1)[1] contains whitespace?

The way set_directive handle quote_char is that if it is present in the value-to-be added, it is escaped and then the whole value is stored:

my "quoted" value -> "my \"quoted\" value"

The get_directive should then do an inverse operation (regarding quotes, not the whitespace shenanigans).

The stripping around separator is buggy as you pointed out, the whitespace after separator is not handled correctly and breaks the following logic. I will fix that.

pspacek · 2016-12-19T07:30:19Z

Please see my in-line comments.

martbab · 2016-12-20T15:16:47Z

Please see my in-line replies to your in-line comments :).

martbab · 2017-01-03T09:02:48Z

Can somebody take over the review of this PR please?

tkrizek · 2017-01-09T11:50:10Z

ipaserver/install/installutils.py

@@ -436,16 +436,31 @@ def format_directive(directive, value, separator, quotes, quote_char):
    fd.close()
    os.chown(filename, st.st_uid, st.st_gid) # reset perms

-def get_directive(filename, directive, separator=' '):
+
+def get_directive(filename, directive, separator=' ', quote_char='\"'):


Is there a reason to escape the double quote in quote_char? It seems to be the same thing as '"'. It seems a bit confusing to me to use the escaped '\"'.

tkrizek · 2017-01-09T12:10:26Z

ipaserver/install/ipa_server_certinstall.py

@@ -134,14 +134,18 @@ def install_http_cert(self):
        dirname = certs.NSS_DIR

        old_cert = installutils.get_directive(paths.HTTPD_NSS_CONF,
-                                              'NSSNickname')
+                                              'NSSNickname', quote_char="'")


Why is it safe to assume the NSSNickname will always be enclosed in single quotes?

mod_nss allows values to be enclosed in double quotes. If double quotes were indeed used, they'd be interpreted as a part of the value, which is not intended. It would be the exact same issue as ticket 6460, except for double quotes instead of single quotes.

You are right. I think that I will delegate the quoting/unquoting transformations to separate functions. That will be more robust IMHO.

tkrizek · 2017-01-09T12:27:30Z

Please see my feedback in in-line comments.

tkrizek · 2017-01-17T11:49:36Z

ipaserver/install/installutils.py

+    :returns: processed value
+    """
+    unescaped_value = "".join(ipautil.unescape_seq(quote_char, value))
+    return unescaped_value.strip(quote_char)


Using strip() causes issues when the actual value starts or ends with the same type of quote as is used to escape it, e.g. 'cn=\'ABC\'' gets unquoted to cn='ABC.

tkrizek · 2017-01-17T11:50:31Z

ipaserver/install/installutils.py

+    :param directive: directive name
+    :param value: value of the directive
+    :param quotes: whether to quote `value` in `quote_char`. If true, then
+        the `quote_char` are first escaped to avoid unparseable directives.


Please remove reference to quote_char from comment.

tkrizek · 2017-01-17T11:51:43Z

ipaserver/install/installutils.py

+    :param filename: input filename
+    :param directive: directive name
+    :param separator: separator between directive and value
+    :param quote_char: the characters that are used in this particular config


Update doc here as well.

tkrizek · 2017-01-17T11:55:51Z

I still managed to find a an issue for certain edge cases. See inline comments for more info.

martbab · 2017-01-17T15:09:08Z

Thanks, I have fixed the docstrings. I have also made directive unquoting less silly.

tkrizek · 2017-01-24T17:01:07Z

I wasn't able to find any more issues with the quoting of certificate names. The directive quoting seems to work properly now.

martbab · 2017-01-25T12:46:41Z

Thank's, let's hope that all this code will be replaced by some proper configuration parsing mechanism in the future.

Add missing parameter descriptions and fix incorrect indentation https://fedorahosted.org/freeipa/ticket/6460

`get_directive` value parsing was improved in order to bring its logic more in-line to changes in `set_directive`: a specified quoting character is now unquoted and stripped from the retrieved value. The function will now also error out when malformed directive is encountered. https://fedorahosted.org/freeipa/ticket/6460

Separate functions were added to installutils module to quote/unquote a string in arbitrary characters. `installutils.get/set_directive` functions will use them to enclose the directive values in double quotes/strip the double quotes from retrieved values to maintain the original behavior. These functions can be used also for custom quoting/unquoting of retrieved values when desired. https://fedorahosted.org/freeipa/ticket/6460

Improve the single/double quote handling during parsing/unparsing of nss.conf's NSSNickname directive. Single quotes are now added/stripped explicitly when handling the certificate nickname. https://fedorahosted.org/freeipa/ticket/6460

martbab · 2017-01-25T14:02:42Z

Fixed upstream
master:
https://fedorahosted.org/freeipa/changeset/e1ed8b5eff40331ba532d37f3fb08814d8a55b77
https://fedorahosted.org/freeipa/changeset/517d43e78b8d8ea0b796a6ff6a379236eaae21df
https://fedorahosted.org/freeipa/changeset/2831b30e9a9de947481c058d8d32e174f951b1c0
https://fedorahosted.org/freeipa/changeset/86f4a93fb3aeb6742acab5abaa1c17b525ea4223

pspacek suggested changes Dec 19, 2016

View reviewed changes

martbab force-pushed the set-get-directive-fix branch from 4b3344f to 3de19d3 Compare December 22, 2016 13:06

tkrizek self-assigned this Jan 3, 2017

tkrizek reviewed Jan 9, 2017

View reviewed changes

martbab force-pushed the set-get-directive-fix branch from 3de19d3 to ed94035 Compare January 13, 2017 09:32

tkrizek reviewed Jan 17, 2017

View reviewed changes

martbab force-pushed the set-get-directive-fix branch from ed94035 to abc6186 Compare January 17, 2017 15:08

tkrizek added the ack Pull Request approved, can be merged label Jan 24, 2017

Martin Babinsky added 4 commits January 25, 2017 13:49

Fix the installutils.set_directive docstring

245fbdc

Add missing parameter descriptions and fix incorrect indentation https://fedorahosted.org/freeipa/ticket/6460

martbab force-pushed the set-get-directive-fix branch from abc6186 to 4028965 Compare January 25, 2017 12:50

martbab added the pushed Pull Request has already been pushed label Jan 25, 2017

martbab closed this Jan 25, 2017

tkrizek mentioned this pull request May 31, 2017

[4.4] NSSNickname enclosed in single quotes causes ipa-server-certinstall failure #834

Closed

Improvements in {get|set}_directive functions #347

Improvements in {get|set}_directive functions #347

martbab commented Dec 16, 2016 •

edited

pspacek left a comment

pspacek Dec 19, 2016

martbab Dec 19, 2016

pspacek Dec 19, 2016

martbab Dec 19, 2016

pspacek Dec 21, 2016

pspacek Dec 19, 2016

pspacek Dec 19, 2016

martbab Dec 19, 2016

martbab Dec 19, 2016

pspacek Dec 21, 2016

pspacek Dec 21, 2016

martbab Dec 21, 2016

pspacek commented Dec 19, 2016

martbab commented Dec 20, 2016

martbab commented Jan 3, 2017

tkrizek Jan 9, 2017

tkrizek Jan 9, 2017

martbab Jan 9, 2017

tkrizek commented Jan 9, 2017

tkrizek Jan 17, 2017 •

edited

tkrizek Jan 17, 2017

tkrizek Jan 17, 2017

tkrizek commented Jan 17, 2017

martbab commented Jan 17, 2017

tkrizek commented Jan 24, 2017

martbab commented Jan 25, 2017

martbab commented Jan 25, 2017

Improvements in {get|set}_directive functions #347

Improvements in {get|set}_directive functions #347

Conversation

martbab commented Dec 16, 2016 • edited

pspacek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pspacek commented Dec 19, 2016

martbab commented Dec 20, 2016

martbab commented Jan 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkrizek commented Jan 9, 2017

tkrizek Jan 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkrizek commented Jan 17, 2017

martbab commented Jan 17, 2017

tkrizek commented Jan 24, 2017

martbab commented Jan 25, 2017

martbab commented Jan 25, 2017

martbab commented Dec 16, 2016 •

edited

tkrizek Jan 17, 2017 •

edited