From e3526b5173e64a892c000da5a43d683c43b1bfdb Mon Sep 17 00:00:00 2001 From: Federico Capoano Date: Thu, 17 Dec 2015 16:24:14 +0100 Subject: [PATCH 1/3] [OpenWrt] Ensured reliable checksums Removed timestamp information from generated archive to ensure checksum of compressed archives with same content doesn't change. --- docs/source/backends/openwrt.rst | 16 ++++++--- netjsonconfig/backends/openwisp/openwisp.py | 7 ++-- netjsonconfig/backends/openwrt/openwrt.py | 38 +++++++++++++-------- tests/openwrt/test_backend.py | 13 ++++++- 4 files changed, 48 insertions(+), 26 deletions(-) diff --git a/docs/source/backends/openwrt.rst b/docs/source/backends/openwrt.rst index cb3d7a0a6..1233d9dea 100644 --- a/docs/source/backends/openwrt.rst +++ b/docs/source/backends/openwrt.rst @@ -107,15 +107,15 @@ Example: ... } ... ] ... }) - >>> bytes = o.generate() - >>> print(bytes) + >>> stream = o.generate() + >>> print(stream) <_io.BytesIO object at 0x7fd2287fb410> - >>> tar = tarfile.open(fileobj=bytes, mode='r') + >>> tar = tarfile.open(fileobj=stream, mode='r:gz') >>> print(tar.getmembers()) [] As you can see from this example, the ``generate`` method does not write to disk, -but returns an ``io.BytesIO`` object which contains a tar.gz file object with the +but returns an instance of ``io.BytesIO`` which contains a tar.gz file object with the following file structure:: /etc/config/network @@ -125,9 +125,15 @@ directly on the OpenWRT router where it can be finally "restored" with ``sysupg sysupgrade -r -Note that the restore command does not apply the configuration, to do this you have +Note that ``sysupgrade -r`` does not apply the configuration, to do this you have to reload the services manually or reboot the router. +.. note:: + the ``generate`` method intentionally sets the timestamp of the tar.gz archive and its + members to ``0`` in order to facilitate comparing two different archives: setting the + timestamp would infact cause the checksum to be different each time even when contents + of the archive are identical. + Write method ------------ diff --git a/netjsonconfig/backends/openwisp/openwisp.py b/netjsonconfig/backends/openwisp/openwisp.py index f120470c0..a99ac0ea9 100644 --- a/netjsonconfig/backends/openwisp/openwisp.py +++ b/netjsonconfig/backends/openwisp/openwisp.py @@ -1,5 +1,4 @@ import re -import time import tarfile from io import BytesIO @@ -159,7 +158,6 @@ def generate(self): if '' in packages: packages.remove('') # for each package create a file with its contents in /etc/config - timestamp = time.time() for package in packages: lines = package.split('\n') package_name = lines[0] @@ -167,8 +165,7 @@ def generate(self): text_contents = 'package {0}\n\n{1}'.format(package_name, text_contents) self._add_file(tar=tar, name='uci/{0}.conf'.format(package_name), - contents=text_contents, - timestamp=timestamp) + contents=text_contents) # prepare template context for install and uninstall scripts template_context = self._get_install_context() # add install.sh to included files @@ -180,7 +177,7 @@ def generate(self): # add tc_script self._add_tc_script() # add files resulting archive - self._add_files(tar, timestamp) + self._add_files(tar) # close archive tar.close() byte_object.seek(0) diff --git a/netjsonconfig/backends/openwrt/openwrt.py b/netjsonconfig/backends/openwrt/openwrt.py index 139483ca1..249d71e9e 100644 --- a/netjsonconfig/backends/openwrt/openwrt.py +++ b/netjsonconfig/backends/openwrt/openwrt.py @@ -1,7 +1,7 @@ import json import re import six -import time +import gzip import tarfile from io import BytesIO from copy import deepcopy @@ -145,8 +145,8 @@ def get_packages(cls): def generate(self): """ - Returns a ``BytesIO`` object representing a tar.gz archive with the - generated final router configuration. + Returns a ``BytesIO`` instance representing an in-memory tar.gz archive + containing the native router configuration. The archive can be installed in OpenWRT with the following command: @@ -155,26 +155,33 @@ def generate(self): :returns: in-memory tar.gz archive, instance of ``BytesIO`` """ uci = self.render(files=False) - byte_object = BytesIO() - tar = tarfile.open(fileobj=byte_object, mode='w:gz') + tar_bytes = BytesIO() + tar = tarfile.open(fileobj=tar_bytes, mode='w') # create a list with all the packages (and remove empty entries) packages = re.split('package ', uci) if '' in packages: packages.remove('') # for each package create a file with its contents in /etc/config - timestamp = time.time() for package in packages: lines = package.split('\n') package_name = lines[0] text_contents = '\n'.join(lines[2:]) self._add_file(tar=tar, name='etc/config/{0}'.format(package_name), - contents=text_contents, - timestamp=timestamp) - self._add_files(tar, timestamp) + contents=text_contents) + self._add_files(tar) tar.close() - byte_object.seek(0) - return byte_object + tar_bytes.seek(0) # set pointer to beginning of stream + # `mtime` parameter of gzip file must be 0, otherwise any checksum operation + # would return a different digest even when content is the same. + # to achieve this we must use the python `gzip` library because the `tarfile` + # library does not seem to offer the possibility to modify the gzip `mtime`. + gzip_bytes = BytesIO() + gz = gzip.GzipFile(fileobj=gzip_bytes, mode='wb', mtime=0) + gz.write(tar_bytes.getvalue()) + gz.close() + gzip_bytes.seek(0) # set pointer to beginning of stream + return gzip_bytes def write(self, name, path='./'): """ @@ -192,7 +199,7 @@ def write(self, name, path='./'): f.write(byte_object.getvalue()) f.close() - def _add_files(self, tar, timestamp): + def _add_files(self, tar): """ adds files specified in self.config['files'] in specified tar object @@ -210,17 +217,18 @@ def _add_files(self, tar, timestamp): self._add_file(tar=tar, name=path, contents=contents, - timestamp=timestamp, mode=file_item.get('mode', DEFAULT_FILE_MODE)) - def _add_file(self, tar, name, contents, timestamp, mode=DEFAULT_FILE_MODE): + def _add_file(self, tar, name, contents, mode=DEFAULT_FILE_MODE): """ adds a single file in tar object """ byte_contents = BytesIO(contents.encode('utf8')) info = tarfile.TarInfo(name=name) info.size = len(contents) - info.mtime = timestamp + # mtime must be 0 or any checksum operation + # will return a different digest even when content is the same + info.mtime = 0 info.type = tarfile.REGTYPE info.mode = int(mode, 8) # permissions converted to decimal notation tar.addfile(tarinfo=info, fileobj=byte_contents) diff --git a/tests/openwrt/test_backend.py b/tests/openwrt/test_backend.py index ad41ee0d0..a5e51bec9 100644 --- a/tests/openwrt/test_backend.py +++ b/tests/openwrt/test_backend.py @@ -3,6 +3,8 @@ import unittest import tarfile from io import BytesIO +from time import sleep +from hashlib import md5 from netjsonconfig import OpenWrt from netjsonconfig.exceptions import ValidationError @@ -303,7 +305,7 @@ def test_file_inclusion(self): crontab = tar.getmember('etc/crontabs/root') contents = tar.extractfile(crontab).read().decode() self.assertEqual(contents, '\n'.join(o.config['files'][0]['contents'])) - self.assertNotEqual(crontab.mtime, 0) + self.assertEqual(crontab.mtime, 0) self.assertEqual(crontab.mode, 420) # second file dummy = tar.getmember('etc/dummy.conf') @@ -348,3 +350,12 @@ def test_file_permissions(self): # check permissions self.assertEqual(script.mode, 493) tar.close() + + def test_checksum(self): + """ ensures checksum of same config doesn't change """ + o = OpenWrt({"general": {"hostname": "test"}}) + # md5 is good enough and won't slow down test execution too much + checksum1 = md5(o.generate().getvalue()).hexdigest() + sleep(1) + checksum2 = md5(o.generate().getvalue()).hexdigest() + self.assertEqual(checksum1, checksum2) From 3624dcda6ea717aa8b575093db7e99d2c75b0671 Mon Sep 17 00:00:00 2001 From: Federico Capoano Date: Thu, 17 Dec 2015 17:44:23 +0100 Subject: [PATCH 2/3] [OpenWisp] Ensured reliable checksums --- netjsonconfig/backends/openwisp/openwisp.py | 14 ++++++++++---- tests/openwisp/test_backend.py | 11 +++++++++++ 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/netjsonconfig/backends/openwisp/openwisp.py b/netjsonconfig/backends/openwisp/openwisp.py index a99ac0ea9..4069674a2 100644 --- a/netjsonconfig/backends/openwisp/openwisp.py +++ b/netjsonconfig/backends/openwisp/openwisp.py @@ -1,4 +1,5 @@ import re +import gzip import tarfile from io import BytesIO @@ -151,8 +152,8 @@ def generate(self): :returns: in-memory tar.gz archive, instance of ``BytesIO`` """ uci = self.render(files=False) - byte_object = BytesIO() - tar = tarfile.open(fileobj=byte_object, mode='w:gz') + tar_bytes = BytesIO() + tar = tarfile.open(fileobj=tar_bytes, mode='w') # create a list with all the packages (and remove empty entries) packages = re.split('package ', uci) if '' in packages: @@ -180,5 +181,10 @@ def generate(self): self._add_files(tar) # close archive tar.close() - byte_object.seek(0) - return byte_object + tar_bytes.seek(0) + gzip_bytes = BytesIO() + gz = gzip.GzipFile(fileobj=gzip_bytes, mode='wb', mtime=0) + gz.write(tar_bytes.getvalue()) + gz.close() + gzip_bytes.seek(0) # set pointer to beginning of stream + return gzip_bytes diff --git a/tests/openwisp/test_backend.py b/tests/openwisp/test_backend.py index 545d5079f..d853da586 100644 --- a/tests/openwisp/test_backend.py +++ b/tests/openwisp/test_backend.py @@ -3,6 +3,8 @@ import tarfile from copy import deepcopy from io import BytesIO +from time import sleep +from hashlib import md5 from netjsonconfig import OpenWisp from netjsonconfig.exceptions import ValidationError @@ -235,3 +237,12 @@ def test_cron(self): contents = tar.extractfile(uninstall).read().decode() self.assertIn('Stopping Cron', contents) tar.close() + + def test_checksum(self): + """ ensures checksum of same config doesn't change """ + o = OpenWisp({"general": {"hostname": "test"}}) + # md5 is good enough and won't slow down test execution too much + checksum1 = md5(o.generate().getvalue()).hexdigest() + sleep(1) + checksum2 = md5(o.generate().getvalue()).hexdigest() + self.assertEqual(checksum1, checksum2) From a0b1373f40144891a9328bf5d1088adbe88585f4 Mon Sep 17 00:00:00 2001 From: Federico Capoano Date: Thu, 17 Dec 2015 18:40:28 +0100 Subject: [PATCH 3/3] Moved generate method details to _generate_contents Different backends can customize the generated content with this method to avoid duplication of code. --- netjsonconfig/backends/openwisp/openwisp.py | 25 ++++------ netjsonconfig/backends/openwrt/openwrt.py | 53 ++++++++++++++------- 2 files changed, 44 insertions(+), 34 deletions(-) diff --git a/netjsonconfig/backends/openwisp/openwisp.py b/netjsonconfig/backends/openwisp/openwisp.py index 4069674a2..87d1ddf1b 100644 --- a/netjsonconfig/backends/openwisp/openwisp.py +++ b/netjsonconfig/backends/openwisp/openwisp.py @@ -1,7 +1,4 @@ import re -import gzip -import tarfile -from io import BytesIO from jinja2 import Environment, PackageLoader @@ -151,9 +148,16 @@ def generate(self): :returns: in-memory tar.gz archive, instance of ``BytesIO`` """ + return super(OpenWisp, self).generate() + + def _generate_contents(self, tar): + """ + Adds configuration files to tarfile instance. + + :param tar: tarfile instance + :returns: None + """ uci = self.render(files=False) - tar_bytes = BytesIO() - tar = tarfile.open(fileobj=tar_bytes, mode='w') # create a list with all the packages (and remove empty entries) packages = re.split('package ', uci) if '' in packages: @@ -177,14 +181,3 @@ def generate(self): self._add_openvpn_scripts() # add tc_script self._add_tc_script() - # add files resulting archive - self._add_files(tar) - # close archive - tar.close() - tar_bytes.seek(0) - gzip_bytes = BytesIO() - gz = gzip.GzipFile(fileobj=gzip_bytes, mode='wb', mtime=0) - gz.write(tar_bytes.getvalue()) - gz.close() - gzip_bytes.seek(0) # set pointer to beginning of stream - return gzip_bytes diff --git a/netjsonconfig/backends/openwrt/openwrt.py b/netjsonconfig/backends/openwrt/openwrt.py index 249d71e9e..c2adca207 100644 --- a/netjsonconfig/backends/openwrt/openwrt.py +++ b/netjsonconfig/backends/openwrt/openwrt.py @@ -154,22 +154,10 @@ def generate(self): :returns: in-memory tar.gz archive, instance of ``BytesIO`` """ - uci = self.render(files=False) tar_bytes = BytesIO() tar = tarfile.open(fileobj=tar_bytes, mode='w') - # create a list with all the packages (and remove empty entries) - packages = re.split('package ', uci) - if '' in packages: - packages.remove('') - # for each package create a file with its contents in /etc/config - for package in packages: - lines = package.split('\n') - package_name = lines[0] - text_contents = '\n'.join(lines[2:]) - self._add_file(tar=tar, - name='etc/config/{0}'.format(package_name), - contents=text_contents) - self._add_files(tar) + self._generate_contents(tar) + self._process_files(tar) tar.close() tar_bytes.seek(0) # set pointer to beginning of stream # `mtime` parameter of gzip file must be 0, otherwise any checksum operation @@ -183,6 +171,27 @@ def generate(self): gzip_bytes.seek(0) # set pointer to beginning of stream return gzip_bytes + def _generate_contents(self, tar): + """ + Adds configuration files to tarfile instance. + + :param tar: tarfile instance + :returns: None + """ + uci = self.render(files=False) + # create a list with all the packages (and remove empty entries) + packages = re.split('package ', uci) + if '' in packages: + packages.remove('') + # for each package create a file with its contents in /etc/config + for package in packages: + lines = package.split('\n') + package_name = lines[0] + text_contents = '\n'.join(lines[2:]) + self._add_file(tar=tar, + name='etc/config/{0}'.format(package_name), + contents=text_contents) + def write(self, name, path='./'): """ Like ``generate`` but writes to disk. @@ -199,10 +208,12 @@ def write(self, name, path='./'): f.write(byte_object.getvalue()) f.close() - def _add_files(self, tar): + def _process_files(self, tar): """ - adds files specified in self.config['files'] - in specified tar object + Adds files specified in self.config['files'] to tarfile instance. + + :param tar: tarfile instance + :returns: None """ # insert additional files for file_item in self.config.get('files', []): @@ -221,7 +232,13 @@ def _add_files(self, tar): def _add_file(self, tar, name, contents, mode=DEFAULT_FILE_MODE): """ - adds a single file in tar object + Adds a single file in tarfile instance. + + :param tar: tarfile instance + :param name: string representing filename or path + :param contents: string representing file contents + :param mode: string representing file mode, defaults to 644 + :returns: None """ byte_contents = BytesIO(contents.encode('utf8')) info = tarfile.TarInfo(name=name)