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

Adding check if secureboot is enabled or not #5929

Merged
merged 1 commit into from
Jul 9, 2024
Merged

Adding check if secureboot is enabled or not #5929

merged 1 commit into from
Jul 9, 2024

Conversation

vaishnavibhat
Copy link
Contributor

The function checks if secureboot is enabled or not from the OS side.
The command output of "lsprop /proc/device-tree/ibm,secure-boot" is used to check the OS status.

@mr-avocado
Copy link

mr-avocado bot commented May 8, 2024

Dear contributor,
Avocado is currently at the end of sprint #105, therefore we are in feature freeze state.
Please avoid merging changes that do not fall into these categories:

  • Bug fixes
  • Documentation updates

The feature freeze will be active until the release planned on 05/06/2024.

@abdhaleegit abdhaleegit mentioned this pull request May 10, 2024
Merged
abdhaleegit
abdhaleegit approved these changes May 10, 2024
View reviewed changes
Copy link
Contributor

@abdhaleegit abdhaleegit left a comment

Choose a reason for hiding this comment

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

LGTM

@abdhaleegit
Copy link
Contributor

@vaishnavibhat fix static fails make use of double qoutes here if '00000002' in line:

richtja
richtja reviewed May 10, 2024
View reviewed changes
avocado/utils/linux.py Outdated

:return: True if secureboot is enabled, False if otherwise
"""
cmd = "lsprop /proc/device-tree/ibm,secure-boot"
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @vaishnavibhat, can you please describe to me what lsprop is and what it does, to be hones I am not familiar with it.

@vaishnavibhat
Copy link
Contributor Author

vaishnavibhat commented May 23, 2024 •
edited
Loading

lsprop is a tool provided by powerpc-utils. It is used to get information about /proc files .

For secureboot check:
#lsprop /proc/device-tree/ibm,secure-boot
/proc/device-tree/ibm,secure-boot
00000002

0 incase of secure boot disabled, 1 (enable and log only - trusted boot) and 2 (enable and enforce - secure boot)

#man lsprop
LSPROP(8) Linux on Power Service Tools LSPROP(8)

NAME
lsprop - list properties

SYNOPSIS
lsprop [-R] [-m max-bytes] [-w num-words] [FILE...]

OVERVIEW
lsprop program is a member of the ppc64-utils suite of utils. Use it
to list properties

DESCRIPTION
lsprop displays properties for FILEs like Open Firmware .properties
word. If the FILE is not set, the current directory is used.

OPTIONS
-R Process recursively

   -m max-bytes
          Read only first max-bytes bytes from the FILEs

   -w num-words
          Display up to num-words words per line

Linux Sep 2010 LSPROP(8)

@vaishnavibhat vaishnavibhat force-pushed the sec_boot branch 2 times, most recently from 8837bff to e60b022 Compare May 23, 2024 10:14
@richtja
Copy link
Contributor

richtja commented May 23, 2024

lsprop is a tool provided by powerpc-utils. It is used to get information about /proc files .

For secureboot check: #lsprop /proc/device-tree/ibm,secure-boot /proc/device-tree/ibm,secure-boot 00000002

0 incase of secure boot disabled, 1 (enable and log only - trusted boot) and 2 (enable and enforce - secure boot)

#man lsprop LSPROP(8) Linux on Power Service Tools LSPROP(8)

NAME lsprop - list properties

SYNOPSIS lsprop [-R] [-m max-bytes] [-w num-words] [FILE...]

OVERVIEW lsprop program is a member of the ppc64-utils suite of utils. Use it to list properties

DESCRIPTION lsprop displays properties for FILEs like Open Firmware .properties word. If the FILE is not set, the current directory is used.

OPTIONS -R Process recursively

   -m max-bytes
          Read only first max-bytes bytes from the FILEs

   -w num-words
          Display up to num-words words per line

Linux Sep 2010 LSPROP(8)

Hi @vaishnavibhat thank you for the description. So IIUIC the whole is_os_secureboot_enabled method will only work on powerpc. Therefore, I would propose to do an arch check and create and raise for example UnsupportedMachineError for unsupported architectures.

@vaishnavibhat vaishnavibhat force-pushed the sec_boot branch 2 times, most recently from 4c5792b to 2623a72 Compare May 30, 2024 06:17
richtja
richtja requested changes Jun 4, 2024
View reviewed changes
Copy link
Contributor

@richtja richtja left a comment

Choose a reason for hiding this comment

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

Hi @vaishnavibhat, thank you for your update. IMO, it is on the right track. I just think that except Exception is too general for this purpose, and we can be more specific. Please let me know what do you think about it.

avocado/utils/linux.py Outdated
for line in process.system_output(cmd).decode("utf-8").splitlines():
if "00000002" in line:
return True
except Exception:
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if Exception is not too general. When lsprop is not in the system, the process.system_output(cmd) will throw FileNotFoundError` am I right?

@abdhaleegit
Copy link
Contributor

abdhaleegit commented Jun 26, 2024 •
edited
Loading

@vaishnavibhat Please fix the duplicate comit.. ammend the same do not create new commit

@vaishnavibhat
Adding check if secureboot is enabled or not
f30c348 
The function checks if secureboot is enabled or not from the OS side.
The command output of "lsprop /proc/device-tree/ibm,secure-boot" is used to check the OS status.

Signed-off-by: Vaishnavi Bhat <[email protected]>
@vaishnavibhat
Copy link
Contributor Author

Resending the patch after taking care of the review comments.

@abdhaleegit
Copy link
Contributor

@richtja Please see if this is mergable

@vaishnavibhat
Copy link
Contributor Author

@richtja Addressed the review comments. Please me know if this looks good.

Thank you

@clebergnu clebergnu self-requested a review July 2, 2024 15:29
richtja
richtja approved these changes Jul 8, 2024
View reviewed changes
Copy link
Contributor

@richtja richtja left a comment

Choose a reason for hiding this comment

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

Hi @vaishnavibhat, after the updates it LGTM. Thank you.

@richtja richtja merged commit 232eefc into avocado-framework:master Jul 9, 2024
57 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richtja richtja richtja approved these changes

@PraveenPenguin PraveenPenguin PraveenPenguin approved these changes

@abdhaleegit abdhaleegit abdhaleegit approved these changes

@clebergnu clebergnu Awaiting requested review from clebergnu

Assignees
No one assigned
Labels
None yet
Projects
Default project
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vaishnavibhat @abdhaleegit @richtja @PraveenPenguin