Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix bug atmos vendor pull URI cannot contain path traversal sequences and git schema #899

Merged
merged 13 commits into from
Jan 22, 2025
40 changes: 40 additions & 0 deletions examples/tests/test-vendor/atmos.yaml
haitham911 marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
base_path: "./"

components:
terraform:
base_path: "components/terraform"
apply_auto_approve: false
deploy_run_init: true
init_run_reconfigure: true
auto_generate_backend_file: false

stacks:
base_path: "stacks"
included_paths:
- "deploy/**/*"
excluded_paths:
- "**/_defaults.yaml"
name_pattern: "{stage}"

vendor:
# Single file
base_path: "./vendor.yaml"

# Directory with multiple files
#base_path: "./vendor"

# Absolute path
#base_path: "vendor.d/vendor1.yaml"

logs:
file: "/dev/stderr"
level: Info

# Custom CLI commands

# No arguments or flags are required
commands:
- name: "test"
description: "Run all tests"
steps:
- atmos vendor pull --everything
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Consider using explicit component selection instead of --everything flag

The --everything flag in the test command will pull all defined components:

  • github/stargazers (demo, github tags)
  • test-components (demo tag)
  • weather (demo tag)
  • my-vpc1 (no tags)

Instead of using --everything, consider one of these more explicit approaches:

  • Use --tags demo to pull all demo components
  • List specific components: atmos vendor pull -c github/stargazers -c weather
  • Create a separate test command for each component group you need to test

This provides better control over which components are pulled during testing and makes the dependencies more explicit.

🔗 Analysis chain

Review security implications of the --everything flag.

The --everything flag in the test command could potentially pull all vendor components without discrimination. Consider being more explicit about which components to test.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check what components would be pulled with --everything flag
atmos vendor pull --dry-run --everything

Length of output: 138


Script:

#!/bin/bash
# Let's check the vendor configuration and related files
fd vendor -t f

# Check for any documentation or configuration related to vendor components
rg -i "vendor" -g "*.{md,yaml,yml}" -A 3 -B 3

Length of output: 34075


Script:

#!/bin/bash
# Let's check the vendor.yaml file in the test-vendor directory to understand the configuration
cat examples/tests/test-vendor/vendor.yaml

# Let's also check if there are any specific components or tags defined
rg "component:|tags:" examples/tests/test-vendor/vendor.yaml -A 2

Length of output: 2092

🧰 Tools
🪛 yamllint (1.35.1)

[warning] 37-37: wrong indentation: expected 2 but found 0

(indentation)


[warning] 40-40: wrong indentation: expected 4 but found 2

(indentation)

13 changes: 13 additions & 0 deletions examples/tests/test-vendor/test-components/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# Example Terraform IPinfo Component

This Terraform module retrieves data from the IPinfo API for a specified IP address. If no IP address is specified, it retrieves data for the requester's IP address.

## Usage

haitham911 marked this conversation as resolved.
Show resolved Hide resolved
### Inputs

- `ip_address` (optional): The IP address to retrieve information for. If not specified, the requester's IP address will be used. The default value is an empty string.

### Outputs

- `metadata`: The data retrieved from IPinfo for the specified IP address, in JSON format.
7 changes: 7 additions & 0 deletions examples/tests/test-vendor/test-components/main.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
data "http" "ipinfo" {
url = var.ip_address != "" ? "https://ipinfo.io/${var.ip_address}" : "https://ipinfo.io"

request_headers = {
Accept = "application/json"
}
}
osterman marked this conversation as resolved.
Show resolved Hide resolved
4 changes: 4 additions & 0 deletions examples/tests/test-vendor/test-components/outputs.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
output "metadata" {
description = "The data retrieved from IPinfo for the specified IP address"
value = jsondecode(data.http.ipinfo.response_body)
}
1 change: 1 addition & 0 deletions examples/tests/test-vendor/test-components/providers.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
provider "http" {}
5 changes: 5 additions & 0 deletions examples/tests/test-vendor/test-components/variables.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
variable "ip_address" {
description = "The IP address to retrieve information for (optional)"
type = string
default = ""
}
5 changes: 5 additions & 0 deletions examples/tests/test-vendor/test-components/versions.tf
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
terraform {
required_version = ">= 1.0.0"

required_providers {}
}
53 changes: 53 additions & 0 deletions examples/tests/test-vendor/vendor.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
apiVersion: atmos/v1
kind: AtmosVendorConfig
metadata:
name: demo-vendoring
description: Atmos vendoring manifest for Atmos demo component library
spec:
# Import other vendor manifests, if necessary
imports: []

