Add image destination transport support for sigstore's cosign image signing #1364

font · 2021-08-27T23:52:40Z

This is a very rough WIP PR to add image destination transport support for signing container images using the cosign signature spec. There are some TODOs throughout and summarized here:

TODO:

mtrmac

Just a fairly surface-level skim for now.

I’m strongly focusing on the public API because Go module semantics requires us to not break it, so it’s fairly important to get it right the first time.

As far as the internal impact on c/image’s copy.Image/PutSignature/policy.json and so on, there isn’t much here yet, it seems.

mtrmac · 2021-08-28T16:12:38Z

copy/copy.go

@@ -123,6 +123,7 @@ type ImageListSelection int
 // Options allows supplying non-default configuration modifying the behavior of CopyImage.
 type Options struct {
 	RemoveSignatures bool   // Remove any pre-existing signatures. SignBy will still add a new signature.
+	Sign             bool   // Adds cosign signature using ephemeral keys.


Something vaguely like CosignSign? We’ll need to start being explicit about which one the caller wants. Anyway, there are quite a few other parameters to (optionally?) provide — so maybe a pointer to a struct that contains all of them.

Yes, I think we need to discuss what are all possible Cosign signing options we can foresee adding in the future. As a result, I think it makes sense to expand this to a struct.

mtrmac · 2021-08-28T16:13:34Z

copy/sign.go

 	"github.com/containers/image/v5/signature"
 	"github.com/containers/image/v5/transports"
 	"github.com/pkg/errors"
 )

 // createSignature creates a new signature of manifest using keyIdentity.
 func (c *copier) createSignature(manifest []byte, keyIdentity string) ([]byte, error) {
+	if keyIdentity == "" {


AFAICS the only caller knows which one of these is the case, so it can just call one of the two directly.

I'd echo that. Would make callers easier to read (like prose).

mtrmac · 2021-08-28T16:14:19Z

copy/sign.go

+		return nil, errors.Wrap(err, "getting key from Fulcio")
+	}
+
+	fmt.Println("Marshalling payload into JSON")


These should go to the copy reportWriter

I think some/most of them should be logrus.Debug. I wouldn't understand what "Marshalling payload into JSON" would mean when seeing that during podman pull ... so many payloads and too much JSON everywhere :^)

Ack, mostly just debug stuff for now.

mtrmac · 2021-08-28T16:15:15Z

copy/sign.go

+	}
+
+	fmt.Println("Generating certificate from Fulcio")
+	if err := mech.GenerateCertificate(); err != nil {


Having GenerateCertificate down here is probably not quite the right place.

It’s not trivially clear to me that having an implied state in mech is preferable; it well might be the best design — just something to think about more. Maybe the certificate should be an explicitly caller-managed object? Or we should have separate verification/signing mechanisms, with signing mechanisms implicitly creating a private key on construction?

E.g.

Would this ever support non-ephemeral keys that the caller just points to, comparable to GPG keys?

Alternatively, do we want to generate an exactly single-use key, so that GenerateCertificate is implied by Sign?

I guess ^^^ not — consider signing a multi-arch image = sign all the per-arch images, and maybe the top-level index as well: in that case we almost certainly want the user to only have one OIDC authentication, and I guess, to only use one private key for all of of the component images. So, the copy structure must be significantly different from this, we can’t do an OIDC request down here in per-component-image signature creation.

mtrmac · 2021-08-28T16:20:08Z

docker/lookaside.go

+// the usage of the BaseURL is defined under docker/distribution registries—separate storage of docs/signature-protocols.md
+// Warning: This function only exposes configuration in registries.d;
+// just because this function returns an URL does not mean that the URL will be used by c/image/docker (e.g. if the registry natively supports X-R-S-S).
+func CosignStorageBaseURL(sys *types.SystemContext, ref types.ImageReference, write bool) (*url.URL, error) {


Is all of this local storage intended to exist in the final version? From a very quick glance it seems basically copy&pasted.

I was assuming, without it ever being discussed, that the signatures would be stored only in the registry using the tag convention, and there wouldn’t be any lookaside mechanism like this. If you do plan to have a lookaside mechanism in the final version (and I assume Cosign doesn’t already define one that copy&pastes the simple signing one), I’d much prefer to think about how to have one mechanism that can store both kinds of signatures; maybe a new naming convention, or perhaps even a new format, that includes a signature type indication along with the signature.

[It is also expected to ~work for the existing lookaside mechanism to store multiple kinds of signatures, because existence of invalid signatures is not a reason to reject an image; so storing Cosign signatures and simple signing signatures in a single look aside should just work, with each policy requirement picking its own kinds of signatures. So, strictly speaking, we might not need any changes to the look aside at all.]

Yes, it's mostly duplicated, with the underlying paths differing between cosign and the original simple signing look aside location. I was trying an expedient way to differentiate between the two formats in the look aside, and thought having a separate path may be the easiest for now since there's no policy configuration for cosign yet. I'll need to think through and discuss this with you further.

mtrmac · 2021-08-28T17:19:11Z

signature/mechanism_sigstore.go

+
+	// TODO: use debug log
+	fmt.Println("Generating ephemeral keys")
+	priv, err := generatePrivateKey()


Besides being a good practice, AFAIK FIPS requires explicit zeroization of all secret material after it is no longer used. How is that going to work?

crypto/ecdsa doesn’t seem to attempt to do this at all.

(Yeah, it’s hard. That’s why the previous implementation only allowed signing via a separate third-party non-Go process.)

mtrmac · 2021-08-28T17:20:22Z

signature/signature.go

+
+// newCosignSignature returns a cosignSignature object with
+// the specified primary contents and appropriate metadata.
+func NewCosignSignature(dockerManifestDigest digest.Digest, dockerReference string) cosignSignature {


I’d prefer the public API to be higher-level, see elsewhere.

mtrmac · 2021-08-28T17:21:46Z

signature/mechanism_sigstore.go

+	return s.SignMessage(bytes.NewReader(payload), options.WithContext(s.ctx))
+}
+
+func (s *sigstoreSigningMechanism) Upload(signature, payload []byte) (*models.LogEntryAnon, error) {


This doesn’t use a context.Context for timeouts/cancellation?

mtrmac · 2021-08-28T17:22:21Z

signature/mechanism_sigstore.go

+		return nil, nil, err
+	}
+
+	tok, err := oauthflow.OIDConnect(oidcIssuer, oidcClientID, "", oauthflow.DefaultIDTokenGetter)


This doesn’t use a context.Context for timeouts/cancellation?

Not currently.

mtrmac · 2021-08-28T17:22:29Z

signature/mechanism_sigstore.go

+		},
+	)
+
+	resp, err := ops.SigningCert(params, bearerAuth)


