Introduce opaque signer.Signer objects by mtrmac · Pull Request #1776 · containers/image

mtrmac · 2023-01-05T03:12:15Z

This adds a new, very opaque, signer.Signer type, allowing users to construct it for the existing simple signing and sigstore implementations, and to pass them to copy.Image in copy.Options.Signers.

The goal is to:

Front-load initialization of the signer
- To allow interactive initialization (passphrase prompts, web browser interactions) immediately, not waiting for layer copies
- To possibly initialize a signer once before a sequence of multiple copy.Image calls (like in skopeo sync)
- To report some failures (incorrect paths) immediately
Remove the dependency of copy.Image on individual signing implementations:
- Notably just including copy.Image won’t necessarily drag in all of the Rekor / Fulcio RPC infrastructure for specialized callers that don’t need it.
- We can add new options to constructors of the individual Signer implementations without having to mirror every single option in a new copy.Options field
Not commit us to any details of the signing API, at least for now.

This is missing tests, and hasn’t actually been used for Fulcio / Rekor yet (compare extremely premature https://github.com/mtrmac/image/tree/fulcio-signing ); filing it early to discuss the API and general approach.

@vrothberg PTAL

vrothberg

LGTM

vrothberg · 2023-01-05T12:01:32Z

copy/sign.go

+	}
+	defer signer.Close()
+
+	c.Printf("%s\n", internalSigner.ProgressMessage(signer))


Note to self: why does this work? Shouldn't it be signer.ProgressMessage()?

Ah, OK got it now ✔️

What would be a good place to document how this is intended to work?

I have no idea. I think it's OK as is and it was immediately obvious in the code. Just when reading the commits one-by-one I was surprised for a moment.

vrothberg · 2023-01-05T12:03:20Z

signature/simple/signer.go

+	"github.com/containers/image/v5/signature/signer"
+)
+
+// simpleSigner is a signer.Signer implementation for simple signing signatures.


Nit: Could we embed GPG in the name? It would be more expressive to me when reading.

(The user-visible documentation is on the new NewSigner function, or possibly for the package, not on package-private simpleSigner.)

The format is not just GPG, and I think it makes sense to be consistent about the name. Originally the format was called “atomic container signatures”. Now we tend to say “simple signing”. IMHO adding another name is not worth it, though I’m not feeling too strongly about it.

GPG is not really essential (from the very start, there’s been at least a documented but rejected "keyType": "X509Certificates"), though I agree that it’s very unlikely any non-GPG format will be added.

Right now NewSigner sort of points at GPG, but probably not quite sufficiently.

Sort of related, there is now a ton of subpackages named …/signature or …/sigstore or …/internal; each individual name seems reasonable, and the small packages seem to be useful to decrease the dependency surface, but it adds up to a fairly bad whole. I’m sure there are better ways to organize, any ideas would be great.

Should it even be signature/simple? signature/simplesigning would be consistent with internal/signature.SimpleSigning.

Moved to signature/simplesigning now, but that’s not intended to stop the search for another name.

simplesigning SGTM
It's somehow a known terminology in our universe.

vrothberg · 2023-01-05T12:07:10Z

copy/copy.go

+
+	// Signers to use to add signatures during the copy.
+	// Callers are still responsible for closing these Signer objects; they can be reused for multiple copy.Image operations in a row.
+	Signers []*signer.Signer


I don't have a strong opinion on that. One worry I have is boilerplate code for callers. skopeo, libimage and CRI-O would all need updates, wouldn't they?

Totally, this would need a common utility. (Not CRI-O, it doesn’t create signatures.)

Right now there is a WIP https://github.com/mtrmac/image/tree/fulcio-signing/pkg/cli/sigstore : just importing copy.Image adds dependencies enough to validate Fulcio + Rekor signatures, and to sign using private keys, but importing this cli/sigstore subpackage drags in the Fulcio + Rekor RPC clients.

I actually fairly strongly think that we need to go further than that, and to expose a config file for signing with Fulcio+Rekor (because there are like 6 individual options to set). And then NewSignerFromSigstoreSigningConfigFile would be a natural utility to share across various copy.Image callers to implement that config file.

If the comment is about the deprecation of existing SignBy… options, I now think that’s something that is eventually a good idea, but should not be done in this PR; it’s an internal cleanup for no end-user benefit and the end-user functionality is much more important short-term.

