Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

One-person review? #94

Open
joshuagl opened this issue Jul 12, 2021 · 9 comments
Open

One-person review? #94

joshuagl opened this issue Jul 12, 2021 · 9 comments
Labels
discussion spec-change Modification to the spec (requirements, schema, etc.)

Comments

@joshuagl
Copy link
Member

SLSA 4 calls for two-person informed review of all submissions. The current framing indicates that levels 1 through 3 are OK with authors submitting code without any review.

Should one of the lower SLSA levels call for some informed review by a single independent reviewer?

@joshuagl joshuagl added spec-change Modification to the spec (requirements, schema, etc.) discussion labels Jul 12, 2021
@jchestershopify
Copy link
Contributor

I agree that some kind of reviewing or four-eyes rule should be in effect at lower SLSA levels.

(Xref #95, which is tangentially related.)

@TomHennen
Copy link
Contributor

This had been discussed but we got feedback that this is a very difficult thing to ask of single-maintainer projects. See #4, #93 for some discussion on why this is hard and what we can do to help projects get 2-party review.

Since the fact is that lots of projects don't have anyone to do an independent review, requiring it at lower SLSA levels would significantly inhibit their ability to adopt SLSA.

@MarkLodato
Copy link
Member

To clarify, the current SLSA 4 requirement is that every change involves two "trusted persons", counting the author. The "trusted person" requirement is to prevent sock puppet reviews. Thus, it is OK for one trusted person to author and a single trusted person to review. If an untrusted account authors, then two trusted persons must review. Also, "data owners" must not be able to bypass this (#110).

Any thoughts on how to relax it for lower levels? A few ideas I've heard:

  • Dropping the "trusted persons" requirement, so any other reviewer is OK.
  • Dropping the "cannot be bypassed" requirement, so that code review isn't a hard blocker.
  • Reducing to some percentage of changes, as is required by the CII Best Practices "Gold" badge.
  • Something else?

Related: #111 for the rationale on why the requirement exists at lower levels.

@nasifimtiazohi
Copy link

@MarkLodato how do we measure "trust" in this case?

@MarkLodato
Copy link
Member

@MarkLodato how do we measure "trust" in this case?

Do you mean "trusted person"? This is defined at https://slsa.dev/spec/v0.1/requirements#definitions:

Trusted persons: Set of persons who are granted the authority to maintain a software project. For example, https://github.com/MarkLodato/dotfiles has just one trusted person (MarkLodato), while https://hg.mozilla.org/mozilla-central has a set of trusted persons with write access to the mozilla-central repository.

@nasifimtiazohi
Copy link

@MarkLodato how do we measure "trust" in this case?

Do you mean "trusted person"? This is defined at https://slsa.dev/spec/v0.1/requirements#definitions:

Trusted persons: Set of persons who are granted the authority to maintain a software project. For example, https://github.com/MarkLodato/dotfiles has just one trusted person (MarkLodato), while https://hg.mozilla.org/mozilla-central has a set of trusted persons with write access to the mozilla-central repository.

Why don't we simply refer to the "trusted persons" as maintainers? "Trust" sounds more like a subjective quality and is hard to measure.

@MarkLodato
Copy link
Member

Why don't we simply refer to the "trusted persons" as maintainers? "Trust" sounds more like a subjective quality and is hard to measure.

Good idea. I'm open to that.

Are there any cases where "maintainers" is worse than "trusted persons"? I can't think of any but it's worth asking.

@nasifimtiazohi
Copy link

Are there any cases where "maintainers" is worse than "trusted persons"? I can't think of any but it's worth asking.

Well, I am not sure about the exact definition of maintainer as well. If it is only about write access to a GitHub project, then a compromised maintainer can add a sock puppet account as another maintainer to bypass the review restrictions. But then again, a compromised maintainer with enough permissions probably can do a lot of other things as well.

On the other hand, a "trusted person" may indicate that trust is established based on some past activity for a certain developer account.

@MarkLodato
Copy link
Member

Let's move the "maintainers" term discussion to #306, which is a larger terminology thread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion spec-change Modification to the spec (requirements, schema, etc.)
Projects
Status: Untriaged
Development

No branches or pull requests

5 participants