This doesn’t use a context.Context for timeouts/cancellation?

It does not today.

vrothberg

Thanks!

By no means a full review (I ran out of time for today).

vrothberg · 2021-08-30T12:35:24Z

copy/copy.go

@@ -751,6 +752,14 @@ func (c *copier) copyOneImage(ctx context.Context, policyContext *signature.Poli
 		sigs = append(sigs, newSig)
 	}

+	if options.Sign {


Should Sign and SignBy conflict?

I think it makes enough sense to publish an image and sign it using both systems, to allow both kinds of consumers to consume it. Eventually we’ll probably recommend one of them, but that’s not a reason to prohibit signing using both.

vrothberg · 2021-08-30T12:37:09Z

signature/mechanism.go

+	GenerateCertificate() error
+	// Sign creates a (non-detached) signature of input using ephemeral keys.
+	// Fails with a SigningNotSupportedError if the mechanism does not support signing.
+	Sign(payload []byte) (signature []byte, err error)
+	Upload(signature, payload []byte) (*models.LogEntryAnon, error)


I had the same question. I think we could move it to ./internal and avoid exposing it on the API.

vrothberg · 2021-08-30T12:43:49Z

copy/sign.go

 	"github.com/containers/image/v5/signature"
 	"github.com/containers/image/v5/transports"
 	"github.com/pkg/errors"
 )

 // createSignature creates a new signature of manifest using keyIdentity.
 func (c *copier) createSignature(manifest []byte, keyIdentity string) ([]byte, error) {
+	if keyIdentity == "" {


I'd echo that. Would make callers easier to read (like prose).

vrothberg · 2021-08-30T12:48:23Z

copy/sign.go

+	}
+	defer mech.Close()
+	if err := mech.SupportsSigning(); err != nil {
+		return nil, errors.Wrap(err, "signing is not supported")


Maybe add "cosign" to the error message to leave some explicit breadcrumb (not sure whether that's set in the error).

As @mtrmac mentioned in another comment: let's use fmt instead of the errors package to slowly migrate over to the standard lib.

vrothberg · 2021-08-30T12:53:45Z

copy/sign.go

+		return nil, errors.Wrap(err, "getting key from Fulcio")
+	}
+
+	fmt.Println("Marshalling payload into JSON")


I think some/most of them should be logrus.Debug. I wouldn't understand what "Marshalling payload into JSON" would mean when seeing that during podman pull ... so many payloads and too much JSON everywhere :^)

vrothberg · 2021-08-30T12:54:39Z

copy/sign.go

+		return nil, err
+	}
+
+	fmt.Println("Signing payload")


This would be helpful on report writer but maybe "Signing image via cosign"?

Ack. Mostly just debug stuff for now. I'll need to make a pass at some point when the code is in a more stable state to make sure all user visible messages are coherent and clear.

vrothberg · 2021-08-30T12:56:03Z

copy/sign.go

+
+	fmt.Println("Sending entry to transparency log")
+	tlogEntry, err := mech.Upload(signature, sigPayload)
+


Lint nit: blank line

vrothberg · 2021-08-30T13:09:02Z

signature/mechanism_sigstore.go

+	if err != nil {
+		return errors.Wrap(err, "marshaling")
+	}
+	canonicalized, err := jsoncanonicalizer.Transform(contents)


I'd love avoid new dependencies if possible. This one seems to be maintained by a single person which is something we should consider. In case we need to canonicalize, maybe we can use another library; ideally one we already use.

vrothberg · 2021-08-30T13:22:28Z

signature/mechanism_sigstore.go

+	}
+	re := rekorEntry(payload, signature, s.cert)
+	returnVal := models.Rekord{
+		APIVersion: swag.String(re.APIVersion()),


swag.String seems to only return a pointer to the input string. Could we do this manually and get rid of adding swag as a new dependency.

Note that I am trying to avoid new dependencies unless urgently necessary to keep our footprint small; it'll add some extra work downstream packaging as well (e.g., Debian has to devendor all dependencies and put them into extra source packages).

Yes, good catch.

Well, https://github.com/sigstore/rekor/blob/main/pkg/generated/models/rekord.go depends on swag anyway, so I don’t think this would buy us anything.

font

@mtrmac @vrothberg Thank you for your early review! There is still lots of work to do and there are many areas that need to be scrubbed. But I think we need to discuss a few things before I can continue moving forward:

The naming convention we want to use i.e. sigstore vs cosign, to avoid confusion and conflicts with existing naming scheme. This will likely impact user visible help strings and manpages for correct usage.
UX and semantics for --sign vs --sign-by. Originally I thought maybe --sign-by being nil could indicate ephemeral key (aka "keyless') signing instead of having to introduce a new flag. But that could be confusing. In any case, we should settle on the right flag and options to consider for future-proofing this in case we want to add bring-your-own-keys, KMS, etc. type of workflows in the future.
The right level of abstraction for introducing this feature's APIs and how to separate it with existing simple signing approach.

font · 2021-08-30T18:55:53Z

copy/sign.go

+		return nil, errors.Wrap(err, "getting key from Fulcio")
+	}
+
+	fmt.Println("Marshalling payload into JSON")


Ack, mostly just debug stuff for now.

font · 2021-08-30T18:56:43Z

copy/copy.go

@@ -123,6 +123,7 @@ type ImageListSelection int
 // Options allows supplying non-default configuration modifying the behavior of CopyImage.
 type Options struct {
 	RemoveSignatures bool   // Remove any pre-existing signatures. SignBy will still add a new signature.
+	Sign             bool   // Adds cosign signature using ephemeral keys.


Yes, I think we need to discuss what are all possible Cosign signing options we can foresee adding in the future. As a result, I think it makes sense to expand this to a struct.

font · 2021-08-30T19:01:25Z

copy/sign.go

+		return nil, err
+	}
+
+	fmt.Println("Signing payload")


Ack. Mostly just debug stuff for now. I'll need to make a pass at some point when the code is in a more stable state to make sure all user visible messages are coherent and clear.

font · 2021-08-30T19:13:53Z

docker/lookaside.go

+// the usage of the BaseURL is defined under docker/distribution registries—separate storage of docs/signature-protocols.md
+// Warning: This function only exposes configuration in registries.d;
+// just because this function returns an URL does not mean that the URL will be used by c/image/docker (e.g. if the registry natively supports X-R-S-S).
+func CosignStorageBaseURL(sys *types.SystemContext, ref types.ImageReference, write bool) (*url.URL, error) {


Yes, it's mostly duplicated, with the underlying paths differing between cosign and the original simple signing look aside location. I was trying an expedient way to differentiate between the two formats in the look aside, and thought having a separate path may be the easiest for now since there's no policy configuration for cosign yet. I'll need to think through and discuss this with you further.

font · 2021-08-30T19:17:25Z

signature/mechanism.go

+	GenerateCertificate() error
+	// Sign creates a (non-detached) signature of input using ephemeral keys.
+	// Fails with a SigningNotSupportedError if the mechanism does not support signing.
+	Sign(payload []byte) (signature []byte, err error)
+	Upload(signature, payload []byte) (*models.LogEntryAnon, error)


Ack, I was trying to leverage the existing APIs that existed for SigningMechanism until I had a better understanding of what all would be needed for this new signing method. I expect this to be further refined, and you say, likely not even made public depending on the requirements.

font · 2021-08-30T19:33:19Z

signature/mechanism_sigstore.go

+		return nil, nil, err
+	}
+
+	tok, err := oauthflow.OIDConnect(oidcIssuer, oidcClientID, "", oauthflow.DefaultIDTokenGetter)


Not currently.

font · 2021-08-30T19:45:55Z

signature/mechanism_sigstore.go

+		},
+	)
+
+	resp, err := ops.SigningCert(params, bearerAuth)


