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

feat: Add notification for secure boot key check #1661

Merged
merged 29 commits into from
Sep 25, 2024
Merged

feat: Add notification for secure boot key check #1661

merged 29 commits into from
Sep 25, 2024

Conversation

jardon
Copy link
Contributor

@jardon jardon commented Sep 9, 2024 •
edited
Loading

  • Add script that checks secure boot config and handles motd and GUI notifications
  • Add systemd service to run script
  • Enable systemd service
  • Add motd key warning section

Closes #1651

image
image

○ check-sb-key.service - Service to check for secure boot key enrollment
     Loaded: loaded (/usr/lib/systemd/system/check-sb-key.service; enabled; preset: disabled)
    Drop-In: /usr/lib/systemd/system/service.d
             └─10-timeout-abort.conf
     Active: inactive (dead) since Thu 2024-09-19 06:54:01 CDT; 2s ago
   Duration: 67ms
    Process: 6815 ExecStart=/usr/libexec/check-sb-key.sh (code=exited, status=0/SUCCESS)
   Main PID: 6815 (code=exited, status=0/SUCCESS)
        CPU: 11ms

Sep 19 06:54:01 fedora systemd[1]: Started check-sb-key.service - Service to check for secure boot key enrollment.
Sep 19 06:54:01 fedora check-sb-key.sh[6829]: /etc/pki/akmods/certs/akmods-ublue.der is not enrolled
Sep 19 06:54:01 fedora systemd[1]: check-sb-key.service: Deactivated successfully.

@jardon jardon force-pushed the key-dbus-notify branch 11 times, most recently from 4f3bbac to e826c08 Compare September 10, 2024 17:37
@jardon jardon marked this pull request as ready for review September 10, 2024 17:38
@jardon jardon requested a review from castrojo as a code owner September 10, 2024 17:38
@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. enhancement New feature or request labels Sep 10, 2024
@jardon jardon mentioned this pull request Sep 10, 2024
Closed
@jardon
feat: Add notification for secure boot key check
62e779b 
- Add script to check for sb enabled and key registration
- Add script for notification
- Add systemd service to run script and notify
@bsherman
Copy link
Contributor

@jardon a comment I make to most contributors... once you start getting feedback on PRs, please don't force push. Doing so erases history of comments and their relevance which makes it harder to track the history of change.

We squash merge most/all PRs in the ublue-os repos, so cleaning up commits is not a concern.

Thanks!

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Sep 11, 2024
@jardon jardon marked this pull request as draft September 11, 2024 18:40
@KyleGospo KyleGospo enabled auto-merge September 15, 2024 03:25
KyleGospo
KyleGospo previously approved these changes Sep 15, 2024
View reviewed changes
@castrojo
Copy link
Member

Ok we good on this one? Last call!

@bsherman bsherman disabled auto-merge September 15, 2024 05:37
@bsherman
Copy link
Contributor

Ok we good on this one? Last call!

i'm actively testing, but i don't think this service works as expected.

@dosubot dosubot bot removed the lgtm This PR has been approved by a maintainer label Sep 15, 2024
@jardon jardon marked this pull request as draft September 15, 2024 19:05
@jardon
Copy link
Contributor Author

jardon commented Sep 18, 2024 •
edited
Loading

If anyone wants to test this in a secure boot enabled environment, rebase onto ghcr.io/jardon/bluefin:key-dbus-notify

@jardon jardon marked this pull request as ready for review September 18, 2024 13:16
@bsherman
Copy link
Contributor

If anyone wants to test this in a secure boot enabled environment, rebase onto ghcr.io/jardon/bluefin:key-dbus-notify

Thank you for sticking with this PR! But also, thanks for going the extra mile and giving us a rebasable image for testing; that's excellent!

bsherman
bsherman requested changes Sep 25, 2024
View reviewed changes
Copy link
Contributor

@bsherman bsherman left a comment

Choose a reason for hiding this comment

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

image

One change to make... a missing double quote in the sb-key-notify but otherwise, i think this is working.

I tested using the image you provided. Thank you! That made it much easier!

And apologies for taking so long to get back to it.

system_files/shared/usr/bin/sb-key-notify Outdated Show resolved Hide resolved
@jardon
Copy link
Contributor Author

jardon commented Sep 25, 2024

I've updated the test image

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Sep 25, 2024
@bsherman
Copy link
Contributor

bsherman commented Sep 25, 2024 •
edited
Loading

I've updated the test image

@jardon any idea why this link is duplicated?

image

@jardon
Copy link
Contributor Author

jardon commented Sep 25, 2024

I've updated the test image

@jardon any idea why this link is duplicated?

image

upstream bug found here: charmbracelet/glamour#347

i could make it a non-plain link but it still prints out the link so it would look more like link-name link-address instead of link-address link-address

@bsherman
Copy link
Contributor

upstream bug found here: charmbracelet/glamour#347

i could make it a non-plain link but it still prints out the link so it would look more like link-name link-address instead of link-address link-address

No worries, sounds like something which will eventually get fixed upstream.

Still good with me.

@castrojo @KyleGospo I'm good with approval/merge on this now.

@castrojo castrojo merged commit 2d9f673 into ublue-os:main Sep 25, 2024
32 of 52 checks passed
@github-actions github-actions bot mentioned this pull request Sep 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bsherman bsherman bsherman approved these changes

@castrojo castrojo Awaiting requested review from castrojo castrojo is a code owner

@KyleGospo KyleGospo Awaiting requested review from KyleGospo

Assignees
No one assigned
Labels
enhancement New feature or request lgtm This PR has been approved by a maintainer size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add an motd warning when ublue's keys are not enrolled
4 participants
@jardon @bsherman @castrojo @KyleGospo