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.

Sign up for GitHub

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

Jump to bottom

Proposal - Signatures for Carvel Artifacts #668

Merged
merged 9 commits into from
Sep 29, 2023
Merged

Proposal - Signatures for Carvel Artifacts #668

merged 9 commits into from
Sep 29, 2023

Conversation

ThomasVitale
Copy link
Contributor

Proposal for introducing signatures for all Carvel artifacts as previously suggested in #619.

@netlify
Copy link

netlify bot commented Aug 8, 2023 •
edited
Loading

✅ Deploy Preview for carvel canceled.

Name Link
🔨 Latest commit ae0e108
🔍 Latest deploy log https://app.netlify.com/sites/carvel/deploys/6516febf1093dc0008cccded

@ThomasVitale
Update proposal metadata
776b4ef 
Signed-off-by: Thomas Vitale <[email protected]>
@ThomasVitale
Improve formatting
c139081 
Signed-off-by: Thomas Vitale <[email protected]>
100mik
100mik reviewed Aug 25, 2023
View reviewed changes
Copy link
Contributor

@100mik 100mik left a comment

Choose a reason for hiding this comment

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

Thank you so much for putting this together 🙏🏼
Just added a few thoughts and called out some "nice to haves"!

proposals/carvel/002-artifact-signatures/README.md Outdated Show resolved Hide resolved
proposals/carvel/002-artifact-signatures/README.md Show resolved Hide resolved
@ThomasVitale
Copy link
Contributor Author

@100mik thanks for your review. I have added examples in GitHub Actions of how to verify the signatures for both OCI and binary artifacts (including links to a couple of demos I made). I have also refined the suggestion for the binary artifacts part and included examples of how to integrate Cosign with GoReleaser, since that's the tool used by all Carvel projects.

@ThomasVitale
Fix typo
62a265f 
Signed-off-by: Thomas Vitale <[email protected]>
joaopapereira
joaopapereira reviewed Sep 13, 2023
View reviewed changes
Copy link
Member

@joaopapereira joaopapereira left a comment

Choose a reason for hiding this comment

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

The proposal looks good.
The only question I would like to see answered before a thumbs up is, what are the plans for our GitHub action? Should it also verify that the binaries are signed? If so, we need to ensure we are backward compatible since we have older versions without signatures.

@ThomasVitale
Copy link
Contributor Author

@joaopapereira thanks for the review. Do you mean this Action? https://github.com/carvel-dev/setup-action I can see we are currently verifying the checksums against the checksums.txt file bundled with the specific release. We could extend that step so that if a signature is included among the artifacts part of the release, then we validate the signature, otherwise we don't. That way we can ensure backward compatibility. What do you think?

@joaopapereira
Copy link
Member

That sounds great to me. I will review it again when you add this part. Thanks for the great work.

@ThomasVitale
Copy link
Contributor Author

@joaopapereira I have added a section to describe the changes suggested for the Carvel GitHub Action.

joaopapereira
joaopapereira approved these changes Sep 19, 2023
View reviewed changes
Copy link
Member

@joaopapereira joaopapereira left a comment

Choose a reason for hiding this comment

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

LGTM

100mik
100mik reviewed Sep 20, 2023
View reviewed changes
Copy link
Contributor

@100mik 100mik left a comment

Choose a reason for hiding this comment

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

Looks good! 🚀

praveenrewar
praveenrewar approved these changes Sep 25, 2023
View reviewed changes
Copy link
Member

@praveenrewar praveenrewar left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you so much for the great work !!

joaopapereira
joaopapereira requested changes Sep 27, 2023
View reviewed changes
Copy link
Member

@joaopapereira joaopapereira left a comment

Choose a reason for hiding this comment

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

We are at the point where we can consider this Lazyly approved. @ThomasVitale please update the status and we will be able to merge it.

proposals/carvel/002-artifact-signatures/README.md Outdated
---
title: "Signatures for Carvel Artifacts"
authors: [ "Thomas Vitale <[email protected]>" ]
status: "in review"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
status: "in review"
status: "accepted"

Copy link
Contributor Author

@ThomasVitale ThomasVitale Sep 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@joaopapereira Thanks, I updated the status

@100mik
Copy link
Contributor

100mik commented Sep 29, 2023

Green lights all the way!

@100mik 100mik merged commit 56245b8 into carvel-dev:develop Sep 29, 2023
4 checks passed
@100mik
Copy link
Contributor

100mik commented Sep 29, 2023

We should probably work towards having a flow that does not require commits to change statuses in the future 🤔

ashpect pushed a commit to ashpect/carvel that referenced this pull request Mar 3, 2024
@ThomasVitale @ashpect
Proposal - Signatures for Carvel Artifacts (carvel-dev#668)
75b4022 
* Proposal - Signatures for Carvel Artifacts

Signed-off-by: Thomas Vitale <[email protected]>

* Update proposal metadata

Signed-off-by: Thomas Vitale <[email protected]>

* Improve formatting

Signed-off-by: Thomas Vitale <[email protected]>

* Update proposal status

Signed-off-by: Thomas Vitale <[email protected]>

* Improve proposal after review

Signed-off-by: Thomas Vitale <[email protected]>

* Refined proposal for binaries + examples

Signed-off-by: Thomas Vitale <[email protected]>

* Fix typo

Signed-off-by: Thomas Vitale <[email protected]>

* Introduce changes to Carvel GH Action

Signed-off-by: Thomas Vitale <[email protected]>

* Update proposal status to 'accepted'

Signed-off-by: Thomas Vitale <[email protected]>

---------

Signed-off-by: Thomas Vitale <[email protected]>
Signed-off-by: ashpect <[email protected]>
@ThomasVitale ThomasVitale mentioned this pull request Apr 14, 2024
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joaopapereira joaopapereira joaopapereira requested changes

@100mik 100mik 100mik approved these changes

@praveenrewar praveenrewar praveenrewar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Carvel
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ThomasVitale @joaopapereira @100mik @praveenrewar