Pair and mob programming

[jchestershopify]

Currently (as of), the requirements doc says of reviewing that:

Every change in the revision's history was agreed to by two trusted persons prior to submission, and both of these trusted persons were strongly authenticated.

The term "review" or "reviewer" implies that second, third etc participants do so asynchronously, following from a change being added to the history in some form.

It's unclear to me if this either includes, or excludes, pair programming and mob programming as acceptable instances of two trusted persons. I am strongly of the opinion that such ought to be acceptable for SLSA, so long as "strongly authenticated" can be satisfied.

[leoluk]

With pair programming, wouldn't there still be a review step where the other person will approve in a code review tool the change that was being collaborated on?

[jchestershopify]

I think I see what you're asking, and it shows I left out some assumed context. Pair programming teams often work on the trunk with frequent small commits. That means there's no separate review step as such. Each pair is developing, reviewing and committing synchronously.

[TomHennen]

I agree with @leoluk, but the second party may still want to give the code a second glance (and not just 'blindly' click approve).

I'd worry about a scenario where Party 1 & 2 collaborate on some change, but since they can't both submit the change, Party 1 will submit it. That gives Party 1 (or an attacker on their system) the opportunity to modify what will be submitted. Having Party 2 review the change can prevent this sort of attack.

Or perhaps there are pair-programming systems that can prevent this sort of thing without an explicit review?

[jchestershopify]

If you have two malicious collaborators, I don't see an asynchronous review step as having a major advantage over a synchronous review.

Another practice usually found in pair programming teams is daily rotations. This makes it harder for two malicious collaborators to consistently work together (since they are broken up after a day or two) and makes it harder to hide malicious changes from other team members (since such changes will need to be started and completed in one day).

[TomHennen]

Oh, I'm suggesting this would be a problem if there's only one malicious collaborator and they're the one that's going to submit the change.

[jchestershopify]

I'm not sure I understand. Could you elaborate on the scenario?

[leoluk]

A "committing to trunk without extra review step" workflow implies that each individual collaborator has permissions to write to the trunk branch, making the review a convention vs. a hard requirement enforced by software that cannot be bypassed.

Some projects like Go go even further and require two approvals by identified employees to avoid sybil attacks: https://github.com/golang/go/issues/40699).

[jchestershopify]

A "committing to trunk without extra review step" workflow implies that each individual collaborator has permissions to write to the trunk branch, making the review a convention vs. a hard requirement enforced by software that cannot be bypassed.

Can't the same kind of automation refuse to accept changes that have only one authored-by or committed-by identity? (It seems like a good idea even before reaching SLSA-4, actually).

[leoluk]

Can't the same kind of automation refuse to accept changes that have only one authored-by or committed-by identity? (It seems like a good idea even before reaching SLSA-4, actually).

That's a "soft" check that can be bypassed by a single person.

[jchestershopify]

That's a "soft" check that can be bypassed by a single person.

I can make this argument about self-reviews. How do you prevent those? Through automation which checks for multiple identities attached to the commit or series of commits.

[leoluk]

I can make this argument about self-reviews. How do you prevent those? Through automation which checks for multiple identities attached to the commit or series of commits.

Code review tools typically have strong checks in place for this that cannot be bypassed. For example, Gerrit can be configured to enforce that the commit author matches the strongly authenticated pusher identity, and that approval is given by someone other than the author. This check cannot be maliciously circumvented by a single contributor.

Other code review tools have similar features. To my understanding, tools like Phabricator where the review process can be circumvented by a contributor would not meet the highest SLSA requirement.

[jchestershopify]

I think there are two separate questions being discussed. The first is whether four-eyes is satisfied by synchronous review. The second is about tooling to assure that four-eyes has occurred.

As currently written, the second issue is already addressed:

The platform ensures that no person can use alternate identities to bypass the two-person review requirement.

