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

Add list-enrolled-keys command #296

Merged
merged 1 commit into from
Apr 22, 2024

Conversation

jimmykarily
Copy link
Contributor

@jimmykarily jimmykarily commented Apr 22, 2024

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

cmd/sbctl/list-enrolled-keys.go Outdated Show resolved Hide resolved
@Foxboron
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@Foxboron
Copy link
Owner

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

@jimmykarily jimmykarily temporarily deployed to Build, sign, release binaries April 22, 2024 15:51 — with GitHub Actions Inactive
@jimmykarily jimmykarily temporarily deployed to Build, sign, release binaries April 22, 2024 15:51 — with GitHub Actions Inactive
@jimmykarily jimmykarily temporarily deployed to Build, sign, release binaries April 22, 2024 15:51 — with GitHub Actions Inactive
to list certificates that are already enrolled on the current system

Signed-off-by: Dimitris Karakasilis <[email protected]>
@jimmykarily
Copy link
Contributor Author

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

done

@Foxboron Foxboron merged commit 8a86eb6 into Foxboron:master Apr 22, 2024
@jimmykarily jimmykarily deleted the list-enrolled-keys branch April 23, 2024 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants