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.

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

[Vote] Should manifests with a refers field that points to something that doesn't exist in the repository be accepted by a registry? #340

Closed
michaelb990 opened this issue Aug 26, 2022 · 16 comments · Fixed by #341

Comments

@michaelb990
Copy link
Contributor

Follow up from #339.

A registry MAY reject a manifest of any type uploaded to the manifest endpoint if it references manifests or blobs that do not exist in the registry. A registry SHOULD accept a manifest with a refers field that references a manifest that does not exist. When a manifest is rejected for these reasons, it MUST result in one or more MANIFEST_BLOB_UNKNOWN errors code-1.

from spec.md.

In the community meeting on 8/25, we talked about changing this to "A registry MUST accept a manifest with a refers field that references a manifest that does not exist."

I'm concerned that this will open users up to accidentally pushing reference artifacts to the wrong repository and not getting feedback about it. I'd like to hear about the scenarios we're trying to support / will be blocked if we don't do this.

cc/ @sudo-bmitch @sajayantony

@sudo-bmitch
Copy link
Contributor

sudo-bmitch commented Aug 26, 2022

The use case I'm thinking of are users that want to ensure an image is complete before it's available to pull. It's similar to pushing blobs before pushing the manifest, but in this case, that descriptor is a reverse link on the child artifact.

The biggest example is ensuring the signature is available so there is never an unsigned image that could be pulled and trigger security alerts.

Over in the working group, we did pretty well putting these to a vote. So I'm happy to resolve this one in a similar way since we have two use cases pushing for different directions.

@michaelb990 michaelb990 changed the title Reference Types: Should manifests with a refers field that points to something that doesn't exist in the repository be accepted by a registry? [Vote] Should manifests with a refers field that points to something that doesn't exist in the repository be accepted by a registry? Sep 1, 2022
@michaelb990
Copy link
Contributor Author

Like this comment if you want the wording to change to "A registry MUST accept a manifest with a refers field that references a manifest that does not exist."

@michaelb990
Copy link
Contributor Author

Like this comment if you want the wording unchanged.

@vsoch
Copy link
Contributor

vsoch commented Sep 2, 2022

I’m confused about the use case and statement. You are saying there is risk in pushing an artifact that refers to something that doesn’t exist. E.g, I can only push my image that has its signature if the signature exists (otherwise I might forget). I’d assume the fix is to require the refers to field to exist first. But the change is saying that the refers to field must NOT exist. Why should it be required to not exist? Sorry if I’m missing something!

@sudo-bmitch
Copy link
Contributor

I’m confused about the use case and statement. You are saying there is risk in pushing an artifact that refers to something that doesn’t exist. E.g, I can only push my image that has its signature if the signature exists (otherwise I might forget). I’d assume the fix is to require the refers to field to exist first. But the change is saying that the refers to field must NOT exist. Why should it be required to not exist? Sorry if I’m missing something!

You can always push the image because the registry doesn't know if it will be later signed. The change here requires registries to allow a signature before the image is pushed. The signature manifest would have the refers field, and the image manifest it refers to would not exist.

@vsoch
Copy link
Contributor

vsoch commented Sep 2, 2022

Ok so the distinction is that I am required to push the signature first, and if I try pushing a signature and the image manifest is refers to does exist the push is rejected? And then as a user I would be required to do the whole process from scratch - deleting the image manifest being referred to, then doing the signature first followed by the image manifest. So the answer to this question, based on that:

[Vote] Should manifests with a refers field that points to something that doesn't exist in the repository be accepted by a registry

is that not only should it be accepted, but it should be required / mandated. And we would do this because we believe it enforces that an image that is supposed to have a signature definitely has it. But is this special to signatures? E.g.,think of all the metadata types to describe an image (that could be generated after) that would not allowed to be pushed because their image manifest already exists. Does that make sense?

