Merge lp:~danilo/landscape-charm/license-file-plain-text into lp:~landscape/landscape-charm/trunk

[Данило Шеган]

Allow license files to be put in as plain text as well

After some discussion, we've decided to support both plain text and base64-encoded data for license-file charm config option (along with URLs to fetch the license from).

This means that we need to base64-decode only when really needed, and base64 module is not helping there.

Allow license files to be put in as plain text as well

After some discussion, we've decided to support both plain text and base64-encoded data for license-file charm config option (along with URLs to fetch the license from).

This means that we need to base64-decode only when really needed, and base64 module is not helping there.

To test, you can use landscape.yaml from https://pastebin.canonical.com/133087/ (and feel free to change license-file values) and then deploy with

  juju-deployer -vdW -w90 -c landscape.yaml landscape

/etc/landscape/license.txt should have the actual value.

To allow people to input license-file field with newlines and accidental padding, we are also allowing whitespaces (\t\r\n and space) in our base64 alphabet. Maybe that's too broad.

Note: there seems to be a Python documentation bug. https://docs.python.org/2.7/library/base64.html talks about b64decode throwing TypeError "if there are non-alphabet characters present in the string", but that doesn't happen:
https://pastebin.canonical.com/133085/ — I will report a Python doc bug later (it doesn't happen on 2.7.3 either). There's actually https://bugs.python.org/issue1466065 already, but I can't reach bugs.python.org atm.

[Geoff Teale (tealeg)]

I might be missing something, but this seems like an incredible brittle way to decide whether something is base64 encoded or not.

review: Needs Fixing

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Success
Revno: 313
Branch: lp:~danilo/landscape-charm/license-file-plain-text
Jenkins: https://ci.lscape.net/job/latch-test/1395/

review: Approve (test results)

[lp:~danilo/landscape-charm/license-file-plain-text]

314. By Данило Шеган on 2015-06-15

Treat whitespace mid-line in license-file value as an indication of plain-text data.

[Данило Шеган (danilo)]

Addressed your review comments, gents. Let me know what you think?

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Success
Revno: 314
Branch: lp:~danilo/landscape-charm/license-file-plain-text
Jenkins: https://ci.lscape.net/job/latch-test/1412/

review: Approve (test results)

[Geoff Teale (tealeg)]

OK, the explanation is good enough for me - by killing whitespace in the middle of lines we've killed of a lot of cases.

I still don't like this kind of mechanism, but I guess there isn't too much we can do.

review: Approve

[Данило Шеган (danilo)]

Oh, I agree I don't like it either: I would have we rather went one way or the other (base64 or plain text), but since we've got SSL certs being base64 already (inspired by apache2 [and haproxy] charms), I'd hate us to not support the same format for license-file as well, even though I believe plain-text is way more natural (I'd like SSL certs to support plain text as well, but not with this branch :)).

[Björn Tillenius (bjornt)]

Looks good, +1

review: Approve

=== modified file 'lib/callbacks/filesystem.py'
--- lib/callbacks/filesystem.py	2015-06-11 12:39:51 +0000
+++ lib/callbacks/filesystem.py	2015-06-15 10:08:40 +0000
@@ -2,6 +2,7 @@
 
 import os
 import base64
+import re
 import urllib2
 
 from charmhelpers.core import host
@@ -12,6 +13,12 @@
 from lib.utils import get_required_data
 
 
+# Python base64 module will decode strings which might contain non-base64
+# alphabet characters.  To ensure we can support both plain text and
+# base64-encoded values, we pre-check strings against a base64 alphabet.
+BASE64_ALPHABET_RE = re.compile("^[A-Za-z0-9+/]+[=]{0,2}$")
+
+
 class LicenseFileUnreadableError(CharmError):
     """Unable to read the license file."""
 
@@ -108,11 +115,21 @@
             except urllib2.URLError:
                 raise LicenseFileUnreadableError(license_file_value)
         else:
+            use_plain_text = False
+            # Strip leading and trailing whitespace from each line
+            # to check for base64-encoded data.
+            stripped_license_value = "".join(
+                [line.strip() for line in license_file_value.splitlines()])
             try:
-                license_data = base64.b64decode(license_file_value)
+                if BASE64_ALPHABET_RE.match(stripped_license_value):
+                    license_data = base64.b64decode(stripped_license_value)
+                else:
+                    use_plain_text = True
             except TypeError:
-                raise LicenseDataBase64DecodeError()
+                use_plain_text = True
 
+            if use_plain_text:
+                license_data = license_file_value
         self._host.write_file(
             self._paths.license_file(), license_data,
             owner="landscape", group="root", perms=0o640)

