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.

Sign up for GitHub

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

continued sign refacc #1098

Merged
merged 10 commits into from
Nov 30, 2021
Merged

continued sign refacc #1098

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
865342a
I eata da spaghetti
Nov 24, 2021
764ef20
decompose `StaticSigner` into individual signing stages
Nov 25, 2021
ad6188e
move rekor upload logic to `RekorSignerWrapper`
Nov 25, 2021
5a1540a
select signing flows based on input variables
Nov 25, 2021
3f5bf27
add comments to the `Signer` implementations
Nov 25, 2021
a99d528
only perform signing actions in `cosign.Signer`
Nov 29, 2021
29b2e61
flip comments on Signer implementations
Nov 30, 2021
04cc55f
staticly assert that impls are impls
Nov 30, 2021
66c3a2c
split Rekor and Fulcio signers into their own packages
Nov 30, 2021
5116c1e
code review feedback
Nov 30, 2021
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
13 changes: 4 additions & 9 deletions cmd/cosign/cli/sign/sign.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ import (
"github.com/sigstore/cosign/cmd/cosign/cli/fulcio/fulcioverifier"
"github.com/sigstore/cosign/cmd/cosign/cli/options"
icos "github.com/sigstore/cosign/internal/pkg/cosign"
ifulcio "github.com/sigstore/cosign/internal/pkg/cosign/fulcio"
irekor "github.com/sigstore/cosign/internal/pkg/cosign/rekor"
"github.com/sigstore/cosign/pkg/cosign"
"github.com/sigstore/cosign/pkg/cosign/pivkey"
"github.com/sigstore/cosign/pkg/cosign/pkcs11key"
Expand Down Expand Up @@ -212,16 +214,9 @@ func signDigest(ctx context.Context, digest name.Digest, payload []byte, ko KeyO
s = &icos.PayloadSigner{
Copy link
Member

Choose a reason for hiding this comment

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

I'd move the payload signer as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

roger

PayloadSigner: sv,
}
s = &icos.FulcioSignerWrapper{
Cert: sv.Cert,
Chain: sv.Chain,
Inner: s,
}
s = ifulcio.NewSigner(s, sv.Cert, sv.Chain)
Copy link
Member

Choose a reason for hiding this comment

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

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

  1. Shouldn't it be predicated on some condition?
  2. 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes :p

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

if ShouldUploadToTlog(ctx, digest, force, ko.RekorURL) {
s = &icos.RekorSignerWrapper{
Inner: s,
RekorURL: ko.RekorURL,
}
s = irekor.NewSigner(s, ko.RekorURL)
}

ociSig, _, err := s.Sign(ctx, bytes.NewReader(payload))
Expand Down
30 changes: 20 additions & 10 deletions internal/pkg/cosign/fulcio.go → internal/pkg/cosign/fulcio/fulcio.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,30 +12,31 @@
// See the License for the specific language governing permissions and
// limitations under the License.

package cosign
package fulcio

import (
"context"
"crypto"
"io"

"github.com/sigstore/cosign/internal/pkg/cosign"
"github.com/sigstore/cosign/pkg/oci"
"github.com/sigstore/cosign/pkg/oci/static"
)

// FulcioSignerWrapper still needs to actually upload keys to Fulcio and receive
// SignerWrapper still needs to actually upload keys to Fulcio and receive
// the resulting `Cert` and `Chain`, which are added to the returned `oci.Signature`
type FulcioSignerWrapper struct {
Inner Signer
type SignerWrapper struct {
Copy link
Member

Choose a reason for hiding this comment

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

This should be private now.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

inner cosign.Signer

Cert, Chain []byte
cert, chain []byte
}

var _ Signer = (*FulcioSignerWrapper)(nil)
var _ cosign.Signer = (*SignerWrapper)(nil)

// Sign implements `Signer`
func (fs *FulcioSignerWrapper) Sign(ctx context.Context, payload io.Reader) (oci.Signature, crypto.PublicKey, error) {
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)
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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)

if err != nil {
return nil, nil, err
}
Expand All @@ -51,7 +52,7 @@ func (fs *FulcioSignerWrapper) Sign(ctx context.Context, payload io.Reader) (oci

// TODO(dekkagaijin): move the fulcio SignerVerififer logic here

opts := []static.Option{static.WithCertChain(fs.Cert, fs.Chain)}
opts := []static.Option{static.WithCertChain(fs.cert, fs.chain)}

// Copy over the other attributes:
if annotations, err := sig.Annotations(); err != nil {
Expand All @@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

This comment could use work :)

Copy link
Member Author

@dekkagaijin dekkagaijin Nov 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

your FACE could use work

func NewSigner(inner cosign.Signer, cert, chain []byte) *SignerWrapper {
Copy link
Member

Choose a reason for hiding this comment

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

return the interface

Copy link
Member Author

Choose a reason for hiding this comment

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

done

return &SignerWrapper{
inner: inner,
cert: cert,
chain: chain,
}
}
29 changes: 19 additions & 10 deletions internal/pkg/cosign/rekor.go → internal/pkg/cosign/rekor/rekor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

package cosign
package rekor

import (
"context"
Expand All @@ -22,6 +22,7 @@ import (
"io"
"os"

"github.com/sigstore/cosign/internal/pkg/cosign"
cosignv1 "github.com/sigstore/cosign/pkg/cosign"
"github.com/sigstore/cosign/pkg/oci"
"github.com/sigstore/cosign/pkg/oci/static"
Expand Down Expand Up @@ -61,18 +62,18 @@ func uploadToTlog(rekorBytes []byte, rekorURL string, upload tlogUploadFn) (*oci
return bundle(entry), nil
}

// RekorSignerWrapper calls a wrapped, inner signer then uploads either the Cert or Pub(licKey) of the results to Rekor, then adds the resulting `Bundle`
type RekorSignerWrapper struct {
Inner Signer
// SignerWrapper calls a wrapped, inner signer then uploads either the Cert or Pub(licKey) of the results to Rekor, then adds the resulting `Bundle`
type SignerWrapper struct {
inner cosign.Signer

RekorURL string
rekorURL string
}

var _ Signer = (*RekorSignerWrapper)(nil)
var _ cosign.Signer = (*SignerWrapper)(nil)

// Sign implements `Signer`
func (rs *RekorSignerWrapper) Sign(ctx context.Context, payload io.Reader) (oci.Signature, crypto.PublicKey, error) {
sig, pub, err := rs.Inner.Sign(ctx, payload)
// Sign implements `cosign.Signer`
func (rs *SignerWrapper) Sign(ctx context.Context, payload io.Reader) (oci.Signature, crypto.PublicKey, error) {
sig, pub, err := rs.inner.Sign(ctx, payload)
if err != nil {
return nil, nil, err
}
Expand Down Expand Up @@ -106,7 +107,7 @@ func (rs *RekorSignerWrapper) Sign(ctx context.Context, payload io.Reader) (oci.
return nil, nil, err
}

bundle, err := uploadToTlog(rekorBytes, rs.RekorURL, func(r *rekGenClient.Rekor, b []byte) (*models.LogEntryAnon, error) {
bundle, err := uploadToTlog(rekorBytes, rs.rekorURL, func(r *rekGenClient.Rekor, b []byte) (*models.LogEntryAnon, error) {
return cosignv1.TLogUpload(ctx, r, sigBytes, payloadBytes, b)
})
if err != nil {
Expand Down Expand Up @@ -146,3 +147,11 @@ func (rs *RekorSignerWrapper) Sign(ctx context.Context, payload io.Reader) (oci.

return newSig, pub, nil
}

// NewSigner returns a `*SignerWrapper` which uploads the signature to Rekor
func NewSigner(inner cosign.Signer, rekorURL string) *SignerWrapper {
return &SignerWrapper{
inner: inner,
rekorURL: rekorURL,
}
}