Skip to content

Commit b603117

Browse files
priyawadhwapriyawadhwa
authored
Refactor sign command (#2388)
This way we don't have to pass in a bunch of arguments, should also make writing tests easier. Signed-off-by: Priya Wadhwa <[email protected]> Signed-off-by: Priya Wadhwa <[email protected]>
1 parent c3c4ea9 commit b603117

File tree

4 files changed

+86
-42
lines changed

4 files changed

+86
-42
lines changed

‎cmd/cosign/cli/sign.go‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -109,12 +109,7 @@ race conditions or (worse) malicious tampering.
109109
OIDCProvider: o.OIDC.Provider,
110110
SkipConfirmation: o.SkipConfirmation,
111111
}
112-
annotationsMap, err := o.AnnotationsMap()
113-
if err != nil {
114-
return err
115-
}
116-
if err := sign.SignCmd(ro, ko, o.Registry, annotationsMap.Annotations, args, o.Cert, o.CertChain, o.Upload,
117-
o.OutputSignature, o.OutputCertificate, o.PayloadPath, o.Force, o.Recursive, o.Attachment, o.NoTlogUpload); err != nil {
112+
if err := sign.SignCmd(ro, ko, *o, args); err != nil {
118113
if o.Attachment == "" {
119114
return fmt.Errorf("signing %v: %w", args, err)
120115
}

‎cmd/cosign/cli/sign/sign.go‎

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -123,9 +123,7 @@ func ParseOCIReference(refStr string, out io.Writer, opts ...name.Option) (name.
123123
}
124124

125125
// nolint
126-
func SignCmd(ro *options.RootOptions, ko options.KeyOpts, regOpts options.RegistryOptions, annotations map[string]interface{},
127-
imgs []string, certPath string, certChainPath string, upload bool, outputSignature, outputCertificate string,
128-
payloadPath string, force bool, recursive bool, attachment string, noTlogUpload bool) error {
126+
func SignCmd(ro *options.RootOptions, ko options.KeyOpts, signOpts options.SignOptions, imgs []string) error {
129127
if options.EnableExperimental() {
130128
if options.NOf(ko.KeyRef, ko.Sk) > 1 {
131129
return &options.KeyParseError{}
@@ -139,48 +137,53 @@ func SignCmd(ro *options.RootOptions, ko options.KeyOpts, regOpts options.Regist
139137
ctx, cancel := context.WithTimeout(context.Background(), ro.Timeout)
140138
defer cancel()
141139

142-
sv, err := SignerFromKeyOpts(ctx, certPath, certChainPath, ko)
140+
sv, err := SignerFromKeyOpts(ctx, signOpts.Cert, signOpts.CertChain, ko)
143141
if err != nil {
144142
return fmt.Errorf("getting signer: %w", err)
145143
}
146144
defer sv.Close()
147145
dd := cremote.NewDupeDetector(sv)
148146

149147
var staticPayload []byte
150-
if payloadPath != "" {
151-
fmt.Fprintln(os.Stderr, "Using payload from:", payloadPath)
152-
staticPayload, err = os.ReadFile(filepath.Clean(payloadPath))
148+
if signOpts.PayloadPath != "" {
149+
fmt.Fprintln(os.Stderr, "Using payload from:", signOpts.PayloadPath)
150+
staticPayload, err = os.ReadFile(filepath.Clean(signOpts.PayloadPath))
153151
if err != nil {
154152
return fmt.Errorf("payload from file: %w", err)
155153
}
156154
}
157155

158156
// Set up an ErrDone consideration to return along "success" paths
159157
var ErrDone error
160-
if !recursive {
158+
if !signOpts.Recursive {
161159
ErrDone = mutate.ErrSkipChildren
162160
}
163-
161+
regOpts := signOpts.Registry
164162
opts, err := regOpts.ClientOpts(ctx)
165163
if err != nil {
166164
return fmt.Errorf("constructing client options: %w", err)
167165
}
166+
am, err := signOpts.AnnotationsMap()
167+
if err != nil {
168+
return fmt.Errorf("getting annotations: %w", err)
169+
}
170+
annotations := am.Annotations
168171
for _, inputImg := range imgs {
169172
ref, err := ParseOCIReference(inputImg, os.Stderr, regOpts.NameOptions()...)
170173
if err != nil {
171174
return err
172175
}
173-
ref, err = GetAttachedImageRef(ref, attachment, opts...)
176+
ref, err = GetAttachedImageRef(ref, signOpts.Attachment, opts...)
174177
if err != nil {
175-
return fmt.Errorf("unable to resolve attachment %s for image %s", attachment, inputImg)
178+
return fmt.Errorf("unable to resolve attachment %s for image %s", signOpts.Attachment, inputImg)
176179
}
177180

178-
if digest, ok := ref.(name.Digest); ok && !recursive {
181+
if digest, ok := ref.(name.Digest); ok && !signOpts.Recursive {
179182
se, err := ociremote.SignedEntity(ref, opts...)
180183
if err != nil {
181184
return fmt.Errorf("accessing image: %w", err)
182185
}
183-
err = signDigest(ctx, digest, staticPayload, ko, regOpts, annotations, upload, outputSignature, outputCertificate, force, recursive, noTlogUpload, dd, sv, se)
186+
err = signDigest(ctx, digest, staticPayload, ko, regOpts, annotations, signOpts.Upload, signOpts.OutputSignature, signOpts.OutputCertificate, signOpts.Force, signOpts.Recursive, signOpts.NoTlogUpload, dd, sv, se)
184187
if err != nil {
185188
return fmt.Errorf("signing digest: %w", err)
186189
}
@@ -200,7 +203,7 @@ func SignCmd(ro *options.RootOptions, ko options.KeyOpts, regOpts options.Regist
200203
}
201204
digest := ref.Context().Digest(d.String())
202205

203-
err = signDigest(ctx, digest, staticPayload, ko, regOpts, annotations, upload, outputSignature, outputCertificate, force, recursive, noTlogUpload, dd, sv, se)
206+
err = signDigest(ctx, digest, staticPayload, ko, regOpts, annotations, signOpts.Upload, signOpts.OutputSignature, signOpts.OutputCertificate, signOpts.Force, signOpts.Recursive, signOpts.NoTlogUpload, dd, sv, se)
204207
if err != nil {
205208
return fmt.Errorf("signing digest: %w", err)
206209
}

‎cmd/cosign/cli/sign/sign_test.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,8 @@ func TestSignCmdLocalKeyAndSk(t *testing.T) {
120120
Sk: true,
121121
},
122122
} {
123-
err := SignCmd(ro, ko, options.RegistryOptions{}, nil, nil, "", "", false, "", "", "", false, false, "", false)
123+
so := options.SignOptions{}
124+
err := SignCmd(ro, ko, so, nil)
124125
if (errors.Is(err, &options.KeyParseError{}) == false) {
125126
t.Fatal("expected KeyParseError")
126127
}

‎test/e2e_test.go‎

Lines changed: 66 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,10 @@ func TestSignVerify(t *testing.T) {
132132

133133
// Now sign the image
134134
ko := options.KeyOpts{KeyRef: privKeyPath, PassFunc: passFunc}
135-
must(sign.SignCmd(ro, ko, options.RegistryOptions{}, nil, []string{imgName}, "", "", true, "", "", "", false, false, "", false), t)
135+
so := options.SignOptions{
136+
Upload: true,
137+
}
138+
must(sign.SignCmd(ro, ko, so, []string{imgName}), t)
136139

137140
// Now verify and download should work!
138141
must(verify(pubKeyPath, imgName, true, nil, ""), t)
@@ -141,9 +144,11 @@ func TestSignVerify(t *testing.T) {
141144
// Look for a specific annotation
142145
mustErr(verify(pubKeyPath, imgName, true, map[string]interface{}{"foo": "bar"}, ""), t)
143146

147+
so.AnnotationOptions = options.AnnotationOptions{
148+
Annotations: []string{"foo=bar"},
149+
}
144150
// Sign the image with an annotation
145-
annotations := map[string]interface{}{"foo": "bar"}
146-
must(sign.SignCmd(ro, ko, options.RegistryOptions{}, annotations, []string{imgName}, "", "", true, "", "", "", false, false, "", false), t)
151+
must(sign.SignCmd(ro, ko, so, []string{imgName}), t)
147152

148153
// It should match this time.
149154
must(verify(pubKeyPath, imgName, true, map[string]interface{}{"foo": "bar"}, ""), t)
@@ -167,7 +172,10 @@ func TestSignVerifyClean(t *testing.T) {
167172

168173
// Now sign the image
169174
ko := options.KeyOpts{KeyRef: privKeyPath, PassFunc: passFunc}
170-
must(sign.SignCmd(ro, ko, options.RegistryOptions{}, nil, []string{imgName}, "", "", true, "", "", "", false, false, "", false), t)
175+
so := options.SignOptions{
176+
Upload: true,
177+
}
178+
must(sign.SignCmd(ro, ko, so, []string{imgName}), t)
171179

172180
// Now verify and download should work!
173181
must(verify(pubKeyPath, imgName, true, nil, ""), t)
@@ -196,7 +204,10 @@ func TestImportSignVerifyClean(t *testing.T) {
196204

197205
// Now sign the image
198206
ko := options.KeyOpts{KeyRef: privKeyPath, PassFunc: passFunc}
199-
must(sign.SignCmd(ro, ko, options.RegistryOptions{}, nil, []string{imgName}, "", "", true, "", "", "", false, false, "", false), t)
207+
so := options.SignOptions{
208+
Upload: true,
209+
}
210+
must(sign.SignCmd(ro, ko, so, []string{imgName}), t)
200211

201212
// Now verify and download should work!
202213
must(verify(pubKeyPath, imgName, true, nil, ""), t)
@@ -453,9 +464,12 @@ func TestRekorBundle(t *testing.T) {
453464
PassFunc: passFunc,
454465
RekorURL: rekorURL,
455466
}
467+
so := options.SignOptions{
468+
Upload: true,
469+
}
456470

457471
// Sign the image
458-
must(sign.SignCmd(ro, ko, options.RegistryOptions{}, nil, []string{imgName}, "", "", true, "", "", "", false, false, "", false), t)
472+
must(sign.SignCmd(ro, ko, so, []string{imgName}), t)
459473
// Make sure verify works
460474
must(verify(pubKeyPath, imgName, true, nil, ""), t)
461475

@@ -485,14 +499,17 @@ func TestDuplicateSign(t *testing.T) {
485499

486500
// Now sign the image
487501
ko := options.KeyOpts{KeyRef: privKeyPath, PassFunc: passFunc}
488-
must(sign.SignCmd(ro, ko, options.RegistryOptions{}, nil, []string{imgName}, "", "", true, "", "", "", false, false, "", false), t)
502+
so := options.SignOptions{
503+
Upload: true,
504+
}
505+
must(sign.SignCmd(ro, ko, so, []string{imgName}), t)
489506

490507
// Now verify and download should work!
491508
must(verify(pubKeyPath, imgName, true, nil, ""), t)
492509
must(download.SignatureCmd(ctx, options.RegistryOptions{}, imgName), t)
493510

494511
// Signing again should work just fine...
495-
must(sign.SignCmd(ro, ko, options.RegistryOptions{}, nil, []string{imgName}, "", "", true, "", "", "", false, false, "", false), t)
512+
must(sign.SignCmd(ro, ko, so, []string{imgName}), t)
496513

497514
se, err := ociremote.SignedEntity(ref, ociremote.WithRemoteOptions(registryClientOpts(ctx)...))
498515
must(err, t)
@@ -588,14 +605,17 @@ func TestMultipleSignatures(t *testing.T) {
588605

589606
// Now sign the image with one key
590607
ko := options.KeyOpts{KeyRef: priv1, PassFunc: passFunc}
591-
must(sign.SignCmd(ro, ko, options.RegistryOptions{}, nil, []string{imgName}, "", "", true, "", "", "", false, false, "", false), t)
608+
so := options.SignOptions{
609+
Upload: true,
610+
}
611+
must(sign.SignCmd(ro, ko, so, []string{imgName}), t)
592612
// Now verify should work with that one, but not the other
593613
must(verify(pub1, imgName, true, nil, ""), t)
594614
mustErr(verify(pub2, imgName, true, nil, ""), t)
595615

596616
// Now sign with the other key too
597617
ko.KeyRef = priv2
598-
must(sign.SignCmd(ro, ko, options.RegistryOptions{}, nil, []string{imgName}, "", "", true, "", "", "", false, false, "", false), t)
618+
must(sign.SignCmd(ro, ko, so, []string{imgName}), t)
599619

600620
// Now verify should work with both
601621
must(verify(pub1, imgName, true, nil, ""), t)
@@ -977,7 +997,10 @@ func TestSaveLoad(t *testing.T) {
977997
ctx := context.Background()
978998
// Now sign the image and verify it
979999
ko := options.KeyOpts{KeyRef: privKeyPath, PassFunc: passFunc}
980-
must(sign.SignCmd(ro, ko, options.RegistryOptions{}, nil, []string{imgName}, "", "", true, "", "", "", false, false, "", false), t)
1000+
so := options.SignOptions{
1001+
Upload: true,
1002+
}
1003+
must(sign.SignCmd(ro, ko, so, []string{imgName}), t)
9811004
must(verify(pubKeyPath, imgName, true, nil, ""), t)
9821005

9831006
// save the image to a temp dir
@@ -1010,7 +1033,10 @@ func TestSaveLoadAttestation(t *testing.T) {
10101033
ctx := context.Background()
10111034
// Now sign the image and verify it
10121035
ko := options.KeyOpts{KeyRef: privKeyPath, PassFunc: passFunc}
1013-
must(sign.SignCmd(ro, ko, options.RegistryOptions{}, nil, []string{imgName}, "", "", true, "", "", "", false, false, "", false), t)
1036+
so := options.SignOptions{
1037+
Upload: true,
1038+
}
1039+
must(sign.SignCmd(ro, ko, so, []string{imgName}), t)
10141040
must(verify(pubKeyPath, imgName, true, nil, ""), t)
10151041

10161042
// now, append an attestation to the image
@@ -1104,7 +1130,11 @@ func TestAttachSBOM(t *testing.T) {
11041130

11051131
// Now sign the sbom with one key
11061132
ko1 := options.KeyOpts{KeyRef: privKeyPath1, PassFunc: passFunc}
1107-
must(sign.SignCmd(ro, ko1, options.RegistryOptions{}, nil, []string{imgName}, "", "", true, "", "", "", false, false, "sbom", false), t)
1133+
so := options.SignOptions{
1134+
Upload: true,
1135+
Attachment: "sbom",
1136+
}
1137+
must(sign.SignCmd(ro, ko1, so, []string{imgName}), t)
11081138

11091139
// Now verify should work with that one, but not the other
11101140
must(verify(pubKeyPath1, imgName, true, nil, "sbom"), t)
@@ -1141,7 +1171,10 @@ func TestTlog(t *testing.T) {
11411171
PassFunc: passFunc,
11421172
RekorURL: rekorURL,
11431173
}
1144-
must(sign.SignCmd(ro, ko, options.RegistryOptions{}, nil, []string{imgName}, "", "", true, "", "", "", false, false, "", false), t)
1174+
so := options.SignOptions{
1175+
Upload: true,
1176+
}
1177+
must(sign.SignCmd(ro, ko, so, []string{imgName}), t)
11451178

11461179
// Now verify should work!
11471180
must(verify(pubKeyPath, imgName, true, nil, ""), t)
@@ -1153,7 +1186,7 @@ func TestTlog(t *testing.T) {
11531186
mustErr(verify(pubKeyPath, imgName, true, nil, ""), t)
11541187

11551188
// Sign again with the tlog env var on
1156-
must(sign.SignCmd(ro, ko, options.RegistryOptions{}, nil, []string{imgName}, "", "", true, "", "", "", false, false, "", false), t)
1189+
must(sign.SignCmd(ro, ko, so, []string{imgName}), t)
11571190
// And now verify works!
11581191
must(verify(pubKeyPath, imgName, true, nil, ""), t)
11591192
}
@@ -1179,7 +1212,10 @@ func TestNoTlog(t *testing.T) {
11791212
PassFunc: passFunc,
11801213
RekorURL: rekorURL,
11811214
}
1182-
must(sign.SignCmd(ro, ko, options.RegistryOptions{}, nil, []string{imgName}, "", "", true, "", "", "", false, false, "", false), t)
1215+
so := options.SignOptions{
1216+
Upload: true,
1217+
}
1218+
must(sign.SignCmd(ro, ko, so, []string{imgName}), t)
11831219

11841220
// Now verify should work!
11851221
must(verify(pubKeyPath, imgName, true, nil, ""), t)
@@ -1191,7 +1227,10 @@ func TestNoTlog(t *testing.T) {
11911227
mustErr(verify(pubKeyPath, imgName, true, nil, ""), t)
11921228

11931229
// Sign again with the tlog env var on with option to not upload tlog
1194-
must(sign.SignCmd(ro, ko, options.RegistryOptions{}, nil, []string{imgName}, "", "", true, "", "", "", false, false, "", true), t)
1230+
so = options.SignOptions{
1231+
NoTlogUpload: true,
1232+
}
1233+
must(sign.SignCmd(ro, ko, so, []string{imgName}), t)
11951234
// And verify it still fails.
11961235
mustErr(verify(pubKeyPath, imgName, true, nil, ""), t)
11971236
}
@@ -1353,9 +1392,11 @@ func TestInvalidBundle(t *testing.T) {
13531392
defer setenv(t, env.VariableExperimental.String(), "1")()
13541393
remoteOpts := ociremote.WithRemoteOptions(registryClientOpts(ctx)...)
13551394
ko := options.KeyOpts{KeyRef: privKeyPath, PassFunc: passFunc, RekorURL: rekorURL}
1356-
regOpts := options.RegistryOptions{}
1357-
1358-
must(sign.SignCmd(ro, ko, regOpts, nil, []string{img1}, "", "", true, "", "", "", true, false, "", true), t)
1395+
so := options.SignOptions{
1396+
Upload: true,
1397+
Force: true,
1398+
}
1399+
must(sign.SignCmd(ro, ko, so, []string{img1}), t)
13591400
// verify image1
13601401
must(verify(pubKeyPath, img1, true, nil, ""), t)
13611402
// extract the bundle from image1
@@ -1380,7 +1421,11 @@ func TestInvalidBundle(t *testing.T) {
13801421
img2 := path.Join(regName, "unrelated")
13811422
imgRef2, _, cleanup := mkimage(t, img2)
13821423
defer cleanup()
1383-
must(sign.SignCmd(ro, ko, regOpts, nil, []string{img2}, "", "", true, "", "", "", false, false, "", true), t)
1424+
so = options.SignOptions{
1425+
Upload: true,
1426+
NoTlogUpload: true,
1427+
}
1428+
must(sign.SignCmd(ro, ko, so, []string{img2}), t)
13841429
must(verify(pubKeyPath, img2, true, nil, ""), t)
13851430

13861431
si2, err := ociremote.SignedEntity(imgRef2, remoteOpts)

0 commit comments

Comments
 (0)