Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

batches: implement commit signing via GitHub apps#52333

Merged
courier-new merged 37 commits into
mainfrom
batches/commit-signing
Jun 14, 2023
Merged

batches: implement commit signing via GitHub apps#52333
courier-new merged 37 commits into
mainfrom
batches/commit-signing

Conversation

@BolajiOlajide

@BolajiOlajide BolajiOlajide commented May 23, 2023

Copy link
Copy Markdown
Contributor

Closes https://github.com/sourcegraph/sourcegraph/issues/48611.

Test plan

Reminder to tag security-team before merging this into main.

@cla-bot cla-bot Bot added the cla-signed label May 23, 2023
@BolajiOlajide BolajiOlajide changed the title batches: Add domain column to github_apps migration (#52332) batches: implement commit signing via GitHub apps May 23, 2023
@sourcegraph-buildkite

sourcegraph-buildkite commented May 25, 2023

Copy link
Copy Markdown
Collaborator

Bundle size report 📦

Initial size Total size Async size Modules
0.01% (+0.22 kb) 0.00% (+0.65 kb) 0.00% (+0.43 kb) 0.00% (0)

Look at the Statoscope report for a full comparison between the commits b148eb6 and 9ee59be or learn more.

Open explanation
  • Initial size is the size of the initial bundle (the one that is loaded when you open the page)
  • Total size is the size of the initial bundle + all the async loaded chunks
  • Async size is the size of all the async loaded chunks
  • Modules is the number of modules in the initial bundle

BolajiOlajide and others added 6 commits May 26, 2023 20:51
Co-authored-by: Becca Steinbrecher <becca.steinbrecher@sourcegraph.com>
Co-authored-by: Milan Freml <kopancek@users.noreply.github.com>
close #52264
Closes #52270
Exposes new `domain` property from #52268

## Test plan
- Make sure there is a GitHub App on your instance
- In the API Console ensure the following query doesn't fail and returns
the `domain` property:
```
query {
  gitHubApps {
    nodes {
      name
      domain
    }
  }
}
````

---------

Co-authored-by: Milan Freml <kopancek@users.noreply.github.com>
Co-authored-by: Bolaji Olajide <25608335+BolajiOlajide@users.noreply.github.com>
Co-authored-by: Kelli Rockwell <kelli@sourcegraph.com>
Closes [#52346](https://github.com/sourcegraph/sourcegraph/issues/52346)

This PR allows the domain to be specified when creating a GitHub app.
Right now it's hard coded to only work with the `repos` domain till we
figure out the UX for creating a Batch Changes GitHub App.

## Test plan

<!-- All pull requests REQUIRE a test plan:
https://docs.sourcegraph.com/dev/background-information/testing_principles
-->
* Create a GitHub app.
* inspect the `github_apps` table for the newly created GitHub app - the
app should have the value of `repos` in the `domain` column.
…52473)

Closes
#[52465](https://github.com/sourcegraph/sourcegraph/issues/52465), setup
for #[52220](https://github.com/sourcegraph/sourcegraph/issues/52220)

Adds new `supportsCommitSigning ` field to the `BatchChangesCodeHosts`
resolver. Only GitHub external services have commit signing for now.
Later on we will support more code hosts. Even though different
mechanisms will support the commit signing (ie SSH keys), that will be
handled downstream.

## Test plan

![image](https://github.com/sourcegraph/sourcegraph/assets/59381432/dd77362f-769e-4f5d-a8aa-ae755344e9fc)
- Nav to `/site-admin/batch-changes`
- Ensure `/graphql?GlobalBatchChangesCodeHosts` returns the correct
value for `supportsCommitSigning `

---------

Co-authored-by: Kelli Rockwell <kelli@sourcegraph.com>
Co-authored-by: Becca Steinbrecher <beccasteinbrecher@gmail.com>
Co-authored-by: Kelli Rockwell <kelli@sourcegraph.com>
Co-authored-by: Becca Steinbrecher <becca.steinbrecher@sourcegraph.com>
Closes [#52324](https://github.com/sourcegraph/sourcegraph/issues/52324)
@BolajiOlajide BolajiOlajide force-pushed the batches/commit-signing branch from e356e40 to b148eb6 Compare May 26, 2023 19:52
courier-new and others added 13 commits June 1, 2023 09:49
This PR is a revert of [52136](https://github.com/sourcegraph/sourcegraph/pull/52136). It creates a `github_app_installs` table that tracks the installation of every GitHub app on the Sourcegraph instance.

Closes #52494

## Test plan

<!-- All pull requests REQUIRE a test plan: https://docs.sourcegraph.com/dev/background-information/testing_principles -->

* Execute the latest migrations with the command `sg migration up`
* Install a GitHub app via the GitHub apps page the attempt to install the app.
* if the installation is successful, the installation ID will be saved in the `installation_id` column in the `github_app_installs` table.
…52898)

We have a named type called `GitHubAppDomain`. However, the constants that we create of the named type are called `Domain`; this can lead to confusion in the future when there's a named type called `Domain`.

I've renamed `BatchesDomain` and `ReposDomain` to `BatchesGitHubAppDomain` and `ReposGitHubAppDomain` to ensure the constants follow the naming of the named type.

## Test plan

<!-- All pull requests REQUIRE a test plan: https://docs.sourcegraph.com/dev/background-information/testing_principles -->
* Every GitHub app-related operation should work as is.
* Just a variable name change.
…t` type (#52733)

Closes #52484.

- Surfaces a `GitHubAppConfiguration` property on `BatchChangesCodeHost`
type
- Normalizes `github_app.base_url`
  - Adds migration to fix past entries
- Adds a check to not create duplicate `batches` GH Apps on the same
`base_url`
- Follow up issue:
#[52792](https://github.com/sourcegraph/sourcegraph/issues/52792)


![image](https://github.com/sourcegraph/sourcegraph/assets/59381432/5265153c-0b26-4544-9f47-44c4253c1c5e)

## Test plan
- Query new field
```
query {
  batchChangesCodeHosts {
    nodes {
      externalServiceURL
      supportsCommitSigning
      commitSigningConfiguration {
        __typename
        	... on GitHubAppConfiguration {
          	appID
            name
            appURL
            logo
          }
      }
    }
  }
}
```
- `sg migration up` successfully adds a trailing `/` on any
`github_apps.base_url` missing it
- Change [this
line](https://github.com/sourcegraph/sourcegraph/pull/52733/files#diff-e8fe19664bf9f7c5e69983ecad802bdf6610083828a4d3faee9eddf430ab068fR108)
to `if domain == types.ReposDomain {`, go through the UI to add a new
GitHub App on an existing `base_url`, observe it error

---------

Co-authored-by: Bolaji Olajide <25608335+BolajiOlajide@users.noreply.github.com>
Co-authored-by: Milan Freml <kopancek@users.noreply.github.com>
Co-authored-by: Kelli Rockwell <kelli@sourcegraph.com>
courier-new and others added 13 commits June 6, 2023 18:04
This PR introduces a background worker that runs every 24 hours. This background worker adds data for new installations of a GitHub app to the database and removes installations that no longer exist on GitHub.

This ensures that Sourcegraph has some level of sync consistency with GitHub.

## Test plan

<!-- All pull requests REQUIRE a test plan: https://docs.sourcegraph.com/dev/background-information/testing_principles -->
* Create a GitHub app and ensure the installation information isn't saved in the database. If you have pre-existing apps before the introduction of #52618 then you already have GitHub apps without installation data saved.
* Starting your SG instance instantiates a background worker that'll backfill the installation information and save it in the `github_app_installs`  table.

@courier-new courier-new left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey @sourcegraph/security-code-review would love a quick review of a couple things here! This PR enables commit signing with Batch Changes via GitHub Apps. We were already able to connect GitHub Apps to Sourcegraph prior to this, so this PR just builds off of that a bit. As such, there's not a whole lot that I think warrants your attention, but I called out just a few areas of interest in the hopes you could give it some eyes! Notably:

  • The middleware layer we use to facilitate the GitHub App connection process, keep state, and handle the installation flow redirects
  • A new background worker used for recording and syncing additional metadata about individual GitHub App Installations
  • In the Batch Changes code, authenticating and pushing commits via a GitHub App Installation, if we find one that works for a given repo

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Here we're passing a couple more parameters back and forth between the frontend, the middleware, and GitHub and store a couple of these in the redis cache as well. We collect a bit more information on the new GitHub App installation, and we handle errors a bit more gracefully.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This background worker requests the current set of installations for a GitHub App on GitHub and then syncs our records in the DB to match it. We also opportunistically perform this sync when an admin visits the GitHub App page in the web UI.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Here we now detect if we're pushing commits to a GitHub code host and try to authenticate with a GitHub App installation. If we can, we'll push a commit with that authentication in order to sign it. If we can't, we just return.

@courier-new courier-new marked this pull request as ready for review June 13, 2023 17:46
@sourcegraph-bot

sourcegraph-bot commented Jun 13, 2023

Copy link
Copy Markdown
Contributor

📖 Storybook live preview

@evict evict left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good! I only have one recommendation for the caching bit. 👍

Comment thread enterprise/cmd/frontend/internal/auth/githubappauth/middleware.go
@courier-new courier-new requested a review from evict June 14, 2023 15:23
@sourcegraph-bot

sourcegraph-bot commented Jun 14, 2023

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff 1a5376d...6c20cff.

Notify File(s)
@courier-new client/web/src/enterprise/batches/settings/BatchChangesCreateGitHubAppPage.tsx
client/web/src/enterprise/batches/settings/BatchChangesSettingsArea.story.tsx
client/web/src/enterprise/batches/settings/BatchChangesSettingsArea.tsx
client/web/src/enterprise/batches/settings/BatchChangesSiteConfigSettingsArea.story.tsx
client/web/src/enterprise/batches/settings/BatchChangesSiteConfigSettingsPage.story.tsx
client/web/src/enterprise/batches/settings/BatchChangesSiteConfigSettingsPage.tsx
client/web/src/enterprise/batches/settings/CheckButton.tsx
client/web/src/enterprise/batches/settings/CodeHostConnectionNode.module.scss
client/web/src/enterprise/batches/settings/CodeHostConnectionNode.story.tsx
client/web/src/enterprise/batches/settings/CodeHostConnectionNode.tsx
client/web/src/enterprise/batches/settings/CodeHostConnections.tsx
client/web/src/enterprise/batches/settings/CommitSigningIntegrationNode.module.scss
client/web/src/enterprise/batches/settings/CommitSigningIntegrationNode.tsx
client/web/src/enterprise/batches/settings/CommitSigningIntegrations.tsx
client/web/src/enterprise/batches/settings/backend.ts
client/web/src/integration/batches.test.ts
cmd/frontend/graphqlbackend/batches.go
@efritz enterprise/cmd/worker/internal/githubapps/BUILD.bazel
enterprise/cmd/worker/internal/githubapps/job.go
enterprise/cmd/worker/internal/githubapps/worker/BUILD.bazel
enterprise/cmd/worker/internal/githubapps/worker/installation_backfill.go
enterprise/cmd/worker/internal/githubapps/worker/installation_backfill_test.go
enterprise/cmd/worker/shared/BUILD.bazel
enterprise/cmd/worker/shared/shared.go
@eseliger client/web/src/enterprise/batches/settings/BatchChangesCreateGitHubAppPage.tsx
client/web/src/enterprise/batches/settings/BatchChangesSettingsArea.story.tsx
client/web/src/enterprise/batches/settings/BatchChangesSettingsArea.tsx
client/web/src/enterprise/batches/settings/BatchChangesSiteConfigSettingsArea.story.tsx
client/web/src/enterprise/batches/settings/BatchChangesSiteConfigSettingsPage.story.tsx
client/web/src/enterprise/batches/settings/BatchChangesSiteConfigSettingsPage.tsx
client/web/src/enterprise/batches/settings/CheckButton.tsx
client/web/src/enterprise/batches/settings/CodeHostConnectionNode.module.scss
client/web/src/enterprise/batches/settings/CodeHostConnectionNode.story.tsx
client/web/src/enterprise/batches/settings/CodeHostConnectionNode.tsx
client/web/src/enterprise/batches/settings/CodeHostConnections.tsx
client/web/src/enterprise/batches/settings/CommitSigningIntegrationNode.module.scss
client/web/src/enterprise/batches/settings/CommitSigningIntegrationNode.tsx
client/web/src/enterprise/batches/settings/CommitSigningIntegrations.tsx
client/web/src/enterprise/batches/settings/backend.ts
client/web/src/integration/batches.test.ts
cmd/frontend/graphqlbackend/batches.go
enterprise/internal/batches/reconciler/executor.go
enterprise/internal/batches/sources/BUILD.bazel
enterprise/internal/batches/sources/github.go
enterprise/internal/batches/sources/github_test.go
enterprise/internal/batches/sources/mocks_test.go
enterprise/internal/batches/sources/sources.go
enterprise/internal/batches/sources/sources_test.go
enterprise/internal/batches/sources/testdata/sources/GithubSource_DuplicateCommit_invalid_ref.yaml
enterprise/internal/batches/sources/testdata/sources/GithubSource_DuplicateCommit_success.yaml
enterprise/internal/batches/sources/testing/fake.go
enterprise/internal/batches/store/BUILD.bazel
enterprise/internal/batches/store/store.go
enterprise/internal/batches/syncer/BUILD.bazel
enterprise/internal/batches/syncer/mocks_test.go
enterprise/internal/batches/syncer/store.go
enterprise/internal/batches/syncer/syncer.go
enterprise/internal/batches/types/code_host.go
internal/extsvc/github/common.go
internal/extsvc/github/testdata/golden/TestV3Client_CreateCommit_failure
internal/extsvc/github/testdata/golden/TestV3Client_CreateCommit_success
internal/extsvc/github/testdata/golden/TestV3Client_GetRef_failure
internal/extsvc/github/testdata/golden/TestV3Client_GetRef_success
internal/extsvc/github/testdata/golden/TestV3Client_UpdateRef_failure
internal/extsvc/github/testdata/golden/TestV3Client_UpdateRef_success
internal/extsvc/github/testdata/vcr/TestV3Client_CreateCommit_failure.yaml
internal/extsvc/github/testdata/vcr/TestV3Client_CreateCommit_success.yaml
internal/extsvc/github/testdata/vcr/TestV3Client_GetRef_failure.yaml
internal/extsvc/github/testdata/vcr/TestV3Client_GetRef_success.yaml
internal/extsvc/github/testdata/vcr/TestV3Client_UpdateRef_failure.yaml
internal/extsvc/github/testdata/vcr/TestV3Client_UpdateRef_success.yaml
internal/extsvc/github/v3.go
internal/extsvc/github/v3_test.go
internal/extsvc/github/v4.go
@sashaostrikov internal/extsvc/github/common.go
internal/extsvc/github/testdata/golden/TestV3Client_CreateCommit_failure
internal/extsvc/github/testdata/golden/TestV3Client_CreateCommit_success
internal/extsvc/github/testdata/golden/TestV3Client_GetRef_failure
internal/extsvc/github/testdata/golden/TestV3Client_GetRef_success
internal/extsvc/github/testdata/golden/TestV3Client_UpdateRef_failure
internal/extsvc/github/testdata/golden/TestV3Client_UpdateRef_success
internal/extsvc/github/testdata/vcr/TestV3Client_CreateCommit_failure.yaml
internal/extsvc/github/testdata/vcr/TestV3Client_CreateCommit_success.yaml
internal/extsvc/github/testdata/vcr/TestV3Client_GetRef_failure.yaml
internal/extsvc/github/testdata/vcr/TestV3Client_GetRef_success.yaml
internal/extsvc/github/testdata/vcr/TestV3Client_UpdateRef_failure.yaml
internal/extsvc/github/testdata/vcr/TestV3Client_UpdateRef_success.yaml
internal/extsvc/github/v3.go
internal/extsvc/github/v3_test.go
internal/extsvc/github/v4.go
@unknwon enterprise/cmd/frontend/internal/auth/githubappauth/BUILD.bazel
enterprise/cmd/frontend/internal/auth/githubappauth/middleware.go
enterprise/cmd/frontend/internal/auth/githubappauth/middleware_test.go
enterprise/cmd/frontend/internal/auth/githubappauth/resolver.go
enterprise/cmd/frontend/internal/auth/githubappauth/resolver_test.go

@courier-new courier-new merged commit a5eba6a into main Jun 14, 2023
@courier-new courier-new deleted the batches/commit-signing branch June 14, 2023 18:12
ErikaRS pushed a commit that referenced this pull request Jun 22, 2023
Co-authored-by: Becca Steinbrecher <beccasteinbrecher@gmail.com>
Co-authored-by: Kelli Rockwell <kelli@sourcegraph.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

batch-changes Issues related to Batch Changes cla-signed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sign commits created by Batch Changes with GitHub App (Tracking)

6 participants