batchchanges: Load custom certs into the http client used to create GitHub Apps by Piszmog · Pull Request #55084 · sourcegraph/sourcegraph-public-snapshot

Piszmog · 2023-07-18T20:10:42Z

Previously, we were using Go's default HTTP Client (http.DefaultClient) to invoke GitHub to create GitHub apps. This client does not work with custom certificates that may need to be used if calling an on-prem GitHub instance.

Changes

Updated to use our httpcli.UncachedExternalClientFactory instead of http.DefaultClient
- This gives us access to logging, tracing, etc... out of the box
Loading certs that are configured in experimentalFeatures.tls.external.certificates into the HTTP client
- This happpens automagically with httpcli.UncachedExternalClientFactory. It has an option called ExternalTransportOpt that loads the certs from tls.external (see recording to see this in action)

Test plan

Added new tests and tested manually (see below for recordings)

Creating an app

Screen.Recording.2023-07-18.at.14.02.22.mov

Validation of custom certs being loaded

Screen.Recording.2023-07-19.at.10.34.45.mov

sourcegraph-bot · 2023-07-19T16:41:09Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff 228f2fa...6faa5b9.

Notify	File(s)
@sourcegraph/delivery	doc/admin/config/batch_changes.md doc/admin/external_service/github.md
@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

…p can only do commit signing

…ishes

courier-new

Great! 🙌 Just left some feedback about duplicating the new docs section in the repo-syncing GH App docs, too. You should definitely add a CHANGELOG entry for this, too. 😄

courier-new · 2023-07-19T17:28:18Z

+
+### Custom Certificates
+
+If you are using a self-signed certificate for your GitHub Enterprise instance, configure `tls.external` under `experimentalFeatures` 


