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.

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

Make TPM-based encryption more explicit #995

Merged
merged 9 commits into from
Jan 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 1 addition & 3 deletions products.d/ALP-Dolomite.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,7 @@ security:
storage:
space_policy: delete
encryption:
method: luks2
pbkd_function: pbkdf2
tpm_luks_open: true
method: tpm_fde
volumes:
- "/"
volume_templates:
Expand Down
12 changes: 11 additions & 1 deletion service/lib/agama/dbus/storage/manager.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
require "agama/dbus/storage/with_iscsi_auth"
require "agama/dbus/with_service_status"
require "agama/storage/volume_templates_builder"
require "agama/storage/encryption_settings"

Yast.import "Arch"

Expand Down Expand Up @@ -117,13 +118,20 @@ def available_devices
proposal.available_devices.map { |d| system_devices_tree.path_for(d) }
end

# List of meaningful mount points for the the current product.
# List of meaningful mount points for the current product.
#
# @return [Array<String>]
def product_mount_points
volume_templates_builder.all.map(&:mount_path).reject(&:empty?)
end

# List of possible encryption methods for the current system and product
#
# @return [Array<String>]
def encryption_methods
Agama::Storage::EncryptionSettings.available_methods.map { |m| m.id.to_s }
end

# Path of the D-Bus object containing the calculated proposal
#
# @return [::DBus::ObjectPath] Proposal object path or root path if no exported proposal yet
Expand Down Expand Up @@ -161,6 +169,8 @@ def calculate_proposal(dbus_settings)

dbus_reader :product_mount_points, "as"

dbus_reader :encryption_methods, "as"

dbus_reader :result, "o"

dbus_method :DefaultVolume, "in mount_path:s, out volume:a{sv}" do |mount_path|
Expand Down
31 changes: 30 additions & 1 deletion service/lib/agama/storage/encryption_settings.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,13 @@ module Storage
class EncryptionSettings
include Y2Storage::SecretAttributes

# @see .encryption_methods
METHODS = [
Y2Storage::EncryptionMethod::LUKS2,
Y2Storage::EncryptionMethod::TPM_FDE
].freeze
private_constant :METHODS

# @!attribute encryption_password
# Password to use when creating new encryption devices
# @return [String, nil] nil if undetermined
Expand All @@ -41,8 +48,30 @@ class EncryptionSettings
# @return [Y2Storage::PbkdFunction, nil] Can be nil if using LUKS1.
attr_accessor :pbkd_function

# All known encryption methods
#
# This includes all the potentially accepted methods, no matter whether they are
# possible for the current system and product.
#
# @return [Array<Y2Storage::EncryptionMethod::Base>]
def self.encryption_methods
METHODS
end

# All methods that can be used to calculate a proposal for the current system and product
#
# @return [Array<Y2Storage::EncryptionMethod::Base>]
def self.available_methods
encryption_methods.reject { |m| m.respond_to?(:possible?) && !m.possible? }
end

# Constructor
def initialize
@method = Y2Storage::EncryptionMethod::LUKS1
# LUKS2 with PBKDF2 is the most sensible option nowadays:
# - LUKS2 offers additional features over LUKS1
# - PBKDF2 keeps memory usage similar to LUKS1 and is supported by Grub2
@method = Y2Storage::EncryptionMethod::LUKS2
@pbkd_function = Y2Storage::PbkdFunction::PBKDF2
end

# Whether the proposal must create encrypted devices
Expand Down
17 changes: 6 additions & 11 deletions service/lib/agama/storage/proposal_settings_reader.rb
Original file line number Diff line number Diff line change
Expand Up @@ -69,24 +69,19 @@ def lvm_reader(settings, value)
# @param settings [Agama::Storage::ProposalSettings]
# @param encryption [Hash]
def encryption_reader(settings, encryption)
method =
if try_tpm_fde?(encryption)
Y2Storage::EncryptionMethod::TPM_FDE
else
Y2Storage::EncryptionMethod.find(encryption.fetch("method", ""))
end
method = Y2Storage::EncryptionMethod.find(encryption.fetch("method", ""))
pbkd_function = Y2Storage::PbkdFunction.find(encryption.fetch("pbkd_function", ""))

