Fix `github_repository_file`: Disable default compute of author and email

[pawnu]

When creating a file without author and email information, github will add the following default author and email values.

      "commit_author": "GitHub",
      "commit_email": "[email protected]",

When the same file is edited, with Computed set to false and no author and email information provided, it will correctly exclude the author and email information during PUT request, i.e. set to null.

      - commit_author       = "GitHub" -> null
      - commit_email        = "[email protected]" -> null

This will allow github to sign your commit with your bot/user's access token. Otherwise, github will expect you to create a signed commit as above default user.

Fixes https://github.com/integrations/terraform-provider-github/issues/1152

[pawnu]

I believe this will need more work as the state will save commit_author, and commit_email as null, and when comparing against the previous commit of the file in question, it will try to change the upstream default author and email information to the local state null even when there has been no change to the file. This means there will be a change every time you run terraform plan to remove the author/email information.

Also, destroying a file with this change still resulted in unsigned commit.

[tjcorr]

This means there will be a change every time you run terraform plan to remove the author/email information.

I'm wondering if this is actually the desired behavior? Let's think of an unusual but possible scenario. Let's say I create a file without specifying the commit author/email using terraform with credentials from user A. Terraform will appropriately not pass this information to GitHub and the commit will get signed by user a by default. Now later, without making any changes to the resource, I rerun terraform but now using credentials from user b. What should happen? Technically the computed committer author/email has indeed changed and we should be updating the file with this new information. I don't think that info is known until Terraform actually applies using whatever credentials are supplied hence why it should always show a change every time you re-run. I agree this seems weird since we didn't specify the committer information in the first place. However maybe it is required if we want to handle the scenario when a committer is explicitly specified and changed from one value to another?

Part of me feels the committer fields should always just be computed and not able to be specified by a user. Then we could safely ignore the fields and only update the file on a SHA change. This would break anyone manually specifying that information. Perhaps there is some value to being able to set the author/email to values like "Terraform Bot" or team email of those responsible for the automation .

This leaves us trying to balance out which approach makes the most sense or if we need to put in some special logic when committer information isn't provided.

[pawnu]

Agreed the scenario you described makes sense when you have users a and b. What this PR was aiming to solve was issue with signing commits.

When you first create a file without committer information with terraform, the author information comes from the App/User token you used and it is signed. When you edit the file without committer information again, the author is set to GitHub user web-flow (https://github.com/web-flow) instead of your App/User, and the commit is not signed as you don't have web-flow's token to sign the commit. I guess this is where the logic should be put in place to resolve this unsigned commit issue.

Perhaps there is some value to being able to set the author/email to values like "Terraform Bot" or team email of those responsible for the automation .

That would be a good option to have as well with a way to sign the commits as the bot/team user.

[rwblokzijl]

Another option might be to add a sign_commit flag that is mutually exclusive with author/email.

Regardless, this fix would be really helpful to us, as we can't require signed commits because of this.

[kfcampbell]

I like the idea of adding a sign_commit flag. If the PR is updated to include it, I'll review and merge!

[rwblokzijl]

@pawnu will you be able to fix this? I would, but I don't know anything about go...

[pawnu]

I can try but I'm a beginner at Go too so it might take some time. Happy if someone takes it from here too as I want this to get fixed soon.

[pawnu]

I think commit_as_token makes more sense?

Also, I couldn't manage to create signed commit for deletion action even with the raw github API by omitting committer/author information so I don't think it's possible with terraform at this instance.

[rwblokzijl]

Thanks for the implementation :)

I personally think sign_commit is more clear to the user while commit_as_token exposes implementation details. As a user just wanting signed commits its unclear this flag would do that.

Regarding the deletion event not being signed: I think this is a bug at the github API. Personally i don't think it should stop us from implementing this flag. I submitted the following bug report feel free to comment on it with further details or copy paste it into a new issue if you want ;)

[pawnu]

I'd say this is still a workaround and we're not intentionally signing commits so I'm not sure. I'd expect a flag like sign_commit to take in your signature key to create sign commits as your specified commit author. Seems like this functionality does exist in go-github https://github.com/google/go-github/blob/master/github/git_commits.go#L91

[rwblokzijl]

I really think this is an intended feature we're using.

Github always "verifies" your commits when doing things through their own verified services (clicking "update branch" on a PR, editing inside the browser, etc). If a commit if verified like this github add the following popup when clicking the vefified tag: However, if the commit is made by a user and signed by a known GPG key it looks as follows: Github marks a commit as verified iff it can verify without a doubt that the github user associated with the commit is actually the one that created it. This is either by them adding a GPG key to their personal account and using that to sign the commit. Or by editing a file while authenticated to github.

In our case we are authenticated to github (by API token). Thus github should verify the commit (unless the username/email doesn't match).

[rwblokzijl]

Actually, given @pawnu and my previous comments I am now in favour of the original implementation. Just signing it by default if the author/email are omitted as this mirrors the github features. @kfcampbell would you be willing to merge given this argument or do you still thing a flag is needed?

[kfcampbell]

Actually, given @pawnu and my previous comments I am now in favour of the original implementation. Just signing it by default if the author/email are omitted as this mirrors the github features.

I believe that the current state of the PR reflects a flag being added, not the original implementation. @rwblokzijl to clarify: you're in favor of not merging this PR at all, and relying on the existing state of the code?

[rwblokzijl]

No, i am in favor of the original state of the PR: If author and email are ommited, allow github to verify the commit. This was achieved by making author and email non-computed.

[pawnu]

I believe this will need more work as the state will save commit_author, and commit_email as null, and when comparing against the previous commit of the file in question, it will try to change the upstream default author and email information to the local state null even when there has been no change to the file. This means there will be a change every time you run terraform plan to remove the author/email information.

So, I guess this resourceGithubRepositoryFileRead is used to read and set the state? It seems to resolve the above issue now.

[kfcampbell]

I'm not sure I'm seeing the correct behavior when testing this locally.

My Terraform file looks like this:

provider "github" {
}

terraform {
  required_providers {
    github = {
      source = "integrations/github"
    }
  }
}


resource "github_repository" "test" {
  name      = "repo-file-test-repo-1"
  auto_init = true
}

resource "github_repository_file" "test" {
  repository     = github_repository.test.name
  branch         = "main"
  file           = "test"
  content        = "baz"
  commit_message = "Managed by Terraform2"
  commit_author  = "Terraform User" # comment to test omission of commit_author
  commit_email   = "[email protected]" # comment to test omission of commit_email
}

On both the main branch and this feature branch, I created a repo with auto-init on, then created a file without the included commit_author and commit_email, then edited that file afterward with a slightly different file content with the commit_author and commit_email present.

In both cases, the result was that omitting the commit author and email did not cause GitHub to use itself for the committer. Instead, it used my local git credentials (same token as I provided in the GITHUB_TOKEN environment var), but picked up my commit_author and email from there. In neither case was the subsequent commit verified:

This change doesn't seem like it creates the desired functionality. What am I missing here?

[pawnu]

@kfcampbell I tested this with github app so I was assuming it would be the same with PAT. I guess there is no way to sign a commit as a user with terraform github at present?

I replicated your tests with my github app and 2 files, and they seem to be working as expected.

File1

File2

[kfcampbell]

:+1: it definitely seems to be an app-only feature. I think we could still merge this functionality if the docs and/or naming were updated to make that clear.

[pawnu]

@kfcampbell Done, I have updated the docs but feel free to edit or clarify further. Thanks