sources:
- component: "github/stargazers"
source: "github.com/cloudposse/atmos.git//examples/demo-library/{{ .Component }}?ref={{.Version}}"
version: "main"
targets:
- "components/terraform/{{ .Component }}/{{.Version}}"
included_paths:
- "**/*.tf"
- "**/*.tfvars"
- "**/*.md"
tags:
- demo
- github

- component: "test-components"
source: "file:///./test-components"
version: "main"
targets:
- "components/terraform/{{ .Component }}/{{.Version}}"
tags:
- demo

- component: "weather"
source: "git::https://github.com/cloudposse/atmos.git//examples/demo-library/{{ .Component }}?ref={{.Version}}"
version: "main"
targets:
- "components/terraform/{{ .Component }}/{{.Version}}"
tags:
- demo
- component: "my-vpc1"
source: "oci://public.ecr.aws/cloudposse/components/terraform/stable/aws/vpc:{{.Version}}"
version: "latest"
targets:
- "components/terraform/infra/my-vpc1"
osterman marked this conversation as resolved.
Show resolved Hide resolved
- component: "vpc-src"
source: "../../demo-library/weather" # required for test URI contain path traversal
targets:
- "components/terraform/vpc-src"
included_paths:
- "**/*.tf"
- "**/*.md"
- "**/*.tftmpl"
- "**/modules/**"
excluded_paths: []
9 changes: 9 additions & 0 deletions internal/exec/vendor_component_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"context"
"errors"
"fmt"
"net/url"
"os"
"path/filepath"
"strings"
Expand Down Expand Up @@ -248,6 +249,14 @@ func ExecuteComponentVendorInternal(
sourceIsLocalFile = true
}
}
u, err := url.Parse(uri)
if err == nil && u.Scheme != "" {
if u.Scheme == "file" {
trimmedPath := strings.TrimPrefix(filepath.ToSlash(u.Path), "/")
uri = filepath.Clean(trimmedPath)
useLocalFileSystem = true
}
}
}

var pType pkgType
Expand Down
2 changes: 1 addition & 1 deletion internal/exec/vendor_model.go
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ func downloadAndInstall(p *pkgAtmosVendor, dryRun bool, atmosConfig schema.Atmos
}
}
// Create temp directory
tempDir, err := os.MkdirTemp("", fmt.Sprintf("atmos-vendor-%d-*", time.Now().Unix()))
tempDir, err := os.MkdirTemp("", "atmos-vendor")
if err != nil {
return installedPkgMsg{
err: fmt.Errorf("failed to create temp directory: %w", err),
Expand Down
30 changes: 24 additions & 6 deletions internal/exec/vendor_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package exec

import (
"fmt"
"net/url"
"os"
"path/filepath"
"sort"
Expand Down Expand Up @@ -361,12 +362,17 @@ func ExecuteAtmosVendorInternal(
if err != nil {
return err
}
err = ValidateURI(uri)
if err != nil {
return err
}

useOciScheme, useLocalFileSystem, sourceIsLocalFile := determineSourceType(&uri, vendorConfigFilePath)
if !useLocalFileSystem {
err = ValidateURI(uri)
if err != nil {
if strings.Contains(uri, "..") {
return fmt.Errorf("Invalid URI '%s': %w. Please ensure the source is a valid local path or a properly formatted URI.", uri, err)
}
return err
}
}

// Determine package type
var pType pkgType
Expand All @@ -384,7 +390,7 @@ func ExecuteAtmosVendorInternal(
if err != nil {
return err
}
targetPath := filepath.Join(vendorConfigFilePath, target)
targetPath := filepath.Join(filepath.ToSlash(vendorConfigFilePath), filepath.ToSlash(target))
pkgName := s.Component
if pkgName == "" {
pkgName = uri
Expand Down Expand Up @@ -514,12 +520,22 @@ func determineSourceType(uri *string, vendorConfigFilePath string) (bool, bool,
useLocalFileSystem := false
sourceIsLocalFile := false
if !useOciScheme {
if absPath, err := u.JoinAbsolutePathWithPath(vendorConfigFilePath, *uri); err == nil {
if absPath, err := u.JoinAbsolutePathWithPath(filepath.ToSlash(vendorConfigFilePath), *uri); err == nil {
uri = &absPath
useLocalFileSystem = true
sourceIsLocalFile = u.FileExists(*uri)
}
parsedURL, err := url.Parse(*uri)
if err == nil && parsedURL.Scheme != "" {
if parsedURL.Scheme == "file" {
trimmedPath := strings.TrimPrefix(filepath.ToSlash(parsedURL.Path), "/")
*uri = filepath.Clean(trimmedPath)
useLocalFileSystem = true
}
}

haitham911 marked this conversation as resolved.
Show resolved Hide resolved
}

return useOciScheme, useLocalFileSystem, sourceIsLocalFile
}

Expand Down Expand Up @@ -556,6 +572,8 @@ func generateSkipFunction(atmosConfig schema.AtmosConfiguration, tempDir string,
if filepath.Base(src) == ".git" {
return true, nil
}
tempDir = filepath.ToSlash(tempDir)
src = filepath.ToSlash(src)

trimmedSrc := u.TrimBasePathFromPath(tempDir+"/", src)

Expand Down
2 changes: 1 addition & 1 deletion tests/fixtures/scenarios/complete/vendor.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ spec:
- test
- networking
- component: "vpc-flow-logs-bucket"
source: "github.com/cloudposse/terraform-aws-components.git//modules/vpc-flow-logs-bucket?ref={{.Version}}"
source: "git::https://github.com/cloudposse/terraform-aws-components.git//modules/vpc-flow-logs-bucket?ref={{.Version}}"
version: "1.323.0"
targets:
- "components/terraform/infra/vpc-flow-logs-bucket/{{.Version}}"
Expand Down
64 changes: 64 additions & 0 deletions tests/test-cases/demo-stacks.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -130,3 +130,67 @@ tests:
stderr:
- "^$"
exit_code: 0

- name: atmos greet with args
enabled: true
description: "Validate atmos custom command greet runs with argument provided."
workdir: "../examples/demo-custom-command/"
command: "atmos"
args:
- "greet"
- "Andrey"
expect:
stdout:
- "Hello, Andrey\n"
stderr:
- "^$"
exit_code: 0

- name: atmos greet without args
enabled: true
description: "Validate atmos custom command greet runs without argument provided."
workdir: "../examples/demo-custom-command/"
command: "atmos"
args:
- "greet"
expect:
stdout:
- "Hello, John Doe\n"
stderr:
- "^$"
exit_code: 0
- name: atmos_vendor_pull
enabled: true
description: "Ensure atmos vendor pull command executes without errors and files are present."
workdir: "../examples/tests/test-vendor/"
haitham911 marked this conversation as resolved.
Show resolved Hide resolved
command: "atmos"
args:
- "vendor"
- "pull"
expect:
file_exists:
- "./components/terraform/github/stargazers/main/main.tf"
- "./components/terraform/github/stargazers/main/outputs.tf"
- "./components/terraform/github/stargazers/main/providers.tf"
- "./components/terraform/github/stargazers/main/variables.tf"
- "./components/terraform/github/stargazers/main/versions.tf"
- "./components/terraform/infra/my-vpc1/main.tf"
- "./components/terraform/infra/my-vpc1/outputs.tf"
- "./components/terraform/infra/my-vpc1/providers.tf"
- "./components/terraform/infra/my-vpc1/variables.tf"
- "./components/terraform/infra/my-vpc1/versions.tf"
- "./components/terraform/test-components/main/main.tf"
- "./components/terraform/test-components/main/outputs.tf"
- "./components/terraform/test-components/main/providers.tf"
- "./components/terraform/test-components/main/variables.tf"
- "./components/terraform/test-components/main/versions.tf"
- "./components/terraform/weather/main/main.tf"
- "./components/terraform/weather/main/outputs.tf"
- "./components/terraform/weather/main/providers.tf"
- "./components/terraform/weather/main/variables.tf"
- "./components/terraform/weather/main/versions.tf"
- "./components/terraform/vpc-src/main.tf"
- "./components/terraform/vpc-src/outputs.tf"
- "./components/terraform/vpc-src/variables.tf"
- "./components/terraform/vpc-src/versions.tf"
exit_code: 0
Loading