Skip to content
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

remove URL fetch of keys/artifacts server-side #735

Merged
merged 1 commit into from
Apr 14, 2022

Conversation

bobcallaway
Copy link
Member

We originally added the ability for Rekor API callers to specify URLs to fetch public keys, artifacts, & signatures to simplify fetching larger objects rather than forcing clients to download them locally and upload them to Rekor for verification.

Now that we have the hashedRekord type, this patch removes the ability for the Rekor server to fetch arbitrary URLs before making entries into the log, largely due to the risk of SSRF.

Note that the option for rekor-cli to specify URLs on the command line remains, as the CLI tool will download the artifacts locally and then insert the relevant contents in the appropriate position within the payload submitted to the Rekor server.

Signed-off-by: Bob Callaway bob.callaway@gmail.com

Signed-off-by: Bob Callaway <bob.callaway@gmail.com>
@bobcallaway bobcallaway requested a review from a team as a code owner March 15, 2022 15:01
@dlorenc
Copy link
Member

dlorenc commented Mar 15, 2022

When and how do you want to roll this out?

@bobcallaway
Copy link
Member Author

It depends on how many releases we have before we go 1.0; given that it does deprecate function, it would be good to announce it in the next release (perhaps just the sharding stuff) and then we have a follow-on release that addresses the API (i.e. move to gRPC) and this fix.

Thoughts?

@dlorenc
Copy link
Member

dlorenc commented Mar 18, 2022

SGTM, so hold this until we cut the next release?

@bobcallaway
Copy link
Member Author

yup, sounds like a plan.

@cpanato we should include a note in the next rekor release noting that the "server side remote fetching of resources will be removed in the next release" (feel free to re-word)

@bobcallaway
Copy link
Member Author

https://github.com/sigstore/rekor/blob/12d1a47c7ac986932a2734cb855c642ac01ffde4/pkg/api/index.go also has another instance of this pattern we should remove.

@haydentherapper
Copy link
Contributor

Also there's options to read sig and public key for PGP using http.Client instead of util.FileOrURLReadCloser

// FetchSignature implements pki.Signature interface

// FetchPublicKey implements pki.PublicKey interface

@haydentherapper
Copy link
Contributor

Did we want to include something in 0.6 around the deprecation of this?
@cpanato fyi

@bobcallaway
Copy link
Member Author

bobcallaway commented Apr 13, 2022 via email

@haydentherapper
Copy link
Contributor

Ah missed it, all good!

@dlorenc
Copy link
Member

dlorenc commented Apr 13, 2022

Should be safe to merge after that goes out!

@dlorenc dlorenc merged commit 82a26a2 into sigstore:main Apr 14, 2022
@github-actions github-actions bot added this to the v1.0.0 milestone Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants