download, downloadunpacked: Check shared directory is not enough. Shared...

download, downloadunpacked: Check shared directory is not enough. Shared directory may be created before completing installation. Check signature file too.

slapos/recipe/download.py

@@ -30,7 +30,7 @@ import shutil
 import zc.buildout
 import logging
 from hashlib import md5
-from downloadunpacked import make_read_only_recursively
+from downloadunpacked import make_read_only_recursively, Signature
 
 class Recipe(object):
   _parts = None
@@ -44,7 +44,8 @@ class Recipe(object):
     self._name = name
     mode = options.get('mode')
     log = logging.getLogger(name)
-    self._shared = shared = (options.get('shared', '').lower() == 'true')
+    self._shared = shared = ((options.get('shared', '').lower() == 'true') and
+                             buildout['buildout'].get('shared-parts', None))
     if mode is not None:
       mode = int(mode, 8)
     self._mode = mode
@@ -54,21 +55,18 @@ class Recipe(object):
 
     destination = options.get('destination', None)
     if destination is None:
-      if shared and buildout['buildout'].get('shared-parts', None):
+      if shared:
         shared_part = buildout['buildout'].get('shared-parts', None)
         shared = os.path.join(shared_part.strip().rstrip('/'), name)
         if not os.path.exists(shared):
           os.makedirs(shared)
-        self._debug_signature_text = []
-        m = md5()
+        self._signature = Signature('.slapos.recipe.build.signature')
         profile_base_location = options.get('_profile_base_location_', '')
         for k, v in sorted(options.items()):
           if profile_base_location:
             v = v.replace(profile_base_location, '${:_profile_base_location_}')
-          option_signature = ('%r: %r' % (k, v)).encode()
-          self._debug_signature_text.append(option_signature)
-          m.update(option_signature)
-        shared = os.path.join(shared, m.hexdigest())
+          self._signature.update(k, v)
+        shared = os.path.join(shared, self._signature.hexdigest())
         self._parts = parts = shared
         log.info('shared directory  %s set for %s', shared, name)
       else:
@@ -87,7 +85,7 @@ class Recipe(object):
     log = logging.getLogger(self._name)
     if self._shared:
       log.info('Checking whether package is installed at shared path: %s', destination)
-      if os.path.exists(destination):
+      if self._signature.test(self._parts):
         log.info('This shared package has been installed by other package')
         return []
 
@@ -109,8 +107,7 @@ class Recipe(object):
         shutil.copyfileobj(fsrc, fdst)
 
     if self._shared:
-      with open(os.path.join(parts, ".slapos.recipe.build.signature"), 'w') as f:
-        f.write('\n'.join(self._debug_signature_text))
+      self._signature.save(parts)
       make_read_only_recursively(self._parts)
     return result
 

slapos/recipe/downloadunpacked.py

@@ -49,24 +49,22 @@ class Recipe:
           'exclusive.')
     self.parts = None
     self.destination = self.options.get('destination', None)
-    self.shared = shared = is_true(options.get('shared', 'false').lower())
+    self.shared = shared = (is_true(options.get('shared', 'false').lower()) and
+                            buildout['buildout'].get('shared-parts', None))
     if self.destination is None:
-      if shared and buildout['buildout'].get('shared-parts', None):
+      if shared:
         shared_part = buildout['buildout'].get('shared-parts', None)
         top_location = options.get('top_location', '')
         shared = os.path.join(shared_part.strip().rstrip('/'), top_location, name)
         if not os.path.exists(shared):
           os.makedirs(shared)
-        self._debug_signature_text = []
-        m = md5()
+        self._signature = Signature('.slapos.recipe.build.signature')
         profile_base_location = options.get('_profile_base_location_', '')
         for k, v in sorted(options.items()):
           if profile_base_location:
             v = v.replace(profile_base_location, '${:_profile_base_location_}')
-          option_signature = ('%r: %r' % (k, v)).encode()
-          self._debug_signature_text.append(option_signature)
-          m.update(option_signature)
-        shared = os.path.join(shared, m.hexdigest())
+          self._signature.update(k, v)
+        shared = os.path.join(shared, self._signature.hexdigest())
         self.parts = shared
         self.logger.info('shared directory  %s set for %s', shared, name)
       else:
@@ -99,7 +97,7 @@ class Recipe:
   def install(self):
     if self.shared:
       self.logger.info('Checking whether package is installed at shared path : %s', self.destination)
-      if os.path.exists(self.destination):
+      if self._signature.test(self.destination):
         self.logger.info('This shared package has been installed by other package')
         return []
     if self.parts is not None:
@@ -160,8 +158,7 @@ class Recipe:
       self.options['url'], self.destination)
 
     if self.shared:
-      with open(os.path.join(self.parts, ".slapos.recipe.build.signature"), 'w') as f:
-        f.write('\n'.join(self._debug_signature_text))
+      self._signature.save(self.parts)
       make_read_only_recursively(self.parts)
       return []
     if self.parts is not None:
@@ -236,3 +233,33 @@ def make_read_only_recursively(path):
       make_read_only(os.path.join(root, dir))
     for file_ in file_list:
       make_read_only(os.path.join(root, file_))
+
+class Signature:
+
+  def __init__(self, filename):
+    self.filename = filename
+    self.item_list = []
+
+  def update(self, key, value):
+    self.item_list.append(('%r: %r' % (key, value)).encode())
+
+  def hexdigest(self):
+    m = md5()
+    for item in self.item_list:
+      m.update(item)
+    return m.hexdigest()
+
+  def dumps(self):
+    return '\n'.join(self.item_list)
+
+  def test(self, folder_path):
+    digest = self.hexdigest()
+    if os.path.basename(folder_path) == digest:
+      target_path = os.path.join(folder_path, self.filename)
+      if os.path.exists(target_path) and open(target_path).read() == self.dumps():
+        return True
+    return False
+
+  def save(self, folder_path):
+    with open(os.path.join(folder_path, self.filename), 'w') as f:
+      f.write(self.dumps())

[Julien Muchembled]

I find strange to have a separator between the key and the value but not between items (when used in hexdigest).

Can you clarify, to make sure we have appropriate separators, to avoid accidental collisions and make the code less suspicious ?

[Yusei Tahara]

We can break compatibility so far, I think '%r: %r' should be '%r:%r'(this is for to make human readable text) and hexdigest should use a result of dumps(). Also we should stop using md5. base64 of sha256 would be good. But then, when we debug on file system, md5 is more convenient than base64 of sha256.

[Julien Muchembled]

hexdigest should use a result of dumps()

Yes, dumps() does add separators between items, and it looks useful to have something consistent.

we should stop using md5

Why ? If you say that base64 is fine, then I guess it does not need to be secure, then md5 is fine.

BTW, I don't know if keys or values can be bytes, but we must keep in mind that %r does not render them the same way on Python 2 and 3. In particular, bytes are actually far from being readable on Python 3. Knowing the data is important to make a good choice.

There also lack a unit test covering the test() method.

[Yusei Tahara]

To use md5 in this context is fine, but if it is used for a password hash, it is not good. I did not want to use md5 here, because I did not want someone copy and paste this code for something to be secure. About bytes, I did not know that it is far from being readable on python 3, I will check what is the best in this context. And I will add a test for test() method later.