=== modified file 'lib/callbacks/tests/test_filesystem.py'
--- lib/callbacks/tests/test_filesystem.py	2015-06-05 09:47:11 +0000
+++ lib/callbacks/tests/test_filesystem.py	2015-06-15 10:08:40 +0000
@@ -10,7 +10,7 @@
 from lib.tests.rootdir import RootDir
 from lib.callbacks.filesystem import (
     EnsureConfigDir, WriteCustomSSLCertificate, WriteLicenseFile,
-    LicenseFileUnreadableError, LicenseDataBase64DecodeError)
+    LicenseFileUnreadableError)
 
 
 class EnsureConfigDirTest(HookenvTest):
@@ -91,7 +91,7 @@
 
         self.assertEqual([], self.host.calls)
 
-    def test_license_file_data(self):
+    def test_license_file_base64_data(self):
         """
         If the config specifies a license file data directly as
         the base64-encoded value, it is decoded and written
@@ -113,25 +113,76 @@
              {'owner': 'landscape', 'group': 'root', 'perms': 0o640})
         ], self.host.calls)
 
-    def test_license_file_bad_data(self):
-        """
-        When license-file is not a URL and not base64-encoded data, fails
-        with LicenseDataBase64DecodeError.
-        """
-        self.addCleanup(setattr, urllib2, "urlopen", urllib2.urlopen)
-
-        manager = ServiceManager([{
-            "service": "landscape",
-            "required_data": [
-                {"config": {
-                    "license-file": "bad data",
-                }},
-            ],
-        }])
-        with self.assertRaises(LicenseDataBase64DecodeError) as error:
-            self.callback(manager, "landscape", None)
-        self.assertEqual(
-            "Error base64-decoding license-file data.", str(error.exception))
+    def test_license_file_base64_with_whitespaces(self):
+        """
+        If the base64-encoded data is formatted into multiple lines with
+        some leading and trailing whitespace, it is still detected as base64
+        and decoded accordingly.
+        """
+        license_data = base64.b64encode('Test license data')
+        formatted_data = " \t" + license_data[:12] + "\t\n" + license_data[12:]
+        manager = ServiceManager([{
+            "service": "landscape",
+            "required_data": [
+                {"config": {
+                    "license-file": formatted_data,
+                }},
+            ],
+        }])
+        self.callback(manager, "landscape", None)
+
+        self.assertEqual([
+            ("write_file", ('/etc/landscape/license.txt', 'Test license data'),
+             {'owner': 'landscape', 'group': 'root', 'perms': 0o640})
+        ], self.host.calls)
+
+    def test_license_file_bad_base64_padding(self):
+        """
+        When license-file is a base64-decoded string with missing padding,
+        it is written out as plain text string.
+        """
+        self.addCleanup(setattr, urllib2, "urlopen", urllib2.urlopen)
+        # Strip '=\n' from the end of the base64-encoded string.
+        license_data = base64.b64encode("Test license data")[:-2]
+        manager = ServiceManager([{
+            "service": "landscape",
+            "required_data": [
+                {"config": {
+                    "license-file": license_data,
+                }},
+            ],
+        }])
+        self.callback(manager, "landscape", None)
+        self.assertEqual([
+            ("write_file",
+             ('/etc/landscape/license.txt', license_data),
+             {'owner': 'landscape', 'group': 'root', 'perms': 0o640})
+        ], self.host.calls)
+
+    def test_license_file_plain_text(self):
+        """
+        When license-file is neither an URL nor base64-encoded data,
+        it is written out as plain text string.
+        """
+        self.addCleanup(setattr, urllib2, "urlopen", urllib2.urlopen)
+
+        # Whitespaces in the middle of the text indicate it is not
+        # base64-encoded data.
+        license_data = "plain text license data"
+        manager = ServiceManager([{
+            "service": "landscape",
+            "required_data": [
+                {"config": {
+                    "license-file": license_data,
+                }},
+            ],
+        }])
+        self.callback(manager, "landscape", None)
+        self.assertEqual([
+            ("write_file",
+             ('/etc/landscape/license.txt', 'plain text license data'),
+             {'owner': 'landscape', 'group': 'root', 'perms': 0o640})
+        ], self.host.calls)
 
     def test_license_file_file_url(self):
         """
@@ -162,7 +213,7 @@
 
     def test_license_file_http_url(self):
         """
-        If the config specifies a license file using a local file:// URL,
+        If the config specifies a license file using a http:// URL,
         contents of that file are transferred verbatim to the license file
         on the unit.
         """
@@ -191,9 +242,8 @@
 
     def test_license_file_bad_url(self):
         """
-        If the config specifies a license file using a local file:// URL,
-        contents of that file are transferred verbatim to the license file
-        on the unit.
+        If the config specifies a license file using a non-existent http URL,
+        LicenseFileUnreadableError is raised.
         """
         self.addCleanup(setattr, urllib2, "urlopen", urllib2.urlopen)