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

Adds support for GitHub App authentication #753

Merged
merged 3 commits into from
May 22, 2021
Merged

Adds support for GitHub App authentication #753

merged 3 commits into from
May 22, 2021

Conversation

alloveras
Copy link
Contributor

Intent

To support authentication through GitHub apps and close #514.

Problem

The current provider implementation does not support GitHub Apps as an authentication mechanism. Furthermore, using the currently supported authentication mechanism (personal OAuth tokens) should be avoided in certain circumstances.

For example, if a user is an administrator of multiple GitHub organizations, their tokens will have administrative privileges over each organization and, unfortunately, there is no way to scope them down. In consequence, the blast radius of Terraform operations run with these tokens may trespass organization boundaries and that's far from ideal from a risk/security point of view.

Finally, tokens generated through a GitHub App have higher rate-limit thresholds than the ones applied to personal OAuth tokens.

Solution

To extend the current provider implementation to support a new configuration sub-block (app_auth). This new sub-block is optional and, conflicts deliberately with the already existing token attribute.

provider "github" {
   app_auth {
      id = "123456789"                                      # Or specified through the `GITHUB_APP_ID` environment variable.
      installation_id = "987654321"                  # Or specified through the `GITHUB_APP_INSTALLATION_ID` environment variable.
      pem_file = "/path/to/pem/file.pem"         # Or specified through the `GITHUB_APP_PEM_FILE` environment variable.
   }
}

Implementation Notes

  • The proposed implementation should be backwards compatible.

  • The app_auth block implementation approach has been favored over separated configuration attributes (e.g: app_auth_id, app_auth_installation_id and app_auth_pem_file) because it provides:

    • Namespaced and shorter attribute names.
    • Simplicity when it comes to managing conflicts between properties (e.g: token vs app_auth).
  • Turns out that Terraform does not evaluate the default value functions of attributes within blocks that haven't been explicitly defined in the configuration (see Default TypeList / TypeSet hashicorp/terraform-plugin-sdk#142). This means that, in order to configure the provider to use a GitHub App through environment variables, the provider block must look like:

provider "github" {
   ...
   app_auth {
      // Empty block to allow the provider configurations to be specified through 
      // environment variables.
      // See: https:/hashicorp/terraform-plugin-sdk/issues/142
   }
}

github/apps.go Outdated Show resolved Hide resolved
@patrickmarabeas
Copy link
Contributor

Can we specify the PEM as a string instead / additionally? It would be preferable to save this as a secret.

@eerkunt
Copy link
Contributor

eerkunt commented Apr 12, 2021

Great implementation! 🎉 Thanks

Not sure if this will solve large-scale implementations. Since, this will only increase available Github API limit from 5000 to 15000. So, yes, it will solve current day problems, but then we you scale out, you will hit another wall.

Maybe, we might have multiple App configuration and have a parameter like on the provider ;

min_required_limit: int

where we set the min_required_limit based on our needs and tell to the provider if you don't have (e.g.) 1000 available API limit, then iterate on the next Github App, till we have 1000 available API limit

Then, IMHO, this can be scalable and when a large-scale implementation occurs, that implementation will just need to add more placeholder Github Apps and add those app's details to the configuration.

.. or .. for authenticating via normal Personal Access Token, it will just stop doing anything before we are hitting on the API limit. Its harder to understand what has been created, what is in the state when half of your terraform resources created then you hit the API limit.

On the other hand, IMHO, this adds a complexity for sure, but I don't think any of the authentication/authorisation solutions we had so far is scalable.

Github itself need to provide a solution to this if they want customer with large-scale codebases/automations being a part of it.

@eerkunt
Copy link
Contributor

eerkunt commented Apr 12, 2021

#755

@jcudit jcudit modified the milestones: v4.9.0, v4.10.0 Apr 17, 2021
Copy link
Contributor

@jcudit jcudit left a comment

Choose a reason for hiding this comment

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

Played around with this locally and think its on the right track 👍🏾. Suggested next steps are:

  • Updates to website/docs/index.html.markdown as we now have a new option for configuring the provider
  • Replacing golang.org/x/oauth2/jws with something else

Feel free to cherry-pick 8621445 to get an example in with this PR as well. Thanks for taking the time and effort to get things this far! Excited to get this across the finish line 🚀

github/apps.go Outdated Show resolved Hide resolved
@alloveras
Copy link
Contributor Author

Thanks for the feedback/suggestions @eerkunt and @jcudit 🙇

I've been a bit busy lately and I couldn't dedicate time to this PR. I hope to have some spare time this weekend to do a second pass and address/reply to all the comments 😀.

@alloveras alloveras requested a review from jcudit April 27, 2021 00:32
@jspiro
Copy link
Contributor

jspiro commented May 5, 2021

@jcudit Could you take a look at @alloveras' updates? This PR seems like it would be hugely helpful.

@alloveras if we wanted to try this, could you provide instructions how to test a branch of this provider?

Copy link
Contributor

@jcudit jcudit left a comment

Choose a reason for hiding this comment

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

This is great! Excited to release this to the community for further testing. I can verify that the credential generation parts are working, however my test org and GitHub App may not have been set up correctly as I was hitting some permissions issues. Early adopters may have to report back on what worked for them so we can document this setup more.

Please cherry-pick 25d9a58 to get some documentation updates in with this PR. Once that's in, we should be able to release this early next week!

@eerkunt
Copy link
Contributor

eerkunt commented May 17, 2021

Hi, could this will be merged soon ?

Copy link
Contributor

@jcudit jcudit left a comment

Choose a reason for hiding this comment

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

Appreciate the patience here. This is a valuable PR to have out there so lets proceed as-is. I'll tack on the docs separately.

Huge thanks to @alloveras for the contribution! ❤️ 🚀 🎉 🥇

@jcudit jcudit merged commit 65a8ed7 into integrations:master May 22, 2021
jcudit pushed a commit that referenced this pull request May 22, 2021
* Adds support for GitHub App authentication

* Cherry-pick 8621445

* Replace deprecated golang.org/x/oauth2/jws
@alloveras alloveras deleted the alloveras-github-app-auth branch May 28, 2021 00:40
jcudit pushed a commit that referenced this pull request Jun 16, 2021
* Adds support for GitHub App authentication

* Cherry-pick 8621445

* Replace deprecated golang.org/x/oauth2/jws
@petracvv
Copy link

I came across this PR but I'm having a difficult time getting the Github App settings and permissions right to get the same results as with a PAT. Does anyone have an example I could follow? Do you need to install the app to the org you are managing rather than an account that is part of that org?

@jcudit
Copy link
Contributor

jcudit commented Sep 7, 2021

@petracvv can we get an issue created with some more detail? Hoping to track and get a fix implemented!

kfcampbell pushed a commit to kfcampbell/terraform-provider-github that referenced this pull request Jul 26, 2022
* Adds support for GitHub App authentication

* Cherry-pick 8621445

* Replace deprecated golang.org/x/oauth2/jws
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.

Feature Request > GitHub App authentication support
6 participants