From ee59992fbe81ac188cecc9300ecb3c2a7ebf2463 Mon Sep 17 00:00:00 2001 From: Zaki Shaikh Date: Mon, 10 Feb 2025 11:57:41 +0530 Subject: [PATCH] Fix GitOps Command Issue on Pushed Commit by Unautorized User Issue: when an unautorized user sends GitOps comment on a pushed commit, PAC is triggering CI since access check is done only for pull_request event in verifyRepoAndUser func of controller. Solution: added a check for push event and Ops comment event type in verifyRepoAndUser func. https://issues.redhat.com/browse/SRVKP-7110 Signed-off-by: Zaki Shaikh --- pkg/pipelineascode/match.go | 44 ++++++++++++++++------- pkg/pipelineascode/pipelineascode_test.go | 18 ++++++++++ 2 files changed, 50 insertions(+), 12 deletions(-) diff --git a/pkg/pipelineascode/match.go b/pkg/pipelineascode/match.go index 14d397969..c6171bd9a 100644 --- a/pkg/pipelineascode/match.go +++ b/pkg/pipelineascode/match.go @@ -132,11 +132,31 @@ is that what you want? make sure you use -n when generating the secret, eg: echo return repo, err } + // Verify whether the sender of the GitOps command (e.g., /test) has the appropriate permissions to + // trigger CI on the repository, as any user is able to comment on a pushed commit in open-source repositories. + if p.event.TriggerTarget == triggertype.Push && opscomments.IsAnyOpsEventType(p.event.EventType) { + status := provider.StatusOpts{ + Status: CompletedStatus, + Title: "Permission denied", + Conclusion: failureConclusion, + DetailsURL: p.event.URL, + } + if allowed, err := p.checkAccessOrErrror(ctx, repo, status, "by GitOps comment on push commit"); !allowed { + return nil, err + } + } + // Check if the submitter is allowed to run this. // on push we don't need to check the policy since the user has pushed to the repo so it has access to it. // on comment we skip it for now, we are going to check later on if p.event.TriggerTarget != triggertype.Push && p.event.EventType != opscomments.NoOpsCommentEventType.String() { - if allowed, err := p.checkAccessOrErrror(ctx, repo, "via "+p.event.TriggerTarget.String()); !allowed { + status := provider.StatusOpts{ + Status: queuedStatus, + Title: "Pending approval, waiting for an /ok-to-test", + Conclusion: pendingConclusion, + DetailsURL: p.event.URL, + } + if allowed, err := p.checkAccessOrErrror(ctx, repo, status, "via "+p.event.TriggerTarget.String()); !allowed { return nil, err } } @@ -245,7 +265,13 @@ func (p *PacRun) getPipelineRunsFromRepo(ctx context.Context, repo *v1alpha1.Rep // if the event is a comment event, but we don't have any match from the keys.OnComment then do the ACL checks again // we skipped previously so we can get the match from the event to the pipelineruns if p.event.EventType == opscomments.NoOpsCommentEventType.String() || p.event.EventType == opscomments.OnCommentEventType.String() { - if allowed, err := p.checkAccessOrErrror(ctx, repo, "by gitops comment"); !allowed { + status := provider.StatusOpts{ + Status: queuedStatus, + Title: "Pending approval, waiting for an /ok-to-test", + Conclusion: pendingConclusion, + DetailsURL: p.event.URL, + } + if allowed, err := p.checkAccessOrErrror(ctx, repo, status, "by GitOps comment on push commit"); !allowed { return nil, err } } @@ -370,7 +396,7 @@ func (p *PacRun) checkNeedUpdate(_ string) (string, bool) { return "", false } -func (p *PacRun) checkAccessOrErrror(ctx context.Context, repo *v1alpha1.Repository, viamsg string) (bool, error) { +func (p *PacRun) checkAccessOrErrror(ctx context.Context, repo *v1alpha1.Repository, status provider.StatusOpts, viamsg string) (bool, error) { allowed, err := p.vcx.IsAllowed(ctx, p.event) if err != nil { return false, err @@ -378,18 +404,12 @@ func (p *PacRun) checkAccessOrErrror(ctx context.Context, repo *v1alpha1.Reposit if allowed { return true, nil } - msg := fmt.Sprintf("User %s is not allowed to trigger CI %s on this repo.", p.event.Sender, viamsg) + msg := fmt.Sprintf("User %s is not allowed to trigger CI %s in this repo.", p.event.Sender, viamsg) if p.event.AccountID != "" { - msg = fmt.Sprintf("User: %s AccountID: %s is not allowed to trigger CI %s on this repo.", p.event.Sender, p.event.AccountID, viamsg) + msg = fmt.Sprintf("User: %s AccountID: %s is not allowed to trigger CI %s in this repo.", p.event.Sender, p.event.AccountID, viamsg) } p.eventEmitter.EmitMessage(repo, zap.InfoLevel, "RepositoryPermissionDenied", msg) - status := provider.StatusOpts{ - Status: queuedStatus, - Title: "Pending approval, needs /ok-to-test", - Conclusion: pendingConclusion, - Text: msg, - DetailsURL: p.event.URL, - } + status.Text = msg if err := p.vcx.CreateStatus(ctx, p.event, status); err != nil { return false, fmt.Errorf("failed to run create status, user is not allowed to run the CI:: %w", err) } diff --git a/pkg/pipelineascode/pipelineascode_test.go b/pkg/pipelineascode/pipelineascode_test.go index 8707c98a7..bead914ea 100644 --- a/pkg/pipelineascode/pipelineascode_test.go +++ b/pkg/pipelineascode/pipelineascode_test.go @@ -465,6 +465,24 @@ func TestRun(t *testing.T) { finalStatusText: "PipelineRun has no taskruns", expectedNumberofCleanups: 10, }, + { + name: "Do not allow unauthorized user to run CI on pushed commit", + runevent: info.Event{ + SHA: "principale", + Organization: "organizationes", + Repository: "lagaffe", + URL: "https://service/documentation", + HeadBranch: "main", + BaseBranch: "main", + Sender: "fantasio", + EventType: "test-all-comment", + TriggerTarget: "push", + }, + tektondir: "testdata/push_branch", + finalStatus: "failure", + finalStatusText: "User fantasio is not allowed to trigger CI by GitOps comment on push commit in this repo.", + skipReplyingOrgPublicMembers: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) {