Merge lp:~chris-gondolin/charms/trusty/storage/trunk into lp:charms/storage

[Chris Stratford]

Added encrypted filesystem support for block devices.
Introduces two new config variables:

encryption_key_map = "{postgres/0: password, postgres/2: password2}"
store_encryption_keys = yes

If store_encryption_keys is "yes" the passwords will be stored in /root/keyfile--dev-vdc (or vdd, vde, etc. whatever the device name is) and an entry written to /etc/crypttab to allow auto-mounting on reboot.

If store_encryption_keys is anything else, the keyfile will be written temporarily to allow cryptsetup to do its work, then removed, any entries for the device in /etc/crypttab will be removed if present and "defaults,noauto" set in fstab to ensure prevent auto-mounting on reboot (which would fail, waiting for a passphrase).

[Tim Van Steenburgh (tvansteenburgh)]

Hey Chris, nice addition to the storage charm, thanks!

My only quibble with this proposal is that there are no new tests to cover the new functionality. Before giving this a +1 I would like to see some tests to cover the new encryption stuff - maybe some unit test coverage of the crypt_fs module, and a new amulet test to test a deployment scenario.

[lp:~chris-gondolin/charms/trusty/storage/trunk]

41. By Chris Stratford on 2014-10-07

[chriss] Added a partition provider to cope with physical disk partitions (on MaaS units for example). Added some amulet tests that should cover both the new provider and encryption.

42. By Chris Stratford on 2014-10-08

[chriss] A bit more logging

43. By Chris Stratford on 2014-10-08

[chriss] Attempt to get juju test to log useful stuff

44. By Chris Stratford on 2014-10-08

[chriss] Removed spin_up_delay, as it did not do what I had hoped. Tests *should* work once bug #1378309 is fixed

[Cory Johns (johnsca)]

Chris,

Thanks for adding the tests for this functionality. The new tests look good, but unfortunately I had some issues running them.

First, I'd just like to point out that Amulet has been updated to fix the issues you are working around and mention in your tests, specifically the issues with the series and local charm not being used properly, and adding relations after deployment not working correctly. So, you can change your d.add('postgresql', 'cs:trusty/postgresql-5') and d.add('storage', '/home/chris/work/charms/trusty/storage') to just d.add('postgresql') and d.add('storage'). Regarding the post-deployment relations, the only thing that you should need to change is to add d.sentry.wait() after adding the relation, to ensure that the system has time to process the change.

