continued sign refacc #1098

dekkagaijin · 2021-11-24T06:22:13Z

This continues to refactor signing operations re: #844 and #931

TODO:

move over fulcio operations as well
perform all signing operations via this new library
perform all attestation operations via this new library
for efficient reuse, expose the rekor client in tlog upload operations

NONE

mattmoor · 2021-11-25T19:19:21Z

internal/pkg/cosign/sign.go

+type SigningResults struct {
+	SignedPayload []byte
+	Signature     []byte
+	Pub           crypto.PublicKey
+	Cert, Chain   []byte
+	Bundle        *oci.Bundle
+
+	OCISignature oci.Signature
+	SignedEntity oci.SignedEntity
+}


The SignedEntity is what's passed in, and virtually all of the stuff on top is accessible from the oci.Signature, so I think the result should just be oci.Signature.

So it seems like this result struct is built up across multiple implementations that must be composed a particular way to work, which isn't really the goal of composition. 🤔

Given the pieces I'm seeing I think the interface should just be:

type Signer interface { Sign(ctx context.Context, payload []byte) (oci.Signature, error) }

roger. The only thing not captured by oci.Signature was the signature's public key

mattmoor · 2021-11-25T19:21:42Z

internal/pkg/cosign/sign.go

+	// Attach the signature to the entity.
+	newSE, err := mutate.AttachSignatureToEntity(results.SignedEntity, results.OCISignature, mutate.WithDupeDetector(sa.DD))
+	if err != nil {
+		return nil, err
+	}
+	results.SignedEntity = newSE


Don't conflate signing with mutating the signed entity to carry that signature. The signer should just sign, let the caller worry about this.

mattmoor · 2021-11-25T19:22:59Z

internal/pkg/cosign/sign.go

