Skip to content

Modify sbat.md to help with readability. #398

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
Merged
merged 1 commit into from
Sep 8, 2021
Merged

Modify sbat.md to help with readability. #398

merged 1 commit into from
Sep 8, 2021

Conversation

@eshiman
Copy link
Contributor

@eshiman eshiman commented Jul 30, 2021

Signed-off-by: Esther Shimanovich eshimanovich@google.com

I've had a bit of trouble understanding SBAT.md, so I thought I might open up a PR with a few additions that might help with readability, and some questions. Hopefully this might be helpful to other people as well.

  1. Can you define disclosure (from the chart included in the file)?
  2. Where do the numbers 5 and 6 in the chart come from? I expected the fields under "after Vendor C's first Update" to be 2 and 3 instead of 5 and 6. Why the big jump from 1 to 5?
  3. Why do you need both upstream's version of grub and downstream's version of grub in the sbat.csv file? Isn't only one used?
  4. Will the product specific generation number continually increase for each global or do they reset? I was a little confused by this sentence: "The product-specific generation number does not reset and continues to monotonically increase over the course of these events." ----What events are you referencing to here?
  5. Where can we look up global generation numbers? Or is that in flux still? This info would be helpful in documentation, even if there is one entry so far.

I highlighted some of the words that were defined in this doc, and I also added an example earlier on, as that would have been helpful for me to visualize some of the new concepts as I read it. I didn't want to make any structural changes so I avoided those. If you respond to my questions above, I am happy to add them to the doc in this PR!

Thanks a bunch!
Esther

@julian-klode
Copy link
Collaborator

julian-klode commented Aug 3, 2021 •
edited
Loading

  1. I don't understand the question
  2. That seems to be a bug IMO. Presumably an earlier spec version required product-specific >= global (which is 4), I don't know for sure. In practice, as long it increases, it won't matter anyway.
  3. Need might be a strong word, but the file format is more consistent this way; arguably you only need those for humans to read.
  4. The table shows that the number resets after a global event. You can essentially consider that a global event will fold in all product-specific events. So if you had shim.foo 1,2,3 (shim 1), than your revocation list looks like {shim.foo < 3}. After a global event shim 2, any shim.foo entries are removed and the list becomes {shim < 2}.
  5. In the repositories of those components? Currently they are all 1, presumably at some point revoked sbat levels will be published on the uefi website with dbx updates

I'll request more review from @jsetje

@julian-klode julian-klode requested a review from jsetje August 3, 2021 10:59
jsetje
jsetje requested changes Aug 3, 2021
View reviewed changes
Copy link
Collaborator

@jsetje jsetje left a comment

Choose a reason for hiding this comment

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

Thanks for taking a look, here are some quick comments.

SBAT.md Outdated
assigned names. Examples of components are shim, GRUB, kernel, hypervisors, etc.

Below is an example of a product and vendor, both in the same `sbat.csv` file. SBAT
is a vendor, as it is a link in the UEFI Secure Boot chain of trust.
Copy link
Collaborator

@jsetje jsetje Aug 3, 2021

Choose a reason for hiding this comment

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

I think you meant to say SBAT acts like a component rather than a vendor. And yes, the sbat version basically does.

@jsetje
Copy link
Collaborator

jsetje commented Aug 3, 2021

Can you define disclosure (from the chart included in the file)?

A disclosure in this context is the event/date where a CVE and fixes for it are made public. It could be worth describing that in a bit more detail, the document was generally written with an audience of system security folks in mind, but the group of folks that have to deal with shim and how it works is actually broader than that. I can try to draft something.

@eshiman eshiman force-pushed the modify-sbat-md branch 2 times, most recently from fe1ed4b to 406f519 Compare August 4, 2021 00:28
@eshiman
Copy link
Contributor Author

eshiman commented Aug 23, 2021

Hey! I put up some changes in response to your feedback 20 days ago. Is there anything else I need to do to push this forward?

@jsetje
Copy link
Collaborator

jsetje commented Aug 23, 2021

Thanks for the ping, this had fallen off my radar.

Also, thank you for the edits, other than the one item I flagged this looks good.

eshiman
eshiman commented Aug 24, 2021
View reviewed changes
SBAT.md Outdated
assigned names. Examples of components are shim, GRUB, kernel, hypervisors, etc.

Below is an example of a product and component, both in the same `sbat.csv` file. `sbat`
is a component, as it is a link in the UEFI Secure Boot chain of trust.
Copy link
Contributor Author

@eshiman eshiman Aug 24, 2021

Choose a reason for hiding this comment

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

I see in my email you left a comment requesting that I switch this to 'sbat' defines the version of the format of the revocation metadata itselfhowever I don't see that comment here. (Was it deleted? I also see in your lgtm comment you wanted me to address something you flagged.)
Does that comment still apply?
I had my specific wording originally because the word "component" seems to be used in a specific way in this document, as you mentioned, and I wanted it to be clear what the word "component" referred to. But I do get that components is an overloaded word. It's entirely possible I didn't understand what you meant by "component" in this document. Is sbat a component as defined above? Or is it just "the version of the format of the revocation metadata itself"? If that is true, should I use a different example to illustrate what a "component" is?
Here's a guess as to what I could switch it to, to clarify the concern you had with the word being overloaded:

**Component** as defined here refers to a component that is committed to a UEFI boot services variable, as opposed to a component loaded from a file in a filesystem. They are used as a link in the UEFI Secure Boot chain of trust and are assigned names...

I'm more than happy to rewrite it in the way you prefer, I just want to make sure that is still what you want, as that comment seems to be gone.

@jsetje
Copy link
Collaborator

jsetje commented Aug 24, 2021

Yes, I tried to turn it back into a review and deleted the comment, same content though.

@eshiman
Modify sbat.md to help with readability.
b82414a 
Signed-off-by: Esther Shimanovich eshimanovich@google.com
@vathpela vathpela merged commit 72a95ae into rhboot:main Sep 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jsetje jsetje jsetje approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@eshiman @julian-klode @jsetje @vathpela