support s390 secure boot (jsc#SLE-9425) #592

wfeldt · 2020-02-21T14:21:12Z

Task

Implement secure boot handling for s390.

Implementation

extend existing secure boot logic to s390
bundle a number of system-dependend logic around secure boot into a new Systeminfo class
use /etc/sysconfig/bootloader::SECURE_BOOT to store the setting; grub2-install is expected to read it from there
the secure boot setting can be changed directly from the installation summary screen
to match it visually, the same has been done for trusted boot
when the secure boot setting is changed, a popup is shown on s390 to remind the admin to
adjust the HMC
on s390, start with the currently active secure boot setting; not the value from sysconfig; this way the user doesn't run into problems when the sysconfig value and the current state disagree

Note

The implementation is incomplete as there's no info on the tools to use to handle secure boot
from the s390 side so far. So the current implementation does

assume every s390 system can do secure boot, and
that the setting is always off

Notes on `auto` setting

see jsc#SLE-9425

Todo

some extensive integration testing

Screenshots

This extends the existing secure boot logic to s390. A number of system-dependend logic has been moved into a new Systeminfo class.

coveralls · 2020-02-21T14:23:50Z

Coverage increased (+0.08%) to 83.957% when pulling 3f71a70 on sw_01 into 9292963 on master.

jreidinger · 2020-02-26T11:24:44Z

src/lib/bootloader/grub2.rb

        )
      ]
+
+      result.push secure_boot_summary if Systeminfo.secure_boot_available?(name)


just style, usually it is used << instead of use.

That is needed as ProposalClient expects a unified view of the bootloader class it works on.

jreidinger · 2020-02-28T13:16:34Z

src/lib/bootloader/grub2_widgets.rb

+    def validate
+      return true if Yast::Mode.config ||
+        !Yast::Arch.s390 ||
+        value == Systeminfo.secure_boot_active?


NP: I find a bit more readable to have it each on separate line and separate return as it is not related. So something like

Suggested change

value == Systeminfo.secure_boot_active?

return true if Yast::Mode.config

return true unless Yast::Arch.s390

return true if value == Systeminfo.secure_boot_active?

I personally wouldn't have formatted it this way. But this is what our rubocop rules allow/enforce. To completely reformat it might even run into some complexity check failures.

jreidinger · 2020-02-28T13:18:55Z

src/lib/bootloader/grub2_widgets.rb

+      Yast::Popup.ContinueCancel(
+        _(
+          "Make sure the Secure Boot setting matches the configuration of the HMC.\n\n" \
+          "Otherwise this system will not boot."


Well, buttons are continue/cancel. So I expect here a bit hint to user what when they click it. Like Cancel to edit it or Continue to use setting.

You mean like here?:

https://github.com/yast/yast-bootloader/blob/master/src/lib/bootloader/grub2_widgets.rb#L388-L390

I've never seen a dialog like 'continue to continue' or 'cancel to cancel'. That's why there's a button text.

jreidinger · 2020-02-28T13:20:29Z

src/lib/bootloader/grub2_widgets.rb

+          "Make sure the Secure Boot setting matches the configuration of the HMC.\n\n" \
+          "Otherwise this system will not boot."
+        )
+      )


One more usability note. There is method focus which is often used in validate to "highlight" given widget. So here it can be something like

res = Yast::Popup.ContinueCancel(...) focus unless res res

https://github.com/yast/yast-yast2/blob/master/library/cwm/src/lib/cwm/abstract_widget.rb#L187

Again, this just mirrors the trusted boot dialog. Consistency across a UI is an important point. It doesn't help to do similar things in a completely different way.

jreidinger · 2020-02-28T13:24:51Z

src/lib/bootloader/grub2base.rb

    # @!attribute console
    #   @return [::Bootloader::SerialConsole] serial console or nil if none
    attr_reader :console

+    # @!attribute stage1
+    #   @return [::Bootloader::Stage1, nil] bootloader stage1, if one is needed
+    attr_reader :stage1


this is a bit strange as grub2EFI does not have stage1. So original intention is to have it only in grub2.rb and if something needs to depend on it, it should check bootloader or ask if method is defined.

That's a valid point and I considered doing it that way. The usage is here:

https://github.com/yast/yast-bootloader/blob/master/src/lib/bootloader/proposal_client.rb#L338-L339

I my view this is a matter of class design. This proposal client code should work with any bootloader. So, basically with Grub2Base. That's why I decided to move the stage1 method.

Inconsistent class interfaces are imho one of the reasons yast code so easily runs into exceptions.

jreidinger · 2020-02-28T13:33:48Z

src/lib/bootloader/proposal_client.rb

      log.info "single_click_action #{option} #{value.inspect} #{devices}"

-      devices.each do |device|
+      devices&.each do |device|


this starting to feel a bit too hacky, as it is related only to "boot_" arguments. What about moving it inside case like:

Suggested change

devices&.each do |device|

bootloader = ::Bootloader::BootloaderFactory.current

case option

when /boot_/

stage1 = bootloader.stage1

devices = option == "boot_mbr" ? stage1.boot_disk_names : stage1.boot_partition_names

log.info "single_click_action #{option} #{value.inspect} #{devices}"

devices.each do |device|

value ? stage1.add_udev_device(device) : stage1.remove_device(device)

end

when "trusted_boot"

bootloader.trusted_boot = value

when "secure_boot"

bootloader.secure_boot = value

end

Yast::Bootloader.proposed_cfg_changed = true

end

jreidinger · 2020-02-28T13:34:23Z

src/lib/bootloader/proposal_client.rb

