On Wed, Oct 22, 2025 at 8:03 AM Piotr P. Karwasz <[email protected]> wrote: > > Hi Matt, > > On 17.10.2025 20:06, Matt Sicker wrote: > > The largest threat of supply chain attacks at this point is from > > third party dependencies, especially given the proliferation of > > Dependabot updates. Changes made to dependencies are never (or > > almost never) reviewed with the same level of rigor as changes made > > to our own code. > > I agree that third-party dependencies can be a supply chain risk. > However, I don’t believe this is a significant concern in our case because: > > - We only have a small number of dependencies. > > - We don’t bundle those dependencies. > > This risk is far more relevant for application developers. For libraries > like ours, I think a much more realistic threat is the compromise of one > of our committer PATs used in personal projects (where code is not > reviewed), which could then be used to push malicious commits to > `logging-log4j2`. If I wanted to attack Log4j, that’s exactly the vector > I would pursue: obtaining temporary access to a token from one of the > ~40 inactive committers. > > > >> - Human oversight: No matter how careful we are, everyone > >> occasionally overlooks small details. That’s exactly what peer > >> review helps catch. > > > > This is what I used to do when I could review the commits@ mailing > > list. Ever since that was flooded with Dependabot-related chatter, I > > have been unable to use the list for post-commit review. It’s not like > > this project moves at a super fast pace! We tend to make quarterly > > releases which surely gives enough time to browse through commits and > > bring up issues to dev@. > > > I agree, Dependabot is a nuisance that needs (and will be addressed). > > However, reviewing via the `commits@` mailing list, even without > Dependabot PRs, has several drawbacks: > > - It is harder to follow changes: instead of a single squashed commit > per PR, we get multiple small commits. > > - Minor fixes (formatting, missing tests, etc.) “post-merge” generate > additional noise in the mailing list. > > - The process is undocumented, so multiple PMC members may review the > same commits redundantly. > > - Although we theoretically have 22 potential reviewers, in practice the > actual number of reviews tends toward zero. > > > > A project with one maintainer using RTC would mean that they can > > only accept external contributions and are otherwise not allowed to > > merge their own code. > > The key point is *active* maintainer. Under RTC, we only need one active > maintainer, plus another committer who occasionally reviews internal PRs.
This is an example of a self-fulfilling prophecy in action. I am currently not actively contributing because of the absurd request in a PR [1] to redo a significant amount of code when all I wanted to do was add a simple getter method. Gary [1] https://github.com/apache/logging-log4j2/pull/3855 > > > >> Personally, I’m flexible about the *order* of review (before or after > >> commit), as long as every commit is reviewed by someone other than the > >> author. If we ever decided to adopt a true CTR model, where every commit > >> is traceably reviewed, I’d be fine with that too. > > > > This is an interesting idea. Making the review process asynchronous > > and non-blocking is my main desire. I’m ok with making follow-up > > changes (it’s software; we can change it infinitely). What I’m not > > fond of is maintaining long-lived branches (an anti-pattern) and > > applying different review standards to different changes (which > > can happen incidentally based on who decides to review the change > > regardless of who made the change). > > The challenge with asynchronous post-commit review is that it still > introduces blockers, just *later* in the process. For example, before > any release, *all* commits must be reviewed. That simply moves the > bottleneck to release time, and peer pressure may push people to > “approve” changes without really reviewing. > > In this specific case, even if I merged your PR without review, nothing > would change for me: I would still have to -1 any release until I > independently review all unreviewed commits. > > Instead of switching to CTR, I suggest we adopt a simple mechanism to > ensure timely and fair reviews: process PRs in creation order whenever > possible. Some time ago I created a GitHub Project board [1] to help > track PR status. We can improve automation later, but for now we can > manage the status manually and use assignees to signal who is handling > each review. > > I have already added your PRs to the board and updated their status: > > - Three PRs are waiting for your responses to review comments. > > - One PR is assigned to me and waiting for my review. > > > > Suppose someone wanted to contribute code to commons-crypto in order > > to support new PQH encryption ciphers recently standardized by NIST > > and IETF. Suppose that person is a cryptographer or works in that > > space in general. Now also suppose that the Commons PMC has > > approximately 0 cryptographic experts. Then nobody feels comfortable > > reviewing the code. > > > If nobody on the PMC can understand a contribution well enough to review > it, then we have two options: > > 1. Reject the PR, or > > 2. Invite the author to become a committer so they can maintain the code. > > However, relying on a single expert is not sustainable. At least two > committers should feel comfortable maintaining a code area. History has > shown we frequently need to modify code we didn’t write ourselves. > > Piotr > > References: > [1] https://github.com/orgs/apache/projects/501/views/1