It does not today.

font · 2021-08-30T19:47:54Z

signature/mechanism_sigstore.go

+	}
+	re := rekorEntry(payload, signature, s.cert)
+	returnVal := models.Rekord{
+		APIVersion: swag.String(re.APIVersion()),


Yes, good catch.

font · 2021-08-30T19:50:16Z

signature/mechanism_sigstore.go

+		// Here, we display the proof and succeed.
+		if existsErr, ok := err.(*entries.CreateLogEntryConflict); ok {
+
+			fmt.Println("Signature already exists. Displaying proof")


Sure, we can just succeed silently, or display that the entry already exists. That will simplify things a bit.

font · 2021-08-30T19:55:31Z

signature/signature.go

+	// Use intermediate variables for these values so that we can take their addresses.
+	// Golang guarantees that they will have a new address on every execution.


Apologies, this code is very much WIP and still needs to be scrubbed of things like this.

Signed-off-by: Ivan Font <ifont@redhat.com>

This adds a new ImageDestinationSigstore private interface with a SupportsSigstoreSignatures method for detecting whether the destination transport i.e. docker only, supports Sigstore signatures. Signed-off-by: Ivan Font <ifont@redhat.com>

Make functions and methods that are not invoked outside package into private unexported functions and methods. Signed-off-by: Ivan Font <ifont@redhat.com>

Signed-off-by: Ivan Font <ifont@redhat.com>

This adds new Sigstore APIs for signing docker manifests using the cosign signing spec and uploading the signature and payload of the result to the Sigstore transparency log. Signed-off-by: Ivan Font <ifont@redhat.com>

font · 2021-10-02T01:03:07Z

Still isn't ready for review necessarily, but I've pushed another update to start addressing some of the feedback.

mtrmac · 2021-10-04T20:13:26Z

@font I realize you are pressed for time; the part that would help us most most of all to see how c/image needs to change would be to see a verification implementation (what are the from-registry inputs, what are the local-client trust configurations and roots, and how do they combine into a yes/no decision).

mtrmac · 2023-01-12T00:16:35Z

#1785 , and pre-requisite PRs, is now a doing everything from this PR that we want to do (an example exception: don’t care to cryptographically validate the Rekor-returned data against that Rekor’s public key).

Thanks for helping with this!

This was referenced Aug 28, 2021

WIP: Add sigstore sign option containers/common#748

Closed

WIP: Add sigstore sign option to push command containers/podman#11348

Closed

mtrmac reviewed Aug 28, 2021

View reviewed changes

vrothberg reviewed Aug 30, 2021

View reviewed changes

font commented Aug 30, 2021

View reviewed changes

font added 13 commits September 22, 2021 12:18

Add support for cosign signature base lookaside

76ddf1f

Signed-off-by: Ivan Font <ifont@redhat.com>

Add Sigstore signing mechanism

eec0c17

Signed-off-by: Ivan Font <ifont@redhat.com>

Add new cosignSignature format

e65be5d

Signed-off-by: Ivan Font <ifont@redhat.com>

Add copy sign functionality for cosign signatures

d010896

Signed-off-by: Ivan Font <ifont@redhat.com>

Add Sign option for cosign signatures

c352c86

Signed-off-by: Ivan Font <ifont@redhat.com>

Add dependencies for sigstore's image signing

f2a1072

Signed-off-by: Ivan Font <ifont@redhat.com>

Temporarily use device float token getter

082ec1c

Signed-off-by: Ivan Font <ifont@redhat.com>

Add new sigstore option struct

ef14907

Signed-off-by: Ivan Font <ifont@redhat.com>

Add Sigstore GetSignatures support for caching

cbc27ee

Signed-off-by: Ivan Font <ifont@redhat.com>

Refactor SigstoreSigning interface methods

a4e8502

Make functions and methods that are not invoked outside package into private unexported functions and methods. Signed-off-by: Ivan Font <ifont@redhat.com>

Create new file for unstrusted cosign signature

071066e

Signed-off-by: Ivan Font <ifont@redhat.com>

Add new Sigstore APIs for signing and uploading

f5374b4

This adds new Sigstore APIs for signing docker manifests using the cosign signing spec and uploading the signature and payload of the result to the Sigstore transparency log. Signed-off-by: Ivan Font <ifont@redhat.com>

font force-pushed the sigstore-sign branch from a020cf8 to f5374b4 Compare October 2, 2021 00:39

mtrmac mentioned this pull request Jan 3, 2022

copy: consider support cosign: verify image while pulling from source & sign image while copying to destination containers/skopeo#1533

Closed

sallyom mentioned this pull request Apr 28, 2022

sigstore test env containers/skopeo#1630

Closed

praveenkumar mentioned this pull request Jun 9, 2022

[Epic] Decouple bundles from released crc binary crc-org/crc#3206

Open

1 task

mtrmac closed this Jan 12, 2023


		fmt.Println("Sending entry to transparency log")
		tlogEntry, err := mech.Upload(signature, sigPayload)

		// Use intermediate variables for these values so that we can take their addresses.
		// Golang guarantees that they will have a new address on every execution.

Add image destination transport support for sigstore's cosign image signing #1364

Add image destination transport support for sigstore's cosign image signing #1364

Conversation

font commented Aug 27, 2021

TODO:

mtrmac left a 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

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

vrothberg left a 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

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

font left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

font commented Oct 2, 2021

mtrmac commented Oct 4, 2021

mtrmac commented Jan 12, 2023