To satisfy this requirement for synchronous reviews, additional tooling would be required to ensure distinct identities. That this tooling is not common doesn't make it impossible. Organisations wanting to perform synchronous reviews would need to bear the additional burden of creating such automation.

As currently drafted, I don't think the requirement prohibits synchronous review:

Every change in the revision's history was agreed to by two trusted persons prior to submission, and both of these trusted persons were strongly authenticated.

And in fact, now that I've done a forensic reading, that might be my answer: the bar for pair programming teams is higher in terms of building automation, but it's not forbidden outright. If you have two strongly authenticated persons attached to the change, and if it is not possible to make changes solo, you meet the requirement.

[MarkLodato]

I agree with @jchestershopify's interpretation. Any suggestions on how to clarify? Perhaps a FAQ?

[jchestershopify]

I think an FAQ entry would be fine. I think finding a concise wording suitable for the core requirements doc would be quite difficult.

[leoluk]

The scenario I have in mind is one where a synchronous review occurs, but the code that ends up pushed is distinct from the code that was reviewed (either maliciously/through a comprimised toolchain, or by accident).

When doing asynchronous review, the reviewer uses a separate device and strongly authenticated session to view an independent copy of the diff and approve the change.

How would synchronous review address this, other then re-reviewing the diff using an asynchronous review tool?

[jchestershopify]

How would synchronous review address this, other then re-reviewing the diff using an asynchronous review tool?

I looked into this a bit. I think that meeting the desired standard would require multiple git signatures to strongly establish authentic identities. That's not natively supported, though there are schemes based on using git notes.

I still think my reading is correct: it's permissible, but tooling needs to capable of distinct authentications at commit time. At the moment it's a schlep.

[leoluk]

I looked into this a bit. I think that meeting the desired standard would require multiple git signatures to strongly establish authentic identities. That's not natively supported, though there are schemes based on using git notes.

I still think my reading is correct: it's permissible, but tooling needs to capable of distinct authentications at commit time. At the moment it's a schlep.

How would this address the threat model of committing a different change than was reviewed?

[jchestershopify]

How would this address the threat model of committing a different change than was reviewed?

Could you elaborate on the scenario? I want to make sure we're really talking about the same thing, I am concerned that we're talking past each other.

[leoluk]

I'm thinking about "malicious Git binary that will add a backdoor when pushing". A review step will catch this, since the review occurs on an independent tamper-proof copy of the commit.

[jchestershopify]

Couldn't an attacker subvert review tools?

My impression was that the requirement is about having multiple humans involved, so that no one human can make an unreviewed change. Tools seems to be an expansion of the requirement.

[leoluk]

My impression was that the requirement is about having multiple humans involved, so that no one human can make an unreviewed change. Tools seems to be an expansion of the requirement.

In the case of pair programming, the tooling (i.e. local environment used to commit the change) can be subverted by a single person.

With code review (say, on GitHub) this would not be the case.

[jchestershopify]

In the case of pair programming, the tooling (i.e. local environment used to commit the change) can be subverted by a single person.

How do they do this without their pair noticing?

[leoluk]

How do they do this without their pair noticing?

By modifying the local tooling to do it on-the-fly.

[jchestershopify]

And they modify the tooling without being noticed how?

[leoluk]

Before the pairing session? Presumably, the environment that is being used is not a freshly set up machine with full attestation to guarantee that it actually commits the same code that was visually agreed upon.

[jchestershopify]

So popping the stack, is your position that trunk-based pairing is to be forbidden? I'm obviously against that position, but if it is the position it should be explicit.

[leoluk]

Yes, my position is that it's impossible to meet SLSA 4 using trunk-based pairing without a separate review step.

[jchestershopify]

Thanks.

How do you feel about two-branch strategies, where there's a development trunk and a release branch? Assuming two branches, promotions could presumably require some additional review.

[jchestershopify]

Expanding a bit, in $JOB-2 I could see assigning an interrupt pair to review and promote commits as soon as they're made on development. Does that track for you?