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

New logic to type LUKS passphrase in grub phase #18270

Merged
merged 1 commit into from Dec 13, 2023

Conversation

@rfan1 rfan1 added WIP Work in progress qe-core Tag used to filter PR's created by QE-Core's members or are assigned to them labels Dec 7, 2023
@rfan1 rfan1 removed the WIP Work in progress label Dec 7, 2023
@lemon-suse
Copy link
Contributor

LGTM.

@rfan1
Copy link
Contributor Author

rfan1 commented Dec 7, 2023

BTW, for failed case https://openqa.suse.de/tests/12948511#step/boot_to_desktop/10, we can use the similar logic:

diff --git a/lib/opensusebasetest.pm b/lib/opensusebasetest.pm
index efa387f8d..abb114df5 100644
--- a/lib/opensusebasetest.pm
+++ b/lib/opensusebasetest.pm
@@ -906,8 +906,13 @@ sub wait_boot {
     reconnect_xen if check_var('VIRSH_VMM_FAMILY', 'xen');
 
     # on s390x svirt encryption is unlocked with workaround_type_encrypted_passphrase before here
-    unlock_if_encrypted unless get_var('S390_ZKVM');
-
+    # with newer grub2 (in TW and SLE15-SP6 currently), entering the passphrase in GRUB2
+    # is enough. The key is passed on during boot, so it's not asked for
+    # a second time.
+    my $need_to_enter_passphrase = is_leap || is_sle('<15-sp6') || is_leap_micro || is_sle_micro || is_alp || check_var('LVM', 0);
+    if ($need_to_enter_passphrase) {
+        unlock_if_encrypted unless get_var('S390_ZKVM');
+    }

However, it might introduce some expected result since all things are controlled via setting

Copy link
Member

@Vogtinator Vogtinator left a comment

Choose a reason for hiding this comment

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

This is a product bug: https://bugzilla.suse.com/show_bug.cgi?id=1205314

With my suggestion, unset LVM is treated as LVM=0.

tests/installation/boot_encrypt.pm Outdated Show resolved Hide resolved
@Vogtinator
Copy link
Member

BTW, for failed case https://openqa.suse.de/tests/12948511#step/boot_to_desktop/10, we can use the similar logic:

See https://progress.opensuse.org/issues/117811#note-29.

@rfan1
Copy link
Contributor Author

rfan1 commented Dec 7, 2023

BTW, for failed case https://openqa.suse.de/tests/12948511#step/boot_to_desktop/10, we can use the similar logic:

See https://progress.opensuse.org/issues/117811#note-29.

Thanks, then need to double check /boot partition is encrypted or not

@rfan1 rfan1 force-pushed the boot_encrypt branch 2 times, most recently from c2f16c9 to b60b8f6 Compare December 8, 2023 07:54
Copy link

github-actions bot commented Dec 8, 2023

Great PR! Please pay attention to the following items before merging:

Files matching lib/**.pm:

  • Consider adding or extending unit tests in t/

This is an automatically generated QA checklist based on modified files.

@rfan1 rfan1 added the WIP Work in progress label Dec 8, 2023
@rfan1
Copy link
Contributor Author

rfan1 commented Dec 8, 2023

Change the code to handle boot_to_desktop as well

@rfan1 rfan1 added WIP Work in progress and removed WIP Work in progress labels Dec 8, 2023
@rfan1 rfan1 removed the WIP Work in progress label Dec 8, 2023
@rfan1
Copy link
Contributor Author

rfan1 commented Dec 11, 2023

@Vogtinator Can I ask for your kindly help to review my PR again?

lib/utils.pm Outdated Show resolved Hide resolved
lib/utils.pm Outdated Show resolved Hide resolved
lib/utils.pm Outdated Show resolved Hide resolved
@rfan1
Copy link
Contributor Author

rfan1 commented Dec 12, 2023

The latest VRs:

sle_yast
staging
kernel_install
kernel_boot_to_desktop

@rfan1 rfan1 removed the WIP Work in progress label Dec 12, 2023
lib/utils.pm Outdated Show resolved Hide resolved
Copy link
Member

@nilxam nilxam left a comment

Choose a reason for hiding this comment

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

we should follow SLE's condition on Leap like to use is_leap('<15.6') here

@czerw
Copy link
Contributor

czerw commented Dec 13, 2023

Could you please add two verification runs as well: installation (maybe is similar) https://openqa.suse.de/tests/13004386 and job https://openqa.suse.de/tests/13004389 which uses that image and it needs to enter password as well.

@Vogtinator
Copy link
Member

With this, workaround_type_encrypted_passphrase can be renamed to unlock_bootloader and all workaround references dropped.

@rfan1
Copy link
Contributor Author

rfan1 commented Dec 13, 2023

With this, workaround_type_encrypted_passphrase can be renamed to unlock_bootloader and all workaround references dropped.

Agree with you!

lib/utils.pm Outdated Show resolved Hide resolved
lib/utils.pm Outdated Show resolved Hide resolved
@rfan1 rfan1 force-pushed the boot_encrypt branch 3 times, most recently from 1db3bb1 to 34f3321 Compare December 13, 2023 12:06
@rfan1 rfan1 merged commit 6258615 into os-autoinst:master Dec 13, 2023
8 checks passed
@DimStar77
Copy link
Contributor

First new test fails are incoming:

@rfan1
Copy link
Contributor Author

rfan1 commented Dec 14, 2023

First new test fails are incoming:

Thanks,

In this job, only /boot partition is encrypted, so it leads to another logic, I think we can un-schedule the test module boot_encrypt here, can you please file a ticket and assign it to me?

@Vogtinator
Copy link
Member

IMO having an encrypted /boot and plain rest just does not make sense whatsoever and that scenario can just be removed.

@rfan1
Copy link
Contributor Author

rfan1 commented Dec 14, 2023

IMO having an encrypted /boot and plain rest just does not make sense whatsoever and that scenario can just be removed.

It should be introduced from ticket, https://progress.opensuse.org/issues/81780. I will ask qe-security team to double check!
@tjyrinki @paolostivanin

@DimStar77
Copy link
Contributor

IMO having an encrypted /boot and plain rest just does not make sense whatsoever and that scenario can just be removed.

That reminds me of https://progress.opensuse.org/issues/120459

@Vogtinator
Copy link
Member

IMO having an encrypted /boot and plain rest just does not make sense whatsoever and that scenario can just be removed.

It should be introduced from ticket, https://progress.opensuse.org/issues/81780. I will double check with qe-security team to double check! @tjyrinki @paolostivanin

If that ticket is what the test is supposed to do then you need the opposite: Plain /boot and encrypted rest.

@nilxam
Copy link
Member

nilxam commented Dec 14, 2023

Two test fails on Leap 15 relate to partition encrypting/decrypting:

  1. crypt_no_lvm https://openqa.opensuse.org/tests/3809925 - it has passpharse prompt for root partition still
  2. all upgrade_cryptlvm scenarios still has boot_encrypt step but there is no passpharse prompt eg. https://openqa.opensuse.org/tests/3809928

please let me know if you need the separate poo ticket for them I can do so.

@rfan1
Copy link
Contributor Author

rfan1 commented Dec 14, 2023

Two test fails on Leap 15 relate to partition encrypting/decrypting:

  1. crypt_no_lvm https://openqa.opensuse.org/tests/3809925 - it has passpharse prompt for root partition still
  2. all upgrade_cryptlvm scenarios still has boot_encrypt step but there is no passpharse prompt eg. https://openqa.opensuse.org/tests/3809928

please let me know if you need the separate poo ticket for them I can do so.

Thanks, please help update https://progress.opensuse.org/issues/152621
I believe the tests can pass again with setting change

@tjyrinki
Copy link
Contributor

IMO having an encrypted /boot and plain rest just does not make sense whatsoever and that scenario can just be removed.

It should be introduced from ticket, https://progress.opensuse.org/issues/81780. I will double check with qe-security team to double check! @tjyrinki @paolostivanin

If that ticket is what the test is supposed to do then you need the opposite: Plain /boot and encrypted rest.

With quick reading I was going to say that yes we have test case "FIPS: Full disk encryption with LUKS (separate unencrypted /boot)", but reading it again I see yes I don't see any point in /boot encrypted only with / non-encrypted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
qe-core Tag used to filter PR's created by QE-Core's members or are assigned to them
Projects
None yet
10 participants