GitHub proxy part 5: OAuth flow to retrieve GitHub identity by greedy52 · Pull Request #49849 · gravitational/teleport

greedy52 · 2024-12-05T22:21:20Z

GitHub proxy MVP #48762

GitHub OAuth flow for authenticated user overview

sequenceDiagram
    participant tsh
    participant client browser
    participant Proxy
    participant Auth
    participant GitHub
                                
    tsh->>Proxy: GitServerClient().CreateGitHubAuthRequest
    Proxy->>Auth: CreateGitHubAuthRequest
    tsh->> client browser: open
    client browser->> GitHub: redirect
    GitHub<<->>Proxy: callback
    Proxy<<->>Auth: verify callback and generate new cert with GitHub user ID
    client browser->>tsh: new cert with GitHub user ID

tsh UX example:

$ tsh status
> Profile URL:        https://teleport-test.stevexin.app:443
  Logged in as:       admin
  Cluster:            teleport-test.stevexin.app
  Roles:              access, auditor, editor
  Kubernetes:         enabled
  Valid until:        2024-12-06 07:36:28 -0500 EST [valid for 10h52m0s]
  Extensions:         login-ip, permit-agent-forwarding, permit-port-forwarding, permit-pty, private-key-policy

$ tsh git login --github-org goteleport-core-test --force
If browser window does not open automatically, open it by clicking on the link:
 http://127.0.0.1:61928/6481c191-f0f5-47e8-ad7e-60c7a84f53ec
Your GitHub username is greedy52.

$ tsh git ls
Type   Organization         Username URL                                     
------ -------------------- -------- --------------------------------------- 
GitHub goteleport-core-test greedy52 https://github.com/goteleport-core-test 

hint: use 'tsh git clone <git-clone-ssh-url>' to clone a new repository
      use 'tsh git config update' to configure an existing repository to use Teleport
      once the repository is cloned or configured, use 'git' as normal

$ tsh status | grep -A2 GitHub
  GitHub username:    greedy52
  Valid until:        2024-12-06 07:36:28 -0500 EST [valid for 10h49m0s]
  Extensions:         login-ip, permit-agent-forwarding, permit-port-forwarding, permit-pty, private-key-policy

TODO

make -C integrations/operator crd

greedy52 · 2024-12-05T22:26:54Z

