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

Authorization fixes: vNext proposal #980

Closed
kfcampbell opened this issue Nov 18, 2021 · 15 comments
Closed

Authorization fixes: vNext proposal #980

kfcampbell opened this issue Nov 18, 2021 · 15 comments
Labels
Status: Stale Used by stalebot to clean house
Milestone

Comments

@kfcampbell
Copy link
Member

Authorization Superissue, v5.0.0 proposal

Recently we've had a lot of papercuts with the provider's authentication mechanism. I've performed a bunch of testing (see results below) and I've come up with a list of behavior that I think is broken in the current release that I think we can fix in the next major version.

Terraform Version

Terraform v1.0.11
on linux_amd64

Affected Resource(s)

Multiple resources: this issue concerns our authorization policies for the terraform provider

Terraform Configuration Files

Here's an example that creates a repository, used in attempting to reproduce #876.

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

provider "github" {
  owner = "kfcampbell-terraform-provider"
  token = "ghp_personal_token_redacted"
}


resource "github_repository" "app_repo" {
  name        = "876-repro"
  description = "App repository"

  visibility = "private"
  delete_branch_on_merge = "true"
}

Behavior

See these issues for a non-comprehensive list of current authorization issues.

I have responded to a few of these threads (#876, #878, #655) with reproduction cases.

I've performed a pretty comprehensive test of authentication results

Release v5.0.0 proposed authorization changes:

  • Deprecate GITHUB_ORGANIZATION environment variable (and organization provider block configuration) in favor of GITHUB_OWNER
  • If no token is present, still process owner variables so that unauthenticated requests may be made against public org-owned repos
  • Improve error handling and messaging around missing tokens/owner configuration (currently it's pretty difficult to figure out this is a problem and act accordingly)
  • Tweaks to avoid pointless requests (for example, when no token or owner is configured, we try a pointless request to e.g. api.github.com//example-repo, which obviously won't succeed)
  • Docs overhaul:
    • Clarify how provider authentication works in the case where both the integrations and hashicorp providers are used (I keep returning to @tibbes's excellent comment here about it)
    • Specify in our docs exactly how the decision tree works for each form of authentication (app, provider PAT config, environment variable PAT config) and when/how they can be combined
  • Return an error in some cases where our requests fail but the provider doesn't return an error code (see scenarios described below)

What's next

I'm going to leave this issue open for a couple weeks and hope it inspires some community discussion! Barring major changes to the plan, I'll start work on an implementation in December, and updates will be published to this issue and repo, of course.

References

Related auth issues:

Authorization tests performed

  • no auth, readonly request
    • does not find an owner when an owner is set, and it should
      • the plan/apply succeed, and i don't think they should
    • does not find an owner when an organization is set, and it should
      • the plan/apply succeed, and i don't think they should
      • did get the deprecation notice, which is good
  • env var token set, no env var owner or org set
    • readonly request
      • infers owner of token as owner of repo (not correct in my case)
  • env var token set, env var owner set
    • readonly request
      • correct retrieves repository information
  • env var token set, env var org set to org
    • readonly request
      • correct retrieves repository information and displays deprecation notice
  • env var token set, env var org set to individual
    • readonly request
      • request fails since it does an org lookup first
      • however, terraform succeeds and i don't think it should
  • provider block owner set, no other config
    • readonly request fails and terraform succeeds with the usual 404
  • provider block owner and token set
    • readonly request
      • correctly returns repository information
  • provider block organization and token set
    • readonly request
      • correctly returns repository info and displays deprecation notice
  • provider block organization and owner, env var owner set
    • readonly request
      • env var config supersedes provider block config
  • provider block owner set, env var token set
    • readonly request
      • correctly returns repository info
  • provider block config is interchangeable (and interoperable) with env var config, and env var config is always dominant from what i can tell
  • app auth, no owner in any form set
    • readonly request
      • 404s on GET /user
  • app auth, env var owner set
    • both readonly and write requests work correctly
  • app auth, env var organization set
    • readonly request
      • correctly returns repository info and displays deprecation notice
  • app auth, env var token set
    • conflicts
  • app auth, provider token set
    • conflicts
@jcogilvie
Copy link

Also related is this: #977

App auth tokens expire in 1hr with no renewal mechanism.

@SizZiKe
Copy link

SizZiKe commented Jan 15, 2022

+1 to this as I'm still seeing that implicit provider passing is broken.

@jcudit jcudit added this to the v5.0.0 milestone Jan 20, 2022
@Satak
Copy link

Satak commented Feb 9, 2022

Any ETA for this? Since the authentication is kinda broken for organizations this really put stall to our GitHub management with Terraform.

@alexdepalex
Copy link

alexdepalex commented Feb 9, 2022

Same here. I can create repo's, but that's basically it. For all other resources, I get the dreaded This resource can only be used in the context of an organization, which makes this Provider pretty useless at this point in time.

@posquit0
Copy link

Same here. I'll be glad if you let me know ETA for this. :)

@kfcampbell
Copy link
Member Author

Apologies for the delay! This project isn't officially supported by GitHub; I've been doing it in my spare time. In the past month or so, I've made a concentrated effort to sell it to the organization as something we should support officially. That effort is still ongoing, and in the meantime I've been granted a couple hours a week to work on it. Velocity on the project should pick up a little bit, although I can't promise a date for this work yet.

@Satak
Copy link

Satak commented Jul 8, 2022

What is the current state of this provider? For me the app authentication is still broken and we need to use PAT.

@alexdepalex
Copy link

Still genuine interest here. @kfcampbell Appreciate you're working on this in your spare time.

@jrarmstro
Copy link

In my case, the issue was that I was passing my API URL, https://github.example/api/v3/ as the base URL rather than simply https://github.example/. This results in calls to e.g. https://github.example/api/v3/api/v3/orgs/... which obviously return 404. The provider ultimately spat out This resource can only be used in the context of an organization, "foo" is a user for a PAT and failed to create OAuth token from GitHub App: {"message":"Bad credentials","documentation_url":"https://docs.github.com/enterprise/3.5/rest"} for app auth.

I find the wording in the documentation particularly confusing:

This is the target GitHub base API endpoint.

which implies to me that I am responsible for appending api/v3/.

@posquit0
Copy link

v5.0.0 was released but this issue is not resolved. I was wondering if there are any updated plans for this issue.
I think this issue should be resolved with a high priority.

I would like to thank you for investing your spare time to maintain this project! 😄

@posquit0
Copy link

posquit0 commented Apr 5, 2023

@kfcampbell
Copy link
Member Author

Hi! I'm sorry that this is still an issue. I haven't been able to make progress on any extended effort/investigation unfortunately. PRs are welcome, however!

@froblesmartin
Copy link

Should this be moved to the v6 milestone now? Also, there are many issues opened about authentication issues. Should those be linked to this one somehow to avoid duplication, and let everybody follow just this issue? 🤔

@kfcampbell kfcampbell changed the title Authorization fixes: v5.0.0 proposal Authorization fixes: vNext proposal Jun 14, 2023
@kfcampbell
Copy link
Member Author

We've stopped using milestones to plan releases for the provider recently, but you're correct we're already at v5 and so the title isn't valid anymore. I've changed the title to reflect this.

Also, there are many issues opened about authentication issues. Should those be linked to this one somehow to avoid duplication, and let everybody follow just this issue?

I'd like to use this as a tracking issue for auth upgrades and we can identify other issues as duplicates on a case-by-case basis.

Copy link

👋 Hey Friends, this issue has been automatically marked as stale because it has no recent activity. It will be closed if no further activity occurs. Please add the Status: Pinned label if you feel that this issue needs to remain open/active. Thank you for your contributions and help in keeping things tidy!

@github-actions github-actions bot added the Status: Stale Used by stalebot to clean house label Mar 11, 2024
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Stale Used by stalebot to clean house
Projects
None yet
Development

No branches or pull requests

9 participants