Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 40 commits
e4c034a
1f04dce
7d7448b
6c69c5c
d5d6f6e
88ba4da
ef7384f
4da9d12
e208b4a
3fd185e
f414666
0356825
9d0f2b2
bec1ad9
96ea870
91173c1
aad1c11
0629763
f41e1fd
3803a16
efa3fa5
3ccbce7
7a8a94d
e53b528
ada0207
a2cb9b9
e2dcffc
a0c70c8
57b3797
c9318ae
f1ee89d
5b1c921
cde7688
667215a
1bd8955
3a09aac
5845e1c
5356a7c
65dce19
8e038e4
7268490
6cf042a
4bdc88a
b4381c5
772f659
4ca058b
8ba939a
a1d45cc
74bf48e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 newNpmVerifier
struct. And we could still use that TUFClient to let the user pass in their own TUF root.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.
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 forprovenanceOpts
/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
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:
in the client (and then potentially a
DefaultVerifierOptions
func if needed - see comment below)? I think that would be more consistent withbuilderOpts
/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 likeoptions.VerifierOpts
, using the latter might be better to keep consistent with the way we provide the other options (possibly including aDefaultVerifierOptions
function to make it easy for clients to get the standard set of options). Or are there pros for using anOptioner
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".)
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 ofoptions.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 newVerifyNpmPackageWithSigstoreTUFClient
.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:
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 separateVerificationOpts
. Although, I could fold it intoVerificationOpts
, 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 intoVerificationOpts
?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 whileVerificationOpts
is focused on cryptographic verification 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.
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 whatStatementsFromBytes
is for, but its not yet implemented for npm.slsa-verifier/verifiers/internal/gha/npm.go
Lines 224 to 229 in 87b5bae
@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.
@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