+        ::Bootloader::BootloaderFactory.current.trusted_boot = value
+      when "secure_boot"
+        ::Bootloader::BootloaderFactory.current.secure_boot = value
+        if value && Yast::Arch.s390


in validation it checks when there is a mismatch, but here it shows always when enabling. Why conditions are different?

Because it is initially always off. Note also the different texts.

jreidinger · 2020-02-28T13:36:05Z

src/lib/bootloader/systeminfo.rb

+      #
+      # @return [Boolean] true if 390x machine has secure boot enabled
+      def s390_secure_boot_active?
+        # FIXME: this is just a stub - replace with real code later


well, I am not sure I want to approve with stub as I worry it will live here for another 20 years :)

This is what was agreed on for the current task. There will be another PBI once we get the missing info.

This extends the existing secure boot logic to s390. A number of system-dependend logic has been moved into a new Systeminfo class.

That is needed as ProposalClient expects a unified view of the bootloader class it works on.

yast-bot · 2020-02-28T15:26:54Z

✔️ Internal Jenkins job #20 successfully finished
✔️ Created IBS submit request #212842

yast-bot · 2020-02-29T00:22:21Z

✔️ Public Jenkins job #46 successfully finished
✔️ Created OBS submit request #780419

support s390 secure boot (jsc#SLE-9425)

637ed78

This extends the existing secure boot logic to s390. A number of system-dependend logic has been moved into a new Systeminfo class.

wfeldt added 2 commits February 21, 2020 16:27

adjust functions and update unit tests

069a2af

add autoyast support

332e6a2

jreidinger reviewed Feb 26, 2020

View reviewed changes

wfeldt added 6 commits February 27, 2020 10:44

minor corrections

d36f9c0

move secure boot handling into Grub2Base class

a945d01

adjust unit tests

9a36027

move #stage1 method into Grub2Base

cab5d9e

That is needed as ProposalClient expects a unified view of the bootloader class it works on.

update documentation

3ccb1a5

added unit tests for Systeminfo class

7c623bc

wfeldt marked this pull request as ready for review February 27, 2020 15:35

jreidinger reviewed Feb 28, 2020

View reviewed changes

adjust code

9ba850a

jreidinger approved these changes Feb 28, 2020

View reviewed changes

minor optimization

4c16d63

wfeldt force-pushed the sw_01 branch from 4b04317 to 4c16d63 Compare February 28, 2020 14:22

wfeldt added 7 commits February 28, 2020 15:22

support s390 secure boot (jsc#SLE-9425)

400db5e

This extends the existing secure boot logic to s390. A number of system-dependend logic has been moved into a new Systeminfo class.

adjust functions and update unit tests

3163414

add autoyast support

ae55c68

minor corrections

107d559

move secure boot handling into Grub2Base class

03e81ec

adjust unit tests

9d9ca8e

move #stage1 method into Grub2Base

b02f3e7

That is needed as ProposalClient expects a unified view of the bootloader class it works on.

wfeldt added 6 commits February 28, 2020 15:22

update documentation

bb791df

added unit tests for Systeminfo class

1ab6d85

adjust code

4ad76c8

minor optimization

e702c6f

Merge branch 'sw_01' of github.com:yast/yast-bootloader into sw_01

b2fa0cb

release 4.2.17

3f71a70

wfeldt merged commit ef272ca into master Feb 28, 2020

wfeldt deleted the sw_01 branch February 28, 2020 14:32

wfeldt mentioned this pull request Mar 23, 2020

support s390 secure boot (jsc#SLE-9425, jsc#SLE-9471, bsc#1166736) #594

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support s390 secure boot (jsc#SLE-9425) #592

support s390 secure boot (jsc#SLE-9425) #592

wfeldt commented Feb 21, 2020 •

edited

coveralls commented Feb 21, 2020 •

edited

jreidinger Feb 26, 2020

jreidinger Feb 28, 2020

wfeldt Feb 28, 2020

jreidinger Feb 28, 2020

wfeldt Feb 28, 2020

jreidinger Feb 28, 2020

wfeldt Feb 28, 2020

jreidinger Feb 28, 2020

wfeldt Feb 28, 2020

jreidinger Feb 28, 2020

jreidinger Feb 28, 2020

wfeldt Feb 28, 2020

jreidinger Feb 28, 2020

wfeldt Feb 28, 2020

yast-bot commented Feb 28, 2020

yast-bot commented Feb 29, 2020

-      devices&.each do |device|
+      bootloader = ::Bootloader::BootloaderFactory.current
+      case option
+      when /boot_/
+        stage1 = bootloader.stage1
+        devices = option == "boot_mbr" ? stage1.boot_disk_names : stage1.boot_partition_names
+        log.info "single_click_action #{option} #{value.inspect} #{devices}"
+        devices.each do |device|
+          value ? stage1.add_udev_device(device) : stage1.remove_device(device)
+        end
+      when "trusted_boot"
+        bootloader.trusted_boot = value
+      when "secure_boot"
+        bootloader.secure_boot = value
+      end
+      Yast::Bootloader.proposed_cfg_changed = true
+    end

support s390 secure boot (jsc#SLE-9425) #592

support s390 secure boot (jsc#SLE-9425) #592

Conversation

wfeldt commented Feb 21, 2020 • edited

Task

Implementation

Note

Notes on auto setting

Todo

Screenshots

coveralls commented Feb 21, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yast-bot commented Feb 28, 2020

yast-bot commented Feb 29, 2020

wfeldt commented Feb 21, 2020 •

edited

Notes on `auto` setting

coveralls commented Feb 21, 2020 •

edited