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

cmd: dockerfile resolve #1120

Closed
wants to merge 10 commits into from

Conversation

Dentrax
Copy link
Member

@Dentrax Dentrax commented Dec 1, 2021

Fixes #648
Fixes #707

Signed-off-by: Furkan furkan.turkal@trendyol.com
Co-authored-by: Batuhan batuhan.apaydin@trendyol.com

cc @developer-guy

@developer-guy developer-guy force-pushed the feature/dockerfile-resolve branch 6 times, most recently from 5c7a1ee to 32367c2 Compare December 2, 2021 10:52
Copy link
Member

@dlorenc dlorenc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit.

cmd/cosign/cli/dockerfile/resolve.go Show resolved Hide resolved
cmd/cosign/cli/dockerfile/resolve.go Outdated Show resolved Hide resolved
@Dentrax Dentrax force-pushed the feature/dockerfile-resolve branch 4 times, most recently from 888beff to bbca50f Compare December 10, 2021 22:04
@developer-guy
Copy link
Member

Kindly ping folks. It seems everything is ok right now, which means it is ready to merge. 🙋🏻‍♂️

@dlorenc
Copy link
Member

dlorenc commented Dec 27, 2021

Will take a look!

@Dentrax Dentrax force-pushed the feature/dockerfile-resolve branch 3 times, most recently from d23b000 to 2e73eb8 Compare February 15, 2022 07:08
@Dentrax
Copy link
Member Author

Dentrax commented Feb 15, 2022

Sorry, we dropped the ball here. I think we resolved all reviews.

@Dentrax
Copy link
Member Author

Dentrax commented Mar 11, 2022

Kind ping here for review 🤗

Signed-off-by: Furkan <furkan.turkal@trendyol.com>
Co-authored-by: Batuhan <batuhan.apaydin@trendyol.com>
Signed-off-by: Furkan <furkan.turkal@trendyol.com>
@ChaosInTheCRD
Copy link
Contributor

ChaosInTheCRD commented Oct 6, 2022

This is great work - but I am wondering whether it makes sense to have functionality like this directly within cosign. Sure it's helpful, but the art of ensuring that pinning images to digest may be better sitting in a different tool (e.g. https://github.com/sethvargo/ratchet), and kind of spans way further than just Dockerfiles.

@dlorenc @Dentrax @mattmoor @imjasonh what are your thoughts on this? Also adding @znewman01 as he has been considering the digest pinning scenario in #2313

@znewman01
Copy link
Contributor

znewman01 commented Oct 7, 2022

I would normally agree—this sounds a little "kitchen sink-y" to me. But I think if we have cosign dockerfile verify in the first place we need to flesh out the rest of the story here.

IMO reason this wouldn't go in another tool like Ratchet would be the separation in the tooling between "resolve" and "verify." It'd be way too easy to "resolve" with Ratchet and wind up with image digests that are untrusted.

TBH my ideal flow would be one-step, checking signatures as part of resolving: cosign dockerfile resolve --key mykey.pub Dockerfile.unpinned > Dockerfile. This is nice because there's no intermediate resolved-but-unverified Dockerfile to accidentally build with.

@github-actions
Copy link

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.

@github-actions
Copy link

This PR was closed because it has been stalled for 10 days with no activity.

@github-actions github-actions bot closed this Nov 23, 2022
@Dentrax Dentrax reopened this Nov 23, 2022
@github-actions
Copy link

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.

Dentrax and others added 7 commits January 3, 2023 17:56
Fixes sigstore#648
Fixes sigstore#707

Signed-off-by: Furkan <furkan.turkal@trendyol.com>
Co-authored-by: Batuhan <batuhan.apaydin@trendyol.com>
Signed-off-by: Batuhan Apaydın <batuhan.apaydin@trendyol.com>
Signed-off-by: Furkan <furkan.turkal@trendyol.com>
Co-authored-by: Batuhan <batuhan.apaydin@trendyol.com>
Signed-off-by: Furkan <furkan.turkal@trendyol.com>
Signed-off-by: Furkan <furkan.turkal@trendyol.com>
Signed-off-by: Furkan <furkan.turkal@trendyol.com>
@Dentrax
Copy link
Member Author

Dentrax commented Jan 3, 2023

Add a support to resolve --from=image:tag cases. Also rebased.

@github-actions
Copy link

github-actions bot commented Feb 3, 2023

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.

@znewman01
Copy link
Contributor

Thanks! Looking again at #648 I see that it proposes that the resolve command map to FROM <verified digest>.

I think we probably want to make this do all the verification of verify, just with different output.

@github-actions
Copy link

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.

@github-actions
Copy link

This PR was closed because it has been stalled for 10 days with no activity.

@github-actions github-actions bot closed this Mar 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a command to resolve image tags to digests in Dockerfiles cosign verify-dockerfile is dangerous
9 participants