LUKS volume encryption

[kalahari]

This makes use of https://gitlab.com/cryptsetup/cryptsetup/ to apply LUKS encryption to volumes when they are created. Two new options were added to support the feature:

dd if=/dev/urandom of=/root/luks-key.bin bs=4096 count=1
docker volume create -d lvm --name foobar --opt size=5G --opt thinpool=volume-thinpool --opt crypt=luks --opt keyfile=/root/luks-key.bin

The key file must be available when the volume is created, and when it is mounted. I chose to use key files so there would be no storing of secrets within docker-lvm-plugin.

I am not a go expert, so I would appreciate any pointers to improve the code. If this PR looks good code wise, I will update the README.md as well.

[shishir-a412ed]

@kalahari This is interesting :) However, lately I have been putting all the features in one file (driver.go) which has resulted into one behemoth code file ! Let me do some modularization and cleanup of the code, and then we can review this.

Also, I was thinking rather than passing these options --opt crypt and --opt keyfile everytime at volume creation, Why not just update it once in the config file /etc/docker/docker-lvm-plugin where we update our volume group name. That ways if a user put these properties in the config file we know he need the volumes to be encrypted.

Shishir

[shishir-a412ed]

@kalahari Meanwhile can you squash these commits into one ?

Shishir

[rhatdan]

Would their be cases where a user wants some volumes encrypted and other not encrypted?

[shishir-a412ed]

@rhatdan It totally depends upon how we want to offer it.

Since the docker volume plugin API is not very generic and cannot be scaled much. We end up putting every new option under --opt. I feel we already have too many options under --opt. With this I agree, the user either gets all the volumes encrypted or none of them. But he doesn't have to worry about passing these options --opt crypt and --opt keyfile everytime he creates a volume.

Shishir

[rhatdan]

Ok then lets document that.

[kalahari]

@shishir-a412ed I think config is a good idea for options, but allowing them to be overridden with --opt on a case by case basis would give both ease of use and flexibility.

Commits squashed.

[shishir-a412ed]

@kalahari Let's keep it at one place. Since you want these to be options (--opt) for ease of use and flexibility, let's do it as options.

One thing I wanted to ask was why do we need --opt crypt=luks ? Do you have plans to extend this for other encryption technologies ? I mean if LUKS is the only one, a user can just pass --opt keyfile=.

Shishir

[kalahari]

@shishir-a412ed Originally I was thinking --opt crypt= would select between cryptsetup's four formats: dm-crypt, LUKS, loop-AES, and TrueCrypt/VeraCrypt. But I don't have a good use case for any of the other formats besides LUKS. I will remove the option and update the pull.

[shishir-a412ed]

@kalahari Can you please squash the commits into one ? In future, you can do a git commit --amend to amend on the same commit and do a force push using git push -u origin master -f where origin is your remote and master is your remote branch. I am guessing in your case it would be feature LUKS.

Shishir

[kalahari]

@shishir-a412ed you caught me in the middle of making changes, should be all good now.

I removed the --opt crypt= option, and added --opt keyfile= to the docs.

[shishir-a412ed]

@kalahari Sorry :) Yeah will take a look now.

Shishir

[shishir-a412ed]

Since luksOpen is called in both Create() and Mount(). And luksClose is called in both Create() and Unmount(). Can we modularize this ? Add 2 methods named luksOpen() and luksClose() and call them from respective places.

Something like:

func luksOpen(vgName, volName, keyFile string) ([]byte, error){
fsDevice := fmt.Sprintf("/dev/%s/%s", vgName, volName)
cmd = exec.Command("cryptsetup", "-d", keyFile, "luksOpen", fsDevice , fmt.Sprintf("luks-%s", volName))
            if out, err := cmd.CombinedOutput(); err != nil {
                                       return out, error
            }
return nil, nil
}

and make the caller (either Create() or Mount()) log and return the error. Similar thing for luksClose().

Shishir

[kalahari]

OK, good stuff. I'm going to absorb all this and make the changes. Then I want to run it through my project's test suite to exercise it a bit. (Volumes and snapshots are created/destroyed during the tests.) Will get back to you this weekend with updated PR.

[kalahari]

@shishir-a412ed I believe I have addressed all your concerns. If you feel strongly that encrypted snapshots should validate the key on creation, I will change that as well.

[shishir-a412ed]

@kalahari This looks good ! Thanks for the quick response. I would like to change a few things, but I think at this point we can merge this. I can do the cleanups later, if need be.

Regarding encrypted snapshots, I think your use case makes sense. Just one question:

1) With this patch a user can take a snapshot of encrypted regular or thin volume even if the key is not present. With this the snapshots (backups) that are created would still be encrypted right ? Later on when the user mounts it inside the container, it will fail if the key is missing.

Rest looks good. Thanks again ! Shishir

[kalahari]

@shishir-a412ed You are correct, the encryption is applied on top of the logical volume. Mounting an encrypted snapshot later will require the original key be present.