Skip to content
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.

Sign up for GitHub

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

Jump to bottom

storage: Tang keyserver and passphrase management for Stratis #18818

Merged
merged 2 commits into from
Aug 9, 2023
Merged

storage: Tang keyserver and passphrase management for Stratis #18818

merged 2 commits into from
Aug 9, 2023

Conversation

mvollmer
Copy link
Member

@mvollmer mvollmer commented May 17, 2023 •
edited by jelly
Loading

Demo: https://youtu.be/HFCzWqqRXHQ

Storage: Stratis pools can now be bound to a Tang server

In addition to a passphrase (or instead of it), a Stratis pool can now use Tang server for encryption.

image

@mvollmer mvollmer added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label May 17, 2023
@mvollmer
Copy link
Member Author

mvollmer commented May 17, 2023 •
edited
Loading

A random thing I just learned: when adding a block device, all unlocking tokens must be available: passphrase and key server. If one of them is missing (wrong passphrase or key server not reachable), adding the block device will fail with this misleading error message:

"Neither the key in the kernel keyring nor Clevis could be used to perform encryption
operations on the devices in the pool; check that either the appropriate key in the keyring
is set or that the Clevis key storage method is available"

@mvollmer mvollmer force-pushed the storage-stratis-clevis branch 3 times, most recently from a3432b6 to a7d9504 Compare May 19, 2023 11:52
@mvollmer
Copy link
Member Author

mvollmer commented May 19, 2023 •
edited
Loading

About Stratis, Tang, and reboots:

  • A Stratis fsys that is bound to tang can only be unlocked via tang during boot. The stratis-fstab-set service will not ask for a passphrase if there is a celvis binding on a pool.
  • With "fail" mode in fstab, a boot will thus reliably break. It's expected that clevis can't unlock the fsys, but it's maybe unexpected that there is no possibility to input a passphrase. But Cockpit should definitely steer people away from this.
  • "nofail" will also fail to mount the fsys since the setup is run without network (unless you have rd.neednet set during boot) This is kinda expected and maybe we can allow this in Cockpit (with a warning?)
  • "netdev" will also fail, and this is unexpected. The mount unit has a dependency on network-online and on fstab-setup (which is the service that invokes clevis), but fstab-setup does not have a dependency on network-online. Thus, systemd starts fstab-setup very early and it will fail, which of course causes the mount unit to fail.

Thus:

  • We need to fix the ordering of fstab-setup for netdev filesystems. Maybe introduce a separate fstab-remote-setup that runs the same code but has a dep on network-online.
  • Changing the AtBoot mount option should select fstab-setup or fstab-remote-setup, as appropriate.
  • Cockpit should refuse to add a Tang server when there are filesystems that are "local", and should warn if there are filesystems that are "nofail". Maybe give the user an option to change all filesystems to "netdev".
  • New filesystems should be forced to "netdev" when a pool has already a tang binding.
  • Maybe warning triangles when netdev is missing for tang pools, and when the wrong setup unit is used.

Alternatively, we might make fstab-setup more robust:

  • If clevis fails, ask for a passphrase. Or better yet, run clevis and the passphrase in parallel and use whatever comes first.
  • Explicitly wait for network-online before starting clevis.

