feat: npm default runner support

[laurentsimon]

This PR adds support for the npm provenance verification. It requires some end-to-end tests in a follow-up PR.

The implementation verifies:

1. The publish attestation
2. The provenance attestation.

NOTE:

• The builder ID for the GitHub runner is correctly set to 'https://github.com/actions/runner', but needs to be updated according to [feature][npm] Set the GitHub runner builder ID #494
• no support for source-branch, source-tag, source-versioned-tag and print-provenance
• no e2e test, in follow-up PR once we've agreed onn the design of this PR

[ianlewis]

nit: If we care that the publish attestation is there, shoudn't we do anything to actually verify it? maybe provide options for it, like --publish-user or something?

[laurentsimon]

we verify its signature in verifyPublishAttesttationSignature(), its headers in verifyIntotoHeaders() and its content in verifyPackageName(). The --attestations-path verifies both attestations, but we may have dedicated --publish-attestation-path and --provenance-path to separate verification for each in the future.

Wdut?

[ianlewis]

I think it's ok for now, as long as npm package verification is an experimental feature.

Though I wonder if we should require it. I'm not sure what the private npm repository experience would be like. Will we get publish attestations for all registries that support SLSA attestations? or could npm repositories support SLSA provenance attestations without necessarily creating publish attestations?

https://github.com/gh-community/npm-provenance-private-beta-community/issues/13#issuecomment-1447569734

[asraa]

Is there a real difference between this and provenance-path? The only thing is that this is supposed to be supporting JSONL (multiple provenance concat). Maybe better to just allow provenance-path to support that and converge these?

[laurentsimon]

I think the publish attestation is not a SLSA provenance: predicateType: https://github.com/npm/attestation/tree/main/specs/publish/v0.1 which is why I'm hesitant to hide them both under the provenance-path. There may be some uses cases where users only have a provenance file and want to verify it anyway, in which case we would support a --provenance-path, e.g. when they store provenance in a differnent registry that does not support the publish attestation.

The term "attestation" is consistent with what npm developers would be used to, as the attestations are fetched thru the npm view | jq .dist.attestations.url

I think the namespacing of verification options (verify-artifact, verify-image, verify-npm-package) lets us adjust to each ecosystem more freely.

That's the motivation, at least. Let's discuss it more.

Wdut?

[laurentsimon]

@ianlewis @mihaimaruseac let us know if you have thoughts on this

[asraa]

On the other hand though, a cosign attestation file alongside the artifact contains many other predicate types, not just the SLSA ones, just like this case.

I agree that the naming of attestation is actually better than provenance...

There may be some uses cases where users only have a provenance file and want to verify it anyway, in which case we would support a --provenance-path, e.g. when they store provenance in a differnent registry that does not support the publish attestation.

Ah, this is a little confusing to me then - provenance allows the SLSA provenance, and --attestation-path may be both, but at least including publish attestation?

[laurentsimon]

related thread https://github.com/gh-community/npm-provenance-private-beta-community/issues/13

[asraa]

Resolving this: let's keep this attestation-name, when we major version bump let's file an issue changing other builders to attestation-name as well?

edit: plus also allowing JSONL in all the verification commands is better

[laurentsimon]

Created #512 for tracking.

[asraa]

does the issuer check need to be added like it is above?

maybe you can consolidate these codes by using a special error case when it's not a known trusted builder in the VerifyWorkflowIdentity

[laurentsimon]

re: consolidate. I separated VerifyBuilderIdentity() and VerifyCertficateSourceRepository().
re: issuer check was done at the start of the function

slsa-verifier/verifiers/internal/gha/builder.go

Line 98 in f0640b1

if !strings.EqualFold(id.Issuer, certOidcIssuer) {

, but I've updated the code with separate VerifyBuilderIdentity() and VerifyCertficateSourceRepository(). Let me know what you think

[asraa]

I'm a little bit concerned there is going to be three dupes of this code, our old code, and the delegator code, but we they are all different cases and it may need to be that implementing general delegator builds may need to refactor some of this later - in my view the npm builder is a special case of the delegator builds with additional verifications and check for the L2 builders.

[laurentsimon]

I'm a little bit concerned there is going to be three dupes of this code, our old code, and the delegator code, but we they are all different cases and it may need to be that implementing general delegator builds may need to refactor some of this later - in my view the npm builder is a special case of the delegator builds with additional verifications and check for the L2 builders.

I agree I'd like to remove the duplication and re-factor the code too. I may have missed: do we already have some BYOB verification?

[asraa]

I may have missed: do we already have some BYOB verification?

Not yet: I had implemented it in the PR I staged here, but was left at generating testcases with test repositories and tagged builders.

[laurentsimon]

Merging this as discussed offline. I have created issues to track some of the remaining questions. I will cut an -rc release