Skip to content

Commit

Permalink
Merge pull request from GHSA-jhqp-vf4w-rpwq
Browse files Browse the repository at this point in the history
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>

defer instead of multiple close calls

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>

oops

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>

don't count jsonnet against max

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>

fix codegen

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>

add caveat about 300x ratio

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>

fix versions

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>

fix tests/lint

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
  • Loading branch information
crenshaw-dev committed Jun 21, 2022
1 parent 45ddd05 commit a92a153
Show file tree
Hide file tree
Showing 26 changed files with 707 additions and 114 deletions.
25 changes: 16 additions & 9 deletions cmd/argocd-repo-server/commands/argocd_repo_server.go
Expand Up @@ -13,6 +13,7 @@ import (
log "github.com/sirupsen/logrus"
"github.com/spf13/cobra"
"google.golang.org/grpc/health/grpc_health_v1"
"k8s.io/apimachinery/pkg/api/resource"

cmdutil "github.com/argoproj/argo-cd/v2/cmd/util"
"github.com/argoproj/argo-cd/v2/common"
Expand Down Expand Up @@ -63,14 +64,15 @@ func getPauseGenerationOnFailureForRequests() int {

func NewCommand() *cobra.Command {
var (
parallelismLimit int64
listenPort int
metricsPort int
cacheSrc func() (*reposervercache.Cache, error)
tlsConfigCustomizer tls.ConfigCustomizer
tlsConfigCustomizerSrc func() (tls.ConfigCustomizer, error)
redisClient *redis.Client
disableTLS bool
parallelismLimit int64
listenPort int
metricsPort int
cacheSrc func() (*reposervercache.Cache, error)
tlsConfigCustomizer tls.ConfigCustomizer
tlsConfigCustomizerSrc func() (tls.ConfigCustomizer, error)
redisClient *redis.Client
disableTLS bool
maxCombinedDirectoryManifestsSize string
)
var command = cobra.Command{
Use: cliName,
Expand All @@ -90,13 +92,17 @@ func NewCommand() *cobra.Command {
cache, err := cacheSrc()
errors.CheckError(err)

maxCombinedDirectoryManifestsQuantity, err := resource.ParseQuantity(maxCombinedDirectoryManifestsSize)
errors.CheckError(err)

metricsServer := metrics.NewMetricsServer()
cacheutil.CollectMetrics(redisClient, metricsServer)
server, err := reposerver.NewServer(metricsServer, cache, tlsConfigCustomizer, repository.RepoServerInitConstants{
ParallelismLimit: parallelismLimit,
ParallelismLimit: parallelismLimit,
PauseGenerationAfterFailedGenerationAttempts: getPauseGenerationAfterFailedGenerationAttempts(),
PauseGenerationOnFailureForMinutes: getPauseGenerationOnFailureForMinutes(),
PauseGenerationOnFailureForRequests: getPauseGenerationOnFailureForRequests(),
MaxCombinedDirectoryManifestsSize: maxCombinedDirectoryManifestsQuantity,
})
errors.CheckError(err)

Expand Down Expand Up @@ -160,6 +166,7 @@ func NewCommand() *cobra.Command {
command.Flags().IntVar(&listenPort, "port", common.DefaultPortRepoServer, "Listen on given port for incoming connections")
command.Flags().IntVar(&metricsPort, "metrics-port", common.DefaultPortRepoServerMetrics, "Start metrics server on given port")
command.Flags().BoolVar(&disableTLS, "disable-tls", env.ParseBoolFromEnv("ARGOCD_REPO_SERVER_DISABLE_TLS", false), "Disable TLS on the gRPC endpoint")
command.Flags().StringVar(&maxCombinedDirectoryManifestsSize, "max-combined-directory-manifests-size", env.StringFromEnv("ARGOCD_REPO_SERVER_MAX_COMBINED_DIRECTORY_MANIFESTS_SIZE", "10M"), "Max combined size of manifest files in a directory-type Application")

tlsConfigCustomizerSrc = tls.AddTLSFlagsToCmd(&command)
cacheSrc = reposervercache.AddCacheFlagsToCmd(&command, func(client *redis.Client) {
Expand Down
3 changes: 2 additions & 1 deletion cmd/argocd/commands/app.go
Expand Up @@ -25,6 +25,7 @@ import (
"github.com/spf13/cobra"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
"k8s.io/apimachinery/pkg/api/resource"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/types"
Expand Down Expand Up @@ -756,7 +757,7 @@ func getLocalObjectsString(app *argoappv1.Application, local, localRepoRoot, app
KustomizeOptions: kustomizeOptions,
KubeVersion: kubeVersion,
Plugins: configManagementPlugins,
}, true)
}, true, resource.MustParse("0"))
errors.CheckError(err)

return res.Manifests
Expand Down
6 changes: 5 additions & 1 deletion docs/operator-manual/argocd-cmd-params-cm.yaml
Expand Up @@ -103,4 +103,8 @@ data:
reposerver.repo.cache.expiration: "24h0m0s"
# Cache expiration default (default 24h0m0s)
reposerver.default.cache.expiration: "24h0m0s"

# Max combined manifest file size for a single directory-type Application. In-memory manifest representation may be as
# much as 300x the manifest file size. Limit this to stay within the memory limits of the repo-server while allowing
# for 300x memory expansion and N Applications running at the same time.
# (example 10M max * 300 expansion * 10 Apps = 30G max theoretical memory usage).
reposerver.max.combined.directory.manifests.size: '10M'
41 changes: 41 additions & 0 deletions docs/operator-manual/security.md
Expand Up @@ -196,3 +196,44 @@ Payloads from webhook events are considered untrusted. Argo CD only examines the
the involved applications of the webhook event (e.g. which repo was modified), then refreshes
the related application for reconciliation. This refresh is the same refresh which occurs regularly
at three minute intervals, just fast-tracked by the webhook event.

## Limiting Directory App Memory Usage

> >2.2.10, 2.1.16, >2.3.5
Directory-type Applications (those whose source is raw JSON or YAML files) can consume significant
[repo-server](architecture.md#repository-server) memory, depending on the size and structure of the YAML files.

To avoid over-using memory in the repo-server (potentially causing a crash and denial of service), set the
`reposerver.max.combined.directory.manifests.size` config option in [argocd-cmd-params-cm](argocd-cmd-params-cm.yaml).

This option limits the combined size of all JSON or YAML files in an individual app. Note that the in-memory
representation of a manifest may be as much as 300x the size of the manifest on disk. Also note that the limit is per
Application. If manifests are generated for multiple applications at once, memory usage will be higher.

**Example:**

Suppose your repo-server has a 10G memory limit, and you have ten Applications which use raw JSON or YAML files. To
calculate the max safe combined file size per Application, divide 10G by 300 * 10 Apps (300 being the worst-case memory
growth factor for the manifests).

```
10G / 300 * 10 = 3M
```

So a reasonably safe configuration for this setup would be a 3M limit per app.

```yaml
apiVersion: v1
kind: ConfigMap
metadata:
name: argocd-cmd-params-cm
data:
reposerver.max.combined.directory.manifests.size: '3M'
```

The 300x ratio assumes a maliciously-crafted manifest file. If you only want to protect against accidental excessive
memory use, it is probably safe to use a smaller ratio.

Keep in mind that if a malicious user can create additional Applications, they can increase the total memory usage.
Grant [App creation privileges](rbac.md) carefully.
6 changes: 6 additions & 0 deletions manifests/base/repo-server/argocd-repo-server-deployment.yaml
Expand Up @@ -98,6 +98,12 @@ spec:
name: argocd-cmd-params-cm
key: reposerver.default.cache.expiration
optional: true
- name: ARGOCD_REPO_SERVER_MAX_COMBINED_DIRECTORY_MANIFESTS_SIZE
valueFrom:
configMapKeyRef:
name: argocd-cmd-params-cm
key: reposerver.max.combined.directory.manifests.size
optional: true
- name: HELM_CACHE_HOME
value: /helm-working-dir
- name: HELM_CONFIG_HOME
Expand Down
6 changes: 6 additions & 0 deletions manifests/core-install.yaml
Expand Up @@ -2984,6 +2984,12 @@ spec:
key: reposerver.default.cache.expiration
name: argocd-cmd-params-cm
optional: true
- name: ARGOCD_REPO_SERVER_MAX_COMBINED_DIRECTORY_MANIFESTS_SIZE
valueFrom:
configMapKeyRef:
key: reposerver.max.combined.directory.manifests.size
name: argocd-cmd-params-cm
optional: true
- name: HELM_CACHE_HOME
value: /helm-working-dir
- name: HELM_CONFIG_HOME
Expand Down
6 changes: 6 additions & 0 deletions manifests/ha/install.yaml
Expand Up @@ -3897,6 +3897,12 @@ spec:
key: reposerver.default.cache.expiration
name: argocd-cmd-params-cm
optional: true
- name: ARGOCD_REPO_SERVER_MAX_COMBINED_DIRECTORY_MANIFESTS_SIZE
valueFrom:
configMapKeyRef:
key: reposerver.max.combined.directory.manifests.size
name: argocd-cmd-params-cm
optional: true
- name: HELM_CACHE_HOME
value: /helm-working-dir
- name: HELM_CONFIG_HOME
Expand Down
6 changes: 6 additions & 0 deletions manifests/ha/namespace-install.yaml
Expand Up @@ -1284,6 +1284,12 @@ spec:
key: reposerver.default.cache.expiration
name: argocd-cmd-params-cm
optional: true
- name: ARGOCD_REPO_SERVER_MAX_COMBINED_DIRECTORY_MANIFESTS_SIZE
valueFrom:
configMapKeyRef:
key: reposerver.max.combined.directory.manifests.size
name: argocd-cmd-params-cm
optional: true
- name: HELM_CACHE_HOME
value: /helm-working-dir
- name: HELM_CONFIG_HOME
Expand Down
6 changes: 6 additions & 0 deletions manifests/install.yaml
Expand Up @@ -3226,6 +3226,12 @@ spec:
key: reposerver.default.cache.expiration
name: argocd-cmd-params-cm
optional: true
- name: ARGOCD_REPO_SERVER_MAX_COMBINED_DIRECTORY_MANIFESTS_SIZE
valueFrom:
configMapKeyRef:
key: reposerver.max.combined.directory.manifests.size
name: argocd-cmd-params-cm
optional: true
- name: HELM_CACHE_HOME
value: /helm-working-dir
- name: HELM_CONFIG_HOME
Expand Down
6 changes: 6 additions & 0 deletions manifests/namespace-install.yaml
Expand Up @@ -613,6 +613,12 @@ spec:
key: reposerver.default.cache.expiration
name: argocd-cmd-params-cm
optional: true
- name: ARGOCD_REPO_SERVER_MAX_COMBINED_DIRECTORY_MANIFESTS_SIZE
valueFrom:
configMapKeyRef:
key: reposerver.max.combined.directory.manifests.size
name: argocd-cmd-params-cm
optional: true
- name: HELM_CACHE_HOME
value: /helm-working-dir
- name: HELM_CONFIG_HOME
Expand Down

0 comments on commit a92a153

Please sign in to comment.