SGTM

I am mostly worried to end up like in a pre-libimage period where the tools have duplicate code that must be maintained separately.

A config file sounds good to me and makes perfect sense. It seems more user friendly (and code friendly) to have one central place to configure the setup rather than passing things from the CLI through the entire call stack.

mtrmac

Now updated:

With tests
The deprecation was moved to another branch, to be done (much?) later
Renaming signature/simple to signature/simplesigning
GPG tests now terminate gpg-agent processes so that we don’t leave several around.

Still a draft because final users don’t exist, but I think this PR is fairly close to finished.

mtrmac · 2023-01-06T05:25:55Z

signature/simplesigning/signer_test.go

+		// FIXME FIXME: gpgme_op_sign with a passphrase succeeds, but somehow confuses the GPGME internal state
+		// so that gpgme_op_verify below never completes (it polls on an already closed FD).
+		// That’s probably a GPG bug, and needs investigating and fixing, but it isn’t related to this “signer” implementation.
+		if len(c.opts) == 0 {


This is a bug that certainly needs tracking down: https://github.com/containers/image/issues/1777 .

“Luckily” it probably only affects long-term processes that sign (and verify?), i.e. not Podman and not CRI-O.

mtrmac · 2023-01-06T05:27:45Z

signature/simplesigning/signer_test.go

+const (
+	// testImageManifestDigest is the Docker manifest digest of "image.manifest.json"
+	testImageManifestDigest = digest.Digest("sha256:20bf21ed457b390829cdbeec8795a7bea1626991fda603e0d01b4e7f60427e55")
+	testGPGHomeDirectory    = "./testdata"


See the commit message about using separate GPG home directories.

There still is some flake that is fairly frequently triggered by two subpackages testing GPG concurrently, resulting in a signing operation failing with “end of file”. I haven’t tracked that one down. Short—term, I’d rather have a flake than an untested signing code, but that’s assuming the pain won’t be that big in CI.

vrothberg

LGTM, nice work!

rhatdan · 2023-01-06T19:37:08Z

@mtrmac is this still Draft?

mtrmac · 2023-01-06T20:42:12Z

Yes, I want to have a testable Skopeo binary with the anticipated features before committing to an API.

Notably the signature/sigstore.NewSignerWithPrivateKeyFileUnstable should not ship like that.

mtrmac · 2023-01-07T09:01:15Z

Updated now:

Split some of the conceptually test changes into GPG signing test improvements #1779
Replaced signature/sigstore.NewSignerWithPrivateKeyFileUnstable (and the current signature/sigstore.SignDockerManifestWithPrivateKeyFileUnstable) with signature/sigstore.NewSigner and the functional-options pattern, which allows adding Rekor and Fulcio as optional dependencies. See https://github.com/mtrmac/image/tree/fulcio-signing for intended use
The signing unit tests in copy now just test identity using a stub Signer, instead of actually running GPG. That way they are truly unit tests, and cheaper.

I think this is ready for final review, marked as draft due to the #1779 dependency.

vrothberg

LGTM

Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

…teKeyFileUnstable ... instead of specifically signature.Sigstore. Should not change behavior, the only caller doesn't care. We will introduce an interface where we should allow creating all kinds of signature formats. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

…Implementation Add a new user-visible type signature/signer.Signer; and internal internal/signer.Signer + internal/signer.SignerImplementation. The purpose is to create a completely opaque type that could be created by external callers, maintained for a time, passed to one or more copy.Image calls, and then destroyed, without exposing _any_ actual API to create signatures. We can, possibly, expose more later; for now, this opaque type will allow us to easily change the internal signing interface, while still allowing a fair bit of flexibility for external callers by giving them options when creating the Signer objects. Only adds new interfaces, does not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

... to contain the core, shared, signing code that can be augmented by optional Rekor and Fulcio dependencies. Only moves around the existing implementation, should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Then construct a single-purpose signer.Signer; we will expose it to users next. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

This replaces NewSignerWithPrivateKeyFileUnstable, and can be extended to add more options in the future without having for all of them to be in the same subpackage (and all dependencies to be included). The WithPrivateKeyFile option could possibly be in its own subpackage, but because c/image/copy depends on it, that would not be likely to remove many dependencies. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

... to match the object implemented in there. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

This is a signer.Signer wrapper around the existing simple signing API, for consistency. It's just a wrapper which does not substantially change the GPG implementation approach; doing that might add more benefits (e.g. an immediate failure on an invalid key ID). That could possibly be improved in the future, for now the goal is only to migrate copy.Image to using a singer.Signer. To run tests, we need to introduce a _new_ GPG home directory, because (go test) runs tests in subpackages in parallel, and the tests in signature and signature/simple can interfere: a shared gpg-agent will be unlocked by one test and expected to be locked by another, causing spurious failures. So, this adds signature/simplesigning/testdata, but all the GPG files are symlinks to the original location. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

…WithSigner They are no exactly identical (apart from one explicit .String() call). Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Consolidate the format specifics and options handling to a single place, and let the rest deal with an undifferentiated list of Signers. This might be user-visible: failures e.g. on invalid key file path are now exposed immediately, instead of only after copying all the large layers. This is a step towards allowing callers to provide their own Signer objects. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Add copy.Options.Signers, so that callers can construct their own Signer objects and c/image/copy doesn't grow dependencies on all possible signature formats (notably on the Fulcio / Rekor RPC clients), and so that interactive initialization is easier. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

In TestCreateSignatures, test just the identity and error handling here; full-scale testing that the images are signed and can be verified is better done in integration tests. Now that it is a bit easier, also test Options.Signers. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

mtrmac · 2023-01-09T14:14:17Z

Rebased (with no changes), ready for merging.

vrothberg

LGTM

mtrmac force-pushed the opaque-signer branch from 6ae0b01 to 5d8ea6e Compare January 5, 2023 03:44

vrothberg reviewed Jan 5, 2023

View reviewed changes

mtrmac force-pushed the opaque-signer branch 2 times, most recently from d0af47e to 2698265 Compare January 6, 2023 05:14

mtrmac mentioned this pull request Aug 27, 2025

Some sequence of multiple GPGME mechanisms / signing + verification operations causes hangs containers/container-libs#223

Open

mtrmac commented Jan 6, 2023

View reviewed changes

mtrmac force-pushed the opaque-signer branch from 2698265 to e5863ed Compare January 6, 2023 05:38

vrothberg approved these changes Jan 6, 2023

View reviewed changes

mtrmac added the kind/feature A request for, or a PR adding, new functionality label Jan 7, 2023

mtrmac force-pushed the opaque-signer branch from e5863ed to 2259f14 Compare January 7, 2023 04:50

mtrmac mentioned this pull request Jan 7, 2023

GPG signing test improvements #1779

Merged

mtrmac force-pushed the opaque-signer branch 10 times, most recently from 7bf6644 to 20a5503 Compare January 7, 2023 08:53

vrothberg reviewed Jan 9, 2023

View reviewed changes

mtrmac added 5 commits January 9, 2023 15:11

Fix a documentation typo

dddb6c8

Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Make SigstoreSigner implement signer.SignerImplementation

db63fe5

Then construct a single-purpose signer.Signer; we will expose it to users next. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

mtrmac added 7 commits January 9, 2023 15:12

Rename signature/sigstore/sign.go to signer.go

1a38a1b

... to match the object implemented in there. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Consolidate the two signing implementations to copier.createSignature…

3237466

…WithSigner They are no exactly identical (apart from one explicit .String() call). Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>

mtrmac force-pushed the opaque-signer branch from 20a5503 to 7150893 Compare January 9, 2023 14:14

mtrmac marked this pull request as ready for review January 9, 2023 14:14

vrothberg approved these changes Jan 9, 2023

View reviewed changes

vrothberg merged commit e12ff5f into containers:main Jan 9, 2023

mtrmac changed the title ~~RFC: Introduce opaque signer.Signer objects~~ Introduce opaque signer.Signer objects Jan 9, 2023

mtrmac deleted the opaque-signer branch January 9, 2023 15:26

Conversation

mtrmac commented Jan 5, 2023

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Jan 6, 2023

Uh oh!

mtrmac commented Jan 6, 2023

Uh oh!

mtrmac commented Jan 7, 2023

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

mtrmac commented Jan 9, 2023

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants