Foxboron / sbctl

:computer: :lock: :key: Secure Boot key manager
MIT License
1.35k stars 71 forks source link

Add list-enrolled-keys command #296

Closed jimmykarily closed 2 months ago

jimmykarily commented 2 months ago

to list certificates that are already enrolled on the current system

Background:

In the Kairos project we need a simple way for our users to enroll custom keys to the firmware. We also need to help them revoke certain keys later. In general, we need a tool that allows them to manage the enrolled keys (list, add, delete).

It was suggested that sbctl could satisfy these (or some of these) requirements. This is a first PR to try this option.


I tried to follow the naming conventions of the other commands to stay consistent but we could discuss a different way to structure this new command to allow us to later add subcommands for other operations (e.g. sbctl enrolled-keys list , sbctl enrolled-keys add etc)

I had a look at the integration tests in order to add one for this command but I didn't find a simple way to run them locally and I don't see those running in Github actions either. Let me know if I missed something. If you don't mind me adding testing related changes to this PR, I could try to make those run locally in a streamlined way. We also do integration testing using VMs in Kairos but I'm afraid the testing related changes would be more than what this PR introduces already. Alternatively, I could try to introduce the testing changes to run integration tests in another PR and then get back to this one to consume.

Let us know what you think!

Note: The code in this PR is mostly a copy of what we have already implemented in Kairos: https://github.com/kairos-io/kairos-sdk/pull/103/files

Foxboron commented 2 months ago

Thanks!

This looks fine. I suspect I need to come up with a better way to structure the command verbs in sbctl, but that can be done later.

I had a look at the integration tests in order to add one for this command but I didn't find a simple way to run them locally and I don't see those running in Github actions either. Let me know if I missed something. If you don't mind me adding testing related changes to this PR, I could try to make those run locally in a streamlined way. We also do integration testing using VMs in Kairos but I'm afraid the testing related changes would be more than what this PR introduces already. Alternatively, I could try to introduce the testing changes to run integration tests in another PR and then get back to this one to consume.

go-uefi has an entire in-memory FS implementation that isn't used by sbctl yet. It would allow us to do full on integration testing. go-uefi also books minimal u-root containers with ovmf to test that everything is working and I plan on doing the same in sbctl at some point.

Example of the efivarfs tests: https://github.com/Foxboron/go-uefi/blob/master/efivarfs/testfs_test.go

Integration tests with qemu+vmtest+ovmf. https://github.com/Foxboron/go-uefi/blob/master/tests/integration_test.go

It might be interesting for you stuff as well, but I haven't found the time to implement this in sbctl yet.

jimmykarily commented 2 months ago

Integration tests with qemu+vmtest+ovmf. https://github.com/Foxboron/go-uefi/blob/master/tests/integration_test.go

Similar stuff happening here :D : https://github.com/kairos-io/kairos/blob/master/tests/tests_suite_test.go#L322

I'll let you make the first choices of tools and structure then before we start contributing to the testing code.

jimmykarily commented 2 months ago

Simplified the code by replacing the CertList struct with a map[string]([]*x509.Certificate). Also addressed the review comment.

Foxboron commented 2 months ago

This looks good to me. Please squash the commits :)

jimmykarily commented 2 months ago

This looks good to me. Please squash the commits :)

done