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

Add GitHub app based authentication methods #217

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

Conversation

gdams
Copy link
Member

@gdams gdams commented Feb 14, 2025

See gdams/go#10 as an example of how this looks. This will allow us to no longer depend on the microsoft github bot for raising submodule updates. Note that the reviewer bot still uses PAT for now.

The app we're using can be seen here: https://github.com/apps/microsoft-build-of-go-bot

I've already requested installation in the Microsoft org via https://github.com/microsoft/github-operations/issues/652

For now I'm leaving the review bot as it is. We could either create a second app for auto approving PRs or investigate branch rules which would allow the app to be exempt from the "required reviews" rule.

@gdams gdams requested a review from a team as a code owner February 14, 2025 12:03
@gdams gdams requested a review from Copilot February 14, 2025 12:04
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This PR introduces GitHub App based authentication to replace the dependency on a Microsoft GitHub bot for submodule updates. Key changes include adding a new GitHubAppAuther implementation in gitcmd, updating flag binding and authentication parsing in sync.go, refactoring functions in gitpr, and updating pipeline configurations to use the new bot identity.

Changes

File Description
gitcmd/gitcmd.go Adds GitHubAppAuther and related JWT/token generation functions; integrates HTTP auth insertion methods.
eng/pipelines/steps/set-bot-git-author.yml Updates bot git author configuration to the new bot identity.
sync/sync.go Introduces new flag bindings and updates authentication parsing to support GitHub App credentials.
gitpr/gitpr.go Refactors GraphQL calls to use the new auth abstraction instead of directly using PAT strings.
buildmodel/commands.go Updates PR submission logic to use the new GitHubApp authentication and reviewer auther.

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

gitcmd/gitcmd.go:129

  • Since 'privkey' is already a []byte from the base64 decoding, you can pass it directly to pem.Decode without converting it again.
block, _ := pem.Decode([]byte(privkey))

Tip: Leave feedback on Copilot's review comments with the 👎 and 👍 buttons to help improve review quality. Learn more

gitcmd/gitcmd.go Outdated Show resolved Hide resolved
@dagood
Copy link
Member

dagood commented Feb 14, 2025

microsoft-build-of-go-bot

I don't think this name reads well--we didn't build "Go Bot". 😄 A few other ideas:

  • Microsoft Bot for Go
  • Microsoft build of Go
    • I prefer lowercase build for consistency in general.
    • No need to put bot in the name. (It actually looks bad in the PR to have bot at the end--it's already labeled.)
  • Bot for Go
    • Presumably this will show up as a Microsoft managed app, so can simplify.
  • Build manager for Go
  • Fork maintenance for Go

Putting this out there ahead of any review in case it takes effort to change the name.

@gdams
Copy link
Member Author

gdams commented Feb 14, 2025

microsoft-build-of-go-bot

I don't think this name reads well--we didn't build "Go Bot". 😄 A few other ideas:

  • Microsoft Bot for Go

  • Microsoft build of Go

    • I prefer lowercase build for consistency in general.
    • No need to put bot in the name. (It actually looks bad in the PR to have bot at the end--it's already labeled.)
  • Bot for Go

    • Presumably this will show up as a Microsoft managed app, so can simplify.
  • Build manager for Go

  • Fork maintenance for Go

Putting this out there ahead of any review in case it takes effort to change the name.

agreed, I've renamed it to Bot for Go

@gdams gdams force-pushed the ghapp branch 2 times, most recently from 64d3035 to 2c2fc21 Compare February 17, 2025 14:10
@gdams
Copy link
Member Author

gdams commented Feb 17, 2025

I've added further app support, we can now create github release issues with the app-based auth too. See gdams/go#11

@gdams gdams force-pushed the ghapp branch 3 times, most recently from 5ea1a9f to 58f3eb5 Compare February 18, 2025 09:17

Choose a reason for hiding this comment

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

PR Overview

This PR introduces GitHub App–based authentication to replace the legacy PAT‐based authentication for various operations. Key changes include adding new utility functions for generating a JWT and fetching an installation token in githubutil, extending authentication support in gitcmd (including a new GitHubAppAuther implementation), and updating pipeline definitions and CLI commands (sync, release, tag, etc.) to accommodate GitHub App authentication.

Changes

File Description
githubutil/githubutil.go Adds GitHub App JWT generation and installation token fetching
gitcmd/gitcmd.go Introduces GitHubAppAuther with HTTP auth injection methods
eng/pipelines/*.yml, sync/sync.go, cmd/releasego Updates flags and argument passing to support GitHub App auth
gitpr/gitpr.go Adjusts PR creation and GraphQL query functions to use auth abstraction

Copilot reviewed 19 out of 19 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (3)

gitcmd/gitcmd.go:102

  • [nitpick] Consider caching and reusing the installation token within GitHubAppAuther to avoid redundant API calls when both InsertAuth and InsertHTTPAuth are invoked in close succession.
func (a GitHubAppAuther) getInstallationToken() (string, error) {

sync/sync.go:733

  • [nitpick] Review the conditional logic that checks for missing GitHub user and PAT fields when GitHub App authentication might be used to ensure that the correct authentication method is applied and unintended skipping is avoided.
case *f.GitHubUser == "" && *f.GitHubAppID == 0:

cmd/releasego/get-images-commit.go:66

  • Ensure that errors encountered during installation token fetching are properly propagated (e.g. by returning the error) instead of silently returning nil, in order to avoid masking authentication failures.
if err != nil { return nil }

Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more

@gdams gdams requested review from qmuntal and mertakman February 18, 2025 17:15
@gdams
Copy link
Member Author

gdams commented Feb 18, 2025

The app has been installed in the Microsoft org. I've kicked off a go-infra-upstream-sync pipeline here to test with the app: https://dev.azure.com/dnceng/internal/_build/results?buildId=2645836&view=results

@gdams
Copy link
Member Author

gdams commented Feb 19, 2025

The app has been installed in the Microsoft org. I've kicked off a go-infra-upstream-sync pipeline here to test with the app: https://dev.azure.com/dnceng/internal/_build/results?buildId=2645836&view=results

The job failed although it did successfully raise microsoft/go-images#372. @dagood any idea what went wrong?

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.

2 participants