-
Notifications
You must be signed in to change notification settings - Fork 522
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: implement sget blob downloads #1190
Conversation
@mattmoor I modified the sget tool a little bit. With this patch it would obtain support for downloading every artifact. IMO, sget should be able to handle both images and blobs. |
475c171
to
bc27fb8
Compare
I have added an e2e-test for the new sget feature... do we want one for |
fb0805e
to
845e604
Compare
Currently, golangci does not print filenames and line numbers due to some issue with the golangci log format. See also: golangci/golangci-lint-action#119 (comment) Signed-off-by: Christian Rebischke <chris@shibumi.dev>
This commit adds a "blob" subcommand to the sget tool. The "blob" subcommand provides an easy and convenient way to download and verify binary large objects (BLOBs). Breaking Changes: The sget image download command can be now found as subcommand: sget image Signed-off-by: Christian Rebischke <chris@shibumi.dev>
f9c5dd2
to
d71b6d3
Compare
6ce0edd fixes an issue with golangci. It does not print the filename + line number on failure.. this is a known bug and tracked here: golangci/golangci-lint-action#119 (comment) |
This commit adds a "blob" subcommand to the sget tool. The "blob" subcommand provides an easy and convenient way to download and verify binary large objects (BLOBs). Breaking Changes: The sget image download command can be now found as subcommand: sget image Signed-off-by: Christian Rebischke <chris@shibumi.dev>
I think that's ready for review now :) |
} | ||
|
||
// TODO(shibumi): we might want to export this function or add this to our package?! | ||
func extractCerts(e *models.LogEntryAnon) ([]*x509.Certificate, 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.
Does it make sense to export this function and add it to another package and then re-import the function here?
Right now we are having the code twice (once in the sget code and once in the cosign code).
} | ||
|
||
// TODO(shibumi): we might want to export this function or add this to our package?! | ||
func isb64(data []byte) bool { |
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.
Does it make sense to export this function and add it to another package and then re-import the function here?
Right now we are having the code twice (once in the sget code and once in the cosign code).
@@ -143,3 +143,4 @@ jobs: | |||
with: | |||
# Required: the version of golangci-lint is required and must be specified without patch version: we always use the latest patch version. | |||
version: latest | |||
args: "--out-${NO_FUTURE}format colored-line-number" |
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 Line fixes the golangci linter... without that line golangci will show errors, but will not show "where" (filename + line numbers) in the github actions workflow, due to golangci's weird log format (Why has Github Workflows a problem with logs starting with "::" at all?! Please god.. don't let this be some other weird issue, we just had log4j a few days ago XD
- uses: actions/setup-go@v2 | ||
with: | ||
go-version: '1.17.x' | ||
- name: build cosign and check |
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.
It might make sense to expand this test with more flag combinations, but I am not sure if this is really necessary.
We should also test the image subcommand.
I have added a few comments. Let me know what you think :) Also, I would like to underline that the current implementation only supports auto-lookups via rekor log. IMO, this should be the default behavior, because it's the best way to ensure that the lookup happens via the tlog. Nevertheless, do we want to add functionality for supporting passing a certificate via CLI? Is this necessary? Opinions? |
I need to think through this one a bit. The This change does a few things:
For OCI storage, we currently allow getting an object by digest or by tag. If you fetch by tags, the object must be signed. If we extend to non-OCI storage, we should try to keep things as safe as possible. I think to make a download "safe" from an arbitary URL we could require that the user specify a key to verify signatures against, OR a hash. The hash MUST be local in this case, not at a URL. I like the idea of allowing other ways to fetch things, and encouraging safe behavior, so this overall makes sense. What do you think of trying to merge the commands back though. To a user, the thing getting downloaded is actually the same, it just differs in how they're retrieved and validated. |
This is exactly what I thought about at first. I struggled with deciding how we branch into the different code segments. How do we know it's a file and not an OCI registry? I thought about looking for EDIT: Ok... OCI registries would start with this prefix as well.. we have to go for another way. Maybe trying to download it as OCI image first, if it fails we fallback to a normal GET request and blob verification?
I see a problem here, but maybe you have an idea. My assumption was that the URL is safe (HSTS, DNSSEC, etc). If it does not, we are having a little trust problem, because from where would the user get the hash? Very likely from the same website the user gets the blob from, right? Does it really make a difference if we provide What do you think? Can you explain this point to me? :) |
I think we have a following options for merging the
Every one of those has its own advantages and disadvantages. |
Would this work for discovery? https://docs.docker.com/registry/spec/api/#api-version-check You could parse the URL like a docker image one, and then make sure it's served from a registry. |
Good idea! I will work something out and update this PR. |
This does not seem to work:
shouldn't I get some JSON response? With docker hub this does not work as well. Only
@dlorenc The alternative is that we just check for What about using |
@jonjohnsonjr any idea the best way to tell if a URL points to a container image? |
I don't love this one either because anything is kind of a blob... Images also store blobs... What about something like |
this sounds reasonable for me. We could default to |
I actually wouldn't mind if sget turned into a safe, universal artifact fetcher over time. That's kind of the idea actually, now that I think about it :) Are there any other projects that fetch artifacts over the full purl spectrum? |
I don't think so. There is a go package for validating/creating package URLs. We do not need to support all package definitions from day 0. The list is very long: https://github.com/package-url/purl-spec/blob/master/PURL-TYPES.rst Does the OCI definition meets your expectations? https://github.com/package-url/purl-spec/blob/master/PURL-TYPES.rst#oci IMO, if we implement purl, we should be consistent. That measn that
will become:
TWill package URL be a standard in the future that we can rely on? I am fine with supporting purl, I am just worried that we might implement something that will never "take off". On the other hand: If nobody starts implementing it nobody will implement it, I guess. :D |
@dlorenc FYI: I forked the purl go implementation (it hasn't received an update since 2 years). Might be useful for SPDX as well. Looks like SPDX implements package URL. |
Idk if it'll take off, but it is seeing usage in the SBOM formats and in the SLSA spec. I'm not aware of anything equivalent either. |
If it's in the SBOM formats and the SLSA spec it should be fine, I guess. I got good feedback for my package-url-go PR: package-url/packageurl-go#28 if it isn't merged until new year I might hack something together with my fork, just for knowing it would work the way we want it to. |
I'm fine using your fork if we need to because the other one is stale. The only real risk of a fork is drift, but if nothing is moving upstream we don't have much drift to worry about. |
HEAD and check the |
@jonjohnsonjr is the content-type special for an OCI registry? I just checked this with Anyway: We are playing around with pkg urls as explicit artifact identifier.. this seem to be more promising right now. |
If you want to check "is this host a registry?" you can do this:
And just check for If you want to check if a URL points to a specific container image you have to jump through some more hoops to transform the image ref to the API call and (sometimes) do the auth handshake:
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
This PR was closed because it has been stalled for 10 days with no activity. |
Summary
This commit adds a "blob" subcommand to the sget tool.
The "blob" subcommand provides an easy and convenient way to download
and verify binary large objects (BLOBs).
Breaking Changes: The sget image download command can be now found as subcommand: sget image
Example Output:
Ticket Link
None
Release Note