Page MenuHomeFreeBSD

geli attach multiple providers (with kernel changes)
AbandonedPublic

Authored by woodsb02 on Jan 31 2017, 3:27 PM.

Details

Summary
Note: This diff is implementation option 1 of 2
Unlike D12644, this implementation modifies the kernel. The master key for each provider is determined and stored in an array, which is passed to the kernel with a single call.

Allow attaching of multiple geli providers at once if they use same
passphrase and keyfiles.

This is helpful when the providers being attached are not used for boot,
and therefore the existing code to first try the cached password when
tasting the providers during boot does not apply.

Multiple providers with the same passphrase and keyfiles can be attached
at the same time during system start-up by adding the following to
rc.conf:

geli_groups="storage backup"
geli_storage_flags="-k /etc/geli/storage.keys"
geli_storage_devices="ada0 ada1"
geli_backup_flags="-j /etc/geli/backup.passfile -k /etc/geli/backup.keys"
geli_backup_devices="ada2 ada3"
Test Plan

Tested using md(4) memory disks to confirm multiple providers can be attached if they use the same passphrase and keyfiles, but each provider fails to attach if the passphrase / keyfile combination fails to decrypt its master key.

mdconfig -s 1m -u 0
mdconfig -s 1m -u 1
mdconfig -s 1m -u 2
mdconfig -s 1m -u 3
geli init md0   # set password to "test"
geli init md1   # set password to "test"
geli init md2   # set password to "test123"
geli init md3   # set password to "test"
geli attach -v md0 md1
geli status
geli detach md0 md1

geli attach -v md0 md1 md2 md3
geli status
geli detach md0 md1 md3

Expected output:

Attached to md0 md1.
Done.
      Name  Status  Components
   md0.eli  ACTIVE  md0
   md1.eli  ACTIVE  md1

geli: Wrong key for md2.
      Name  Status  Components
   md0.eli  ACTIVE  md0
   md1.eli  ACTIVE  md1
   md3.eli  ACTIVE  md3

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 12692
Build 12963: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
woodsb02 updated this revision to Diff 24710.Feb 4 2017, 4:51 AM
woodsb02 edited edge metadata.

Clear the cached passphrase from memory after finished attaching each provider!

I implemented attaching multiple providers through to the kernel because by default the geom request is passed through to the kernel with all of the arguments that were given to userland. I could not easily think of how to modify the geom request to remove arguments 2-N and only pass argument 1, and then look through each of them with a modified request each time. Alternatively I could have tried creating a new request which has the same information but with only a single argument, and then looping through each provider creating a new request each time.

Hi Pawel, have you had any thoughts about whether this should be done entirely in userland by either modifying the geom request, or creating new geom requests for each disk listed as an argument (copying the other elements of the first request)?

wblock added inline comments.Feb 17 2017, 3:36 PM
sbin/geom/class/eli/geli.8
193–194

s/will be/is/

217–218

Do not parenthesize this.

221–222

s/Allows/Allow/
Also, there is a space before the period on this line.

225

s/Allows/Allow/

227

s/Allows/Allow/

381–383

Use .Fx for FreeBSD:

section to find which metadata version is supported by which
.Fx
version.
391

Make parenthesized part a separate sentence.

402

Do not parenthesize this.

448

Parenthesized part is an aside, an interruption in the flow which requires the reader to stop and reconsider. Don't do that. Either make it part of the same sentence or add a separate sentence.

468–469

Parenthesized aside.

469

s/will be/is/

469–472

Remove parens from aside.

645

Parenthesized aside.

666–667

"etc" is nonspecific and assumes the user knows what is meant. This is kind of a confusing sentence anyway. Sensitive data is not in memory due to the cache, but because of it. So maybe:

Please note that sensitive data might still be present in memory like filesystem
cache or other temporary locations after an encrypted device has been suspended.
680

s/example/example,/

682

s/is/is a/

688

An aside, but also pretty mysterious. How do you have part of the passphrase?

812–813

An aside, which doesn't appear to need to be an aside.

818

s/will be/is/

822

Try to eliminate this aside:

When set to 1, the passphrase entered at boot, before the root
filesystem is mounted, will be visible.
woodsb02 marked 14 inline comments as done.Feb 20 2017, 12:44 PM
woodsb02 added inline comments.
sbin/geom/class/eli/geli.8
221–222

This is a list of geli's most important features, not a description of what an option does. The plural makes sense in the context (more obvious when viewing the compiled version of the manpage).

225

This is a list of geli's most important features, not a description of what an option does. The plural makes sense in the context (more obvious when viewing the compiled version of the manpage).

227

This is a list of geli's most important features, not a description of what an option does. The plural makes sense in the context (more obvious when viewing the compiled version of the manpage).

448

It is not intended as an aside, it is intended as an explanation as to what is meant by the term "last close" (translate from developer speak to sysadmin speak).
I wanted to keep the terminology "last close" as it is likely more correct for developers reading this man page, but explain what this means to a sysadmin.

688

This is further explained in the "init -J" option, as described in the next line.
What are your thoughts about whether the 4 sentences from that other section should just be repeated here?

If
newpassfile is given as	-, standard input will
be used.  Only the first line (excluding new-
line character)	is taken from the given	file.
This argument can be specified multiple	times,
which has the effect of	reassembling a single
passphrase split across	multiple files.	 Can-
not be combined	with the -P option.

Note that the same would apply for the next options down (-k), where we might need to repeat the 4 sentences of the init -K section.

woodsb02 updated this revision to Diff 25413.Feb 20 2017, 12:45 PM
woodsb02 edited edge metadata.
  • Incorporate manpage improvements noted by wblock
wblock added a comment.Mar 3 2017, 4:17 PM

This looks better, thank you. A few additional suggestions, and a note that textproc/igor can be used to check for some basic mdoc formatting standards, sentences starting on a new line and that type of thing, along with a few thousand misspellings common in the FreeBSD community. Thanks!

sbin/geom/class/eli/geli.8
221–222

Sorry, I don't understand the response. The suggested change is just to make it less passive.

225

As above, this is just de-passification.

227

As above, this is just de-passification.

391

Thank you, this is better. But please start new sentences on new lines.

448

Parens pretty much automatically denote an aside ("hold that thought while we explain it"). Here, it can be part of the same sentence, so parens are not needed:

Mark provider to detach on last close, after the last filesystem on it has been unmounted.

Also, please start new sentences on new lines.

468–469

As above, parens are not needed:

Detach on last close, after the last filesystem has been unmounted.
471

Try to avoid the nonspecific "it":

detached after the filesystem has been created.
652

Sorry, this is my fault. Let me try to fix it:

This functionality is useful for laptops.
Suspending a laptop should not leave an encrypted device attached.
The
.Cm suspend
subcommand can be used rather than closing all files and directories from
filesystems on the encrypted device, unmounting the filesystem, and
detaching the device.
688

How about:

Specifies a file which contains the passphrase component of the User Key
or a part of it.
See the
.Cm init Op J
option.

Other sections can do the same thing for .Cm init -K.

woodsb02 updated this revision to Diff 28927.May 28 2017, 5:24 AM

Update diff to apply cleanly after recent geli changes by allanjude and mav

woodsb02 updated this revision to Diff 33243.Sep 20 2017, 5:47 PM

Update diff to apply cleanly against latest FreeBSD 12-current (r323818)

woodsb02 edited the test plan for this revision. (Show Details)Sep 22 2017, 1:59 PM
mav added a subscriber: mav.Sep 22 2017, 3:00 PM
woodsb02 updated this revision to Diff 33334.Sep 22 2017, 3:33 PM
woodsb02 edited the test plan for this revision. (Show Details)

Remove accidental changest to rc.conf involving ATM (svn merge errors)

woodsb02 edited the test plan for this revision. (Show Details)Sep 22 2017, 3:41 PM
woodsb02 edited the summary of this revision. (Show Details)Oct 12 2017, 3:13 PM
woodsb02 added a reviewer: mav.
delphij added inline comments.Oct 12 2017, 4:48 PM
etc/rc.d/geli
87

