From 254d1d32d09d61ece09d11cb9f52abc1fd702521 Mon Sep 17 00:00:00 2001 From: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Date: Fri, 14 Jul 2023 13:03:48 -0400 Subject: [PATCH] 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> --- server/application/application.go | 11 ++----- server/application/application_test.go | 43 +++++++++++++++++++++++--- 2 files changed, 41 insertions(+), 13 deletions(-) diff --git a/server/application/application.go b/server/application/application.go index 0a82be5f2f35c..4bf9a858ef4db 100644 --- 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 index 2dcefc121dfca..3bc4903e9d2dc 100644 --- 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)