From 570d9e10280ac94ae61426bbb8c205aae1e80856 Mon Sep 17 00:00:00 2001 From: Sujeily Fonseca-Gonzalez Date: Sat, 25 Jun 2022 10:13:29 -0400 Subject: [PATCH] feat: Argo CD v2.3.5 (#52) * fix(ui): Applications page incorrectly resets to tiles view. Fixes #8702 (#8718) Signed-off-by: Yuan Tang * fix: correct jsonnet paths resolution (#8721) Signed-off-by: Alexander Matyushentsev * chore: Bump stable version of application set addon (#8744) Signed-off-by: Alexander Matyushentsev * fix: Retry checkbox unchecked unexpectedly; Sync up with YAML (#8682) (#8720) Signed-off-by: Keith Chong * Bump version to 2.3.1 * Bump version to 2.3.1 * Merge pull request from GHSA-2f5v-8r3f-8pww * fix: application resource APIs must enforce project restrictions Signed-off-by: Alexander Matyushentsev * Fix unit tests Signed-off-by: jannfis Co-authored-by: jannfis * chore: remove lint-docs CI task (#8722) (#8858) * chore: remove lint-docs CI task Signed-off-by: Alexander Matyushentsev * chore: remove not longer necessary url-allow-list Signed-off-by: Alexander Matyushentsev Co-authored-by: Alexander Matyushentsev * chore: fix imports (#8859) Signed-off-by: Michael Crenshaw * Bump version to 2.3.2 * Bump version to 2.3.2 * fix: Set QPS and burst rate for resource ops client (#8915) * fix: Set QPS and burst rate for resource ops client Signed-off-by: jannfis * fix: prevent excessive repo-server disk usage for large repos (#8845) (#8897) fix: prevent excessive repo-server disk usage for large repos (#8845) (#8897) Signed-off-by: Michael Crenshaw * fix: bump gitops engine version to v0.6.2 Signed-off-by: Alexander Matyushentsev * docs: update v2.4+ roadmap items (#8593) Signed-off-by: ishitasequeira * docs: reflect v2.3 release changes in roadmap.md (#8747) docs: reflect v2.3 release changes in roadmap.md (#8747) Signed-off-by: Alexander Matyushentsev * Bump version to 2.3.3 * Bump version to 2.3.3 * fix: Fix docs build error (#8895) * work with specific jinja version Signed-off-by: pashavictorovich * fix: fix broken monaco editor collapse icons (#8709) Signed-off-by: Alexander Matyushentsev * chore: upgrade to go 1.17.8 (#8866) (#9004) * chore: upgrade to go 1.17.8 Signed-off-by: Michael Crenshaw * chore: use 1.17 so it's always latest in the series Signed-off-by: Michael Crenshaw * fix: allow cli/ui to follow logs (#8987) (#9065) Signed-off-by: Daniel Helfand * Merge pull request from GHSA-xmg8-99r8-jc2j Signed-off-by: Michael Crenshaw Co-authored-by: Michael Crenshaw * Merge pull request from GHSA-6gcg-hp2x-q54h * fix: do not allow symlinks from directory-type applications Signed-off-by: Michael Crenshaw * chore: add new util file Signed-off-by: Michael Crenshaw * chore: lint Signed-off-by: Michael Crenshaw * chore: use t.TempDir for simpler tests Signed-off-by: Michael Crenshaw * address comments Signed-off-by: Michael Crenshaw * Merge pull request from GHSA-r642-gv9p-2wjj Signed-off-by: jannfis Co-authored-by: Michael Crenshaw Co-authored-by: Michael Crenshaw * Bump version to 2.3.4 * Bump version to 2.3.4 * test: fix ErrorContains (#9445) Signed-off-by: Michael Crenshaw * fix: missing Helm params (#9565) (#9566) * fix: missing Helm params Signed-off-by: Michael Crenshaw * use absolute paths, fix tests Signed-off-by: Michael Crenshaw * fix race in test Signed-off-by: Michael Crenshaw * chore: upgrade golangci-lint to v1.46.2 (#9448) * chore: upgrade golangci-lint to v1.46.2 Because: * Installation of golangci-lint v1.45.2 is currently broken and fails silently due to a redacted dependency (https://github.com/blizzy78/varnamelen/issues/13) This commit: * Upgrades golangci-lint to v1.46.2 Signed-off-by: Tommaso Sardelli * fix: lint Signed-off-by: Michael Crenshaw * fix: lint Signed-off-by: Tommaso Sardelli Co-authored-by: Michael Crenshaw Signed-off-by: Michael Crenshaw * fix: test race (#9469) Signed-off-by: Michael Crenshaw * chore: lint issues Signed-off-by: Michael Crenshaw * chore: update golangci-lint (#8988) * chore: update golangci-lint Signed-off-by: Michael Crenshaw * chore: remove obsolete repo-server unit test (#9559) Signed-off-by: Alexander Matyushentsev * chore: Make unit tests run on platforms other than amd64 (#8995) Signed-off-by: jannfis Co-authored-by: Michael Crenshaw Signed-off-by: Michael Crenshaw * chore: eliminate go-mpatch dependency (#9045) * chore: eliminate go-mpatch dependency Signed-off-by: Michael Crenshaw * chore: abstract out resource list function Signed-off-by: Michael Crenshaw * chore: don't exit the program in anything but the main function Signed-off-by: Michael Crenshaw * chore: better error messages Signed-off-by: Michael Crenshaw * chore: better error messages Signed-off-by: Michael Crenshaw * test: directory app manifest generation (#9503) * test: directory app manifest generation Signed-off-by: Michael Crenshaw * git doesn't support empty dirs Signed-off-by: Michael Crenshaw * Merge pull request from GHSA-h4w9-6x78-8vrj Signed-off-by: Michael Crenshaw * Merge pull request from GHSA-2m7h-86qq-fp4v Signed-off-by: Michael Crenshaw fix references Signed-off-by: Michael Crenshaw use long enough state param for oauth2 Signed-off-by: Michael Crenshaw typo Signed-off-by: Michael Crenshaw more entropy Signed-off-by: Michael Crenshaw fix test Signed-off-by: Michael Crenshaw * Merge pull request from GHSA-q4w5-4gq2-98vm Signed-off-by: Michael Crenshaw * Merge pull request from GHSA-jhqp-vf4w-rpwq Signed-off-by: Michael Crenshaw defer instead of multiple close calls Signed-off-by: Michael Crenshaw oops Signed-off-by: Michael Crenshaw don't count jsonnet against max Signed-off-by: Michael Crenshaw fix codegen Signed-off-by: Michael Crenshaw add caveat about 300x ratio Signed-off-by: Michael Crenshaw fix versions Signed-off-by: Michael Crenshaw fix tests/lint Signed-off-by: Michael Crenshaw * chore: fix docs gen Signed-off-by: Michael Crenshaw * Bump version to 2.3.5 * Bump version to 2.3.5 * docs: Changes for v2.3.5 Documented key decision factors to use Argo CD v2.3.5. Contributes to: automation-saas/automation-saas/native-AWS#1972 Signed-off-by: Sujeily Fonseca Co-authored-by: Yuan Tang Co-authored-by: Alexander Matyushentsev Co-authored-by: Keith Chong Co-authored-by: argo-bot Co-authored-by: jannfis Co-authored-by: Michael Crenshaw Co-authored-by: Ishita Sequeira <46771830+ishitasequeira@users.noreply.github.com> Co-authored-by: pasha-codefresh Co-authored-by: Daniel Helfand Co-authored-by: Tommaso Sardelli --- .github/workflows/ci-build.yaml | 8 +- .github/workflows/image.yaml | 2 +- .github/workflows/release.yaml | 2 +- .golangci.yml | 22 - CHANGES.md | 36 +- Dockerfile | 4 +- VERSION | 2 +- .../commands/argocd_repo_server.go | 25 +- .../commands/admin/project_allowlist.go | 45 +- .../commands/admin/project_allowlist_test.go | 49 +- cmd/argocd/commands/app.go | 3 +- cmd/argocd/commands/login.go | 11 +- controller/sync.go | 8 +- .../operator-manual/argocd-cmd-params-cm.yaml | 6 +- docs/operator-manual/security.md | 43 +- .../server-commands/argocd-repo-server.md | 45 +- docs/requirements.txt | 3 +- go.mod | 1 - go.sum | 2 - hack/installers/install-lint-tools.sh | 2 +- manifests/base/kustomization.yaml | 2 +- .../argocd-repo-server-deployment.yaml | 6 + manifests/core-install.yaml | 12 +- manifests/core-install/kustomization.yaml | 2 +- manifests/ha/base/kustomization.yaml | 2 +- manifests/ha/install.yaml | 18 +- manifests/ha/namespace-install.yaml | 18 +- manifests/install.yaml | 18 +- manifests/namespace-install.yaml | 18 +- pkg/apiclient/grpcproxy.go | 6 +- .../application/v1alpha1/cluster_constants.go | 10 +- reposerver/repository/repository.go | 306 ++++++--- .../repository/repository_norace_test.go | 47 -- reposerver/repository/repository_test.go | 629 +++++++++++++++++- .../repository/testdata/circular-link/a.json | 1 + .../repository/testdata/circular-link/b.json | 1 + .../testdata/in-bounds-link/app/cm.link.yaml | 1 + .../testdata/in-bounds-link/cm.yaml | 2 + .../in-bounds-values-file-link/Chart.yaml | 2 + .../in-bounds-values-file-link/values-2.yaml | 1 + .../in-bounds-values-file-link/values.yaml | 1 + .../testdata/invalid-json/invalid.json | 1 + .../testdata/irrelevant-yaml/irrelevant.yaml | 1 + .../testdata/irrelevant-yaml/relevant.yaml | 2 + .../repository/testdata/json-list/list.json | 2 + .../testdata/jsonnet-and-json/test.json | 1 + .../testdata/jsonnet-and-json/test.jsonnet | 1 + .../testdata/link-to-nowhere/nowhere.json | 1 + .../non-manifest-file/not-json-or-yaml | 0 .../out-of-bounds-link/out-of-bounds.json | 1 + .../out-of-bounds-values-file-link/Chart.yaml | 2 + .../values.yaml | 1 + .../repository/testdata/out-of-bounds.json | 0 .../repository/testdata/out-of-bounds.yaml | 1 + .../partially-valid-yaml/partially-valid.yaml | 4 + .../repository/testdata/several-files/0.json | 1 + .../repository/testdata/several-files/0.yaml | 2 + .../repository/testdata/several-files/1.json | 1 + .../repository/testdata/several-files/1.yaml | 2 + .../repository/testdata/several-files/2.json | 1 + .../repository/testdata/several-files/2.yaml | 2 + .../repository/testdata/several-files/3.json | 1 + .../repository/testdata/several-files/3.yaml | 2 + .../repository/testdata/several-files/4.json | 1 + .../repository/testdata/several-files/4.yaml | 2 + .../testdata/several-files/README.md | 1 + .../repository/testdata/valid-json/valid.json | 1 + .../testdata/values-files/Chart.yaml | 2 + .../values-files/caps-extn-values.YAML | 0 .../testdata/values-files/exclude.yaml | 1 + .../values-files/has-the-word-values.yaml | 4 + .../testdata/values-files/values.yaml | 1 + .../testdata/values-files/values.yml | 0 .../yaml-with-empty-document/has-empty.yaml | 4 + server/server.go | 3 + server/server_test.go | 393 +++++++++++ test/container/Dockerfile | 2 +- test/e2e/fixture/fixture.go | 4 +- test/e2e/selective_sync_test.go | 5 +- test/remote/Dockerfile | 2 +- .../components/application-urls.test.ts | 20 + .../components/application-urls.tsx | 35 +- ui/src/app/login/components/login.tsx | 10 +- ui/src/app/webpack.config.js | 4 + ui/src/assets/fonts.css | 5 + util/dex/dex.go | 16 +- util/dex/dex_test.go | 2 +- util/helm/helm.go | 29 +- util/helm/helm_test.go | 73 +- util/helm/testdata/dependency/Chart.lock | 9 + util/io/bytereadseeker.go | 4 +- util/io/bytereadseeker_test.go | 71 ++ util/io/files/util.go | 35 + util/io/files/util_test.go | 63 ++ util/io/path/resolved.go | 1 + util/lua/custom_actions_test.go | 7 - util/oidc/oidc.go | 24 +- util/rand/rand.go | 41 +- util/rand/rand_test.go | 18 +- util/session/sessionmanager.go | 26 +- 100 files changed, 1922 insertions(+), 449 deletions(-) delete mode 100644 .golangci.yml delete mode 100644 reposerver/repository/repository_norace_test.go create mode 120000 reposerver/repository/testdata/circular-link/a.json create mode 120000 reposerver/repository/testdata/circular-link/b.json create mode 120000 reposerver/repository/testdata/in-bounds-link/app/cm.link.yaml create mode 100644 reposerver/repository/testdata/in-bounds-link/cm.yaml create mode 100644 reposerver/repository/testdata/in-bounds-values-file-link/Chart.yaml create mode 100644 reposerver/repository/testdata/in-bounds-values-file-link/values-2.yaml create mode 120000 reposerver/repository/testdata/in-bounds-values-file-link/values.yaml create mode 100644 reposerver/repository/testdata/invalid-json/invalid.json create mode 100644 reposerver/repository/testdata/irrelevant-yaml/irrelevant.yaml create mode 100644 reposerver/repository/testdata/irrelevant-yaml/relevant.yaml create mode 100644 reposerver/repository/testdata/json-list/list.json create mode 100644 reposerver/repository/testdata/jsonnet-and-json/test.json create mode 100644 reposerver/repository/testdata/jsonnet-and-json/test.jsonnet create mode 120000 reposerver/repository/testdata/link-to-nowhere/nowhere.json create mode 100644 reposerver/repository/testdata/non-manifest-file/not-json-or-yaml create mode 120000 reposerver/repository/testdata/out-of-bounds-link/out-of-bounds.json create mode 100644 reposerver/repository/testdata/out-of-bounds-values-file-link/Chart.yaml create mode 120000 reposerver/repository/testdata/out-of-bounds-values-file-link/values.yaml create mode 100644 reposerver/repository/testdata/out-of-bounds.json create mode 100644 reposerver/repository/testdata/out-of-bounds.yaml create mode 100644 reposerver/repository/testdata/partially-valid-yaml/partially-valid.yaml create mode 100644 reposerver/repository/testdata/several-files/0.json create mode 100644 reposerver/repository/testdata/several-files/0.yaml create mode 100644 reposerver/repository/testdata/several-files/1.json create mode 100644 reposerver/repository/testdata/several-files/1.yaml create mode 100644 reposerver/repository/testdata/several-files/2.json create mode 100644 reposerver/repository/testdata/several-files/2.yaml create mode 100644 reposerver/repository/testdata/several-files/3.json create mode 100644 reposerver/repository/testdata/several-files/3.yaml create mode 100644 reposerver/repository/testdata/several-files/4.json create mode 100644 reposerver/repository/testdata/several-files/4.yaml create mode 100644 reposerver/repository/testdata/several-files/README.md create mode 100644 reposerver/repository/testdata/valid-json/valid.json create mode 100644 reposerver/repository/testdata/values-files/Chart.yaml create mode 100644 reposerver/repository/testdata/values-files/caps-extn-values.YAML create mode 100644 reposerver/repository/testdata/values-files/exclude.yaml create mode 100644 reposerver/repository/testdata/values-files/has-the-word-values.yaml create mode 100644 reposerver/repository/testdata/values-files/values.yaml create mode 100644 reposerver/repository/testdata/values-files/values.yml create mode 100644 reposerver/repository/testdata/yaml-with-empty-document/has-empty.yaml create mode 100644 ui/src/app/applications/components/application-urls.test.ts create mode 100644 util/helm/testdata/dependency/Chart.lock create mode 100644 util/io/bytereadseeker_test.go create mode 100644 util/io/files/util.go create mode 100644 util/io/files/util_test.go diff --git a/.github/workflows/ci-build.yaml b/.github/workflows/ci-build.yaml index 766c4f41f42a..e2ea3544c806 100644 --- a/.github/workflows/ci-build.yaml +++ b/.github/workflows/ci-build.yaml @@ -12,7 +12,7 @@ on: env: # Golang version to use across CI steps - GOLANG_VERSION: '1.17.6' + GOLANG_VERSION: '1.17' jobs: check-go: @@ -61,10 +61,10 @@ jobs: - name: Checkout code uses: actions/checkout@v2 - name: Run golangci-lint - uses: golangci/golangci-lint-action@v2 + uses: golangci/golangci-lint-action@v3 with: - version: v1.38.0 - args: --timeout 10m --exclude SA5011 + version: v1.46.2 + args: --timeout 10m --exclude SA5011 --verbose test-go: name: Run unit tests for Go packages diff --git a/.github/workflows/image.yaml b/.github/workflows/image.yaml index bf52f3143e67..96f01e8820fe 100644 --- a/.github/workflows/image.yaml +++ b/.github/workflows/image.yaml @@ -10,7 +10,7 @@ on: types: [ labeled, unlabeled, opened, synchronize, reopened ] env: - GOLANG_VERSION: '1.17.6' + GOLANG_VERSION: '1.17' jobs: publish: diff --git a/.github/workflows/release.yaml b/.github/workflows/release.yaml index 2f3c5ac1e3fc..33d19f6ec387 100644 --- a/.github/workflows/release.yaml +++ b/.github/workflows/release.yaml @@ -12,7 +12,7 @@ on: - '!release-v0*' env: - GOLANG_VERSION: '1.17.6' + GOLANG_VERSION: '1.17' jobs: prepare-release: diff --git a/.golangci.yml b/.golangci.yml deleted file mode 100644 index ebe2918e8d17..000000000000 --- a/.golangci.yml +++ /dev/null @@ -1,22 +0,0 @@ -run: - timeout: 2m - skip-files: - - ".*\\.pb\\.go" - skip-dirs: - - pkg/client/ - - vendor/ -linters: - enable: - - vet - - deadcode - - goimports - - varcheck - - structcheck - - ineffassign - - unconvert - - unparam -linters-settings: - goimports: - local-prefixes: github.com/argoproj/argo-cd -service: - golangci-lint-version: 1.21.0 diff --git a/CHANGES.md b/CHANGES.md index e20459c7da0f..3e2a99b91d6f 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,11 +1,39 @@ # ArgoCD Forked from: [argoproj/argo-cd](https://github.com/argoproj/argo-cd) -## v2.3.3 (Base) -Argo CD's latest stable release, as of May 13, 2022, is v2.3.3. +## v2.4.2 (Base) +Argo CD's latest stable release, as of June 25, 2022, is v2.4.2. The list of Argo CD releases can be accessed [here](https://github.com/argoproj/argo-cd/releases) + +## v2.3.5 (Fork) +Argo CD has breaking changes for plugins for v2.4.x: + +>Update plugins to use newly-prefixed environment variables +If you use plugins that depend on user-supplied environment variables, then they must be updated to be compatible with Argo CD 2.4. Here is an example of user-supplied environment variables in the plugin section of an Application spec: + +``` +apiVersion: argoproj.io/v1alpha1 +kind: Application +spec: + source: + plugin: + env: + - name: FOO + value: bar +Going forward, all user-supplied environment variables will be prefixed with ARGOCD_ENV_ before being sent to the plugin's init, generate, or discover commands. This prevents users from setting potentially-sensitive environment variables. +``` + +>If you have written a custom plugin which handles user-provided environment variables, update it to handle the new prefix. + +>If you use a third-party plugin which does not explicitly advertise Argo CD 2.4 support, it might not handle the prefixed environment variables. Open an issue with the plugin's authors and confirm support before upgrading to Argo CD 2.4. + +The above means that none of the applications will be able to use a user-defined backend service because the Argo CD Vault Plugin currently doesn't provide support to understand the prefixes. + +The [release post](https://blog.argoproj.io/breaking-changes-in-argo-cd-2-4-29e3c2ac30c9) mentions the following: + +> We'll continue publishing security patches for 2.3.x until 2.6.0 is released. + +Because of the above, we proceeded to use v2.3.5, which is the latest 2.3.x version. -## v2.3.3 (Fork) -The changes were rebased based on v2.3.3. This section details the enhancements made to Argo CD Extensions. ### Resource Customization ConfigMap Pulls in resource overrides from the resource customization `ConfigMap`. This `ConfigMap` will only exist if created by diff --git a/Dockerfile b/Dockerfile index 81a2c00d9c35..ccfdef500ec7 100644 --- a/Dockerfile +++ b/Dockerfile @@ -4,7 +4,7 @@ ARG BASE_IMAGE=docker.io/library/ubuntu:21.10 # Initial stage which pulls prepares build dependencies and CLI tooling we need for our final image # Also used as the image in CI jobs so needs all dependencies #################################################################################################### -FROM docker.io/library/golang:1.17.6 as builder +FROM docker.io/library/golang:1.17 as builder RUN echo 'deb http://deb.debian.org/debian buster-backports main' >> /etc/apt/sources.list @@ -102,7 +102,7 @@ RUN HOST_ARCH='amd64' NODE_ENV='production' NODE_ONLINE_ENV='online' NODE_OPTION #################################################################################################### # Argo CD Build stage which performs the actual build of Argo CD binaries #################################################################################################### -FROM docker.io/library/golang:1.17.6 as argocd-build +FROM docker.io/library/golang:1.17 as argocd-build WORKDIR /go/src/github.com/argoproj/argo-cd diff --git a/VERSION b/VERSION index 0bee604df761..cc6c9a491e0b 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -2.3.3 +2.3.5 diff --git a/cmd/argocd-repo-server/commands/argocd_repo_server.go b/cmd/argocd-repo-server/commands/argocd_repo_server.go index 1d3c2eaf1ae1..6483ca7bf4e0 100644 --- a/cmd/argocd-repo-server/commands/argocd_repo_server.go +++ b/cmd/argocd-repo-server/commands/argocd_repo_server.go @@ -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" @@ -68,14 +69,15 @@ func getSubmoduleEnabled() bool { 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, @@ -95,15 +97,19 @@ func NewCommand() *cobra.Command { cache, err := cacheSrc() errors.CheckError(err) + maxCombinedDirectoryManifestsQuantity, err := resource.ParseQuantity(maxCombinedDirectoryManifestsSize) + errors.CheckError(err) + askPassServer := askpass.NewServer() 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(), SubmoduleEnabled: getSubmoduleEnabled(), + MaxCombinedDirectoryManifestsSize: maxCombinedDirectoryManifestsQuantity, }, askPassServer) errors.CheckError(err) @@ -168,6 +174,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) { diff --git a/cmd/argocd/commands/admin/project_allowlist.go b/cmd/argocd/commands/admin/project_allowlist.go index 2d9dd85b363f..0860e62d0b77 100644 --- a/cmd/argocd/commands/admin/project_allowlist.go +++ b/cmd/argocd/commands/admin/project_allowlist.go @@ -2,6 +2,7 @@ package admin import ( "bufio" + "fmt" "io" "io/ioutil" "os" @@ -63,7 +64,10 @@ func NewProjectAllowListGenCommand() *cobra.Command { }() } - globalProj := generateProjectAllowList(clientConfig, clusterRoleFileName, projName) + resourceList, err := getResourceList(clientConfig) + errors.CheckError(err) + globalProj, err := generateProjectAllowList(resourceList, clusterRoleFileName, projName) + errors.CheckError(err) yamlBytes, err := yaml.Marshal(globalProj) errors.CheckError(err) @@ -78,23 +82,38 @@ func NewProjectAllowListGenCommand() *cobra.Command { return command } -func generateProjectAllowList(clientConfig clientcmd.ClientConfig, clusterRoleFileName string, projName string) v1alpha1.AppProject { +func getResourceList(clientConfig clientcmd.ClientConfig) ([]*metav1.APIResourceList, error) { + config, err := clientConfig.ClientConfig() + if err != nil { + return nil, fmt.Errorf("error while creating client config: %s", err) + } + disco, err := discovery.NewDiscoveryClientForConfig(config) + if err != nil { + return nil, fmt.Errorf("error while creating discovery client: %s", err) + } + serverResources, err := disco.ServerPreferredResources() + if err != nil { + return nil, fmt.Errorf("error while getting server resources: %s", err) + } + return serverResources, nil +} + +func generateProjectAllowList(serverResources []*metav1.APIResourceList, clusterRoleFileName string, projName string) (*v1alpha1.AppProject, error) { yamlBytes, err := ioutil.ReadFile(clusterRoleFileName) - errors.CheckError(err) + if err != nil { + return nil, fmt.Errorf("error reading cluster role file: %s", err) + } var obj unstructured.Unstructured err = yaml.Unmarshal(yamlBytes, &obj) - errors.CheckError(err) + if err != nil { + return nil, fmt.Errorf("error unmarshalling cluster role file yaml: %s", err) + } clusterRole := &rbacv1.ClusterRole{} err = scheme.Scheme.Convert(&obj, clusterRole, nil) - errors.CheckError(err) - - config, err := clientConfig.ClientConfig() - errors.CheckError(err) - disco, err := discovery.NewDiscoveryClientForConfig(config) - errors.CheckError(err) - serverResources, err := disco.ServerPreferredResources() - errors.CheckError(err) + if err != nil { + return nil, fmt.Errorf("error converting cluster role yaml into ClusterRole struct: %s", err) + } resourceList := make([]metav1.GroupKind, 0) for _, rule := range clusterRole.Rules { @@ -140,5 +159,5 @@ func generateProjectAllowList(clientConfig clientcmd.ClientConfig, clusterRoleFi Spec: v1alpha1.AppProjectSpec{}, } globalProj.Spec.NamespaceResourceWhitelist = resourceList - return globalProj + return &globalProj, nil } diff --git a/cmd/argocd/commands/admin/project_allowlist_test.go b/cmd/argocd/commands/admin/project_allowlist_test.go index dea4d29297b9..c4634fb9310c 100644 --- a/cmd/argocd/commands/admin/project_allowlist_test.go +++ b/cmd/argocd/commands/admin/project_allowlist_test.go @@ -1,57 +1,20 @@ package admin import ( - "reflect" "testing" "github.com/stretchr/testify/assert" - "github.com/undefinedlabs/go-mpatch" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/discovery" - restclient "k8s.io/client-go/rest" - "k8s.io/client-go/tools/clientcmd" ) func TestProjectAllowListGen(t *testing.T) { - useMock := true - rules := clientcmd.NewDefaultClientConfigLoadingRules() - overrides := &clientcmd.ConfigOverrides{} - clientConfig := clientcmd.NewNonInteractiveDeferredLoadingClientConfig(rules, overrides) - - if useMock { - var patchClientConfig *mpatch.Patch - patchClientConfig, err := mpatch.PatchInstanceMethodByName(reflect.TypeOf(clientConfig), "ClientConfig", func(*clientcmd.DeferredLoadingClientConfig) (*restclient.Config, error) { - return nil, nil - }) - assert.NoError(t, err) - - patch, err := mpatch.PatchMethod(discovery.NewDiscoveryClientForConfig, func(c *restclient.Config) (*discovery.DiscoveryClient, error) { - return &discovery.DiscoveryClient{LegacyPrefix: "/api"}, nil - }) - assert.NoError(t, err) - - var patchSeverPreferredResources *mpatch.Patch - discoClient := &discovery.DiscoveryClient{} - patchSeverPreferredResources, err = mpatch.PatchInstanceMethodByName(reflect.TypeOf(discoClient), "ServerPreferredResources", func(*discovery.DiscoveryClient) ([]*metav1.APIResourceList, error) { - res := metav1.APIResource{ - Name: "services", - Kind: "Service", - } - resourceList := []*metav1.APIResourceList{{APIResources: []metav1.APIResource{res}}} - return resourceList, nil - }) - assert.NoError(t, err) - - defer func() { - err = patchClientConfig.Unpatch() - assert.NoError(t, err) - err = patch.Unpatch() - assert.NoError(t, err) - err = patchSeverPreferredResources.Unpatch() - err = patch.Unpatch() - }() + res := metav1.APIResource{ + Name: "services", + Kind: "Service", } + resourceList := []*metav1.APIResourceList{{APIResources: []metav1.APIResource{res}}} - globalProj := generateProjectAllowList(clientConfig, "testdata/test_clusterrole.yaml", "testproj") + globalProj, err := generateProjectAllowList(resourceList, "testdata/test_clusterrole.yaml", "testproj") + assert.NoError(t, err) assert.True(t, len(globalProj.Spec.NamespaceResourceWhitelist) > 0) } diff --git a/cmd/argocd/commands/app.go b/cmd/argocd/commands/app.go index 57d6845406dc..5a885893bc51 100644 --- a/cmd/argocd/commands/app.go +++ b/cmd/argocd/commands/app.go @@ -26,6 +26,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" @@ -776,7 +777,7 @@ func getLocalObjectsString(app *argoappv1.Application, local, localRepoRoot, app ApiVersions: apiVersions, Plugins: configManagementPlugins, TrackingMethod: trackingMethod, - }, true, &git.NoopCredsStore{}) + }, true, &git.NoopCredsStore{}, resource.MustParse("0")) errors.CheckError(err) return res.Manifests diff --git a/cmd/argocd/commands/login.go b/cmd/argocd/commands/login.go index 037985a85959..d9f2af6b0fdf 100644 --- a/cmd/argocd/commands/login.go +++ b/cmd/argocd/commands/login.go @@ -200,7 +200,10 @@ func oauth2Login(ctx context.Context, port int, oidcSettings *settingspkg.OIDCCo // completionChan is to signal flow completed. Non-empty string indicates error completionChan := make(chan string) // stateNonce is an OAuth2 state nonce - stateNonce := rand.RandString(10) + // According to the spec (https://www.rfc-editor.org/rfc/rfc6749#section-10.10), this must be guessable with + // probability <= 2^(-128). The following call generates one of 52^24 random strings, ~= 2^136 possibilities. + stateNonce, err := rand.String(24) + errors.CheckError(err) var tokenString string var refreshToken string @@ -210,7 +213,8 @@ func oauth2Login(ctx context.Context, port int, oidcSettings *settingspkg.OIDCCo } // PKCE implementation of https://tools.ietf.org/html/rfc7636 - codeVerifier := rand.RandStringCharset(43, "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~") + codeVerifier, err := rand.StringFromCharset(43, "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~") + errors.CheckError(err) codeChallengeHash := sha256.Sum256([]byte(codeVerifier)) codeChallenge := base64.RawURLEncoding.EncodeToString(codeChallengeHash[:]) @@ -294,7 +298,8 @@ func oauth2Login(ctx context.Context, port int, oidcSettings *settingspkg.OIDCCo opts = append(opts, oauth2.SetAuthURLParam("code_challenge_method", "S256")) url = oauth2conf.AuthCodeURL(stateNonce, opts...) case oidcutil.GrantTypeImplicit: - url = oidcutil.ImplicitFlowURL(oauth2conf, stateNonce, opts...) + url, err = oidcutil.ImplicitFlowURL(oauth2conf, stateNonce, opts...) + errors.CheckError(err) default: log.Fatalf("Unsupported grant type: %v", grantType) } diff --git a/controller/sync.go b/controller/sync.go index a50aa1c0e6a7..2726108c450d 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -140,7 +140,13 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, state *v1alpha } atomic.AddUint64(&syncIdPrefix, 1) - syncId := fmt.Sprintf("%05d-%s", syncIdPrefix, rand.RandString(5)) + randSuffix, err := rand.String(5) + if err != nil { + state.Phase = common.OperationError + state.Message = fmt.Sprintf("Failed generate random sync ID: %v", err) + return + } + syncId := fmt.Sprintf("%05d-%s", syncIdPrefix, randSuffix) logEntry := log.WithFields(log.Fields{"application": app.Name, "syncId": syncId}) initialResourcesRes := make([]common.ResourceSyncResult, 0) diff --git a/docs/operator-manual/argocd-cmd-params-cm.yaml b/docs/operator-manual/argocd-cmd-params-cm.yaml index 1cd602380348..eccd0e5b8178 100644 --- a/docs/operator-manual/argocd-cmd-params-cm.yaml +++ b/docs/operator-manual/argocd-cmd-params-cm.yaml @@ -103,4 +103,8 @@ data: reposerver.repo.cache.expiration: "24h0m0s" # Cache expiration default (default 24h0m0s) reposerver.default.cache.expiration: "24h0m0s" - \ No newline at end of file + # 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' diff --git a/docs/operator-manual/security.md b/docs/operator-manual/security.md index 4040b776ae54..92eef692eb5a 100644 --- a/docs/operator-manual/security.md +++ b/docs/operator-manual/security.md @@ -210,4 +210,45 @@ Argo CD logs payloads of most API requests except request that are considered se can be found in [server/server.go](https://github.com/argoproj/argo-cd/blob/abba8dddce8cd897ba23320e3715690f465b4a95/server/server.go#L516). Argo CD does not log IP addresses of clients requesting API endpoints, since the API server is typically behind a proxy. Instead, it is recommended -to configure IP addresses logging in the proxy server that sits in front of the API server. \ No newline at end of file +to configure IP addresses logging in the proxy server that sits in front of the API server. + +## 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. diff --git a/docs/operator-manual/server-commands/argocd-repo-server.md b/docs/operator-manual/server-commands/argocd-repo-server.md index a72b89d46b5a..bfcd6703b15e 100644 --- a/docs/operator-manual/server-commands/argocd-repo-server.md +++ b/docs/operator-manual/server-commands/argocd-repo-server.md @@ -13,27 +13,28 @@ argocd-repo-server [flags] ### Options ``` - --default-cache-expiration duration Cache expiration default (default 24h0m0s) - --disable-tls Disable TLS on the gRPC endpoint - -h, --help help for argocd-repo-server - --logformat string Set the logging format. One of: text|json (default "text") - --loglevel string Set the logging level. One of: debug|info|warn|error (default "info") - --metrics-port int Start metrics server on given port (default 8084) - --parallelismlimit int Limit on number of concurrent manifests generate requests. Any value less the 1 means no limit. - --port int Listen on given port for incoming connections (default 8081) - --redis string Redis server hostname and port (e.g. argocd-redis:6379). - --redis-ca-certificate string Path to Redis server CA certificate (e.g. /etc/certs/redis/ca.crt). If not specified, system trusted CAs will be used for server certificate validation. - --redis-client-certificate string Path to Redis client certificate (e.g. /etc/certs/redis/client.crt). - --redis-client-key string Path to Redis client key (e.g. /etc/certs/redis/client.crt). - --redis-insecure-skip-tls-verify Skip Redis server certificate validation. - --redis-use-tls Use TLS when connecting to Redis. - --redisdb int Redis database. - --repo-cache-expiration duration Cache expiration for repo state, incl. app lists, app details, manifest generation, revision meta-data (default 24h0m0s) - --revision-cache-expiration duration Cache expiration for cached revision (default 3m0s) - --sentinel stringArray Redis sentinel hostname and port (e.g. argocd-redis-ha-announce-0:6379). - --sentinelmaster string Redis sentinel master group name. (default "master") - --tlsciphers string The list of acceptable ciphers to be used when establishing TLS connections. Use 'list' to list available ciphers. (default "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384:TLS_RSA_WITH_AES_256_GCM_SHA384") - --tlsmaxversion string The maximum SSL/TLS version that is acceptable (one of: 1.0|1.1|1.2|1.3) (default "1.3") - --tlsminversion string The minimum SSL/TLS version that is acceptable (one of: 1.0|1.1|1.2|1.3) (default "1.2") + --default-cache-expiration duration Cache expiration default (default 24h0m0s) + --disable-tls Disable TLS on the gRPC endpoint + -h, --help help for argocd-repo-server + --logformat string Set the logging format. One of: text|json (default "text") + --loglevel string Set the logging level. One of: debug|info|warn|error (default "info") + --max-combined-directory-manifests-size string Max combined size of manifest files in a directory-type Application (default "10M") + --metrics-port int Start metrics server on given port (default 8084) + --parallelismlimit int Limit on number of concurrent manifests generate requests. Any value less the 1 means no limit. + --port int Listen on given port for incoming connections (default 8081) + --redis string Redis server hostname and port (e.g. argocd-redis:6379). + --redis-ca-certificate string Path to Redis server CA certificate (e.g. /etc/certs/redis/ca.crt). If not specified, system trusted CAs will be used for server certificate validation. + --redis-client-certificate string Path to Redis client certificate (e.g. /etc/certs/redis/client.crt). + --redis-client-key string Path to Redis client key (e.g. /etc/certs/redis/client.crt). + --redis-insecure-skip-tls-verify Skip Redis server certificate validation. + --redis-use-tls Use TLS when connecting to Redis. + --redisdb int Redis database. + --repo-cache-expiration duration Cache expiration for repo state, incl. app lists, app details, manifest generation, revision meta-data (default 24h0m0s) + --revision-cache-expiration duration Cache expiration for cached revision (default 3m0s) + --sentinel stringArray Redis sentinel hostname and port (e.g. argocd-redis-ha-announce-0:6379). + --sentinelmaster string Redis sentinel master group name. (default "master") + --tlsciphers string The list of acceptable ciphers to be used when establishing TLS connections. Use 'list' to list available ciphers. (default "TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384:TLS_RSA_WITH_AES_256_GCM_SHA384") + --tlsmaxversion string The maximum SSL/TLS version that is acceptable (one of: 1.0|1.1|1.2|1.3) (default "1.3") + --tlsminversion string The minimum SSL/TLS version that is acceptable (one of: 1.0|1.1|1.2|1.3) (default "1.2") ``` diff --git a/docs/requirements.txt b/docs/requirements.txt index faf48017ebc1..50064668f8db 100644 --- a/docs/requirements.txt +++ b/docs/requirements.txt @@ -1,4 +1,5 @@ mkdocs==1.2.3 mkdocs-material==7.1.7 markdown_include==0.6.0 -pygments==2.7.4 \ No newline at end of file +pygments==2.7.4 +jinja2===3.0.3 \ No newline at end of file diff --git a/go.mod b/go.mod index 2524460e6a4b..3c500ba07f8a 100644 --- a/go.mod +++ b/go.mod @@ -64,7 +64,6 @@ require ( github.com/spf13/cobra v1.2.1 github.com/spf13/pflag v1.0.5 github.com/stretchr/testify v1.7.0 - github.com/undefinedlabs/go-mpatch v1.0.6 github.com/whilp/git-urls v0.0.0-20191001220047-6db9661140c0 github.com/yuin/gopher-lua v0.0.0-20200816102855-ee81675732da golang.org/x/crypto v0.0.0-20210817164053-32db794688a5 diff --git a/go.sum b/go.sum index 3b0bf0a20b12..0ba8126f97e3 100644 --- a/go.sum +++ b/go.sum @@ -963,8 +963,6 @@ github.com/tmc/grpc-websocket-proxy v0.0.0-20201229170055-e5319fda7802/go.mod h1 github.com/ugorji/go v1.1.4/go.mod h1:uQMGLiO92mf5W77hV/PUCpI3pbzQx3CRekS0kk+RGrc= github.com/ugorji/go v1.1.7/go.mod h1:kZn38zHttfInRq0xu/PH0az30d+z6vm202qpg1oXVMw= github.com/ugorji/go/codec v1.1.7/go.mod h1:Ax+UKWsSmolVDwsd+7N3ZtXu+yMGCf907BLYF3GoBXY= -github.com/undefinedlabs/go-mpatch v1.0.6 h1:h8q5ORH/GaOE1Se1DMhrOyljXZEhRcROO7agMqWXCOY= -github.com/undefinedlabs/go-mpatch v1.0.6/go.mod h1:TyJZDQ/5AgyN7FSLiBJ8RO9u2c6wbtRvK827b6AVqY4= github.com/urfave/cli v1.20.0/go.mod h1:70zkFmudgCuE/ngEzBv17Jvp/497gISqfk5gWijbERA= github.com/urfave/cli v1.22.1/go.mod h1:Gos4lmkARVdJ6EkW0WaNv/tZAAMe9V7XWyB60NtXRu0= github.com/urfave/cli v1.22.2/go.mod h1:Gos4lmkARVdJ6EkW0WaNv/tZAAMe9V7XWyB60NtXRu0= diff --git a/hack/installers/install-lint-tools.sh b/hack/installers/install-lint-tools.sh index 4c9bc3bb1305..a77e67c6fb0c 100755 --- a/hack/installers/install-lint-tools.sh +++ b/hack/installers/install-lint-tools.sh @@ -1,4 +1,4 @@ #!/bin/bash set -eux -o pipefail -GO111MODULE=on go get github.com/golangci/golangci-lint/cmd/golangci-lint@v1.38.0 +GO111MODULE=on go install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.46.2 diff --git a/manifests/base/kustomization.yaml b/manifests/base/kustomization.yaml index fbebf801f347..4c0eff7b9ce3 100644 --- a/manifests/base/kustomization.yaml +++ b/manifests/base/kustomization.yaml @@ -5,7 +5,7 @@ kind: Kustomization images: - name: quay.io/argoproj/argocd newName: quay.io/argoproj/argocd - newTag: v2.3.3 + newTag: v2.3.5 resources: - ./application-controller - ./dex diff --git a/manifests/base/repo-server/argocd-repo-server-deployment.yaml b/manifests/base/repo-server/argocd-repo-server-deployment.yaml index 4fd65788d1bc..f14dce215c47 100644 --- a/manifests/base/repo-server/argocd-repo-server-deployment.yaml +++ b/manifests/base/repo-server/argocd-repo-server-deployment.yaml @@ -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 diff --git a/manifests/core-install.yaml b/manifests/core-install.yaml index 795c4e624663..8a0f985acd44 100644 --- a/manifests/core-install.yaml +++ b/manifests/core-install.yaml @@ -9686,13 +9686,19 @@ 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 value: /helm-working-dir - name: HELM_DATA_HOME value: /helm-working-dir - image: quay.io/argoproj/argocd:v2.3.3 + image: quay.io/argoproj/argocd:v2.3.5 imagePullPolicy: Always livenessProbe: failureThreshold: 3 @@ -9741,7 +9747,7 @@ spec: - -n - /usr/local/bin/argocd - /var/run/argocd/argocd-cmp-server - image: quay.io/argoproj/argocd:v2.3.3 + image: quay.io/argoproj/argocd:v2.3.5 name: copyutil volumeMounts: - mountPath: /var/run/argocd @@ -9906,7 +9912,7 @@ spec: key: controller.default.cache.expiration name: argocd-cmd-params-cm optional: true - image: quay.io/argoproj/argocd:v2.3.3 + image: quay.io/argoproj/argocd:v2.3.5 imagePullPolicy: Always livenessProbe: httpGet: diff --git a/manifests/core-install/kustomization.yaml b/manifests/core-install/kustomization.yaml index 537d08515061..59b882bd9ceb 100644 --- a/manifests/core-install/kustomization.yaml +++ b/manifests/core-install/kustomization.yaml @@ -11,4 +11,4 @@ resources: images: - name: quay.io/argoproj/argocd newName: quay.io/argoproj/argocd - newTag: v2.3.3 + newTag: v2.3.5 diff --git a/manifests/ha/base/kustomization.yaml b/manifests/ha/base/kustomization.yaml index 28998dce399c..c29cdc394887 100644 --- a/manifests/ha/base/kustomization.yaml +++ b/manifests/ha/base/kustomization.yaml @@ -11,7 +11,7 @@ patchesStrategicMerge: images: - name: quay.io/argoproj/argocd newName: quay.io/argoproj/argocd - newTag: v2.3.3 + newTag: v2.3.5 resources: - ../../base/application-controller - ../../base/dex diff --git a/manifests/ha/install.yaml b/manifests/ha/install.yaml index 65c4abf9286c..c072b567dafc 100644 --- a/manifests/ha/install.yaml +++ b/manifests/ha/install.yaml @@ -10516,7 +10516,7 @@ spec: - -n - /usr/local/bin/argocd - /shared/argocd-dex - image: quay.io/argoproj/argocd:v2.3.3 + image: quay.io/argoproj/argocd:v2.3.5 imagePullPolicy: Always name: copyutil volumeMounts: @@ -10549,7 +10549,7 @@ spec: containers: - command: - argocd-notifications - image: quay.io/argoproj/argocd:v2.3.3 + image: quay.io/argoproj/argocd:v2.3.5 imagePullPolicy: Always livenessProbe: tcpSocket: @@ -10776,13 +10776,19 @@ 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 value: /helm-working-dir - name: HELM_DATA_HOME value: /helm-working-dir - image: quay.io/argoproj/argocd:v2.3.3 + image: quay.io/argoproj/argocd:v2.3.5 imagePullPolicy: Always livenessProbe: failureThreshold: 3 @@ -10831,7 +10837,7 @@ spec: - -n - /usr/local/bin/argocd - /var/run/argocd/argocd-cmp-server - image: quay.io/argoproj/argocd:v2.3.3 + image: quay.io/argoproj/argocd:v2.3.5 name: copyutil volumeMounts: - mountPath: /var/run/argocd @@ -11058,7 +11064,7 @@ spec: key: server.http.cookie.maxnumber name: argocd-cmd-params-cm optional: true - image: quay.io/argoproj/argocd:v2.3.3 + image: quay.io/argoproj/argocd:v2.3.5 imagePullPolicy: Always livenessProbe: httpGet: @@ -11254,7 +11260,7 @@ spec: key: controller.default.cache.expiration name: argocd-cmd-params-cm optional: true - image: quay.io/argoproj/argocd:v2.3.3 + image: quay.io/argoproj/argocd:v2.3.5 imagePullPolicy: Always livenessProbe: httpGet: diff --git a/manifests/ha/namespace-install.yaml b/manifests/ha/namespace-install.yaml index 36f8ace29d51..ac0091dcf690 100644 --- a/manifests/ha/namespace-install.yaml +++ b/manifests/ha/namespace-install.yaml @@ -7812,7 +7812,7 @@ spec: - -n - /usr/local/bin/argocd - /shared/argocd-dex - image: quay.io/argoproj/argocd:v2.3.3 + image: quay.io/argoproj/argocd:v2.3.5 imagePullPolicy: Always name: copyutil volumeMounts: @@ -7845,7 +7845,7 @@ spec: containers: - command: - argocd-notifications - image: quay.io/argoproj/argocd:v2.3.3 + image: quay.io/argoproj/argocd:v2.3.5 imagePullPolicy: Always livenessProbe: tcpSocket: @@ -8072,13 +8072,19 @@ 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 value: /helm-working-dir - name: HELM_DATA_HOME value: /helm-working-dir - image: quay.io/argoproj/argocd:v2.3.3 + image: quay.io/argoproj/argocd:v2.3.5 imagePullPolicy: Always livenessProbe: failureThreshold: 3 @@ -8127,7 +8133,7 @@ spec: - -n - /usr/local/bin/argocd - /var/run/argocd/argocd-cmp-server - image: quay.io/argoproj/argocd:v2.3.3 + image: quay.io/argoproj/argocd:v2.3.5 name: copyutil volumeMounts: - mountPath: /var/run/argocd @@ -8354,7 +8360,7 @@ spec: key: server.http.cookie.maxnumber name: argocd-cmd-params-cm optional: true - image: quay.io/argoproj/argocd:v2.3.3 + image: quay.io/argoproj/argocd:v2.3.5 imagePullPolicy: Always livenessProbe: httpGet: @@ -8550,7 +8556,7 @@ spec: key: controller.default.cache.expiration name: argocd-cmd-params-cm optional: true - image: quay.io/argoproj/argocd:v2.3.3 + image: quay.io/argoproj/argocd:v2.3.5 imagePullPolicy: Always livenessProbe: httpGet: diff --git a/manifests/install.yaml b/manifests/install.yaml index 13d04fcb16c8..be505550582d 100644 --- a/manifests/install.yaml +++ b/manifests/install.yaml @@ -9886,7 +9886,7 @@ spec: - -n - /usr/local/bin/argocd - /shared/argocd-dex - image: quay.io/argoproj/argocd:v2.3.3 + image: quay.io/argoproj/argocd:v2.3.5 imagePullPolicy: Always name: copyutil volumeMounts: @@ -9919,7 +9919,7 @@ spec: containers: - command: - argocd-notifications - image: quay.io/argoproj/argocd:v2.3.3 + image: quay.io/argoproj/argocd:v2.3.5 imagePullPolicy: Always livenessProbe: tcpSocket: @@ -10110,13 +10110,19 @@ 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 value: /helm-working-dir - name: HELM_DATA_HOME value: /helm-working-dir - image: quay.io/argoproj/argocd:v2.3.3 + image: quay.io/argoproj/argocd:v2.3.5 imagePullPolicy: Always livenessProbe: failureThreshold: 3 @@ -10165,7 +10171,7 @@ spec: - -n - /usr/local/bin/argocd - /var/run/argocd/argocd-cmp-server - image: quay.io/argoproj/argocd:v2.3.3 + image: quay.io/argoproj/argocd:v2.3.5 name: copyutil volumeMounts: - mountPath: /var/run/argocd @@ -10388,7 +10394,7 @@ spec: key: server.http.cookie.maxnumber name: argocd-cmd-params-cm optional: true - image: quay.io/argoproj/argocd:v2.3.3 + image: quay.io/argoproj/argocd:v2.3.5 imagePullPolicy: Always livenessProbe: httpGet: @@ -10578,7 +10584,7 @@ spec: key: controller.default.cache.expiration name: argocd-cmd-params-cm optional: true - image: quay.io/argoproj/argocd:v2.3.3 + image: quay.io/argoproj/argocd:v2.3.5 imagePullPolicy: Always livenessProbe: httpGet: diff --git a/manifests/namespace-install.yaml b/manifests/namespace-install.yaml index cb7f9eb15a52..1f66c85939b4 100644 --- a/manifests/namespace-install.yaml +++ b/manifests/namespace-install.yaml @@ -7182,7 +7182,7 @@ spec: - -n - /usr/local/bin/argocd - /shared/argocd-dex - image: quay.io/argoproj/argocd:v2.3.3 + image: quay.io/argoproj/argocd:v2.3.5 imagePullPolicy: Always name: copyutil volumeMounts: @@ -7215,7 +7215,7 @@ spec: containers: - command: - argocd-notifications - image: quay.io/argoproj/argocd:v2.3.3 + image: quay.io/argoproj/argocd:v2.3.5 imagePullPolicy: Always livenessProbe: tcpSocket: @@ -7406,13 +7406,19 @@ 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 value: /helm-working-dir - name: HELM_DATA_HOME value: /helm-working-dir - image: quay.io/argoproj/argocd:v2.3.3 + image: quay.io/argoproj/argocd:v2.3.5 imagePullPolicy: Always livenessProbe: failureThreshold: 3 @@ -7461,7 +7467,7 @@ spec: - -n - /usr/local/bin/argocd - /var/run/argocd/argocd-cmp-server - image: quay.io/argoproj/argocd:v2.3.3 + image: quay.io/argoproj/argocd:v2.3.5 name: copyutil volumeMounts: - mountPath: /var/run/argocd @@ -7684,7 +7690,7 @@ spec: key: server.http.cookie.maxnumber name: argocd-cmd-params-cm optional: true - image: quay.io/argoproj/argocd:v2.3.3 + image: quay.io/argoproj/argocd:v2.3.5 imagePullPolicy: Always livenessProbe: httpGet: @@ -7874,7 +7880,7 @@ spec: key: controller.default.cache.expiration name: argocd-cmd-params-cm optional: true - image: quay.io/argoproj/argocd:v2.3.3 + image: quay.io/argoproj/argocd:v2.3.5 imagePullPolicy: Always livenessProbe: httpGet: diff --git a/pkg/apiclient/grpcproxy.go b/pkg/apiclient/grpcproxy.go index 61394756e421..553641c14365 100644 --- a/pkg/apiclient/grpcproxy.go +++ b/pkg/apiclient/grpcproxy.go @@ -100,7 +100,11 @@ func (c *client) executeRequest(fullMethodName string, msg []byte, md metadata.M } func (c *client) startGRPCProxy() (*grpc.Server, net.Listener, error) { - serverAddr := fmt.Sprintf("%s/argocd-%s.sock", os.TempDir(), rand.RandString(16)) + randSuffix, err := rand.String(16) + if err != nil { + return nil, nil, fmt.Errorf("failed to generate random socket filename: %w", err) + } + serverAddr := fmt.Sprintf("%s/argocd-%s.sock", os.TempDir(), randSuffix) ln, err := net.Listen("unix", serverAddr) if err != nil { diff --git a/pkg/apis/application/v1alpha1/cluster_constants.go b/pkg/apis/application/v1alpha1/cluster_constants.go index 13895eaf690f..d9fc0052de2d 100644 --- a/pkg/apis/application/v1alpha1/cluster_constants.go +++ b/pkg/apis/application/v1alpha1/cluster_constants.go @@ -48,17 +48,17 @@ var ( K8sMaxIdleConnections = env.ParseNumFromEnv(EnvK8sClientMaxIdleConnections, 500, 0, math.MaxInt32) // K8sTLSHandshakeTimeout defines the maximum duration to wait for a TLS handshake to complete - K8sTLSHandshakeTimeout = env.ParseDurationFromEnv(EnvK8sTLSHandshakeTimeout, 10*time.Second, 0, math.MaxInt32) + K8sTLSHandshakeTimeout = env.ParseDurationFromEnv(EnvK8sTLSHandshakeTimeout, 10*time.Second, 0, math.MaxInt32*time.Second) // K8sTCPTimeout defines the TCP timeout to use when performing K8s API requests - K8sTCPTimeout = env.ParseDurationFromEnv(EnvK8sTCPTimeout, 30*time.Second, 0, math.MaxInt32) + K8sTCPTimeout = env.ParseDurationFromEnv(EnvK8sTCPTimeout, 30*time.Second, 0, math.MaxInt32*time.Second) // K8sTCPKeepAlive defines the interval for sending TCP keep alive to K8s API server - K8sTCPKeepAlive = env.ParseDurationFromEnv(EnvK8sTCPKeepAlive, 30*time.Second, 0, math.MaxInt32) + K8sTCPKeepAlive = env.ParseDurationFromEnv(EnvK8sTCPKeepAlive, 30*time.Second, 0, math.MaxInt32*time.Second) // K8sTCPIdleConnTimeout defines the duration for keeping idle TCP connections to the K8s API server - K8sTCPIdleConnTimeout = env.ParseDurationFromEnv(EnvK8sTCPIdleConnTimeout, 5*time.Minute, 0, math.MaxInt32) + K8sTCPIdleConnTimeout = env.ParseDurationFromEnv(EnvK8sTCPIdleConnTimeout, 5*time.Minute, 0, math.MaxInt32*time.Second) // K8sServerSideTimeout defines which server side timeout to send with each API request - K8sServerSideTimeout = env.ParseDurationFromEnv(EnvK8sTCPTimeout, 32*time.Second, 0, math.MaxInt32) + K8sServerSideTimeout = env.ParseDurationFromEnv(EnvK8sTCPTimeout, 0, 0, math.MaxInt32*time.Second) ) diff --git a/reposerver/repository/repository.go b/reposerver/repository/repository.go index 33dd2e95cf31..b435ae501639 100644 --- a/reposerver/repository/repository.go +++ b/reposerver/repository/repository.go @@ -18,6 +18,12 @@ import ( "strings" "time" + kubeyaml "k8s.io/apimachinery/pkg/util/yaml" + + "k8s.io/apimachinery/pkg/api/resource" + + "github.com/argoproj/argo-cd/v2/util/io/files" + "github.com/Masterminds/semver/v3" "github.com/TomOnTime/utfutil" "github.com/argoproj/gitops-engine/pkg/utils/kube" @@ -69,6 +75,8 @@ const ( ociPrefix = "oci://" ) +var ErrExceededMaxCombinedManifestFileSize = errors.New("exceeded max combined manifest file size") + // Service implements ManifestService interface type Service struct { gitCredsStore git.CredsStore @@ -94,6 +102,7 @@ type RepoServerInitConstants struct { PauseGenerationOnFailureForMinutes int PauseGenerationOnFailureForRequests int SubmoduleEnabled bool + MaxCombinedDirectoryManifestsSize resource.Quantity } // NewService returns a new instance of the Manifest service @@ -383,7 +392,7 @@ func (s *Service) runManifestGen(ctx context.Context, repoRoot, commitSHA, cache var manifestGenResult *apiclient.ManifestResponse opContext, err := opContextSrc() if err == nil { - manifestGenResult, err = GenerateManifests(ctx, opContext.appPath, repoRoot, commitSHA, q, false, s.gitCredsStore) + manifestGenResult, err = GenerateManifests(ctx, opContext.appPath, repoRoot, commitSHA, q, false, s.gitCredsStore, s.initConstants.MaxCombinedDirectoryManifestsSize) } if err != nil { @@ -769,7 +778,7 @@ func getRepoCredential(repoCredentials []*v1alpha1.RepoCreds, repoURL string) *v } // GenerateManifests generates manifests from a path -func GenerateManifests(ctx context.Context, appPath, repoRoot, revision string, q *apiclient.ManifestRequest, isLocal bool, gitCredsStore git.CredsStore) (*apiclient.ManifestResponse, error) { +func GenerateManifests(ctx context.Context, appPath, repoRoot, revision string, q *apiclient.ManifestRequest, isLocal bool, gitCredsStore git.CredsStore, maxCombinedManifestQuantity resource.Quantity) (*apiclient.ManifestResponse, error) { var targetObjs []*unstructured.Unstructured var dest *v1alpha1.ApplicationDestination @@ -810,7 +819,8 @@ func GenerateManifests(ctx context.Context, appPath, repoRoot, revision string, if directory = q.ApplicationSource.Directory; directory == nil { directory = &v1alpha1.ApplicationSourceDirectory{} } - targetObjs, err = findManifests(appPath, repoRoot, env, *directory, q.EnabledSourceTypes) + logCtx := log.WithField("application", q.AppName) + targetObjs, err = findManifests(logCtx, appPath, repoRoot, env, *directory, q.EnabledSourceTypes, maxCombinedManifestQuantity) } if err != nil { return nil, err @@ -1012,47 +1022,30 @@ func ksShow(appLabelKey, appPath string, ksonnetOpts *v1alpha1.ApplicationSource var manifestFile = regexp.MustCompile(`^.*\.(yaml|yml|json|jsonnet)$`) // findManifests looks at all yaml files in a directory and unmarshals them into a list of unstructured objects -func findManifests(appPath string, repoRoot string, env *v1alpha1.Env, directory v1alpha1.ApplicationSourceDirectory, enabledManifestGeneration map[string]bool) ([]*unstructured.Unstructured, error) { - var objs []*unstructured.Unstructured - err := filepath.Walk(appPath, func(path string, f os.FileInfo, err error) error { - if err != nil { - return err - } - if f.IsDir() { - if path != appPath && !directory.Recurse { - return filepath.SkipDir - } else { - return nil - } - } - - if !manifestFile.MatchString(f.Name()) { - return nil - } - - relPath, err := filepath.Rel(appPath, path) - if err != nil { - return err - } - if directory.Exclude != "" && glob.Match(directory.Exclude, relPath) { - return nil - } +func findManifests(logCtx *log.Entry, appPath string, repoRoot string, env *v1alpha1.Env, directory v1alpha1.ApplicationSourceDirectory, enabledManifestGeneration map[string]bool, maxCombinedManifestQuantity resource.Quantity) ([]*unstructured.Unstructured, error) { + // Validate the directory before loading any manifests to save memory. + potentiallyValidManifests, err := getPotentiallyValidManifests(logCtx, appPath, repoRoot, directory.Recurse, directory.Include, directory.Exclude, maxCombinedManifestQuantity) + if err != nil { + logCtx.Errorf("failed to get potentially valid manifests: %s", err) + return nil, fmt.Errorf("failed to get potentially valid manifests: %w", err) + } - if directory.Include != "" && !glob.Match(directory.Include, relPath) { - return nil - } + var objs []*unstructured.Unstructured + for _, potentiallyValidManifest := range potentiallyValidManifests { + manifestPath := potentiallyValidManifest.path + manifestFileInfo := potentiallyValidManifest.fileInfo - if strings.HasSuffix(f.Name(), ".jsonnet") { + if strings.HasSuffix(manifestFileInfo.Name(), ".jsonnet") { if !discovery.IsManifestGenerationEnabled(v1alpha1.ApplicationSourceTypeDirectory, enabledManifestGeneration) { - return nil + continue } vm, err := makeJsonnetVm(appPath, repoRoot, directory.Jsonnet, env) if err != nil { - return err + return nil, err } - jsonStr, err := vm.EvaluateFile(path) + jsonStr, err := vm.EvaluateFile(manifestPath) if err != nil { - return status.Errorf(codes.FailedPrecondition, "Failed to evaluate jsonnet %q: %v", f.Name(), err) + return nil, status.Errorf(codes.FailedPrecondition, "Failed to evaluate jsonnet %q: %v", manifestFileInfo.Name(), err) } // attempt to unmarshal either array or single object @@ -1064,49 +1057,207 @@ func findManifests(appPath string, repoRoot string, env *v1alpha1.Env, directory var jsonObj unstructured.Unstructured err = json.Unmarshal([]byte(jsonStr), &jsonObj) if err != nil { - return status.Errorf(codes.FailedPrecondition, "Failed to unmarshal generated json %q: %v", f.Name(), err) + return nil, status.Errorf(codes.FailedPrecondition, "Failed to unmarshal generated json %q: %v", manifestFileInfo.Name(), err) } objs = append(objs, &jsonObj) } } else { - out, err := utfutil.ReadFile(path, utfutil.UTF8) + err := getObjsFromYAMLOrJson(logCtx, manifestPath, manifestFileInfo.Name(), &objs) if err != nil { - return err + return nil, err } - if strings.HasSuffix(f.Name(), ".json") { - var obj unstructured.Unstructured - err = json.Unmarshal(out, &obj) - if err != nil { - return status.Errorf(codes.FailedPrecondition, "Failed to unmarshal %q: %v", f.Name(), err) - } - objs = append(objs, &obj) - } else { - yamlObjs, err := kube.SplitYAML(out) - if err != nil { - if len(yamlObjs) > 0 { - // If we get here, we had a multiple objects in a single YAML file which had some - // valid k8s objects, but errors parsing others (within the same file). It's very - // likely the user messed up a portion of the YAML, so report on that. - return status.Errorf(codes.FailedPrecondition, "Failed to unmarshal %q: %v", f.Name(), err) - } - // Otherwise, let's see if it looks like a resource, if yes, we return error - if bytes.Contains(out, []byte("apiVersion:")) && - bytes.Contains(out, []byte("kind:")) && - bytes.Contains(out, []byte("metadata:")) { - return status.Errorf(codes.FailedPrecondition, "Failed to unmarshal %q: %v", f.Name(), err) - } - // Otherwise, it might be a unrelated YAML file which we will ignore - return nil - } - objs = append(objs, yamlObjs...) + } + } + return objs, nil +} + +// getObjsFromYAMLOrJson unmarshals the given yaml or json file and appends it to the given list of objects. +func getObjsFromYAMLOrJson(logCtx *log.Entry, manifestPath string, filename string, objs *[]*unstructured.Unstructured) error { + reader, err := utfutil.OpenFile(manifestPath, utfutil.UTF8) + if err != nil { + return status.Errorf(codes.FailedPrecondition, "Failed to open %q", manifestPath) + } + defer func() { + err := reader.Close() + if err != nil { + logCtx.Errorf("failed to close %q - potential memory leak", manifestPath) + } + }() + if strings.HasSuffix(filename, ".json") { + var obj unstructured.Unstructured + decoder := json.NewDecoder(reader) + err = decoder.Decode(&obj) + if err != nil { + return status.Errorf(codes.FailedPrecondition, "Failed to unmarshal %q: %v", filename, err) + } + if decoder.More() { + return status.Errorf(codes.FailedPrecondition, "Found multiple objects in %q. Only single objects are allowed in JSON files.", filename) + } + *objs = append(*objs, &obj) + } else { + yamlObjs, err := splitYAMLOrJSON(reader) + if err != nil { + if len(yamlObjs) > 0 { + // If we get here, we had a multiple objects in a single YAML file which had some + // valid k8s objects, but errors parsing others (within the same file). It's very + // likely the user messed up a portion of the YAML, so report on that. + return status.Errorf(codes.FailedPrecondition, "Failed to unmarshal %q: %v", filename, err) + } + // Read the whole file to check whether it looks like a manifest. + out, err := utfutil.ReadFile(manifestPath, utfutil.UTF8) + // Otherwise, let's see if it looks like a resource, if yes, we return error + if bytes.Contains(out, []byte("apiVersion:")) && + bytes.Contains(out, []byte("kind:")) && + bytes.Contains(out, []byte("metadata:")) { + return status.Errorf(codes.FailedPrecondition, "Failed to unmarshal %q: %v", filename, err) + } + // Otherwise, it might be an unrelated YAML file which we will ignore + } + *objs = append(*objs, yamlObjs...) + } + return nil +} + +// splitYAMLOrJSON reads a YAML or JSON file and gets each document as an unstructured object. If the unmarshaller +// encounters an error, objects read up until the error are returned. +func splitYAMLOrJSON(reader goio.Reader) ([]*unstructured.Unstructured, error) { + d := kubeyaml.NewYAMLOrJSONDecoder(reader, 4096) + var objs []*unstructured.Unstructured + for { + u := &unstructured.Unstructured{} + if err := d.Decode(&u); err != nil { + if err == goio.EOF { + break } + return objs, fmt.Errorf("failed to unmarshal manifest: %v", err) } + if u == nil { + continue + } + objs = append(objs, u) + } + return objs, nil +} + +// getPotentiallyValidManifestFile checks whether the given path/FileInfo may be a valid manifest file. Returns a non-nil error if +// there was an error that should not be handled by ignoring the file. Returns non-nil realFileInfo if the file is a +// potential manifest. Returns a non-empty ignoreMessage if there's a message that should be logged about why the file +// was skipped. If realFileInfo is nil and the ignoreMessage is empty, there's no need to log the ignoreMessage; the +// file was skipped for a mundane reason. +// +// The file is still only a "potentially" valid manifest file because it could be invalid JSON or YAML, or it might not +// be a valid Kubernetes resource. This function tests everything possible without actually reading the file. +// +// repoPath must be absolute. +func getPotentiallyValidManifestFile(path string, f os.FileInfo, appPath, repoRoot, include, exclude string) (realFileInfo os.FileInfo, warning string, err error) { + relPath, err := filepath.Rel(appPath, path) + if err != nil { + return nil, "", fmt.Errorf("failed to get relative path of %q: %w", path, err) + } + + if !manifestFile.MatchString(f.Name()) { + return nil, "", nil + } + + // If the file is a symlink, these will be overridden with the destination file's info. + var relRealPath = relPath + realFileInfo = f + + if files.IsSymlink(f) { + realPath, err := filepath.EvalSymlinks(path) + if err != nil { + if os.IsNotExist(err) { + return nil, fmt.Sprintf("destination of symlink %q is missing", relPath), nil + } + return nil, "", fmt.Errorf("failed to evaluate symlink at %q: %w", relPath, err) + } + if !files.Inbound(realPath, repoRoot) { + return nil, "", fmt.Errorf("illegal filepath in symlink at %q", relPath) + } + realFileInfo, err = os.Stat(realPath) + if err != nil { + if os.IsNotExist(err) { + // This should have been caught by filepath.EvalSymlinks, but check again since that function's docs + // don't promise to return this error. + return nil, fmt.Sprintf("destination of symlink %q is missing at %q", relPath, realPath), nil + } + return nil, "", fmt.Errorf("failed to get file info for symlink at %q to %q: %w", relPath, realPath, err) + } + relRealPath, err = filepath.Rel(repoRoot, realPath) + if err != nil { + return nil, "", fmt.Errorf("failed to get relative path of %q: %w", realPath, err) + } + } + + // FileInfo.Size() behavior is platform-specific for non-regular files. Allow only regular files, so we guarantee + // accurate file sizes. + if !realFileInfo.Mode().IsRegular() { + return nil, fmt.Sprintf("ignoring symlink at %q to non-regular file %q", relPath, relRealPath), nil + } + + if exclude != "" && glob.Match(exclude, relPath) { + return nil, "", nil + } + + if include != "" && !glob.Match(include, relPath) { + return nil, "", nil + } + + return realFileInfo, "", nil +} + +type potentiallyValidManifest struct { + path string + fileInfo os.FileInfo +} + +// getPotentiallyValidManifests ensures that 1) there are no errors while checking for potential manifest files in the given dir +// and 2) the combined file size of the potentially-valid manifest files does not exceed the limit. +func getPotentiallyValidManifests(logCtx *log.Entry, appPath string, repoRoot string, recurse bool, include string, exclude string, maxCombinedManifestQuantity resource.Quantity) ([]potentiallyValidManifest, error) { + maxCombinedManifestFileSize := maxCombinedManifestQuantity.Value() + var currentCombinedManifestFileSize = int64(0) + + var potentiallyValidManifests []potentiallyValidManifest + err := filepath.Walk(appPath, func(path string, f os.FileInfo, err error) error { + if err != nil { + return err + } + + if f.IsDir() { + if path != appPath && !recurse { + return filepath.SkipDir + } + return nil + } + + realFileInfo, warning, err := getPotentiallyValidManifestFile(path, f, appPath, repoRoot, include, exclude) + if err != nil { + return fmt.Errorf("invalid manifest file %q: %w", path, err) + } + if realFileInfo == nil { + if warning != "" { + logCtx.Warnf("skipping manifest file %q: %s", path, warning) + } + return nil + } + // Don't count jsonnet file size against max. It's jsonnet's responsibility to manage memory usage. + if !strings.HasSuffix(f.Name(), ".jsonnet") { + // We use the realFileInfo size (which is guaranteed to be a regular file instead of a symlink or other + // non-regular file) because .Size() behavior is platform-specific for non-regular files. + currentCombinedManifestFileSize += realFileInfo.Size() + if maxCombinedManifestFileSize != 0 && currentCombinedManifestFileSize > maxCombinedManifestFileSize { + return ErrExceededMaxCombinedManifestFileSize + } + } + potentiallyValidManifests = append(potentiallyValidManifests, potentiallyValidManifest{path: path, fileInfo: f}) return nil }) if err != nil { + // Not wrapping, because this error should be wrapped by the caller. return nil, err } - return objs, nil + + return potentiallyValidManifests, nil } func makeJsonnetVm(appPath string, repoRoot string, sourceJsonnet v1alpha1.ApplicationSourceJsonnet, env *v1alpha1.Env) (*jsonnet.VM, error) { @@ -1412,8 +1563,12 @@ func populateHelmAppDetails(res *apiclient.RepoAppDetailsResponse, appPath strin return err } - if err := loadFileIntoIfExists(filepath.Join(appPath, "values.yaml"), &res.Helm.Values); err != nil { - return err + if resolvedValuesPath, _, err := pathutil.ResolveFilePath(appPath, repoRoot, "values.yaml", []string{}); err == nil { + if err := loadFileIntoIfExists(resolvedValuesPath, &res.Helm.Values); err != nil { + return err + } + } else { + log.Warnf("Values file %s is not allowed: %v", filepath.Join(appPath, "values.yaml"), err) } var resolvedSelectedValueFiles []pathutil.ResolvedFilePath // drop not allowed values files @@ -1421,10 +1576,10 @@ func populateHelmAppDetails(res *apiclient.RepoAppDetailsResponse, appPath strin if resolvedFile, _, err := pathutil.ResolveFilePath(appPath, repoRoot, file, q.GetValuesFileSchemes()); err == nil { resolvedSelectedValueFiles = append(resolvedSelectedValueFiles, resolvedFile) } else { - log.Debugf("Values file %s is not allowed: %v", file, err) + log.Warnf("Values file %s is not allowed: %v", file, err) } } - params, err := h.GetParameters(resolvedSelectedValueFiles) + params, err := h.GetParameters(resolvedSelectedValueFiles, appPath, repoRoot) if err != nil { return err } @@ -1443,15 +1598,16 @@ func populateHelmAppDetails(res *apiclient.RepoAppDetailsResponse, appPath strin return nil } -func loadFileIntoIfExists(path string, destination *string) error { - info, err := os.Stat(path) +func loadFileIntoIfExists(path pathutil.ResolvedFilePath, destination *string) error { + stringPath := string(path) + info, err := os.Stat(stringPath) if err == nil && !info.IsDir() { - if bytes, err := ioutil.ReadFile(path); err != nil { - *destination = string(bytes) - } else { + bytes, err := ioutil.ReadFile(stringPath); + if err != nil { return err } + *destination = string(bytes) } return nil diff --git a/reposerver/repository/repository_norace_test.go b/reposerver/repository/repository_norace_test.go deleted file mode 100644 index 6cf429b1c1bb..000000000000 --- a/reposerver/repository/repository_norace_test.go +++ /dev/null @@ -1,47 +0,0 @@ -//go:build !race -// +build !race - -package repository - -import ( - "os" - "path/filepath" - "sync" - "testing" - - "github.com/stretchr/testify/assert" - - argoappv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" - "github.com/argoproj/argo-cd/v2/reposerver/apiclient" -) - -func TestHelmDependencyWithConcurrency(t *testing.T) { - - // !race: - // Un-synchronized use of a random source, will be fixed when this is merged: - // https://github.com/argoproj/argo-cd/issues/4728 - - cleanup := func() { - _ = os.Remove(filepath.Join("../../util/helm/testdata/helm2-dependency", helmDepUpMarkerFile)) - _ = os.RemoveAll(filepath.Join("../../util/helm/testdata/helm2-dependency", "charts")) - } - cleanup() - defer cleanup() - - helmRepo := argoappv1.Repository{Name: "bitnami", Type: "helm", Repo: "https://charts.bitnami.com/bitnami"} - var wg sync.WaitGroup - wg.Add(3) - for i := 0; i < 3; i++ { - go func() { - res, err := helmTemplate("../../util/helm/testdata/helm2-dependency", "../..", nil, &apiclient.ManifestRequest{ - ApplicationSource: &argoappv1.ApplicationSource{}, - Repos: []*argoappv1.Repository{&helmRepo}, - }, false) - - assert.NoError(t, err) - assert.NotNil(t, res) - wg.Done() - }() - } - wg.Wait() -} diff --git a/reposerver/repository/repository_test.go b/reposerver/repository/repository_test.go index 52e70e668270..b23e8ec44303 100644 --- a/reposerver/repository/repository_test.go +++ b/reposerver/repository/repository_test.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" goio "io" + "io/fs" "io/ioutil" "os" "os/exec" @@ -16,6 +17,9 @@ import ( "testing" "time" + log "github.com/sirupsen/logrus" + "k8s.io/apimachinery/pkg/api/resource" + "github.com/ghodss/yaml" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -144,11 +148,81 @@ func TestGenerateYamlManifestInDir(t *testing.T) { assert.Equal(t, countOfManifests, len(res1.Manifests)) // this will test concatenated manifests to verify we split YAMLs correctly - res2, err := GenerateManifests(context.Background(), "./testdata/concatenated", "/", "", &q, false, &git.NoopCredsStore{}) + res2, err := GenerateManifests(context.Background(), "./testdata/concatenated", "/", "", &q, false, &git.NoopCredsStore{}, resource.MustParse("0")) assert.NoError(t, err) assert.Equal(t, 3, len(res2.Manifests)) } +func Test_GenerateManifests_NoOutOfBoundsAccess(t *testing.T) { + testCases := []struct { + name string + outOfBoundsFilename string + outOfBoundsFileContents string + mustNotContain string // Optional string that must not appear in error or manifest output. If empty, use outOfBoundsFileContents. + }{ + { + name: "out of bounds JSON file should not appear in error output", + outOfBoundsFilename: "test.json", + outOfBoundsFileContents: `{"some": "json"}`, + }, + { + name: "malformed JSON file contents should not appear in error output", + outOfBoundsFilename: "test.json", + outOfBoundsFileContents: "$", + }, + { + name: "out of bounds JSON manifest should not appear in manifest output", + outOfBoundsFilename: "test.json", + // JSON marshalling is deterministic. So if there's a leak, exactly this should appear in the manifests. + outOfBoundsFileContents: `{"apiVersion":"v1","kind":"Secret","metadata":{"name":"test","namespace":"default"},"type":"Opaque"}`, + }, + { + name: "out of bounds YAML manifest should not appear in manifest output", + outOfBoundsFilename: "test.yaml", + outOfBoundsFileContents: "apiVersion: v1\nkind: Secret\nmetadata:\n name: test\n namespace: default\ntype: Opaque", + mustNotContain: `{"apiVersion":"v1","kind":"Secret","metadata":{"name":"test","namespace":"default"},"type":"Opaque"}`, + }, + } + + for _, testCase := range testCases { + testCaseCopy := testCase + t.Run(testCaseCopy.name, func(t *testing.T) { + t.Parallel() + + outOfBoundsDir := t.TempDir() + outOfBoundsFile := path.Join(outOfBoundsDir, testCaseCopy.outOfBoundsFilename) + err := os.WriteFile(outOfBoundsFile, []byte(testCaseCopy.outOfBoundsFileContents), os.FileMode(0444)) + require.NoError(t, err) + + repoDir := t.TempDir() + err = os.Symlink(outOfBoundsFile, path.Join(repoDir, testCaseCopy.outOfBoundsFilename)) + require.NoError(t, err) + + var mustNotContain = testCaseCopy.outOfBoundsFileContents + if testCaseCopy.mustNotContain != "" { + mustNotContain = testCaseCopy.mustNotContain + } + + q := apiclient.ManifestRequest{Repo: &argoappv1.Repository{}, ApplicationSource: &argoappv1.ApplicationSource{}} + res, err := GenerateManifests(context.Background(), repoDir, "", "", &q, false, &git.NoopCredsStore{}, resource.MustParse("0")) + require.Error(t, err) + assert.NotContains(t, err.Error(), mustNotContain) + assert.Contains(t, err.Error(), "illegal filepath") + assert.Nil(t, res) + }) + } +} + +func TestGenerateManifests_MissingSymlinkDestination(t *testing.T) { + repoDir := t.TempDir() + err := os.Symlink("/obviously/does/not/exist", path.Join(repoDir, "test.yaml")) + require.NoError(t, err) + + q := apiclient.ManifestRequest{Repo: &argoappv1.Repository{}, ApplicationSource: &argoappv1.ApplicationSource{}} + _, err = GenerateManifests(context.Background(), repoDir, "", "", &q, false, &git.NoopCredsStore{}, resource.MustParse("0")) + require.NoError(t, err) +} + func TestGenerateManifests_K8SAPIResetCache(t *testing.T) { service := newService("../..") @@ -302,29 +376,6 @@ func TestGenerateKsonnetManifest(t *testing.T) { assert.Equal(t, "https://kubernetes.default.svc", res.Server) } -func TestGenerateHelmChartWithDependencies(t *testing.T) { - service := newService("../..") - - cleanup := func() { - _ = os.Remove(filepath.Join("../../util/helm/testdata/helm2-dependency", helmDepUpMarkerFile)) - _ = os.RemoveAll(filepath.Join("../../util/helm/testdata/helm2-dependency", "charts")) - } - cleanup() - defer cleanup() - - helmRepo := argoappv1.Repository{Name: "bitnami", Type: "helm", Repo: "https://charts.bitnami.com/bitnami"} - q := apiclient.ManifestRequest{ - Repo: &argoappv1.Repository{}, - ApplicationSource: &argoappv1.ApplicationSource{ - Path: "./util/helm/testdata/helm2-dependency", - }, - Repos: []*argoappv1.Repository{&helmRepo}, - } - res1, err := service.GenerateManifest(context.Background(), &q) - assert.Nil(t, err) - assert.Len(t, res1.Manifests, 10) -} - func TestManifestGenErrorCacheByNumRequests(t *testing.T) { // Returns the state of the manifest generation cache, by querying the cache for the previously set result @@ -1080,7 +1131,7 @@ func TestGenerateFromUTF16(t *testing.T) { Repo: &argoappv1.Repository{}, ApplicationSource: &argoappv1.ApplicationSource{}, } - res1, err := GenerateManifests(context.Background(), "./testdata/utf-16", "/", "", &q, false, &git.NoopCredsStore{}) + res1, err := GenerateManifests(context.Background(), "./testdata/utf-16", "/", "", &q, false, &git.NoopCredsStore{}, resource.MustParse("0")) assert.Nil(t, err) assert.Equal(t, 2, len(res1.Manifests)) } @@ -1096,12 +1147,15 @@ func TestListApps(t *testing.T) { "app-parameters/multi": "Kustomize", "app-parameters/single-app-only": "Kustomize", "app-parameters/single-global": "Kustomize", + "in-bounds-values-file-link": "Helm", "invalid-helm": "Helm", "invalid-kustomize": "Kustomize", "kustomization_yaml": "Kustomize", "kustomization_yml": "Kustomize", "my-chart": "Helm", "my-chart-2": "Helm", + "out-of-bounds-values-file-link": "Helm", + "values-files": "Helm", } assert.Equal(t, expectedApps, res.Apps) } @@ -1440,6 +1494,7 @@ func runWithTempTestdata(t *testing.T, path string, runner func(t *testing.T, pa tempDir := mkTempParameters("./testdata/app-parameters") defer os.RemoveAll(tempDir) runner(t, filepath.Join(tempDir, "app-parameters", path)) + os.RemoveAll(tempDir) } func TestGenerateManifestsWithAppParameterFile(t *testing.T) { @@ -1641,11 +1696,11 @@ func TestFindResources(t *testing.T) { for i := range testCases { tc := testCases[i] t.Run(tc.name, func(t *testing.T) { - objs, err := findManifests("testdata/app-include-exclude", ".", nil, argoappv1.ApplicationSourceDirectory{ + objs, err := findManifests(&log.Entry{}, "testdata/app-include-exclude", ".", nil, argoappv1.ApplicationSourceDirectory{ Recurse: true, Include: tc.include, Exclude: tc.exclude, - }, map[string]bool{}) + }, map[string]bool{}, resource.MustParse("0")) if !assert.NoError(t, err) { return } @@ -1659,10 +1714,10 @@ func TestFindResources(t *testing.T) { } func TestFindManifests_Exclude(t *testing.T) { - objs, err := findManifests("testdata/app-include-exclude", ".", nil, argoappv1.ApplicationSourceDirectory{ + objs, err := findManifests(&log.Entry{}, "testdata/app-include-exclude", ".", nil, argoappv1.ApplicationSourceDirectory{ Recurse: true, Exclude: "subdir/deploymentSub.yaml", - }, map[string]bool{}) + }, map[string]bool{}, resource.MustParse("0")) if !assert.NoError(t, err) || !assert.Len(t, objs, 1) { return @@ -1672,10 +1727,10 @@ func TestFindManifests_Exclude(t *testing.T) { } func TestFindManifests_Exclude_NothingMatches(t *testing.T) { - objs, err := findManifests("testdata/app-include-exclude", ".", nil, argoappv1.ApplicationSourceDirectory{ + objs, err := findManifests(&log.Entry{}, "testdata/app-include-exclude", ".", nil, argoappv1.ApplicationSourceDirectory{ Recurse: true, Exclude: "nothing.yaml", - }, map[string]bool{}) + }, map[string]bool{}, resource.MustParse("0")) if !assert.NoError(t, err) || !assert.Len(t, objs, 2) { return @@ -1685,6 +1740,469 @@ func TestFindManifests_Exclude_NothingMatches(t *testing.T) { []string{"nginx-deployment", "nginx-deployment-sub"}, []string{objs[0].GetName(), objs[1].GetName()}) } +func tempDir(t *testing.T) string { + dir, err := ioutil.TempDir(".", "") + require.NoError(t, err) + t.Cleanup(func() { + err = os.RemoveAll(dir) + if err != nil { + panic(err) + } + }) + absDir, err := filepath.Abs(dir) + require.NoError(t, err) + return absDir +} + +func walkFor(t *testing.T, root string, testPath string, run func(info fs.FileInfo)) { + var hitExpectedPath = false + err := filepath.Walk(root, func(path string, info fs.FileInfo, err error) error { + if path == testPath { + require.NoError(t, err) + hitExpectedPath = true + run(info) + } + return nil + }) + require.NoError(t, err) + assert.True(t, hitExpectedPath, "did not hit expected path when walking directory") +} + +func Test_getPotentiallyValidManifestFile(t *testing.T) { + // These tests use filepath.Walk instead of os.Stat to get file info, because FileInfo from os.Stat does not return + // true for IsSymlink like os.Walk does. + + // These tests do not use t.TempDir() because those directories can contain symlinks which cause test to fail + // InBound checks. + + t.Run("non-JSON/YAML is skipped with an empty ignore message", func(t *testing.T) { + appDir := tempDir(t) + filePath := filepath.Join(appDir, "not-json-or-yaml") + file, err := os.OpenFile(filePath, os.O_RDONLY|os.O_CREATE, 0644) + require.NoError(t, err) + err = file.Close() + require.NoError(t, err) + + walkFor(t, appDir, filePath, func(info fs.FileInfo) { + realFileInfo, ignoreMessage, err := getPotentiallyValidManifestFile(filePath, info, appDir, appDir, "", "") + assert.Nil(t, realFileInfo) + assert.Empty(t, ignoreMessage) + assert.NoError(t, err) + }) + }) + + t.Run("circular link should throw an error", func(t *testing.T) { + appDir := tempDir(t) + + aPath := filepath.Join(appDir, "a.json") + bPath := filepath.Join(appDir, "b.json") + err := os.Symlink(bPath, aPath) + require.NoError(t, err) + err = os.Symlink(aPath, bPath) + require.NoError(t, err) + + walkFor(t, appDir, aPath, func(info fs.FileInfo) { + realFileInfo, ignoreMessage, err := getPotentiallyValidManifestFile(aPath, info, appDir, appDir, "", "") + assert.Nil(t, realFileInfo) + assert.Empty(t, ignoreMessage) + assert.Error(t, err) + assert.Contains(t, err.Error(), "too many links") + }) + }) + + t.Run("symlink with missing destination should throw an error", func(t *testing.T) { + appDir := tempDir(t) + + aPath := filepath.Join(appDir, "a.json") + bPath := filepath.Join(appDir, "b.json") + err := os.Symlink(bPath, aPath) + require.NoError(t, err) + + walkFor(t, appDir, aPath, func(info fs.FileInfo) { + realFileInfo, ignoreMessage, err := getPotentiallyValidManifestFile(aPath, info, appDir, appDir, "", "") + assert.Nil(t, realFileInfo) + assert.NotEmpty(t, ignoreMessage) + assert.NoError(t, err) + }) + }) + + t.Run("out-of-bounds symlink should throw an error", func(t *testing.T) { + appDir := tempDir(t) + + linkPath := filepath.Join(appDir, "a.json") + err := os.Symlink("..", linkPath) + require.NoError(t, err) + + walkFor(t, appDir, linkPath, func(info fs.FileInfo) { + realFileInfo, ignoreMessage, err := getPotentiallyValidManifestFile(linkPath, info, appDir, appDir, "", "") + assert.Nil(t, realFileInfo) + assert.Empty(t, ignoreMessage) + assert.Error(t, err) + assert.Contains(t, err.Error(), "illegal filepath in symlink") + }) + }) + + t.Run("symlink to a non-regular file should be skipped with warning", func(t *testing.T) { + appDir := tempDir(t) + + dirPath := filepath.Join(appDir, "test.dir") + err := os.MkdirAll(dirPath, 0644) + require.NoError(t, err) + linkPath := filepath.Join(appDir, "test.json") + err = os.Symlink(dirPath, linkPath) + require.NoError(t, err) + + walkFor(t, appDir, linkPath, func(info fs.FileInfo) { + realFileInfo, ignoreMessage, err := getPotentiallyValidManifestFile(linkPath, info, appDir, appDir, "", "") + assert.Nil(t, realFileInfo) + assert.Contains(t, ignoreMessage, "non-regular file") + assert.NoError(t, err) + }) + }) + + t.Run("non-included file should be skipped with no message", func(t *testing.T) { + appDir := tempDir(t) + + filePath := filepath.Join(appDir, "not-included.yaml") + file, err := os.OpenFile(filePath, os.O_RDONLY|os.O_CREATE, 0644) + require.NoError(t, err) + err = file.Close() + require.NoError(t, err) + + walkFor(t, appDir, filePath, func(info fs.FileInfo) { + realFileInfo, ignoreMessage, err := getPotentiallyValidManifestFile(filePath, info, appDir, appDir, "*.json", "") + assert.Nil(t, realFileInfo) + assert.Empty(t, ignoreMessage) + assert.NoError(t, err) + }) + }) + + t.Run("excluded file should be skipped with no message", func(t *testing.T) { + appDir := tempDir(t) + + filePath := filepath.Join(appDir, "excluded.json") + file, err := os.OpenFile(filePath, os.O_RDONLY|os.O_CREATE, 0644) + require.NoError(t, err) + err = file.Close() + require.NoError(t, err) + + walkFor(t, appDir, filePath, func(info fs.FileInfo) { + realFileInfo, ignoreMessage, err := getPotentiallyValidManifestFile(filePath, info, appDir, appDir, "", "excluded.*") + assert.Nil(t, realFileInfo) + assert.Empty(t, ignoreMessage) + assert.NoError(t, err) + }) + }) + + t.Run("symlink to a regular file is potentially valid", func(t *testing.T) { + appDir := tempDir(t) + + filePath := filepath.Join(appDir, "regular-file") + file, err := os.OpenFile(filePath, os.O_RDONLY|os.O_CREATE, 0644) + require.NoError(t, err) + err = file.Close() + require.NoError(t, err) + + linkPath := filepath.Join(appDir, "link.json") + err = os.Symlink(filePath, linkPath) + require.NoError(t, err) + + walkFor(t, appDir, linkPath, func(info fs.FileInfo) { + realFileInfo, ignoreMessage, err := getPotentiallyValidManifestFile(linkPath, info, appDir, appDir, "", "") + assert.NotNil(t, realFileInfo) + assert.Empty(t, ignoreMessage) + assert.NoError(t, err) + }) + }) + + t.Run("a regular file is potentially valid", func(t *testing.T) { + appDir := tempDir(t) + + filePath := filepath.Join(appDir, "regular-file.json") + file, err := os.OpenFile(filePath, os.O_RDONLY|os.O_CREATE, 0644) + require.NoError(t, err) + err = file.Close() + require.NoError(t, err) + + walkFor(t, appDir, filePath, func(info fs.FileInfo) { + realFileInfo, ignoreMessage, err := getPotentiallyValidManifestFile(filePath, info, appDir, appDir, "", "") + assert.NotNil(t, realFileInfo) + assert.Empty(t, ignoreMessage) + assert.NoError(t, err) + }) + }) + + t.Run("realFileInfo is for the destination rather than the symlink", func(t *testing.T) { + appDir := tempDir(t) + + filePath := filepath.Join(appDir, "regular-file") + file, err := os.OpenFile(filePath, os.O_RDONLY|os.O_CREATE, 0644) + require.NoError(t, err) + err = file.Close() + require.NoError(t, err) + + linkPath := filepath.Join(appDir, "link.json") + err = os.Symlink(filePath, linkPath) + require.NoError(t, err) + + walkFor(t, appDir, linkPath, func(info fs.FileInfo) { + realFileInfo, ignoreMessage, err := getPotentiallyValidManifestFile(linkPath, info, appDir, appDir, "", "") + assert.NotNil(t, realFileInfo) + assert.Equal(t, filepath.Base(filePath), realFileInfo.Name()) + assert.Empty(t, ignoreMessage) + assert.NoError(t, err) + }) + }) +} + +func Test_getPotentiallyValidManifests(t *testing.T) { + // Tests which return no manifests and an error check to make sure the directory exists before running. A missing + // directory would produce those same results. + + logCtx := log.WithField("test", "test") + + t.Run("unreadable file throws error", func(t *testing.T) { + appDir := t.TempDir() + unreadablePath := filepath.Join(appDir, "unreadable.json") + err := os.WriteFile(unreadablePath, []byte{}, 0666) + require.NoError(t, err) + err = os.Chmod(appDir, 0000) + require.NoError(t, err) + + manifests, err := getPotentiallyValidManifests(logCtx, appDir, appDir, false, "", "", resource.MustParse("0")) + assert.Empty(t, manifests) + assert.Error(t, err) + + // allow cleanup + err = os.Chmod(appDir, 0777) + if err != nil { + panic(err) + } + }) + + t.Run("no recursion when recursion is disabled", func(t *testing.T) { + manifests, err := getPotentiallyValidManifests(logCtx, "./testdata/recurse", "./testdata/recurse", false, "", "", resource.MustParse("0")) + assert.Len(t, manifests, 1) + assert.NoError(t, err) + }) + + t.Run("recursion when recursion is enabled", func(t *testing.T) { + manifests, err := getPotentiallyValidManifests(logCtx, "./testdata/recurse", "./testdata/recurse", true, "", "", resource.MustParse("0")) + assert.Len(t, manifests, 2) + assert.NoError(t, err) + }) + + t.Run("non-JSON/YAML is skipped", func(t *testing.T) { + manifests, err := getPotentiallyValidManifests(logCtx, "./testdata/non-manifest-file", "./testdata/non-manifest-file", false, "", "", resource.MustParse("0")) + assert.Empty(t, manifests) + assert.NoError(t, err) + }) + + t.Run("circular link should throw an error", func(t *testing.T) { + require.DirExists(t, "./testdata/circular-link") + manifests, err := getPotentiallyValidManifests(logCtx, "./testdata/circular-link", "./testdata/circular-link", false, "", "", resource.MustParse("0")) + assert.Empty(t, manifests) + assert.Error(t, err) + }) + + t.Run("out-of-bounds symlink should throw an error", func(t *testing.T) { + require.DirExists(t, "./testdata/out-of-bounds-link") + manifests, err := getPotentiallyValidManifests(logCtx, "./testdata/out-of-bounds-link", "./testdata/out-of-bounds-link", false, "", "", resource.MustParse("0")) + assert.Empty(t, manifests) + assert.Error(t, err) + }) + + t.Run("symlink to a regular file works", func(t *testing.T) { + repoRoot, err := filepath.Abs("./testdata/in-bounds-link") + require.NoError(t, err) + appPath, err := filepath.Abs("./testdata/in-bounds-link/app") + require.NoError(t, err) + manifests, err := getPotentiallyValidManifests(logCtx, appPath, repoRoot, false, "", "", resource.MustParse("0")) + assert.Len(t, manifests, 1) + assert.NoError(t, err) + }) + + t.Run("symlink to nowhere should be ignored", func(t *testing.T) { + manifests, err := getPotentiallyValidManifests(logCtx, "./testdata/link-to-nowhere", "./testdata/link-to-nowhere", false, "", "", resource.MustParse("0")) + assert.Empty(t, manifests) + assert.NoError(t, err) + }) + + t.Run("link to over-sized manifest fails", func(t *testing.T) { + repoRoot, err := filepath.Abs("./testdata/in-bounds-link") + require.NoError(t, err) + appPath, err := filepath.Abs("./testdata/in-bounds-link/app") + require.NoError(t, err) + // The file is 35 bytes. + manifests, err := getPotentiallyValidManifests(logCtx, appPath, repoRoot, false, "", "", resource.MustParse("34")) + assert.Empty(t, manifests) + assert.ErrorIs(t, err, ErrExceededMaxCombinedManifestFileSize) + }) + + t.Run("group of files should be limited at precisely the sum of their size", func(t *testing.T) { + // There is a total of 10 files, ech file being 10 bytes. + manifests, err := getPotentiallyValidManifests(logCtx, "./testdata/several-files", "./testdata/several-files", false, "", "", resource.MustParse("365")) + assert.Len(t, manifests, 10) + assert.NoError(t, err) + + manifests, err = getPotentiallyValidManifests(logCtx, "./testdata/several-files", "./testdata/several-files", false, "", "", resource.MustParse("100")) + assert.Empty(t, manifests) + assert.ErrorIs(t, err, ErrExceededMaxCombinedManifestFileSize) + }) +} + +func Test_findManifests(t *testing.T) { + logCtx := log.WithField("test", "test") + noRecurse := argoappv1.ApplicationSourceDirectory{Recurse: false} + + t.Run("unreadable file throws error", func(t *testing.T) { + appDir := t.TempDir() + unreadablePath := filepath.Join(appDir, "unreadable.json") + err := os.WriteFile(unreadablePath, []byte{}, 0666) + require.NoError(t, err) + err = os.Chmod(appDir, 0000) + require.NoError(t, err) + + manifests, err := findManifests(logCtx, appDir, appDir, nil, noRecurse, nil, resource.MustParse("0")) + assert.Empty(t, manifests) + assert.Error(t, err) + + // allow cleanup + err = os.Chmod(appDir, 0777) + if err != nil { + panic(err) + } + }) + + t.Run("no recursion when recursion is disabled", func(t *testing.T) { + manifests, err := findManifests(logCtx, "./testdata/recurse", "./testdata/recurse", nil, noRecurse, nil, resource.MustParse("0")) + assert.Len(t, manifests, 2) + assert.NoError(t, err) + }) + + t.Run("recursion when recursion is enabled", func(t *testing.T) { + recurse := argoappv1.ApplicationSourceDirectory{Recurse: true} + manifests, err := findManifests(logCtx, "./testdata/recurse", "./testdata/recurse", nil, recurse, nil, resource.MustParse("0")) + assert.Len(t, manifests, 4) + assert.NoError(t, err) + }) + + t.Run("non-JSON/YAML is skipped", func(t *testing.T) { + manifests, err := findManifests(logCtx, "./testdata/non-manifest-file", "./testdata/non-manifest-file", nil, noRecurse, nil, resource.MustParse("0")) + assert.Empty(t, manifests) + assert.NoError(t, err) + }) + + t.Run("circular link should throw an error", func(t *testing.T) { + require.DirExists(t, "./testdata/circular-link") + manifests, err := findManifests(logCtx, "./testdata/circular-link", "./testdata/circular-link", nil, noRecurse, nil, resource.MustParse("0")) + assert.Empty(t, manifests) + assert.Error(t, err) + }) + + t.Run("out-of-bounds symlink should throw an error", func(t *testing.T) { + require.DirExists(t, "./testdata/out-of-bounds-link") + manifests, err := findManifests(logCtx, "./testdata/out-of-bounds-link", "./testdata/out-of-bounds-link", nil, noRecurse, nil, resource.MustParse("0")) + assert.Empty(t, manifests) + assert.Error(t, err) + }) + + t.Run("symlink to a regular file works", func(t *testing.T) { + repoRoot, err := filepath.Abs("./testdata/in-bounds-link") + require.NoError(t, err) + appPath, err := filepath.Abs("./testdata/in-bounds-link/app") + require.NoError(t, err) + manifests, err := findManifests(logCtx, appPath, repoRoot, nil, noRecurse, nil, resource.MustParse("0")) + assert.Len(t, manifests, 1) + assert.NoError(t, err) + }) + + t.Run("symlink to nowhere should be ignored", func(t *testing.T) { + manifests, err := findManifests(logCtx, "./testdata/link-to-nowhere", "./testdata/link-to-nowhere", nil, noRecurse, nil, resource.MustParse("0")) + assert.Empty(t, manifests) + assert.NoError(t, err) + }) + + t.Run("link to over-sized manifest fails", func(t *testing.T) { + repoRoot, err := filepath.Abs("./testdata/in-bounds-link") + require.NoError(t, err) + appPath, err := filepath.Abs("./testdata/in-bounds-link/app") + require.NoError(t, err) + // The file is 35 bytes. + manifests, err := findManifests(logCtx, appPath, repoRoot, nil, noRecurse, nil, resource.MustParse("34")) + assert.Empty(t, manifests) + assert.ErrorIs(t, err, ErrExceededMaxCombinedManifestFileSize) + }) + + t.Run("group of files should be limited at precisely the sum of their size", func(t *testing.T) { + // There is a total of 10 files, each file being 10 bytes. + manifests, err := findManifests(logCtx, "./testdata/several-files", "./testdata/several-files", nil, noRecurse, nil, resource.MustParse("365")) + assert.Len(t, manifests, 10) + assert.NoError(t, err) + + manifests, err = findManifests(logCtx, "./testdata/several-files", "./testdata/several-files", nil, noRecurse, nil, resource.MustParse("364")) + assert.Empty(t, manifests) + assert.ErrorIs(t, err, ErrExceededMaxCombinedManifestFileSize) + }) + + t.Run("jsonnet isn't counted against size limit", func(t *testing.T) { + // Each file is 36 bytes. Only the 36-byte json file should be counted against the limit. + manifests, err := findManifests(logCtx, "./testdata/jsonnet-and-json", "./testdata/jsonnet-and-json", nil, noRecurse, nil, resource.MustParse("36")) + assert.Len(t, manifests, 2) + assert.NoError(t, err) + + manifests, err = findManifests(logCtx, "./testdata/jsonnet-and-json", "./testdata/jsonnet-and-json", nil, noRecurse, nil, resource.MustParse("35")) + assert.Empty(t, manifests) + assert.ErrorIs(t, err, ErrExceededMaxCombinedManifestFileSize) + }) + + t.Run("partially valid YAML file throws an error", func(t *testing.T) { + require.DirExists(t, "./testdata/partially-valid-yaml") + manifests, err := findManifests(logCtx, "./testdata/partially-valid-yaml", "./testdata/partially-valid-yaml", nil, noRecurse, nil, resource.MustParse("0")) + assert.Empty(t, manifests) + assert.Error(t, err) + }) + + t.Run("invalid manifest throws an error", func(t *testing.T) { + require.DirExists(t, "./testdata/invalid-manifests") + manifests, err := findManifests(logCtx, "./testdata/invalid-manifests", "./testdata/invalid-manifests", nil, noRecurse, nil, resource.MustParse("0")) + assert.Empty(t, manifests) + assert.Error(t, err) + }) + + t.Run("irrelevant YAML gets skipped, relevant YAML gets parsed", func(t *testing.T) { + manifests, err := findManifests(logCtx, "./testdata/irrelevant-yaml", "./testdata/irrelevant-yaml", nil, noRecurse, nil, resource.MustParse("0")) + assert.Len(t, manifests, 1) + assert.NoError(t, err) + }) + + t.Run("multiple JSON objects in one file throws an error", func(t *testing.T) { + require.DirExists(t, "./testdata/json-list") + manifests, err := findManifests(logCtx, "./testdata/json-list", "./testdata/json-list", nil, noRecurse, nil, resource.MustParse("0")) + assert.Empty(t, manifests) + assert.Error(t, err) + }) + + t.Run("invalid JSON throws an error", func(t *testing.T) { + require.DirExists(t, "./testdata/invalid-json") + manifests, err := findManifests(logCtx, "./testdata/invalid-json", "./testdata/invalid-json", nil, noRecurse, nil, resource.MustParse("0")) + assert.Empty(t, manifests) + assert.Error(t, err) + }) + + t.Run("valid JSON returns manifest and no error", func(t *testing.T) { + manifests, err := findManifests(logCtx, "./testdata/valid-json", "./testdata/valid-json", nil, noRecurse, nil, resource.MustParse("0")) + assert.Len(t, manifests, 1) + assert.NoError(t, err) + }) + + t.Run("YAML with an empty document doesn't throw an error", func(t *testing.T) { + manifests, err := findManifests(logCtx, "./testdata/yaml-with-empty-document", "./testdata/yaml-with-empty-document", nil, noRecurse, nil, resource.MustParse("0")) + assert.Len(t, manifests, 1) + assert.NoError(t, err) + }) +} + func TestTestRepoOCI(t *testing.T) { service := newService(".") _, err := service.TestRepository(context.Background(), &apiclient.TestRepositoryRequest{ @@ -1864,3 +2382,52 @@ func runGit(t *testing.T, workDir string, args ...string) string { require.NoError(t, err, stringOut) return stringOut } + +func Test_findHelmValueFilesInPath(t *testing.T) { + t.Run("does not exist", func(t *testing.T) { + files, err := findHelmValueFilesInPath("/obviously/does/not/exist") + assert.Error(t, err) + assert.Empty(t, files) + }) + t.Run("values files", func(t *testing.T) { + files, err := findHelmValueFilesInPath("./testdata/values-files") + assert.NoError(t, err) + assert.Len(t, files, 4) + }) +} + +func Test_populateHelmAppDetails(t *testing.T) { + res := apiclient.RepoAppDetailsResponse{} + q := apiclient.RepoServerAppDetailsQuery{ + Repo: &argoappv1.Repository{}, + Source: &argoappv1.ApplicationSource{ + Helm: &argoappv1.ApplicationSourceHelm{ValueFiles: []string{"exclude.yaml", "has-the-word-values.yaml"}}, + }, + } + appPath, err := filepath.Abs("./testdata/values-files/") + require.NoError(t, err) + err = populateHelmAppDetails(&res, appPath, appPath, &q) + require.NoError(t, err) + assert.Len(t, res.Helm.Parameters, 3) + assert.Len(t, res.Helm.ValueFiles, 4) +} + +func Test_populateHelmAppDetails_values_symlinks(t *testing.T) { + t.Run("inbound", func(t *testing.T) { + res := apiclient.RepoAppDetailsResponse{} + q := apiclient.RepoServerAppDetailsQuery{Repo: &argoappv1.Repository{}, Source: &argoappv1.ApplicationSource{}} + err := populateHelmAppDetails(&res, "./testdata/in-bounds-values-file-link/", "./testdata/in-bounds-values-file-link/", &q) + require.NoError(t, err) + assert.NotEmpty(t, res.Helm.Values) + assert.NotEmpty(t, res.Helm.Parameters) + }) + + t.Run("out of bounds", func(t *testing.T) { + res := apiclient.RepoAppDetailsResponse{} + q := apiclient.RepoServerAppDetailsQuery{Repo: &argoappv1.Repository{}, Source: &argoappv1.ApplicationSource{}} + err := populateHelmAppDetails(&res, "./testdata/out-of-bounds-values-file-link/", "./testdata/out-of-bounds-values-file-link/", &q) + require.NoError(t, err) + assert.Empty(t, res.Helm.Values) + assert.Empty(t, res.Helm.Parameters) + }) +} diff --git a/reposerver/repository/testdata/circular-link/a.json b/reposerver/repository/testdata/circular-link/a.json new file mode 120000 index 000000000000..ce080bc5ebcc --- /dev/null +++ b/reposerver/repository/testdata/circular-link/a.json @@ -0,0 +1 @@ +b.json \ No newline at end of file diff --git a/reposerver/repository/testdata/circular-link/b.json b/reposerver/repository/testdata/circular-link/b.json new file mode 120000 index 000000000000..8eacec086127 --- /dev/null +++ b/reposerver/repository/testdata/circular-link/b.json @@ -0,0 +1 @@ +a.json \ No newline at end of file diff --git a/reposerver/repository/testdata/in-bounds-link/app/cm.link.yaml b/reposerver/repository/testdata/in-bounds-link/app/cm.link.yaml new file mode 120000 index 000000000000..ecb1855f4775 --- /dev/null +++ b/reposerver/repository/testdata/in-bounds-link/app/cm.link.yaml @@ -0,0 +1 @@ +../cm.yaml \ No newline at end of file diff --git a/reposerver/repository/testdata/in-bounds-link/cm.yaml b/reposerver/repository/testdata/in-bounds-link/cm.yaml new file mode 100644 index 000000000000..e4ec2f4d22c2 --- /dev/null +++ b/reposerver/repository/testdata/in-bounds-link/cm.yaml @@ -0,0 +1,2 @@ +apiVersion: v1 +kind: ServiceAccount \ No newline at end of file diff --git a/reposerver/repository/testdata/in-bounds-values-file-link/Chart.yaml b/reposerver/repository/testdata/in-bounds-values-file-link/Chart.yaml new file mode 100644 index 000000000000..f01dd5407e5a --- /dev/null +++ b/reposerver/repository/testdata/in-bounds-values-file-link/Chart.yaml @@ -0,0 +1,2 @@ +name: my-chart +version: 1.1.0 diff --git a/reposerver/repository/testdata/in-bounds-values-file-link/values-2.yaml b/reposerver/repository/testdata/in-bounds-values-file-link/values-2.yaml new file mode 100644 index 000000000000..aad07b2bfc83 --- /dev/null +++ b/reposerver/repository/testdata/in-bounds-values-file-link/values-2.yaml @@ -0,0 +1 @@ +some: yaml diff --git a/reposerver/repository/testdata/in-bounds-values-file-link/values.yaml b/reposerver/repository/testdata/in-bounds-values-file-link/values.yaml new file mode 120000 index 000000000000..d860891957ff --- /dev/null +++ b/reposerver/repository/testdata/in-bounds-values-file-link/values.yaml @@ -0,0 +1 @@ +values-2.yaml \ No newline at end of file diff --git a/reposerver/repository/testdata/invalid-json/invalid.json b/reposerver/repository/testdata/invalid-json/invalid.json new file mode 100644 index 000000000000..558ed37d93c5 --- /dev/null +++ b/reposerver/repository/testdata/invalid-json/invalid.json @@ -0,0 +1 @@ +[ diff --git a/reposerver/repository/testdata/irrelevant-yaml/irrelevant.yaml b/reposerver/repository/testdata/irrelevant-yaml/irrelevant.yaml new file mode 100644 index 000000000000..2f0be7139258 --- /dev/null +++ b/reposerver/repository/testdata/irrelevant-yaml/irrelevant.yaml @@ -0,0 +1 @@ +some: [irrelevant, yaml] diff --git a/reposerver/repository/testdata/irrelevant-yaml/relevant.yaml b/reposerver/repository/testdata/irrelevant-yaml/relevant.yaml new file mode 100644 index 000000000000..c66c31b731c7 --- /dev/null +++ b/reposerver/repository/testdata/irrelevant-yaml/relevant.yaml @@ -0,0 +1,2 @@ +apiVersion: v1 +kind: ConfigMap diff --git a/reposerver/repository/testdata/json-list/list.json b/reposerver/repository/testdata/json-list/list.json new file mode 100644 index 000000000000..75da3b904143 --- /dev/null +++ b/reposerver/repository/testdata/json-list/list.json @@ -0,0 +1,2 @@ +{"apiVersion": "v1", "kind": "ConfigMap"} +{"apiVersion": "v1", "kind": "ConfigMap"} diff --git a/reposerver/repository/testdata/jsonnet-and-json/test.json b/reposerver/repository/testdata/jsonnet-and-json/test.json new file mode 100644 index 000000000000..f2a3121863cd --- /dev/null +++ b/reposerver/repository/testdata/jsonnet-and-json/test.json @@ -0,0 +1 @@ +{"apiVersion": "v1", "kind": "Pod"} diff --git a/reposerver/repository/testdata/jsonnet-and-json/test.jsonnet b/reposerver/repository/testdata/jsonnet-and-json/test.jsonnet new file mode 100644 index 000000000000..f2a3121863cd --- /dev/null +++ b/reposerver/repository/testdata/jsonnet-and-json/test.jsonnet @@ -0,0 +1 @@ +{"apiVersion": "v1", "kind": "Pod"} diff --git a/reposerver/repository/testdata/link-to-nowhere/nowhere.json b/reposerver/repository/testdata/link-to-nowhere/nowhere.json new file mode 120000 index 000000000000..5425ec0feb1e --- /dev/null +++ b/reposerver/repository/testdata/link-to-nowhere/nowhere.json @@ -0,0 +1 @@ +nowhere \ No newline at end of file diff --git a/reposerver/repository/testdata/non-manifest-file/not-json-or-yaml b/reposerver/repository/testdata/non-manifest-file/not-json-or-yaml new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/reposerver/repository/testdata/out-of-bounds-link/out-of-bounds.json b/reposerver/repository/testdata/out-of-bounds-link/out-of-bounds.json new file mode 120000 index 000000000000..828c1b4cd37d --- /dev/null +++ b/reposerver/repository/testdata/out-of-bounds-link/out-of-bounds.json @@ -0,0 +1 @@ +../out-of-bounds.json \ No newline at end of file diff --git a/reposerver/repository/testdata/out-of-bounds-values-file-link/Chart.yaml b/reposerver/repository/testdata/out-of-bounds-values-file-link/Chart.yaml new file mode 100644 index 000000000000..f01dd5407e5a --- /dev/null +++ b/reposerver/repository/testdata/out-of-bounds-values-file-link/Chart.yaml @@ -0,0 +1,2 @@ +name: my-chart +version: 1.1.0 diff --git a/reposerver/repository/testdata/out-of-bounds-values-file-link/values.yaml b/reposerver/repository/testdata/out-of-bounds-values-file-link/values.yaml new file mode 120000 index 000000000000..4e034a19440a --- /dev/null +++ b/reposerver/repository/testdata/out-of-bounds-values-file-link/values.yaml @@ -0,0 +1 @@ +../out-of-bounds.yaml \ No newline at end of file diff --git a/reposerver/repository/testdata/out-of-bounds.json b/reposerver/repository/testdata/out-of-bounds.json new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/reposerver/repository/testdata/out-of-bounds.yaml b/reposerver/repository/testdata/out-of-bounds.yaml new file mode 100644 index 000000000000..aad07b2bfc83 --- /dev/null +++ b/reposerver/repository/testdata/out-of-bounds.yaml @@ -0,0 +1 @@ +some: yaml diff --git a/reposerver/repository/testdata/partially-valid-yaml/partially-valid.yaml b/reposerver/repository/testdata/partially-valid-yaml/partially-valid.yaml new file mode 100644 index 000000000000..a57a81a9dca5 --- /dev/null +++ b/reposerver/repository/testdata/partially-valid-yaml/partially-valid.yaml @@ -0,0 +1,4 @@ +apiVersion: v1 +kind: ConfigMap +--- +invalid: diff --git a/reposerver/repository/testdata/several-files/0.json b/reposerver/repository/testdata/several-files/0.json new file mode 100644 index 000000000000..dbcf15c7b9b0 --- /dev/null +++ b/reposerver/repository/testdata/several-files/0.json @@ -0,0 +1 @@ +{"apiVersion": "v1", "kind": "ConfigMap"} diff --git a/reposerver/repository/testdata/several-files/0.yaml b/reposerver/repository/testdata/several-files/0.yaml new file mode 100644 index 000000000000..c66c31b731c7 --- /dev/null +++ b/reposerver/repository/testdata/several-files/0.yaml @@ -0,0 +1,2 @@ +apiVersion: v1 +kind: ConfigMap diff --git a/reposerver/repository/testdata/several-files/1.json b/reposerver/repository/testdata/several-files/1.json new file mode 100644 index 000000000000..dbcf15c7b9b0 --- /dev/null +++ b/reposerver/repository/testdata/several-files/1.json @@ -0,0 +1 @@ +{"apiVersion": "v1", "kind": "ConfigMap"} diff --git a/reposerver/repository/testdata/several-files/1.yaml b/reposerver/repository/testdata/several-files/1.yaml new file mode 100644 index 000000000000..c66c31b731c7 --- /dev/null +++ b/reposerver/repository/testdata/several-files/1.yaml @@ -0,0 +1,2 @@ +apiVersion: v1 +kind: ConfigMap diff --git a/reposerver/repository/testdata/several-files/2.json b/reposerver/repository/testdata/several-files/2.json new file mode 100644 index 000000000000..dbcf15c7b9b0 --- /dev/null +++ b/reposerver/repository/testdata/several-files/2.json @@ -0,0 +1 @@ +{"apiVersion": "v1", "kind": "ConfigMap"} diff --git a/reposerver/repository/testdata/several-files/2.yaml b/reposerver/repository/testdata/several-files/2.yaml new file mode 100644 index 000000000000..c66c31b731c7 --- /dev/null +++ b/reposerver/repository/testdata/several-files/2.yaml @@ -0,0 +1,2 @@ +apiVersion: v1 +kind: ConfigMap diff --git a/reposerver/repository/testdata/several-files/3.json b/reposerver/repository/testdata/several-files/3.json new file mode 100644 index 000000000000..dbcf15c7b9b0 --- /dev/null +++ b/reposerver/repository/testdata/several-files/3.json @@ -0,0 +1 @@ +{"apiVersion": "v1", "kind": "ConfigMap"} diff --git a/reposerver/repository/testdata/several-files/3.yaml b/reposerver/repository/testdata/several-files/3.yaml new file mode 100644 index 000000000000..c66c31b731c7 --- /dev/null +++ b/reposerver/repository/testdata/several-files/3.yaml @@ -0,0 +1,2 @@ +apiVersion: v1 +kind: ConfigMap diff --git a/reposerver/repository/testdata/several-files/4.json b/reposerver/repository/testdata/several-files/4.json new file mode 100644 index 000000000000..dbcf15c7b9b0 --- /dev/null +++ b/reposerver/repository/testdata/several-files/4.json @@ -0,0 +1 @@ +{"apiVersion": "v1", "kind": "ConfigMap"} diff --git a/reposerver/repository/testdata/several-files/4.yaml b/reposerver/repository/testdata/several-files/4.yaml new file mode 100644 index 000000000000..c66c31b731c7 --- /dev/null +++ b/reposerver/repository/testdata/several-files/4.yaml @@ -0,0 +1,2 @@ +apiVersion: v1 +kind: ConfigMap diff --git a/reposerver/repository/testdata/several-files/README.md b/reposerver/repository/testdata/several-files/README.md new file mode 100644 index 000000000000..b4a1e1f9aa20 --- /dev/null +++ b/reposerver/repository/testdata/several-files/README.md @@ -0,0 +1 @@ +This file shouldn't be counted in the manifest file size limit, because it isn't JSON or YAML. diff --git a/reposerver/repository/testdata/valid-json/valid.json b/reposerver/repository/testdata/valid-json/valid.json new file mode 100644 index 000000000000..dbcf15c7b9b0 --- /dev/null +++ b/reposerver/repository/testdata/valid-json/valid.json @@ -0,0 +1 @@ +{"apiVersion": "v1", "kind": "ConfigMap"} diff --git a/reposerver/repository/testdata/values-files/Chart.yaml b/reposerver/repository/testdata/values-files/Chart.yaml new file mode 100644 index 000000000000..0c9526bab8cc --- /dev/null +++ b/reposerver/repository/testdata/values-files/Chart.yaml @@ -0,0 +1,2 @@ +name: my-chart +version: 1.1.0 \ No newline at end of file diff --git a/reposerver/repository/testdata/values-files/caps-extn-values.YAML b/reposerver/repository/testdata/values-files/caps-extn-values.YAML new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/reposerver/repository/testdata/values-files/exclude.yaml b/reposerver/repository/testdata/values-files/exclude.yaml new file mode 100644 index 000000000000..c9bb22cd260f --- /dev/null +++ b/reposerver/repository/testdata/values-files/exclude.yaml @@ -0,0 +1 @@ +exclude: yaml diff --git a/reposerver/repository/testdata/values-files/has-the-word-values.yaml b/reposerver/repository/testdata/values-files/has-the-word-values.yaml new file mode 100644 index 000000000000..911b485e5ef9 --- /dev/null +++ b/reposerver/repository/testdata/values-files/has-the-word-values.yaml @@ -0,0 +1,4 @@ +has: + the: + word: + values: yaml diff --git a/reposerver/repository/testdata/values-files/values.yaml b/reposerver/repository/testdata/values-files/values.yaml new file mode 100644 index 000000000000..55262d50ff71 --- /dev/null +++ b/reposerver/repository/testdata/values-files/values.yaml @@ -0,0 +1 @@ +values: yaml diff --git a/reposerver/repository/testdata/values-files/values.yml b/reposerver/repository/testdata/values-files/values.yml new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/reposerver/repository/testdata/yaml-with-empty-document/has-empty.yaml b/reposerver/repository/testdata/yaml-with-empty-document/has-empty.yaml new file mode 100644 index 000000000000..20335c7ff509 --- /dev/null +++ b/reposerver/repository/testdata/yaml-with-empty-document/has-empty.yaml @@ -0,0 +1,4 @@ +apiVersion: v1 +kind: ConfigMap +--- +--- diff --git a/server/server.go b/server/server.go index 921f47ce0946..d8b43526956d 100644 --- a/server/server.go +++ b/server/server.go @@ -951,6 +951,9 @@ func (a *ArgoCDServer) Authenticate(ctx context.Context) (context.Context, error } if !argoCDSettings.AnonymousUserEnabled { return ctx, claimsErr + } else { + // nolint:staticcheck + ctx = context.WithValue(ctx, "claims", "") } } diff --git a/server/server_test.go b/server/server_test.go index fba450acb539..c28cc2ab6417 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -3,6 +3,8 @@ package server import ( "context" "fmt" + "io" + "net/http" "net/http/httptest" "strings" "testing" @@ -12,6 +14,7 @@ import ( "github.com/golang-jwt/jwt/v4" log "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "google.golang.org/grpc/metadata" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes/fake" @@ -432,6 +435,396 @@ func TestAuthenticate(t *testing.T) { } } +func dexMockHandler(t *testing.T, url string) func(http.ResponseWriter, *http.Request) { + return func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.RequestURI { + case "/api/dex/.well-known/openid-configuration": + _, err := io.WriteString(w, fmt.Sprintf(` +{ + "issuer": "%[1]s/api/dex", + "authorization_endpoint": "%[1]s/api/dex/auth", + "token_endpoint": "%[1]s/api/dex/token", + "jwks_uri": "%[1]s/api/dex/keys", + "userinfo_endpoint": "%[1]s/api/dex/userinfo", + "device_authorization_endpoint": "%[1]s/api/dex/device/code", + "grant_types_supported": [ + "authorization_code", + "refresh_token", + "urn:ietf:params:oauth:grant-type:device_code" + ], + "response_types_supported": [ + "code" + ], + "subject_types_supported": [ + "public" + ], + "id_token_signing_alg_values_supported": [ + "RS256", "HS256" + ], + "code_challenge_methods_supported": [ + "S256", + "plain" + ], + "scopes_supported": [ + "openid", + "email", + "groups", + "profile", + "offline_access" + ], + "token_endpoint_auth_methods_supported": [ + "client_secret_basic", + "client_secret_post" + ], + "claims_supported": [ + "iss", + "sub", + "aud", + "iat", + "exp", + "email", + "email_verified", + "locale", + "name", + "preferred_username", + "at_hash" + ] +}`, url)) + if err != nil { + t.Fail() + } + default: + w.WriteHeader(404) + } + } +} + +func getTestServer(t *testing.T, anonymousEnabled bool, withFakeSSO bool) (argocd *ArgoCDServer, dexURL string) { + cm := test.NewFakeConfigMap() + if anonymousEnabled { + cm.Data["users.anonymous.enabled"] = "true" + } + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Start with a placeholder. We need the server URL before setting up the real handler. + })) + ts.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + dexMockHandler(t, ts.URL)(w, r) + }) + if withFakeSSO { + cm.Data["url"] = ts.URL + cm.Data["dex.config"] = ` +connectors: + # OIDC + - type: OIDC + id: oidc + name: OIDC + config: + issuer: https://auth.example.gom + clientID: test-client + clientSecret: $dex.oidc.clientSecret` + } + secret := test.NewFakeSecret() + kubeclientset := fake.NewSimpleClientset(cm, secret) + appClientSet := apps.NewSimpleClientset() + argoCDOpts := ArgoCDServerOpts{ + Namespace: test.FakeArgoCDNamespace, + KubeClientset: kubeclientset, + AppClientset: appClientSet, + } + if withFakeSSO { + argoCDOpts.DexServerAddr = ts.URL + } + argocd = NewServer(context.Background(), argoCDOpts) + return argocd, ts.URL +} + +func TestAuthenticate_3rd_party_JWTs(t *testing.T) { + type testData struct { + test string + anonymousEnabled bool + claims jwt.RegisteredClaims + expectedErrorContains string + expectedClaims interface{} + } + var tests = []testData{ + { + test: "anonymous disabled, no audience", + anonymousEnabled: false, + claims: jwt.RegisteredClaims{}, + expectedErrorContains: "no audience found in the token", + expectedClaims: nil, + }, + { + test: "anonymous enabled, no audience", + anonymousEnabled: true, + claims: jwt.RegisteredClaims{}, + expectedErrorContains: "", + expectedClaims: "", + }, + { + test: "anonymous disabled, unexpired token, admin claim", + anonymousEnabled: false, + claims: jwt.RegisteredClaims{Audience: jwt.ClaimStrings{"test-client"}, Subject: "admin", ExpiresAt: jwt.NewNumericDate(time.Now().Add(time.Hour * 24))}, + expectedErrorContains: "id token signed with unsupported algorithm", + expectedClaims: nil, + }, + { + test: "anonymous enabled, unexpired token, admin claim", + anonymousEnabled: true, + claims: jwt.RegisteredClaims{Audience: jwt.ClaimStrings{"test-client"}, Subject: "admin", ExpiresAt: jwt.NewNumericDate(time.Now().Add(time.Hour * 24))}, + expectedErrorContains: "", + expectedClaims: "", + }, + { + test: "anonymous disabled, expired token, admin claim", + anonymousEnabled: false, + claims: jwt.RegisteredClaims{Audience: jwt.ClaimStrings{"test-client"}, Subject: "admin", ExpiresAt: jwt.NewNumericDate(time.Now())}, + expectedErrorContains: "token is expired", + expectedClaims: jwt.RegisteredClaims{Issuer:"sso"}, + }, + { + test: "anonymous enabled, expired token, admin claim", + anonymousEnabled: true, + claims: jwt.RegisteredClaims{Audience: jwt.ClaimStrings{"test-client"}, Subject: "admin", ExpiresAt: jwt.NewNumericDate(time.Now())}, + expectedErrorContains: "", + expectedClaims: "", + }, + } + + for _, testData := range tests { + testDataCopy := testData + + t.Run(testDataCopy.test, func(t *testing.T) { + t.Parallel() + + // Must be declared here to avoid race. + ctx := context.Background() //nolint:ineffassign,staticcheck + + argocd, dexURL := getTestServer(t, testDataCopy.anonymousEnabled, true) + testDataCopy.claims.Issuer = fmt.Sprintf("%s/api/dex", dexURL) + token := jwt.NewWithClaims(jwt.SigningMethodHS256, testDataCopy.claims) + tokenString, err := token.SignedString([]byte("key")) + require.NoError(t, err) + ctx = metadata.NewIncomingContext(context.Background(), metadata.Pairs(apiclient.MetaDataTokenKey, tokenString)) + + ctx, err = argocd.Authenticate(ctx) + claims := ctx.Value("claims") + if testDataCopy.expectedClaims == nil { + assert.Nil(t, claims) + } else { + assert.Equal(t, testDataCopy.expectedClaims, claims) + } + if testDataCopy.expectedErrorContains != "" { + assert.Error(t, err, "Authenticate should have thrown an error and blocked the request") + assert.Contains(t, err.Error(), testDataCopy.expectedErrorContains) + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestAuthenticate_no_request_metadata(t *testing.T) { + type testData struct { + test string + anonymousEnabled bool + expectedErrorContains string + expectedClaims interface{} + } + var tests = []testData{ + { + test: "anonymous disabled", + anonymousEnabled: false, + expectedErrorContains: "no session information", + expectedClaims: nil, + }, + { + test: "anonymous enabled", + anonymousEnabled: true, + expectedErrorContains: "", + expectedClaims: "", + }, + } + + for _, testData := range tests { + testDataCopy := testData + + t.Run(testDataCopy.test, func(t *testing.T) { + t.Parallel() + + argocd, _ := getTestServer(t, testDataCopy.anonymousEnabled, true) + ctx := context.Background() + + ctx, err := argocd.Authenticate(ctx) + claims := ctx.Value("claims") + assert.Equal(t, testDataCopy.expectedClaims, claims) + if testDataCopy.expectedErrorContains != "" { + assert.Error(t, err, "Authenticate should have thrown an error and blocked the request") + assert.Contains(t, err.Error(), testDataCopy.expectedErrorContains) + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestAuthenticate_no_SSO(t *testing.T) { + type testData struct { + test string + anonymousEnabled bool + expectedErrorMessage string + expectedClaims interface{} + } + var tests = []testData{ + { + test: "anonymous disabled", + anonymousEnabled: false, + expectedErrorMessage: "SSO is not configured", + expectedClaims: nil, + }, + { + test: "anonymous enabled", + anonymousEnabled: true, + expectedErrorMessage: "", + expectedClaims: "", + }, + } + + for _, testData := range tests { + testDataCopy := testData + + t.Run(testDataCopy.test, func(t *testing.T) { + t.Parallel() + + // Must be declared here to avoid race. + ctx := context.Background() //nolint:ineffassign,staticcheck + + argocd, dexURL := getTestServer(t, testDataCopy.anonymousEnabled, false) + token := jwt.NewWithClaims(jwt.SigningMethodHS256, jwt.RegisteredClaims{Issuer: fmt.Sprintf("%s/api/dex", dexURL)}) + tokenString, err := token.SignedString([]byte("key")) + require.NoError(t, err) + ctx = metadata.NewIncomingContext(context.Background(), metadata.Pairs(apiclient.MetaDataTokenKey, tokenString)) + + ctx, err = argocd.Authenticate(ctx) + claims := ctx.Value("claims") + assert.Equal(t, testDataCopy.expectedClaims, claims) + if testDataCopy.expectedErrorMessage != "" { + assert.Error(t, err, "Authenticate should have thrown an error and blocked the request") + assert.Contains(t, err.Error(), testDataCopy.expectedErrorMessage) + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestAuthenticate_bad_request_metadata(t *testing.T) { + type testData struct { + test string + anonymousEnabled bool + metadata metadata.MD + expectedErrorMessage string + expectedClaims interface{} + } + var tests = []testData{ + { + test: "anonymous disabled, empty metadata", + anonymousEnabled: false, + metadata: metadata.MD{}, + expectedErrorMessage: "no session information", + expectedClaims: nil, + }, + { + test: "anonymous enabled, empty metadata", + anonymousEnabled: true, + metadata: metadata.MD{}, + expectedErrorMessage: "", + expectedClaims: "", + }, + { + test: "anonymous disabled, empty tokens", + anonymousEnabled: false, + metadata: metadata.MD{apiclient.MetaDataTokenKey: []string{}}, + expectedErrorMessage: "no session information", + expectedClaims: nil, + }, + { + test: "anonymous enabled, empty tokens", + anonymousEnabled: true, + metadata: metadata.MD{apiclient.MetaDataTokenKey: []string{}}, + expectedErrorMessage: "", + expectedClaims: "", + }, + { + test: "anonymous disabled, bad tokens", + anonymousEnabled: false, + metadata: metadata.Pairs(apiclient.MetaDataTokenKey, "bad"), + expectedErrorMessage: "token contains an invalid number of segments", + expectedClaims: nil, + }, + { + test: "anonymous enabled, bad tokens", + anonymousEnabled: true, + metadata: metadata.Pairs(apiclient.MetaDataTokenKey, "bad"), + expectedErrorMessage: "", + expectedClaims: "", + }, + { + test: "anonymous disabled, bad auth header", + anonymousEnabled: false, + metadata: metadata.MD{"authorization": []string{"Bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJhZG1pbiJ9.TGGTTHuuGpEU8WgobXxkrBtW3NiR3dgw5LR-1DEW3BQ"}}, + expectedErrorMessage: "no audience found in the token", + expectedClaims: nil, + }, + { + test: "anonymous enabled, bad auth header", + anonymousEnabled: true, + metadata: metadata.MD{"authorization": []string{"Bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJhZG1pbiJ9.TGGTTHuuGpEU8WgobXxkrBtW3NiR3dgw5LR-1DEW3BQ"}}, + expectedErrorMessage: "", + expectedClaims: "", + }, + { + test: "anonymous disabled, bad auth cookie", + anonymousEnabled: false, + metadata: metadata.MD{"grpcgateway-cookie": []string{"argocd.token=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJhZG1pbiJ9.TGGTTHuuGpEU8WgobXxkrBtW3NiR3dgw5LR-1DEW3BQ"}}, + expectedErrorMessage: "no audience found in the token", + expectedClaims: nil, + }, + { + test: "anonymous enabled, bad auth cookie", + anonymousEnabled: true, + metadata: metadata.MD{"grpcgateway-cookie": []string{"argocd.token=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJhZG1pbiJ9.TGGTTHuuGpEU8WgobXxkrBtW3NiR3dgw5LR-1DEW3BQ"}}, + expectedErrorMessage: "", + expectedClaims: "", + }, + } + + for _, testData := range tests { + testDataCopy := testData + + t.Run(testDataCopy.test, func(t *testing.T) { + t.Parallel() + + // Must be declared here to avoid race. + ctx := context.Background() //nolint:ineffassign,staticcheck + + argocd, _ := getTestServer(t, testDataCopy.anonymousEnabled, true) + ctx = metadata.NewIncomingContext(context.Background(), testDataCopy.metadata) + + ctx, err := argocd.Authenticate(ctx) + claims := ctx.Value("claims") + assert.Equal(t, testDataCopy.expectedClaims, claims) + if testDataCopy.expectedErrorMessage != "" { + assert.Error(t, err, "Authenticate should have thrown an error and blocked the request") + assert.Contains(t, err.Error(), testDataCopy.expectedErrorMessage) + } else { + assert.NoError(t, err) + } + }) + } +} + func Test_getToken(t *testing.T) { token := "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c" t.Run("Empty", func(t *testing.T) { diff --git a/test/container/Dockerfile b/test/container/Dockerfile index 3ef69dd1eec3..5c3d91e90c2e 100644 --- a/test/container/Dockerfile +++ b/test/container/Dockerfile @@ -2,7 +2,7 @@ FROM redis:6.2.6 as redis FROM node:12.18.4 as node -FROM golang:1.17.6 as golang +FROM golang:1.17 as golang FROM registry:2.7.1 as registry diff --git a/test/e2e/fixture/fixture.go b/test/e2e/fixture/fixture.go index 4513789e83c4..bf3c184145fa 100644 --- a/test/e2e/fixture/fixture.go +++ b/test/e2e/fixture/fixture.go @@ -554,7 +554,9 @@ func EnsureCleanState(t *testing.T) { FailOnErr(Run("", "mkdir", "-p", TmpDir)) // random id - unique across test runs - postFix := "-" + strings.ToLower(rand.RandString(5)) + randString, err := rand.String(5) + CheckError(err) + postFix := "-" + strings.ToLower(randString) id = t.Name() + postFix name = DnsFriendly(t.Name(), "") deploymentNamespace = DnsFriendly(fmt.Sprintf("argocd-e2e-%s", t.Name()), postFix) diff --git a/test/e2e/selective_sync_test.go b/test/e2e/selective_sync_test.go index 050a5c0c00fd..1738264c509f 100644 --- a/test/e2e/selective_sync_test.go +++ b/test/e2e/selective_sync_test.go @@ -7,6 +7,7 @@ import ( "github.com/argoproj/gitops-engine/pkg/health" . "github.com/argoproj/gitops-engine/pkg/sync/common" + "github.com/stretchr/testify/require" . "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" "github.com/argoproj/argo-cd/v2/test/e2e/fixture" @@ -110,7 +111,9 @@ func TestSelectiveSyncWithNamespace(t *testing.T) { } func getNewNamespace(t *testing.T) string { - postFix := "-" + strings.ToLower(rand.RandString(5)) + randStr, err := rand.String(5) + require.NoError(t, err) + postFix := "-" + strings.ToLower(randStr) name := fixture.DnsFriendly(t.Name(), "") return fixture.DnsFriendly(fmt.Sprintf("argocd-e2e-%s", name), postFix) } diff --git a/test/remote/Dockerfile b/test/remote/Dockerfile index b9e144a38b92..a16d85fc8878 100644 --- a/test/remote/Dockerfile +++ b/test/remote/Dockerfile @@ -1,4 +1,4 @@ -FROM golang:1.17.6 AS go +FROM golang:1.17 AS go RUN go install github.com/mattn/goreman@latest && \ go install github.com/kisielk/godepgraph@latest diff --git a/ui/src/app/applications/components/application-urls.test.ts b/ui/src/app/applications/components/application-urls.test.ts new file mode 100644 index 000000000000..c9063561d01a --- /dev/null +++ b/ui/src/app/applications/components/application-urls.test.ts @@ -0,0 +1,20 @@ +import {ExternalLink, InvalidExternalLinkError} from './application-urls'; + +test('rejects malicious URLs', () => { + expect(() => { + const _ = new ExternalLink('javascript:alert("hi")'); + }).toThrowError(InvalidExternalLinkError); + expect(() => { + const _ = new ExternalLink('data:text/html;

hi

'); + }).toThrowError(InvalidExternalLinkError); +}); + +test('allows absolute URLs', () => { + expect(new ExternalLink('https://localhost:8080/applications').ref).toEqual('https://localhost:8080/applications'); +}); + +test('allows relative URLs', () => { + // @ts-ignore + window.location = new URL('https://localhost:8080/applications'); + expect(new ExternalLink('/applications').ref).toEqual('/applications'); +}); diff --git a/ui/src/app/applications/components/application-urls.tsx b/ui/src/app/applications/components/application-urls.tsx index ff743fc3d63f..1d64bc8a43b9 100644 --- a/ui/src/app/applications/components/application-urls.tsx +++ b/ui/src/app/applications/components/application-urls.tsx @@ -1,7 +1,15 @@ import {DropDownMenu} from 'argo-ui'; import * as React from 'react'; -class ExternalLink { +export class InvalidExternalLinkError extends Error { + constructor(message: string) { + super(message); + Object.setPrototypeOf(this, InvalidExternalLinkError.prototype); + this.name = 'InvalidExternalLinkError'; + } +} + +export class ExternalLink { public title: string; public ref: string; @@ -14,13 +22,36 @@ class ExternalLink { this.title = url; this.ref = url; } + if (!ExternalLink.isValidURL(this.ref)) { + throw new InvalidExternalLinkError('Invalid URL'); + } + } + + private static isValidURL(url: string): boolean { + try { + const parsedUrl = new URL(url); + return parsedUrl.protocol !== 'javascript:' && parsedUrl.protocol !== 'data:'; + } catch (TypeError) { + try { + // Try parsing as a relative URL. + const parsedUrl = new URL(url, window.location.origin); + return parsedUrl.protocol !== 'javascript:' && parsedUrl.protocol !== 'data:'; + } catch (TypeError) { + return false; + } + } } } export const ApplicationURLs = ({urls}: {urls: string[]}) => { const externalLinks: ExternalLink[] = []; for (const url of urls || []) { - externalLinks.push(new ExternalLink(url)); + try { + const externalLink = new ExternalLink(url); + externalLinks.push(externalLink); + } catch (InvalidExternalLinkError) { + continue; + } } // sorted alphabetically & links with titles first diff --git a/ui/src/app/login/components/login.tsx b/ui/src/app/login/components/login.tsx index 8662d4c2f132..db67ff185cf7 100644 --- a/ui/src/app/login/components/login.tsx +++ b/ui/src/app/login/components/login.tsx @@ -20,7 +20,7 @@ interface State { loginError: string; loginInProgress: boolean; returnUrl: string; - ssoLoginError: string; + hasSsoLoginError: boolean; } export class Login extends React.Component, State> { @@ -31,13 +31,13 @@ export class Login extends React.Component, State> { public static getDerivedStateFromProps(props: RouteComponentProps<{}>): Partial { const search = new URLSearchParams(props.history.location.search); const returnUrl = search.get('return_url') || ''; - const ssoLoginError = search.get('sso_error') || ''; - return {ssoLoginError, returnUrl}; + const hasSsoLoginError = search.get('has_sso_error') === 'true'; + return {hasSsoLoginError, returnUrl}; } constructor(props: RouteComponentProps<{}>) { super(props); - this.state = {authSettings: null, loginError: null, returnUrl: null, ssoLoginError: null, loginInProgress: false}; + this.state = {authSettings: null, loginError: null, returnUrl: null, hasSsoLoginError: false, loginInProgress: false}; } public async componentDidMount() { @@ -69,7 +69,7 @@ export class Login extends React.Component, State> { )} - {this.state.ssoLoginError &&
{this.state.ssoLoginError}
} + {this.state.hasSsoLoginError &&
Login failed.
} {authSettings && !authSettings.userLoginsDisabled && (
or diff --git a/ui/src/app/webpack.config.js b/ui/src/app/webpack.config.js index 3981b0971970..5b6b179f124b 100644 --- a/ui/src/app/webpack.config.js +++ b/ui/src/app/webpack.config.js @@ -88,6 +88,10 @@ const config = { { from: 'node_modules/redoc/bundles/redoc.standalone.js', to: 'assets/scripts/redoc.standalone.js' + }, + { + from: 'node_modules/monaco-editor/min/vs/base/browser/ui/codicons/codicon', + to: 'assets/fonts' } ] }), diff --git a/ui/src/assets/fonts.css b/ui/src/assets/fonts.css index 979dba40d449..559a17687114 100644 --- a/ui/src/assets/fonts.css +++ b/ui/src/assets/fonts.css @@ -1,3 +1,8 @@ +@font-face { + font-family: "codicon"; + src: url("./fonts/codicon.ttf") format("truetype"); +} + /* === Heebo - 300 */ @font-face { font-family: 'Heebo'; diff --git a/util/dex/dex.go b/util/dex/dex.go index 735b7cbb7297..3f109b225cfc 100644 --- a/util/dex/dex.go +++ b/util/dex/dex.go @@ -3,20 +3,18 @@ package dex import ( "bytes" "fmt" - "html" "io/ioutil" "net/http" "net/http/httputil" "net/url" "path" - "regexp" "strconv" + log "github.com/sirupsen/logrus" + "github.com/argoproj/argo-cd/v2/util/errors" ) -var messageRe = regexp.MustCompile(`

(.*)([\s\S]*?)<\/p>`) - func decorateDirector(director func(req *http.Request), target *url.URL) func(req *http.Request) { return func(req *http.Request) { director(req) @@ -44,16 +42,10 @@ func NewDexHTTPReverseProxy(serverAddr string, baseHRef string) func(writer http if err != nil { return err } - var message string - matches := messageRe.FindSubmatch(b) - if len(matches) > 1 { - message = html.UnescapeString(string(matches[1])) - } else { - message = "Unknown error" - } + log.Errorf("received error from dex: %s", string(b)) resp.ContentLength = 0 resp.Header.Set("Content-Length", strconv.Itoa(0)) - resp.Header.Set("Location", fmt.Sprintf("%s?sso_error=%s", path.Join(baseHRef, "login"), url.QueryEscape(message))) + resp.Header.Set("Location", fmt.Sprintf("%s?has_sso_error=true", path.Join(baseHRef, "login"))) resp.StatusCode = http.StatusSeeOther resp.Body = ioutil.NopCloser(bytes.NewReader(make([]byte, 0))) return nil diff --git a/util/dex/dex_test.go b/util/dex/dex_test.go index 16806038603f..026eb6caf5c6 100644 --- a/util/dex/dex_test.go +++ b/util/dex/dex_test.go @@ -408,7 +408,7 @@ func Test_DexReverseProxy(t *testing.T) { assert.Equal(t, http.StatusSeeOther, resp.StatusCode) location, _ := resp.Location() fmt.Printf("%s %s\n", resp.Status, location.RequestURI()) - assert.True(t, strings.HasPrefix(location.RequestURI(), "/login?sso_error")) + assert.True(t, strings.HasPrefix(location.RequestURI(), "/login?has_sso_error=true")) }) t.Run("Invalid URL for Dex reverse proxy", func(t *testing.T) { diff --git a/util/helm/helm.go b/util/helm/helm.go index 0c3a3a184e8d..8b83881fe096 100644 --- a/util/helm/helm.go +++ b/util/helm/helm.go @@ -6,10 +6,11 @@ import ( "net/url" "os" "os/exec" - "path" + "path/filepath" "strings" "github.com/ghodss/yaml" + log "github.com/sirupsen/logrus" "github.com/argoproj/argo-cd/v2/util/config" executil "github.com/argoproj/argo-cd/v2/util/exec" @@ -28,7 +29,7 @@ type Helm interface { // Template returns a list of unstructured objects from a `helm template` command Template(opts *TemplateOpts) (string, error) // GetParameters returns a list of chart parameters taking into account values in provided YAML files. - GetParameters(valuesFiles []pathutil.ResolvedFilePath) (map[string]string, error) + GetParameters(valuesFiles []pathutil.ResolvedFilePath, appPath, repoRoot string) (map[string]string, error) // DependencyBuild runs `helm dependency build` to download a chart's dependencies DependencyBuild() error // Init runs `helm init --client-only` @@ -130,12 +131,19 @@ func Version(shortForm bool) (string, error) { return strings.TrimSpace(version), nil } -func (h *helm) GetParameters(valuesFiles []pathutil.ResolvedFilePath) (map[string]string, error) { - out, err := h.cmd.inspectValues(".") - if err != nil { - return nil, err +func (h *helm) GetParameters(valuesFiles []pathutil.ResolvedFilePath, appPath, repoRoot string) (map[string]string, error) { + var values []string + // Don't load values.yaml if it's an out-of-bounds link. + if resolved, _, err := pathutil.ResolveFilePath(appPath, repoRoot, "values.yaml", []string{}); err == nil { + fmt.Println(resolved) + out, err := h.cmd.inspectValues(".") + if err != nil { + return nil, err + } + values = append(values, out) + } else { + log.Warnf("Values file %s is not allowed: %v", filepath.Join(appPath, "values.yaml"), err) } - values := []string{out} for i := range valuesFiles { file := string(valuesFiles[i]) var fileValues []byte @@ -143,11 +151,10 @@ func (h *helm) GetParameters(valuesFiles []pathutil.ResolvedFilePath) (map[strin if err == nil && (parsedURL.Scheme == "http" || parsedURL.Scheme == "https") { fileValues, err = config.ReadRemoteFile(file) } else { - filePath := path.Join(h.cmd.WorkDir, file) - if _, err := os.Stat(filePath); os.IsNotExist(err) { + if _, err := os.Stat(file); os.IsNotExist(err) { continue } - fileValues, err = ioutil.ReadFile(filePath) + fileValues, err = ioutil.ReadFile(file) } if err != nil { return nil, fmt.Errorf("failed to read value file %s: %s", file, err) @@ -158,7 +165,7 @@ func (h *helm) GetParameters(valuesFiles []pathutil.ResolvedFilePath) (map[strin output := map[string]string{} for _, file := range values { values := map[string]interface{}{} - if err = yaml.Unmarshal([]byte(file), &values); err != nil { + if err := yaml.Unmarshal([]byte(file), &values); err != nil { return nil, fmt.Errorf("failed to parse values: %s", err) } flatVals(values, output) diff --git a/util/helm/helm_test.go b/util/helm/helm_test.go index acccffe6f40b..bee3cdbdd2d8 100644 --- a/util/helm/helm_test.go +++ b/util/helm/helm_test.go @@ -1,10 +1,11 @@ package helm import ( - "fmt" - "os" + "path/filepath" "testing" + "github.com/stretchr/testify/require" + "github.com/argoproj/argo-cd/v2/util/io/path" "github.com/argoproj/gitops-engine/pkg/utils/kube" @@ -53,11 +54,16 @@ func TestHelmTemplateParams(t *testing.T) { } func TestHelmTemplateValues(t *testing.T) { - h, err := NewHelmApp("./testdata/redis", []HelmRepository{}, false, "", "", false) + repoRoot := "./testdata/redis" + repoRootAbs, err := filepath.Abs(repoRoot) + require.NoError(t, err) + h, err := NewHelmApp(repoRootAbs, []HelmRepository{}, false, "", "", false) assert.NoError(t, err) + valuesPath, _, err := path.ResolveFilePath(repoRootAbs, repoRootAbs, "values-production.yaml", nil) + require.NoError(t, err) opts := TemplateOpts{ Name: "test", - Values: []path.ResolvedFilePath{"values-production.yaml"}, + Values: []path.ResolvedFilePath{valuesPath}, } objs, err := template(h, &opts) assert.Nil(t, err) @@ -74,59 +80,48 @@ func TestHelmTemplateValues(t *testing.T) { } func TestHelmGetParams(t *testing.T) { - h, err := NewHelmApp("./testdata/redis", nil, false, "", "", false) + repoRoot := "./testdata/redis" + repoRootAbs, err := filepath.Abs(repoRoot) + require.NoError(t, err) + h, err := NewHelmApp(repoRootAbs, nil, false, "", "", false) assert.NoError(t, err) - params, err := h.GetParameters(nil) + params, err := h.GetParameters(nil, repoRootAbs, repoRootAbs) assert.Nil(t, err) slaveCountParam := params["cluster.slaveCount"] - assert.Equal(t, slaveCountParam, "1") + assert.Equal(t, "1", slaveCountParam) } func TestHelmGetParamsValueFiles(t *testing.T) { - h, err := NewHelmApp("./testdata/redis", nil, false, "", "", false) + repoRoot := "./testdata/redis" + repoRootAbs, err := filepath.Abs(repoRoot) + require.NoError(t, err) + h, err := NewHelmApp(repoRootAbs, nil, false, "", "", false) assert.NoError(t, err) - params, err := h.GetParameters([]path.ResolvedFilePath{"values-production.yaml"}) + valuesPath, _, err := path.ResolveFilePath(repoRootAbs, repoRootAbs, "values-production.yaml", nil) + require.NoError(t, err) + params, err := h.GetParameters([]path.ResolvedFilePath{valuesPath}, repoRootAbs, repoRootAbs) assert.Nil(t, err) slaveCountParam := params["cluster.slaveCount"] - assert.Equal(t, slaveCountParam, "3") + assert.Equal(t, "3", slaveCountParam) } func TestHelmGetParamsValueFilesThatExist(t *testing.T) { - h, err := NewHelmApp("./testdata/redis", nil, false, "", "", false) + repoRoot := "./testdata/redis" + repoRootAbs, err := filepath.Abs(repoRoot) + require.NoError(t, err) + h, err := NewHelmApp(repoRootAbs, nil, false, "", "", false) assert.NoError(t, err) - params, err := h.GetParameters([]path.ResolvedFilePath{"values-missing.yaml", "values-production.yaml"}) + valuesMissingPath, _, err := path.ResolveFilePath(repoRootAbs, repoRootAbs, "values-missing.yaml", nil) + require.NoError(t, err) + valuesProductionPath, _, err := path.ResolveFilePath(repoRootAbs, repoRootAbs, "values-production.yaml", nil) + require.NoError(t, err) + params, err := h.GetParameters([]path.ResolvedFilePath{valuesMissingPath, valuesProductionPath}, repoRootAbs, repoRootAbs) assert.Nil(t, err) slaveCountParam := params["cluster.slaveCount"] - assert.Equal(t, slaveCountParam, "3") -} - -func TestHelmDependencyBuild(t *testing.T) { - testCases := map[string]string{"Helm": "dependency", "Helm2": "helm2-dependency"} - helmRepos := []HelmRepository{{Name: "bitnami", Repo: "https://charts.bitnami.com/bitnami"}} - for name := range testCases { - t.Run(name, func(t *testing.T) { - chart := testCases[name] - clean := func() { - _ = os.RemoveAll(fmt.Sprintf("./testdata/%s/charts", chart)) - _ = os.RemoveAll(fmt.Sprintf("./testdata/%s/Chart.lock", chart)) - } - clean() - defer clean() - h, err := NewHelmApp(fmt.Sprintf("./testdata/%s", chart), helmRepos, false, "", "", false) - assert.NoError(t, err) - err = h.Init() - assert.NoError(t, err) - _, err = h.Template(&TemplateOpts{Name: "wordpress"}) - assert.Error(t, err) - err = h.DependencyBuild() - assert.NoError(t, err) - _, err = h.Template(&TemplateOpts{Name: "wordpress"}) - assert.NoError(t, err) - }) - } + assert.Equal(t, "3", slaveCountParam) } func TestHelmTemplateReleaseNameOverwrite(t *testing.T) { diff --git a/util/helm/testdata/dependency/Chart.lock b/util/helm/testdata/dependency/Chart.lock new file mode 100644 index 000000000000..c80c71d15f6a --- /dev/null +++ b/util/helm/testdata/dependency/Chart.lock @@ -0,0 +1,9 @@ +dependencies: +- name: mongodb + repository: https://charts.bitnami.com/bitnami + version: 7.8.10 +- name: eventstore + repository: https://eventstore.github.io/EventStore.Charts + version: 0.2.5 +digest: sha256:94b7537c078de2547173e28289cbae155893cd797aa9296f6c78ba922d6a1e8a +generated: "2022-05-19T11:55:59.559189-04:00" diff --git a/util/io/bytereadseeker.go b/util/io/bytereadseeker.go index 48aed29c43fd..43c246c2eced 100644 --- a/util/io/bytereadseeker.go +++ b/util/io/bytereadseeker.go @@ -14,7 +14,7 @@ type byteReadSeeker struct { offset int64 } -func (f byteReadSeeker) Read(b []byte) (int, error) { +func (f *byteReadSeeker) Read(b []byte) (int, error) { if f.offset >= int64(len(f.data)) { return 0, io.EOF } @@ -23,7 +23,7 @@ func (f byteReadSeeker) Read(b []byte) (int, error) { return n, nil } -func (f byteReadSeeker) Seek(offset int64, whence int) (int64, error) { +func (f *byteReadSeeker) Seek(offset int64, whence int) (int64, error) { switch whence { case 1: offset += f.offset diff --git a/util/io/bytereadseeker_test.go b/util/io/bytereadseeker_test.go new file mode 100644 index 000000000000..5784d563e9f5 --- /dev/null +++ b/util/io/bytereadseeker_test.go @@ -0,0 +1,71 @@ +package io + +import ( + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "io" + "testing" +) + +func TestByteReadSeeker_Read(t *testing.T) { + inString := "hello world" + reader := NewByteReadSeeker([]byte(inString)) + var bytes = make([]byte, 11) + n, err := reader.Read(bytes) + require.NoError(t, err) + assert.Equal(t, len(inString), n) + assert.Equal(t, inString, string(bytes)) + _, err = reader.Read(bytes) + assert.ErrorIs(t, err, io.EOF) +} + +func TestByteReadSeeker_Seek_Start(t *testing.T) { + inString := "hello world" + reader := NewByteReadSeeker([]byte(inString)) + offset, err := reader.Seek(6, io.SeekStart) + require.NoError(t, err) + assert.Equal(t, int64(6), offset) + var bytes = make([]byte, 5) + n, err := reader.Read(bytes) + require.NoError(t, err) + assert.Equal(t, 5, n) + assert.Equal(t, "world", string(bytes)) +} + +func TestByteReadSeeker_Seek_Current(t *testing.T) { + inString := "hello world" + reader := NewByteReadSeeker([]byte(inString)) + offset, err := reader.Seek(3, io.SeekCurrent) + require.NoError(t, err) + assert.Equal(t, int64(3), offset) + offset, err = reader.Seek(3, io.SeekCurrent) + require.NoError(t, err) + assert.Equal(t, int64(6), offset) + var bytes = make([]byte, 5) + n, err := reader.Read(bytes) + require.NoError(t, err) + assert.Equal(t, 5, n) + assert.Equal(t, "world", string(bytes)) +} + +func TestByteReadSeeker_Seek_End(t *testing.T) { + inString := "hello world" + reader := NewByteReadSeeker([]byte(inString)) + offset, err := reader.Seek(-5, io.SeekEnd) + require.NoError(t, err) + assert.Equal(t, int64(6), offset) + var bytes = make([]byte, 5) + n, err := reader.Read(bytes) + require.NoError(t, err) + assert.Equal(t, 5, n) + assert.Equal(t, "world", string(bytes)) +} + +func TestByteReadSeeker_Seek_OutOfBounds(t *testing.T) { + inString := "hello world" + reader := NewByteReadSeeker([]byte(inString)) + _, err := reader.Seek(12, io.SeekStart) + assert.Error(t, err) + _, err = reader.Seek(-1, io.SeekStart) + assert.Error(t, err) +} \ No newline at end of file diff --git a/util/io/files/util.go b/util/io/files/util.go new file mode 100644 index 000000000000..7bb2cd13d960 --- /dev/null +++ b/util/io/files/util.go @@ -0,0 +1,35 @@ +package files + +import ( + "io/fs" + "os" + "path/filepath" + "strings" +) + +// Inbound will validate if the given candidate path is inside the +// baseDir. This is useful to make sure that malicious candidates +// are not targeting a file outside of baseDir boundaries. +// Considerations: +// - baseDir must be absolute path. Will return false otherwise +// - candidate can be absolute or relative path +// - candidate should not be symlink as only syntatic validation is +// applied by this function +func Inbound(candidate, baseDir string) bool { + if !filepath.IsAbs(baseDir) { + return false + } + var target string + if filepath.IsAbs(candidate) { + target = filepath.Clean(candidate) + } else { + target = filepath.Join(baseDir, candidate) + } + return strings.HasPrefix(target, filepath.Clean(baseDir)+string(os.PathSeparator)) +} + +// IsSymlink return true if the given FileInfo relates to a +// symlink file. Returns false otherwise. +func IsSymlink(fi os.FileInfo) bool { + return fi.Mode()&fs.ModeSymlink == fs.ModeSymlink +} diff --git a/util/io/files/util_test.go b/util/io/files/util_test.go new file mode 100644 index 000000000000..f7a34f9ec234 --- /dev/null +++ b/util/io/files/util_test.go @@ -0,0 +1,63 @@ +package files_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/argoproj/argo-cd/v2/util/io/files" +) + +func TestInbound(t *testing.T) { + type testcase struct { + name string + candidate string + basedir string + expected bool + } + cases := []testcase{ + { + name: "will return true if candidate is inbound", + candidate: "/home/test/app/readme.md", + basedir: "/home/test", + expected: true, + }, + { + name: "will return false if candidate is not inbound", + candidate: "/home/test/../readme.md", + basedir: "/home/test", + expected: false, + }, + { + name: "will return true if candidate is relative inbound", + candidate: "./readme.md", + basedir: "/home/test", + expected: true, + }, + { + name: "will return false if candidate is relative outbound", + candidate: "../readme.md", + basedir: "/home/test", + expected: false, + }, + { + name: "will return false if basedir is relative", + candidate: "/home/test/app/readme.md", + basedir: "./test", + expected: false, + }, + } + for _, c := range cases { + c := c + t.Run(c.name, func(t *testing.T) { + // given + t.Parallel() + + // when + inbound := files.Inbound(c.candidate, c.basedir) + + // then + assert.Equal(t, c.expected, inbound) + }) + } +} \ No newline at end of file diff --git a/util/io/path/resolved.go b/util/io/path/resolved.go index 0343a379f049..abc3ff1de975 100644 --- a/util/io/path/resolved.go +++ b/util/io/path/resolved.go @@ -11,6 +11,7 @@ import ( ) // ResolvedFilePath represents a resolved file path and intended to prevent unintentional use of not verified file path. +// It is always either a URL or an absolute path. type ResolvedFilePath string // resolveSymbolicLinkRecursive resolves the symlink path recursively to its diff --git a/util/lua/custom_actions_test.go b/util/lua/custom_actions_test.go index b530ffe4f588..3108ec5909e8 100644 --- a/util/lua/custom_actions_test.go +++ b/util/lua/custom_actions_test.go @@ -7,9 +7,6 @@ import ( "path/filepath" "strings" "testing" - "time" - - "github.com/undefinedlabs/go-mpatch" "github.com/argoproj/gitops-engine/pkg/diff" "github.com/ghodss/yaml" @@ -87,13 +84,9 @@ func TestLuaResourceActionsScript(t *testing.T) { action, err := vm.GetResourceAction(obj, test.Action) assert.NoError(t, err) - // freeze time so that lua test has predictable time output (will return 0001-01-01T00:00:00Z) - patch, err := mpatch.PatchMethod(time.Now, func() time.Time { return time.Time{} }) assert.NoError(t, err) result, err := vm.ExecuteResourceAction(obj, action.ActionLua) assert.NoError(t, err) - err = patch.Unpatch() - assert.NoError(t, err) expectedObj := getObj(filepath.Join(dir, test.ExpectedOutputPath)) // Ideally, we would use a assert.Equal to detect the difference, but the Lua VM returns a object with float64 instead of the original int32. As a result, the assert.Equal is never true despite that the change has been applied. diff --git a/util/oidc/oidc.go b/util/oidc/oidc.go index 1727331ec118..20b9a5fef8b8 100644 --- a/util/oidc/oidc.go +++ b/util/oidc/oidc.go @@ -144,7 +144,12 @@ func (a *ClientApp) oauth2Config(scopes []string) (*oauth2.Config, error) { // generateAppState creates an app state nonce func (a *ClientApp) generateAppState(returnURL string, w http.ResponseWriter) (string, error) { - randStr := rand.RandString(10) + // According to the spec (https://www.rfc-editor.org/rfc/rfc6749#section-10.10), this must be guessable with + // probability <= 2^(-128). The following call generates one of 52^24 random strings, ~= 2^136 possibilities. + randStr, err := rand.String(24) + if err != nil { + return "", fmt.Errorf("failed to generate app state: %w", err) + } if returnURL == "" { returnURL = a.baseHRef } @@ -283,7 +288,12 @@ func (a *ClientApp) HandleLogin(w http.ResponseWriter, r *http.Request) { case GrantTypeAuthorizationCode: url = oauth2Config.AuthCodeURL(stateNonce, opts...) case GrantTypeImplicit: - url = ImplicitFlowURL(oauth2Config, stateNonce, opts...) + url, err = ImplicitFlowURL(oauth2Config, stateNonce, opts...) + if err != nil { + log.Errorf("Failed to initiate implicit login flow: %v", err) + http.Error(w, "Failed to initiate implicit login flow", http.StatusInternalServerError) + return + } default: http.Error(w, fmt.Sprintf("Unsupported grant type: %v", grantType), http.StatusInternalServerError) return @@ -415,10 +425,14 @@ func (a *ClientApp) handleImplicitFlow(r *http.Request, w http.ResponseWriter, s // ImplicitFlowURL is an adaptation of oauth2.Config::AuthCodeURL() which returns a URL // appropriate for an OAuth2 implicit login flow (as opposed to authorization code flow). -func ImplicitFlowURL(c *oauth2.Config, state string, opts ...oauth2.AuthCodeOption) string { +func ImplicitFlowURL(c *oauth2.Config, state string, opts ...oauth2.AuthCodeOption) (string, error) { opts = append(opts, oauth2.SetAuthURLParam("response_type", "id_token")) - opts = append(opts, oauth2.SetAuthURLParam("nonce", rand.RandString(10))) - return c.AuthCodeURL(state, opts...) + randString, err := rand.String(24) + if err != nil { + return "", fmt.Errorf("failed to generate nonce for implicit flow URL: %w", err) + } + opts = append(opts, oauth2.SetAuthURLParam("nonce", randString)) + return c.AuthCodeURL(state, opts...), nil } // OfflineAccess returns whether or not 'offline_access' is a supported scope diff --git a/util/rand/rand.go b/util/rand/rand.go index 8a942c8123b8..1e748bf93bb7 100644 --- a/util/rand/rand.go +++ b/util/rand/rand.go @@ -1,37 +1,30 @@ package rand import ( - "math/rand" - "time" + "crypto/rand" + "fmt" + "math/big" ) const letterBytes = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ" -const ( - letterIdxBits = 6 // 6 bits to represent a letter index - letterIdxMask = 1<= 0; { - if remain == 0 { - cache, remain = src.Int63(), letterIdxMax - } - if idx := int(cache & letterIdxMask); idx < len(charset) { - b[i] = charset[idx] - i-- + maxIdx := big.NewInt(int64(len(charset))) + for i := 0; i < n; i++ { + randIdx, err := rand.Int(rand.Reader, maxIdx) + if err != nil { + return "", fmt.Errorf("failed to generate random string: %w", err) } - cache >>= letterIdxBits - remain-- + // randIdx is necessarily safe to convert to int, because the max came from an int. + randIdxInt := int(randIdx.Int64()) + b[i] = charset[randIdxInt] } - return string(b) + return string(b), nil } diff --git a/util/rand/rand_test.go b/util/rand/rand_test.go index 62002c409f5f..d696c0e22c8d 100644 --- a/util/rand/rand_test.go +++ b/util/rand/rand_test.go @@ -2,15 +2,17 @@ package rand import ( "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestRandString(t *testing.T) { - ss := RandStringCharset(10, "A") - if ss != "AAAAAAAAAA" { - t.Errorf("Expected 10 As, but got %q", ss) - } - ss = RandStringCharset(5, "ABC123") - if len(ss) != 5 { - t.Errorf("Expected random string of length 10, but got %q", ss) - } + ss, err := StringFromCharset(10, "A") + require.NoError(t, err) + assert.Equal(t, "AAAAAAAAAA", ss) + + ss, err = StringFromCharset(5, "ABC123") + require.NoError(t, err) + assert.Len(t, ss, 5) } diff --git a/util/session/sessionmanager.go b/util/session/sessionmanager.go index d7cbb60aeabf..6a8047ec86ea 100644 --- a/util/session/sessionmanager.go +++ b/util/session/sessionmanager.go @@ -210,7 +210,7 @@ func (mgr *SessionManager) Parse(tokenString string) (jwt.Claims, string, error) token, err := jwt.ParseWithClaims(tokenString, &claims, func(token *jwt.Token) (interface{}, error) { // Don't forget to validate the alg is what you expect: if _, ok := token.Method.(*jwt.SigningMethodHMAC); !ok { - return nil, fmt.Errorf("Unexpected signing method: %v", token.Header["alg"]) + return nil, fmt.Errorf("unexpected signing method: %v", token.Header["alg"]) } return argoCDSettings.ServerSignature, nil }) @@ -262,7 +262,7 @@ func (mgr *SessionManager) Parse(tokenString string) (jwt.Claims, string, error) } if account.PasswordMtime != nil && issuedAt.Before(*account.PasswordMtime) { - return nil, "", fmt.Errorf("Account password has changed since token issued") + return nil, "", fmt.Errorf("account password has changed since token issued") } newToken := "" @@ -477,7 +477,7 @@ func (mgr *SessionManager) VerifyToken(tokenString string) (jwt.Claims, string, // IDP signed token prov, err := mgr.provider() if err != nil { - return claims, "", err + return nil, "", err } // Token must be verified for at least one audience @@ -489,16 +489,30 @@ func (mgr *SessionManager) VerifyToken(tokenString string) (jwt.Claims, string, break } } + + // The token verification has failed. If the token has expired, we will + // return a dummy claims only containing a value for the issuer, so the + // UI can handle expired tokens appropriately. if err != nil { - return claims, "", err + if strings.HasPrefix(err.Error(), "oidc: token is expired") { + claims = jwt.RegisteredClaims{ + Issuer: "sso", + } + return claims, "", err + } + return nil, "", err } + if idToken == nil { - return claims, "", fmt.Errorf("No audience found in the token") + return nil, "", fmt.Errorf("no audience found in the token") } var claims jwt.MapClaims err = idToken.Claims(&claims) - return claims, "", err + if err != nil { + return nil, "", err + } + return claims, "", nil } }