From 9d8ed353aa70cfabd450a1065103daf5b4cb4324 Mon Sep 17 00:00:00 2001 From: Bolaji Olajide <25608335+BolajiOlajide@users.noreply.github.com> Date: Tue, 28 Nov 2023 00:38:12 +0100 Subject: [PATCH] code-search: add configuration for rejecting unverified commits (#58385) --- internal/batches/reconciler/BUILD.bazel | 1 + internal/batches/reconciler/executor.go | 14 ++++++++++++-- internal/batches/sources/testing/fake.go | 13 +++++++------ internal/conf/BUILD.bazel | 1 + internal/conf/batch_changes.go | 11 +++++++++++ schema/schema.go | 3 +++ schema/site.schema.json | 8 ++++++++ 7 files changed, 43 insertions(+), 8 deletions(-) create mode 100644 internal/conf/batch_changes.go diff --git a/internal/batches/reconciler/BUILD.bazel b/internal/batches/reconciler/BUILD.bazel index af8b31150ed..77f1137a821 100644 --- a/internal/batches/reconciler/BUILD.bazel +++ b/internal/batches/reconciler/BUILD.bazel @@ -18,6 +18,7 @@ go_library( "//internal/batches/store", "//internal/batches/types", "//internal/batches/webhooks", + "//internal/conf", "//internal/database", "//internal/errcode", "//internal/gitserver", diff --git a/internal/batches/reconciler/executor.go b/internal/batches/reconciler/executor.go index a81646489ff..c11785da13b 100644 --- a/internal/batches/reconciler/executor.go +++ b/internal/batches/reconciler/executor.go @@ -18,6 +18,7 @@ import ( "github.com/sourcegraph/sourcegraph/internal/batches/store" btypes "github.com/sourcegraph/sourcegraph/internal/batches/types" "github.com/sourcegraph/sourcegraph/internal/batches/webhooks" + "github.com/sourcegraph/sourcegraph/internal/conf" "github.com/sourcegraph/sourcegraph/internal/database" "github.com/sourcegraph/sourcegraph/internal/errcode" "github.com/sourcegraph/sourcegraph/internal/gitserver" @@ -644,6 +645,8 @@ func (e *executor) pushCommit(ctx context.Context, opts protocol.CreateCommitFro } func (e *executor) runAfterCommit(ctx context.Context, css sources.ChangesetSource, resp *protocol.CreateCommitFromPatchResponse, remoteRepo *types.Repo, opts protocol.CreateCommitFromPatchRequest) (err error) { + rejectUnverifiedCommit := conf.RejectUnverifiedCommit() + // If we're pushing to a GitHub code host, we should check if a GitHub App is // configured for Batch Changes to sign commits on this code host with. if _, ok := css.(*sources.GitHubSource); ok { @@ -652,11 +655,15 @@ func (e *executor) runAfterCommit(ctx context.Context, css sources.ChangesetSour if err != nil { switch err { case sources.ErrNoGitHubAppConfigured: + if rejectUnverifiedCommit { + return errors.New("no GitHub App configured to sign commit, rejecting unverified commit") + } // If we didn't find any GitHub Apps configured for this code host, it's a // noop; commit signing is not set up for this code host. - break default: - // We shouldn't block on this error, but we should still log it. + if rejectUnverifiedCommit { + return errors.Wrap(err, "failed to get GitHub App for commit verification") + } log15.Error("Failed to get GitHub App authenticated ChangesetSource", "err", err) } } else { @@ -684,6 +691,9 @@ func (e *executor) runAfterCommit(ctx context.Context, css sources.ChangesetSour return errors.Wrap(err, "failed to update changeset with commit verification") } } else { + if rejectUnverifiedCommit { + return errors.Wrap(err, "commit created with GitHub App was not signed, rejecting unverified commit") + } log15.Warn("Commit created with GitHub App was not signed", "changeset", e.ch.ID, "commit", newCommit.SHA) } } diff --git a/internal/batches/sources/testing/fake.go b/internal/batches/sources/testing/fake.go index bcaefcb8057..ee2618a8d4b 100644 --- a/internal/batches/sources/testing/fake.go +++ b/internal/batches/sources/testing/fake.go @@ -217,6 +217,7 @@ func (s *FakeChangesetSource) ExternalServices() types.ExternalServices { return types.ExternalServices{s.Svc} } + func (s *FakeChangesetSource) LoadChangeset(ctx context.Context, c *sources.Changeset) error { s.LoadChangesetCalled = true @@ -239,12 +240,6 @@ func (s *FakeChangesetSource) LoadChangeset(ctx context.Context, c *sources.Chan return nil } -type noReposErr struct{ name string } - -func (e noReposErr) Error() string { - return "no " + e.name + " repository set on Changeset" -} - func (s *FakeChangesetSource) CloseChangeset(ctx context.Context, c *sources.Changeset) error { s.CloseChangesetCalled = true @@ -324,3 +319,9 @@ func (s *FakeChangesetSource) BuildCommitOpts(repo *types.Repo, _ *btypes.Change s.BuildCommitOptsCalled = true return sources.BuildCommitOptsCommon(repo, spec, cfg) } + +type noReposErr struct{ name string } + +func (e noReposErr) Error() string { + return "no " + e.name + " repository set on Changeset" +} diff --git a/internal/conf/BUILD.bazel b/internal/conf/BUILD.bazel index ced5672a0c5..702921c386a 100644 --- a/internal/conf/BUILD.bazel +++ b/internal/conf/BUILD.bazel @@ -5,6 +5,7 @@ go_library( name = "conf", srcs = [ "auth.go", + "batch_changes.go", "client.go", "cody_validators.go", "computed.go", diff --git a/internal/conf/batch_changes.go b/internal/conf/batch_changes.go new file mode 100644 index 00000000000..b200d734d66 --- /dev/null +++ b/internal/conf/batch_changes.go @@ -0,0 +1,11 @@ +package conf + +// RejectUnverifiedCommit returns a boolean indicating if unverified commits in changesets +// created by a Batch Change should result in an error. +func RejectUnverifiedCommit() bool { + cfg := Get().SiteConfig().BatchChangesRejectUnverifiedCommit + if cfg == nil { + return false + } + return *cfg +} diff --git a/schema/schema.go b/schema/schema.go index 5a3b006e1d8..579c9e59cdd 100644 --- a/schema/schema.go +++ b/schema/schema.go @@ -2564,6 +2564,8 @@ type SiteConfiguration struct { BatchChangesEnabled *bool `json:"batchChanges.enabled,omitempty"` // BatchChangesEnforceForks description: When enabled, all branches created by batch changes will be pushed to forks of the original repository. BatchChangesEnforceForks bool `json:"batchChanges.enforceForks,omitempty"` + // BatchChangesRejectUnverifiedCommit description: Reject unverified commits when creating a Batch Change + BatchChangesRejectUnverifiedCommit *bool `json:"batchChanges.rejectUnverifiedCommit,omitempty"` // BatchChangesRestrictToAdmins description: When enabled, only site admins can create and apply batch changes. BatchChangesRestrictToAdmins *bool `json:"batchChanges.restrictToAdmins,omitempty"` // BatchChangesRolloutWindows description: Specifies specific windows, which can have associated rate limits, to be used when reconciling published changesets (creating or updating). All days and times are handled in UTC. @@ -2838,6 +2840,7 @@ func (v *SiteConfiguration) UnmarshalJSON(data []byte) error { delete(m, "batchChanges.disableWebhooksWarning") delete(m, "batchChanges.enabled") delete(m, "batchChanges.enforceForks") + delete(m, "batchChanges.rejectUnverifiedCommit") delete(m, "batchChanges.restrictToAdmins") delete(m, "batchChanges.rolloutWindows") delete(m, "branding") diff --git a/schema/site.schema.json b/schema/site.schema.json index 8e59f8024c8..425a903fa00 100644 --- a/schema/site.schema.json +++ b/schema/site.schema.json @@ -1968,6 +1968,14 @@ } ] }, + "batchChanges.rejectUnverifiedCommit": { + "description": "Reject unverified commits when creating a Batch Change", + "type": "boolean", + "default": false, + "!go": { + "pointer": true + } + }, "gitserver.diskUsageWarningThreshold": { "description": "Disk usage threshold at which to display warning notification. Value is a percentage.", "type": "integer",