-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
storage: Readonly crypto improvements #20148
storage: Readonly crypto improvements #20148
Conversation
121dd94
to
8024dc0
Compare
b147676
to
de226a9
Compare
0de8c72
to
2209146
Compare
2209146
to
ee393f3
Compare
We don't currently get coverage reports as comments here. Results as of ee393f3: https://cockpit-logs.us-east-1.linodeobjects.com/pull-20148-ee393f37-20240318-154529-fedora-39-devel/Coverage/lcov/github-pr.diff.gcov.html Left uncovered: Reopening LUKS during formatting with a tang keyserver, where the tang server provides a wrong passphrase. |
test/verify/check-storage-luks TestStorageLuks.testFormatReadOnly runs out of memory on rhel-9-4. |
91554c0
to
3f7f1af
Compare
The man page mentions both, so let's not get confused when someone uses "read-only".
UDisks2 allows any block object to be used to update configuration items for any other block, and the mounting dialog was abusing that when mounting an encrypted filesystem: It would use the backing block to update the fstab entries of the cleartext block. This works perfectly fine, except that UDisks2 only guarantees proper notification ordering for the block used to make the method call. Thus, when calling UpdateConfiguration on the backing block, the "Configuration" property of the cleartext block would not necessarily be up-to-date yet when mount_at accesses it.
Previously, Cockpit would set the crypttab "readonly" option, but would not actually unlock crypto devices with the correct readonly-ness. One needs to pass this explicitly in the "Unlock" UDisks2 method, as it turns out. Also, Cockpit needs to reopen the crypt devices whenever readonly-ness changes, such as during formatting, and configuration changes. (And we need to workaround a unfortunate limitation in clevis-luks-unlock.)
3f7f1af
to
f1d0e23
Compare
} catch (error) { | ||
dlg.set_values({ needs_explicit_passphrase: true }); | ||
throw error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 3 added lines are not executed by any test.
const clear_dev = "luks-" + block.IdUUID; | ||
return cockpit.spawn(["clevis", "luks", "unlock", "-d", dev, "-n", clear_dev], | ||
{ superuser: true }); | ||
const clear_dev = luksname || "luks-" + block.IdUUID; |
There was a problem hiding this comment.
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.
PassInput("passphrase", _("Passphrase"), | ||
{ | ||
visible: vals => vals.needs_explicit_passphrase, | ||
validate: val => !val.length && _("Passphrase cannot be empty"), |
There was a problem hiding this comment.
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.
@@ -399,10 +432,13 @@ export function mounting_dialog(client, block, mode, forced_options, subvol) { | |||
opts = opts.concat(forced_options); | |||
if (vals.mount_options?.extra) | |||
opts = opts.concat(parse_options(vals.mount_options.extra)); | |||
// XXX - take subvols into account. | |||
const crypto_unlock_readonly = vals.mount_options?.ro ?? opt_ro; |
There was a problem hiding this comment.
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.
} else | ||
return Promise.resolve(); | ||
return block; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function can now throw an exception, do we test for that? I don't see the promise underneath handle the it. (no .catch()
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I don't think this does the right thing, users just call mounting_dialog
without handling anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception is caught by the dialog machinery and turned into an error in the dialog itself.
(Exceptions thrown by async functions are equivalent to rejected Promises.)
return client.blocks_cleartext[path]; | ||
} catch (error) { | ||
dlg.set_values({ needs_explicit_passphrase: true }); | ||
throw error; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe throwing here will break, as it is unhandled by the caller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also caught by the dialog machinery.
} | ||
|
||
export async function unlock_with_type(client, block, passphrase, passphrase_type) { | ||
export async function unlock_with_type(client, block, passphrase, passphrase_type, override_readonly) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit of a nit, but why not override_readonly = false
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
override_readonly
has three meanings:
- null / undefined: don't override, take readonly-nesss from /etc/crypttab
- true: make it readonly, regradless of what cryttab says
- false: make it readwrite, regardless of what crypttab says
The default if not specified is "don't override".
storage: Improvements to read-only encrypted filesystems
Cockpit now unlocks encrypted filesystems with a "read-only" encryption layer when the filesystem itself is mounted read-only.