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: add nil/len guards for RunCommand #126

Merged
merged 3 commits into from
Oct 19, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion cmd/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ var (
runDir string
materialsPaths []string
productsPaths []string
noCommand bool
)

var runCmd = &cobra.Command{
Expand All @@ -23,7 +24,7 @@ files before command execution) and 'products' (i.e. files after command
execution) and stores them together with other information (executed command,
return value, stdout, stderr, ...) to a link metadata file, which is signed
with the passed key. Returns nonzero value on failure and zero otherwise.`,
Args: cobra.MinimumNArgs(1),
Args: cobra.MinimumNArgs(0),
PreRunE: getKeyCert,
RunE: run,
}
Expand Down Expand Up @@ -131,6 +132,14 @@ operating systems. It is done by replacing all line separators
with a new line character.`,
)

runCmd.Flags().BoolVarP(
&noCommand,
"no-command",
"x",
false,
`Indicate that there is no command to be executed for the step.`,
)

runCmd.Flags().StringVar(
&spiffeUDS,
"spiffe-workload-api-path",
Expand All @@ -141,6 +150,14 @@ with a new line character.`,
}

func run(cmd *cobra.Command, args []string) error {
if noCommand && len(args) > 0 {
return fmt.Errorf("command arguments passed with --no-command/-x flag")
}

if !noCommand && len(args) == 0 {
return fmt.Errorf("no command arguments passed, please specify or use --no-command option")
}

block, err := intoto.InTotoRun(stepName, runDir, materialsPaths, productsPaths, args, key, []string{"sha256"}, exclude, lStripPaths, lineNormalization)
if err != nil {
return fmt.Errorf("failed to create link metadata: %w", err)
Expand Down
1 change: 1 addition & 0 deletions doc/in-toto_run.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ in-toto run [flags]
-d, --metadata-directory string Directory to store link metadata (default "./")
-n, --name string Name used to associate the resulting link metadata
with the corresponding step defined in an in-toto layout.
-x, --no-command Indicate that there is no command to be executed for the step.
--normalize-line-endings Enable line normalization in order to support different
operating systems. It is done by replacing all line separators
with a new line character.
Expand Down
15 changes: 12 additions & 3 deletions in_toto/runlib.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ var ErrSymCycle = errors.New("symlink cycle detected")
// ErrUnsupportedHashAlgorithm signals a missing hash mapping in getHashMapping
var ErrUnsupportedHashAlgorithm = errors.New("unsupported hash algorithm detected")

var ErrEmptyCommandArgs = errors.New("the command args are empty")

// visitedSymlinks is a hashset that contains all paths that we have visited.
var visitedSymlinks Set

Expand Down Expand Up @@ -247,6 +249,9 @@ NOTE: Since stdout and stderr are captured, they cannot be seen during the
command execution.
*/
func RunCommand(cmdArgs []string, runDir string) (map[string]interface{}, error) {
if len(cmdArgs) == 0 {
return nil, ErrEmptyCommandArgs
}

cmd := exec.Command(cmdArgs[0], cmdArgs[1:]...)

Expand Down Expand Up @@ -298,9 +303,13 @@ func InTotoRun(name string, runDir string, materialPaths []string, productPaths
return linkMb, err
}

byProducts, err := RunCommand(cmdArgs, runDir)
if err != nil {
return linkMb, err
// make sure that we only run RunCommand if cmdArgs is not nil or empty
byProducts := map[string]interface{}{}
if len(cmdArgs) != 0 {
byProducts, err = RunCommand(cmdArgs, runDir)
if err != nil {
return linkMb, err
}
}

products, err := RecordArtifacts(productPaths, hashAlgorithms, gitignorePatterns, lStripPaths, lineNormalization)
Expand Down
41 changes: 41 additions & 0 deletions in_toto/runlib_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,23 @@ func TestRunCommand(t *testing.T) {
}
}

func TestRunCommandErrors(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Can / should we also test by invoking InTotoRun?

Copy link
Member

Choose a reason for hiding this comment

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

Oh whoops, I missed your comment on the PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes we can. I will add a few more tests for this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@adityasaky I had a closer look on this. We cannot test InTotoRun for it, because we do not fail if the cmdArgs are empty. If we would fail if cmdArgs is empty we would no longer support link files without commands:

	// make sure that we only run RunCommand if cmdArgs is not nil or empty
	var byProducts map[string]interface{}
	if cmdArgs != nil && len(cmdArgs) != 0 {
		byProducts, err = RunCommand(cmdArgs, runDir)
		if err != nil {
			return linkMb, err
		}
	}

See line 3 that checks for empty cmdArgs

Copy link
Member

Choose a reason for hiding this comment

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

Reviving this one. I see what you mean but I think it's okay to test that the no command flag works as intended and still results in a link, no?

Copy link
Member

@adityasaky adityasaky Oct 13, 2022

Choose a reason for hiding this comment

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

@shibumi can you take a look? I added a test checking if link metadata is successfully generated. Also caught that a minor difference in byProducts had crept in.

tables := []struct {
CmdArgs []string
RunDir string
ExpectedError error
}{
{nil, "", ErrEmptyCommandArgs},
{[]string{}, "", ErrEmptyCommandArgs},
}
for _, table := range tables {
_, err := RunCommand(table.CmdArgs, table.RunDir)
if !errors.Is(err, ErrEmptyCommandArgs) {
t.Errorf("RunCommand did not provoke expected error. Got: %s, want: %s", err, ErrEmptyCommandArgs)
}
}
}

func TestInTotoRun(t *testing.T) {
// Successfully run InTotoRun
linkName := "Name"
Expand Down Expand Up @@ -420,6 +437,30 @@ func TestInTotoRun(t *testing.T) {
}},
},
},
{[]string{"alice.pub"}, []string{"foo.tar.gz"}, []string{}, validKey, []string{"sha256"}, Metablock{
Signed: Link{
Name: linkName,
Type: "link",
Materials: map[string]interface{}{
"alice.pub": map[string]interface{}{
"sha256": "f051e8b561835b7b2aa7791db7bc72f2613411b0b7d428a0ac33d45b8c518039",
},
},
Products: map[string]interface{}{
"foo.tar.gz": map[string]interface{}{
"sha256": "52947cb78b91ad01fe81cd6aef42d1f6817e92b9e6936c1e5aabb7c98514f355",
},
},
ByProducts: map[string]interface{}{},
Command: []string{},
Environment: map[string]interface{}{},
},
Signatures: []Signature{{
KeyID: "be6371bc627318218191ce0780fd3183cce6c36da02938a477d2e4dfae1804a6",
Sig: "f4a2d468965d595b4d29615fb2083ef7ac22a948e1530925612d73ba580ce9765d93db7b7ed1b9755d96f13a6a1e858c64693c2f7adcb311afb28cb57fbadc0c",
}},
},
},
}

for _, table := range tablesCorrect {
Expand Down