settings.encryption.method = method if method
settings.encryption.method = method if available_method?(method)
settings.encryption.pbkd_function = pbkd_function if pbkd_function
end

# @param encryption [Hash]
# @param method [Y2Storage::EncryptionMethod::Base, nil]
# @return [Boolean]
def try_tpm_fde?(encryption)
return false unless encryption["tpm_luks_open"] == true
def available_method?(method)
return false unless method
Copy link
Contributor

Choose a reason for hiding this comment

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

NP: this condition should be already covered by #include?.


Y2Storage::EncryptionMethod::TPM_FDE.possible?
EncryptionSettings.available_methods.include?(method)
end

# @param settings [Agama::Storage::ProposalSettings]
Expand Down
6 changes: 6 additions & 0 deletions service/package/rubygem-agama.changes
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
-------------------------------------------------------------------
Thu Jan 18 08:35:01 UTC 2024 - Ancor Gonzalez Sosa <ancor@suse.com>

- New default encryption settings: LUKS2 with PBKDF2.
- Expose encryption methods at D-Bus API (gh#openSUSE/agama#995).

-------------------------------------------------------------------
Tue Jan 16 10:49:14 UTC 2024 - Michal Filka <mfilka@suse.com>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@
"LVM" => false,
"SystemVGDevices" => [],
"EncryptionPassword" => "",
"EncryptionMethod" => "luks1",
"EncryptionPBKDFunction" => "",
"EncryptionMethod" => "luks2",
"EncryptionPBKDFunction" => "pbkdf2",
"SpacePolicy" => "keep",
"SpaceActions" => {},
"Volumes" => []
Expand Down
12 changes: 6 additions & 6 deletions service/test/agama/storage/proposal_settings_reader_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,8 @@
),
encryption: an_object_having_attributes(
password: nil,
method: Y2Storage::EncryptionMethod::LUKS1,
pbkd_function: nil
method: Y2Storage::EncryptionMethod::LUKS2,
pbkd_function: Y2Storage::PbkdFunction::PBKDF2
),
space: an_object_having_attributes(
policy: :keep,
Expand All @@ -127,13 +127,13 @@

expect(settings).to have_attributes(
encryption: an_object_having_attributes(
method: Y2Storage::EncryptionMethod::LUKS1
method: Y2Storage::EncryptionMethod::LUKS2
)
)
end
end

context "when the config contains an unknown encryption function" do
context "when the config contains an unknown password derivation function" do
let(:config_data) do
{
"storage" => {
Expand All @@ -144,12 +144,12 @@
}
end

it "does not set a PBKD function" do
it "sets the default derivation function" do
settings = subject.read

expect(settings).to have_attributes(
encryption: an_object_having_attributes(
pbkd_function: be_nil
pbkd_function: Y2Storage::PbkdFunction::PBKDF2
)
)
end
Expand Down
5 changes: 5 additions & 0 deletions web/package/cockpit-agama.changes
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
-------------------------------------------------------------------
Thu Jan 18 08:33:52 UTC 2024 - Ancor Gonzalez Sosa <ancor@suse.com>

- Make TPM-based encryption more explicit (gh#openSUSE/agama#995)

-------------------------------------------------------------------
Tue Jan 16 15:27:28 UTC 2024 - José Iván López González <jlopez@suse.com>

Expand Down
14 changes: 14 additions & 0 deletions web/src/assets/styles/blocks.scss
Original file line number Diff line number Diff line change
Expand Up @@ -430,3 +430,17 @@ ul[data-of="agama/timezones"] {
}
}
}

.tpm-hint {
container-type: inline-size;
container-name: tpm-info;
text-align: start;

.pf-v5-c-alert__title {
margin-block-end: var(--spacer-small);
}

.pf-v5-c-alert__description {
max-inline-size: 100%;
}
}
2 changes: 2 additions & 0 deletions web/src/assets/styles/layout.scss
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@
grid-area: body;
overflow: auto;
padding-block: var(--spacer-normal);
container-type: inline-size;
container-name: agama-page-content;
}

