From dfa3acf38979aca1f0eadf93fec8a83a5877595e Mon Sep 17 00:00:00 2001 From: Sebastian Bezold Date: Tue, 13 Aug 2024 16:04:16 +0200 Subject: [PATCH 1/5] feat: add support for GitHub enterprise Signed-off-by: Sebastian Bezold --- .gitignore | 1 + operator.md | 18 +++++---- pkg/config/operator/operator.go | 5 +++ pkg/config/operator/operator_test.go | 9 +++++ pkg/ghclients/ghclients.go | 8 +++- pkg/ghclients/ghclients_test.go | 56 ++++++++++++++++++++++++++++ pkg/policies/security/security.go | 9 ++++- 7 files changed, 96 insertions(+), 10 deletions(-) diff --git a/.gitignore b/.gitignore index d93bbc9c..b7d99f97 100644 --- a/.gitignore +++ b/.gitignore @@ -2,3 +2,4 @@ cmd/allstar/allstar *.pem .vscode allstar.ref +.idea/ \ No newline at end of file diff --git a/operator.md b/operator.md index be7ab389..189428ed 100644 --- a/operator.md +++ b/operator.md @@ -55,12 +55,14 @@ conditions on enforcement actions, ex: pinging an issue twice at the same time. ## Configuration via Environment Variables Allstar supports various operator configuration options which can be set via environment variables: -|Name|Description|Default| -|----|----|----| -|APP_ID|The application ID of the created GitHub App.|| -|PRIVATE_KEY|The raw value of the private key for the GitHub App. KEY_SECRET must be set to "direct".|| -|KEY_SECRET|The name of a secret containing a private key.|| -|DO_NOTHING_ON_OPT_OUT|Boolean flag which defines if allstar should do nothing and skip the corresponding checks when a repository is opted out.|false| -|ALLSTAR_LOG_LEVEL|The minimum logging level that allstar should use when emitting logs. Acceptable values are: panic ; fatal ; error ; warn ; info ; debug ; trace|info| -|NOTICE_PING_DURATION_HOURS|The duration (in hours) to wait between pinging notice actions, such as updating a GitHub issue.|24| + +| Name | Description | Default | +|----------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------|---------| +| APP_ID | The application ID of the created GitHub App. || +| PRIVATE_KEY | The raw value of the private key for the GitHub App. KEY_SECRET must be set to "direct". || +| KEY_SECRET | The name of a secret containing a private key. || +| ALLSTAR_GHE_URL | The URL of the GitHub Enterprise instance to use. Leave empty to use github.com || +| DO_NOTHING_ON_OPT_OUT | Boolean flag which defines if allstar should do nothing and skip the corresponding checks when a repository is opted out. | false | +| ALLSTAR_LOG_LEVEL | The minimum logging level that allstar should use when emitting logs. Acceptable values are: panic ; fatal ; error ; warn ; info ; debug ; trace | info | +| NOTICE_PING_DURATION_HOURS | The duration (in hours) to wait between pinging notice actions, such as updating a GitHub issue. | 24 | diff --git a/pkg/config/operator/operator.go b/pkg/config/operator/operator.go index 636138a9..7abad7de 100644 --- a/pkg/config/operator/operator.go +++ b/pkg/config/operator/operator.go @@ -42,6 +42,9 @@ const setKeySecret = "gcpsecretmanager://projects/allstar-ossf/secrets/allstar-p var KeySecret string +// GitHubEnterpriseUrl allows to configure the usage a GitHub enterprise instance +var GitHubEnterpriseUrl string + // OrgConfigRepo is the name of the expected org-level repo to contain config. const OrgConfigRepo = ".allstar" @@ -126,6 +129,8 @@ func setVars() { KeySecret = setKeySecret } + GitHubEnterpriseUrl = osGetenv("ALLSTAR_GHE_URL") + doNothingOnOptOutStr := osGetenv("DO_NOTHING_ON_OPT_OUT") doNothingOnOptOut, err := strconv.ParseBool(doNothingOnOptOutStr) if err == nil { diff --git a/pkg/config/operator/operator_test.go b/pkg/config/operator/operator_test.go index 23c45232..4aa53ab9 100644 --- a/pkg/config/operator/operator_test.go +++ b/pkg/config/operator/operator_test.go @@ -27,6 +27,7 @@ func TestSetVars(t *testing.T) { Name string AppID string KeySecret string + GitHubEnterpriseUrl string NoticePingDurationHrs string PrivateKey string DoNothingOnOptOut string @@ -42,6 +43,7 @@ func TestSetVars(t *testing.T) { Name: "NoVars", AppID: "", KeySecret: "", + GitHubEnterpriseUrl: "", DoNothingOnOptOut: "", ExpAppID: setAppID, ExpKeySecret: setKeySecret, @@ -53,6 +55,7 @@ func TestSetVars(t *testing.T) { Name: "SetVars", AppID: "123", KeySecret: "asdf", + GitHubEnterpriseUrl: "https://ghe.example.com", DoNothingOnOptOut: "true", ExpAppID: 123, ExpKeySecret: "asdf", @@ -169,6 +172,9 @@ func TestSetVars(t *testing.T) { if in == "KEY_SECRET" { return test.KeySecret } + if in == "ALLSTAR_GHE_URL" { + return test.GitHubEnterpriseUrl + } if in == "DO_NOTHING_ON_OPT_OUT" { return test.DoNothingOnOptOut } @@ -190,6 +196,9 @@ func TestSetVars(t *testing.T) { if diff := cmp.Diff(test.ExpKeySecret, KeySecret); diff != "" { t.Errorf("Unexpected results. (-want +got):\n%s", diff) } + if diff := cmp.Diff(test.GitHubEnterpriseUrl, GitHubEnterpriseUrl); diff != "" { + t.Errorf("Unexpected results. (-want +got):\n%s", diff) + } if diff := cmp.Diff(test.ExpDoNothingOnOptOut, DoNothingOnOptOut); diff != "" { t.Errorf("Unexpected results. (-want +got):\n%s", diff) } diff --git a/pkg/ghclients/ghclients.go b/pkg/ghclients/ghclients.go index 187e8032..ba5d240c 100644 --- a/pkg/ghclients/ghclients.go +++ b/pkg/ghclients/ghclients.go @@ -98,10 +98,16 @@ func (g *GHClients) Get(i int64) (*github.Client, error) { } else { tr, err = ghinstallationNew(ctr, operator.AppID, i, g.key) } + + c := github.NewClient(&http.Client{Transport: tr}) + if operator.GitHubEnterpriseUrl != "" { + c, err = c.WithEnterpriseURLs(operator.GitHubEnterpriseUrl, operator.GitHubEnterpriseUrl) + } if err != nil { return nil, err } - g.clients[i] = github.NewClient(&http.Client{Transport: tr}) + + g.clients[i] = c return g.clients[i], nil } diff --git a/pkg/ghclients/ghclients_test.go b/pkg/ghclients/ghclients_test.go index 094d6463..73a443cd 100644 --- a/pkg/ghclients/ghclients_test.go +++ b/pkg/ghclients/ghclients_test.go @@ -23,6 +23,7 @@ import ( "github.com/bradleyfalzon/ghinstallation/v2" "github.com/google/go-cmp/cmp" + "github.com/ossf/allstar/pkg/config/operator" ) func TestGet(t *testing.T) { @@ -132,3 +133,58 @@ func TestGetKey(t *testing.T) { }) } } + +func TestGitHubEnterpriseClient(t *testing.T) { + tests := []struct { + Name string + GitHubEnterpriseUrl string + installId int64 + expectedBaseUrl string + expectedUploadUrl string + }{ + { + Name: "ZeroAppId", + GitHubEnterpriseUrl: "https://ghezero.example.com", + installId: 0, + expectedBaseUrl: "https://ghezero.example.com/api/v3/", + expectedUploadUrl: "https://ghezero.example.com/api/uploads/", + }, + { + Name: "NonZeroAppId", + GitHubEnterpriseUrl: "https://ghenonzero.example.com", + installId: 123, + expectedBaseUrl: "https://ghenonzero.example.com/api/v3/", + expectedUploadUrl: "https://ghenonzero.example.com/api/uploads/", + }, + { + Name: "NoGitHubEnterpriseUrl", + GitHubEnterpriseUrl: "", + installId: 0, + expectedBaseUrl: "https://api.github.com/", + expectedUploadUrl: "https://uploads.github.com/", + }, + } + + for _, test := range tests { + t.Run(test.Name, func(t *testing.T) { + operator.GitHubEnterpriseUrl = test.GitHubEnterpriseUrl + + ghc, err := NewGHClients(context.Background(), http.DefaultTransport) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + c, err := ghc.Get(test.installId) + if err != nil { + t.Fatalf("Unexpected error: %v", err) + } + + if c.BaseURL.String() != test.expectedBaseUrl { + t.Errorf("Did not read GitHub instance URL from operator config.\nExpected %s, got %s", test.expectedBaseUrl, c.BaseURL.String()) + } + if c.UploadURL.String() != test.expectedUploadUrl { + t.Errorf("Did not read GitHub upload URL from operator config\nExpected %s, got %s", test.expectedUploadUrl, c.UploadURL.String()) + } + }) + } +} diff --git a/pkg/policies/security/security.go b/pkg/policies/security/security.go index 29297a8c..01ce20bd 100644 --- a/pkg/policies/security/security.go +++ b/pkg/policies/security/security.go @@ -20,6 +20,7 @@ import ( "fmt" "github.com/ossf/allstar/pkg/config" + "github.com/ossf/allstar/pkg/config/operator" "github.com/ossf/allstar/pkg/policydef" "github.com/google/go-github/v59/github" @@ -97,7 +98,13 @@ func (s Security) Name() string { // configuration stored in the org/repo, implementing policydef.Policy.Check() func (s Security) Check(ctx context.Context, c *github.Client, owner, repo string) (*policydef.Result, error) { - v4c := githubv4.NewClient(c.Client()) + + var v4c v4client + if operator.GitHubEnterpriseUrl == "" { + v4c = githubv4.NewClient(c.Client()) + } else { + v4c = githubv4.NewEnterpriseClient(operator.GitHubEnterpriseUrl, c.Client()) + } return check(ctx, c, v4c, owner, repo) } From cfb608c4a9c83c68e74a44bfbd39b4b0dc95c379 Mon Sep 17 00:00:00 2001 From: Sebastian Bezold Date: Thu, 15 Aug 2024 15:40:03 +0200 Subject: [PATCH 2/5] fix: ensure correct enterprise API URLs for GraphQL + http client Signed-off-by: Sebastian Bezold --- pkg/ghclients/ghclients.go | 35 ++++++++++++++++++++++++------- pkg/policies/security/security.go | 2 +- 2 files changed, 29 insertions(+), 8 deletions(-) diff --git a/pkg/ghclients/ghclients.go b/pkg/ghclients/ghclients.go index ba5d240c..f1b7a4eb 100644 --- a/pkg/ghclients/ghclients.go +++ b/pkg/ghclients/ghclients.go @@ -19,6 +19,7 @@ package ghclients import ( "context" "net/http" + "strings" "github.com/bradleyfalzon/ghinstallation/v2" "github.com/google/go-github/v59/github" @@ -92,25 +93,45 @@ func (g *GHClients) Get(i int64) (*github.Client, error) { } var tr http.RoundTripper - var err error if i == 0 { - tr, err = ghinstallationNewAppsTransport(ctr, operator.AppID, g.key) + appTransport, _ := ghinstallationNewAppsTransport(ctr, operator.AppID, g.key) + // other than clien.WithEnterpriseUrls, setting the BaseUrl plainly, we need to ensure the /api/v3 ending + appTransport.BaseURL = fullEnterpriseApiUrl(operator.GitHubEnterpriseUrl) + tr = appTransport } else { - tr, err = ghinstallationNew(ctr, operator.AppID, i, g.key) + ghiTransport, _ := ghinstallationNew(ctr, operator.AppID, i, g.key) + if operator.GitHubEnterpriseUrl != "" { + ghiTransport.BaseURL = fullEnterpriseApiUrl(operator.GitHubEnterpriseUrl) + } + tr = ghiTransport + } c := github.NewClient(&http.Client{Transport: tr}) if operator.GitHubEnterpriseUrl != "" { - c, err = c.WithEnterpriseURLs(operator.GitHubEnterpriseUrl, operator.GitHubEnterpriseUrl) - } - if err != nil { - return nil, err + newC, err := c.WithEnterpriseURLs(operator.GitHubEnterpriseUrl, operator.GitHubEnterpriseUrl) + if err != nil { + return nil, err + } + c = newC } g.clients[i] = c return g.clients[i], nil } +// fullEnterpriseApiUrl ensures the base url is in the correct format for GitHub Enterprise usage +func fullEnterpriseApiUrl(baseUrl string) string { + if !strings.HasSuffix(baseUrl, "/") { + baseUrl += "/" + } + if !strings.HasSuffix(baseUrl, "api/v3/") { + baseUrl += "api/v3/" + } + + return baseUrl +} + func getKeyFromSecretReal(ctx context.Context, keySecretVal string) ([]byte, error) { v, err := runtimevar.OpenVariable(ctx, keySecretVal) if err != nil { diff --git a/pkg/policies/security/security.go b/pkg/policies/security/security.go index 01ce20bd..aad78875 100644 --- a/pkg/policies/security/security.go +++ b/pkg/policies/security/security.go @@ -103,7 +103,7 @@ func (s Security) Check(ctx context.Context, c *github.Client, owner, if operator.GitHubEnterpriseUrl == "" { v4c = githubv4.NewClient(c.Client()) } else { - v4c = githubv4.NewEnterpriseClient(operator.GitHubEnterpriseUrl, c.Client()) + v4c = githubv4.NewEnterpriseClient(operator.GitHubEnterpriseUrl+"/api/graphql", c.Client()) } return check(ctx, c, v4c, owner, repo) } From 485f3a820285e5e869c72db47964612c19c28e8c Mon Sep 17 00:00:00 2001 From: Sebastian Bezold Date: Thu, 15 Aug 2024 16:22:19 +0200 Subject: [PATCH 3/5] docs: add hints on GHE operating specifics Signed-off-by: Sebastian Bezold --- operator.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/operator.md b/operator.md index 189428ed..b190314b 100644 --- a/operator.md +++ b/operator.md @@ -66,3 +66,16 @@ Allstar supports various operator configuration options which can be set via env | ALLSTAR_LOG_LEVEL | The minimum logging level that allstar should use when emitting logs. Acceptable values are: panic ; fatal ; error ; warn ; info ; debug ; trace | info | | NOTICE_PING_DURATION_HOURS | The duration (in hours) to wait between pinging notice actions, such as updating a GitHub issue. | 24 | +## Self-hosted GitHub Enterprise specifics + +In case you want to operate Allstar with a self-hosted GitHub Enterprise instance, you need to set the `ALLSTAR_GHE_URL` environment variable to the URL of your GitHub Enterprise instance URL. +The different API endpoints for API and upload are appended automatically. + +Example: + +Given, your GHE instance URL is "https://my-ghe.example.com", you need to set the following environment variables: + +```shell +export ALLSTAR_GHE_URL="https://my-ghe.example.com" +export GH_HOST="my-ghe.example.com" # This is somehow used within a dependency and (as of now) is unclear how to set it programmatically. +``` \ No newline at end of file From 5e1eea08b71a800d14b9d71100cbc1a819027cc4 Mon Sep 17 00:00:00 2001 From: Sebastian Bezold Date: Wed, 11 Sep 2024 10:27:50 +0200 Subject: [PATCH 4/5] chore: adapt GH_HOST env variable comment Signed-off-by: Sebastian Bezold --- .gitignore | 2 +- operator.md | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.gitignore b/.gitignore index b7d99f97..e53bdb4b 100644 --- a/.gitignore +++ b/.gitignore @@ -2,4 +2,4 @@ cmd/allstar/allstar *.pem .vscode allstar.ref -.idea/ \ No newline at end of file +.idea/ diff --git a/operator.md b/operator.md index b190314b..3db2c965 100644 --- a/operator.md +++ b/operator.md @@ -77,5 +77,5 @@ Given, your GHE instance URL is "https://my-ghe.example.com", you need to set th ```shell export ALLSTAR_GHE_URL="https://my-ghe.example.com" -export GH_HOST="my-ghe.example.com" # This is somehow used within a dependency and (as of now) is unclear how to set it programmatically. -``` \ No newline at end of file +export GH_HOST="my-ghe.example.com" # This is used by the Scorecard dependency. Might result in errors if not set. +``` From 5a663670fbea4ad961a0f1bc2aa90d67b8423086 Mon Sep 17 00:00:00 2001 From: Sebastian Bezold Date: Wed, 11 Sep 2024 10:45:54 +0200 Subject: [PATCH 5/5] fix: handle errors when creating gh client transport Signed-off-by: Sebastian Bezold --- pkg/ghclients/ghclients.go | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/pkg/ghclients/ghclients.go b/pkg/ghclients/ghclients.go index f1b7a4eb..49c37ec9 100644 --- a/pkg/ghclients/ghclients.go +++ b/pkg/ghclients/ghclients.go @@ -94,12 +94,20 @@ func (g *GHClients) Get(i int64) (*github.Client, error) { var tr http.RoundTripper if i == 0 { - appTransport, _ := ghinstallationNewAppsTransport(ctr, operator.AppID, g.key) + appTransport, err := ghinstallationNewAppsTransport(ctr, operator.AppID, g.key) + if err != nil { + return nil, err + } // other than clien.WithEnterpriseUrls, setting the BaseUrl plainly, we need to ensure the /api/v3 ending - appTransport.BaseURL = fullEnterpriseApiUrl(operator.GitHubEnterpriseUrl) + if operator.GitHubEnterpriseUrl != "" { + appTransport.BaseURL = fullEnterpriseApiUrl(operator.GitHubEnterpriseUrl) + } tr = appTransport } else { - ghiTransport, _ := ghinstallationNew(ctr, operator.AppID, i, g.key) + ghiTransport, err := ghinstallationNew(ctr, operator.AppID, i, g.key) + if err != nil { + return nil, err + } if operator.GitHubEnterpriseUrl != "" { ghiTransport.BaseURL = fullEnterpriseApiUrl(operator.GitHubEnterpriseUrl) }