From 91554c08d6c57058a9a846f34943fcdd6574dcd8 Mon Sep 17 00:00:00 2001 From: Marius Vollmer Date: Fri, 8 Mar 2024 11:41:52 +0200 Subject: [PATCH] storage: Correctly maintain actual LUKS device readonly-ness 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.) --- pkg/storaged/block/format-dialog.jsx | 38 ++++++--- pkg/storaged/crypto/actions.jsx | 4 +- pkg/storaged/crypto/keyslots.jsx | 50 +++++++++--- pkg/storaged/dialog.jsx | 4 +- pkg/storaged/filesystem/mounting-dialog.jsx | 83 +++++++++++++------ test/verify/check-storage-luks | 90 +++++++++++++++++++-- 6 files changed, 216 insertions(+), 53 deletions(-) diff --git a/pkg/storaged/block/format-dialog.jsx b/pkg/storaged/block/format-dialog.jsx index 0d12e87b803e..df9f60db8bfa 100644 --- a/pkg/storaged/block/format-dialog.jsx +++ b/pkg/storaged/block/format-dialog.jsx @@ -176,7 +176,7 @@ function format_dialog_internal(client, path, start, size, enable_dos_extended, const content_block = block.IdUsage == "crypto" ? client.blocks_cleartext[path] : block; const offer_keep_keys = block.IdUsage == "crypto"; - const unlock_before_format = offer_keep_keys && !content_block; + const unlock_before_format = offer_keep_keys && (!content_block || content_block.ReadOnly); const create_partition = (start !== undefined); @@ -285,6 +285,8 @@ function format_dialog_internal(client, path, start, size, enable_dos_extended, extract_option(crypto_split_options, "noauto"); extract_option(crypto_split_options, "nofail"); extract_option(crypto_split_options, "_netdev"); + extract_option(crypto_split_options, "readonly"); + extract_option(crypto_split_options, "read-only"); const crypto_extra_options = unparse_options(crypto_split_options); let [, old_dir, old_opts] = get_fstab_config(block, true, @@ -494,6 +496,8 @@ function format_dialog_internal(client, path, start, size, enable_dos_extended, if (is_encrypted(vals)) { let opts = []; if (is_filesystem(vals)) { + if (vals.mount_options?.ro) + opts.push("readonly"); if (!mount_now || vals.at_boot == "never") opts.push("noauto"); if (vals.at_boot == "nofail") @@ -572,17 +576,24 @@ function format_dialog_internal(client, path, start, size, enable_dos_extended, if (config_items.length > 0) options["config-items"] = { t: 'a(sa{sv})', v: config_items }; - function maybe_unlock() { + async function maybe_unlock() { const content_block = client.blocks_cleartext[path]; - if (content_block) + if (content_block) { + if (content_block.ReadOnly) { + const block_crypto = client.blocks_crypto[path]; + await block_crypto.Lock({}); + await unlock_with_type(client, block, vals.old_passphrase, existing_passphrase_type, false); + } return content_block; + } - return (unlock_with_type(client, block, vals.old_passphrase, existing_passphrase_type) - .catch(error => { - dlg.set_values({ needs_explicit_passphrase: true }); - return Promise.reject(error); - }) - .then(() => client.blocks_cleartext[path])); + try { + await unlock_with_type(client, block, vals.old_passphrase, existing_passphrase_type, false); + return client.blocks_cleartext[path]; + } catch (error) { + dlg.set_values({ needs_explicit_passphrase: true }); + throw error; + } } function format() { @@ -644,6 +655,15 @@ function format_dialog_internal(client, path, start, size, enable_dos_extended, if (is_encrypted(vals)) remember_passphrase(new_block, vals.passphrase); + if (is_encrypted(vals) && is_filesystem(vals) && vals.mount_options?.ro) { + const block_crypto = await client.wait_for(() => block_crypto_for_block(path)); + await block_crypto.Lock({}); + if (vals.passphrase) + await block_crypto.Unlock(vals.passphrase, { "read-only": { t: "b", v: true } }); + else + await unlock_with_type(client, block, vals.old_passphrase, existing_passphrase_type, true); + } + if (is_filesystem(vals) && mount_now) { const block_fsys = await client.wait_for(() => block_fsys_for_block(path)); await client.mount_at(client.blocks[block_fsys.path], mount_point); diff --git a/pkg/storaged/crypto/actions.jsx b/pkg/storaged/crypto/actions.jsx index d6139c8283f9..ad7a8ec19ef5 100644 --- a/pkg/storaged/crypto/actions.jsx +++ b/pkg/storaged/crypto/actions.jsx @@ -23,7 +23,6 @@ import client from "../client"; import { get_existing_passphrase, unlock_with_type } from "./keyslots.jsx"; import { set_crypto_auto_option } from "../utils.js"; import { dialog_open, PassInput } from "../dialog.jsx"; -import { remember_passphrase } from "../anaconda.jsx"; const _ = cockpit.gettext; @@ -45,8 +44,7 @@ export function unlock(block) { Action: { Title: _("Unlock"), action: async function (vals) { - await crypto.Unlock(vals.passphrase, {}); - remember_passphrase(block, vals.passphrase); + await unlock_with_type(client, block, vals.passphrase, null); await set_crypto_auto_option(block, true); } } diff --git a/pkg/storaged/crypto/keyslots.jsx b/pkg/storaged/crypto/keyslots.jsx index 9a6962d6fbe9..5ce7f5b581d7 100644 --- a/pkg/storaged/crypto/keyslots.jsx +++ b/pkg/storaged/crypto/keyslots.jsx @@ -38,7 +38,10 @@ import { dialog_open, SelectOneRadio, TextInput, PassInput, Skip } from "../dialog.jsx"; -import { decode_filename, encode_filename, get_block_mntopts, block_name, for_each_async, get_children, parse_options, unparse_options, edit_crypto_config } from "../utils.js"; +import { + decode_filename, encode_filename, get_block_mntopts, block_name, for_each_async, get_children, + parse_options, extract_option, unparse_options, edit_crypto_config +} from "../utils.js"; import { StorageButton } from "../storage-controls.jsx"; import clevis_luks_passphrase_sh from "./clevis-luks-passphrase.sh"; @@ -72,22 +75,51 @@ export function clevis_recover_passphrase(block, just_type) { .then(output => output.trim()); } -function clevis_unlock(block) { +async function clevis_unlock(client, block, luksname, readonly) { const dev = decode_filename(block.Device); - 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; + + if (readonly) { + // HACK - clevis-luks-unlock can not unlock things readonly. + // But see https://github.com/latchset/clevis/pull/317 (merged + // Feb 2023, unreleased as of Feb 2024). + const passphrase = await clevis_recover_passphrase(block, false); + const crypto = client.blocks_crypto[block.path]; + const unlock_options = { "read-only": { t: "b", v: readonly } }; + await crypto.Unlock(passphrase, unlock_options); + return; + } + + await cockpit.spawn(["clevis", "luks", "unlock", "-d", dev, "-n", clear_dev], + { superuser: true }); } -export async function unlock_with_type(client, block, passphrase, passphrase_type) { +export async function unlock_with_type(client, block, passphrase, passphrase_type, override_readonly) { const crypto = client.blocks_crypto[block.path]; + let readonly = false; + let luksname = null; + + for (const c of block.Configuration) { + if (c[0] == "crypttab") { + const options = parse_options(decode_filename(c[1].options.v)); + readonly = extract_option(options, "readonly") || extract_option(options, "read-only"); + luksname = decode_filename(c[1].name.v); + break; + } + } + + if (override_readonly !== null && override_readonly !== undefined) + readonly = override_readonly; + + const unlock_options = { "read-only": { t: "b", v: readonly } }; + if (passphrase) { - await crypto.Unlock(passphrase, {}); + await crypto.Unlock(passphrase, unlock_options); remember_passphrase(block, passphrase); } else if (passphrase_type == "stored") { - await crypto.Unlock("", {}); + await crypto.Unlock("", unlock_options); } else if (passphrase_type == "clevis") { - await clevis_unlock(block); + await clevis_unlock(client, block, luksname, readonly); } else { // This should always be caught and should never show up in the UI throw new Error("No passphrase"); diff --git a/pkg/storaged/dialog.jsx b/pkg/storaged/dialog.jsx index c76a1b067817..fe5f64f1390c 100644 --- a/pkg/storaged/dialog.jsx +++ b/pkg/storaged/dialog.jsx @@ -863,11 +863,12 @@ export const SelectSpace = (tag, title, options) => { }; }; -const CheckBoxComponent = ({ tag, val, title, tooltip, update_function }) => { +const CheckBoxComponent = ({ tag, val, title, tooltip, disabled, update_function }) => { return ( {title} @@ -905,6 +906,7 @@ export const CheckBoxes = (tag, title, options) => { tag={ftag} val={fval} title={field.title} + disabled={field.disabled} tooltip={field.tooltip} options={options} update_function={fchange} />; diff --git a/pkg/storaged/filesystem/mounting-dialog.jsx b/pkg/storaged/filesystem/mounting-dialog.jsx index 3b6db3a8992f..2975f89509cb 100644 --- a/pkg/storaged/filesystem/mounting-dialog.jsx +++ b/pkg/storaged/filesystem/mounting-dialog.jsx @@ -52,7 +52,10 @@ export const mount_options = (opt_ro, extra_options, is_visible) => { extra: extra_options || false }, fields: [ - { title: _("Mount read only"), tag: "ro" }, + { + title: _("Mount read only"), + tag: "ro", + }, { title: _("Custom mount options"), tag: "extra", type: "checkboxWithInput" }, ] }); @@ -107,7 +110,7 @@ export function mounting_dialog(client, block, mode, forced_options, subvol) { const is_filesystem_mounted = is_mounted(client, block, subvol); - function maybe_update_config(new_dir, new_opts, passphrase, passphrase_type) { + function maybe_update_config(new_dir, new_opts, passphrase, passphrase_type, crypto_unlock_readonly) { let new_config = null; let all_new_opts; @@ -183,17 +186,31 @@ export function mounting_dialog(client, block, mode, forced_options, subvol) { } async function maybe_unlock() { - const crypto = client.blocks_crypto[block.path]; - if (mode == "mount" && crypto) { - try { - await unlock_with_type(client, block, passphrase, passphrase_type); - return await client.wait_for(() => client.blocks_cleartext[block.path]); - } catch (error) { - dlg.set_values({ needs_explicit_passphrase: true }); - throw error; + if (mode == "mount" || mode == "update") { + let crypto = client.blocks_crypto[block.path]; + const backing = client.blocks[block.CryptoBackingDevice]; + + if (backing && block.ReadOnly != crypto_unlock_readonly) { + // We are working on a open crypto device, but it + // has the wrong readonly-ness. Close it so that we can reopen it below. + crypto = client.blocks_crypto[backing.path]; + await crypto.Lock({}); } - } else - return block; + + if (crypto) { + try { + await unlock_with_type(client, client.blocks[crypto.path], + passphrase, passphrase_type, crypto_unlock_readonly); + return await client.wait_for(() => client.blocks_cleartext[crypto.path]); + } catch (error) { + passphrase_type = null; + dlg.set_values({ needs_explicit_passphrase: true }); + throw error; + } + } + } + + return block; } function maybe_lock() { @@ -255,18 +272,17 @@ export function mounting_dialog(client, block, mode, forced_options, subvol) { mode == "update", subvol) }), - mount_options(opt_ro, extra_options), + mount_options(opt_ro, extra_options, null), at_boot_input(at_boot), ]; - if (block.IdUsage == "crypto" && mode == "mount") - fields = fields.concat([ - PassInput("passphrase", _("Passphrase"), - { - visible: vals => vals.needs_explicit_passphrase, - validate: val => !val.length && _("Passphrase cannot be empty"), - }) - ]); + fields = fields.concat([ + PassInput("passphrase", _("Passphrase"), + { + visible: vals => vals.needs_explicit_passphrase, + validate: val => !val.length && _("Passphrase cannot be empty"), + }) + ]); } const mode_title = { @@ -313,6 +329,17 @@ export function mounting_dialog(client, block, mode, forced_options, subvol) { const usage = get_active_usage(client, block.path, null, null, false, subvol); + function update_explicit_passphrase(vals_ro) { + const backing = client.blocks[block.CryptoBackingDevice]; + let need_passphrase = (block.IdUsage == "crypto" && mode == "mount"); + if (backing) { + // XXX - take subvols into account. + if (block.ReadOnly != vals_ro) + need_passphrase = true; + } + dlg.set_values({ needs_explicit_passphrase: need_passphrase && !passphrase_type }); + } + const dlg = dialog_open({ Title: cockpit.format(mode_title[mode], old_dir_for_display), Fields: fields, @@ -320,6 +347,8 @@ export function mounting_dialog(client, block, mode, forced_options, subvol) { update: function (dlg, vals, trigger) { if (trigger == "at_boot") dlg.set_options("at_boot", { explanation: mount_explanation[vals.at_boot] }); + if (trigger == "mount_options") + update_explicit_passphrase(vals.mount_options.ro); }, Action: { Title: mode_action[mode], @@ -343,10 +372,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; return (maybe_update_config(client.add_mount_point_prefix(vals.mount_point), unparse_options(opts), vals.passphrase, - passphrase_type) + passphrase_type, + crypto_unlock_readonly) .then(() => maybe_set_crypto_options(vals.mount_options?.ro, opts.indexOf("noauto") == -1, vals.at_boot == "nofail", @@ -356,9 +388,10 @@ export function mounting_dialog(client, block, mode, forced_options, subvol) { }, Inits: [ init_active_usage_processes(client, usage, old_dir), - (block.IdUsage == "crypto" && mode == "mount") - ? init_existing_passphrase(block, true, type => { passphrase_type = type }) - : null + init_existing_passphrase(block, true, type => { + passphrase_type = type; + update_explicit_passphrase(dlg.get_value("mount_options")?.ro ?? opt_ro); + }), ] }); } diff --git a/test/verify/check-storage-luks b/test/verify/check-storage-luks index 664a15cacd38..e7fbaf3b1d87 100755 --- a/test/verify/check-storage-luks +++ b/test/verify/check-storage-luks @@ -166,6 +166,20 @@ class TestStorageLuks(storagelib.StorageCase): self.assert_in_configuration(dev, "crypttab", "options", "readonly") self.assert_in_configuration(cleartext_dev, "fstab", "opts", "ro") + # Check that the clear text device is actually readonly + self.assertEqual(m.execute(f"lsblk -no RO {cleartext_dev}").strip(), "1") + + # Change "read only" mount option. This should reopen LUKS as + # necessary and ask for a passphrase. + b.click(self.card_desc("ext4 filesystem", "Mount point") + " button") + self.dialog_wait_open() + self.dialog_set_val("mount_options.ro", val=False) + self.dialog_set_val("passphrase", "vainu-reku-toma-rolle-kaja") + self.dialog_apply() + self.dialog_wait_close() + + self.assertEqual(m.execute(f"lsblk -no RO {cleartext_dev}").strip(), "0") + # Delete the partition. self.click_card_dropdown("Partition", "Delete") self.confirm() @@ -186,6 +200,54 @@ class TestStorageLuks(storagelib.StorageCase): # FIXME: race condition after unmounting; hard to investigate, re-check after storage redesign self.allow_browser_errors("validateDOMNesting.*cannot appear as a child.* ul") + def testFormatReadOnly(self): + m = self.machine + b = self.browser + + self.login_and_go("/storage") + + disk = self.add_ram_disk(400) + self.click_card_row("Storage", name=disk) + + # Create a encrypted filesystem and mount it readonly. This + # needs to reopen the LUKS layer after formatting. + + self.click_card_dropdown("Unformatted data", "Format") + self.dialog({"type": "ext4", + "mount_point": "/run/foo", + "mount_options.ro": True, + "crypto": "luks1", + "passphrase": "vainu-reku-toma-rolle-kaja", + "passphrase2": "vainu-reku-toma-rolle-kaja"}) + b.wait_visible(self.card("ext4 filesystem")) + + uuid = m.execute(f"cryptsetup luksUUID {disk}").strip() + cleartext_dev = "/dev/mapper/luks-" + uuid + + self.assert_in_configuration(disk, "crypttab", "options", "readonly") + self.assert_in_configuration(cleartext_dev, "fstab", "opts", "ro") + self.assertEqual(m.execute(f"lsblk -no RO {cleartext_dev}").strip(), "1") + + # Reformat it, while keeping the encryption layer and keeping + # it read-only. This needs to reopen LUKS twice, once to make + # the cleartext device writable for formatting, and once to + # make it read-only again, as requested. + + self.click_card_dropdown("ext4 filesystem", "Format") + self.dialog_wait_open() + self.dialog_wait_val("mount_point", "/run/foo") + self.dialog_wait_val("type", "ext4") + self.dialog_wait_val("crypto", " keep") + self.dialog_wait_val("crypto_options", "") + b.wait_visible(self.dialog_field("mount_options.ro") + ":checked") + self.dialog_set_val("type", "xfs") + self.dialog_set_val("old_passphrase", "vainu-reku-toma-rolle-kaja") + self.dialog_apply() + self.dialog_wait_close() + b.wait_visible(self.card("xfs filesystem")) + + self.assertEqual(m.execute(f"lsblk -no RO {cleartext_dev}").strip(), "1") + def testNoFsys(self): m = self.machine b = self.browser @@ -223,12 +285,10 @@ class TestStorageLuks(storagelib.StorageCase): b.click(self.card_button("Locked data", "Unlock")) self.dialog({"passphrase": "vainu-reku-toma-rolle-kaja"}) - # Try to format it, just for kicks - self.click_card_dropdown("Unformatted data", "Format") - self.dialog({"type": "ext4", - "mount_point": "/run/foo"}) - b.wait_visible(self.card("ext4 filesystem")) - self.wait_mounted("ext4 filesystem") + # Should be unlocked readonly + uuid = m.execute(f"cryptsetup luksUUID {disk}").strip() + cleartext_dev = "/dev/mapper/luks-" + uuid + self.assertEqual(m.execute(f"lsblk -no RO {cleartext_dev}").strip(), "1") # Now create a empty, encrypted partition self.click_card_dropdown("Solid State Drive", "Create partition table") @@ -548,6 +608,24 @@ class TestStorageNBDE(storagelib.StorageCase, packagelib.PackageCase): self.dialog_wait_close() self.wait_mounted("ext4 filesystem") + # LUKS should be read/write + uuid = m.execute(f"cryptsetup luksUUID {dev}").strip() + cleartext_dev = "/dev/mapper/luks-" + uuid + self.assertEqual(m.execute(f"lsblk -no RO {cleartext_dev}").strip(), "0") + + # Remount "readonly", LUKS should be readonly + b.click(self.card_button("ext4 filesystem", "Unmount")) + self.confirm() + b.click(self.card_button("Filesystem", "Mount")) + self.dialog_wait_open() + self.dialog_set_val("mount_options.ro", val=True) + self.dialog_apply() + self.dialog_wait_close() + self.wait_mounted("ext4 filesystem") + + # Now LUKS should be readonly + self.assertEqual(m.execute(f"lsblk -no RO {cleartext_dev}").strip(), "1") + # Edit the key, without providing an existing passphrase # b.click(panel + "ul li:nth-child(2) [aria-label='Edit']")