search

slsa-framework / slsa

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

SLSA 1.0 Predicate for npm provenance #798

Open feelepxyz opened 1 year ago

feelepxyz commented 1 year ago

👋 I've been looking at the v1 actions spec to see what we want to include in the provenance statement generated by the npm CLI in an untrusted workflow (when running npm publish --provenance).

I'm currently thinking we should omit the external parameters deployment, release, inputs, vars as we have no way of telling if these have been forged or not. Also, I don't think there's a way to extract vars without having access to the github context, which the npm CLI does not have.

I'm thinking the predicate would look like this, were all properties can be checked against the new Fulcio cert extensions:

{
  "_type": "https://in-toto.io/Statement/v1",
  "subject": [{
    "name": "pkg:npm/sigstore@1.2.0",
    "digest": {
      "sha512": "16bf7e5b59e40522190a425047b8c39ffcc8d145cdb15a69fbb9834240a764e2311bda7ac8d5c1c7dc67b47b1f532607139e570e4915577fab61bae4cc079eb0"
    }
  }],
  "predicateType": "https://slsa.dev/provenance/v1",
  "predicate": {
    "buildDefinition": {
      "buildType": "https://slsa-framework.github.io/github-actions-buildtypes/workflow/v1",
      "externalParameters": {
        "workflow": {
          "ref": "refs/heads/main",
          "repository": "https://github.com/sigstore/sigstore-js",
          "path": ".github/workflow/release.yml"
        }
      },
      "systemParameters": {
        "github": {
          "event_name": "push",
          "repository_id": "495574555",
          "repository_owner_id": "71096353"
        }
      },
      "resolvedDependencies": [
        {
          "uri": "git+https://github.com/sigstore/sigstore-js@refs/heads/main",
          "digest": {
            "gitCommit": "5b8c0801d1f5d105351a403f58c38269de93f680"
          }
        }
      ]
    },
    "runDetails": {
      "builder": {
        "id": "https://github.com/actions/runner/github-hosted"
      },
      "metadata": {
        "invocationId": "https://github.com/sigstore/sigstore-js/actions/runs/1536140711/attempts/1",
        "startedOn": "2023-01-01T12:34:56Z"
      }
    }
  }
}

Does this seem reasonable and look right?

cc @ianlewis @MarkLodato @laurentsimon @kommendorkapten @bdehamer

kommendorkapten commented 1 year ago

To populate the content of any of the external parameters: deployment, release, inputs you would also need access to the GitHub context? Or are they values available via environment variables?

But as they are forgeable when running in a normal workflow, I support the idea of not populating them for npm.

But from a general perspective, it makes sense to have them if they could be added in a non-forgeable, so I don't think the linked v1 actions spec has to be updated.

feelepxyz commented 1 year ago

Or are they values available via environment variables?

You can fish these out by parsing the event.json file at GITHUB_EVENT_PATH which should contain these bits.

But from a general perspective, it makes sense to have them if they could be added in a non-forgeable

💯 yeah for sure makes sense from a trusted/reusable workflow for example.

so I don't think the linked v1 actions spec has to be updated.

Ah yes, the actions spec is only for a untrusted/top-level workflow right?

MarkLodato commented 1 year ago

<aside> I'm going to state a controversial opinion: the Fulcio cert should contain an embedded, JSON-encoded SLSA Provenance predicate rather than a bunch of separate fields. The current design is that the cert is the (GitHub-attested) provenance, and then this provenance is re-encoded in the payload and signed again, but mixed with unattested fields (or really fields attested by the workload, not GitHub). That model is hard to understand and leads to conflicts like what fields you should or should not put in the provenance. IMO it would be much cleaner to have separate layers to make it obvious what party attests to what information. </aside>

I think the original question can be rephrased as follows: is it OK for SLSA L2 provenance to contain external parameters that are not guaranteed by the build system?

The conflict is in the following Provenance is authentic sub-requirements, specifically those labeled (3) and (4) below:

  1. The data in the provenance MUST be obtained from the build service
  2. The build system MUST have some security control to prevent tenants from tampering with the provenance [but with no minimum strength]
  3. Any field that is not marked as REQUIRED for Build L2 [...] MAY be generated by a tenant of the build system
  4. There MAY be external parameters that are not sufficiently captured in the provenance.

The situation in the original post is that:

I don't have an opinion right away, but want to see if I can first precisely capture the problem in terms of the specification.

laurentsimon commented 1 year ago

"buildType": "https://slsa-framework.github.io/github-actions-buildtypes/workflow/v1",

Do you want to add a self-hosted vs github-hosted? I'm fine either way.

But as they are forgeable when running in a normal workflow, I support the idea of not populating them for npm.

+1 from me too

ianlewis commented 1 year ago

But as they are forgeable when running in a normal workflow, I support the idea of not populating them for npm.

👍

"buildType": "https://slsa-framework.github.io/github-actions-buildtypes/workflow/v1",

Do you want to add a self-hosted vs github-hosted? I'm fine either way.

BTW, this is what is suggested in the "official" community repo for GHA. https://github.com/slsa-framework/github-actions-buildtypes/tree/main/workflow/v1#description

I wonder if this isn't something that shouldn't be captured in the builder ID rather than the buildType? The fact that it's github-hosted seems to be captured there at least, though perhaps this should also reflect the fact that it's coming from the npm cli?

"id": "https://github.com/actions/runner/github-hosted"

laurentsimon commented 1 year ago

I wonder if this isn't something that shouldn't be captured in the builder ID rather than the buildType? The fact that it's github-hosted seems to be captured there at least, though perhaps this should also reflect the fact that it's coming from the npm cli?

+1, I got confused. It is in the builder.id, you're correct. The example uses it as well. Ignore my previous comment.

feelepxyz commented 1 year ago

FYI we've got a PR up for this here: https://github.com/npm/cli/pull/6613