Add cross-review question

[frozencemetery]

shim-review is meant to be distros reviewing each other and right now it's very much not.

Signed-off-by: Robbie Harwood rharwood@redhat.com

[nicholasbishop]

I think it might be worth considering why other people aren't reviewing before adding this question. I think a lot of people don't feel very empowered to do reviews, or don't feel that their reviews will be treated as valid by the "core" reviewers from redhat/canonical/etc. There have been some difficulties with getting discussions moved to public places, and a lot of missing (or hard-to-discover) docs.

None of which is to say that the main reviewers aren't overburdened by the review load! But I think there are some issues that should be fixed before there can be an expectation of others contributing to reviews.

[frozencemetery]

If you have suggestions on things do, feel free :)

I think the idea that there's a valid/invalid set is definitely a problem, given that the whole point of shim review is that distros are trusted to review each other rather than requiring Microsoft to become shim/grub2/Linux subject matter experts. But as I'm one of the "main reviewers", I don't know how to find out what the problems are without asking others to try it and report issues.

[frozencemetery]

Also, in my mind the issue is less about load on "main reviewers" and more about throughput time. After all, I can (and do) just stop reviewing.

Submitters quickly become frustrated when they don't get feedback. For reasons we can only speculate about, they tend to complain and try to "escalate" rather than reviewing other submissions. Maybe they can't find out how, maybe they think this is a service they're entitled to, maybe there are other discoverability issues - I don't know. But we need less of that.

[pnardini]

Can we expand the set of "main" reviewers that are empowered to apply the accepted label on shim submissions? This would probably go a long way towards addressing the valid / invalid review fear that's been identified here.

For starters, perhaps allow one person from each distro that is either "sufficiently trusted" or (has had a shim previously accepted + done at least n reviews) to become an approver as well?

[frozencemetery]

I don't see how that would help - there isn't a set of reviewers that meet that criteria today, so we wouldn't gain any throughput. But that's just my opinion and I don't speak for others.

[nicholasbishop]

Here's a different framing that might help clarify why we don't get more people volunteering to do reviews:

Let's say that I'm a developer on the fictional SomeSmallLinuxDistro. I care about providing secure boot support for my users; it's pretty much required for a good user experience on UEFI these days. I know that shim review is required, and that the shim-review project wants everyone to help with reviews. Once #297 is merged there will even be instructions in the repo on how to do it. Great! So I open up someone else's submission. I go through all the steps to review it, and write up a report in a comment. Assuming no issues were found, two things can happen:

1. Someone who has permissions to apply an accepted label does so. The question here is: will the person who has those permissions actually accept my report? I'm just some random developer that the main reviewers don't know, so it seems likely that they are going to go through everything again and verify. So did my review actually accomplish anything? It's very hard to tell, and I might not feel like my effort was well spent if I have no indication that my review did anything.
2. The review sits there for months with no additional comment or accepted label. Everyone knows this probably means that another buffer overflow bug has been found in shim or grub, and the chosen few are working hard behind the scenes to fix it. But in the meantime, my review work has been wasted because everyone has to wait for the vuln announcement and probably a new shim release.

These outcomes aren't fun for anybody. Nobody is intentionally trying to make things hard for others, but that is the way things are currently. The uncertainty of whether or not the work of a reviewer that cannot apply the accepted tag will be of value is a significant barrier to getting more people to contribute to the review process. If expanding the group of reviewers that can apply the accepted tag is not an option, we need to find another way to have reviewers be confident that their reviews matter because right now there's really no guarantee that they do.

[aronowski]

@nicholasbishop

I'm just some random developer that the main reviewers don't know, so it seems likely that they are going to go through everything again and verify. So did my review actually accomplish anything? It's very hard to tell, and I might not feel like my effort was well spent if I have no indication that my review did anything

I would consider an answer if everything is well-explained. Or, since this hypothetical reviewer may not yet be trusted, consider their answer as a hint on what to focus on.

This is surprisingly effective sometimes once issuer responds and explains their reasoning. Later on a conversation takes place and this shows what knowledge and understanding of the ongoing processes both parties have.

---

@frozencemetery

the whole point of shim review is that distros are trusted to review each other rather

This knowledge is there but at least for me was not obvious back in the day. One can see this changing right now, but I believe there is indeed something that might be done. More on that later.

But still the reviewing part from the point of distro A might sometimes just be scratching the surface of the implementation of distro B as they might set things up in a completely different way. For instance I myself am more involved in Fedora/RHEL implementations rather than Debian's and may sometimes lack the required knowledge to tell if something's right or wrong (that's why I prefer asking on the details so I can learn distro B's implementation).

