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: Readonly crypto improvements #20148

Merged
merged 4 commits into from
Mar 26, 2024
Merged

storage: Readonly crypto improvements #20148

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9a6f8ea
storage: Allow both "readonly" and "read-only" spelling in crypttab
mvollmer Mar 6, 2024
c3439be
storage: Always set config on cleartext block during mounting
mvollmer Mar 6, 2024
88e5a75
storage: Disable "Format" action for read-only devices
mvollmer Mar 7, 2024
f1d0e23
storage: Correctly maintain actual LUKS device readonly-ness
mvollmer Mar 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 29 additions & 9 deletions pkg/storaged/block/format-dialog.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,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);

Expand Down Expand Up @@ -230,6 +230,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,
Expand Down Expand Up @@ -438,6 +440,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")
Expand Down Expand Up @@ -516,17 +520,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;
Comment on lines +537 to +539
Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Member Author

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.

}
}

function format() {
Expand Down Expand Up @@ -588,6 +599,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);
Expand Down
4 changes: 1 addition & 3 deletions pkg/storaged/crypto/actions.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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);
}
}
Expand Down
54 changes: 44 additions & 10 deletions pkg/storaged/crypto/keyslots.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import cockpit from "cockpit";
import React from "react";
import client from "../client.js";

import { CardBody, CardHeader, CardTitle } from '@patternfly/react-core/dist/esm/components/Card/index.js';
import { Checkbox } from "@patternfly/react-core/dist/esm/components/Checkbox/index.js";
Expand All @@ -38,7 +39,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";
Expand Down Expand Up @@ -72,22 +76,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;
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.


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) {
Copy link
Member

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?

Copy link
Member Author

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".

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");
Expand Down Expand Up @@ -187,7 +220,8 @@ export function init_existing_passphrase(block, just_type, callback) {
return {
title: _("Unlocking disk"),
func: dlg => {
return get_existing_passphrase(block, just_type).then(passphrase => {
const backing = client.blocks[block.CryptoBackingDevice];
return get_existing_passphrase(backing || block, just_type).then(passphrase => {
if (!passphrase)
dlg.set_values({ needs_explicit_passphrase: true });
if (callback)
Expand Down
4 changes: 3 additions & 1 deletion pkg/storaged/dialog.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
<Checkbox data-field={tag} data-field-type="checkbox"
id={tag}
isChecked={val}
isDisabled={disabled}
label={
<>
{title}
Expand Down Expand Up @@ -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} />;
Expand Down
87 changes: 61 additions & 26 deletions pkg/storaged/filesystem/mounting-dialog.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,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" },
]
});
Expand Down Expand Up @@ -168,7 +171,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;

Expand Down Expand Up @@ -244,17 +247,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" && is_filesystem_mounted)) {
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() {
Expand Down Expand Up @@ -316,18 +333,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"),
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.

})
]);
}

const mode_title = {
Expand Down Expand Up @@ -374,11 +390,26 @@ 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,
Teardown: TeardownMessage(usage, old_dir),
update: update_at_boot_input,
update: function (dlg, vals, trigger) {
update_at_boot_input(dlg, vals, trigger);
if (trigger == "mount_options")
update_explicit_passphrase(vals.mount_options.ro);
},
Action: {
Title: mode_action[mode],
disable_on_error: usage.Teardown,
Expand All @@ -401,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;
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.

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",
Expand All @@ -414,9 +448,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);
}),
]
});
}
Loading