+	return &SigningResults{
+		Signature:     sig,
+		SignedPayload: req.SignaturePayload,
+		SignedEntity:  req.SignedEntity,


This seems to be the only use of req.SignedEntity. What are our expectations of the Payload?

I was thinking a prior step might want to modify the req.SignedEntity, which is immutable, so I wanted to return the actual Entity in the results.

Our expectations for payload are to be able to upload it to Rekor, but we can get that from the oci.Signature

mattmoor · 2021-11-25T19:25:24Z

cmd/cosign/cli/sign/sign.go

+	if err != nil {
+		return nil, errors.Wrap(err, "getting key from Fulcio")


This saves ~3 LoC and is less clear, I'd revert.

If you want to save 2 LoC without losing clarity, you can fold the call into the if:

if k, err = fulcio.NewSigner(...); err != nil { return nil, errors.Wrap(err, "getting key from Fulcio") }

Also, I would expect that ultimately the bonus logic in fulcioverifier just becomes yet-another composition of the Signer interface.

right, already have a TODO for it

mattmoor · 2021-11-25T19:51:45Z

pkg/oci/remote/write.go

@@ -74,7 +74,7 @@ func WriteSignedImageIndexImages(ref name.Reference, sii oci.SignedImageIndex, o
 func WriteSignatures(repo name.Repository, se oci.SignedEntity, opts ...Option) error {
 	o := makeOptions(repo, opts...)

-	// Access the signature list to publish
+	// Access the signature list to published


this is an incorrect correction

Signed-off-by: Jake Sanders <jsand@google.com>

mattmoor

I like the interface a lot more. Left a few comments inline, but at a high level:

Can we shuffle the package structure around so that implementations generally live in their own packages with private types and public ctors in terms of the interface?
As I was building up oci I was trying to ensure fairly high coverage as I went, and it should now be fairly high. Can we try to do the same here?

mattmoor · 2021-11-29T23:58:20Z

internal/pkg/cosign/sign.go

+)
+
+type Signer interface {
+	Sign(ctx context.Context, payload io.Reader) (oci.Signature, crypto.PublicKey, error)


Isn't the public key available from the oci.Signature?

nope ¯\(ツ)/¯

It's not stored in the registry, so that makes sense.

What are callers consuming it to do today?

bump this comment?

internal/pkg/cosign/sign.go

mattmoor · 2021-11-30T00:02:36Z

internal/pkg/cosign/sign.go

+	signatureoptions "github.com/sigstore/sigstore/pkg/signature/options"
+)
+
+type Signer interface {


This is where you really want good comments documenting the expectations.

I like the interface a lot more. Left a few comments inline, but at a high level:

Can we shuffle the package structure around so that implementations generally live in their own packages with private types and public ctors in terms of the interface?

yed

As I was building up oci I was trying to ensure fairly high coverage as I went, and it should now be fairly high. Can we try to do the same here?

Will do, but I'd like to have the package structure stable before I go implementing all the accoutrements for testing

internal/pkg/cosign/rekor.go

Signed-off-by: Jake Sanders <jsand@google.com>

mattmoor · 2021-11-30T02:45:57Z

internal/pkg/cosign/fulcio/fulcio.go

 // the resulting `Cert` and `Chain`, which are added to the returned `oci.Signature`
-type FulcioSignerWrapper struct {
-	Inner Signer
+type SignerWrapper struct {


This should be private now.

mattmoor · 2021-11-30T02:46:06Z

internal/pkg/cosign/fulcio/fulcio.go

@@ -77,3 +78,12 @@ func (fs *FulcioSignerWrapper) Sign(ctx context.Context, payload io.Reader) (oci

 	return newSig, pub, nil
 }
+
+// NewSigner returns a *SignerWrapper which signs and uploads the given payload to Fulcio.
+func NewSigner(inner cosign.Signer, cert, chain []byte) *SignerWrapper {


return the interface

mattmoor · 2021-11-30T02:46:26Z

internal/pkg/cosign/fulcio/fulcio.go

@@ -77,3 +78,12 @@ func (fs *FulcioSignerWrapper) Sign(ctx context.Context, payload io.Reader) (oci

 	return newSig, pub, nil
 }
+
+// NewSigner returns a *SignerWrapper which signs and uploads the given payload to Fulcio.


This comment could use work :)

your FACE could use work

mattmoor · 2021-11-30T02:49:30Z

internal/pkg/cosign/fulcio/fulcio.go

-	sig, pub, err := fs.Inner.Sign(ctx, payload)
+// Sign implements `cosign.Signer`
+func (fs *SignerWrapper) Sign(ctx context.Context, payload io.Reader) (oci.Signature, crypto.PublicKey, error) {
+	sig, pub, err := fs.inner.Sign(ctx, payload)


I don't really grok why this is wrapping any Signer instead of having a specific Signer implementation embedded here. Can this truly be composed with other Signer implementations arbitrarily? What other compositions make sense?

not quite arbitrarily but we made the Signer abstraction for a reason. I think noting the requirements for our public instance in comments would be a legitimate way of documenting them (vs. using the type system)

mattmoor · 2021-11-30T02:50:03Z

cmd/cosign/cli/sign/sign.go

@@ -212,16 +214,9 @@ func signDigest(ctx context.Context, digest name.Digest, payload []byte, ko KeyO
 	s = &icos.PayloadSigner{


I'd move the payload signer as well.

Signed-off-by: Jake Sanders <jsand@google.com>

mattmoor · 2021-11-30T05:05:26Z

cmd/cosign/cli/sign/sign.go

-		return nil
+	var s icos.Signer
+	s = ipayload.NewSigner(sv, nil, nil)
+	s = ifulcio.NewSigner(s, sv.Cert, sv.Chain)


A few things about this line don't sit right with me:

Shouldn't it be predicated on some condition?

I still don't like the fact that the fulcio Signer wraps another random Signer where we have zero visibility into what it does. I think the reality here is that the payload Signer is doing the fulcio signing, and the fulcio Signer is just decorating the resulting oci.Signature to match. I think this is a fragile composition, and I think I'd rather see payload's Signer be a hidden implementation detail of a fulcio Signer than allow arbitrary composition here.

yes :p

"TODO:"
"* move over fulcio operations as well"

mattmoor

Let's land this and keep going :)

dekkagaijin force-pushed the sign-refacc branch 7 times, most recently from ab9569d to 153228d Compare November 25, 2021 03:36

dekkagaijin marked this pull request as ready for review November 25, 2021 04:15

dekkagaijin changed the title ~~WIP: continued sign refacc~~ continued sign refacc Nov 25, 2021

dekkagaijin requested a review from mattmoor November 25, 2021 04:24

mattmoor reviewed Nov 25, 2021

View reviewed changes

dekkagaijin force-pushed the sign-refacc branch 3 times, most recently from e47eec1 to 4c135b6 Compare November 29, 2021 23:46

Jake Sanders added 6 commits November 29, 2021 15:54

I eata da spaghetti

865342a

Signed-off-by: Jake Sanders <jsand@google.com>

decompose StaticSigner into individual signing stages

764ef20

Signed-off-by: Jake Sanders <jsand@google.com>

move rekor upload logic to RekorSignerWrapper

ad6188e

Signed-off-by: Jake Sanders <jsand@google.com>

select signing flows based on input variables

5a1540a

Signed-off-by: Jake Sanders <jsand@google.com>

add comments to the Signer implementations

3f5bf27

Signed-off-by: Jake Sanders <jsand@google.com>

only perform signing actions in cosign.Signer

a99d528

Signed-off-by: Jake Sanders <jsand@google.com>

dekkagaijin force-pushed the sign-refacc branch from 4c135b6 to a99d528 Compare November 29, 2021 23:54

mattmoor reviewed Nov 30, 2021

View reviewed changes

Jake Sanders added 3 commits November 29, 2021 16:50

flip comments on Signer implementations

29b2e61

Signed-off-by: Jake Sanders <jsand@google.com>

staticly assert that impls are impls

04cc55f

Signed-off-by: Jake Sanders <jsand@google.com>

split Rekor and Fulcio signers into their own packages

66c3a2c

Signed-off-by: Jake Sanders <jsand@google.com>

mattmoor reviewed Nov 30, 2021

View reviewed changes

code review feedback

5116c1e

Signed-off-by: Jake Sanders <jsand@google.com>

dekkagaijin force-pushed the sign-refacc branch from dce5922 to 5116c1e Compare November 30, 2021 03:22

mattmoor reviewed Nov 30, 2021

View reviewed changes

mattmoor approved these changes Nov 30, 2021

View reviewed changes

dekkagaijin merged commit 304c2b2 into sigstore:main Nov 30, 2021

github-actions bot added this to the v1.4.0 milestone Nov 30, 2021

dekkagaijin deleted the sign-refacc branch December 3, 2021 00:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

continued sign refacc #1098

continued sign refacc #1098

dekkagaijin commented Nov 24, 2021 •

edited

Loading

mattmoor Nov 25, 2021

mattmoor Nov 25, 2021

dekkagaijin Nov 30, 2021

mattmoor Nov 25, 2021

dekkagaijin Nov 29, 2021

mattmoor Nov 25, 2021

dekkagaijin Nov 29, 2021 •

edited

Loading

mattmoor Nov 25, 2021

mattmoor Nov 25, 2021

dekkagaijin Nov 29, 2021

mattmoor Nov 25, 2021

dekkagaijin Nov 29, 2021

mattmoor left a comment

mattmoor Nov 29, 2021

dekkagaijin Nov 30, 2021 •

edited

Loading

mattmoor Nov 30, 2021

mattmoor Nov 30, 2021

mattmoor Nov 30, 2021

dekkagaijin Nov 30, 2021

mattmoor Nov 30, 2021

dekkagaijin Nov 30, 2021

mattmoor Nov 30, 2021

dekkagaijin Nov 30, 2021

mattmoor Nov 30, 2021

dekkagaijin Nov 30, 2021 •

edited

Loading

mattmoor Nov 30, 2021

dekkagaijin Nov 30, 2021

mattmoor Nov 30, 2021

dekkagaijin Nov 30, 2021

mattmoor Nov 30, 2021

dekkagaijin Nov 30, 2021

mattmoor left a comment

		if err != nil {
		return nil, errors.Wrap(err, "getting key from Fulcio")

		@@ -212,16 +214,9 @@ func signDigest(ctx context.Context, digest name.Digest, payload []byte, ko KeyO
		s = &icos.PayloadSigner{

continued sign refacc #1098

continued sign refacc #1098

Conversation

dekkagaijin commented Nov 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dekkagaijin Nov 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dekkagaijin Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dekkagaijin Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

dekkagaijin commented Nov 24, 2021 •

edited

Loading

dekkagaijin Nov 29, 2021 •

edited

Loading

dekkagaijin Nov 30, 2021 •

edited

Loading

dekkagaijin Nov 30, 2021 •

edited

Loading