If you have suggestions on things do, feel free :)

I have been thinking and may help with these ideas

• have this and #297 pull request accepted as well as make sure the shim-review repository is self-contained, meaning all the required knowledge to write a review is already here or at least clearly links to external resources (e.g. a question about vmlinuz binary implementing NX support among other Microsoft requirements, which links to a file that quotes/summarizes Microsoft's requirements)
• write a complete FAQ/walkthrough on how to prepare the complete bootchain for a review and what information can be found where. This might well mean pointing to external programs/resources and a summary/quickstart on how to use them in practice (e.g. how to statically analyze a binary for Microsoft's requirements) and working around common bugs and limitations.
• maybe add an entry like Give us feedback on what do you think might be improved in the process that will make people more willing to express themselves? Like you said: I don't know how to find out what the problems are without asking others to try it and report issues.

shim-review is meant to be distros reviewing each other and right now it's very much not

That's right. I myself am not a distro per se. ;)

[jsegitz]

I agree with Nicholas. There needs to be a clear way how a reviewer that isn't part of the "core" group and review something and this then results in a state that will allow the requester to get their shim signed by MS.

In most cases that I've seen this didn't happen. Someone reviewed the submission but there was no further action and the reviewer itself can't mark the submission as valid.

#297 is a good start, but probably not sufficient.

[mheese]

@nicholasbishop and @aronowski are spot on with this problem. I wouldn't feel "empowered" to provide a peer review. Sure, I can do it (as I did for example for Kamil now because he asked for it), but there is only that much of what I understand about it too. To begin with, I didn't even understand when he peer reviewed my submission that he isn't even an official reviewer.

For example, one of the core problems is: while I understand really well now what needs to be reviewed for the shim itself, it's not even clear to me what are all the review requirements for additional bootloaders like grub2 and Linux itself.

Here are my $0.02 on what could help the overall situation:

1. Have an "official" comittee/panel with dedicated contacts, presence, etc. which goes beyond just that GitHub repository here. Right now everybody ends up submitting their reviews here because Microsoft explicitly asks for it. IMHO, creating this as a group as maybe part of the Linux Foundation might help? They also have the right resources on the "business" side and community outreach side to assist with that.
2. Enforce participation in that group: SomeSmallLinuxDistro wants to get their binaries signed? Sure, but you have to participate. As I find myself in exactly that type of situation here in Shim 15.7 for Hedgehog ONIE and SONiC #334 , I can tell you that we would be more than willing to help out the best we can if it means we make progress on our submission.
3. I think something like this is key because I totally understand @frozencemetery if the official reviewers do not have the bandwidth to review every single request from SomeSmallLinuxDistro.
4. The Add REVIEW.md #297 is a very good start, but for sure not sufficient.
5. A "framework" for both creating submissions, as well as testing submissions would help. We all know that if we would have automated tools/scripts which test some of these things, both reviewers as well as people who do submissions would feel a lot more comfortable about what they are doing. For example, I probably checked everything 4x-5x times before I created my issue, but Kamil still found a simple thing with regards to the SBAT version numbers. If there would be a tool which for example can do the static analysis of all involved binaries and check off on page alignments, NX_COMPAT, etc. that would already go a long way in helping reviewers as well as providing confidence for people who do submissions.
6. I sort of even started with that for our own things already, but I also know it's probably impossible to do for everybody: a test framework which can use the shim, grub, Linux binaries and test if Secure Boot is really working the way it should, that would also really help. For example, while testing my bootchain I realized that for ONIE the standard kernel version is too old, and doesn't even include Secure Boot lockdown. So it was a piece of cake to kexec into an unsigned kernel.
7. Stage the reviews: not everybody is in expert on everything. As I mentioned above, I could do reviews for the shim by now, but probably not the other parts. This part of the process could help getting different experts involved in everything.

[steve-mcintyre]

I'm trying to work out a better way forwards now. I'm building a list of all those who appear to be interested in reviewing shims and I'll be contacting those people shortly...

[jaromaz]

@steve-mcintyre as you build your list please keep in mind user @aronowski , who is currently making many reviews and contributions to this group.

[steve-mcintyre]

@steve-mcintyre as you build your list please keep in mind user @aronowski , who is currently making many reviews and contributions to this group.

He's right near the top of my list already :-)

[steve-mcintyre]

Closing this PR, as we're doing other (different) work around this now.