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

feat: support signed commits for resource 'github_repository_file' #2102

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

wparr-circle
Copy link
Contributor

@wparr-circle wparr-circle commented Jan 15, 2024

Resolves #879


Before the change?

  • Currently github_repository_file modifies files via the github content API. Which means there is limited support for signed commits (ie. anything which supports automatic signing via API). However there is no support for signing using a custom PGP key this way.

After the change?

  • Adds support for sensitive variables 'pgp_signing_key' and 'pgp_signing_key_passphrase' which contains an armored PGP private key and an optional passphrase (if the key is locked). This can be used to sign commits when paired with 'use_contents_api = false', where we manipulate a commit and push it to the reference rather than using the contents API to provide a higher level interface.
image (note unverified due to github not having public key of the pgp key used in test and author/committer being mismatched).

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes
  • No

@wparr-circle wparr-circle force-pushed the gpg-sign branch 3 times, most recently from e6ede50 to 65dbdd2 Compare January 16, 2024 11:03
@wparr-circle wparr-circle marked this pull request as ready for review January 16, 2024 11:03
@nickfloyd
Copy link
Contributor

Hey @wparr-circle Thanks for the contributions here. Please run lint when you get the chance! It looks like CI is getting hung up on that. Thanks.

@nickfloyd nickfloyd added the Type: Feature New feature or request label Jan 18, 2024
@wparr-circle
Copy link
Contributor Author

Ran against linters now @nickfloyd! Thanks :)

@kfcampbell
Copy link
Member

@wparr-circle do you mind explaining more about the below part of your writeup? I'm not sure I understand, sorry.

where we manipulate a commit and push it to the reference rather than using the contents API to provide a higher level interface.

@wparr-circle
Copy link
Contributor Author

@kfcampbell Sure no problem! Sorry if I wasn't clear.
Current implementation of this resource is utilising the GitHub Contents API.
We get some verified signature support using this like auto sign for bots/github actions.
However, for the use case of GPG based signing - we can't leverage the contents API. Rather we need to manipulate the git tree directly.

Does that help explain?

I left the old contents API way of working as the default behaviour, because of the size of change creeping up.

@marek-karwacki-rdx
Copy link

Hi, is there a timeline on this feature? Thanks

@M0NsTeRRR
Copy link

Hello, is something missing @kfcampbell to get this merged ?

@ahanafy
Copy link

ahanafy commented Jul 24, 2024

Landed at this PR after realizing the resource doesn't support signing. @kfcampbell do you have any direction or feedback on this PR to get it completed? Trying to get an idea on whether this feature is planned for this resource or if its not achievable?

Copy link
Contributor

@nickfloyd nickfloyd left a comment

Choose a reason for hiding this comment

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

Hey thanks for the contributions here! ❤️

@kfcampbell
Copy link
Member

@wparr-circle we just merged #2100 and we're ready to go with this before we cut a release with your new features in it. I didn't anticipate that there might be a merge conflict between your two PRs though, and I'm wondering if you'd feel comfortable resolving the conflict. If you'd prefer that Nick and I do it, please let me know and we'll get to it!

Thanks for the contributions, and I'm looking forward to seeing commit signing in the wild.

@wparr-circle
Copy link
Contributor Author

Hey @kfcampbell @nickfloyd thanks for getting around to looking at these 👀
Let me quickly fix the conflicts

@ahanafy
Copy link

ahanafy commented Sep 20, 2024

@wparr-circle / @nickfloyd / @kfcampbell I think this PR will need to have conflicts resolved and review occur pretty tightly together in order to get it past the goal. Thoughts?

@rwblokzijl
Copy link
Contributor

@nickfloyd / @kfcampbell Thank you for all the work on this provider! At the risk of sounding too pushy, we would love to see this PR merged. Would it be possible to review again?

@wparr-circle
Copy link
Contributor Author

@nickfloyd sorry I haven't had time to follow up on this until now. I've just resolved merge conflicts - please review when you have a chance 🙏

@rwblokzijl
Copy link
Contributor

rwblokzijl commented Jan 24, 2025

Maybe it would be better if this resource supported using environment variables to set pgp_signing_key and/or pgp_signing_key_passphrase. That way they don't have to be stored in the state. Perhaps the specific env var to use could be passed to the resource. Eg:

resource "github_repository_file" "file" {
  ...
  pgp_signing_key_env = "SECRET_PGP_SIGNING_KEY"
  ...
}

@rwblokzijl
Copy link
Contributor

Actually, terraform just released Write-only attributes and i think this is the perfect usecase. By marking the key as write-only it won't be persisted in the state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

github_repository_file - commit signing support
7 participants