That said, with those changes I am still getting failures (http://pastebin.ubuntu.com/8838368/), as well as hook failures (http://pastebin.ubuntu.com/8838352/). This happened on both lxc and aws, on newly bootstrapped environments.

Additionally, `make lint` had some complaints about the new code, just some small quibbles about line length and spacing: http://pastebin.ubuntu.com/8837062/

Finally, some of the existing tests in `make test` are having issues with the new code. It mostly seems like they need some additional mocks (http://pastebin.ubuntu.com/8837040/) or new data values (http://pastebin.ubuntu.com/8837008/), but one in particular seems like it might be an issue in the new code: http://pastebin.ubuntu.com/8837016/

review: Needs Fixing

[lp:~chris-gondolin/charms/trusty/storage/trunk]

45. By Chris Stratford on 2014-11-06

[chriss] PEP8 fixes. Amulet test fixes now amulet itself is fixed

[Chris Stratford (chris-gondolin)]

The Amulet tests should now work (d.sentry.wait() after adding the relation doesn't appear to be enough, so there's a sleep in there too now, which should fix the errors you saw)

make lint should also be happy now.

I've got to do some reading up on mocker before fixing the make test problems.

[lp:~chris-gondolin/charms/trusty/storage/trunk]

46. By Chris Stratford on 2014-11-06

[chriss] Unit test fixes

[Chris Stratford (chris-gondolin)]

Ok, the unit tests now pass (even the ones that weren't a result of my changes :-)

There still aren't any for the crypt_fs functions or partition storage type additions (yet).

[lp:~chris-gondolin/charms/trusty/storage/trunk]

47. By Chris Stratford on 2014-11-07

[chriss] Add unit tests for crypt_fs. Minor fixes.

[Matt Bruzek (mbruzek)]

+1 LGTM

review: Approve

[Adam Collard (adam-collard)]

This broke the charm :(

http://paste.ubuntu.com/9458517/

There is nothing in the charm that attempts to install cryptsetup-bin package (which installs the cryptsetup script that this MP relies on).

[Chris Stratford (chris-gondolin)]

Adam, I'm curious to know what system you installed it on to get that error, as it looks like cryptsetup-bin is standard on all recent Ubuntu installs.

I'll fix the bug, but it would be good to know for future reference.

[David Britton (dpb)]

Hi @Chris --

Adam tested on openstack with official cloud images. Matt tested on lxc. Could be a difference between the images there. :(

=== modified file 'config.yaml'
--- config.yaml	2014-02-10 21:51:04 +0000
+++ config.yaml	2014-11-07 12:56:29 +0000
@@ -8,21 +8,36 @@
         default: local
         description: |
             The backend storage provider.  Will be mounted at root, and
-            can be one of, local, block-storage-broker or nfs.  If you set to
-            block-storage-broker, you should provide a suitable value for
-            volume_size.
+            can be one of, local, block-storage-broker, nfs or partition.
+            If you set to block-storage-broker, you should provide a suitable
+            value for volume_size.
     volume_map:
         type: string
         default: ""
         description: |
           YAML map as e.g. "{postgres/0: vol-0000010, postgres/1: vol-0000016}".
-          A service unit to specific block storage volume-id that should be
-          attached to each unit. This requires that provider is set to
-          block-storage-broker and that volume-ids specified match a listing
-          from nova volume-list. If a related unit does not have a volume-id
-          specified, a new volume of volume_size will be created for that unit.
+          A service unit to specific volume-id that should be attached to each
+          unit.  If the provider is set to block-storage-broker, volume-ids
+          specified should match a listing from nova volume-list.
+          If a related unit does not have a volume-id specified, a new volume
+          of volume_size will be created for that unit.
+          If the provider is set to partition, volume-ids should be a disk
+          partition name (e.g. /dev/sdb1)
     volume_size:
         type: int
         description: |
             The volume size in GB to request from the block-storage-broker.
         default: 5
+    encryption_key_map:
+        type: string
+        default: ""
+        description: |
+            YAML map as e.g. "{postgres/0: password, postgres/1: password2}"
+            If set, the unit's volume will be encrypted with the matching key
+            (unless the volume is already formatted)
+    store_encryption_keys:
+        type: string
+        default: "yes"
+        description: |
+            If the volume is encrypted, this will write the details to
+            disk to allow auto-mounting.
 
=== added directory 'files'
=== added file 'files/crypt_mount.sh'
--- files/crypt_mount.sh	1970-01-01 00:00:00 +0000
+++ files/crypt_mount.sh	2014-11-07 12:56:29 +0000
@@ -0,0 +1,19 @@
+#!/bin/bash
+#
+# Simple scrip to assist with mounting a crypted filesystem
+# Assumes an fstab entry already exists
+
+scriptname=$(basename $0)
+dev=$1
+
+set -eu
+
+if [ "${dev}" = "" ]; then
+    echo "Usage: ${scriptname} <device>"
+    echo "e.g. ${scriptname} vdc"
+    exit 1
+fi
+
+cryptsetup open --type luks /dev/${dev} ${dev}
+mount /dev/mapper/${dev}
+exit 0
 
=== modified file 'hooks/common_util.py'
--- hooks/common_util.py	2014-08-27 11:01:14 +0000
+++ hooks/common_util.py	2014-11-07 12:56:29 +0000
@@ -6,6 +6,8 @@
 from time import sleep
 from stat import S_ISBLK, ST_MODE
 import json
+import crypt_fs
+
 
 FSTAB_FILE = "/etc/fstab"
 EC2_METADATA_URL = "http://169.254.169.254/latest/meta-data"
@@ -167,38 +169,81 @@
 
 def _read_partition_table(device_path):
     """Call blockdev to read a valid partition table and return exit code"""
+    # blockdev doesn't like loopback, so we lie
+    if device_path.startswith("/dev/loop"):
+        return 0
     return subprocess.call(
         "blockdev --rereadpt %s" % device_path, shell=True)
 
 
 def _assert_fstype(device_path, fstype):
     """Return C{True} if a filesystem is of type C{fstype}"""
-    command = "file -s %s | egrep -q %s" % (device_path, fstype)
+    # /dev/mapper/blah is usually a symlink
+    real_device_path = os.path.realpath(device_path)
+    command = "file -s %s | egrep -q %s" % (real_device_path, fstype)
     return subprocess.call(command, shell=True) == 0
 
 
+def _get_mkfs_command(device_path, encryption):
+    mkfs_command = None
+    encrypted_dev_path = None
+    if encryption:
+        if crypt_fs.is_encrypted(device_path):
+            encrypted_dev_path = crypt_fs.get_encrypted_dev_path(device_path)
+            log("Found an encrypted device {}".format(encrypted_dev_path))
+        else:
+            encrypted_dev_path = crypt_fs.encrypt_device(device_path)
+        mkfs_command = ["mkfs.ext4", encrypted_dev_path]
+        crypt_fs.write_encryption_details(device_path)
+
+        if _assert_fstype(encrypted_dev_path, "ext4"):
+            log("{} already encrypted and formatted - "
+                "skipping mkfs.ext4.".format(device_path))
+            mkfs_command = None
+    else:
+        mkfs_command = ["mkfs.ext4", device_path]
+    return mkfs_command, encrypted_dev_path
+
+
 def _format_device(device_path):
     """
     Create an ext4 partition, if needed, for C{device_path} and fsck that
     partition.
     """
+    encryption = True if crypt_fs.get_encryption_key() else False
+    command = []
+
     result = _read_partition_table(device_path)
     if result == 1:
         log("INFO: %s is busy, no fsck performed. Assuming formatted." %
             device_path)
+        encryption = False
     elif result == 0:
         # Create an ext4 filesystem if NOT already present
         # use e.g. LABEl=vol-000012345
         if _assert_fstype(device_path, "ext4"):
             log("%s already formatted - skipping mkfs.ext4." % device_path)
-        else:
-            command = "mkfs.ext4 %s" % device_path
-            log("NOTICE: Running: %s" % command)
-            subprocess.check_call(command, shell=True)
-        if subprocess.call("fsck -p %s" % device_path, shell=True) != 0:
+            encryption = False
+        else:
+            mkfs_command, encrypted_dev_path = _get_mkfs_command(device_path,
+                                                                 encryption)
+            if mkfs_command:
+                log("NOTICE: Running: %s" % " ".join(mkfs_command))
+                subprocess.check_call(mkfs_command, shell=False)
+
+        if encryption:
+            command = ["fsck", "-p", encrypted_dev_path]
+        else:
+            command = ["fsck", "-p", device_path]
+        if subprocess.call(command, shell=False) != 0:
             log("ERROR: fsck -p %s failed" % device_path)
             sys.exit(1)
 
+    if encryption:
+        return encrypted_dev_path
+    else:
+        return device_path
+
 
 def _get_from_relation(relation, key, default=None):
     relids = hookenv.relation_ids(relation)
@@ -213,8 +258,8 @@
 
 def mount_volume(device_path=None):
     """
-    Mount the attached volume, nfs or block-storage-broker type, to the
-    principal requested C{mountpoint}.
+    Mount the attached volume, partition, nfs or block-storage-broker
+    type, to the principal requested C{mountpoint}.
 
     If the C{mountpoint} required relation data does not exist, log the
     issue and exit. Otherwise attempt to initialize and mount the blockstorage
@@ -255,13 +300,16 @@
                 "nfs-relation-changed hook needs to run first.")
             return
 
-        device_path = "%s:%s" % (nfs_server, nfs_path)
-    elif provider == "block-storage-broker":
-        if device_path is None:
-            log("Waiting for persistent nova device provided by "
-                "block-storage-broker.")
-            sys.exit(0)
+        new_device_path = "%s:%s" % (nfs_server, nfs_path)
+    elif provider in ("block-storage-broker", "partition"):
+        # Specific to block-storage
+        if provider == "block-storage-broker":
+            if device_path is None:
+                log("Waiting for persistent nova device provided by "
+                    "block-storage-broker.")
+                sys.exit(0)
 
+        # Common to any partition-like device
         _assert_block_device(device_path)
         if os.path.exists("%s1" % device_path):
             # Then we are supporting upgrade-charm path for deprecated
@@ -271,10 +319,14 @@
             device_path = "%s1" % device_path
             log("Mounting %s as the device path as the root device is already "
                 "partitioned." % device_path)
-        _format_device(device_path)
-        _set_label(device_path, mountpoint)
+        # _format_device may create a new device_path if it's encrypted
+        new_device_path = _format_device(device_path)
+        _set_label(new_device_path, mountpoint)
         fstype = subprocess.check_output(
-            "blkid -o value -s TYPE %s" % device_path, shell=True).strip()
+            "blkid -o value -s TYPE %s" % new_device_path, shell=True).strip()
+        if crypt_fs.is_encrypted(device_path):
+            if hookenv.config("store_encryption_keys") != "yes":
+                options = "defaults,noauto"
     else:
         log("ERROR: Unknown provider %s. Cannot mount volume." % hookenv.ERROR)
         sys.exit(1)
@@ -284,12 +336,12 @@
 
     options_string = "" if options == "defaults" else " -o %s" % options
     command = "mount -t %s%s %s %s" % (
-        fstype, options_string, device_path, mountpoint)
+        fstype, options_string, new_device_path, mountpoint)
     subprocess.check_call(command, shell=True)
     log("Mount (%s) successful" % mountpoint)
     with open(FSTAB_FILE, "a") as output_file:
-        output_file.write(
-            "%s %s %s %s 0 0\n" % (device_path, mountpoint, fstype, options))
+        output_file.write("{} {} {} {} 0 0\n".format(
+                          new_device_path, mountpoint, fstype, options))
     # publish changes to data relation and exit
     for relid in hookenv.relation_ids("data"):
         hookenv.relation_set(
 
=== added file 'hooks/crypt_fs.py'
--- hooks/crypt_fs.py	1970-01-01 00:00:00 +0000
+++ hooks/crypt_fs.py	2014-11-07 12:56:29 +0000
@@ -0,0 +1,164 @@
+"""Common python utilities for storage providers"""
+from charmhelpers.core import hookenv
+import os
+import subprocess
+import sys
+import yaml
+from yaml.constructor import ConstructorError
+import shutil
+
+
+CRYPTTAB_FILE = "/etc/crypttab"
+KEYFILE_DIR = "/root"
+CRYPT_MOUNT_SCRIPT = "/usr/local/sbin/crypt_mount.sh"
+
+
+def log(message, level=None):
+    """Quaint little log wrapper for juju logging"""
+    hookenv.log(message, level)
+
+
+# Format a Luks device
+def encrypt_device(device_path):
+    keyfile = _write_encryption_keyfile(device_path)
+    # Create the encrypted volume
+    command = ["cryptsetup",
+               "--key-file", keyfile,
+               "--batch-mode",
+               "luksFormat", device_path]
+    log("NOTICE: Running: {}".format(command))
+    if subprocess.call(command, shell=False) != 0:
+        log("ERROR: {} failed".format(command))
+        sys.exit(1)
+    return get_encrypted_dev_path(device_path)
+
+
+# Return the Luks device path if it exists
+# Perform a LuksOpen if it doesn't
+def get_encrypted_dev_path(device_path):
+    keyfile = _write_encryption_keyfile(device_path)
+    newname = "/dev/mapper/{}".format(os.path.basename(device_path))
+    if os.path.exists(newname):
+        return newname
+    else:
+        basedev = os.path.basename(device_path)
+        command = ["cryptsetup",
+                   "--key-file", keyfile,
+                   "open",
+                   "--type", "luks",
+                   device_path,
+                   basedev]
+        log("NOTICE: Running: {}".format(command))
+        if subprocess.call(command, shell=False) != 0:
+            log("ERROR: {} failed".format(command))
+            sys.exit(1)
+        return newname
+
+
+def is_encrypted(device_path):
+    command = ["cryptsetup", "isLuks", device_path]
+    if subprocess.call(command, shell=False) == 0:
+        return True
+    else:
+        return False
+
+
+def _get_encryption_uuid(device_path):
+    command = ["cryptsetup", "luksUUID", device_path]
+    try:
+        uuid = subprocess.check_output(command, shell=False).rstrip()
+        return uuid
+    except subprocess.CalledProcessError:
+        log("ERROR: {} failed".format(command))
+        return None
+
+
+def get_encryption_key():
+    # Get first unit of first data relation
+    # (hopefully only unit of only data relation)
+    relid = hookenv.relation_ids("data")[0]
+    unit = hookenv.related_units(relid)[0]
+    log("get_encryption_key() unit: {}".format(unit))
+    encryption_key_map = hookenv.config("encryption_key_map")
+    encryption_key = None
+    if encryption_key_map and unit:
+        log("get_encryption_key() Have map and unit")
+        try:
+            encryption_key_map = yaml.load(encryption_key_map)
+            if encryption_key_map:
+                encryption_key = encryption_key_map.get(unit, None)
+        except ConstructorError as e:
+            log("Invalid YAML in 'encryption_key_map': {}".format(e),
+                hookenv.WARNING)
+            return None
+    if not encryption_key:
+        log("get_encryption_key() Failed to find key")
+    return encryption_key
+
+
+def _get_encryption_keyfile_name(device_path):
+    return "{}/keyfile-{}".format(KEYFILE_DIR, device_path.replace("/", "-"))
+
+
+# Store the encryption key, as it's needed by the Luks commands
+# We will delete it later if the store_encryption_keys config variable
+# is not "yes"
+def _write_encryption_keyfile(device_path):
+    encryption_key = get_encryption_key()
+    keyfile = _get_encryption_keyfile_name(device_path)
+    # Write out the keyfile
+    log("Writing keyfile: {}".format(keyfile))
+    with open(keyfile, "w") as f:
+        os.chmod(keyfile, 0600)
+        f.write(encryption_key)
+    return keyfile
+
+
+def _delete_encryption_keyfile(device_path):
+    keyfile = _get_encryption_keyfile_name(device_path)
+    if os.path.isfile(keyfile):
+        log("Deleting keyfile: {}".format(keyfile))
+        os.unlink(keyfile)
+
+
+# Write the encryption details to the FS to allow auto-mounting
+# If store_encryption_keys config option is not "yes", this will
+# remove the current device_path from the file
+def write_encryption_details(device_path):
+    keyfile = _write_encryption_keyfile(device_path)
+    basedev = os.path.basename(device_path)
+    uuid = _get_encryption_uuid(device_path)
+    newCrypttabFile = CRYPTTAB_FILE + ".new"
+    if uuid is not None:
+        log("Writing encryption details to {}".format(CRYPTTAB_FILE))
+        # Read original
+        lines = []
+        if os.path.exists(CRYPTTAB_FILE):
+            with open(CRYPTTAB_FILE, "r") as fr:
+                lines = fr.readlines()
+
+        # Write new (removing any existing lines for our device)
+        with open(newCrypttabFile, "w") as fw:
+            for line in lines:
+                if not line.startswith((basedev + " ", basedev + "\t")):
+                    fw.write(line)
+            if hookenv.config("store_encryption_keys") == "yes":
+                fw.write("{} UUID={} {} luks\n".format(basedev, uuid, keyfile))
+
+        # Move into place
+        if os.path.exists(CRYPTTAB_FILE):
+            os.rename(CRYPTTAB_FILE, CRYPTTAB_FILE + ".orig")
+        os.rename(newCrypttabFile, CRYPTTAB_FILE)
+
+        # Remove the keyfile if we don't want it saved
+        if hookenv.config("store_encryption_keys") != "yes":
+            _delete_encryption_keyfile(device_path)
+    else:
+        log("Unable to write encryption details to".format(CRYPTTAB_FILE))
+
+    # Add a simple script to help mount the crypted filesystem
+    # (only needed is not auto-mounting)
+    src = os.path.join(hookenv.charm_dir(), "files", "crypt_mount.sh")
+    dest = "/usr/local/sbin/crypt_mount.sh"
+    shutil.copyfile(src, dest)
+    os.chmod(dest, 0755)
 
=== added directory 'hooks/storage-provider.d/partition'
=== added file 'hooks/storage-provider.d/partition/data-relation-changed'
--- hooks/storage-provider.d/partition/data-relation-changed	1970-01-01 00:00:00 +0000
+++ hooks/storage-provider.d/partition/data-relation-changed	2014-11-07 12:56:29 +0000
@@ -0,0 +1,31 @@
+#!/usr/bin/python
+
+import sys
+import yaml
+from yaml.constructor import ConstructorError
+
+from charmhelpers.core import hookenv
+import common_util
+
+log = hookenv.log
+
+partition = None
+volume_map = hookenv.config().get("volume_map", None)
+relids = hookenv.relation_ids("data")
+for relid in relids:
+    for unit in hookenv.related_units(relid):
+        if volume_map is not None:
+            try:
+                volume_map = yaml.load(volume_map)
+                if volume_map:
+                    partition = volume_map.get(unit, None)
+            except ConstructorError as e:
+                hookenv.log(
+                    "invalid YAML in 'volume-map': {}".format(e),
+                    hookenv.WARNING)
+
+if partition is None:
+    log("No data-relation-changed hook fired. Awaiting data-relation")
+    sys.exit(0)
+
+common_util.mount_volume(partition)
 
=== modified file 'hooks/test_common_util.py'
--- hooks/test_common_util.py	2014-03-21 22:53:22 +0000
+++ hooks/test_common_util.py	2014-11-07 12:56:29 +0000
@@ -1,4 +1,5 @@
 import common_util as util
+import crypt_fs
 import json
 import mocker
 import os
@@ -13,6 +14,7 @@
         self.maxDiff = None
         util.FSTAB_FILE = self.makeFile()
         util.hookenv = TestHookenv({"provider": "nfs"})
+        crypt_fs.hookenv = TestHookenv()
         self.addCleanup(setattr, os, "environ", os.environ)
         self.charm_dir = self.makeDir()
         os.environ = {"CHARM_DIR": self.charm_dir}
@@ -537,7 +539,7 @@
 
         # Assert fsck is called
         fsck = self.mocker.replace(subprocess.call)
-        fsck("fsck -p /dev/vdz", shell=True)
+        fsck(["fsck", "-p", "/dev/vdz"], shell=False)
         self.mocker.result(0)   # successful fsck command exit
         self.mocker.replay()
 
@@ -561,9 +563,9 @@
         ext4_check("/dev/vdz", "ext4")
         self.mocker.result(False)
         fsck = self.mocker.replace(subprocess.check_call)
-        fsck("mkfs.ext4 /dev/vdz", shell=True)
+        fsck(["mkfs.ext4", "/dev/vdz"], shell=False)
         fsck = self.mocker.replace(subprocess.call)
-        fsck("fsck -p /dev/vdz", shell=True)
+        fsck(["fsck", "-p", "/dev/vdz"], shell=False)
         self.mocker.result(0)   # Sucessful command exit 0
         self.mocker.replay()
 
@@ -586,9 +588,9 @@
         ext4_check("/dev/vdz", "ext4")
         self.mocker.result(False)
         fsck = self.mocker.replace(subprocess.check_call)
-        fsck("mkfs.ext4 /dev/vdz", shell=True)
+        fsck(["mkfs.ext4", "/dev/vdz"], shell=False)
         fsck = self.mocker.replace(subprocess.call)
-        fsck("fsck -p /dev/vdz", shell=True)
+        fsck(["fsck", "-p", "/dev/vdz"], shell=False)
         self.mocker.result(1)
         self.mocker.replay()
 
@@ -733,7 +735,7 @@
             util.hookenv.is_logged(message), "Not logged- %s" % message)
 
         # Wrote proper fstab mount info to mount on reboot
-        fstab_content = "me.com:/nfs/server/path /mnt/this nfs someopts 0 0"
+        fstab_content = "me.com:/nfs/server/path /mnt/this nfs someopts 0 0\n"
         with open(util.FSTAB_FILE) as input_file:
             self.assertEqual(input_file.read(), fstab_content)
 
@@ -789,9 +791,9 @@
         exists = self.mocker.replace(os.path.exists)
         exists("/dev/vdx1")
         self.mocker.result(False)
-        create_partition = self.mocker.replace(
-            util._format_device)
+        create_partition = self.mocker.replace(util._format_device)
         create_partition("/dev/vdx")
+        self.mocker.result("/dev/vdx")
 
         _set_label = self.mocker.replace(util._set_label)
         _set_label("/dev/vdx", "/mnt/this")
@@ -806,6 +808,11 @@
 
         mount = self.mocker.replace(subprocess.check_call)
         mount("mount -t ext4 /dev/vdx /mnt/this", shell=True)
+
+        is_encrypted = self.mocker.replace(crypt_fs.is_encrypted)
+        is_encrypted("/dev/vdx")
+        self.mocker.result(False)
+
         self.mocker.replay()
 
         self.assertEqual(util.get_provider(), "block-storage-broker")
@@ -815,7 +822,7 @@
             util.hookenv.is_logged(message), "Not logged- %s" % message)
 
         # Wrote proper fstab mount info to mount on reboot
-        fstab_content = "/dev/vdx /mnt/this ext4 default 0 0"
+        fstab_content = "/dev/vdx /mnt/this ext4 defaults 0 0\n"
         with open(util.FSTAB_FILE) as input_file:
             self.assertEqual(input_file.read(), fstab_content)
 
@@ -849,9 +856,9 @@
         exists = self.mocker.replace(os.path.exists)
         exists("/dev/vdx1")
         self.mocker.result(True)
-        create_partition = self.mocker.replace(
-            util._format_device)
+        create_partition = self.mocker.replace(util._format_device)
         create_partition("/dev/vdx1")
+        self.mocker.result("/dev/vdx1")
 
         _set_label = self.mocker.replace(util._set_label)
         _set_label("/dev/vdx1", "/mnt/this")
@@ -861,6 +868,10 @@
         get_fstype("blkid -o value -s TYPE /dev/vdx1", shell=True)
         self.mocker.result("ext4\n")
 
+        is_encrypted = self.mocker.replace(crypt_fs.is_encrypted)
+        is_encrypted("/dev/vdx1")
+        self.mocker.result(False)
+
         exists("/mnt/this")
         self.mocker.result(True)
 
@@ -878,7 +889,7 @@
                 util.hookenv.is_logged(message), "Not logged- %s" % message)
 
         # Wrote proper fstab mount info to mount on reboot
-        fstab_content = "/dev/vdx1 /mnt/this ext4 default 0 0"
+        fstab_content = "/dev/vdx1 /mnt/this ext4 defaults 0 0\n"
         with open(util.FSTAB_FILE) as input_file:
             self.assertEqual(input_file.read(), fstab_content)
 
 
=== added file 'hooks/test_crypt_fs.py'
--- hooks/test_crypt_fs.py	1970-01-01 00:00:00 +0000
+++ hooks/test_crypt_fs.py	2014-11-07 12:56:29 +0000
@@ -0,0 +1,248 @@
+import crypt_fs
+import os
+import shutil
+import mocker
+from testing import TestHookenv
+
+
+class TestCryptFs(mocker.MockerTestCase):
+
+    def setUp(self):
+        self.maxDiff = None
+        crypt_fs.hookenv = TestHookenv()
+        crypt_fs.KEYFILE_DIR = "/tmp"
+        crypt_fs.CRYPTTAB_FILE = "/tmp/crypttab"
+        self.addCleanup(setattr, os, "environ", os.environ)
+        self.charm_dir = self.makeDir()
+        os.environ = {"CHARM_DIR": self.charm_dir}
+
+    def test_get_encryption_key_if_exists(self):
+        """
+        L{get_encryption_key} returns the encryption key for a
+        specific unit
+        """
+        crypt_fs.hookenv._config = (
+            ("encryption_key_map", "{data/0: password1}"),
+        )
+        self.assertEqual(crypt_fs.get_encryption_key(), "password1")
+
+    def test_get_encryption_key_if_not_exists(self):
+        """
+        L{get_encryption_key} returns None when no key found
+        """
+        crypt_fs.hookenv._config = (
+            ("encryption_key_map", "{data/1: password1}"),
+        )
+        self.assertEqual(crypt_fs.get_encryption_key(), None)
+
+    def test_write_encryption_keyfile(self):
+        """
+        L{_write_encryption_keyfile} writes the key to disk in
+        crypt_fs.KEYFILE_DIR
+        """
+        crypt_fs.hookenv._config = (
+            ("encryption_key_map", "{data/0: password1}"),
+        )
+        keyfile_path = os.path.join(crypt_fs.KEYFILE_DIR,
+                                    "keyfile--dev-something")
+        if os.path.exists(keyfile_path):
+            os.unlink(keyfile_path)
+        crypt_fs._write_encryption_keyfile("/dev/something")
+        self.assertTrue(os.path.exists(keyfile_path))
+        with open(keyfile_path) as infile:
+            key = infile.read()
+        self.assertEqual(key, "password1")
+
+    def test_delete_encryption_keyfile(self):
+        """
+        L{_delete_encryption_keyfile} should remove the key from
+        crypt_fs.KEYFILE_DIR
+        """
+        keyfile_path = os.path.join(crypt_fs.KEYFILE_DIR,
+                                    "keyfile--dev-something")
+        with open(keyfile_path, "w") as outfile:
+            outfile.write("test")
+        self.assertTrue(os.path.exists(keyfile_path))
+        crypt_fs._delete_encryption_keyfile("/dev/something")
+        self.assertFalse(os.path.exists(keyfile_path))
+
+    def test_write_encryption_details_no_uuid_no_crypttab(self):
+        """
+        L{write_encryption_details} Adds a crypttab entry for
+        the newly created filesystem.
+        It should leave it well alone for this test
+        """
+        crypt_fs.hookenv._config = (
+            ("encryption_key_map", "{data/0: password1}"),
+        )
+
+        encryption_uuid = self.mocker.replace(crypt_fs._get_encryption_uuid)
+        encryption_uuid("/dev/absent")
+        self.mocker.result(None)
+
+        copyfile = self.mocker.replace(shutil.copyfile)
+        src = os.path.join(crypt_fs.hookenv.charm_dir(),
+                           "files",
+                           "crypt_mount.sh")
+        copyfile(src, "/usr/local/sbin/crypt_mount.sh")
+
+        chmod = self.mocker.replace(os.chmod)
+        chmod("/usr/local/sbin/crypt_mount.sh", 0755)
+
+        self.mocker.replay()
+
+        if os.path.exists(crypt_fs.CRYPTTAB_FILE):
+            os.unlink(crypt_fs.CRYPTTAB_FILE)
+        self.assertFalse(os.path.exists(crypt_fs.CRYPTTAB_FILE))
+        crypt_fs.write_encryption_details("/dev/absent")
+        self.assertFalse(os.path.exists(crypt_fs.CRYPTTAB_FILE))
+
+    def test_write_encryption_details_no_uuid_have_crypttab(self):
+        """
+        L{write_encryption_details} Adds a crypttab entry for
+        the newly created filesystem
+        It should leave the existing file untouched in this test
+        """
+        crypt_fs.hookenv._config = (
+            ("encryption_key_map", "{data/0: password1}"),
+        )
+
+        encryption_uuid = self.mocker.replace(crypt_fs._get_encryption_uuid)
+        encryption_uuid("/dev/absent")
+        self.mocker.result(None)
+
+        copyfile = self.mocker.replace(shutil.copyfile)
+        src = os.path.join(crypt_fs.hookenv.charm_dir(),
+                           "files",
+                           "crypt_mount.sh")
+        copyfile(src, "/usr/local/sbin/crypt_mount.sh")
+
+        chmod = self.mocker.replace(os.chmod)
+        chmod("/usr/local/sbin/crypt_mount.sh", 0755)
+
+        self.mocker.replay()
+
+        crypttab_content = "sda_crypt UUID=SOME-EXISTING-UUID none luks\n"
+        with open(crypt_fs.CRYPTTAB_FILE, "w") as outfile:
+            outfile.write(crypttab_content)
+        self.assertTrue(os.path.exists(crypt_fs.CRYPTTAB_FILE))
+        crypt_fs.write_encryption_details("/dev/absent")
+        with open(crypt_fs.CRYPTTAB_FILE) as infile:
+            self.assertEqual(infile.read(), crypttab_content)
+
+    def test_write_encryption_details_have_uuid_no_crypttab(self):
+        """
+        L{write_encryption_details} Adds a crypttab entry for
+        the newly created filesystem
+        It should leave the existing file untouched in this test
+        """
+        crypt_fs.hookenv._config = (
+            ("encryption_key_map", "{data/0: password1}"),
+            ("store_encryption_keys", "yes"),
+        )
+
+        encryption_uuid = self.mocker.replace(crypt_fs._get_encryption_uuid)
+        encryption_uuid("/dev/something")
+        self.mocker.result("SOME-NEW-UUID")
+
+        copyfile = self.mocker.replace(shutil.copyfile)
+        src = os.path.join(crypt_fs.hookenv.charm_dir(),
+                           "files",
+                           "crypt_mount.sh")
+        copyfile(src, "/usr/local/sbin/crypt_mount.sh")
+
+        chmod = self.mocker.replace(os.chmod)
+        chmod("/usr/local/sbin/crypt_mount.sh", 0755)
+
+        self.mocker.replay()
+
+        if os.path.exists(crypt_fs.CRYPTTAB_FILE):
+            os.unlink(crypt_fs.CRYPTTAB_FILE)
+        self.assertFalse(os.path.exists(crypt_fs.CRYPTTAB_FILE))
+        crypt_fs.write_encryption_details("/dev/something")
+        keyfile_path = os.path.join(crypt_fs.KEYFILE_DIR,
+                                    "keyfile--dev-something")
+        crypttab_content = (
+            "something UUID=SOME-NEW-UUID {} luks\n".format(keyfile_path))
+        with open(crypt_fs.CRYPTTAB_FILE) as infile:
+            self.assertEqual(infile.read(), crypttab_content)
+
+    def test_write_encryption_details_have_uuid_have_crypttab(self):
+        """
+        L{write_encryption_details} Adds a crypttab entry for
+        the newly created filesystem
+        It should add to the file for this test
+        """
+        crypt_fs.hookenv._config = (
+            ("encryption_key_map", "{data/0: password1}"),
+            ("store_encryption_keys", "yes"),
+        )
+
+        encryption_uuid = self.mocker.replace(crypt_fs._get_encryption_uuid)
+        encryption_uuid("/dev/something")
+        self.mocker.result("SOME-NEW-UUID")
+
+        copyfile = self.mocker.replace(shutil.copyfile)
+        src = os.path.join(crypt_fs.hookenv.charm_dir(),
+                           "files",
+                           "crypt_mount.sh")
+        copyfile(src, "/usr/local/sbin/crypt_mount.sh")
+
+        chmod = self.mocker.replace(os.chmod)
+        chmod("/usr/local/sbin/crypt_mount.sh", 0755)
+
+        self.mocker.replay()
+
+        crypttab_content = "sda_crypt UUID=SOME-EXISTING-UUID none luks\n"
+        with open(crypt_fs.CRYPTTAB_FILE, "w") as outfile:
+            outfile.write(crypttab_content)
+        self.assertTrue(os.path.exists(crypt_fs.CRYPTTAB_FILE))
+        crypt_fs.write_encryption_details("/dev/something")
+        keyfile_path = os.path.join(crypt_fs.KEYFILE_DIR,
+                                    "keyfile--dev-something")
+        crypttab_content = (
+            "sda_crypt UUID=SOME-EXISTING-UUID none luks\n"
+            "something UUID=SOME-NEW-UUID {} luks\n".format(keyfile_path))
+        with open(crypt_fs.CRYPTTAB_FILE) as infile:
+            self.assertEqual(infile.read(), crypttab_content)
+
+    def test_write_encryption_details_have_uuid_have_duplicate_crypttab(self):
+        """
+        L{write_encryption_details} Adds a crypttab entry for
+        the newly created filesystem
+        It should replace an existing entry in this test
+        """
+        crypt_fs.hookenv._config = (
+            ("encryption_key_map", "{data/0: password1}"),
+            ("store_encryption_keys", "yes"),
+        )
+
+        encryption_uuid = self.mocker.replace(crypt_fs._get_encryption_uuid)
+        encryption_uuid("/dev/something")
+        self.mocker.result("SOME-NEW-UUID")
+
+        copyfile = self.mocker.replace(shutil.copyfile)
+        src = os.path.join(crypt_fs.hookenv.charm_dir(),
+                           "files",
+                           "crypt_mount.sh")
+        copyfile(src, "/usr/local/sbin/crypt_mount.sh")
+
+        chmod = self.mocker.replace(os.chmod)
+        chmod("/usr/local/sbin/crypt_mount.sh", 0755)
+
+        self.mocker.replay()
+
+        crypttab_content = (
+            "sda_crypt UUID=SOME-EXISTING-UUID none luks\n"
+            "something UUID=AN-OLD-UUID none luks\n")
+        with open(crypt_fs.CRYPTTAB_FILE, "w") as outfile:
+            outfile.write(crypttab_content)
+        self.assertTrue(os.path.exists(crypt_fs.CRYPTTAB_FILE))
+        crypt_fs.write_encryption_details("/dev/something")
+        keyfile_path = os.path.join(crypt_fs.KEYFILE_DIR,
+                                    "keyfile--dev-something")
+        crypttab_content = (
+            "sda_crypt UUID=SOME-EXISTING-UUID none luks\n"
+            "something UUID=SOME-NEW-UUID {} luks\n".format(keyfile_path))
+        with open(crypt_fs.CRYPTTAB_FILE) as infile:
+            self.assertEqual(infile.read(), crypttab_content)
 
=== added file 'tests/10-crypt-keep-keyfile'
--- tests/10-crypt-keep-keyfile	1970-01-01 00:00:00 +0000
+++ tests/10-crypt-keep-keyfile	2014-11-07 12:56:29 +0000
@@ -0,0 +1,79 @@
+#!/usr/bin/python3
+#
+# Tests the partition provider with disk encryption
+# Almost identical to 20-crypt-lose-keyfile only we want to
+# make sure everything works AND the keyfile still exists
+
+import amulet
+import time
+import logging
+
+d = amulet.Deployment(series="trusty")
+
+# Add units
+d.add('postgresql', 'postgresql')
+d.add('storage', 'storage')
+
+# Set the unit configs
+d.configure("storage", {"provider": "partition", "volume_map": "{postgresql/0: /dev/loop0}", "encryption_key_map": "{postgresql/0: password1}", "store_encryption_keys": "yes"})
+
+try:
+    d.setup(timeout=900)
+    d.sentry.wait()
+except amulet.helpers.TimeoutError:
+    amulet.raise_status(amulet.FAIL, msg="Environment wasn't stood up in time")
+except:
+    raise
+
+unit = d.sentry['postgresql/0']
+
+# Create a test device
+if unit.run("dd if=/dev/zero of=/testfile bs=1024 count=10240 2>&1")[1] != 0:
+    amulet.raise_status(amulet.FAIL, msg="dd failed creating /testfile")
+if unit.run("losetup /dev/loop0 /testfile")[1] != 0:
+    amulet.raise_status(amulet.FAIL, msg="losetup failed creating /dev/loop0")
+time.sleep(10)
+
+# Add relations
+d.relate('postgresql:data', 'storage:data')
+d.sentry.wait()
+time.sleep(30)
+
+
+# Check mounts
+mounts = unit.run("cat /proc/mounts")[0].split("\n")
+mountsOk = False
+for line in mounts:
+    if "/dev/mapper/loop0" in line and "/srv/data" in line:
+        mountsOk = True
+if not mountsOk:
+    amulet.raise_status(amulet.FAIL, msg="/dev/mapper/loop0 mount missing")
+
+# Check fstab
+fstab = unit.run("cat /etc/fstab")[0].split("\n")
+fstabOk = False
+for line in fstab:
+    if "/dev/mapper/loop0" in line and "/srv/data" in line:
+        fstabOk = True
+if not fstabOk:
+    amulet.raise_status(amulet.FAIL, msg="/dev/mapper/loop0 fstab entry missing")
+
+# Check crypt status
+cryptstatus = unit.run("cryptsetup status /dev/mapper/loop0")[0].split("\n")
+isActive = False
+isLuks = False
+for line in cryptstatus:
+    if "/dev/mapper/loop0 is active and is in use" in line:
+        isActive = True
+    if "type:" in line and "LUKS1" in line:
+        isLuks = True
+if not (isActive and isLuks):
+    amulet.raise_status(amulet.FAIL, msg="Crypt status incorrect")
+
+# Make sure keyfile exists
+if not "keyfile--dev-loop0" in unit.directory_contents("/root")["files"]:
+    amulet.raise_status(amulet.FAIL, msg="Keyfile not found")
+
+# And has the correct data
+if unit.file_contents("/root/keyfile--dev-loop0") != "password1":
+    amulet.raise_status(amulet.FAIL, msg="Keyfile incorrect")
 
=== added file 'tests/20-crypt-lose-keyfile'
--- tests/20-crypt-lose-keyfile	1970-01-01 00:00:00 +0000
+++ tests/20-crypt-lose-keyfile	2014-11-07 12:56:29 +0000
@@ -0,0 +1,73 @@
+#!/usr/bin/python3
+#
+# Tests the partition provider with disk encryption
+# Almost identical to 10-crypt-keep-keyfile only we want to
+# make sure everything works AND the keyfile is deleted
+
+import amulet
+import time
+
+d = amulet.Deployment(series="trusty")
+
+# Add units
+d.add('postgresql', 'postgresql')
+d.add('storage', 'storage')
+
+# Set the unit configs
+d.configure("storage", {"provider": "partition", "volume_map": "{postgresql/0: /dev/loop0}", "encryption_key_map": "{postgresql/0: password1}", "store_encryption_keys": "no"})
+
+try:
+    d.setup(timeout=900)
+    d.sentry.wait()
+except amulet.helpers.TimeoutError:
+    amulet.raise_status(amulet.FAIL, msg="Environment wasn't stood up in time")
+except:
+    raise
+
+unit = d.sentry.unit['postgresql/0']
+
+# Create a test device
+if unit.run("dd if=/dev/zero of=/testfile bs=1024 count=10240 2>&1")[1] != 0:
+    amulet.raise_status(amulet.FAIL, msg="dd failed creating /testfile")
+if unit.run("losetup /dev/loop0 /testfile")[1] != 0:
+    amulet.raise_status(amulet.FAIL, msg="losetup failed creating /dev/loop0")
+time.sleep(10)
+
+# Add relations
+d.relate('postgresql:data', 'storage:data')
+d.sentry.wait()
+time.sleep(30)
+
+# Check mounts
+mounts = unit.run("cat /proc/mounts")[0].split("\n")
+mountsOk = False
+for line in mounts:
+    if "/dev/mapper/loop0" in line and "/srv/data" in line:
+        mountsOk = True
+if not mountsOk:
+    amulet.raise_status(amulet.FAIL, msg="/dev/mapper/loop0 mount missing")
+
+# Check fstab
+fstab = unit.run("cat /etc/fstab")[0].split("\n")
+fstabOk = False
+for line in fstab:
+    if "/dev/mapper/loop0" in line and "/srv/data" in line:
+        fstabOk = True
+if not fstabOk:
+    amulet.raise_status(amulet.FAIL, msg="/dev/mapper/loop0 fstab entry missing")
+
+# Check crypt status
+cryptstatus = unit.run("cryptsetup status /dev/mapper/loop0")[0].split("\n")
+isActive = False
+isLuks = False
+for line in cryptstatus:
+    if "/dev/mapper/loop0 is active and is in use" in line:
+        isActive = True
+    if "type:" in line and "LUKS1" in line:
+        isLuks = True
+if not (isActive and isLuks):
+    amulet.raise_status(amulet.FAIL, msg="Crypt status incorrect")
+
+# Make sure keyfile does not exist
+if "keyfile--dev-loop0" in unit.directory_contents("/root")["files"]:
+    amulet.raise_status(amulet.FAIL, msg="Keyfile found when not wanted")