[FFE] Image Store Proxy must handle compressed images

The source code for 1.0.2 is available at:

http://launchpad.net/image-store-proxy/1.0/1.0.2/+download/image-store-proxy-1.0.2.tar.gz

The relevant code change between 1.0.1 and 1.0.2 is attached. The rest of the diff is documentation and tests.

I've attached the complete debdiff.

Can the user or third party ever control the file name argument? Things like

  + status, output = commands.getstatusoutput("gunzip %s" % localPath)

are never robust, since localPath could contain spaces, or worse, semicolons and other shell commands. That's why Python has an excellent subprocess module, which avoids intermediate shells, and still makes it comfortable to capture status and stdout/err.

The new feature is fine, it's small enough. Howver, I'd appreciate if the gunzip calling could be cleaned up, such as

  gunzip = subprocess.Popen(['gunzip', localPath], stdout=subprocess.PIPE, stderr=subprocess.PIPE)
  (output, output_err) = gunzip.communicate()
  status = gunzip.returncode

No, this is not a security issue. localPath is based on the sha256, which is checked right above for validity.

Scott Moser pointed out that he wants to use .tar.gz on the published images, because using the --sparse option of tar greatly reduces the size of the resulting file in comparison to plain gzip, so I went ahead and added support for .tar.gz on the image store proxy too, in addition to .gz.

This is all in release 1.0.3:

http://launchpad.net/image-store-proxy/1.0/1.0.3/+download/image-store-proxy-1.0.3.tar.gz

I've also added some logic to check that the hash used in the filename is indeed a hash, just for being pedantic. In practice, the image data must be signed by Canonical itself before getting to this point, so it should be safe no matter what.

Oh, again, this was all unittested, and manually tested using the fake store api.

I've added the complete debdiff.

So, I rely on your word that this will only ever see save file names. However, I really advise you to stop using shells and strings for running commands from programs. It's bad style, involves useless shells, is a potential source of bugs and security holes, and is not even less effort than doing it properly.

This bug was fixed in the package image-store-proxy - 1.0.3-0ubuntu1

---------------
image-store-proxy (1.0.3-0ubuntu1) karmic; urgency=low

  * New upstream release (LP: #445714):
    - support gzip and tar+gz compressed images.
  * Fix restart init action to use STARTTIME only if it's set.

 -- Mathias Gug <email address hidden> Thu, 08 Oct 2009 22:31:34 -0400

Martin,

Please rest assured I understand very well all of the consequences of using one approach or the other, and opt to use one or the other depending on specific needs. In fact, if you look through the code you'll see that in some cases subprocess was used.

In the case at hand I could very well have avoided it, but the fact that:

 1) it was safe;
 2) it was a one liner;
 3) It is rarely executed; and
 4) I had very little time to implement the system

Guided me towards using this approach.

Also note that it's easy for people to get Subprocess wrong. We should probably implement a wrapper which implements the equivalent of "getstatusoutput()" in a safe way.

In your example above, you actually got it pretty much right. One detail I'd change on it is that you pipe the error output and the standard output into different file descriptors. This means that if the command run outputs error messages interleaved with normal output, the normal output and the error output will be disjoint, and so will be hard to interpret in a log or in the stdout. The proper way would be to use the option stderr=subprocess.STDOUT, so that the error can be observed at the correct places.

Either way, I agree with you, it's a good practice to use subprocess. Let's have more of it.