New Access Control criteria - second-party code review

[evankanderson]

As discussed today. This ties in with e.g. OSPS-DO-11 that contributors (and reviewers?) need to be vetted. (Also #119)

[funnelfiasco]

Do you mean that two reviewers are required, or does the "two distinct individuals" include the person who wrote the change? I think you intended the latter. Not quite sure what I'd suggest to clarify yet, but I'll think about it more.

[funnelfiasco]

Oh, if I'd looked at the implementation, I'd see that you say "at least one approval" :-)

[evankanderson]

I'm assuming the person(s) who authored the change implicitly approve of it.

Interestingly, when working in pair authoring setups with co-authored-by, you can sometimes end up needing more than two approvals with the default GitHub approval mechanism. That's really just a "you need other tooling than GitHub" situation (for which things like Prow exist).

I'm happy to accept suggestions on verbiage; I'm not as familiar with what the standard language may be for these types of controls. (I care about the implementation of the control, but almost zero about the naming and standardization...)

[SecurityCRob]

I like the idea of additional review prior to commit

[david-a-wheeler]

This criterion cannot be met by 1-person projects. The vast majority of OSS projects are 1-person projects (per TAC issue 101), and this is not under the control of maintainers. Maintainers can take steps to make it easier to join a project, and they should, but maintainers don't decide how others spend their time.

For projects with 2-3 developers who are not doing this as part of their daytime work, requiring second review can greatly slow development. You're waiting for the other guy to be available, which may be next month or maybe the month after that. Maybe.

Don't get me wrong, I'm a huge fan of the advantages of review before merging. I wrote a book on Software Inspection (a form of multi-person review). But while describing it is easy, and enforcing this is relatively easy, that doesn't make extra people available for the vast majority of OSS projects.

If the intent is that this baseline cannot be applied by the vast majority of OSS projects, then that needs to be made clearer, e.g., "this baseline is only intended for larger projects with multiple funded developers" or whatever is the intent. We should make that clearer if that's the intent.

[evankanderson]

If the intent is that this baseline cannot be applied by the vast majority of OSS projects, then that needs to be made clearer, e.g., "this baseline is only intended for larger projects with multiple funded developers" or whatever is the intent. We should make that clearer if that's the intent.

I totally agree! There are a number of other level-1 criteria that seemed biased towards multi-maintainer projects, for example OSPS-DO-01 (public discussions about proposed changes), OSPS-AC-02 (lowest available permissions), and OSPS-AC-03 (require pull requests).

[david-a-wheeler]

I've been on a couple 2-3 person projects including one right now where we enforce this and—yeah— it slows things down, but it dramatically reduces the defects we introduce.

I agree with you! I have no doubt that it dramatically reduces defects. When you have many developers, human review is an excellent thing. NB: I was co-editor of an IEEE book specifically on software inspections. You don't have to convince me of the value of human review; I have tons of evidence to help you support that claim.

I think the discussion on this criterion is helping me crystallize a key difference between the "OpenSSF Best Practices Badge" criteria and the "Security Baseline" criteria here:

• The OpenSSF Best Practices Badge focuses on increasing the security of all OSS projects, and since most are single-person projects, and many of the rest are not much bigger, mandates like multi-person review are not considered until the "gold" level. If a project can't reach even the lowest level of some set of criteria, the project will tend to ignore that set. The Best Practices Badge has no criteria that requires multiple maintainers until "gold".
• It appears that the "Security Baseline" is focusing on OSS projects with multiple maintainers, including those where there is a foundation operating as the OSS project's steward.

I realize this PR is focused on a specific criterion, but I think this discussion about a specific criterion is helping to make this distinction clearer.

Some of the other distinctions, such as "we only focus on security", aren't as convincing to me. For example, having automated tests is critically necessary for security long-term, because you can't modify code or update dependencies in a timely way to eliminate vulnerabilities without automated tests. In short, many things you might say "aren't a security issue" turn out to be security issues. On the other hand, "must have multiple developers" is a clear distinctor.

Is this the road we wish to continue to go down?

[eddie-knight]

Good points, @david-a-wheeler.

One thing I want to highlight is that ANY project should be able to reach level 1.

If a project can't get a second reviewer, or just doesn't bother to... I think it's fair to say that it's not fully mature.

[david-a-wheeler]

If a project can't get a second reviewer, or just doesn't bother to... I think it's fair to say that it's not fully mature.

I think that confirms the distinction I'm pointing out. Most OSS, including a lot of software used in proprietary software and enterprise systems, cannot do this, because there are no extra people. It's not a "bother to"; you can't conjure up other people when you want them to exist.

Calling them "not fully mature" is, I think, not quite correct or maybe misleading. Many such OSS projects been around for many years and are widely depended on. The internet wouldn't run without them. Trivial example: xz utils was released on January 14, 2009, about 16 years ago, and is basically 1 person (Lasse Collin). Per Wikipedia, "a number of Linux distributions, including Fedora, Slackware, Ubuntu, and Debian use xz for compressing their software packages". It's been hard to get a second person to co-maintain xz utils. The effort to bring in a second person brought in someone who turned out to be a malicious attacker who inserted a backdoor, caught in 2024.

Would it be better if such software had multiple maintainers? Of course that's generally true, though xz utils is a cautionary tale. In at least some cases, the current maintainer should make changes to increase the likelihood of gaining other maintainers, and it might succeed. But sometimes those others don't appear, even when you "do everything right".

[eddie-knight]

I think it's really interesting that you brought up xzutils, because I see that as cautionary in a different way. 🤔

With hindsight— If adopters considered maintenance capabilities as part of the project's maturity, it may not have been baked into critical infrastructure prior to resolving the single maintainer issue.

Because it was baked in without multiple maintainers, we came to a point in history where a piece of critical infrastructure was being maintained by a single person who could be overwhelmed by external pressure to add an untrustworthy maintainer.

[david-a-wheeler]

With hindsight— If adopters considered maintenance capabilities as part of the project's maturity, it may not have been baked into critical infrastructure prior to resolving the single maintainer issue.

It's likely they did consider that it was single-maintainer. But it would be hard to implement systems without single-person projects, as they are quite common.

[mlieberman85]

So we have a requirement here in the SLSA build track around this. Can we use similar language and map to SLSA?

[puerco]

+1 on aligning on SLSA, especially around bots and maturity level.

[david-a-wheeler]

Suggested change

	least one approval before merging.
	least one approval before merging.
	Bots do not count as one of those two persons.

[david-a-wheeler]

Suggested change

	second-party approval of commits before merging
	at least one second-party approval of commits before merging

[puerco]

The changes proposed in this PR are consolidated in #134