Maybe continue here? (so that one empty group won't prevent the following groups being attached)

sbin/geom/class/eli/geom_eli.c
918

Why do we need an array (versus just doing these operations in one single loop, and discard the key right after each gctl_issue) here? The "Attached to" and "\n" can be done by checking i's value below.

The benefit of doing the loop is that we would be able to distinguish individual failures.

I like the idea by the way.

Thanks for reviewing!

etc/rc.d/geli
87

Good point - I will update the review with this

sbin/geom/class/eli/geom_eli.c
918

This is because the geom request is passed to the kernel with all of the providers listed on the command line. Without this change the kernel errors out if there is more than one provider listed.

I have also submitted an alternative implementation of this change which does not modify the kernel. Instead it creates new child geom requests and issues them to the kernel one-by-one.

See D12644.

woodsb02 retitled this revision from geli: Allow attaching of multiple providers at once if they use same passphrase and keyfiles to geli attach multiple providers (with kernel changes).Oct 13 2017, 12:05 AM
woodsb02 edited the summary of this revision. (Show Details)
sobomax requested changes to this revision.Nov 12 2017, 1:24 AM
sobomax added inline comments.
sbin/geom/class/eli/geom_eli.c
513
strlen(cached_passphrase) > 0

or

cached_passphrase[0] != '\0'
514
srlcpy(passbuf, cached_passphrase, sizeof(passbuf));
527
srlcpy(passbuf, cached_passphrase, sizeof(passbuf));
955

Not sure if you need to zero-out the whole buf.

cached_passphrase[0] = '\0'

might suffice.

This revision now requires changes to proceed.Nov 12 2017, 1:24 AM
sobomax added inline comments.Nov 12 2017, 1:33 AM
etc/rc.d/geli
37

Use {} around var name consistently:

start_precmd='[ -n "${geli_make_list}" -o -n "${geli_groups}" ]'
sbin/geom/class/eli/geom_eli.c
527

Sorry, actually should be (note sizeof() of wrong argument, should be size of the DESTINATION buffer:

srlcpy(cached_passphrase, passbuf, sizeof(cached_passphrase));
woodsb02 updated this revision to Diff 35137.Nov 12 2017, 5:02 AM
woodsb02 marked 9 inline comments as done.

Implement comment from delphij:

  • If device group in rc script is empty, use continue instead of break to carry on with the next device group instead of skipping all devices groups.

Implement comments from sobomax:

  • Use strlen instead of strcmp to check for empty cached_passphrase
  • Use strlcpy instead of memcpy for strings
  • Consistently use curly braces to surround variable names in rc script

Thanks for the review delphij and sobomax. Comments incorporated.

sbin/geom/class/eli/geom_eli.c
955

I think the entire passphrase should be zeroed out, to prevent it sticking around in memory in clear text.

delphij added inline comments.Nov 12 2017, 5:26 AM
sbin/geom/class/eli/geom_eli.c
953–955

Unrelated to your change, but 'key' is on stack and therefore explicit_bzero() should be used, or it could be optimized away because key is undefined once out of scope.

955

I agree (if I was you I would use explicit_bzero() here too, but it would be a bug of the compiler if it optimize the bzero() away).

jilles added a subscriber: jilles.Nov 12 2017, 12:33 PM
jilles added inline comments.
etc/rc.d/geli
37

I think this comment was wrong. This code is a shell script, not a Makefile, and therefore $(geli_make_list) and ${geli_make_list} do something completely different. What is intended here is calling the function and checking its output, not checking a variable (which will not exist).

The same change was made to D12644.

Note that calling a function like this invokes it in a subshell environment and in a new process. Try to avoid this in new code since it is slower and causes changes to global variables and other state to be lost.

sobomax added inline comments.Nov 12 2017, 1:04 PM
etc/rc.d/geli
37

Aw, perhaps @jilles is right. Then geli_groups should be in {} and geli_make_list in ().

woodsb02 updated this revision to Diff 35159.Nov 12 2017, 5:35 PM

Implement comment from jilles:

  • Revert previous change from $(geli_make_list) and ${geli_make_list}
woodsb02 marked 2 inline comments as done.Nov 12 2017, 5:36 PM
woodsb02 added inline comments.
etc/rc.d/geli
37

Indeed, good pickup jilles. Thanks

woodsb02 marked 2 inline comments as done.Nov 12 2017, 5:36 PM
woodsb02 added inline comments.Nov 12 2017, 5:44 PM
sbin/geom/class/eli/geom_eli.c
953–955

That seems like the right idea.
There are 39 instances of bzero in sbin/geom/class/eli/geom_eli.c, of which 22 remain after removing those for the provider metadata (including salt) and sector buffer (for storing metadata to disk).
Do you think I should change all 39 to explicit_bzero, the 22, just mine, or none for now?

woodsb02 abandoned this revision.Jun 26 2018, 1:53 PM
woodsb02 marked 16 inline comments as done.

Abandon this change in favour of the review which only touches userland: D12644.

The 16 comments closed with this comment are updated in D12644, rather than in this review.