New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding support to mount an encrypted filesystem #3249

Merged
merged 2 commits into from Jan 18, 2019

Conversation

Projects
None yet
4 participants
@chriswue
Copy link
Contributor

chriswue commented Dec 20, 2018

- What I did
The PR has 3 commits:

  1. A losetup utility container allowing to setup a loop device backed by a regular file, including test and docs
  2. A dm-crypt container allowing to map an device with dm-crypt including tests and docs
  3. A couple of examples

Obviously I have no access to the linuxkit hub so a contributor would have to built+push the containers (all .yml files assume that this will happen as they use linuxkit/... images)

Fixes: #3245

- How I did it
Tried to follow https://github.com/linuxkit/linuxkit/blob/master/docs/packages.md and practices and structure of the other existing packages.

- How to verify it
Build it locally with:

linuxkit pkg build pkg/losetup
linuxkit pkg build pkg/dm-crypt

linuxkit build -format kernel+initrd examples/dm-crypt.yml
linuxkit run -disk size=20M dm-crypt

- Description for the changelog
Adding support and examples for loop device and dm-crypt support

- A picture of a cute animal (not mandatory but encouraged)

sorry no time to sift through the copyright implications

@deitch

This comment has been minimized.

Copy link
Collaborator

deitch commented Dec 20, 2018

Overall, I like this idea. I had planned at some point to make it able to boot off an encrypted filesystem, and then eventually store/retrieve keys from tpm, but haven't found the time yet.

I have a few small comments (inline).

Separately, CI is failing, but that looks like trust issues, mainly because the images don't exist and weren't pushed out.

Like to get some comments from the other maintainers, in addition to these.

Nice work!

WORKDIR /
COPY --from=mirror /out/ /

COPY crypto.sh /crypto.sh

This comment has been minimized.

@deitch

deitch Dec 20, 2018

Collaborator

I think this should be copied just as crypto rather than crypto.sh. The fact that it is a shell is irrelevant to the command, and it probably will be go-ified at some point anyway. Much easier if you can change just the hash version than the command name.

This comment has been minimized.

@chriswue

chriswue Dec 21, 2018

Contributor

good point, should be fixed now

WORKDIR /
COPY --from=mirror /out/ /

COPY loopy.sh /loopy.sh

This comment has been minimized.

@deitch

deitch Dec 20, 2018

Collaborator

Same comment as with crypto.sh. I think this should just be

COPY loopy.sh loopy

This requires changing it everywhere else in this PR, but search-and-replace should make it easy.

This comment has been minimized.

@deitch

deitch Dec 20, 2018

Collaborator

BTW, I love that you called it "loopy". :-)

This comment has been minimized.

@chriswue

chriswue Dec 21, 2018

Contributor

was inspired by "mountie" :)
fixed as well

@deitch

This comment has been minimized.

Copy link
Collaborator

deitch commented Dec 20, 2018

I also think some of the docs should go in docs/ rather than in the specific pkg/. Someone who already knows about crypt or losetup will look there, but someone looking to answer, "how can I do a loopback device" or "how can I use an encrypted filesystem" will start by going to docs/

@chriswue chriswue force-pushed the chriswue:master branch from f0bb2d4 to 1721ea2 Dec 21, 2018

@chriswue

This comment has been minimized.

Copy link
Contributor

chriswue commented Dec 21, 2018

@deitch: I expanded the dm-crypt documentation a little bit and moved it into docs/

Thanks for the review

@rn
Copy link
Member

rn left a comment

Thanks for providing the packages, and tests, and documentation.

A few minor nits which should be easy to addresss.

Once we are happy with this, I'll set up the package signing and push them and would rekick the CI

image: linuxkit/mount:v0.6
command: ["/usr/bin/mountie", "/dev/sda1", "/var/external"]
- name: loop
image: linuxkit/losetup:v0.6

This comment has been minimized.

@rn

rn Dec 21, 2018

Member

can you use the has provided by linuxkit pkg show-tag instead of the v0.6 here. That way, when I actually build/push/sign the package, the CI should succeed.