footer {
Expand Down
30 changes: 27 additions & 3 deletions web/src/client/storage.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) [2022-2023] SUSE LLC
* Copyright (c) [2022-2024] SUSE LLC
*
* All Rights Reserved.
*
Expand Down Expand Up @@ -46,6 +46,17 @@ const ZFCP_CONTROLLER_IFACE = "org.opensuse.Agama.Storage1.ZFCP.Controller";
const ZFCP_DISKS_NAMESPACE = "/org/opensuse/Agama/Storage1/zfcp_disks";
const ZFCP_DISK_IFACE = "org.opensuse.Agama.Storage1.ZFCP.Disk";

/**
* Enum for the encryption method values
*
* @readonly
* @enum { string }
*/
const EncryptionMethods = Object.freeze({
LUKS2: "luks2",
TPM: "tpm_fde"
});

Comment on lines +49 to +59
Copy link
Contributor

Choose a reason for hiding this comment

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

As I wrote in the commit message, following here the same approac as the current network code. We could, however, goes for what we did with client/phase or client/status instead and move it to a client/storage-encryption-methods.js or client/storage/encryption-methods.js

export const LUKS2 = "luks2";
export const TPM = "tpm_fde";

export default {
  LUKS2,
  TPM
};

and then import needed values

import { TPM } from "~/client/storage/encryption-methods"
import * as EncriptionMethods from "~/client/storage/encryption-methods"

No strong/clear opinion. Something to think about for future enhancements.

/**
* Removes properties with undefined value
*
Expand Down Expand Up @@ -227,6 +238,7 @@ class ProposalManager {
* @typedef {object} ProposalSettings
* @property {string} bootDevice
* @property {string} encryptionPassword
* @property {string} encryptionMethod
* @property {boolean} lvm
* @property {string} spacePolicy
* @property {string[]} systemVGDevices
Expand Down Expand Up @@ -283,6 +295,16 @@ class ProposalManager {
return proxy.ProductMountPoints;
}

/**
* Gets the list of encryption methods accepted by the proposal
*
* @returns {Promise<string[]>}
*/
async getEncryptionMethods() {
const proxy = await this.proxies.proposalCalculator;
return proxy.EncryptionMethods;
}

/**
* Obtains the default volume for the given mount path
*
Expand Down Expand Up @@ -345,6 +367,7 @@ class ProposalManager {
spacePolicy: proxy.SpacePolicy,
systemVGDevices: proxy.SystemVGDevices,
encryptionPassword: proxy.EncryptionPassword,
encryptionMethod: proxy.EncryptionMethod,
volumes: proxy.Volumes.map(this.buildVolume),
// NOTE: strictly speaking, installation devices does not belong to the settings. It
// should be a separate method instead of an attribute in the settings object.
Expand All @@ -365,7 +388,7 @@ class ProposalManager {
* @param {ProposalSettings} settings
* @returns {Promise<number>} 0 on success, 1 on failure
*/
async calculate({ bootDevice, encryptionPassword, lvm, spacePolicy, systemVGDevices, volumes }) {
async calculate({ bootDevice, encryptionPassword, encryptionMethod, lvm, spacePolicy, systemVGDevices, volumes }) {
const dbusVolume = (volume) => {
return removeUndefinedCockpitProperties({
MountPath: { t: "s", v: volume.mountPath },
Expand All @@ -381,6 +404,7 @@ class ProposalManager {
const settings = removeUndefinedCockpitProperties({
BootDevice: { t: "s", v: bootDevice },
EncryptionPassword: { t: "s", v: encryptionPassword },
EncryptionMethod: { t: "s", v: encryptionMethod },
LVM: { t: "b", v: lvm },
SpacePolicy: { t: "s", v: spacePolicy },
SystemVGDevices: { t: "as", v: systemVGDevices },
Expand Down Expand Up @@ -1420,4 +1444,4 @@ class StorageClient extends WithIssues(
), STORAGE_OBJECT
) { }

export { StorageClient };
export { StorageClient, EncryptionMethods };
Loading