Add additional reviewers

[jsegitz]

To answer the question, yes, reviewers can be added; but there isn't a list per se. We just need people willing to do the work, and hopefully cross-review so people aren't approving their own.

Originally posted by @cyphermox in #73 (comment)

lets continue this discussion here and not in my review request. So I need final approval but in general SUSE would be willing to help out here. Oracle also would be willing to chime in. How do we want to take it from here? Do we need to discuss this further or is it enough to just add us (with the understanding that one doesn't review the submission of the organization one belongs to)

[blschatz]

We (Schatz Forensic) are happy to help with review as well.

[AlexeyPetrenkoOracle]

+1 for more reviewers and precise review requirements.

I think it's clear for everyone that this process is barely working now. I have a request which is open for six and a half months, and there is still no ETA to resolve it.
As a result, Oracle Linux carries shim with known bugs for months, which is quite disturbing since shim is a security component, and we need a way to release the updates promptly.

Oracle would definitely be interested in helping with this. And I can volunteer myself.

[blschatz]

@AlexeyPetrenkoOracle : I'm happy to review your submission like I did recently for @jrpark-lge 's submission. All I'd ask is that you commit a working dockerfile into your submission tree and I'll do the work.

Note: I'm not on the acceptance team, and have no recognised affiliation with that team.

My hope here is that by working together we can come up with a testing methodology that is easy to reproduce, thereby making the job of the acceptance team easier. Then they can "review the reviews" rather than undertake the actual reviews.

[cyphermox]

Just do reviews, and assign another review so we can cross-check, I guess. There isn't much more than doing the work.

The only informal rule I've followed so far was "don't review your own".

In doubt, I'm not sure it's necessarily a good idea to spend inordinate amounts of time debugging why an build can't be reproduced, best ask the issue owner first. I just can't justify working many hours doing shim reviews, as I am (and we're all) busy with "real work".

I think what we're missing here above all is people doing reviews, and perhaps some people from Microsoft who will actively watch issues here, and possibly contribute to the reviewing.

[blschatz]

Thanks @cyphermox. It isn't clear how I assign another review. How do I do that?

[YustasSwamp]

+1 for cross-reviewing if it speeds up the process.

[arrfab]

+1 to also , as mentioned by @cyphermox to have also Microsoft participating in such reviews. It seems they enforce that rule but nobody is really actively spending business hours on this either, meaning no SLA for critical bits like a review before being able to have it processed by Microsoft.

[jsegitz]

We exchanged some emails between Microsoft and the current reviewers, but that didn't lead anywhere. Do we want to pick it up here? Judging from the reaction from Microsoft we need to be able to tag the reviews as "accepted", otherwise they will not proceed

[Doncuppjr]

Who has the power to give the 'Accepted' blessing?

[xnox]

@Doncuppjr good question. I wonder that too. I guess collaborators on this repository?

[Doncuppjr]

Yeah turns out, any registered user can toggle the flags.

[xnox]

@Doncuppjr not sure.... I can't change labels on the issues. Maybe you have special powers?

[xnox]

Reading rejects of past issues, and or comments. Common themes are:

• Non reproducible builds, incomplete instructions to reproduce the builds, build reproducibility degrades with time
• Certs / shim submission is attempted to be used for signing something else but grub & linux kernel, i.e. other bootloaders that don't enforce things or non-linux operating systems
• Dubious code - i.e. trying to sign grub without patches applied, no clear indication which grubs / sources are used
• out of date shim, with partial patches. I.e. trying to sign acient shim, shim with patches dropped, shim with extra unreviewed patches, etc

I wonder if we can make shim builds reproducible, or for example somehow binary patch the shim. Ideally the only difference between various shims should be the certificates. Because then we can do batch submission of identical shim builds which only have the cert as the difference.

Grub is still pain. As upstream grub still does not have complete secureboot patches, and it's not easy to automated review that everyone's individual grubs do the right thing and have enough patches to do the right thing.

Shim upstream. It seems like a few people have cherrypicks on top of v15, and yet v16 is not out yet. It would be nice to get an uptodate shim released.

[julian-klode]

A shim 16 would be super nice, yes.

[bofhbug]

Yeah turns out, any registered user can toggle the flags.

@Doncuppjr
it must be some settings / rights thing, as i cant toggle the flags
mybe the "Restrict editing to collaborators only" right?

[bofhbug]

"If you don’t see [the label] edit buttons, that’s because you don’t have permission to edit the issue. You can ask the repository owner to add you as a collaborator to get access."
form: https://guides.github.com/features/issues/

[blschatz]

"Just do reviews, and assign another review so we can cross-check, I guess. There isn't much more than doing the work."
@cyphermox - So you want two reviews by non-approvers, then you'll approve? Who do we ask for approval and what's the best way?

BTW: I had a positive review 7 months ago

[julian-klode]

You might have noticed the whole lot of grub and shim updates going on, hence people were fairly busy.

I think focus should be on getting a shim 16, and then review submissions of that, rather than all the weird git snapshots and cherry picked commits on top of 15. Because that makes reviews a whole lot easier.

There's also some work planned for getting builds [more] reproducible (to embed hashes of mokmanager and the other thing into shim binary, rather than ephemeral cert), which helps a lot too.

If we can end up in a situation where people can submit reviews and have a GitHub action that verifies reproducibility things would be super nice.

[joseph-zeronsoftn]

Nothing has been accepted in 2020.
Does shim-review still work?
A month has passed since the BootHole issue was also fixed.
Does anyone know?

[xnox]

Nothing has been accepted in 2020.
Does shim-review still work?
A month has passed since the BootHole issue was also fixed.
Does anyone know?

Lots of shim-reviews have been completed and reviewed in private for BootHole using keybase ahead of CRD.

[joseph-zeronsoftn]

@xnox Thanks for your answer :)
So, if the review was conducted privately, my submission will be reviewed soon, too?

Oh, and what does CRD stand for?

And, As far as I know, I am aware that BootHole is an issue in GRUB.
Is this not related to the version of the shim?

[pcmoore]

Oh, and what does CRD stand for?

In this context I'm assuming it stands for "Coordinated Release Date".

[miray-tf]

@xnox

Most review requests here on Github have gone completely unanswered for about a year. So currently shim-review in the recommended way on Github does not work.

You wrote that lots of reviews have been done through keybase. Is there a team to join on keybase to get a review? Or is there any publicly available information?

I noticed that some review requests include a Docker file. Is that now the preferred way to submit a review request?
Or there any preferred platform to build shim on?

I can imagine that the reviewers are very busy but we have not seen any progress for our request which we originally submitted in February. But since our EV CodeSigning certificate has expired more than a month ago, it's really getting crucial and urgent for us. We have also successfully addressed the BootHole issue and resubmitted our request with updated & fixed versions of Shim and GRUB in August. But there has also been no progress.

Do you know of anything else we can do to get Shim reviewed? Like use a specific Linux distribution to build Shim or use spefific tags or branches as base for Shim and GRUB? If that information is documented somewhere I would also appreciate a link.

Any information you could provide will be highly appreciated. Thank you in advance.

[julian-klode]

I have cleaned up the queue now as a first step, added the new BootHole related questions, and I think we are in a good position to move forward now with reviews. I'll try to do some reviews soon too, but so far have only done cleanup work :)

If you submit something for review, it's useful if you review something else, even if you are not a committer to this repo, such that (a) you gain understanding of the review process and (b) reviews can possibly be sped up and trust can be learned.

I noticed that some review requests include a Docker file. Is that now the preferred way to submit a review request?

Yes, certainly. I added an Ubuntu based example to the repo, but you are of course free to use other images. You likely want a mainstream stable distro like Debian stable, RHEL, SUSE, or Ubuntu LTS.