feat(appset): Restrict scm provider urls (argoproj#14286)

* 9353: Restrict scm provider urls Signed-off-by: gmuselli <[email protected]> * 9353: Enforce restriction Signed-off-by: gmuselli <[email protected]> * 9353: Fix after review Signed-off-by: gmuselli <[email protected]> * 9353: Remove comment Signed-off-by: gmuselli <[email protected]> * 9353: Fix units tests Signed-off-by: Geoffrey Muselli <[email protected]> * 9353: Code review, update comment Signed-off-by: gmuselli <[email protected]> * 9353: Code review, update comment 2 Signed-off-by: gmuselli <[email protected]> * 9353: Remove doc issues Signed-off-by: gmuselli <[email protected]> * 9353: Fix e2e Signed-off-by: gmuselli <[email protected]> * 9353: Fix e2e goTemplate Signed-off-by: gmuselli <[email protected]> * 9353: Fix e2e pullRequestGenerator Signed-off-by: gmuselli <[email protected]> --------- Signed-off-by: gmuselli <[email protected]> Signed-off-by: Geoffrey Muselli <[email protected]>
LinAnt · Jul 27, 2023 · 433ba36 · 433ba36
1 parent f1607fe
commit 433ba36
Show file tree

Hide file tree

Showing 16 changed files with 520 additions and 26 deletions.
diff --git a/Makefile b/Makefile
@@ -460,6 +460,7 @@ start-e2e-local: mod-vendor-local dep-ui-local cli-local
 	BIN_MODE=$(ARGOCD_BIN_MODE) \
 	ARGOCD_APPLICATION_NAMESPACES=argocd-e2e-external \
 	ARGOCD_APPLICATIONSET_CONTROLLER_NAMESPACES=argocd-e2e-external \
+	ARGOCD_APPLICATIONSET_CONTROLLER_ALLOWED_SCM_PROVIDERS=http://127.0.0.1:8341,http://127.0.0.1:8342,http://127.0.0.1:8343,http://127.0.0.1:8344 \
 	ARGOCD_E2E_TEST=true \
 		goreman -f $(ARGOCD_PROCFILE) start ${ARGOCD_START}
 

diff --git a/applicationset/controllers/requeue_after_test.go b/applicationset/controllers/requeue_after_test.go
@@ -60,9 +60,9 @@ func TestRequeueAfter(t *testing.T) {
 		"List":                    generators.NewListGenerator(),
 		"Clusters":                generators.NewClusterGenerator(k8sClient, ctx, appClientset, "argocd"),
 		"Git":                     generators.NewGitGenerator(mockServer),
-		"SCMProvider":             generators.NewSCMProviderGenerator(fake.NewClientBuilder().WithObjects(&corev1.Secret{}).Build(), generators.SCMAuthProviders{}, ""),
+		"SCMProvider":             generators.NewSCMProviderGenerator(fake.NewClientBuilder().WithObjects(&corev1.Secret{}).Build(), generators.SCMAuthProviders{}, "", []string{""}),
 		"ClusterDecisionResource": generators.NewDuckTypeGenerator(ctx, fakeDynClient, appClientset, "argocd"),
-		"PullRequest":             generators.NewPullRequestGenerator(k8sClient, generators.SCMAuthProviders{}, ""),
+		"PullRequest":             generators.NewPullRequestGenerator(k8sClient, generators.SCMAuthProviders{}, "", []string{""}),
 	}
 
 	nestedGenerators := map[string]generators.Generator{

diff --git a/applicationset/generators/pull_request.go b/applicationset/generators/pull_request.go
@@ -26,13 +26,15 @@ type PullRequestGenerator struct {
 	selectServiceProviderFunc func(context.Context, *argoprojiov1alpha1.PullRequestGenerator, *argoprojiov1alpha1.ApplicationSet) (pullrequest.PullRequestService, error)
 	auth                      SCMAuthProviders
 	scmRootCAPath             string
+	allowedSCMProviders       []string
 }
 
-func NewPullRequestGenerator(client client.Client, auth SCMAuthProviders, scmRootCAPath string) Generator {
+func NewPullRequestGenerator(client client.Client, auth SCMAuthProviders, scmRootCAPath string, allowedScmProviders []string) Generator {
 	g := &PullRequestGenerator{
-		client:        client,
-		auth:          auth,
-		scmRootCAPath: scmRootCAPath,
+		client:              client,
+		auth:                auth,
+		scmRootCAPath:       scmRootCAPath,
+		allowedSCMProviders: allowedScmProviders,
 	}
 	g.selectServiceProviderFunc = g.selectServiceProvider
 	return g
@@ -120,10 +122,16 @@ func (g *PullRequestGenerator) GenerateParams(appSetGenerator *argoprojiov1alpha
 // selectServiceProvider selects the provider to get pull requests from the configuration
 func (g *PullRequestGenerator) selectServiceProvider(ctx context.Context, generatorConfig *argoprojiov1alpha1.PullRequestGenerator, applicationSetInfo *argoprojiov1alpha1.ApplicationSet) (pullrequest.PullRequestService, error) {
 	if generatorConfig.Github != nil {
+		if !ScmProviderAllowed(applicationSetInfo, generatorConfig.Github.API, g.allowedSCMProviders) {
+			return nil, fmt.Errorf("scm provider not allowed: %s", generatorConfig.Github.API)
+		}
 		return g.github(ctx, generatorConfig.Github, applicationSetInfo)
 	}
 	if generatorConfig.GitLab != nil {
 		providerConfig := generatorConfig.GitLab
+		if !ScmProviderAllowed(applicationSetInfo, providerConfig.API, g.allowedSCMProviders) {
+			return nil, fmt.Errorf("scm provider not allowed: %s", providerConfig.API)
+		}
 		token, err := g.getSecretRef(ctx, providerConfig.TokenRef, applicationSetInfo.Namespace)
 		if err != nil {
 			return nil, fmt.Errorf("error fetching Secret token: %v", err)
@@ -132,6 +140,9 @@ func (g *PullRequestGenerator) selectServiceProvider(ctx context.Context, genera
 	}
 	if generatorConfig.Gitea != nil {
 		providerConfig := generatorConfig.Gitea
+		if !ScmProviderAllowed(applicationSetInfo, providerConfig.API, g.allowedSCMProviders) {
+			return nil, fmt.Errorf("scm provider not allowed: %s", generatorConfig.Gitea.API)
+		}
 		token, err := g.getSecretRef(ctx, providerConfig.TokenRef, applicationSetInfo.Namespace)
 		if err != nil {
 			return nil, fmt.Errorf("error fetching Secret token: %v", err)
@@ -140,6 +151,9 @@ func (g *PullRequestGenerator) selectServiceProvider(ctx context.Context, genera
 	}
 	if generatorConfig.BitbucketServer != nil {
 		providerConfig := generatorConfig.BitbucketServer
+		if !ScmProviderAllowed(applicationSetInfo, providerConfig.API, g.allowedSCMProviders) {
+			return nil, fmt.Errorf("scm provider not allowed: %s", providerConfig.API)
+		}
 		if providerConfig.BasicAuth != nil {
 			password, err := g.getSecretRef(ctx, providerConfig.BasicAuth.PasswordRef, applicationSetInfo.Namespace)
 			if err != nil {

diff --git a/applicationset/generators/pull_request_test.go b/applicationset/generators/pull_request_test.go
@@ -273,3 +273,80 @@ func TestPullRequestGetSecretRef(t *testing.T) {
 		})
 	}
 }
+
+func TestAllowedSCMProviderPullRequest(t *testing.T) {
+	cases := []struct {
+		name           string
+		providerConfig *argoprojiov1alpha1.PullRequestGenerator
+		expectedError  string
+	}{
+		{
+			name: "Error Github",
+			providerConfig: &argoprojiov1alpha1.PullRequestGenerator{
+				Github: &argoprojiov1alpha1.PullRequestGeneratorGithub{
+					API: "https://myservice.mynamespace.svc.cluster.local",
+				},
+			},
+			expectedError: "failed to select pull request service provider: scm provider not allowed: https://myservice.mynamespace.svc.cluster.local",
+		},
+		{
+			name: "Error Gitlab",
+			providerConfig: &argoprojiov1alpha1.PullRequestGenerator{
+				GitLab: &argoprojiov1alpha1.PullRequestGeneratorGitLab{
+					API: "https://myservice.mynamespace.svc.cluster.local",
+				},
+			},
+			expectedError: "failed to select pull request service provider: scm provider not allowed: https://myservice.mynamespace.svc.cluster.local",
+		},
+		{
+			name: "Error Gitea",
+			providerConfig: &argoprojiov1alpha1.PullRequestGenerator{
+				Gitea: &argoprojiov1alpha1.PullRequestGeneratorGitea{
+					API: "https://myservice.mynamespace.svc.cluster.local",
+				},
+			},
+			expectedError: "failed to select pull request service provider: scm provider not allowed: https://myservice.mynamespace.svc.cluster.local",
+		},
+		{
+			name: "Error Bitbucket",
+			providerConfig: &argoprojiov1alpha1.PullRequestGenerator{
+				BitbucketServer: &argoprojiov1alpha1.PullRequestGeneratorBitbucketServer{
+					API: "https://myservice.mynamespace.svc.cluster.local",
+				},
+			},
+			expectedError: "failed to select pull request service provider: scm provider not allowed: https://myservice.mynamespace.svc.cluster.local",
+		},
+	}
+
+	for _, testCase := range cases {
+		testCaseCopy := testCase
+
+		t.Run(testCaseCopy.name, func(t *testing.T) {
+			t.Parallel()
+
+			pullRequestGenerator := NewPullRequestGenerator(nil, SCMAuthProviders{}, "", []string{
+				"github.myorg.com",
+				"gitlab.myorg.com",
+				"gitea.myorg.com",
+				"bitbucket.myorg.com",
+				"azuredevops.myorg.com",
+			})
+
+			applicationSetInfo := argoprojiov1alpha1.ApplicationSet{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "set",
+				},
+				Spec: argoprojiov1alpha1.ApplicationSetSpec{
+					Generators: []argoprojiov1alpha1.ApplicationSetGenerator{{
+						PullRequest: testCaseCopy.providerConfig,
+					}},
+				},
+			}
+
+			_, err := pullRequestGenerator.GenerateParams(&applicationSetInfo.Spec.Generators[0], &applicationSetInfo)
+
+			assert.Error(t, err, "Must return an error")
+			assert.Equal(t, testCaseCopy.expectedError, err.Error())
+		})
+	}
+}
diff --git a/applicationset/generators/scm_provider.go b/applicationset/generators/scm_provider.go
@@ -9,9 +9,12 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
+	log "github.com/sirupsen/logrus"
+
 	"github.com/argoproj/argo-cd/v2/applicationset/services/github_app_auth"
 	"github.com/argoproj/argo-cd/v2/applicationset/services/scm_provider"
 	"github.com/argoproj/argo-cd/v2/applicationset/utils"
+	"github.com/argoproj/argo-cd/v2/common"
 	argoprojiov1alpha1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
 )
 
@@ -26,18 +29,20 @@ type SCMProviderGenerator struct {
 	// Testing hooks.
 	overrideProvider scm_provider.SCMProviderService
 	SCMAuthProviders
-	scmRootCAPath string
+	scmRootCAPath       string
+	allowedSCMProviders []string
 }
 
 type SCMAuthProviders struct {
 	GitHubApps github_app_auth.Credentials
 }
 
-func NewSCMProviderGenerator(client client.Client, providers SCMAuthProviders, scmRootCAPath string) Generator {
+func NewSCMProviderGenerator(client client.Client, providers SCMAuthProviders, scmRootCAPath string, allowedSCMProviders []string) Generator {
 	return &SCMProviderGenerator{
-		client:           client,
-		SCMAuthProviders: providers,
-		scmRootCAPath:    scmRootCAPath,
+		client:              client,
+		SCMAuthProviders:    providers,
+		scmRootCAPath:       scmRootCAPath,
+		allowedSCMProviders: allowedSCMProviders,
 	}
 }
 
@@ -60,6 +65,26 @@ func (g *SCMProviderGenerator) GetTemplate(appSetGenerator *argoprojiov1alpha1.A
 	return &appSetGenerator.SCMProvider.Template
 }
 
+func ScmProviderAllowed(applicationSetInfo *argoprojiov1alpha1.ApplicationSet, url string, allowedScmProviders []string) bool {
+	if url == "" || len(allowedScmProviders) == 0 {
+		return true
+	}
+
+	for _, allowedScmProvider := range allowedScmProviders {
+		if url == allowedScmProvider {
+			return true
+		}
+	}
+
+	log.WithFields(log.Fields{
+		common.SecurityField: common.SecurityMedium,
+		"applicationset":     applicationSetInfo.Name,
+		"appSetNamespace":    applicationSetInfo.Namespace,
+	}).Debugf("attempted to use disallowed SCM %q", url)
+
+	return false
+}
+
 func (g *SCMProviderGenerator) GenerateParams(appSetGenerator *argoprojiov1alpha1.ApplicationSetGenerator, applicationSetInfo *argoprojiov1alpha1.ApplicationSet) ([]map[string]interface{}, error) {
 	if appSetGenerator == nil {
 		return nil, EmptyAppSetGeneratorError
@@ -77,12 +102,18 @@ func (g *SCMProviderGenerator) GenerateParams(appSetGenerator *argoprojiov1alpha
 	if g.overrideProvider != nil {
 		provider = g.overrideProvider
 	} else if providerConfig.Github != nil {
+		if !ScmProviderAllowed(applicationSetInfo, providerConfig.Github.API, g.allowedSCMProviders) {
+			return nil, fmt.Errorf("scm provider not allowed: %s", providerConfig.Github.API)
+		}
 		var err error
 		provider, err = g.githubProvider(ctx, providerConfig.Github, applicationSetInfo)
 		if err != nil {
 			return nil, fmt.Errorf("scm provider: %w", err)
 		}
 	} else if providerConfig.Gitlab != nil {
+		if !ScmProviderAllowed(applicationSetInfo, providerConfig.Gitlab.API, g.allowedSCMProviders) {
+			return nil, fmt.Errorf("scm provider not allowed: %s", providerConfig.Gitlab.API)
+		}
 		token, err := g.getSecretRef(ctx, providerConfig.Gitlab.TokenRef, applicationSetInfo.Namespace)
 		if err != nil {
 			return nil, fmt.Errorf("error fetching Gitlab token: %v", err)
@@ -92,6 +123,9 @@ func (g *SCMProviderGenerator) GenerateParams(appSetGenerator *argoprojiov1alpha
 			return nil, fmt.Errorf("error initializing Gitlab service: %v", err)
 		}
 	} else if providerConfig.Gitea != nil {
+		if !ScmProviderAllowed(applicationSetInfo, providerConfig.Gitea.API, g.allowedSCMProviders) {
+			return nil, fmt.Errorf("scm provider not allowed: %s", providerConfig.Gitea.API)
+		}
 		token, err := g.getSecretRef(ctx, providerConfig.Gitea.TokenRef, applicationSetInfo.Namespace)
 		if err != nil {
 			return nil, fmt.Errorf("error fetching Gitea token: %v", err)
@@ -102,6 +136,9 @@ func (g *SCMProviderGenerator) GenerateParams(appSetGenerator *argoprojiov1alpha
 		}
 	} else if providerConfig.BitbucketServer != nil {
 		providerConfig := providerConfig.BitbucketServer
+		if !ScmProviderAllowed(applicationSetInfo, providerConfig.API, g.allowedSCMProviders) {
+			return nil, fmt.Errorf("scm provider not allowed: %s", providerConfig.API)
+		}
 		var scmError error
 		if providerConfig.BasicAuth != nil {
 			password, err := g.getSecretRef(ctx, providerConfig.BasicAuth.PasswordRef, applicationSetInfo.Namespace)
@@ -116,6 +153,9 @@ func (g *SCMProviderGenerator) GenerateParams(appSetGenerator *argoprojiov1alpha
 			return nil, fmt.Errorf("error initializing Bitbucket Server service: %v", scmError)
 		}
 	} else if providerConfig.AzureDevOps != nil {
+		if !ScmProviderAllowed(applicationSetInfo, providerConfig.AzureDevOps.API, g.allowedSCMProviders) {
+			return nil, fmt.Errorf("scm provider not allowed: %s", providerConfig.AzureDevOps.API)
+		}
 		token, err := g.getSecretRef(ctx, providerConfig.AzureDevOps.AccessTokenRef, applicationSetInfo.Namespace)
 		if err != nil {
 			return nil, fmt.Errorf("error fetching Azure Devops access token: %v", err)

diff --git a/applicationset/generators/scm_provider_test.go b/applicationset/generators/scm_provider_test.go
@@ -200,3 +200,89 @@ func TestSCMProviderGenerateParams(t *testing.T) {
 		})
 	}
 }
+
+func TestAllowedSCMProvider(t *testing.T) {
+	cases := []struct {
+		name           string
+		providerConfig *argoprojiov1alpha1.SCMProviderGenerator
+		expectedError  string
+	}{
+		{
+			name: "Error Github",
+			providerConfig: &argoprojiov1alpha1.SCMProviderGenerator{
+				Github: &argoprojiov1alpha1.SCMProviderGeneratorGithub{
+					API: "https://myservice.mynamespace.svc.cluster.local",
+				},
+			},
+			expectedError: "scm provider not allowed: https://myservice.mynamespace.svc.cluster.local",
+		},
+		{
+			name: "Error Gitlab",
+			providerConfig: &argoprojiov1alpha1.SCMProviderGenerator{
+				Gitlab: &argoprojiov1alpha1.SCMProviderGeneratorGitlab{
+					API: "https://myservice.mynamespace.svc.cluster.local",
+				},
+			},
+			expectedError: "scm provider not allowed: https://myservice.mynamespace.svc.cluster.local",
+		},
+		{
+			name: "Error Gitea",
+			providerConfig: &argoprojiov1alpha1.SCMProviderGenerator{
+				Gitea: &argoprojiov1alpha1.SCMProviderGeneratorGitea{
+					API: "https://myservice.mynamespace.svc.cluster.local",
+				},
+			},
+			expectedError: "scm provider not allowed: https://myservice.mynamespace.svc.cluster.local",
+		},
+		{
+			name: "Error Bitbucket",
+			providerConfig: &argoprojiov1alpha1.SCMProviderGenerator{
+				BitbucketServer: &argoprojiov1alpha1.SCMProviderGeneratorBitbucketServer{
+					API: "https://myservice.mynamespace.svc.cluster.local",
+				},
+			},
+			expectedError: "scm provider not allowed: https://myservice.mynamespace.svc.cluster.local",
+		},
+		{
+			name: "Error AzureDevops",
+			providerConfig: &argoprojiov1alpha1.SCMProviderGenerator{
+				AzureDevOps: &argoprojiov1alpha1.SCMProviderGeneratorAzureDevOps{
+					API: "https://myservice.mynamespace.svc.cluster.local",
+				},
+			},
+			expectedError: "scm provider not allowed: https://myservice.mynamespace.svc.cluster.local",
+		},
+	}
+
+	for _, testCase := range cases {
+		testCaseCopy := testCase
+
+		t.Run(testCaseCopy.name, func(t *testing.T) {
+			t.Parallel()
+
+			scmGenerator := &SCMProviderGenerator{allowedSCMProviders: []string{
+				"github.myorg.com",
+				"gitlab.myorg.com",
+				"gitea.myorg.com",
+				"bitbucket.myorg.com",
+				"azuredevops.myorg.com",
+			}}
+
+			applicationSetInfo := argoprojiov1alpha1.ApplicationSet{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "set",
+				},
+				Spec: argoprojiov1alpha1.ApplicationSetSpec{
+					Generators: []argoprojiov1alpha1.ApplicationSetGenerator{{
+						SCMProvider: testCaseCopy.providerConfig,
+					}},
+				},
+			}
+
+			_, err := scmGenerator.GenerateParams(&applicationSetInfo.Spec.Generators[0], &applicationSetInfo)
+
+			assert.Error(t, err, "Must return an error")
+			assert.Equal(t, testCaseCopy.expectedError, err.Error())
+		})
+	}
+}