+	// Auth was successful, return session, certificate, etc. to caller.
+	return a.makeGithubAuthResponse(ctx, req, userState, userResp, params.SessionTTL)
+}
+
+func (a *Server) makeGithubAuthResponse(


just some refactoring to break the big function. same below to split getGithubUserAndTeams

Tener

First pass.

Tener · 2024-12-06T09:43:17Z


+	// Preserve states like GitHub identities across logins.
+	// TODO(greedy52) implement a way to remove the identity or find a way to
+	// avoid keeping the identity forever.


Should we set some kind of TTL corresponding to the TTL returned by Github for the auth request?

I don't think Github provide any meaningful TTL for the identity itself. we might need to introduce something like role.max_external_identity_ttl = 168h

Yes, I think we should put a limit here. A week feels like a reasonable compromise.

Tener

I did another round and I feel comfortable enough approving this. Notably I was already familiar with trickier areas of this PR but it still required above-average effort due to accumulated complexity.

Tener · 2024-12-09T09:59:36Z

+	// TODO(greedy52) make "github-org" optional. Most likely there is only a
+	// single Git server configured anyway so do a "list" op then use the
+	// organization from that Git server. If more than one Git servers are
+	// found, prompt the user to pick one.


A very nice idea.

Tener · 2024-12-09T10:03:26Z

 // ValidateGithubAuthCallback validates Github auth callback redirect
 func (a *Server) validateGithubAuthCallback(ctx context.Context, diagCtx *SSODiagContext, q url.Values) (*authclient.GithubAuthResponse, error) {
-	logger := log.WithFields(logrus.Fields{teleport.ComponentKey: "github"})
+	logger := a.logger.With(teleport.ComponentKey, "github")


I feel like validateGithubAuthCallback and friends could benefit from refactoring. Right now is probably not the best time, but the original code didn't anticipate the use-cases that have been added with time and it shows. Possibly the opportunity for this will arrive in the future?

smallinsky · 2024-12-09T12:48:26Z

+	}
+
+	in.Request.ConnectorID = uuid
+	in.Request.ConnectorSpec = spec


The ConnectorSpec is marked as Used only in test

teleport/integrations/operator/crdgen/testdata/protofiles/teleport/legacy/types/types.proto

Line 5197 in 6995108

GithubConnectorSpecV3 ConnectorSpec = 15 [(gogoproto.jsontag) = "connector_spec,omitempty"];

i did update api/proto/teleport/legacy/types/types.proto to indicate that ConnectorSpec is for both flows (basically the spec is used when not using "native" GitHub connector). I had a comment in PR description that i need to run make -C integrations/operator crd before merge.

github-actions · 2024-12-10T16:44:13Z

🤖 Vercel preview here: https://docs-4q81q907q-goteleport.vercel.app/docs

github-actions · 2024-12-10T17:21:07Z

🤖 Vercel preview here: https://docs-ie4zozxl8-goteleport.vercel.app/docs

* GitHub proxy part 5: OAuth flow to retrieve GitHub identity * review comments round1 * review comments round 2 and update tsh git list * make -C integrations/operator crd * make -C integrations/terraform docs * fix flaky test

* GitHub Proxy part 1: github integration resource (#48999) * github integration resource * fix lib/web * revert withSecrets * use static credentials * address review comments * fix ut * GitHub Proxy part 2: git_server resource, service, and RBAC (#49393) * git_server resource and role.allow.github_permissions * implicit RO on KindGitServer * review comments * fix ut * make -C integrations/operator crd * fix ut again * make crds-up-to-date and make -C integrations/terraform docs * GitHub proxy part 1.5: integration in web ui (#49561) * GitHub proxy part 1.5: integration in web ui * fix lint * GitHub Proxy part 3.5: caching PluginStaticCredentials (#49472) * GitHub Proxy part 3.5: caching PluginStaticCredentials * fix lint * GitHub proxy part 2.5: git_server cache (#49564) * GitHub proxy part 2.5: git_server cache * revert event * fix getAll * review comments * GitHub Proxy part 3: gen github user cert and export CA (#49396) * GitHub Proxy part 3: gen github user cert and export CA * address pr comment * minor refactor * use cache * fix build and cache * GitHub proxy part 4: `tsh git ls` with unified resource (#49596) * GitHub proxy part 4: tsh git ls * fix ut * update username note * fix * GitHub proxy part 5: OAuth flow to retrieve GitHub identity (#49849) * GitHub proxy part 5: OAuth flow to retrieve GitHub identity * review comments round1 * review comments round 2 and update tsh git list * make -C integrations/operator crd * make -C integrations/terraform docs * fix flaky test * GitHub proxy part 6.5: tsh git ssh/clone/config (#50044) * GitHub proxy part 6.5: tsh git ssh/clone/config * review comments * fix test * fix ut for lookpath * fix logger and update dependency version * go mod tidy for integrations * GitHub proxy part 7: audit events (#49923) * GitHub proxy part 7: audit events * make Git Command consistent * fix typo * GitHub proxy: git command recorder (#50505) * GitHub proxy: recording git command * address review * review comments * allow flags after repository for git-upload-pack * GitHub proxy part 6: proxing Git using SSH transport (#49980) * GitHub proxy part 6: proxing Git using SSH transport * better command parsing and update suite * refactor * revert unnecearrty files * address review comments * ut fix * revert localsite_test.go * change special suffix to teleport-github-org for routing * fix routing ut * minor typo edit * fix ut after sshca change * add UT to sshutils * minor review comments * fix api ut because of special suffix change * GitServerReadOnlyClient * downgrade error to warning * run go mod tidy. not sure why it's needed * rename mock.go to mock_test.go * GitHub Proxy: complete audit event flow and add an enterprise guard (#51049) * fix lint and remove accidently checked-in binary * Fix flaky git.TestForwardServer test (#51112)

…ional#49849) * GitHub proxy part 5: OAuth flow to retrieve GitHub identity * review comments round1 * review comments round 2 and update tsh git list * make -C integrations/operator crd * make -C integrations/terraform docs * fix flaky test

greedy52 added the no-changelog Indicates that a PR does not require a changelog entry label Dec 5, 2024

greedy52 self-assigned this Dec 5, 2024

greedy52 mentioned this pull request Dec 5, 2024

GitHub proxy MVP #48762

Closed

9 tasks

greedy52 commented Dec 5, 2024

View reviewed changes

greedy52 force-pushed the STeve/48762_github_oauth_flow branch from f30da58 to 7e9fd6e Compare December 6, 2024 01:48

greedy52 requested review from Tener, r0mant and smallinsky December 6, 2024 01:49

greedy52 marked this pull request as ready for review December 6, 2024 01:49

github-actions Bot added size/lg tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels Dec 6, 2024

github-actions Bot requested review from creack and fspmarshall December 6, 2024 01:49

GitHub proxy part 5: OAuth flow to retrieve GitHub identity

5ab9743

greedy52 force-pushed the STeve/48762_github_oauth_flow branch from 7e9fd6e to 5ab9743 Compare December 6, 2024 02:17

Tener reviewed Dec 6, 2024

View reviewed changes

review comments round1

b5d38f5

greedy52 requested a review from Tener December 9, 2024 01:59

Tener approved these changes Dec 9, 2024

View reviewed changes

smallinsky reviewed Dec 9, 2024

View reviewed changes

review comments round 2 and update tsh git list

c20d7ef

greedy52 force-pushed the STeve/48762_github_oauth_flow branch from 3ee441e to c20d7ef Compare December 10, 2024 03:06

greedy52 requested a review from smallinsky December 10, 2024 03:10

smallinsky approved these changes Dec 10, 2024

View reviewed changes

public-teleport-github-review-bot Bot removed request for creack, fspmarshall and r0mant December 10, 2024 08:26

make -C integrations/operator crd

7fc3873

greedy52 temporarily deployed to vercel December 10, 2024 16:33 — with GitHub Actions Inactive

greedy52 added 2 commits December 10, 2024 11:59

make -C integrations/terraform docs

7f9eaf2

fix flaky test

e1703d8

greedy52 temporarily deployed to vercel December 10, 2024 17:10 — with GitHub Actions Inactive

greedy52 added this pull request to the merge queue Dec 10, 2024

Merged via the queue into master with commit 80a58fc Dec 10, 2024

greedy52 deleted the STeve/48762_github_oauth_flow branch December 10, 2024 17:50

Conversation

greedy52 commented Dec 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tener left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tener left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Dec 10, 2024

Uh oh!

github-actions Bot commented Dec 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

greedy52 commented Dec 5, 2024 •

edited

Loading