This would make things a bit simpler for Cockpit. (We still need to force netdev for tanged filesystems, but don't need to worry about which setup unit to use.)

@mvollmer mvollmer force-pushed the storage-stratis-clevis branch 3 times, most recently from 7311dcc to 7b838e5 Compare May 22, 2023 10:55
@mvollmer mvollmer removed the no-test For doc/workflow changes, or experiments which don't need a full CI run, label May 24, 2023
This was referenced May 24, 2023
Closed
Merged
@mvollmer mvollmer force-pushed the storage-stratis-clevis branch 2 times, most recently from 35c9daf to 0121bca Compare May 24, 2023 11:15
@mvollmer mvollmer marked this pull request as ready for review May 24, 2023 11:15
@mvollmer mvollmer requested a review from KKoukiou May 24, 2023 11:26
@mvollmer mvollmer force-pushed the storage-stratis-clevis branch 5 times, most recently from ff025d1 to c1b5523 Compare May 26, 2023 12:25
@mvollmer
Copy link
Member Author

mvollmer commented May 29, 2023 •
edited
Loading

Alright, I totally missed the "clevis_info" argument to CreatePool. Stratis allows the creation of a pool without a passphrase but clevis_info instead. We should support this here as well, and also make sure that pools without passphrases work as expected. Adding a passphrase to a pool without one should probably be part of #18842.

@mvollmer mvollmer marked this pull request as draft May 29, 2023 06:53
@mvollmer mvollmer changed the title storage: Clevis Tang for Stratis pools storage: Tang keyserver and passphrase management for Stratis May 29, 2023
@mvollmer
Copy link
Member Author

Adding a passphrase to a pool without one should probably be part of #18842.

Ah, heck, let's do it all here. It's all connected.

@mvollmer mvollmer added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label May 29, 2023
@mvollmer mvollmer force-pushed the storage-stratis-clevis branch 2 times, most recently from 5fa3f8f to 8355d51 Compare June 15, 2023 13:04
@mvollmer mvollmer requested a review from garrett July 4, 2023 12:12
@mvollmer
Copy link
Member Author

mvollmer commented Jul 4, 2023

@garrett , please take another look. New demo: https://youtu.be/HFCzWqqRXHQ

I have kept the "Add" / "Remove"` terminology for passphrases and keyservers. I couldn't think of anything better... can you?

@mvollmer mvollmer force-pushed the storage-stratis-clevis branch 3 times, most recently from fcf8d4a to 7ce862c Compare July 5, 2023 10:01
@mvollmer
Copy link
Member Author

mvollmer commented Jul 5, 2023

Alternatively, we might make fstab-setup more robust:
[...]
* Explicitly wait for network-online before starting clevis.

This seems to be the plan, see stratis-storage/stratisd#3348. With that solution, "nofail" is good enough, and since it is the default used by Cockpit, we probably don't need to do anything special.

@mvollmer mvollmer force-pushed the storage-stratis-clevis branch 2 times, most recently from 169c51f to 4f3ab2f Compare July 6, 2023 09:22
@mvollmer
Copy link
Member Author

mvollmer commented Jul 6, 2023

When SHA-1 is still needed:

The code that constructs the dialog does not know whether SHA-1 is needed, unfortunately. This was worked on in #18908 and has now been merged.

jelly
jelly requested changes Jul 6, 2023
View reviewed changes
Copy link
Member

@jelly jelly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is my comment correct about the translation? Otherwise LGTM.

pkg/storaged/stratis-panel.jsx Outdated Show resolved Hide resolved
@mvollmer
Copy link
Member Author

mvollmer commented Jul 7, 2023 •
edited
Loading

Hmm, now there is a failure on Arch:

warning: Command failed: cmd: "/usr/sbin/clevis" "luks" "bind" "-d" "/dev/sda" "-e" "1" "-t" "2" "tang" "{\"adv\":{\"payload\":\"eyJrZXlzIjogW3siYWxnIjogIkVTNTEyIiwgImt0eSI6ICJFQyIsICJjcnYiOiAiUC01MjEiLCAieCI6ICJBSGoya3FlM1k5Vmd6WXVrY1BTZGo2MkJUcy1taHFGV0cyXzZ2UkhTVmV3bTZ0YjJ4SFUtZGFIYkxCQlVNMkJrTmhBdzZNTlpqeHZMcjdTOU1JYmpYVTU1IiwgInkiOiAiQUlaOExrbm1udHlnSHN4WllCdG1DNDRGV2VfV1NKWUlvZG9WNkhodnliZGY3dGxRcDE1STBjYUgxamxnQ2lOS2VXM2U3bWdQNWRjMnlrUUVjMFFnN3dRYyIsICJrZXlfb3BzIjogWyJ2ZXJpZnkiXX0sIHsiYWxnIjogIkVDTVIiLCAia3R5IjogIkVDIiwgImNydiI6ICJQLTUyMSIsICJ4IjogIkFTb2JnRDBQRjgyd1Z3QTZYYXdMRW9idmxHRWNMUmhlR1dCdXRhbUlQd2FhTVFUeHBHZUNCYTk4b0dHblVPYUtfU0g0eUh6cS1lVGVRU2pkS05SWFNpekIiLCAieSI6ICJBWGFVN2k0RWx4TVFJbkVSdnBqTXd2YnlpNkQyM1RCQU5ja1l3N3BibFhtLXpJaDlLUldCRGdEZEFvY1RQQ2JINUkxWldOb1ZYRG5kMWNMWXlzS3FIdndCIiwgImtleV9vcHMiOiBbImRlcml2ZUtleSJdfV19\",\"protected\":\"eyJhbGciOiJFUzUxMiIsImN0eSI6Imp3ay1zZXQranNvbiJ9\",\"signature\":\"AbeUCV01KbM6yi_Ccp-nGvgKjdozlwo14REw9w6MREILhfY1PjEPcQ2UlmomrPkphzHuSGKYc4JGMpPwnw7CSmqzAX4Mz_FbX_ZyZABMGNJ4xw0cn109zFJHwazsc8x2YR59GuuxTUcGBYl7G5Bfycw9CWMpzdvUxOxARcHGAuaVPAhL\"},\"url\":\"10.111.112.5\"}", exit reason: 2 stdout:  stderr: 
Usage: clevis luks bind [-y] [-f] [-s SLT] [-k KEY] [-t TOKEN_ID] -d DEV PIN CFG

Binds a LUKS device using the specified policy:

  -f           Do not prompt for LUKSMeta initialization

  -d DEV       The LUKS device on which to perform binding

  -y           Automatically answer yes for all questions

  -s SLT       The LUKS slot to use

  -t TKN_ID    The LUKS token ID to use; only available for LUKS2

  -k KEY       Non-interactively read LUKS password from KEY file
  -k -         Non-interactively read LUKS password from standard input

clevis luks bind is missing the -e option.

@mvollmer
Copy link
Member Author

mvollmer commented Jul 7, 2023

clevis luks bind is missing the -e option.

Ah, this is because Arch now has stratisd 3.5. @jelly, looks like clevis needs to be udpated as well...

@mvollmer mvollmer requested a review from jelly August 8, 2023 07:32
@mvollmer
Copy link
Member Author

mvollmer commented Aug 8, 2023

clevis luks bind is missing the -e option.

Ah, this is because Arch now has stratisd 3.5. @jelly, looks like clevis needs to be udpated as well...

Seems fixed.

@mvollmer mvollmer mentioned this pull request Aug 8, 2023
Closed
cockpituous
cockpituous reviewed Aug 8, 2023
View reviewed changes
pkg/storaged/client.js
return client.stratis_manager.CreatePool(name, [false, 0],
devs,
key_desc ? [true, key_desc] : [false, ""],
[false, ["", ""]]);
clevis_info ? [true, clevis_info] : [false, ["", ""]]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This added line is not executed by any test. Details

pkg/storaged/stratis-details.jsx
Title: _("Add passphrase"),
Fields: [
PassInput("passphrase", _("Passphrase"),
{ validate: val => !val.length && _("Passphrase cannot be empty") }),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This added line is not executed by any test. Details

pkg/storaged/stratis-details.jsx
PassInput("passphrase", _("Passphrase"),
{ validate: val => !val.length && _("Passphrase cannot be empty") }),
PassInput("passphrase2", _("Confirm"),
{ validate: (val, vals) => vals.passphrase.length && vals.passphrase != val && _("Passphrases do not match") })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This added line is not executed by any test. Details

pkg/storaged/stratis-details.jsx
PassInput("old_passphrase", _("Old passphrase"),
{
visible: vals => !keydesc_set,
validate: val => !val.length && _("Passphrase cannot be empty")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This added line is not executed by any test. Details

pkg/storaged/stratis-details.jsx
validate: val => !val.length && _("Passphrase cannot be empty")
}),
PassInput("new_passphrase", _("New passphrase"),
{ validate: val => !val.length && _("Passphrase cannot be empty") }),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This added line is not executed by any test. Details

pkg/storaged/stratis-details.jsx
return start("clevis").catch(unlock_with_keyring);
} else if (!key_desc && clevis_info) {
return start("clevis");
} else if (key_desc && !clevis_info) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This added line is not executed by any test. Details

pkg/storaged/stratis-details.jsx

const encrypted = key_desc || clevis_info;
const can_tang = encrypted && (!clevis_info || clevis_info[0] == "tang");
const tang_url = (can_tang && clevis_info) ? JSON.parse(clevis_info[1]).url : null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This added line is not executed by any test. Details

pkg/storaged/stratis-panel.jsx
empty_warning: _("No block devices are available."),
validate: function (disks) {
if (disks.length === 0)
return _("At least one block device is needed.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This added line is not executed by any test. Details

pkg/storaged/stratis-panel.jsx
},
spaces: get_available_spaces(client)
}),
CheckBoxes("encrypt_pass", client.features.stratis_crypto_binding ? _("Encryption") : "",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This added line is not executed by any test. Details

pkg/storaged/stratis-panel.jsx
tag: "on",
title: (client.features.stratis_crypto_binding
? _("Use a passphrase")
: _("Encrypt data"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This added line is not executed by any test. Details

@jelly jelly dismissed garrett’s stale review August 9, 2023 09:57

stale review comment

@jelly jelly merged commit 4133eb9 into cockpit-project:main Aug 9, 2023
31 checks passed
@mvollmer mvollmer mentioned this pull request Aug 11, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cockpituous cockpituous cockpituous left review comments

@martinpitt martinpitt martinpitt left review comments

@jelly jelly jelly approved these changes

@KKoukiou KKoukiou Awaiting requested review from KKoukiou

@garrett garrett Awaiting requested review from garrett

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mvollmer @garrett @martinpitt @jelly @cockpituous