Do not apply format expansions to passwords

get_one_cloud() and get_one_cloud_osc() iterate over config
values and try to expand any variables in those values by
calling value.format(), however some config values
(e.g. password) should never have format() applied to them, not
only might that change the password but it will also cause the
format() function to raise an exception if it can not parse the
format string. Examples would be single brace (e.g. 'foo{')
which raises an ValueError because it's looking for a matching
end brace or a brace pair with a key value that cannot be found
(e.g. 'foo{bar}') which raises a KeyError.

It is not reasonsable to try to escape any braces because:

1) Escaping all braces breaks valid use of the format string syntax.

2) Trying to determine exactly which braces should be escaped and
which should be preserved is a daunting task and likely would not be
robust.

3) Some strings might look like valid format syntax but still should
be escaped (e.g. "foo{bar}", if this appeared in a password we
wouldn't escape it and there would be a key error on the 'bar' key.

4) In general passwords should never be modified, you never want to
apply formatting to them.

The right approach is to maintain a list of config values which are
excluded from having formatting applied to them. At the moment that
list just includes 'password' but perhaps down the road other
exceptions might crop up. This patch follows this approach,
the list of excluded values can easily be updated if others are
discovered.

Change-Id: I187bdec582d4c2cc6c7fda47a1538194137c616b
Closes-Bug: 1635696
Signed-off-by: John Dennis <jdennis@redhat.com>

2 files changed, 64 insertions, 2 deletions

diff --git a/os_client_config/config.py b/os_client_config/config.py
index 4c054bf..ce4f045 100644
--- a/os_client_config/config.py
+++ b/os_client_config/config.py
@@ -69,6 +69,8 @@ VENDOR_FILES = [
 
 BOOL_KEYS = ('insecure', 'cache')
 
+FORMAT_EXCLUSIONS = frozenset(['password'])
+
 
 # NOTE(dtroyer): This turns out to be not the best idea so let's move
 #                overriding defaults to a kwarg to OpenStackConfig.__init__()
@@ -1089,7 +1091,7 @@ class OpenStackConfig(object):
 
         # If any of the defaults reference other values, we need to expand
         for (key, value) in config.items():
-            if hasattr(value, 'format'):
+            if hasattr(value, 'format') and key not in FORMAT_EXCLUSIONS:
                 config[key] = value.format(**config)
 
         force_ipv4 = config.pop('force_ipv4', self.force_ipv4)
@@ -1184,7 +1186,7 @@ class OpenStackConfig(object):
 
         # If any of the defaults reference other values, we need to expand
         for (key, value) in config.items():
-            if hasattr(value, 'format'):
+            if hasattr(value, 'format') and key not in FORMAT_EXCLUSIONS:
                 config[key] = value.format(**config)
 
         force_ipv4 = config.pop('force_ipv4', self.force_ipv4)
diff --git a/os_client_config/tests/test_config.py b/os_client_config/tests/test_config.py
index 5a8a99c..8ea6ee1 100644
--- a/os_client_config/tests/test_config.py
+++ b/os_client_config/tests/test_config.py
@@ -376,6 +376,66 @@ class TestConfig(base.TestCase):
         self.assertEqual(region, {'name': 'no-cloud-region', 'values': {}})
 
 
+class TestExcludedFormattedConfigValue(base.TestCase):
+    # verify LaunchPad bug #1635696
+    #
+    # get_one_cloud() and get_one_cloud_osc() iterate over config
+    # values and try to expand any variables in those values by
+    # calling value.format(), however some config values
+    # (e.g. password) should never have format() applied to them, not
+    # only might that change the password but it will also cause the
+    # format() function to raise an exception if it can not parse the
+    # format string. Examples would be single brace (e.g. 'foo{')
+    # which raises an ValueError because it's looking for a matching
+    # end brace or a brace pair with a key value that cannot be found
+    # (e.g. 'foo{bar}') which raises a KeyError.
+
+    def setUp(self):
+        super(TestExcludedFormattedConfigValue, self).setUp()
+
+        self.args = dict(
+            auth_url='http://example.com/v2',
+            username='user',
+            project_name='project',
+            region_name='region2',
+            snack_type='cookie',
+            os_auth_token='no-good-things',
+        )
+
+        self.options = argparse.Namespace(**self.args)
+
+    def test_get_one_cloud_password_brace(self):
+        c = config.OpenStackConfig(config_files=[self.cloud_yaml],
+                                   vendor_files=[self.vendor_yaml])
+
+        password = 'foo{'       # Would raise ValueError, single brace
+        self.options.password = password
+        cc = c.get_one_cloud(
+            cloud='_test_cloud_regions', argparse=self.options, validate=False)
+        self.assertEqual(cc.password, password)
+
+        password = 'foo{bar}'   # Would raise KeyError, 'bar' not found
+        self.options.password = password
+        cc = c.get_one_cloud(
+            cloud='_test_cloud_regions', argparse=self.options, validate=False)
+        self.assertEqual(cc.password, password)
+
+    def test_get_one_cloud_osc_password_brace(self):
+        c = config.OpenStackConfig(config_files=[self.cloud_yaml],
+                                   vendor_files=[self.vendor_yaml])
+        password = 'foo{'       # Would raise ValueError, single brace
+        self.options.password = password
+        cc = c.get_one_cloud_osc(
+            cloud='_test_cloud_regions', argparse=self.options, validate=False)
+        self.assertEqual(cc.password, password)
+
+        password = 'foo{bar}'   # Would raise KeyError, 'bar' not found
+        self.options.password = password
+        cc = c.get_one_cloud_osc(
+            cloud='_test_cloud_regions', argparse=self.options, validate=False)
+        self.assertEqual(cc.password, password)
+
+
 class TestConfigArgparse(base.TestCase):
 
     def setUp(self):