Backport of cleanup: tidy up helper package some more into release/1.…

…3.x (#14394)

This pull request was automerged via backport-assistant

client/allocrunner/taskrunner/getter/getter_test.go

@@ -19,7 +19,7 @@ import (
 	clientconfig "github.com/hashicorp/nomad/client/config"
 	"github.com/hashicorp/nomad/client/interfaces"
 	"github.com/hashicorp/nomad/client/taskenv"
-	"github.com/hashicorp/nomad/helper"
+	"github.com/hashicorp/nomad/helper/escapingfs"
 	"github.com/hashicorp/nomad/nomad/mock"
 	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/stretchr/testify/require"
@@ -31,11 +31,11 @@ type noopReplacer struct {
 }
 
 func clientPath(taskDir, path string, join bool) (string, bool) {
-	if !filepath.IsAbs(path) || (helper.PathEscapesSandbox(taskDir, path) && join) {
+	if !filepath.IsAbs(path) || (escapingfs.PathEscapesSandbox(taskDir, path) && join) {
 		path = filepath.Join(taskDir, path)
 	}
 	path = filepath.Clean(path)
-	if taskDir != "" && !helper.PathEscapesSandbox(taskDir, path) {
+	if taskDir != "" && !escapingfs.PathEscapesSandbox(taskDir, path) {
 		return path, false
 	}
 	return path, true

client/client.go

@@ -2788,7 +2788,7 @@ func (c *Client) consulDiscoveryImpl() error {
 		// datacenterQueryLimit, the next heartbeat will pick
 		// a new set of servers so it's okay.
 		shuffleStrings(dcs[1:])
-		dcs = dcs[0:helper.MinInt(len(dcs), datacenterQueryLimit)]
+		dcs = dcs[0:helper.Min(len(dcs), datacenterQueryLimit)]
 	}
 
 	// Query for servers in this client's region only

client/taskenv/env.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/hashicorp/nomad/helper"
 	hargs "github.com/hashicorp/nomad/helper/args"
+	"github.com/hashicorp/nomad/helper/escapingfs"
 	"github.com/hashicorp/nomad/lib/cpuset"
 	"github.com/hashicorp/nomad/nomad/structs"
 	"github.com/hashicorp/nomad/plugins/drivers"
@@ -341,7 +342,7 @@ func (t *TaskEnv) replaceEnvClient(arg string) string {
 // directory path fields of this TaskEnv
 func (t *TaskEnv) checkEscape(testPath string) bool {
 	for _, p := range []string{t.clientTaskDir, t.clientSharedAllocDir} {
-		if p != "" && !helper.PathEscapesSandbox(p, testPath) {
+		if p != "" && !escapingfs.PathEscapesSandbox(p, testPath) {
 			return false
 		}
 	}

command/agent/agent.go

@@ -24,8 +24,8 @@ import (
 	"github.com/hashicorp/nomad/client/state"
 	"github.com/hashicorp/nomad/command/agent/consul"
 	"github.com/hashicorp/nomad/command/agent/event"
-	"github.com/hashicorp/nomad/helper"
 	"github.com/hashicorp/nomad/helper/bufconndialer"
+	"github.com/hashicorp/nomad/helper/escapingfs"
 	"github.com/hashicorp/nomad/helper/pluginutils/loader"
 	"github.com/hashicorp/nomad/helper/uuid"
 	"github.com/hashicorp/nomad/lib/cpuset"
@@ -865,7 +865,7 @@ func (a *Agent) setupNodeID(config *nomad.Config) error {
 			return err
 		}
 		// Persist this configured nodeID to our data directory
-		if err := helper.EnsurePath(fileID, false); err != nil {
+		if err := escapingfs.EnsurePath(fileID, false); err != nil {
 			return err
 		}
 		if err := ioutil.WriteFile(fileID, []byte(config.NodeID), 0600); err != nil {
@@ -877,7 +877,7 @@ func (a *Agent) setupNodeID(config *nomad.Config) error {
 	// If we still don't have a valid node ID, make one.
 	if config.NodeID == "" {
 		id := uuid.Generate()
-		if err := helper.EnsurePath(fileID, false); err != nil {
+		if err := escapingfs.EnsurePath(fileID, false); err != nil {
 			return err
 		}
 		if err := ioutil.WriteFile(fileID, []byte(id), 0600); err != nil {

command/operator_debug.go

@@ -27,6 +27,7 @@ import (
 	"github.com/hashicorp/nomad/api"
 	"github.com/hashicorp/nomad/api/contexts"
 	"github.com/hashicorp/nomad/helper"
+	"github.com/hashicorp/nomad/helper/escapingfs"
 	"github.com/hashicorp/nomad/version"
 	"github.com/posener/complete"
 )
@@ -729,7 +730,7 @@ func (c *OperatorDebugCommand) mkdir(paths ...string) error {
 	joinedPath := c.path(paths...)
 
 	// Ensure path doesn't escape the sandbox of the capture directory
-	escapes := helper.PathEscapesSandbox(c.collectDir, joinedPath)
+	escapes := escapingfs.PathEscapesSandbox(c.collectDir, joinedPath)
 	if escapes {
 		return fmt.Errorf("file path escapes capture directory")
 	}
@@ -1273,7 +1274,7 @@ func (c *OperatorDebugCommand) writeBytes(dir, file string, data []byte) error {
 	}
 
 	// Ensure filename doesn't escape the sandbox of the capture directory
-	escapes := helper.PathEscapesSandbox(c.collectDir, filePath)
+	escapes := escapingfs.PathEscapesSandbox(c.collectDir, filePath)
 	if escapes {
 		return fmt.Errorf("file path \"%s\" escapes capture directory \"%s\"", filePath, c.collectDir)
 	}

helper/escapingfs/escapes.go

@@ -97,3 +97,25 @@ func PathEscapesAllocDir(base, prefix, path string) (bool, error) {
 
 	return false, nil
 }
+
+// PathEscapesSandbox returns whether previously cleaned path inside the
+// sandbox directory (typically this will be the allocation directory)
+// escapes.
+func PathEscapesSandbox(sandboxDir, path string) bool {
+	rel, err := filepath.Rel(sandboxDir, path)
+	if err != nil {
+		return true
+	}
+	if strings.HasPrefix(rel, "..") {
+		return true
+	}
+	return false
+}
+
+// EnsurePath is used to make sure a path exists
+func EnsurePath(path string, dir bool) error {
+	if !dir {
+		path = filepath.Dir(path)
+	}
+	return os.MkdirAll(path, 0755)
+}

helper/escapingfs/escapes_test.go

@@ -1,6 +1,7 @@
 package escapingfs
 
 import (
+	"fmt"
 	"io/ioutil"
 	"os"
 	"path/filepath"
@@ -141,3 +142,88 @@ func Test_PathEscapesAllocDir(t *testing.T) {
 		require.True(t, escape)
 	})
 }
+
+func TestPathEscapesSandbox(t *testing.T) {
+	cases := []struct {
+		name     string
+		path     string
+		dir      string
+		expected bool
+	}{
+		{
+			// this is the ${NOMAD_SECRETS_DIR} case
+			name:     "ok joined absolute path inside sandbox",
+			path:     filepath.Join("/alloc", "/secrets"),
+			dir:      "/alloc",
+			expected: false,
+		},
+		{
+			name:     "fail unjoined absolute path outside sandbox",
+			path:     "/secrets",
+			dir:      "/alloc",
+			expected: true,
+		},
+		{
+			name:     "ok joined relative path inside sandbox",
+			path:     filepath.Join("/alloc", "./safe"),
+			dir:      "/alloc",
+			expected: false,
+		},
+		{
+			name:     "fail unjoined relative path outside sandbox",
+			path:     "./safe",
+			dir:      "/alloc",
+			expected: true,
+		},
+		{
+			name:     "ok relative path traversal constrained to sandbox",
+			path:     filepath.Join("/alloc", "../../alloc/safe"),
+			dir:      "/alloc",
+			expected: false,
+		},
+		{
+			name:     "ok unjoined absolute path traversal constrained to sandbox",
+			path:     filepath.Join("/alloc", "/../alloc/safe"),
+			dir:      "/alloc",
+			expected: false,
+		},
+		{
+			name:     "ok unjoined absolute path traversal constrained to sandbox",
+			path:     "/../alloc/safe",
+			dir:      "/alloc",
+			expected: false,
+		},
+		{
+			name:     "fail joined relative path traverses outside sandbox",
+			path:     filepath.Join("/alloc", "../../../unsafe"),
+			dir:      "/alloc",
+			expected: true,
+		},
+		{
+			name:     "fail unjoined relative path traverses outside sandbox",
+			path:     "../../../unsafe",
+			dir:      "/alloc",
+			expected: true,
+		},
+		{
+			name:     "fail joined absolute path tries to transverse outside sandbox",
+			path:     filepath.Join("/alloc", "/alloc/../../unsafe"),
+			dir:      "/alloc",
+			expected: true,
+		},
+		{
+			name:     "fail unjoined absolute path tries to transverse outside sandbox",
+			path:     "/alloc/../../unsafe",
+			dir:      "/alloc",
+			expected: true,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			caseMsg := fmt.Sprintf("path: %v\ndir: %v", tc.path, tc.dir)
+			escapes := PathEscapesSandbox(tc.dir, tc.path)
+			require.Equal(t, tc.expected, escapes, caseMsg)
+		})
+	}
+}

helper/funcs.go

@@ -568,20 +568,6 @@ func CheckNamespaceScope(provided string, requested []string) []string {
 	return nil
 }
 
-// PathEscapesSandbox returns whether previously cleaned path inside the
-// sandbox directory (typically this will be the allocation directory)
-// escapes.
-func PathEscapesSandbox(sandboxDir, path string) bool {
-	rel, err := filepath.Rel(sandboxDir, path)
-	if err != nil {
-		return true
-	}
-	if strings.HasPrefix(rel, "..") {
-		return true
-	}
-	return false
-}
-
 // StopFunc is used to stop a time.Timer created with NewSafeTimer
 type StopFunc func()
 

helper/funcs_test.go

@@ -2,7 +2,6 @@ package helper
 
 import (
 	"fmt"
-	"path/filepath"
 	"reflect"
 	"sort"
 	"testing"
@@ -428,91 +427,6 @@ func TestCheckNamespaceScope(t *testing.T) {
 	}
 }
 
-func TestPathEscapesSandbox(t *testing.T) {
-	cases := []struct {
-		name     string
-		path     string
-		dir      string
-		expected bool
-	}{
-		{
-			// this is the ${NOMAD_SECRETS_DIR} case
-			name:     "ok joined absolute path inside sandbox",
-			path:     filepath.Join("/alloc", "/secrets"),
-			dir:      "/alloc",
-			expected: false,
-		},
-		{
-			name:     "fail unjoined absolute path outside sandbox",
-			path:     "/secrets",
-			dir:      "/alloc",
-			expected: true,
-		},
-		{
-			name:     "ok joined relative path inside sandbox",
-			path:     filepath.Join("/alloc", "./safe"),
-			dir:      "/alloc",
-			expected: false,
-		},
-		{
-			name:     "fail unjoined relative path outside sandbox",
-			path:     "./safe",
-			dir:      "/alloc",
-			expected: true,
-		},
-		{
-			name:     "ok relative path traversal constrained to sandbox",
-			path:     filepath.Join("/alloc", "../../alloc/safe"),
-			dir:      "/alloc",
-			expected: false,
-		},
-		{
-			name:     "ok unjoined absolute path traversal constrained to sandbox",
-			path:     filepath.Join("/alloc", "/../alloc/safe"),
-			dir:      "/alloc",
-			expected: false,
-		},
-		{
-			name:     "ok unjoined absolute path traversal constrained to sandbox",
-			path:     "/../alloc/safe",
-			dir:      "/alloc",
-			expected: false,
-		},
-		{
-			name:     "fail joined relative path traverses outside sandbox",
-			path:     filepath.Join("/alloc", "../../../unsafe"),
-			dir:      "/alloc",
-			expected: true,
-		},
-		{
-			name:     "fail unjoined relative path traverses outside sandbox",
-			path:     "../../../unsafe",
-			dir:      "/alloc",
-			expected: true,
-		},
-		{
-			name:     "fail joined absolute path tries to transverse outside sandbox",
-			path:     filepath.Join("/alloc", "/alloc/../../unsafe"),
-			dir:      "/alloc",
-			expected: true,
-		},
-		{
-			name:     "fail unjoined absolute path tries to transverse outside sandbox",
-			path:     "/alloc/../../unsafe",
-			dir:      "/alloc",
-			expected: true,
-		},
-	}
-
-	for _, tc := range cases {
-		t.Run(tc.name, func(t *testing.T) {
-			caseMsg := fmt.Sprintf("path: %v\ndir: %v", tc.path, tc.dir)
-			escapes := PathEscapesSandbox(tc.dir, tc.path)
-			require.Equal(t, tc.expected, escapes, caseMsg)
-		})
-	}
-}
-
 func Test_NewSafeTimer(t *testing.T) {
 	t.Run("zero", func(t *testing.T) {
 		timer, stop := NewSafeTimer(0)

nomad/blocked_evals.go

@@ -528,7 +528,7 @@ func (b *BlockedEvals) unblock(computedClass, quota string, index uint64) {
 	// because any node could potentially be feasible.
 	numEscaped := len(b.escaped)
 	numQuotaLimit := 0
-	unblocked := make(map[*structs.Evaluation]string, helper.MaxInt(numEscaped, 4))
+	unblocked := make(map[*structs.Evaluation]string, helper.Max(numEscaped, 4))
 
 	if numEscaped != 0 && computedClass != "" {
 		for id, wrapped := range b.escaped {

nomad/server.go

@@ -928,7 +928,7 @@ func (s *Server) setupBootstrapHandler() error {
 			// walk all datacenter until it finds enough hosts to
 			// form a quorum.
 			shuffleStrings(dcs[1:])
-			dcs = dcs[0:helper.MinInt(len(dcs), datacenterQueryLimit)]
+			dcs = dcs[0:helper.Min(len(dcs), datacenterQueryLimit)]
 		}
 
 		nomadServerServiceName := s.config.ConsulConfig.ServerServiceName