-
Notifications
You must be signed in to change notification settings - Fork 50
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
feat: VerifyNpmPackage API with supplied tuf client #768
base: main
Are you sure you want to change the base?
feat: VerifyNpmPackage API with supplied tuf client #768
Conversation
Signed-off-by: Ramon Petgrave <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great, thanks so much for working on this. I have a few minor nits but overall it's looking good.
@@ -87,9 +90,9 @@ type mockSigstoreTufClient struct { | |||
fileContentMap map[string]string | |||
} | |||
|
|||
// NewMockSigstoreTufClient returns an instance of the mock client, | |||
// newMockSigstoreTufClient returns an instance of the mock client, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not for right now but in the future we might consider pulling out this mock client into its own test package so others can use it in tests (https://google.github.io/styleguide/go/best-practices.html#creating-test-helper-packages)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll be considering that. right now, the mock is very simple, so I guess folks can just copy-paste it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll followup in #773
Thanks for the review! I was also looking into logging in #772, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. A few minor nits but overall LGTM
provenanceOpts *options.ProvenanceOpts, | ||
builderOpts *options.BuilderOpts, | ||
sigstoreTufClient utils.SigstoreTufClient, | ||
) ([]byte, *utils.TrustedBuilderID, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this func is following from VerifyNpmPackage
but I wonder if it should return the data in a more structured manner to make it easier for callers to inspect the fields they're interested in. At the moment, you'd have to parse the attestation yourself to get information from it (except builder id which is helpfully extracted). That's quite involved because the structure of the attestation is complex.
I think this is beyond the scope of what this PR is trying to do, so don't worry about it for these changes but maybe something to think about for the future? Even just having slsa-verifier
expose some of its functions for parsing an attestation could work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the returned []byte
, I think that's what StatementsFromBytes
is for, but its not yet implemented for npm.
- https://pkg.go.dev/github.com/slsa-framework/slsa-verifier/[email protected]/verifiers/utils#StatementFromBytes
slsa-verifier/verifiers/internal/gha/npm.go
Lines 224 to 229 in 87b5bae
func (n *Npm) verifiedProvenanceBytes() ([]byte, error) { // TODO(#493): prune the provenance and return only // verified fields. // NOTE: we currently don't verify the materials' commit sha. return []byte{}, nil }
@laurentsimon, @ianlewis , regarding #493, is it now safe to return the entire provenance, once verified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so.
if it should return the data in a more structured manner
@ramonpetgrave64 That's maybe what we discussed a few weeks ago, to have a layered approach to verification where the inner layer returns a structured "condense" / summary for callers to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added printing in a new commit
@slugclub thanks again. @ianlewis @laurentsimon , please take a look |
register/register.go
Outdated
@@ -34,6 +34,12 @@ type SLSAVerifier interface { | |||
provenanceOpts *options.ProvenanceOpts, | |||
builderOpts *options.BuilderOpts, | |||
) ([]byte, *utils.TrustedBuilderID, error) | |||
|
|||
VerifyNpmPackageWithSigstoreTUFClient(ctx context.Context, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we chatted about offline, we may want this API to take the npm verification material directly and another API responsible for fetching this target file from TUF. Then this API could verify metadata offline, and the latter API would be responsible for network calls. I think this would also make testing easier since you don't have to mock the Sigstore TUF client, just need the verification material struct initialized.
Another change, outside of the scope of this PR, would be to update https://github.com/slsa-framework/slsa-verifier/blob/main/verifiers/internal/gha/trusted_root.go to use TrustedMaterial rather than Cosign's APIs. Like you're already doing in https://github.com/slsa-framework/slsa-verifier/blob/main/verifiers/internal/gha/bundle.go#L162, rather than passing the TUF client, you would pass in the trusted root material.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Speaking about this offline again, we have our own struct to parse the material, but the spec/layout of the material isn't defined in npmjs docs. Until we can be sure that npmjs will commit to this layout, I think it's better to hide this detail from the user, or let the user stub out their own SigstoreTUFClient
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should recommend implementing a SigstoreTUFClient
to bypass TUF. I think this API is fine for the current use case, though taking in the verification material (keys) directly could be another way to implement this API while not requiring knowledge of the npm struct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you want to provide an option tuf client as input (for unit testsing, etc), in the long term consider using variadic options / arguments. For a quick turnaround, you may add a pointer to a tuf client in the existing builderOpts https://github.com/slsa-framework/slsa-verifier/blob/main/options/options.go#L36 and keep the existing API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're referring to using variadic options like in this blog post, I decided on a bit of a hybrid approach in my latest commits.
|
||
### Npmjs | ||
|
||
With `VerifyNpmPackageWithSigstoreTUFClient`, you can pass in your own TUF client with custom options. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you want to hack on the existing API or create a cleaner / new set of APIs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't call it a hack. But your other comment got me thinking I could expose the Npm
struct externally, or make a new NpmVerifier
struct. And we could still use that TUFClient to let the user pass in their own TUF root.
type NpmVerifier struct {
Ctx context.Context
AttestationBytes []byte
PovenanceOpts *options.ProvenanceOpts
BuilderOpts *options.BuilderOpts
VerifierOpts *options.NpmVerifierOpts // hypothetical new type
}
type VerifierOpts struct { // hypothetical new type
Logger *log.Logger
offline bool // whether to force offline verification
}
type NpmVerifierOpts struct { // hypothetical new type
*VerifierOpts
TUFClient *utils.TUFClient
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@laurentsimon Alright I decided to do both this new struct and the variadic arguments in options.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another question following from Laurent's - how much leeway do we have to change this API? Does it have many users currently? Would it be possible to make a new release with a breaking change? (Not saying we necessarily have to do that just wondering what the constraints are here).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose we could have lots of leeway. Personally, I'd rather we get this into v2, and then later on add the breaking changes in a new v3.
Signed-off-by: Ramon Petgrave <[email protected]>
Signed-off-by: Ramon Petgrave <[email protected]>
Signed-off-by: Ramon Petgrave <[email protected]>
Signed-off-by: Ramon Petgrave <[email protected]>
Signed-off-by: Ramon Petgrave <[email protected]>
Add a new Post-Commit workflow, to make these renovate-bot updates a bit easier. Previously, we had to clone the PR locally, run `make package`, and then push to the PR. Now we would just need to use the github UI to invoke this new workflow against the PR number. We could also copy this over to the slsa-github-generator repo. > A workflow to run against renovate-bot's PRs, > such as `make package` after it updates the package.json and package-lock.json files. > The potentially untrusted code is first run inside a low-privilege Job, and the diff is uploaded as an artifact. > Then a higher-privilege Job applies the diff and pushes the changes to the PR. > It's important to only run this workflow against PRs from trusted sources, after also reviewing the changes! ## Testing. Tested in my own private fork, where when applicable, it pushed a commit of changes to `dist/` folders - https://github.com/ramonpetgrave64/slsa-verifier/actions/runs/8806815483 - https://github.com/ramonpetgrave64/slsa-verifier/pull/8/commits - https://github.com/ramonpetgrave64/slsa-verifier/actions/runs/8806841353 - https://github.com/ramonpetgrave64/slsa-verifier/pull/16/commits --------- Signed-off-by: Ramon Petgrave <[email protected]> Signed-off-by: Ramon Petgrave <[email protected]> Signed-off-by: Ramon Petgrave <[email protected]>
…#717) [![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [@actions/core](https://togithub.com/actions/toolkit/tree/main/packages/core) ([source](https://togithub.com/actions/toolkit/tree/HEAD/packages/core)) | [`1.10.0` -> `1.10.1`](https://renovatebot.com/diffs/npm/@actions%2fcore/1.10.0/1.10.1) | [![age](https://developer.mend.io/api/mc/badges/age/npm/@actions%2fcore/1.10.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/@actions%2fcore/1.10.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/@actions%2fcore/1.10.0/1.10.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/@actions%2fcore/1.10.0/1.10.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- > [!WARNING] > Some dependencies could not be looked up. Check the Dependency Dashboard for more information. --- ### Release Notes <details> <summary>actions/toolkit (@​actions/core)</summary> ### [`v1.10.1`](https://togithub.com/actions/toolkit/blob/HEAD/packages/core/RELEASES.md#1101) - Fix error message reference in oidc utils [#​1511](https://togithub.com/actions/toolkit/pull/1511) </details> --- ### Configuration 📅 **Schedule**: Branch creation - "before 4am on the first day of the month" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/slsa-framework/slsa-verifier). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy44LjEiLCJ1cGRhdGVkSW5WZXIiOiIzNy4zNDAuMTAiLCJ0YXJnZXRCcmFuY2giOiJtYWluIn0=--> --------- Signed-off-by: Mend Renovate <[email protected]> Signed-off-by: github-actions <[email protected]> Co-authored-by: github-actions <[email protected]> Signed-off-by: Ramon Petgrave <[email protected]>
Signed-off-by: Ramon Petgrave <[email protected]>
Signed-off-by: Ramon Petgrave <[email protected]>
Signed-off-by: Ramon Petgrave <[email protected]>
Signed-off-by: Ramon Petgrave <[email protected]>
Signed-off-by: Ramon Petgrave <[email protected]>
Signed-off-by: Ramon Petgrave <[email protected]>
Signed-off-by: Ramon Petgrave <[email protected]>
docs/API-Library.md
Outdated
verifierOptioners := []options.VerifierOptioner{ | ||
options.WithSigstoreTUFClient(client), | ||
} | ||
_, outBuilderID, err := apiVerify.VerifyNpmPackageWithSigstoreTUFClient(context.Background(), attestations, tarballHash, provenanceOpts, builderOpts, verifierOptioners...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@slugclub small change in the interface. We're passing in some variadic arguments, instead of passing in the the client directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to using options to set the TUF client. Is there a reason to pass this in as a variadic options.VerifierOptioner
rather than something like a *options.VerifierOpts
like what we have on lines 332 and 333 for provenanceOpts
/builderOpts
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only so that we don't break the current API
docs/API-Library.md
Outdated
verifierOptioners := []options.VerifierOptioner{ | ||
options.WithSigstoreTUFClient(client), | ||
} | ||
_, outBuilderID, err := apiVerify.VerifyNpmPackageWithSigstoreTUFClient(context.Background(), attestations, tarballHash, provenanceOpts, builderOpts, verifierOptioners...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to using options to set the TUF client. Is there a reason to pass this in as a variadic options.VerifierOptioner
rather than something like a *options.VerifierOpts
like what we have on lines 332 and 333 for provenanceOpts
/builderOpts
?
options/options.go
Outdated
SigstoreTUFClient apiUtils.SigstoreTUFClient | ||
} | ||
|
||
// VerifierOptioner is a function that sets options for the verifier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason to use this approach for creating options as opposed to something like:
verifierOpts := &options.VerifierOpts{
SigstoreTUFClient: client,
... // Anything else that needs to be set up
}
in the client (and then potentially a DefaultVerifierOptions
func if needed - see comment below)? I think that would be more consistent with builderOpts
/provenanceOpts
and clearer for clients to understand?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it would be more clear to understand, but my reason is so that we won't me merging in a breaking change at this time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using variadic seems OK if we need to not make a breaking change to the api. In terms of using an Optioner
vs something like options.VerifierOpts
, using the latter might be better to keep consistent with the way we provide the other options (possibly including a DefaultVerifierOptions
function to make it easy for clients to get the standard set of options). Or are there pros for using an Optioner
that I'm missing? (I'm not very familiar with that pattern sorry)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also new to the pattern, but I see it's used a bit in sigstore-go for making optional arguments. (I prefer to call the returned func an "optioner", instead of an "option".)
- https://github.com/sigstore/sigstore-go/blob/87bfa5b1c9fcf161c773559f3b470a36ddea4552/pkg/verify/signed_entity.go#L123
- https://github.com/sigstore/sigstore-go/blob/87bfa5b1c9fcf161c773559f3b470a36ddea4552/cmd/conformance/main.go#L306
I could change it to accept a variadic options.VerifierOpts
, but then I'd need code to check that the receiver has no more than one of options.VerifierOpts
. This seems problematic. And If you recall, that was my original approach, but now it seems we could agree the variadic option funcs seem to be a good compromise to making a whole new VerifyNpmPackageWithSigstoreTUFClient
.
func (v *GCBVerifier) VerifyNpmPackage(ctx context.Context,
attestations []byte, tarballHash string,
provenanceOpts *options.ProvenanceOpts,
builderOpts *options.BuilderOpts,
verifierOpts ...options.VerifierOpts,
) ([]byte, *utils.TrustedBuilderID, error) {
if len(verifierOpts) > 1 {
panic("There should be at most one instance of verifierOpts supplied.")
}
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm yeah I agree that checking there's at most one options.VerifierOpts
isn't the nicest.
Where I'm getting stuck is that ideally the API function and its arguments would provide a "well-lit path" to users i.e. the function signature would make it easy for clients to:
a) understand what the function is doing
b) call the function correctly/with their desired arguments.
I think right now it's more complex than it needs to be. There's three different sets of options you have to pass in and the options themselves are specified two different ways. I know that's a by-product of hacking on the existing API (which I don't believe was necessarily designed to be part of an external library).
I don't want to block this change from merging - I know it's been in the works for a while now and I definitely think this restructuring to allow a user to pass in a TUF client is a big improvement. Maybe for the future it be good to either:
- Introduce an additional function with a simpler signature (I'm curious to hear Laurent explain why that might be a bad idea)
- Make a breaking change to this API
We should also make sure the example documentation is super clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep I agree with @loosebazooka that if we need to be strict about API breakages, the best option would be to accept a variadic VerifierOpts
and check for 0 or 1 at runtime.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@slugclub Thanks! I've added it and renamed it to ClientOpts
, because now we have a separate VerificationOpts
. Although, I could fold it into VerificationOpts
, instead. That might make more sense. wdyt @loosebazooka ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah sounds good. Thanks everyone for the fruitful discussion here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just making sure: you'd prefer it folded into ClientOpts
folded into VerificationOpts
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel super strongly about this, but I think these configurations as separate structs seems reasonable, as we can grow ClientOpts
to include configuration for other service dependencies of slsa-verifier while VerificationOpts
is focused on cryptographic verification options.
|
||
### Npmjs | ||
|
||
With `VerifyNpmPackageWithSigstoreTUFClient`, you can pass in your own TUF client with custom options. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another question following from Laurent's - how much leeway do we have to change this API? Does it have many users currently? Would it be possible to make a new release with a breaking change? (Not saying we necessarily have to do that just wondering what the constraints are here).
Signed-off-by: Ramon Petgrave <[email protected]>
Signed-off-by: Ramon Petgrave <[email protected]>
Signed-off-by: Ramon Petgrave <[email protected]>
Signed-off-by: Ramon Petgrave <[email protected]>
Signed-off-by: Ramon Petgrave <[email protected]>
On the sigstore clients we actually just accept a trusted_root.json as an input to the verifier -- this would require depending on sigstore-go instead of cosign and I'm not sure where we're at here. However this eliminates the need for consumers to define a whole TUF client, they may compute |
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 <[email protected]>
I could see a need for both - a user may gather their own root material out of band, at which point they should pass that material via a trusted root file, to align with the other Sigstore clients (and sigstore-go has an API for this). A developer integrating with the API may be opinionated on TUF settings and construct their own client (or pass options). |
I just don't think slsa-verifier should be involved in the lower-level details of the TUF client. |
options/options.go
Outdated
SigstoreTUFClient apiUtils.SigstoreTUFClient | ||
} | ||
|
||
// VerifierOptioner is a function that sets options for the verifier. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could change it to accept a variadic options.VerifierOpts, but then I'd need code to check that the receiver has no more than one of options.VerifierOpts
Can you explain this part to me a little. The optioner pattern seems a bit strange. But I can't seem to follow the rationale.
Could you say by what you mean on lower-level details? Do you mean the configuration of the TUF client? In terms on an API, I think we could see a few different options:
|
Signed-off-by: Ramon Petgrave <[email protected]>
Signed-off-by: Ramon Petgrave <[email protected]>
I mean the trusted_root.json file, and managing potential refreshes in a long-running process.
But a user still has to know how to prepare a trusted_root.json properly. Ideally, the Sigstore TUF client can handle those cache-control options and make it easy for the user. I would rather the user instead supply the trusted_root.json to their own TUF client, and then supply that to slsa-verifier. For the CLI, if we wanted the same kind of customization, then supplying the trusted_root.json directly makes more sense, but we may still have to worry about refreshes. We can consider that feature later if there is community ask for it. |
I think there may be some conflation of "roots". https://pkg.go.dev/github.com/sigstore/[email protected]/pkg/tuf#Options.WithRoot refers to the TUF root, as in the root of trust for the TUF metadata. The "trusted root" is the Sigstore spec-compliant file that contains all of the roots of trust for the services within Sigstore's infra. Regarding "we may still have to worry about refreshes", in the case that someone would supply a trusted root file directly, that effectively says "bypass TUF, don't worry about keeping metadata up to date". For a user, they could either supply a TUF client which goes and fetches a "trusted root", or they supply the "trusted root" directly bypassing TUF (if they don't maintain their own TUF repo, for example, or want offline verification). As for TUF caching options, I think that's mostly up to the library, and I don't think we must expose that right of the bat. Some users might be opinionated, so for example in Cosign, we allow disabling the cache, but we don't expose every TUF client option. |
Alright. For now, it's simpler and more flexible to the user for slsa-verifier to accept a sigstore TUF client. |
Signed-off-by: Ramon Petgrave <[email protected]>
This PR
./docs/Api-Library.md
Offline rekor verification already works so long as the provenance is a valid sigstore bundle, though we could consider adding an explicit option to enforce offline rekor verifification.
Fixes #493
Testing
./docs/Api-Library.md
Followups
slsa-verifier/verifiers/internal/gha/verifier.go
Line 89 in e7a8f74