slsa-framework / slsa

Supply-chain Levels for Software Artifacts
https://slsa.dev
Other
1.55k stars 225 forks source link

Feedback on v1.0 RC #653

Open jchestershopify opened 1 year ago

jchestershopify commented 1 year ago

I'm a little unsure how this requirement works:

It MUST NOT be possible for two builds that overlap in time to influence one another. [doc]

I appreciate the intent, certainly. I shouldn't be able to write a build that reachers sideways into another container or VM and interfere with its operation.

But I don't know how to consolidate the phrasing with the existence of external state. Often two jobs may be both using a shared resource (eg reading and writing to a blob store). They can potentially race on that external state.

To uphold this requirement as phrased would require locking access to external state, which might be difficult or impossible to achieve. Is that the intention?


As for the rest, all I had was nits. Firstly:

For example, if a producer wishes to distribute their artifact through a package ecosystem that requires explicit metadata about the build process in the form of a config file. That metadata includes the artifact’s source repository and build parameters that stay constant between builds. The producer MUST complete that config file and keep it up to date. [doc]

It's unclear to me if the last sentence is true of the example or whether it is true in general. I think it's meant to be true of the example, in which case I suggest the last sentence be changed to:

In this example, the producer would be REQUIRED to complete that config file and keep it up to date.

Next some bikeshedding:

Accuracy: How resistant is the provenance generation to tampering within the build process? [doc]

Accuracy in this description sounds more like integrity, but later it includes factors such as ensuring that provenance is obtained from the build service and that the build service has controls to prevent tenants from tampering with the provenance.

I'm not sure what other words might have been canvassed. I think something like "trustworthiness" might go further; insofar as it is a word that expresses the degree to which something can justifiably be trusted (vs whether it is trusted, which is a property of the consumer's policies).

Next:

Completeness: SHOULD be complete, but there MAY be external parameters that are not sufficiently captured in the provenance. [doc]

I got confused by this one at first. When I start to parse the sentence I begin with "SHOULD be complete" and I am immediately unsure what is meant to be complete. It's not until the end of the sentence that I have a particular object. I'd suggest starting the sentence with provenance:

Completeness: provenance SHOULD be complete, but there MAY be external parameters that are not sufficiently captured.

Finally:

All listed under "low privilege". [doc] [...] All listed under “low privilege” and “medium privilege”. [doc]

I think these are vestigial. From context they are probably meant to read "Project contributors" and "Project maintainer" respectively.

arewm commented 1 year ago

To add onto the first nit

For example, if a producer wishes to distribute their artifact through a package ecosystem that requires explicit metadata about the build process in the form of a config file. That metadata includes the artifact’s source repository and build parameters that stay constant between builds. The producer MUST complete that config file and keep it up to date. [doc]

This whole block doesn't make sense to me.

Additionally, there is no description of what type of configuration file might be of interest here or in https://slsa.dev/spec/v1.0-rc1/terminology. I assume that the most relevant line is

A tenant defines the build by specifying parameters through some sort of external interface, often including a reference to a configuration file in source controldoc

If so, can we formalize this terminology on that page?

Generally, when referring to specific terminology in the specification, can we link to the item on the terminology page on its first use (or even better, every use)?

arewm commented 1 year ago

@jchestershopify, I updated the wording around isolated and ephemeral. Does the content change in #700 resolve this issue? You can see the latest rendered content at https://slsa.dev/spec/v1.0/requirements.

jchestershopify commented 1 year ago

I think #700 is an improvement! Though it may be worth cross-referencing externalParameters, I had to skim some other docs to find its definition.

arewm commented 1 year ago

I created #751 as well to clarify some points made here. I think that #717 should hopefully add additional clarification -- especially around the generated provenance accuracy as it needs to come from the trusted control plane.