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

Print the EFI certs in the state command #98

Merged
merged 6 commits into from
Apr 17, 2024
Merged

Print the EFI certs in the state command #98

merged 6 commits into from
Apr 17, 2024

Conversation

Itxaka
Copy link
Member

@Itxaka Itxaka commented Apr 17, 2024

@Itxaka Itxaka requested a review from a team April 17, 2024 12:25
Copy link

codecov bot commented Apr 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 52.89%. Comparing base (d729cc7) to head (ff2252f).
Report is 3 commits behind head on main.

❗ Current head ff2252f differs from pull request most recent head d36d504. Consider uploading reports for the commit d36d504 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #98   +/-   ##
=======================================
  Coverage   52.89%   52.89%           
=======================================
  Files          16       16           
  Lines        1104     1104           
=======================================
  Hits          584      584           
  Misses        423      423           
  Partials       97       97           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Itxaka
Copy link
Member Author

Itxaka commented Apr 17, 2024

non-efi system:

image

efi system:

image

// GetEfiCertsCommonNames returns a simple list of the Common names of the certs
func GetEfiCertsCommonNames() types.EfiCerts {
var data types.EfiCerts
certs, _ := GetAllCerts() // Ignore errors here, we dont care about them
Copy link
Contributor

@jimmykarily jimmykarily Apr 17, 2024

Choose a reason for hiding this comment

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

This is still in the sdk. We should let the caller decide whether it cares about the errors or not. Otherwise, the caller has no way to get the error (e.g. to debug) without using replace in go.mod.

Since this is an exported function, feel free to ignore the error in the detectKairos function below (if the caller of that function wants to debug, it can call GetEfiCertsCommonNames() directly.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I dont know about that. This returns the certs if it can get them, otherwise its an empty eficerts. There is no promise here of returning errors or not, debugging and such, this is mostly an shortcut to not have to do the same thing in the state when getting the names and such, so its a commodity that promises nothing else in the signature of the call.

For getting the actual errors, and the full certificates (not just a list of the common names) there is another function that actually returns an error.

Maybe this should not be public, or maybe I should just move all the mangling into the state instead....

Copy link
Member Author

Choose a reason for hiding this comment

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

updated, now its a commodity method in state just to clean up the actual stuff coming from the signatures

Copy link
Contributor

@jimmykarily jimmykarily left a comment

Choose a reason for hiding this comment

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

Nice!

@Itxaka Itxaka requested review from jimmykarily and a team April 17, 2024 14:36
@Itxaka Itxaka merged commit 599359e into main Apr 17, 2024
9 checks passed
@Itxaka Itxaka deleted the certs_in_state branch April 17, 2024 14:57
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.

2 participants