-
Notifications
You must be signed in to change notification settings - Fork 125
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
Meta: improving the shim review process #345
Comments
Thanks for the thread, Steven. Here's my take on that.
IMHO, while the process is official, it may well serve as a learning space for people who do not necessarily have prior experience. Like some sort of science club at a university where students from different backgrounds can come, ask questions, make mistakes and learn how to do things the correct way.
Yes, I am interested.
In regard to this, I'd wait for more automation, like Joseph's review bot and discuss, what are the most detailed things to look at, where automation is tough, let's say using correct generation numbers in regard to what happened historically.
And it does not have the recent fixes I incorporated just before you created this PR. :-(
Yes.
Yes, and that's a good thing - we, or at least I, need more craziness in this world! |
Thank you so much @steve-mcintyre for all your hard work and dedication, those suggestion sounds great I always bump into one or two of those names whenever I am reading any review here and their feedback is always on point!
Having 3 reviewers would be ideal and great, I would suggest maybe if it's the vendor's 1st or 2nd submission? just in case if someone missed something, then it can be two reviewers including 1 trusted reviewer? I guess one of the main point is that we don't have enough trusted reviewers but overloading them with more submission might back fire? but we can start with 3 reviewers per review as you mentioned and see how it goes? we can always change / adopt later on
Totally agree
This would be very helpful |
Thanks @steve-mcintyre and @aronowski and all the other. I'm not the most active of the peer reviewer, but if I can help let me know and I try. |
Hello @steve-mcintyre
Great to see your initiative and enthusiasm to heal this small world, I've really appreciated and recognized all of your submissions - thanks for them!
Fantastic, I do recognize those reviewers for quite some time as well - I am all for promoting them to trusted ones, that makes perfect sense.
Having reviewed a little so far, I welcome any docs improvements. I like them, I will read them thoroughly and comment on them when I discover anything unclear.
Anything that could be automated is a great improvement that saves time and prevents stupid mistakes, I will definitely try it.
|
@steve-mcintyre thank you for all your work. I'm interested to help with the reviews.
I'll go rough the wiki and see what we can add. In my experience the most confusion is around how SBAT entries should be constructed. Maybe we can link to the wiki directly in the README.md, so that it is easier to find.
Automation looks really good. Especially if we can standardize for the checks for patches. Regarding the key protection requirements it seems that some HSM providers provide key attestation, maybe that can help to automate the checks for the certificates inside the shim. Regarding the amount of reviewers I think the idea proposed by @SherifNagy is good approach. Have up to 3 reviewers for the first review and then 2 for the following reviews. We just need to make sure that we have enough reviewers from different organizations. |
Thank @steve-mcintyre, @SherifNagy and others. I was just assigned to be a reviewer by @vathpela. Thank everyone. I will try to do my best to help each release. |
Thanks @steve-mcintyre for moving this forward! I agree with mostly everything that has been said, so I wont repeat. Here there are some extra thoughts:
I agree, but I I was thinking more than five, I think we would run into into the same issue of burnout not so long down the line.
Don't we? I guess it comes down to the definition of formal, hence the bold markings, but we could get something. Call it workshops, meetings, conferences, twich streaming or whatever is in fashion these days ;)
The review is not only about the final binary, or even the binary creation, but definitely many steps on this field can be automated. We can add a checkbox to the list, something like "passes xxx github action". |
I thought those 5 will be "trusted" reviewers along side the existing trusted reviewers crowd |
Yes, that's exactly it. These 5 are also just the extra people I'm planning to add now if they're interested. I fully expect we'll add and remove more people in the future! |
Given "write" access here to @aronowski and @THS-on
|
Yup!
Nod. I'm definitely leaning towards multiple reviewers, but automation can make reviews less time-consudming too.
Argh. Could you point me at a PR/diff for those so I can merge them please? Then a review of my changes would be great!
grin |
Sounds like a fair suggestion, yes. :-) |
Awesome! :-)
Let's see what we can do to make SBAT clearer for people, definitely.
Nod. |
grin. If people would find it interesting, I think we could happily organise a video call or something. Hell, I'm planning on going to FOSDEM again next year and a meetup there might work?
👍 |
That would be awesome! I am planning to go to FOSDEM as well "so far:)" |
That should do it!
It just might! |
@steve-mcintyre yes, I'm definitely also interested in becoming a reviewer. I also won't repeat what has been said already, so I'm just trying to add some points.
|
Introductory clarificationsIn regard to the recent promotions and communication with our community, I feel there are some ways that may make the environment more welcoming, inclusive and easier for newcomers to both get into as both bootchain developers and reviewers. I've been thinking and here are some proposals. Let me know what you think about them and once some are agreed to be included in the future, I'll make them separate issues, so their status can be maintained more easily. They are mentioned in such an order, since they depend on each other in a chronological way. Code of Conduct improvementsWe already have a Code of Conduct in both the
However, what I'd also suggest adding here in the context of inclusivity are the mentions that, among others:
Improving the document for a review applicationIn regard to the inclusivity I mentioned above, I'd improve the application document template to make it easier for people, who see it for the first time, to understand, what they are asked for. For example, let's consider someone who read about SBAT, implemented it successfully in their binaries, but due to the lack of expressing themselves with the vocabulary used in this environment, may mistake a global generation with a product-specific generation. I recall seeing applications, where the question
gave people a hard time. Maybe the "have you set" part was so suggestive, that people thought it refers to their downstream work rather than making sure the upstream entry is
Reference/dummy application(s)After introducing the issue template improvements I suggested, what if there was some sort of reference application or more, that presents a dummy company applying to have their shim signed and provides detailed answers to the questions the committee asks for? For instance, that dummy company wants to have the 15.7 release of UEFI shim signed, but works with older binutils that cause the common bug to be present. Therefore, they explain in their review, why they apply the patch that fixes it. In this case, newcomers could also base their answers on that dummy applications. If there's a common base, like a distro family, most of the tougher questions would cover the distro-specific things. Reorganization of documentsAfter the suggestions above have been introduced, then I'd focus on reorganizing the documents, so, for instance, the Instead, move the questions to another file like
Now, I understand that this delegation of the questions to another file than
ConclusionsSo, what do you think? Are the ideas worthwhile? |
When I was going through the reviews I noticed a couple of things, which I summarized below. Signing and Embedded CertificateThere is no clear refrence on how the certificates embedded in the shim should look like.
Patches to Shim and GRUB2We now often had that patches for shim that are required for submission, that were not released. This is fine, but takes more time to review. It would speed up the review when the upstream commit is directly referenced in the patches or submission. Reviewing GRUB2 patches can be more time consuming depending from where the patches were taken. If the GRUB2 build is based on for example the patches from Fedora, Debian or Ubuntu it would be nice to have some (semi-)automated way on to check if and how the patch sets diverge. GRUB2 ModulesWe ask for the list of included GRUB2 modules, but from a submitters perspective it is not clear, which modules are allowed and which might cause the submission to be rejected. I think we can come up with a list of pre-approved modules. SBATRegarding SBAT I noticed the following:
I would propose that we have a tracking issue or wiki entry for this. Ephemeral keys for kernel modules and the alternativesThe simplest way to prohibit a kernel to load older kernel modules, is to use an ephemeral key during build time. There are reasons, why this might not be implemented:
The question is how we should then proceed in these cases. Possible solutions to mitigate the impact of a vulnerability in a kernel module are:
Do we accept all those strategies? Companies being merged or boughtWe had it multiple times now that companies merged or bought while submitting a shim for review (Neverware/Google, Micro Focus/OpenText, ITRenew/IronMountain). How do we want to handle this? |
I will try to think out loud here with you.
I don't think there is any advantage , but I recall I saw some reviews where the comments were the CA is valid for so many years and that wasn't great
Maybe in terms of validating the organisation? but in any case, the shim has to be signed with EV cert before being submitted to MSFT
Unless we start asking for signing logs and review those logs, I think it is a matter of trust
That did trip me off actually, but we "Rocky team" came up with attributes at least for the certs
I think all modules are allowed, as long as they are not vulnerable "but I might be wrong here" I know some distro ship NTFS support for example and some don't, both got accepted reviews
Agree with you here, and it did happen that I did increase my shim SBAT level where I didn't needed to do that, however I thought there was some talks about having a centrializes database where we can track SBAT level s for signed shims that gets reviewed somehow?
I think in this case, they need to be reviewed as a new entity , since the infrastructure might be different, processes and even people might not be same after the merge / acquisition |
Sorry for going quiet, folks - struggling with some random illness here for the last few days... :-( |
Maybe? I've not had great experiences with the LF so far, but maybe that's just me.
ACK. I think that more sharing of knowledge and experience would help, but even then it can be daunting.
OK, let's try and organise that then. Now we get the fun of trying to organise a meeting for people from ~all time zones, heh! Mail me at 93sam@debian.org - see below... Initial agenda is at https://docs.google.com/document/d/1lL3Qh-YPxaEKOOVi9YcMrqJ22LPJpZ5UKVZDHMiP75c/edit - please ask for edit access if you want it. |
Link is giving me 500 error btw:) |
I think I broke framadate with a very large poll :-( Trying something else instead... |
oooops :D , maybe https://www.when2meet.com/ ? |
Instead, all of you please mail me at 93sam@debian.org with your available times and dates in the timeframe of Sun 15 to Sat 28th October. To accommodate everybody, we may need to pick an uncomfortable time and date - be warned! Deadline for that is next Wednesday (11 October 23:59:59 UTC). I'll post the result here ASAP afterwards. |
Some companies have internal rules about how they can issue certs, and developers are not allowed to deviate from them. That's the main reason I've seen.
Argh. We should really be more consistent here; all of these key combinations work for now, but may not if the firmware folks decide to arbitrarily tighten up in future.
Good question! In Debian we've set up separate key/cert pairs for each of the binaries that we sign further, and I think that can be a valuable thing to do in terms of revocation. Each of the certs describes its usage and the year it was created; these keys should also be much more limited in length than the CA, for hopefully obvious reasons. They should be controlled via HSM, ideally.
👍
👍
There's been some discussion about this, but we don't have a definitive list. I think that we should be looking for a minimal set of modules, particularly filesystems. Let's talk about this.
👍
Good point, yes. I've been suggesting improving the docs around this and making them much easier to find. I'm glad it's not just me! :-)
Nod.
Let's discuss.
How much does it affect us? |
Mainly affects contact verification and if they use EV certificates, they might still want to submit one with the EV certificate of the old company. |
OK, I think we have a winning time and date for the meeting. For the 7 of us providing data, all of us can make Monday 16th October, 1500 UTC == 1600 BST == 0800 PDT There were a couple of other options for the middle weekend or later in the second week, but I've chosen the first available slot. Let's not delay any more than we have to! I'll mail everybody with this as well. Repeating: the agenda is at https://docs.google.com/document/d/1lL3Qh-YPxaEKOOVi9YcMrqJ22LPJpZ5UKVZDHMiP75c/edit . Let me know if you'd like edit access. |
OK, so I think we had a good and productive conversation but we ran out of time. :-/ Checking the data from people, it looks like the same time next Monday is feasible for a followup. Let's do that! Monday 23rd October, 1500 UTC == 1600 BST == 0800 PDT |
My apology for missing the meeting, I added the wrong timezone in my calendar, that's my bad ! I am okay with having a short meeting next week as @steve-mcintyre was suggesting |
I see a worrying tendency from the new reviewers to try to be helpful and streamline the submission process. This is counter productive. The questions must be vague enough that it is not clear what the right answer is, otherwise we cannot judge if they really know what they're doing. The checklists we have are very basic and only a part of establishing that this is a trust-able vendor. You also need to rely on your instincts and feelings, and not just accept something that looks sketchy because it ticks all the boxes in the checklist. You need to assume that every new vendor is just here to p0wn systems and you are the last line of defense. I wish we could take the submission requests private such that people can't copy each others answers, which is a fairly significant issue with the current process where people then "look" smart but they actually have no clue what they're doing. Remember, cheating is commonplace. |
Some
|
Improving the shim review process
We've had quite some problems getting shims reviewed over the last couple of years, and it has not been a great experience for anybody involved:
So, let's work out a better process. I'm suggesting a few things here, please respond in comments below.
We need more reviewers
The hope so far has been that shim submiiters would also review some of the submissions from other people. However, that has been a difficult / daunting thing for many. Spending time doing this has often been a thankless task, with little feedback to suggest that it's actually useful or valued.
Some people have enthusiastically stepped up, nonetheless! I'm explicitly calling out a number of people who've been active in reviewing shim submissions despite the problems. I've spent a chunk of the last week reading and counting review comments in the last 150-ish submissions, and some names are particularly prominent here!
Others have helped too, I'm not going to list everybody here right now! (But see below...) However, I think that these 5 people have clearly already shown they really understand what's need out of a review. I'd like to offer each of these people trusted reviewer status right now. Guys: please talk to me about this, and let me know if you're interested?
I really want to thank everybody who has helped with reviews, not just these people! There are also a few developers who have contacted me out of band offering to help and asking how to do it. So, let's make that easier for people in the future. If you want to help, read the docs (see below!), look at some example reviews, and start doing it! If you're not sure on something, then be clear and say so. If you think the docs are unclear, say so (and ideally propose improvements!). Once you've reviewed a few submissions and we can see how you're working, we can compare your results with others and if we think you're doing a good job then you get to be trusted. I'm not going to propose formal training here, as I don't think we have enough people to make that work. But we should be able to learn from each other and the docs.
How does that sound?
We need more trust
Some of the reviews only get comments from one reviewer, which means it's likely that we'll have missed things or made mistakes. Judgement calls might go different ways, depending on the reviewer. What I'd love to see going forwards is a more distributed set of reviews. Could we wait until we get (for example) three independent reviews of each submission, with at least one from an anointed/trusted reviewer? Is that too many, or too few? Feedback welcome!
Tagging things with labels only works for trusted reviewers, so instead of that let's just say what we think in the comments. Once a submission has enough positive reviews, a trusted person can apply the "accepted" label and we move on.
We need more and better docs
I started writing some docs for the shim review process a while back, adding them to the shim wiki. Others have helped: thanks to @pnardini in particular here. But I've not managed to document everything I wanted, and I also get the impression that people are not finding those docs.
There's currently a guide for reviewers and a guide for submitters. I hope they're useful? I'd also like to collect together some best practices / HOWTO docs for developers trying to do the right thing with their Secure Boot stack. Some of the well-established projects and vendors have spent a lot of time and effort working out how to do things. Some of these steps are obvious, a lot of them are not - let's share and make it easier for new people to do the right thing!
Could you help with these docs?
It's been suggested (@pnardini again!) that we should move those docs from the shim wiki into the shim-review repo directly, to make it easier for everybody to find them and also easier to collaborate on updating docs. I think that's a great idea, so I've added #344 to do the initial import of current wiki state.
Automation
Some of what we're looking for in reviews can be automated, of course. @jclab-joseph in #340 is working on a bot to do some of this for us. Let's hope this helps too. :-)
Feedback (and patches!) welcome
How do my suggestions sound? Will this help? Am I sounding crazy? Have I missed something important? You know what to do!
List of people
Here's a full-ish list of people who've contributed or shown interest in shim reviews in the last couple of years, so everybody gets notified about this discussion. Apologies for the spam if you didn't want to hear more...
@ecos-platypus @aronowski @dennis-tseng99 @tSU-RooT @THS-on @pjbrown
@SherifNagy @ClaudioGranatiero-10zig @evilteq @keithdlopresto
@realnickel @nicholasbishop @Doncuppjr @christopherco @Burmash
@miray-tf @mheese @rehakp @mikebeaton @tedbranston @christoph-at-unicon
The text was updated successfully, but these errors were encountered: