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

Added support for ssh signed commits and completed gpg signed commit work #710

Merged
merged 3 commits into from
Jun 18, 2024

Conversation

dlactin
Copy link
Contributor

@dlactin dlactin commented May 11, 2024

Building upon the work done in PR 428 to add support for SSH signed commits.

Updated Documentation to include instructions and examples for adding an SSH signing key along with links to repository provider documentation for setting up commit verification.

Updated deployment to include volumes for SSH key secrets and the respective volume mounts.

@jannfis
Copy link
Contributor

jannfis commented May 16, 2024

Hey @dlactin, thanks for this PR.

I was wondering what the value-add for this would be, because Argo CD would not be able to validate those commits signed by SSH just yet?

@dlactin
Copy link
Contributor Author

dlactin commented May 16, 2024

Hey @jannfis, in our case we require signed commits on our infrastructure repositories. So we are unable to have image updater commit to any of our protected branches without this change.

Adding commit signing capability to image updater will allow verified commits to the target repo, commit validation with ArgoCD would be a bonus when that feature is available.

Screenshot 2024-05-16 at 9 34 02 AM

@jannfis
Copy link
Contributor

jannfis commented May 16, 2024

Thanks for the clarification, @dlactin. The feature makes sense to me, then. Also, I guess with Source Verification Policies hopefully coming to Argo CD, we could as well integrate verification of SSH signatures and Image Updater would already know how to do sign using SSH then.

+1 from me for this feature. Please give us some time to review.

Copy link
Contributor

@jannfis jannfis left a comment

Choose a reason for hiding this comment

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

I got a couple of comments, PTAL.

docs/basics/update-methods.md Outdated Show resolved Hide resolved
docs/basics/update-methods.md Show resolved Hide resolved
docs/basics/update-methods.md Outdated Show resolved Hide resolved
docs/basics/update-methods.md Outdated Show resolved Hide resolved
ext/git/writer.go Outdated Show resolved Hide resolved
ext/git/writer.go Outdated Show resolved Hide resolved
@dlactin dlactin force-pushed the ssh-signed-commits branch 2 times, most recently from acc51ad to 04cf492 Compare June 3, 2024 21:16
@dlactin dlactin requested a review from jannfis June 4, 2024 21:05
@jannfis
Copy link
Contributor

jannfis commented Jun 12, 2024

Just as a heads up, this is good to go. Waiting for #737 to be merged, then we'd need to rebase this one on top of the Git client changes. It shouldn't be too much work though.

@jannfis
Copy link
Contributor

jannfis commented Jun 14, 2024

@dlactin Thank you for your patience! #737 has been merged, and as expected left a couple of conflicts for this PR. I can support you to resolve them, or let you do it on your own. Please let me know what you prefer.

@dlactin
Copy link
Contributor Author

dlactin commented Jun 14, 2024

@dlactin Thank you for your patience! #737 has been merged, and as expected left a couple of conflicts for this PR. I can support you to resolve them, or let you do it on your own. Please let me know what you prefer.

Hey, not a problem!

I won't have time to finish this up until sometime next week so feel free to resolve them if you'd like! Otherwise I'm happy to pick this back up later.

manifests/install.yaml Outdated Show resolved Hide resolved
@dlactin dlactin requested a review from chengfang June 18, 2024 17:53
Copy link
Contributor

@jannfis jannfis left a comment

Choose a reason for hiding this comment

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

Awesome, thank you so much @dlactin, and also thank you @chengfang for the additional review!

LGTM.

@jannfis jannfis merged commit ae73f74 into argoproj-labs:master Jun 18, 2024
9 checks passed
jannfis pushed a commit that referenced this pull request Jun 19, 2024
sribiere-jellysmack pushed a commit to sribiere-jellysmack/argocd-image-updater that referenced this pull request Aug 13, 2024
sribiere-jellysmack pushed a commit to sribiere-jellysmack/argocd-image-updater that referenced this pull request Aug 13, 2024
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