image: linuxkit/losetup:v0.6
command: ["/loopy", "--create", "/var/external/storage_file"]
- name: dm-crypt
image: linuxkit/dm-crypt:v0.6

This comment has been minimized.

@rn

rn Dec 21, 2018

Member

ditto

image: linuxkit/format:v0.6
command: ["/usr/bin/format", "/dev/sda"]
- name: dm-crypt
image: linuxkit/dm-crypt:v0.6

This comment has been minimized.

@rn

rn Dec 21, 2018

Member

ditto

alpine-baselayout \
cryptsetup \
e2fsprogs

This comment has been minimized.

@rn

rn Dec 21, 2018

Member

nit: remove one of the empty lines

WORKDIR /
COPY --from=mirror /out/ /

COPY crypto.sh /crypto

This comment has been minimized.

@rn

rn Dec 21, 2018

Member
  • I prefer having consistent naming. The file is called crypto.sh on the filesystem but crypto in the container.
  • It should be in /usr/bin to be consistent with things things like mountie
  • maybe called it mount-crypt or something, though not sure about that.

This comment has been minimized.

@deitch

deitch Dec 21, 2018

Collaborator

@rn that was my request. This is in sh now, but will likely convert to pure binary (along with everything else we do). Keeping it named just crypto made it consistent as "just an executable". Same for loopy.

This comment has been minimized.

@chriswue

chriswue Dec 22, 2018

Contributor

Ok, so I'll move it into /usr/bin but keep the name as is. I tried to avoid the term mount since it's not actually mounting anything - it's mapping a device.

fi

if [ ! -f "$container_file" ]; then
if [ $create = true ]; then

This comment has been minimized.

@rn

rn Dec 21, 2018

Member

I'm not sure we want to create the file here. What's the use case for it?

This comment has been minimized.

@chriswue

chriswue Dec 22, 2018

Contributor

Well, there didn't seem to a nice way to create a file of arbitrary size in linuxkit. I'm using the loop device as a portable storage location hence creating it if it doesn't exist made things easier. I suppose one could make another package to create files of arbitrary size if they don't exists but not sure if that's starting to get a bit too granular.

- linuxkit/runc:83d0edb4552b1a5df1f0976f05f442829eac38fe
onboot:
- name: dm-crypt
image: linuxkit/dm-crypt:v0.6

This comment has been minimized.

@rn

rn Dec 21, 2018

Member

use the hash from that package here

- linuxkit/runc:83d0edb4552b1a5df1f0976f05f442829eac38fe
onboot:
- name: dm-crypt
image: linuxkit/dm-crypt:v0.6

This comment has been minimized.

@rn

rn Dec 21, 2018

Member

use the hash from that package here

- linuxkit/runc:83d0edb4552b1a5df1f0976f05f442829eac38fe
onboot:
- name: dm-crypt
image: linuxkit/dm-crypt:v0.6

This comment has been minimized.

@rn

rn Dec 21, 2018

Member

use the hash from that package here

- linuxkit/runc:83d0edb4552b1a5df1f0976f05f442829eac38fe
onboot:
- name: losetup
image: linuxkit/losetup:v0.6

This comment has been minimized.

@rn

rn Dec 21, 2018

Member

use the hash from that package here

@chriswue chriswue force-pushed the chriswue:master branch from 1721ea2 to 7395a5f Dec 22, 2018

@chriswue

This comment has been minimized.

Copy link
Contributor

chriswue commented Dec 22, 2018

Ok, I've updated the PR

  • All packages should be used with hash
  • Moved loopy and crypto into /usr/bin
  • Squashed the examples into the dm-crypt commit since the fixups became too annoying.
@deitch

This comment has been minimized.

Copy link
Collaborator

deitch commented Dec 23, 2018

I am good with this, subject to @rn signing off as well. @rn, if you are ok with it, I am happy to build the packages on arm64/amd64 and push them out so we can see ci pass (or fail).

@rn
Copy link
Member

