fix(security): don't allow app enumeration via RevisionChartDetails (#…

…14512) * fix(security): don't allow app enumeration via RevisionChartDetails Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> * better app name Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> --------- Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
argoproj · Jul 14, 2023 · 9bf5e50 · 9bf5e50
1 parent 1ee4389
commit 9bf5e50
Show file tree

Hide file tree

Showing 2 changed files with 41 additions and 13 deletions.
diff --git a/server/application/application.go b/server/application/application.go
@@ -1371,17 +1371,12 @@ func (s *Server) RevisionMetadata(ctx context.Context, q *application.RevisionMe
 
 // RevisionChartDetails returns the helm chart metadata, as fetched from the reposerver
 func (s *Server) RevisionChartDetails(ctx context.Context, q *application.RevisionMetadataQuery) (*appv1.ChartDetails, error) {
-	appName := q.GetName()
-	appNs := s.appNamespaceOrDefault(q.GetAppNamespace())
-	a, err := s.appLister.Applications(appNs).Get(appName)
+	a, err := s.getApplicationEnforceRBACInformer(ctx, rbacpolicy.ActionGet, q.GetAppNamespace(), q.GetName())
 	if err != nil {
-		return nil, fmt.Errorf("error getting app by name: %w", err)
-	}
-	if err := s.enf.EnforceErr(ctx.Value("claims"), rbacpolicy.ResourceApplications, rbacpolicy.ActionGet, a.RBACName(s.ns)); err != nil {
-		return nil, fmt.Errorf("error enforcing claims: %w", err)
+		return nil, err
 	}
 	if a.Spec.Source.Chart == "" {
-		return nil, fmt.Errorf("no chart found for application: %v", appName)
+		return nil, fmt.Errorf("no chart found for application: %v", a.QualifiedName())
 	}
 	repo, err := s.db.GetRepository(ctx, a.Spec.Source.RepoURL)
 	if err != nil {

diff --git a/server/application/application_test.go b/server/application/application_test.go
@@ -88,14 +88,14 @@ func fakeAppList() *apiclient.AppList {
 	}
 }
 
-func fakeResolveRevesionResponse() *apiclient.ResolveRevisionResponse {
+func fakeResolveRevisionResponse() *apiclient.ResolveRevisionResponse {
 	return &apiclient.ResolveRevisionResponse{
 		Revision:          "f9ba9e98119bf8c1176fbd65dbae26a71d044add",
 		AmbiguousRevision: "HEAD (f9ba9e98119bf8c1176fbd65dbae26a71d044add)",
 	}
 }
 
-func fakeResolveRevesionResponseHelm() *apiclient.ResolveRevisionResponse {
+func fakeResolveRevisionResponseHelm() *apiclient.ResolveRevisionResponse {
 	return &apiclient.ResolveRevisionResponse{
 		Revision:          "0.7.*",
 		AmbiguousRevision: "0.7.* (0.7.2)",
@@ -113,11 +113,12 @@ func fakeRepoServerClient(isHelm bool) *mocks.RepoServerServiceClient {
 	mockWithFilesClient.On("Send", mock.Anything).Return(nil)
 	mockWithFilesClient.On("CloseAndRecv").Return(&apiclient.ManifestResponse{}, nil)
 	mockRepoServiceClient.On("GenerateManifestWithFiles", mock.Anything, mock.Anything).Return(mockWithFilesClient, nil)
+	mockRepoServiceClient.On("GetRevisionChartDetails", mock.Anything, mock.Anything).Return(&appsv1.ChartDetails{}, nil)
 
 	if isHelm {
-		mockRepoServiceClient.On("ResolveRevision", mock.Anything, mock.Anything).Return(fakeResolveRevesionResponseHelm(), nil)
+		mockRepoServiceClient.On("ResolveRevision", mock.Anything, mock.Anything).Return(fakeResolveRevisionResponseHelm(), nil)
 	} else {
-		mockRepoServiceClient.On("ResolveRevision", mock.Anything, mock.Anything).Return(fakeResolveRevesionResponse(), nil)
+		mockRepoServiceClient.On("ResolveRevision", mock.Anything, mock.Anything).Return(fakeResolveRevisionResponse(), nil)
 	}
 
 	return &mockRepoServiceClient
@@ -722,8 +723,31 @@ func TestNoAppEnumeration(t *testing.T) {
 			},
 		}
 	})
+	testHelmApp := newTestApp(func(app *appsv1.Application) {
+		app.Name = "test-helm"
+		app.Spec.Source.Path = ""
+		app.Spec.Source.Chart = "test"
+		app.Status.Resources = []appsv1.ResourceStatus{
+			{
+				Group:     deployment.GroupVersionKind().Group,
+				Kind:      deployment.GroupVersionKind().Kind,
+				Version:   deployment.GroupVersionKind().Version,
+				Name:      deployment.Name,
+				Namespace: deployment.Namespace,
+				Status:    "Synced",
+			},
+		}
+		app.Status.History = []appsv1.RevisionHistory{
+			{
+				ID: 0,
+				Source: appsv1.ApplicationSource{
+					TargetRevision: "something-old",
+				},
+			},
+		}
+	})
 	testDeployment := kube.MustToUnstructured(&deployment)
-	appServer := newTestAppServerWithEnforcerConfigure(f, t, testApp, testDeployment)
+	appServer := newTestAppServerWithEnforcerConfigure(f, t, testApp, testHelmApp, testDeployment)
 
 	noRoleCtx := context.Background()
 	// nolint:staticcheck
@@ -833,6 +857,15 @@ func TestNoAppEnumeration(t *testing.T) {
 		assert.Equal(t, permissionDeniedErr.Error(), err.Error(), "error message must be _only_ the permission error, to avoid leaking information about app existence")
 	})
 
+	t.Run("RevisionChartDetails", func(t *testing.T) {
+		_, err := appServer.RevisionChartDetails(adminCtx, &application.RevisionMetadataQuery{Name: pointer.String("test-helm")})
+		assert.NoError(t, err)
+		_, err = appServer.RevisionChartDetails(noRoleCtx, &application.RevisionMetadataQuery{Name: pointer.String("test-helm")})
+		assert.Equal(t, permissionDeniedErr.Error(), err.Error(), "error message must be _only_ the permission error, to avoid leaking information about app existence")
+		_, err = appServer.RevisionChartDetails(adminCtx, &application.RevisionMetadataQuery{Name: pointer.String("doest-not-exist")})
+		assert.Equal(t, permissionDeniedErr.Error(), err.Error(), "error message must be _only_ the permission error, to avoid leaking information about app existence")
+	})
+
 	t.Run("ManagedResources", func(t *testing.T) {
 		_, err := appServer.ManagedResources(adminCtx, &application.ResourcesQuery{ApplicationName: pointer.String("test")})
 		assert.NoError(t, err)