From 14f681e3ee7c38731943b98f92277e88a3db109d Mon Sep 17 00:00:00 2001 From: pasha-codefresh Date: Thu, 28 Mar 2024 14:38:03 +0200 Subject: [PATCH] Merge pull request from GHSA-jhwx-mhww-rgc3 * sec: limit helm index max size Signed-off-by: pashakostohrys * sec: limit helm index max size Signed-off-by: pashakostohrys * feat: fix tests and linter Signed-off-by: pashakostohrys --------- Signed-off-by: pashakostohrys --- .../commands/argocd_repo_server.go | 6 ++++++ reposerver/repository/repository.go | 7 ++++--- reposerver/repository/repository_test.go | 2 +- util/helm/client.go | 10 +++++----- util/helm/client_test.go | 14 ++++++++++---- util/helm/mocks/Client.go | 2 +- 6 files changed, 27 insertions(+), 14 deletions(-) diff --git a/cmd/argocd-repo-server/commands/argocd_repo_server.go b/cmd/argocd-repo-server/commands/argocd_repo_server.go index 69358d2a91efd..05f8fa9114e12 100644 --- a/cmd/argocd-repo-server/commands/argocd_repo_server.go +++ b/cmd/argocd-repo-server/commands/argocd_repo_server.go @@ -66,6 +66,7 @@ func NewCommand() *cobra.Command { streamedManifestMaxTarSize string streamedManifestMaxExtractedSize string helmManifestMaxExtractedSize string + helmRegistryMaxIndexSize string disableManifestMaxExtractedSize bool ) var command = cobra.Command{ @@ -108,6 +109,9 @@ func NewCommand() *cobra.Command { helmManifestMaxExtractedSizeQuantity, err := resource.ParseQuantity(helmManifestMaxExtractedSize) errors.CheckError(err) + helmRegistryMaxIndexSizeQuantity, err := resource.ParseQuantity(helmRegistryMaxIndexSize) + errors.CheckError(err) + askPassServer := askpass.NewServer() metricsServer := metrics.NewMetricsServer() cacheutil.CollectMetrics(redisClient, metricsServer) @@ -123,6 +127,7 @@ func NewCommand() *cobra.Command { StreamedManifestMaxExtractedSize: streamedManifestMaxExtractedSizeQuantity.ToDec().Value(), StreamedManifestMaxTarSize: streamedManifestMaxTarSizeQuantity.ToDec().Value(), HelmManifestMaxExtractedSize: helmManifestMaxExtractedSizeQuantity.ToDec().Value(), + HelmRegistryMaxIndexSize: helmRegistryMaxIndexSizeQuantity.ToDec().Value(), }, askPassServer) errors.CheckError(err) @@ -204,6 +209,7 @@ func NewCommand() *cobra.Command { command.Flags().StringVar(&streamedManifestMaxTarSize, "streamed-manifest-max-tar-size", env.StringFromEnv("ARGOCD_REPO_SERVER_STREAMED_MANIFEST_MAX_TAR_SIZE", "100M"), "Maximum size of streamed manifest archives") command.Flags().StringVar(&streamedManifestMaxExtractedSize, "streamed-manifest-max-extracted-size", env.StringFromEnv("ARGOCD_REPO_SERVER_STREAMED_MANIFEST_MAX_EXTRACTED_SIZE", "1G"), "Maximum size of streamed manifest archives when extracted") command.Flags().StringVar(&helmManifestMaxExtractedSize, "helm-manifest-max-extracted-size", env.StringFromEnv("ARGOCD_REPO_SERVER_HELM_MANIFEST_MAX_EXTRACTED_SIZE", "1G"), "Maximum size of helm manifest archives when extracted") + command.Flags().StringVar(&helmRegistryMaxIndexSize, "helm-registry-max-index-size", env.StringFromEnv("ARGOCD_REPO_SERVER_HELM_MANIFEST_MAX_INDEX_SIZE", "1G"), "Maximum size of registry index file") command.Flags().BoolVar(&disableManifestMaxExtractedSize, "disable-helm-manifest-max-extracted-size", env.ParseBoolFromEnv("ARGOCD_REPO_SERVER_DISABLE_HELM_MANIFEST_MAX_EXTRACTED_SIZE", false), "Disable maximum size of helm manifest archives when extracted") tlsConfigCustomizerSrc = tls.AddTLSFlagsToCmd(&command) cacheSrc = reposervercache.AddCacheFlagsToCmd(&command, func(client *redis.Client) { diff --git a/reposerver/repository/repository.go b/reposerver/repository/repository.go index 12e00fb855999..be89af0401393 100644 --- a/reposerver/repository/repository.go +++ b/reposerver/repository/repository.go @@ -107,6 +107,7 @@ type RepoServerInitConstants struct { StreamedManifestMaxExtractedSize int64 StreamedManifestMaxTarSize int64 HelmManifestMaxExtractedSize int64 + HelmRegistryMaxIndexSize int64 DisableHelmManifestMaxExtractedSize bool } @@ -2345,7 +2346,7 @@ func (s *Service) newHelmClientResolveRevision(repo *v1alpha1.Repository, revisi return helmClient, version.String(), nil } - index, err := helmClient.GetIndex(noRevisionCache) + index, err := helmClient.GetIndex(noRevisionCache, s.initConstants.HelmRegistryMaxIndexSize) if err != nil { return nil, "", err } @@ -2423,7 +2424,7 @@ func checkoutRevision(gitClient git.Client, revision string, submoduleEnabled bo } func (s *Service) GetHelmCharts(ctx context.Context, q *apiclient.HelmChartsRequest) (*apiclient.HelmChartsResponse, error) { - index, err := s.newHelmClient(q.Repo.Repo, q.Repo.GetHelmCreds(), q.Repo.EnableOCI, q.Repo.Proxy, helm.WithChartPaths(s.chartPaths)).GetIndex(true) + index, err := s.newHelmClient(q.Repo.Repo, q.Repo.GetHelmCreds(), q.Repo.EnableOCI, q.Repo.Proxy, helm.WithChartPaths(s.chartPaths)).GetIndex(true, s.initConstants.HelmRegistryMaxIndexSize) if err != nil { return nil, err } @@ -2458,7 +2459,7 @@ func (s *Service) TestRepository(ctx context.Context, q *apiclient.TestRepositor _, err := helm.NewClient(repo.Repo, repo.GetHelmCreds(), repo.EnableOCI, repo.Proxy).TestHelmOCI() return err } else { - _, err := helm.NewClient(repo.Repo, repo.GetHelmCreds(), repo.EnableOCI, repo.Proxy).GetIndex(false) + _, err := helm.NewClient(repo.Repo, repo.GetHelmCreds(), repo.EnableOCI, repo.Proxy).GetIndex(false, s.initConstants.HelmRegistryMaxIndexSize) return err } }, diff --git a/reposerver/repository/repository_test.go b/reposerver/repository/repository_test.go index 6c0c27ca58fde..225d68dc45235 100644 --- a/reposerver/repository/repository_test.go +++ b/reposerver/repository/repository_test.go @@ -113,7 +113,7 @@ func newServiceWithMocks(t *testing.T, root string, signed bool) (*Service, *git chart := "my-chart" oobChart := "out-of-bounds-chart" version := "1.1.0" - helmClient.On("GetIndex", mock.AnythingOfType("bool")).Return(&helm.Index{Entries: map[string]helm.Entries{ + helmClient.On("GetIndex", mock.AnythingOfType("bool"), mock.Anything).Return(&helm.Index{Entries: map[string]helm.Entries{ chart: {{Version: "1.0.0"}, {Version: version}}, oobChart: {{Version: "1.0.0"}, {Version: version}}, }}, nil) diff --git a/util/helm/client.go b/util/helm/client.go index 75bd30d1fea13..8b99cd67c6904 100644 --- a/util/helm/client.go +++ b/util/helm/client.go @@ -56,7 +56,7 @@ type indexCache interface { type Client interface { CleanChartCache(chart string, version string) error ExtractChart(chart string, version string, passCredentials bool, manifestMaxExtractedSize int64, disableManifestMaxExtractedSize bool) (string, argoio.Closer, error) - GetIndex(noCache bool) (*Index, error) + GetIndex(noCache bool, maxIndexSize int64) (*Index, error) GetTags(chart string, noCache bool) (*TagsList, error) TestHelmOCI() (bool, error) } @@ -230,7 +230,7 @@ func (c *nativeHelmChart) ExtractChart(chart string, version string, passCredent }), nil } -func (c *nativeHelmChart) GetIndex(noCache bool) (*Index, error) { +func (c *nativeHelmChart) GetIndex(noCache bool, maxIndexSize int64) (*Index, error) { indexLock.Lock(c.repoURL) defer indexLock.Unlock(c.repoURL) @@ -244,7 +244,7 @@ func (c *nativeHelmChart) GetIndex(noCache bool) (*Index, error) { if len(data) == 0 { start := time.Now() var err error - data, err = c.loadRepoIndex() + data, err = c.loadRepoIndex(maxIndexSize) if err != nil { return nil, err } @@ -297,7 +297,7 @@ func (c *nativeHelmChart) TestHelmOCI() (bool, error) { return true, nil } -func (c *nativeHelmChart) loadRepoIndex() ([]byte, error) { +func (c *nativeHelmChart) loadRepoIndex(maxIndexSize int64) ([]byte, error) { indexURL, err := getIndexURL(c.repoURL) if err != nil { return nil, err @@ -332,7 +332,7 @@ func (c *nativeHelmChart) loadRepoIndex() ([]byte, error) { if resp.StatusCode != http.StatusOK { return nil, errors.New("failed to get index: " + resp.Status) } - return io.ReadAll(resp.Body) + return io.ReadAll(io.LimitReader(resp.Body, maxIndexSize)) } func newTLSConfig(creds Creds) (*tls.Config, error) { diff --git a/util/helm/client_test.go b/util/helm/client_test.go index 6fba279df07d0..ad613ca3bd7eb 100644 --- a/util/helm/client_test.go +++ b/util/helm/client_test.go @@ -37,12 +37,12 @@ func (f *fakeIndexCache) GetHelmIndex(_ string, indexData *[]byte) error { func TestIndex(t *testing.T) { t.Run("Invalid", func(t *testing.T) { client := NewClient("", Creds{}, false, "") - _, err := client.GetIndex(false) + _, err := client.GetIndex(false, 10000) assert.Error(t, err) }) t.Run("Stable", func(t *testing.T) { client := NewClient("https://argoproj.github.io/argo-helm", Creds{}, false, "") - index, err := client.GetIndex(false) + index, err := client.GetIndex(false, 10000) assert.NoError(t, err) assert.NotNil(t, index) }) @@ -51,7 +51,7 @@ func TestIndex(t *testing.T) { Username: "my-password", Password: "my-username", }, false, "") - index, err := client.GetIndex(false) + index, err := client.GetIndex(false, 10000) assert.NoError(t, err) assert.NotNil(t, index) }) @@ -63,12 +63,18 @@ func TestIndex(t *testing.T) { require.NoError(t, err) client := NewClient("https://argoproj.github.io/argo-helm", Creds{}, false, "", WithIndexCache(&fakeIndexCache{data: data.Bytes()})) - index, err := client.GetIndex(false) + index, err := client.GetIndex(false, 10000) assert.NoError(t, err) assert.Equal(t, fakeIndex, *index) }) + t.Run("Limited", func(t *testing.T) { + client := NewClient("https://argoproj.github.io/argo-helm", Creds{}, false, "") + _, err := client.GetIndex(false, 100) + + assert.ErrorContains(t, err, "unexpected end of stream") + }) } func Test_nativeHelmChart_ExtractChart(t *testing.T) { diff --git a/util/helm/mocks/Client.go b/util/helm/mocks/Client.go index 6dc25e4affd0b..0acae845a3d33 100644 --- a/util/helm/mocks/Client.go +++ b/util/helm/mocks/Client.go @@ -59,7 +59,7 @@ func (_m *Client) ExtractChart(chart string, version string, passCredentials boo } // GetIndex provides a mock function with given fields: noCache -func (_m *Client) GetIndex(noCache bool) (*helm.Index, error) { +func (_m *Client) GetIndex(noCache bool, maxIndexSize int64) (*helm.Index, error) { ret := _m.Called(noCache) var r0 *helm.Index