From 4a8e7ad5fd41988c38beb9cf3ff1d1dd04ae4ce8 Mon Sep 17 00:00:00 2001
From: Ian Lewis <ianlewis@google.com>
Date: Tue, 26 Jul 2022 08:19:40 +0900
Subject: [PATCH] Fix support for --signature="" (#615)

---
 .github/workflows/generator_generic_slsa3.yml |  7 ++++
 internal/builders/generic/attest.go           | 32 +++++++++++--------
 internal/builders/generic/attest_test.go      | 16 ++++++++++
 internal/builders/generic/main.go             |  2 +-
 4 files changed, 43 insertions(+), 14 deletions(-)

diff --git a/.github/workflows/generator_generic_slsa3.yml b/.github/workflows/generator_generic_slsa3.yml
index 015d9be09..50da4ba62 100644
--- a/.github/workflows/generator_generic_slsa3.yml
+++ b/.github/workflows/generator_generic_slsa3.yml
@@ -114,6 +114,13 @@ jobs:
           UNTRUSTED_ATTESTATION_NAME: "${{ inputs.attestation-name }}"
         run: |
           set -euo pipefail
+          # NOTE: The generator binary allows the attestation to be "" in which
+          # case it does not sign or generate provenance. However, this workflow
+          # requires it to be non-empty so we validate it here.
+          if [ "$UNTRUSTED_ATTESTATION_NAME" == "" ]; then
+            echo "attestation-name cannot be empty."
+            exit 5
+          fi
           # Create and sign provenance.
           # Note: The builder verifies that the UNTRUSTED_ATTESTATION_NAME is located
           # in the current directory.
diff --git a/internal/builders/generic/attest.go b/internal/builders/generic/attest.go
index 369803fdd..4f5b12dba 100644
--- a/internal/builders/generic/attest.go
+++ b/internal/builders/generic/attest.go
@@ -133,7 +133,7 @@ func (b *provenanceOnlyBuild) URI() string {
 }
 
 // attestCmd returns the 'attest' command.
-func attestCmd() *cobra.Command {
+func attestCmd(provider slsa.ClientProvider) *cobra.Command {
 	var predicatePath string
 	var attPath string
 	var subjects string
@@ -148,15 +148,13 @@ run in the context of a Github Actions workflow.`,
 		Run: func(cmd *cobra.Command, args []string) {
 			ghContext, err := github.GetWorkflowContext()
 			check(err)
-
-			// Verify the extension path and extension.
-			err = utils.VerifyAttestationPath(attPath)
-			check(err)
-
 			var parsedSubjects []intoto.Subject
 			// We don't actually care about the subjects if we aren't writing an attestation.
 			if attPath != "" {
-				var err error
+				// Verify the extension path and extension.
+				err = utils.VerifyAttestationPath(attPath)
+				check(err)
+
 				parsedSubjects, err = parseSubjects(subjects)
 				check(err)
 
@@ -170,15 +168,23 @@ run in the context of a Github Actions workflow.`,
 			b := provenanceOnlyBuild{
 				GithubActionsBuild: slsa.NewGithubActionsBuild(parsedSubjects, ghContext),
 			}
-			// TODO(github.com/slsa-framework/slsa-github-generator/issues/124): Remove
-			if utils.IsPresubmitTests() {
-				b.WithClients(&slsa.NilClientProvider{})
+			if provider != nil {
+				b.WithClients(provider)
+			} else {
+				// TODO(github.com/slsa-framework/slsa-github-generator/issues/124): Remove
+				if utils.IsPresubmitTests() {
+					b.WithClients(&slsa.NilClientProvider{})
+				}
 			}
 
 			g := slsa.NewHostedActionsGenerator(&b)
-			// TODO(github.com/slsa-framework/slsa-github-generator/issues/124): Remove
-			if utils.IsPresubmitTests() {
-				g.WithClients(&slsa.NilClientProvider{})
+			if provider != nil {
+				g.WithClients(provider)
+			} else {
+				// TODO(github.com/slsa-framework/slsa-github-generator/issues/124): Remove
+				if utils.IsPresubmitTests() {
+					g.WithClients(&slsa.NilClientProvider{})
+				}
 			}
 
 			p, err := g.Generate(ctx)
diff --git a/internal/builders/generic/attest_test.go b/internal/builders/generic/attest_test.go
index 6750a8450..b4ae28422 100644
--- a/internal/builders/generic/attest_test.go
+++ b/internal/builders/generic/attest_test.go
@@ -1,6 +1,7 @@
 package main
 
 import (
+	"bytes"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -9,6 +10,7 @@ import (
 	slsav02 "github.com/in-toto/in-toto-golang/in_toto/slsa_provenance/v0.2"
 
 	"github.com/slsa-framework/slsa-github-generator/internal/errors"
+	"github.com/slsa-framework/slsa-github-generator/slsa"
 )
 
 // TestParseSubjects tests the parseSubjects function.
@@ -145,3 +147,17 @@ func TestParseSubjects(t *testing.T) {
 		})
 	}
 }
+
+// Test_attestCmd tests the attest command.
+func Test_attestCmd(t *testing.T) {
+	t.Run("empty attestation path", func(t *testing.T) {
+		t.Setenv("GITHUB_CONTEXT", "{}")
+
+		c := attestCmd(&slsa.NilClientProvider{})
+		c.SetOut(new(bytes.Buffer))
+		c.SetArgs([]string{"--signature", ""})
+		if err := c.Execute(); err != nil {
+			t.Errorf("unexpected failure: %v", err)
+		}
+	})
+}
diff --git a/internal/builders/generic/main.go b/internal/builders/generic/main.go
index cdc070147..475ca2ec6 100644
--- a/internal/builders/generic/main.go
+++ b/internal/builders/generic/main.go
@@ -44,7 +44,7 @@ For more information on SLSA, visit https://slsa.dev`,
 		},
 	}
 	c.AddCommand(versionCmd())
-	c.AddCommand(attestCmd())
+	c.AddCommand(attestCmd(nil))
 	return c
 }