rn left a comment

Thanks for the update. Looks like some of the previous comments were not addressed. Could you please comment? Thanks

WORKDIR /
COPY --from=mirror /out/ /

COPY crypto.sh /usr/bin/crypto

This comment has been minimized.

@rn

rn Dec 29, 2018

Member

This still has the crypto.sh -> crypto rename which neither @deitch nor me liked. Could you fix this?

This comment has been minimized.

@chriswue

chriswue Dec 29, 2018

Contributor

Well, it was actually @deitch who suggested to do it like this, see #3249 (comment)

This comment has been minimized.

@deitch

deitch Dec 30, 2018

Collaborator

Guilty as charged. I just wanted it to be crypto in the final package, not crypto.sh. @rn wanted it to be consistent from source to final package. I don't mind it being crypto.sh if it is hidden, but it is exposed when you call it, hence the desire for crypto (without the .sh extension). Maybe we should just keep it crypto I both src and pkg?

@@ -0,0 +1,27 @@
# LinuxKit losetup

This comment has been minimized.

@rn

rn Dec 29, 2018

Member

Could you move this documentation to ./docs as also @deitch suggested? One reason to have it in ./docs is that if it stays here and someone fixes, say a typo, then the content hash of the package changes and we'd have to rebuild, although the package itself did not change.

WORKDIR /
COPY --from=mirror /out/ /

COPY loopy.sh /usr/bin/loopy

This comment has been minimized.

@rn

rn Dec 29, 2018

Member

this also has the loopy.sh -> loopy rename

This comment has been minimized.

@chriswue

chriswue Dec 29, 2018

Contributor

same as per #3249 (comment)

This comment has been minimized.

@deitch

deitch Dec 30, 2018

Collaborator

Yes again. If @rn is ok with it, let's just name it loopy in the src and be consistent. I would say that is what we do elsewhere, but I think almost all of the sh code is replaced by go-sourced binaries by now. :-)

echo "Device $dmdev_name doesn't seem to contain a filesystem, creating one."
# dd will write the device until it's full and then return with an error because "no space left"
dd if=/dev/zero of="$dmdev_name" || true
mkfs.ext4 "$dmdev_name"

This comment has been minimized.

@rn

rn Dec 29, 2018

Member

any reason this is not formated with the format package?

@rn

This comment has been minimized.

Copy link
Member

rn commented Jan 2, 2019

Thanks for the response. let's leave the .sh rename out. Could you just move the doc from pkg/losetup/README.md either to ./docs or incorporate it into crypto setup doc?

Adding losetup utility package to core pkg
Signed-off-by: Christian Wuerdig <christian.wuerdig@gmail.com>

@chriswue chriswue force-pushed the chriswue:master branch from 7395a5f to ec85490 Jan 13, 2019

@chriswue

This comment has been minimized.

Copy link
Contributor

chriswue commented Jan 13, 2019

Moved pkg/losetup/README.md to docs/losetup.md

@rn
Copy link
Member

rn left a comment

Thanks for moving the doc. Unfortunately, this also changed the hash of the losetup package and this needs to be updated where used to linuxkit/losetup:b05ffc8641cc955abe45f6730cbe6d723b63bd3f

I have set up signing for these packages and pushed them for all arches, so once you update the YAMLs where the package is used, the CI should hopefully pass now.

Adding dm-crypt as core pkg
Signed-off-by: Christian Wuerdig <christian.wuerdig@gmail.com>

@chriswue chriswue force-pushed the chriswue:master branch from ec85490 to a9bc737 Jan 17, 2019

@chriswue

This comment has been minimized.

Copy link
Contributor

chriswue commented Jan 17, 2019

PR updated with correct SHA

@rn

rn approved these changes Jan 18, 2019

Copy link
Member

rn left a comment

Thanks for the patience. CI passed

@rn rn merged commit eeb2d54 into linuxkit:master Jan 18, 2019

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
ci/datakit/linuxkit-ci All tests passed
Details
dco-signed All commits are signed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment