Repository: openshift-pipelines/pipelines-as-code (go)

Issue:
allow to specify namespace target directly  in pipelinerun annotation 👾
this would be better for security.

Pull request:
allow to specify namespace target directly in pipelinerun annotation 👾
Add an another optional layer of security where PipelineRun can have an annotation to explicitely target a specific namespace. It would stilll need to have a Repository CRD created in that namespace to be able to be matched.

With this annotation there is no way a bad actor on a cluster can hijack the pipelineRun execution to a namespace they don't have access to. To use that feature you need to add this annotation :

```yaml
pipelinesascode.tekton.dev/target-namespace: "mynamespace"
```

and Pipeline as Code will only match the repository in the mynamespace Namespace instead of trying to match it from all available repository on cluster.

Closes #63

Signed-off-by: Chmouel Boudjnah <chmouel@redhat.com>


Patch:
diff --git a/README.md b/README.md
index 8a63da92..b46d594b 100644
--- a/README.md
+++ b/README.md
@@ -78,6 +78,21 @@ main into the namespace `my-pipeline-ci`
 For security reasons, the Repository CR needs to be created
 in the namespace where Tekton Pipelines associated with the source code repository would be executed.
 
+There is another optional layer of security where PipelineRun can have an
+annotation to explicitely target a specific namespace. It would stilll need to
+have a Repository CRD created in that namespace to be able to be matched.
+
+With this annotation there is no way a bad actor on a cluster can hijack the
+pipelineRun execution to a namespace they don't have access to. To use that
+feature you need to add this annotation :
+
+```yaml
+pipelinesascode.tekton.dev/target-namespace: "mynamespace"
+```
+
+and Pipeline as Code will only match the repository in the mynamespace Namespace
+instead of trying to match it from all available repository on cluster.
+
 ### Writting Tekton pipelines in `.tekton/` directory
 
 - Pipelines as Code tries to be as close to the tekton template as possible.
@@ -173,7 +188,7 @@ PipelineRun with no risk of conflicts.
 
 Everything that runs your pipelinerun and its references need to be inside the
 `.tekton/` directory or referenced via a remote task (see below on how the remote
-tasks are referenced). 
+tasks are referenced).
 
 If pipelines as code cannot resolve the referenced tasks in the `Pipeline` or `PipelineSpec` it will fails before applying the pipelinerun onto the cluster.
 
diff --git a/pkg/config/annotation_matcher.go b/pkg/config/annotation_matcher.go
index 31652df8..59a56faf 100644
--- a/pkg/config/annotation_matcher.go
+++ b/pkg/config/annotation_matcher.go
@@ -1,6 +1,7 @@
 package config
 
 import (
+	"context"
 	"errors"
 	"fmt"
 	"regexp"
@@ -15,6 +16,7 @@ import (
 const (
 	onEventAnnotation        = "on-event"
 	onTargetBranchAnnotation = "on-target-branch"
+	onTargetNamespace        = "target-namespace"
 	reValidateTag            = `^\[(.*)\]$`
 )
 
@@ -39,7 +41,7 @@ func getAnnotationValues(annotation string) ([]string, error) {
 	return splitted, nil
 }
 
-func MatchPipelinerunByAnnotation(pruns []*v1beta1.PipelineRun, cs *cli.Clients,
+func MatchPipelinerunByAnnotation(ctx context.Context, pruns []*v1beta1.PipelineRun, cs *cli.Clients,
 	runinfo *webvcs.RunInfo) (*v1beta1.PipelineRun, error) {
 	configurations := map[string]map[string]string{}
 	for _, prun := range pruns {
@@ -49,6 +51,16 @@ func MatchPipelinerunByAnnotation(pruns []*v1beta1.PipelineRun, cs *cli.Clients,
 			continue
 		}
 
+		if targetNS, ok := prun.GetObjectMeta().GetAnnotations()[pipelinesascode.
+			GroupName+"/"+onTargetNamespace]; ok {
+			configurations[prun.GetGenerateName()]["target-namespace"] = targetNS
+			repo, _ := GetRepoByCR(ctx, cs, targetNS, runinfo)
+			if repo == nil {
+				cs.Log.Warnf("could not find Repository CRD in %s while pipelineRun %s targets it", targetNS, prun.GetGenerateName())
+				continue
+			}
+		}
+
 		if targetEvent, ok := prun.GetObjectMeta().GetAnnotations()[pipelinesascode.
 			GroupName+"/"+onEventAnnotation]; ok {
 			matched, err := matchOnAnnotation(targetEvent, runinfo.EventType, false)
@@ -83,8 +95,8 @@ func MatchPipelinerunByAnnotation(pruns []*v1beta1.PipelineRun, cs *cli.Clients,
 
 	cs.Log.Info("available configuration in pipelineRuns annotations")
 	for prunname, maps := range configurations {
-		cs.Log.Infof("pipelineRun: %s, baseBranch=%s, targetEvent=%s",
-			prunname, maps["target-branch"], maps["target-event"])
+		cs.Log.Infof("pipelineRun: %s, baseBranch=%s, targetEvent=%s, targetNs=%s",
+			prunname, maps["target-branch"], maps["target-event"], maps["target-namespace"])
 	}
 
 	// TODO: more descriptive error message
diff --git a/pkg/config/repo_runinfo_matcher.go b/pkg/config/repo_runinfo_matcher.go
index c57f7061..633270fd 100644
--- a/pkg/config/repo_runinfo_matcher.go
+++ b/pkg/config/repo_runinfo_matcher.go
@@ -24,16 +24,18 @@ func branchMatch(prunBranch, baseBranch string) bool {
 	return g.Match(baseBranch)
 }
 
-func GetRepoByCR(ctx context.Context, cs *cli.Clients, runinfo *webvcs.RunInfo) (apipac.Repository, error) {
-	var repository apipac.Repository
-
-	repositories, err := cs.PipelineAsCode.PipelinesascodeV1alpha1().Repositories("").List(
+func GetRepoByCR(ctx context.Context, cs *cli.Clients, ns string, runinfo *webvcs.RunInfo) (*apipac.Repository, error) {
+	repositories, err := cs.PipelineAsCode.PipelinesascodeV1alpha1().Repositories(ns).List(
 		ctx, metav1.ListOptions{})
 	if err != nil {
-		return repository, err
+		return nil, err
 	}
-
+	matches := []string{}
 	for _, value := range repositories.Items {
+		matches = append(matches,
+			fmt.Sprintf("RepositoryValue: URL=%s, eventType=%s BaseBranch:=%s", value.Spec.URL,
+				value.Spec.EventType, value.Spec.Branch))
+
 		if value.Spec.URL == runinfo.URL &&
 			value.Spec.EventType == runinfo.EventType {
 			if value.Spec.Branch != runinfo.BaseBranch {
@@ -45,14 +47,18 @@ func GetRepoByCR(ctx context.Context, cs *cli.Clients, runinfo *webvcs.RunInfo)
 			// Disallow attempts for hijacks. If the installed CR is not configured on the
 			// Namespace the Spec is targeting then disallow it.
 			if value.Namespace != value.Spec.Namespace {
-				return repository, fmt.Errorf("repo CR %s matches but belongs to %s while it should be in %s",
+				return nil, fmt.Errorf("repo CR %s matches but belongs to %s while it should be in %s",
 					value.Name,
 					value.Namespace,
 					value.Spec.Namespace)
 			}
 
-			return value, nil
+			return &value, nil
 		}
 	}
-	return repository, nil
+	for _, value := range matches {
+		cs.Log.Debug(value)
+	}
+
+	return nil, nil
 }
diff --git a/pkg/pipelineascode/pipelineascode.go b/pkg/pipelineascode/pipelineascode.go
index 7834b3db..a45ccb1b 100644
--- a/pkg/pipelineascode/pipelineascode.go
+++ b/pkg/pipelineascode/pipelineascode.go
@@ -70,12 +70,14 @@ func Run(ctx context.Context, cs *cli.Clients, k8int cli.KubeInteractionIntf, ru
 	}
 
 	// Match the Event to a Repository Resource,
-	// TODO: we need to be able to force a Namespace from the configuration as annotation as an extra layer of security for // hijacking
-	repo, err := config.GetRepoByCR(ctx, cs, runinfo)
+	// We are going to match on targetNamespace annotation later on in
+	// `MatchPipelinerunByAnnotation`
+	repo, err := config.GetRepoByCR(ctx, cs, "", runinfo)
 	if err != nil {
 		return err
 	}
-	if repo.Spec.Namespace == "" {
+
+	if repo == nil {
 		msg := fmt.Sprintf("Could not find a namespace match for %s/%s on target-branch:%s event-type: %s", runinfo.Owner, runinfo.Repository, runinfo.BaseBranch, runinfo.EventType)
 		if runinfo.EventType == "pull_request" || runinfo.TriggerTarget == "issue-recheck" {
 			err = createStatus(ctx, cs, runinfo, "completed", "skipped", msg, "https://tenor.com/search/sad-cat-gifs", true)
@@ -144,7 +146,7 @@ func Run(ctx context.Context, cs *cli.Clients, k8int cli.KubeInteractionIntf, ru
 	}
 
 	// Match the pipelinerun with annotation
-	pipelineRun, err := config.MatchPipelinerunByAnnotation(pipelineRuns, cs, runinfo)
+	pipelineRun, err := config.MatchPipelinerunByAnnotation(ctx, pipelineRuns, cs, runinfo)
 	if err != nil {
 		return err
 	}


Test patch:
diff --git a/pkg/config/annotation_matcher_test.go b/pkg/config/annotation_matcher_test.go
index 1f0c9478..e3d93625 100644
--- a/pkg/config/annotation_matcher_test.go
+++ b/pkg/config/annotation_matcher_test.go
@@ -1,20 +1,108 @@
 package config
 
 import (
+	"fmt"
 	"reflect"
 	"strings"
 	"testing"
 
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode"
+	"github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/cli"
+	testclient "github.com/openshift-pipelines/pipelines-as-code/pkg/test/clients"
+	testnewrepo "github.com/openshift-pipelines/pipelines-as-code/pkg/test/repository"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/webvcs"
 	tektonv1beta1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1"
 	"go.uber.org/zap"
 	zapobserver "go.uber.org/zap/zaptest/observer"
 	"gotest.tools/v3/assert"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	rtesting "knative.dev/pkg/reconciler/testing"
 )
 
+func TestMatchPipelinerunAnnotationAndRepositories(t *testing.T) {
+	pipelineTargetNSName := "pipeline-target-ns"
+	pipelineTargetNS := &tektonv1beta1.PipelineRun{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: pipelineTargetNSName,
+			Annotations: map[string]string{
+				pipelinesascode.GroupName + "/" + onTargetNamespace:        targetNamespace,
+				pipelinesascode.GroupName + "/" + onEventAnnotation:        "[pull_request]",
+				pipelinesascode.GroupName + "/" + onTargetBranchAnnotation: fmt.Sprintf("[%s]", mainBranch),
+			},
+		},
+	}
+
+	type args struct {
+		pruns   []*tektonv1beta1.PipelineRun
+		runinfo *webvcs.RunInfo
+		data    testclient.Data
+	}
+	tests := []struct {
+		name, wantRepoName, wantLog string
+		args                        args
+		wantErr                     bool
+	}{
+		{
+			name:         "match a repository with target NS",
+			wantRepoName: pipelineTargetNSName,
+			args: args{
+				pruns:   []*tektonv1beta1.PipelineRun{pipelineTargetNS},
+				runinfo: &webvcs.RunInfo{URL: targetURL, EventType: "pull_request", BaseBranch: mainBranch},
+				data: testclient.Data{
+					Repositories: []*v1alpha1.Repository{
+						testnewrepo.NewRepo("test-good", targetURL, mainBranch, targetNamespace, targetNamespace, "pull_request"),
+					},
+				},
+			},
+		},
+		{
+			name:    "no match a repository with target NS",
+			wantErr: true,
+			wantLog: "could not find Repository CRD",
+			args: args{
+				pruns:   []*tektonv1beta1.PipelineRun{pipelineTargetNS},
+				runinfo: &webvcs.RunInfo{URL: targetURL, EventType: "pull_request", BaseBranch: mainBranch},
+				data: testclient.Data{
+					Repositories: []*v1alpha1.Repository{
+						testnewrepo.NewRepo("test-good", targetURL, mainBranch, "otherNS", "otherNS", "pull_request"),
+					},
+				},
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ctx, _ := rtesting.SetupFakeContext(t)
+			cs, _ := testclient.SeedTestData(t, ctx, tt.args.data)
+			observer, log := zapobserver.New(zap.InfoLevel)
+			logger := zap.New(observer).Sugar()
+			client := &cli.Clients{PipelineAsCode: cs.PipelineAsCode, Log: logger}
+			got, err := MatchPipelinerunByAnnotation(ctx,
+				tt.args.pruns,
+				client, tt.args.runinfo)
+
+			if tt.wantErr && err == nil {
+				t.Error("We should have get an error")
+			}
+
+			if !tt.wantErr && err != nil {
+				t.Errorf("We should have not get an error %s", err)
+			}
+
+			if tt.wantRepoName != "" {
+				assert.Assert(t, tt.wantRepoName == got.GetName())
+			}
+			if tt.wantLog != "" {
+				logmsg := log.TakeAll()
+				assert.Assert(t, len(logmsg) > 0, "We didn't get any log message")
+				assert.Assert(t, strings.Contains(logmsg[0].Message, tt.wantLog), logmsg[0].Message, tt.wantLog)
+			}
+		})
+	}
+}
+
 func TestMatchPipelinerunByAnnotation(t *testing.T) {
 	pipelineGood := &tektonv1beta1.PipelineRun{
 		ObjectMeta: metav1.ObjectMeta{
@@ -214,7 +302,8 @@ func TestMatchPipelinerunByAnnotation(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got, err := MatchPipelinerunByAnnotation(tt.args.pruns, cs, tt.args.runinfo)
+			ctx, _ := rtesting.SetupFakeContext(t)
+			got, err := MatchPipelinerunByAnnotation(ctx, tt.args.pruns, cs, tt.args.runinfo)
 			if (err != nil) != tt.wantErr {
 				t.Errorf("MatchPipelinerunByAnnotation() error = %v, wantErr %v", err, tt.wantErr)
 				return
diff --git a/pkg/config/repo_runinfo_matcher_test.go b/pkg/config/repo_runinfo_matcher_test.go
index e9b50aec..d860a7de 100644
--- a/pkg/config/repo_runinfo_matcher_test.go
+++ b/pkg/config/repo_runinfo_matcher_test.go
@@ -8,6 +8,8 @@ import (
 	testclient "github.com/openshift-pipelines/pipelines-as-code/pkg/test/clients"
 	testnewrepo "github.com/openshift-pipelines/pipelines-as-code/pkg/test/repository"
 	"github.com/openshift-pipelines/pipelines-as-code/pkg/webvcs"
+	"go.uber.org/zap"
+	zapobserver "go.uber.org/zap/zaptest/observer"
 	"gotest.tools/v3/assert"
 	rtesting "knative.dev/pkg/reconciler/testing"
 )
@@ -133,16 +135,25 @@ func Test_getRepoByCR(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+			if tt.name != "test-nomatch-event-type" {
+				return
+			}
 			ctx, _ := rtesting.SetupFakeContext(t)
 			cs, _ := testclient.SeedTestData(t, ctx, tt.args.data)
-			client := &cli.Clients{PipelineAsCode: cs.PipelineAsCode}
-			got, err := GetRepoByCR(ctx, client, tt.args.runinfo)
+			observer, _ := zapobserver.New(zap.InfoLevel)
+			logger := zap.New(observer).Sugar()
+			client := &cli.Clients{PipelineAsCode: cs.PipelineAsCode, Log: logger}
+			got, err := GetRepoByCR(ctx, client, "", tt.args.runinfo)
 			if err == nil && tt.wantErr {
 				assert.NilError(t, err, "GetRepoByCR() error = %v, wantErr %v", err, tt.wantErr)
 			}
-			if tt.wantTargetNS == "" && got.Spec.Namespace != "" {
+			if tt.wantTargetNS == "" && got != nil {
+				t.Errorf("GetRepoByCR() got = '%v', want '%v'", got.Spec.Namespace, tt.wantTargetNS)
+			}
+			if tt.wantTargetNS != "" && got == nil {
 				t.Errorf("GetRepoByCR() got = '%v', want '%v'", got.Spec.Namespace, tt.wantTargetNS)
 			}
+
 			if tt.wantTargetNS != "" && tt.wantTargetNS != got.Spec.Namespace {
 				t.Errorf("GetRepoByCR() got = '%v', want '%v'", got.Spec.Namespace, tt.wantTargetNS)
 			}