-
Notifications
You must be signed in to change notification settings - Fork 381
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.
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
Add support for reading and writing Cosign attachments to c/image/docker #1595
Conversation
f5363f9
to
1e5089f
Compare
if err != nil { | ||
return nil | ||
} | ||
var ociConfig imgspecv1.Image // Most fields empty by default |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TO DO: Only fetch the manifest if we actually have a new signature to add (and if we continue to update the DiffID list).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this blocking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/Only fetch the manifest/Only fetch the config/; we do need to always fetch the manifest to detect whether we have a fresh attachment to upload (assuming there is at least one Cosign attachment provided to PutSignaturesWithFormat
).
logrus.Debugf("Adding new signature, digest %s", sigDesc.Digest.String()) | ||
} | ||
|
||
configBlob, err := json.Marshal(ociConfig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TO DO: Only upload an updated config and manifest if we added at least one new signature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this blocking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both are inefficiencies, and affect only users who opted in to use-cosign-attachments
, and actually have an attachment to copy.
So, IMHO, it can wait.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Recorded both in #1601 .
It's not actually dealing with the lookaside; just with the configuration. And we are going to introduce more configuration. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
…rageBaseURL This will allow us to only load the configuration once. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This will allow us to load the configuration and then ask for multiple items. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This is a bit more repetitive in most callers. The benefit is that we only read the files once per newImageSource, even if there are multiple mirrors. We will also read more items from the config. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... so that users can choose whether to do the extra manifest lookups, and record signatures. NOTE: This defaults to false. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
…t.fetchManifest Signed-off-by: Miloslav Trmač <mitr@redhat.com>
We need it for writing signatures. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
We are going to need a way to upload to a tag without affecting dockerImageDestination.manifestDigest. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
NOTE design decisions: - We can read Cosign data from lookaside - We ONLY write Cosign data to Cosign attachments, never to lookaside; because lookaside is set up by default, that would be too confusing. - We ONLY use Cosign attachments at all if the user opts in via registries.d. One concern is performance impact of the extra round-trip for large-scale operations like (skopeo sync). Short-term, a much more worrying is the risk that we probably have the "is this failure just a missing atachment manifest, or a real failure reading it?" heuristic wrong, so without an opt-in, _all_ image reads are going to fail. This might eventually go away after more testing. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
In manual testing, this is now good enough to copy an image between So, calling this ready for review. It could definitely do with more unit tests (at least of the config handling), and integration tests in the Skopeo repo. |
Note the most important risk mitigation: The code reading/writing Cosign attachments is opt-in, so it hopefully won’t break any other users. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One concern is performance impact of the extra round-trip
for large-scale operations like (skopeo sync).
Short-term, a much more worrying is the risk that we probably
have the "is this failure just a missing atachment manifest,
or a real failure reading it?" heuristic wrong, so without an
opt-in, all image reads are going to fail. This might eventually
go away after more testing.
Code and design LGTM, nice work!
Feel free to merge |
LGTM |
copy.Image can now copy non-image OCI artifacts. Added support for sigstore signatures: they (and related cosign attachments) can be copied along with images after opt-in in registries.d. Signatures can be created by copy.Image and enforced via policy.json (currently with public/private key pairs only). Now requires Go 1.17. GPGMe now must be new enough to be visible via pkg-config. github.com/pkg/errors is no longer used; that might affect caller-observable error types (in particular, errors.{As,Is} might need to be used instead of pkg/errors.Cause). Changes default paths on FreeBSD. - Remove unused Makefile variables - Config files should live in /usr/local on FreeBSD - docker: validate received parts - Use go env to fetch the go path - docker: add workaround for CloudFront - Improve errors messages when image missing from list - Stop calling gpgme-config - Fix codespell errors - Make sure github.com/opencontainers/runc >= 1.1.2 is used - Cirrus: use Ubuntu 22.04 LTS - Merge pull request containers#1576 from mtrmac/private-image - Merge pull request containers#1577 from mtrmac/mocks - Merge pull request containers#1571 from mtrmac/go1.17 - Merge pull request containers#1578 from mtrmac/sourced-image-struct - Fix error on parallel multiple image pullings with additionallayerstore - Merge pull request containers#1579 from mtrmac/copy-layers-refactor - Reject OCI artifacts in manifest.OCI1.ImageID - Reject OCI artifacts in manifest.OCI1.Inspect - Refuse to convert non-image OCI artifacts to Docker formats - Reject OCI artifacts in image.manifestOCI1.OCIConfig - Introduce SourcedImage.CanChangeLayerCompression, use it in copy.Image - Use an updated CI image - Use strings.ReplaceAll instead of strings.Replace(..., -1) - Move the main helper removal case to the main path on RemoveAllAuthentication - Merge pull request containers#1588 from mtrmac/pkg_errors - Merge pull request containers#1589 from mtrmac/private-dest-impls - Merge pull request containers#1590 from mtrmac/private-src-impls - Merge pull request containers#1592 from mtrmac/blobcache-wrap-private - Use "io.ReadAll" instead of "os.ReadAll" - Merge pull request containers#1596 from mtrmac/cosign-payload - Generalize copy.Image to be able to copy signatures with any format - Merge pull request containers#1593 from mtrmac/cosign-sigs - Introduce signature.Cosign as a format - Add use-cosign-attachments to registries.d/*.yaml - Add support for reading and writing Cosign attachments, incl. signatures - Merge pull request containers#1595 from mtrmac/cosign-docker - Add support for creating Cosign signatures - Fix a long-standing incorrect comment - Fix JSON syntax in the policy.json(5) man page - Correctly decode Cosign-generated payloads - Add Cosign verification support - s/sigstore/lookaside/g in comments and documentation - Refer to lookasideStorage instead of signatureStorage in code - Add lookaside and lookaside-staging, hide sigstore and sigstore-staging - Merge pull request containers#1605 from mtrmac/sigstore - Fix a typo in error messages - Remove a copy&pasted test entry - Add context to some test failures - Use more valid data in TestPRSignedByIsSignatureAuthorAccepted - Generalize keyPath/keyData exclusivity checks - Remove repetition in tests - Accept multiple keyrings in newEphemeralGPGSigningMechanism - Allow accepting multiple GPG keyrings via signedBy.keyPaths - Switch to golang native error wrapping - Point out use-sigstore-registries in sigstoreSigned documentation - Use .pub extension for public keys in sigstoreSigned examples - copy: print copy info once when writer==io.Discard - Silence a "potentially unused parameter" warning - Read signatures from UnparsedImage instead of ImageSource directly - Consolidate reading messages, and checking for support, into a helper - build(deps): bump github.com/containers/storage from 1.40.0 to 1.40.2 - build(deps): bump github.com/docker/docker - build(deps): bump github.com/klauspost/compress from 1.15.2 to 1.15.3 - build(deps): bump github.com/klauspost/compress from 1.15.3 to 1.15.4 - build(deps): bump github.com/docker/docker - build(deps): bump github.com/proglottis/gpgme from 0.1.1 to 0.1.2 - build(deps): bump github.com/vbauerster/mpb/v7 from 7.4.1 to 7.4.2 - build(deps): bump github.com/imdario/mergo from 0.3.12 to 0.3.13 - build(deps): bump github.com/klauspost/compress from 1.15.4 to 1.15.5 - build(deps): bump github.com/sylabs/sif/v2 from 2.7.0 to 2.7.1 - build(deps): bump github.com/klauspost/compress from 1.15.5 to 1.15.6 - build(deps): bump github.com/stretchr/testify from 1.7.1 to 1.7.2 - build(deps): bump github.com/docker/docker - build(deps): bump github.com/stretchr/testify from 1.7.2 to 1.7.4 - build(deps): bump github.com/stretchr/testify from 1.7.4 to 1.7.5 - build(deps): bump github.com/stretchr/testify from 1.7.5 to 1.8.0 - build(deps): bump github.com/klauspost/compress from 1.15.6 to 1.15.7 - build(deps): bump github.com/proglottis/gpgme from 0.1.2 to 0.1.3 - build(deps): bump github.com/klauspost/compress from 1.15.7 to 1.15.8 - build(deps): bump github.com/sirupsen/logrus from 1.8.1 to 1.9.0 - build(deps): bump github.com/theupdateframework/go-tuf - build(deps): bump github.com/BurntSushi/toml from 1.1.0 to 1.2.0 Signed-off-by: Miloslav Trmač <mitr@redhat.com>
copy.Image can now copy non-image OCI artifacts. Added support for sigstore signatures: they (and related cosign attachments) can be copied along with images after opt-in in registries.d. Signatures can be created by copy.Image and enforced via policy.json (currently with public/private key pairs only). Now requires Go 1.17. GPGMe now must be new enough to be visible via pkg-config. github.com/pkg/errors is no longer used; that might affect caller-observable error types (in particular, errors.{As,Is} might need to be used instead of pkg/errors.Cause). Changes default paths on FreeBSD. - Remove unused Makefile variables - Config files should live in /usr/local on FreeBSD - docker: validate received parts - Use go env to fetch the go path - docker: add workaround for CloudFront - Improve errors messages when image missing from list - Stop calling gpgme-config - Fix codespell errors - Make sure github.com/opencontainers/runc >= 1.1.2 is used - Cirrus: use Ubuntu 22.04 LTS - Merge pull request containers#1576 from mtrmac/private-image - Merge pull request containers#1577 from mtrmac/mocks - Merge pull request containers#1571 from mtrmac/go1.17 - Merge pull request containers#1578 from mtrmac/sourced-image-struct - Fix error on parallel multiple image pullings with additionallayerstore - Merge pull request containers#1579 from mtrmac/copy-layers-refactor - Reject OCI artifacts in manifest.OCI1.ImageID - Reject OCI artifacts in manifest.OCI1.Inspect - Refuse to convert non-image OCI artifacts to Docker formats - Reject OCI artifacts in image.manifestOCI1.OCIConfig - Introduce SourcedImage.CanChangeLayerCompression, use it in copy.Image - Use an updated CI image - Use strings.ReplaceAll instead of strings.Replace(..., -1) - Move the main helper removal case to the main path on RemoveAllAuthentication - Merge pull request containers#1588 from mtrmac/pkg_errors - Merge pull request containers#1589 from mtrmac/private-dest-impls - Merge pull request containers#1590 from mtrmac/private-src-impls - Merge pull request containers#1592 from mtrmac/blobcache-wrap-private - Use "io.ReadAll" instead of "os.ReadAll" - Merge pull request containers#1596 from mtrmac/cosign-payload - Generalize copy.Image to be able to copy signatures with any format - Merge pull request containers#1593 from mtrmac/cosign-sigs - Introduce signature.Cosign as a format - Add use-cosign-attachments to registries.d/*.yaml - Add support for reading and writing Cosign attachments, incl. signatures - Merge pull request containers#1595 from mtrmac/cosign-docker - Add support for creating Cosign signatures - Fix a long-standing incorrect comment - Fix JSON syntax in the policy.json(5) man page - Correctly decode Cosign-generated payloads - Add Cosign verification support - s/sigstore/lookaside/g in comments and documentation - Refer to lookasideStorage instead of signatureStorage in code - Add lookaside and lookaside-staging, hide sigstore and sigstore-staging - Merge pull request containers#1605 from mtrmac/sigstore - Fix a typo in error messages - Remove a copy&pasted test entry - Add context to some test failures - Use more valid data in TestPRSignedByIsSignatureAuthorAccepted - Generalize keyPath/keyData exclusivity checks - Remove repetition in tests - Accept multiple keyrings in newEphemeralGPGSigningMechanism - Allow accepting multiple GPG keyrings via signedBy.keyPaths - Switch to golang native error wrapping - Point out use-sigstore-registries in sigstoreSigned documentation - Use .pub extension for public keys in sigstoreSigned examples - copy: print copy info once when writer==io.Discard - Silence a "potentially unused parameter" warning - Read signatures from UnparsedImage instead of ImageSource directly - Consolidate reading messages, and checking for support, into a helper - build(deps): bump github.com/containers/storage from 1.40.0 to 1.40.2 - build(deps): bump github.com/docker/docker - build(deps): bump github.com/klauspost/compress from 1.15.2 to 1.15.3 - build(deps): bump github.com/klauspost/compress from 1.15.3 to 1.15.4 - build(deps): bump github.com/docker/docker - build(deps): bump github.com/proglottis/gpgme from 0.1.1 to 0.1.2 - build(deps): bump github.com/vbauerster/mpb/v7 from 7.4.1 to 7.4.2 - build(deps): bump github.com/imdario/mergo from 0.3.12 to 0.3.13 - build(deps): bump github.com/klauspost/compress from 1.15.4 to 1.15.5 - build(deps): bump github.com/sylabs/sif/v2 from 2.7.0 to 2.7.1 - build(deps): bump github.com/klauspost/compress from 1.15.5 to 1.15.6 - build(deps): bump github.com/stretchr/testify from 1.7.1 to 1.7.2 - build(deps): bump github.com/docker/docker - build(deps): bump github.com/stretchr/testify from 1.7.2 to 1.7.4 - build(deps): bump github.com/stretchr/testify from 1.7.4 to 1.7.5 - build(deps): bump github.com/stretchr/testify from 1.7.5 to 1.8.0 - build(deps): bump github.com/klauspost/compress from 1.15.6 to 1.15.7 - build(deps): bump github.com/proglottis/gpgme from 0.1.2 to 0.1.3 - build(deps): bump github.com/klauspost/compress from 1.15.7 to 1.15.8 - build(deps): bump github.com/sirupsen/logrus from 1.8.1 to 1.9.0 - build(deps): bump github.com/theupdateframework/go-tuf - build(deps): bump github.com/BurntSushi/toml from 1.1.0 to 1.2.0 Signed-off-by: Miloslav Trmač <mitr@redhat.com>
⚠️ Warning: This is write-only code, as in I haven’t read it after myself, and it has no tests yet. Might be completely broken.It really needs an integration test, and interoperability testing.Note: design decisions:
We can read Cosign data from lookaside
We only write Cosign data to Cosign attachments, never to lookaside; because lookaside is set up by default, that
would be too confusing.
We only use Cosign attachments at all if the user opts in via
registries.d
, optionuse-cosign-attachments
.One concern is performance impact of the extra round-trip for large-scale operations like (skopeo sync).
Short-term, a much more worrying is the risk that we probably have the "is this failure just a missing atachment manifest, or a real failure reading it?" heuristic wrong, so without an opt-in, all image reads are going to fail. This might eventually go away after more testing.
Depends on unmerged #1594.