-
Notifications
You must be signed in to change notification settings - Fork 785
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
proxy: Verify *either* toplevel or target #2400
base: main
Are you sure you want to change the base?
Conversation
I am not aware of a reason not to just cache this for the life of the proxy, like we do other global state. Prep for further changes. Signed-off-by: Colin Walters <[email protected]>
dd4b1c9
to
8de83ca
Compare
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.
Thanks!
Implementation LGTM, just some naming nits.
8de83ca
to
732ddcc
Compare
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.
LGTM. Please mark “ready for review” when appropriate.
732ddcc
to
30a0e99
Compare
OK I went to add tests and got burned by the fact that actually But, using my own test image, I did at least verify that
(i.e. my original bug) will correctly cause the tests to fail. It should be trivial to get someone with creds to upload a non-manifest-list fixture to quay.io/libpod - do you know who to ping? Chris? |
(Only compile tested locally) An issue was raised in that a current Red Hat internal build system was performing a signature just on the per-architecture manifest, but the current proxy code is expecting a signature on the manifest list. To quote Miloslav from that bug: > Podman signs both the per-platform items and the top level, > and enforces signatures only on per-platform items. cosign, > by default, signs only the top level (and has an option to > sign everything), I’m not sure which one it enforces. > I don’t immediately recall other platforms. We believe the current proxy code is secure since we always require signatures (if configured) on the manifest list, and the manifest list covers the individual manifests via sha256 digest. However, we want to support signatures only being present on the per-arch manifest too in order to be flexible. Yet, we can't hard switch to requiring signatures on the per-arch manifests as that would immediately break anyone relying on the current behavior of validating the toplevel. Change the logic here to: - Verify signature on the manifest list, and cache the error (if any) - Fetch the manifest - Verify signature on the manifest - Allow if *either* signature was accepted; conversely, only error if signature validation failed on *both* the manifest list and manifest This also switches things to cache the manifest upfront instead of doing it lazily on `GetManifest/GetConfig`; in practice callers were always immediately requesting those anyways. The use case of just fetching blobs exists (e.g. to resume an interrupted fetch), but is relatively obscure and in general I think it's good to re-verify signatures on each operation. Signed-off-by: Colin Walters <[email protected]>
30a0e99
to
21bf7d8
Compare
It'd also be useful to have sigstore-signed fixtures there of course. |
@edsantiago probably has a good idea of what images exist / are appropriate to use from If we have no requirements for how the subject images {are, are not} signed: in the worst case, given a multi-platform image, we can read the index, choose an instance, and use a digest reference as a single-platform image. |
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.
(These are all non-blocking aesthetics.)
config := proxyConfig{ | ||
policyFilePath: tempd + "/policy.json", | ||
} | ||
err := os.WriteFile(config.policyFilePath, []byte("{ \"default\": [ { \"type\":\"reject\"} ] }"), 0o644) |
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.
(This might be a tiny bit easier to read with raw string literals per https://go.dev/ref/spec#String_literals.)
if err != nil { | ||
err = fmt.Errorf("Testing optional image %s: %v", knownNotExtantImage, err) | ||
} | ||
assert.NoError(t, err) |
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.
assert.NoError(t, err, "testing image %s", knownNotExtantImage)
@@ -701,9 +682,13 @@ func (h *proxyHandler) close() { | |||
err := image.src.Close() | |||
if err != nil { | |||
// This shouldn't be fatal | |||
logrus.Warnf("Failed to close image %s: %v", transports.ImageName(image.cachedimg.Reference()), err) | |||
logrus.Warnf("Failed to close image %s: %v", transports.ImageName(image.img.Reference()), err) |
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.
(Pre-existing, non-blocking: using image.src.Reference()
here would more obviously correct — but it ends up being the same value in the end.)
Short answer: anything you want, it's under our control with multiple people having access. Longer answer: CI VM images now include a local cache registry running on port 60333, with copies of all images required by buildah and podman. I'd like to extend that to skopeo too. Purpose is to reduce our exposure to quay flakes. Can signed images be served by a simple local registry process? If not, is there a way to make it possible? |
sigstore signatures are ordinary objects on a registry, and can be served that way. simple signing signatures are served using plain HTTP. It should not be very hard (set up a directory as a staging destination in |
… alternatively, we could also design a way to store simple-signing signatures on the registry (perhaps not using exactly the sigstore tag convention, but using OCI referrers as intended). We’ve been converging on sigstore as the future, so it’s unclear that it is necessary, and in some sense, having this convenient feature sigstore-only might motivate users to migrate, so maybe we should not be adding that. |
Tangential to this PR landing but...per your comment on the bug:
The next problem is, if we start trying to move bootc itself over to more like what podman/skopeo does (which includes hopefully unified storage) it will also necessarily imply that we'd need to ensure we replicate this "accept top-level-only" on at least the pull code path we use for bootc. I don't know what that would look like exactly...maybe some sort of new (When we do that type of stuff though we'll definitely want a basic progress bar... #658 is related in that I think a lot of these "script skopeo on the CLI" cases would really more nicely generalize to "language-independent c/image RPC API") |
Ugh, I’d prefer not to have that option, recognizing that it really might be unavoidable. That makes me more motivated to argue that this PR should just do what c/image does The implementation would probably be a new field in |
I looked at this, but it's a bit logistically annoying to do in the tests at the moment.
Ideally there'd be a git repository defining the images in quay.io/libpod - in this case a periodic sync like e.g. |
Hmm. Maybe what would be helpful here is for the proxy to expose back to the client the signature state - especially the case when we detect only the manifest list was signed. I could then add something to bootc to loudly warn if this is the case...the problem is that there are going to be users that aren't paying attention to the journal/process-stdout for the often unattended systems that are a primary target for bootc. But it may help. |
Or have the proxy warn directly, and direct proxy’s stderr to a log? |
@edsantiago ok but can you link me to the procedure or code or process for adding images there? |
I've lost the thread here, so this is the question I am answering:
Answer: someone with credentials (me, @baude, not sure who else) pushes it. If it is me, I'm neurotic about generating reproducible, documented, clearly tagged multiarch images. I will be glad to help craft something then push it. Please let me know how/where to begin with the image building process. |
This sounds good to me but to achieve reproducible documented process I'd expect a a git repository to contain that, that's what I was looking for. At this point I'm basically just repeating #2400 (comment) |
IIUC you are referencing this part of the comment:
That's not something I can ever imagine myself doing. I can envision myself doing a one-time You mentioned |
Sure, that's fine. There's nothing special about the image, it could actually literally be any image; the goal is a fixture that is explicitly not a manifest list.
For the general reason that often creating something is only 15% of the work, the remaining 85% is maintaining it over time. Having images be pushed from a git repository with clear ownership and audibility is just better. But, if it's not true for the other images there today I wouldn't say we need to start doing it now just for this one. That said, to combine these two things, I think it would actually be useful to have test fixtures set up with cosign; the current skopeo test suite does signing locally, which is fine, but it's handy to just have pre-made images with both good and bad signatures, etc. That'd be handy right now, but I can get by with just the not-manifest-listed fixture to start. |
FWIW compare existing https://github.com/containers/skopeo/blob/main/systemtest/make-noarch-manifest . It’s not automated anywhere, but it is recorded in the same directory as the test that uses it. |
A friendly reminder that this PR had no activity for 30 days. |
proxy: Move policycontext into global state
I am not aware of a reason not to just cache this for the life
of the proxy, like we do other global state.
Prep for further changes.
Signed-off-by: Colin Walters [email protected]
proxy: Verify either toplevel or target
An issue was raised in that a current Red Hat internal build system
was performing a signature just on the per-architecture manifest,
but the current proxy code is expecting a signature on the manifest
list.
To quote Miloslav from that bug:
We believe the current proxy code is secure since
we always require signatures (if configured) on the manifest
list, and the manifest list covers the individual manifests
via sha256 digest.
However, we want to support signatures only being present
on the per-arch manifest too in order to be flexible.
Yet, we can't hard switch to requiring signatures on the
per-arch manifests as that would immediately break anyone
relying on the current behavior of validating the toplevel.
Change the logic here to:
if signature validation failed on both the manifest list and
manifest
This also switches things to cache the manifest upfront instead
of doing it lazily on
GetManifest/GetConfig
; in practicecallers were always immediately requesting those anyways.
The use case of just fetching blobs exists (e.g. to resume
an interrupted fetch), but is relatively obscure and
in general I think it's good to re-verify signatures on
each operation.
Signed-off-by: Colin Walters [email protected]