-
Notifications
You must be signed in to change notification settings - Fork 49
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
SLSA v1.0 verification support for npm packages built by the "Trusted Builder" #449
Comments
To get the ball rolling, below are possible options to our CLI arguments. Feel free to add other options. Option 1
Will we ever need to support npm "things" that are not tarballs? Option 2
The ecosystem may mirror the ones OSV uses. Option 3
|
I'm leaning towards option 2 as that simplifies the help interface, fewer commands that just redirect to the proper implementation. |
That is unless we need to have ecosystem specific options in the future for some reason but I can potentially foresee some verification options that we'll want for some ecosystems but not others. |
+1 on what @ianlewis said. I'm also worried about such options. I'm leaning towards option 3 to have namespaces. Option 1 seems OK-ish, but I'm worried that in the future we will end up adding a I think it's beneficial to be explicit with command names rather than trying to be generic. |
Right, if we have ecosystem specific options then option 2 is no longer viable, it will cause more complexities in handling the flags, making sure all needed options exist. In that case, option 3 too. |
I played a bit with the provenance generated by the runner for npm in https://github.com/laurentsimon/provenance-npm-test/blob/main/download-verify.sh Looks like we'd have at least 2 options: # 1. Both attestation (publish and slsa attestations in a single file, probably the common case)
slsa-verifier verify-npm-package "$tarball_url" \
--attestations-path "$attestations_url" \
--source-uri github.com/repo/name \
--package-name "$package_name"
# 2. Attestation in a separate file
slsa-verifier verify-npm-package "$tarball_url" \
--provenance-path "$provenance_attestation_file" \
--publish-attestation-path "$publish_attestation_file" \
--source-uri github.com/repo/name \
--package-name "$package_name"
# 3. From URL - this requires more thinking because the provenance may be private and require login to access, ie it may need to shell out to npm CLI
slsa-verifier download-npm-package "$package_name" \
--source-uri github.com/repo/name \
--tarball-path "$tarball_file" # NOTE: we could let users pipe the result wherever they want instead of having the tarball-path option Starting with command 1 would make sense, I think. Thoughts? |
Is this a url that we'll download from? Or maybe a path to a directory of attestations (sigstore bundles)? If it's a directory then I assume we'll check all for matching subjects? Can you provide what you expect the full set of commands to be? i.e. how will folks download the tarball and attestations? Do they need to parse the attestation urls out of the API and download them manually? Can we maybe do a lot of this work for them? |
sorry, the
|
Ok, so we need them to do something like
|
That's what I proposed, but up for discussion. Note that there are 2 attestations returned by |
Right. I'd like to make it easier on users and have the verifier download the package tgz and attestations itself and output them somewhere if they verify but maybe it's simpler if users download it themselves. Currently you can download a package tarball with Something like: $ npm pack --with-attestations @laurentsimon/provenance-npm-test@1.0.0
...
laurentsimon-provenance-npm-test-1.0.0.tgz
laurentsimon-provenance-npm-test-1.0.0.attestations.json And then slsa-verifier verify-npm-package laurentsimon-provenance-npm-test-1.0.0.tgz \
--attestation-path laurentsimon-provenance-npm-test-1.0.0.attestations.json \
--source-uri github.com/laurentsimon/provenance-npm-test \
--package-name @laurentsimon/provenance-npm-test Here we can just take the full attestation list and parse out the SLSA attestations we care about rather than complicating the npm cli with separate options for build and publish attestations (and potentially more if they are added) or making users parse it out themselves. wdut? BTW, do we need a |
Yes, downloading the attestation is a feature we could offer in the future. I stayed away from it mostly due to complications around authentication, e.g., if the package is private. I'm under the impression we would have the shell out to +1 on asking npm to have npm pack (or another command) download the attestation.
That's what the PR I sent does.
Users may want to explicitly verify the package name so I added it. I'm not sure what you mean by "taken" from the package.json. I know it's in the manifest, but we won't unpack the tarball ourselves. Or do you mean that if users care they can unpack the tarball and see for themselves? NOTE: to prevent downgrade attacks, I also added a |
Yeah, I was wondering why we wouldn't just unpack it and look at the manifest. Why would the option that the user supplies ever be different from that value? Is unpacking it problematic? I kind of feel like the value we should really be checking is the expected git ref/tag. Maybe rewording the option would make that clearer? So I think we can check two things:
|
Thinking about it again, the tarball is untrusted and the subject should be treated the same way. I do think we should check the manifest contains the right package name though since that's the name under which the package actually gets installed. You don't want the tarball to overwrite an already installed package for example. |
Good call. I think I took the view that the publish attestation guarantees that already (the registry verifies it at provenance / package upload). |
Yeah, I suppose it's a question for the verifier whether we trust the publish attestation though I'm not totally sure of the guarantees it provides. My understanding was that it attests to when and by whom the package was published, but I'm not sure what else it might guarantee. Not sure if there is any info anywhere. Maybe we can create an issue in the private beta repo and see if we can find out? |
From its content, the publish attestation only attests to the package name and version: {
"_type": "https://in-toto.io/Statement/v0.1",
"subject": [
{
"name": "pkg:npm/%40laurentsimon/provenance-npm-test@1.0.0",
"digest": {
"sha512": "29d19f26233f4441328412b34fd73ed104ecfef62f14097890cccf7455b521b65c5acff851849faa85c85395aa22d401436f01f3afb61b19c780e906c88c7f20"
}
}
],
"predicateType": "https://github.com/npm/attestation/tree/main/specs/publish/v0.1",
"predicate": {
"name": "@laurentsimon/provenance-npm-test",
"version": "1.0.0",
"registry": "https://registry.npmjs.org"
}
}
If we verify this publish attestation, we implicitly what it attests to. I agree that the verification it does needs to be documented. |
Right, I think the implicit assumption is "verified that the registry attests to the publication of the package" -- just in case, let's say, the registry store was compromised. I don't think there's any other implicit assumption. +1 on posting whether it implicitly verifies the build attestation.
I would assume the benefit of having this be an ecosystem specific command is so that we fetch the attestation already, knowing the ecosystem stores it? Otherwise, could we not achieve the same benefit (for the trusted builders) by some shell scripting and the existing |
initial implementation in #495 |
Fixes #614, #450, #449, #515 Adds support for NPM CLIs build provenances, generated when running `npm publish --provenance --access public` from a [GitHub Actions workflow](https://github.com/ramonpetgrave64/gundam-visor/blob/599500821344b070902a7a5666064bfdaba715df/.github/workflows/npm-publish.yml#L21). ## Testing - added unit tests for some new helper functions - added regression test cases ## Future work - #493, so we can do `--print-provenance` - implemented in #768 (comment) --------- Signed-off-by: Ramon Petgrave <ramon.petgrave64@gmail.com>
Verification support for the "Trusted Builder" as defined in RFC-0049
The text was updated successfully, but these errors were encountered: