Skip to content

Commit

Permalink
Enable l2-resource-asset-archive test (#684)
Browse files Browse the repository at this point in the history
This is a bit more of an involved change to get this test working.
Making two major changes.

Firstly, historically YAML would always send absolute paths instead of
relative paths to the engine. This was wrong, the engine expects that
paths should be relative so that we don't save absolute paths to the
state file.

Secondly, YAML would error if you tried to access what it considered
"unsafe" paths. That was either paths that clearly accessed outside the
Pulumi.yaml directory, or non-constant paths to the remote asset and
archive types. This seems overly restrictive, none of our other
languages try to protect the user like this and it seems reasonable to
expect to be able to do similar filesystem accesses from YAML.

---------

Co-authored-by: Thomas Gummerer <t.gummerer@gmail.com>
  • Loading branch information
Frassle and tgummerer authored Nov 18, 2024
1 parent 3c4dea6 commit 61b83d7
Show file tree
Hide file tree
Showing 11 changed files with 54 additions and 113 deletions.
6 changes: 6 additions & 0 deletions .changes/unreleased/Improvements-684.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
component: runtime
kind: Improvements
body: Pass asset and archive paths as is to the engine
time: 2024-11-15T14:00:42.874578613Z
custom:
PR: "684"
1 change: 0 additions & 1 deletion cmd/pulumi-language-yaml/language_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,6 @@ var expectedFailures = map[string]string{
"l2-invoke-options": "cannot assign expression",
"l2-provider-grpc-config-schema-secret": "Detected a secret leak in state",
"l2-parameterized-resource": "could not load schema for subpackage, provider not known",
"l2-resource-asset-archive": "Argument must be a constant or contained in the project dir",
}

func TestLanguage(t *testing.T) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
name: l2-resource-asset-archive
runtime: yaml
main: subdir
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
data in a folder
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
resources:
ass:
type: asset-archive:AssetResource
properties:
value:
fn::FileAsset: ../test.txt
arc:
type: asset-archive:ArchiveResource
properties:
value:
fn::FileArchive: ../archive.tar
dir:
type: asset-archive:ArchiveResource
properties:
value:
fn::FileArchive: ../folder
assarc:
type: asset-archive:ArchiveResource
properties:
value:
fn::AssetArchive:
string:
fn::StringAsset: file contents
file:
fn::FileAsset: ../test.txt
folder:
fn::FileArchive: ../folder
archive:
fn::FileArchive: ../archive.tar
remoteass:
type: asset-archive:AssetResource
properties:
value:
fn::RemoteAsset: https://raw.githubusercontent.com/pulumi/pulumi/master/cmd/pulumi-test-language/testdata/l2-resource-asset-archive/test.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
packageDeclarationVersion: 1
name: asset-archive
version: 5.0.0
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
text
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
packageDeclarationVersion: 1
name: asset-archive
version: 5.0.0
78 changes: 2 additions & 76 deletions pkg/pulumiyaml/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -2279,7 +2279,6 @@ func (e *programEvaluator) evaluateBuiltinSecret(s *ast.SecretExpr) (interface{}
}

func (e *programEvaluator) evaluateInterpolatedBuiltinAssetArchive(x, s ast.Expr) (interface{}, bool) {
_, isConstant := s.(*ast.StringExpr)
v, b := e.evaluateExpr(s)
if !b {
return nil, false
Expand All @@ -2295,26 +2294,12 @@ func (e *programEvaluator) evaluateInterpolatedBuiltinAssetArchive(x, s ast.Expr
case *ast.StringAssetExpr:
return pulumi.NewStringAsset(value), true
case *ast.FileArchiveExpr:
path, err := e.sanitizePath(value, isConstant)
if err != nil {
return e.error(s, err.Error())
}
return pulumi.NewFileArchive(path), true
return pulumi.NewFileArchive(value), true
case *ast.FileAssetExpr:
path, err := e.sanitizePath(value, isConstant)
if err != nil {
return e.error(s, err.Error())
}
return pulumi.NewFileAsset(path), true
return pulumi.NewFileAsset(value), true
case *ast.RemoteArchiveExpr:
if !isConstant {
return e.error(s, "Argument to fn::remoteArchiveExpr must be a constantr")
}
return pulumi.NewRemoteArchive(value), true
case *ast.RemoteAssetExpr:
if !isConstant {
return e.error(s, "Argument to fn::remoteAssetExpr must be a constant")
}
return pulumi.NewRemoteAsset(value), true

}
Expand All @@ -2324,76 +2309,17 @@ func (e *programEvaluator) evaluateInterpolatedBuiltinAssetArchive(x, s ast.Expr
return createAssetArchiveF(v)
}

func (e *programEvaluator) sanitizePath(path string, isConstant bool) (string, error) {
path = filepath.Clean(path)
isAbsolute := filepath.IsAbs(path)
var err error
if !e.pulumiCtx.DryRun() {
path, err = filepath.EvalSymlinks(path)
if err != nil {
return "", fmt.Errorf("Error reading file at path %v: %w", path, err)
}
}

path, err = filepath.Abs(path)
if err != nil {
return "", fmt.Errorf("Error reading file at path %v: %w", path, err)
}
isSubdirectory := false
relPath, err := filepath.Rel(e.Runner.cwd, path)
if err != nil {
return "", fmt.Errorf("Error reading file at path %v: %w", path, err)
}

if !strings.HasPrefix(relPath, "../") {
isSubdirectory = true
}

isSafe := isSubdirectory || (isConstant && isAbsolute)
if !isSafe {
return "", fmt.Errorf("Argument must be a constant or contained in the project dir")
}
// Evaluate symlinks to ensure we don't escape the current project dir
// Compute the absolute path to use a prefix to check if we're relative
// Security, defense in depth: prevent path traversal exploits from leaking any information
// (secrets, tokens, ...) from outside the project directory.
//
// Allow subdirectory paths, these are valid constructions of the form:
//
// * "./README.md"
// * "${pulumi.cwd}/README.md"
// * ... etc
//
// Allow constant paths that are absolute, therefore reviewable:
//
// * /etc/lsb-release
// * /usr/share/nginx/html
// * /var/run/secrets/kubernetes.io/serviceaccount/token
//
// Forbidding parent directory path traversals (Path Traversal vulnerability):
//
// * ../../etc/shadow
// * ../../.ssh/id_rsa.pub
return path, nil
}

func (e *programEvaluator) evaluateBuiltinReadFile(s *ast.ReadFileExpr) (interface{}, bool) {
expr, ok := e.evaluateExpr(s.Path)
if !ok {
return nil, false
}

_, isConstant := s.Path.(*ast.StringExpr)

readFileF := e.lift(func(args ...interface{}) (interface{}, bool) {
path, ok := args[0].(string)
if !ok {
return e.error(s.Path, fmt.Sprintf("Argument to fn::readFile must be a string, got %v", reflect.TypeOf(args[0])))
}
path, err := e.sanitizePath(path, isConstant)
if err != nil {
return e.error(s, err.Error())
}
data, err := os.ReadFile(path)
if err != nil {
e.error(s.Path, fmt.Sprintf("Error reading file at path %v: %v", path, err))
Expand Down
37 changes: 1 addition & 36 deletions pkg/pulumiyaml/run_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -450,9 +450,7 @@ variables:
assert.Equal(t, assets["str"].(pulumi.Asset).Text(), "this is home")
assert.Equal(t, assets["strIter"].(pulumi.Asset).Text(), "start bar end")
assert.Equal(t, assets["away"].(pulumi.Asset).URI(), "example.org/asset")
filePath, err := filepath.Abs("./README.md")
assert.NoError(t, err)
assert.Equal(t, assets["local"].(pulumi.Asset).Path(), filePath)
assert.Equal(t, assets["local"].(pulumi.Asset).Path(), "./README.md")
assert.Equal(t, assets["folder"].(pulumi.Archive).Assets()["docs"].(pulumi.Archive).URI(), "example.org/docs")
})
}
Expand Down Expand Up @@ -1640,39 +1638,6 @@ variables:
})
}

// TestReadFileForbidsPathTraversal ensures that we forbid certain malicious path behaviors which
// allow escaping the project directory in static YAML.
//
// The example program uses a non-constant path which escapes the project directory.
//
// Non-constant paths which read from the project dir are considered safe, likely as uses of
// ${pulumi.cwd}, see above. Constant, absolute path are also permitted, sometimes necessary to use
// a secret or token.
func TestReadFileForbidsPathTraversal(t *testing.T) {
t.Parallel()

text := `
name: test-readfile
runtime: yaml
outputs:
readme:
fn::readFile: ${pulumi.cwd}/../../go.mod # imagine this is /etc/shadow, /var/run/secrets/tokens, etc.
`

tmpl := yamlTemplate(t, strings.TrimSpace(text))
diags := testTemplateSyntaxDiags(t, tmpl, func(r *Runner) {})

var diagStrings []string
for _, v := range diags {
diagStrings = append(diagStrings, diagString(v))
}
assert.ElementsMatch(t, diagStrings,
[]string{
"<stdin>:5:5: Argument must be a constant or contained in the project dir",
},
)
}

func TestJoinTemplate(t *testing.T) {
t.Parallel()

Expand Down

0 comments on commit 61b83d7

Please sign in to comment.