This configuration step for self-signed certificates would also be required for normal repo-syncing GitHub App set up, right? If so, we probably should double up this section on doc/admin/external_service/github.md (you. might have noticed, the GH Apps part of that doc and this one share the exact same structure and contents, this one's just tailored for any BC-specific nuances and language).

Ah, thank you. I forgot about that doc. I'll copy this section to there as well.

courier-new · 2023-07-19T17:30:46Z

+
+### Custom Certificates
+
+If you are using a self-signed certificate for your GitHub Enterprise instance, configure `tls.external` under `experimentalFeatures` 


Separately, is this experimental feature documented elsewhere? I take from this PR that it's pre-existing functionality, so it'd be cool if we could link to any additional details about it here so an admin could understand where else the cert might be used in SG.

Unfortunately it is not documented anywhere. I dug into when it was first introduced. It was added on Jan 13, 2020 (commit 42ec4ab1).

It probably not an experimental feature anymore 😀

Ah yes, probably not. 😂 Okay, maybe log an issue to GA and document it and call it good? Idk if that's "Code Search" territory or not lol but at least then we'll have a record about this conversation. 😛

lol sounds good I'll create an issue and see where it falls

…certs

…itHub Apps (#55084) Previously, we were using Go's default HTTP Client (`http.DefaultClient`) to invoke GitHub to create GitHub apps. This client does not work with custom certificates that may need to be used if calling an on-prem GitHub instance. ### Changes * Updated to use our `httpcli.UncachedExternalClientFactory` instead of `http.DefaultClient` * This gives us access to logging, tracing, etc... out of the box * Loading certs that are configured in `experimentalFeatures.tls.external.certificates` into the HTTP client * This happpens automagically with `httpcli.UncachedExternalClientFactory`. It has an option called `ExternalTransportOpt` that loads the certs from `tls.external` (see recording to see this in action) ## Test plan Added new tests and tested manually. (cherry picked from commit 97fad93)

…sed to create GitHub Apps (#55238) Previously, we were using Go's default HTTP Client (`http.DefaultClient`) to invoke GitHub to create GitHub apps. This client does not work with custom certificates that may need to be used if calling an on-prem GitHub instance. ### Changes * Updated to use our `httpcli.UncachedExternalClientFactory` instead of `http.DefaultClient` * This gives us access to logging, tracing, etc... out of the box * Loading certs that are configured in `experimentalFeatures.tls.external.certificates` into the HTTP client * This happpens automagically with `httpcli.UncachedExternalClientFactory`. It has an option called `ExternalTransportOpt` that loads the certs from `tls.external` (see recording to see this in action) ## Test plan Added new tests and tested manually (see below for recordings) ### Creating an app https://github.com/sourcegraph/sourcegraph/assets/38407415/ffb78a7b-43a1-41bf-b22e-b2e9f595122f ### Validation of custom certs being loaded https://github.com/sourcegraph/sourcegraph/assets/38407415/47135e09-1fad-4086-82ce-056d4067e386 <br> Backport 97fad93 from #55084 Co-authored-by: Randell Callahan <piszmogcode@gmail.com>

…itHub Apps (#55084) Previously, we were using Go's default HTTP Client (`http.DefaultClient`) to invoke GitHub to create GitHub apps. This client does not work with custom certificates that may need to be used if calling an on-prem GitHub instance. ### Changes * Updated to use our `httpcli.UncachedExternalClientFactory` instead of `http.DefaultClient` * This gives us access to logging, tracing, etc... out of the box * Loading certs that are configured in `experimentalFeatures.tls.external.certificates` into the HTTP client * This happpens automagically with `httpcli.UncachedExternalClientFactory`. It has an option called `ExternalTransportOpt` that loads the certs from `tls.external` (see recording to see this in action) ## Test plan Added new tests and tested manually.

Load custom certs into the http client used to create GitHub Apps

995d329

cla-bot Bot added the cla-signed label Jul 18, 2023

Piszmog added 6 commits July 18, 2023 14:14

Use uncached http client

eda45a1

Update docs about custom certificates and commit signing

961aefa

Add tests

755e1bd

Simplify the creation of the github client

6d87126

Update bazel

8f9cdac

Remove unnecessary field

5acfaa2

Piszmog marked this pull request as ready for review July 19, 2023 16:39

Piszmog requested a review from a team July 19, 2023 16:39

Piszmog added the backport 5.1 label Jul 19, 2023

Update batch change commit signing docs to clarify that the Github ap…

450df6a

…p can only do commit signing

Piszmog changed the title ~~codesearch: Load custom certs into the http client used to create GitHub Apps~~ batchchanges: Load custom certs into the http client used to create GitHub Apps Jul 19, 2023

Piszmog requested a review from courier-new July 19, 2023 16:59

Reset MockCreateGitHubApp to nil when TestGithubAppAuthMiddleware fin…

28bf26e

…ishes

courier-new approved these changes Jul 19, 2023

View reviewed changes

Piszmog added 6 commits July 19, 2023 13:04

Add information when the custom certificate for github apps is available

012c2e2

Add entry for custom certs and creating github apps

efbc80c

Merge remote-tracking branch 'origin/main' into rc/bc-commit-signing-…

97b7bc2

…certs

Trick buildkite

014c5f2

Merge remote-tracking branch 'origin/main' into rc/bc-commit-signing-…

b94720a

…certs

Merge main

6faa5b9

Piszmog merged commit 97fad93 into main Jul 24, 2023

Piszmog deleted the rc/bc-commit-signing-certs branch July 24, 2023 17:58

github-actions Bot mentioned this pull request Jul 24, 2023

[Backport 5.1] batchchanges: Load custom certs into the http client used to create GitHub Apps #55238

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

batchchanges: Load custom certs into the http client used to create GitHub Apps#55084

batchchanges: Load custom certs into the http client used to create GitHub Apps#55084
Piszmog merged 15 commits into
mainfrom
rc/bc-commit-signing-certs

Piszmog commented Jul 18, 2023 •

edited

Loading

Uh oh!

sourcegraph-bot commented Jul 19, 2023 •

edited

Loading

Uh oh!

courier-new left a comment

Uh oh!

courier-new Jul 19, 2023

Uh oh!

Piszmog Jul 19, 2023

Uh oh!

courier-new Jul 19, 2023

Uh oh!

Piszmog Jul 19, 2023 •

edited

Loading

Uh oh!

courier-new Jul 19, 2023 •

edited

Loading

Uh oh!

Piszmog Jul 19, 2023

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants


		### Custom Certificates

		If you are using a self-signed certificate for your GitHub Enterprise instance, configure `tls.external` under `experimentalFeatures`

Conversation

Piszmog commented Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Test plan

Creating an app

Validation of custom certs being loaded

Uh oh!

sourcegraph-bot commented Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

courier-new left a comment

Choose a reason for hiding this comment

Uh oh!

courier-new Jul 19, 2023

Choose a reason for hiding this comment

Uh oh!

Piszmog Jul 19, 2023

Choose a reason for hiding this comment

Uh oh!

courier-new Jul 19, 2023

Choose a reason for hiding this comment

Uh oh!

Piszmog Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

courier-new Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Piszmog Jul 19, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Piszmog commented Jul 18, 2023 •

edited

Loading

sourcegraph-bot commented Jul 19, 2023 •

edited

Loading

Piszmog Jul 19, 2023 •

edited

Loading

courier-new Jul 19, 2023 •

edited

Loading