If I understand this correctly it feels like a weird way of solving the original problem of ensuring the signature is there, because ultimately it would lead to people re-pulling, deleting, and then re-pushing things in the correct order. There could be two other ideas - perhaps it should be a custom requirement for only a specific kind of artifact, signatures (e.g., there is no harm in pushing new metadata in an artifact that points to an image that exists) or it should be enforced via the pull - the registry requires all image manifests to be signed (and if the signature isn't pushed yet, no go), and no worries about the temporal order that you do things. But maybe I'm overthinking it.

@sudo-bmitch
Copy link
Contributor

Ok so the distinction is that I am required to push the signature first, and if I try pushing a signature and the image manifest is refers to does exist the push is rejected?

This is for the registry server, saying what it's required to accept. If we say the registry may reject (or "should accept") the signature that is pushed before the image, then clients that want portability must push the image first. This change removes the ability for a registry to reject the early signature push, giving clients the option to push the signature and image in either order.

@vsoch
Copy link
Contributor

vsoch commented Sep 2, 2022

Ok gotcha! I guess that didn’t come across clearly from the statement. It’s hard to vote when the language leads me to multiple conclusions that were wrong, doh. Thanks for the explanation!

@sudo-bmitch
Copy link
Contributor

Reading I think I'm seeing the confusion and why the blob exception was written the way it was. We should say "if", so changing:

A registry MUST accept a manifest with a refers field that references a manifest that does not exist.

to:

A registry MUST accept a manifest with a refers field if it references a manifest that does not exist.

But even that feels like it's missing it since it can be implied that an invalid manifest suddenly overrides other checks by setting a refers field to an unknown manifest. Perhaps:

A registry MUST NOT reject a manifest because a refers field references a manifest that does not exist.

Is there a better word than "because"?

@vsoch
Copy link
Contributor

vsoch commented Sep 2, 2022

The last one is much more clear! You could also use “when.”

@sajayantony
Copy link
Member

sajayantony commented Sep 6, 2022

One of the scenarios here was about storing a reference without storing, say a large image. If we don't support registries from being able to store a reference artifact without the image that it points to, we probably need to define that this is not a goal or a way in which we can accomplish this goal?
Should we have a semantic that registries MUST accept, but may purge unreferenced Manifests like blobs?

/cc @vbatts @mikebrow

@sudo-bmitch
Copy link
Contributor

One of the scenarios here was about storing a reference without storing, say a large image. If we don't support registries from being able to store a reference artifact without the image that it points to, we probably need to define that this is not a goal or a way in which we can accomplish this goal?

Both SHOULD and MUST allow a registry to support this. I think it's more a question of whether a client can assume that's supported and whether those artifacts are portable to any OCI registry.

Should we have a semantic that registries MUST accept, but may purge unreferenced Manifests like blobs?

It starts to get into GC, but I'd like to see this defined.

@vsoch
Copy link
Contributor

vsoch commented Sep 6, 2022

I think that would be a nice compromise. I guess it would be up to the registry to communicate the practice?

@nima
Copy link

nima commented Sep 8, 2022

I'm concerned that this will open users up to accidentally pushing reference artifacts to the wrong repository and not getting feedback about it. I'd like to hear about the scenarios we're trying to support / will be blocked if we don't do this.

I don't think we addressed this in this discussion.

What if the server, in its response, would return a blob with a list of all artifacts that point to it, and clients can be set to verify this. Something along the lines of...

  1. push a(M) (artifact of manifest M)
  2. push M --list-artifacts // validate that a(M) is in there; this is expected to succeed.
  3. push M' --list-artifacts // validate that a(M') is in there; this is not expected to succeed.

@sudo-bmitch
Copy link
Contributor

What if the server, in its response, would return a blob with a list of all artifacts that point to it, and clients can be set to verify this.

I think this meant manifest, rather than blob. A blob has no structure to a registry, it can contain anything.

Is it normal for an HTTP PUT request to respond with a body? How would those APIs look? Overall I think that's a dramatically bigger change than we're describing here.

Something along the lines of...

  1. push a(M) (artifact of manifest M)
  2. push M --list-artifacts // validate that a(M) is in there; this is expected to succeed.
  3. push M' --list-artifacts // validate that a(M') is in there; this is not expected to succeed.

Does this mean pushing a manifest without first pushing an artifact would fail? If so, that's the reverse of what we're addressing here and would break existing registry use cases.

@michaelb990
Copy link
Contributor Author

On the call today we decided to move forward with stronger language around not rejecting manifests that refer to digests that aren't currently in the registry. @sudo-bmitch will open a PR for that to close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants