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

Solutions to "Registry MUST allow subject that references a missing manifest" #483

Closed
neersighted opened this issue Oct 17, 2023 · 15 comments

Comments

@neersighted
Copy link

neersighted commented Oct 17, 2023

  • This is a set of proposed solutions to Allow registries to reject non-existent subjects in manifests #459
  • This issue and set of options were jointly authored by @dmcgowan, @neersighted, and @sudo-bmitch.
  • We would like to poll for opinions on what the best way to move forward is; please 👍 any of the three options (multiple is encouraged!) that you believe would be suitable for you to move forward with.
  • We attempted to fairly and accurately summarize the problem and the pros/cons of each potential option here. I'd encourage discussion/clarification of the options in this issue, but anyone who is lost/not sure of the problem should refer back to/comment on the original issue (and resulting comment thread).

Problem Statement

  • The subject field and referrers API (collectively, referrers) allow registry responses to be altered without changing the digest of content
  • In the past, a simple HEAD to get a digest would be enough to determine that the content present in a cache/local copy/mirror was the same as in the remote registry
  • Now, to determine if "local state" (also, cache/mirror) is in sync with a remote registry, one the following must be performed:
    1. Recursively walk the manifest and referrers APIs to discover all the related content
    2. Assume that if you can see the digest in the registry, all the referrers that interest you are already uploaded
  • Some registry implementations have modeled the content as a distributed acyclic graph, where blobs are leaf/terminal nodes and manifests may not have dangling references.
    • This allows for modeling the correctness of content/preventing the upload of incorrect/rejected content as soom as possible.
    • This also allows for stronger references between objects, potentially aiding in access or GC.
  • If some registries require the manifest to be pushed before the referrers to that manifest, clients must support pushing referrers last (either as the default, or as a fallback) for portability
@neersighted
Copy link
Author

neersighted commented Oct 17, 2023

Opt 1: no change

  • Con
    • Disallows registries from enforcing hash tree integrity and allows references to manifests which have not been verified by the registry
    • Docker Hub and ECR will not be conformant
  • Pro
    • Clients implementations can be simplified, and unconditionally push referrers first
    • It may be easier to make assumptions about the state of the referrers API given the visibility of a manifest digest

Addendum: non-conformant registries could implement "automatic transactions" based on the subject digest

  • Con
    • Clients pushing a referrer manifest would get a 404 trying to pull it until the subject manifest is pushed
  • Pro
    • Could be implemented before a transaction API is standardized to minimize broken clients

@neersighted
Copy link
Author

neersighted commented Oct 17, 2023

Opt 2: change to "SHOULD", allowing registries to reject

  • Con
    • Content may be accessible without the associated metadata (users may attempt to run unsigned images)
    • Two assumptions (registry supports referreres first, referrers are live when the manifest is live) must be layered if you choose to assume that digest visibility indicates completeness of registry state
  • Pro
    • Allows registries to ensure all references are to verified/uploadable content/manifests
    • Clients are not surprised by otherwise-conformant registries deviating from spec behavior
    • Docker Hub and ECR are conformant

Addendum: tags could be suggested as a content visibility hint to clients

  • Con
    • Tags are the only way to delay visiblity (based on referrers) of content in a registry
    • Content can be correctly mirrored by digest
      • Content discovery can be done out of band (publish a digest without using a tag)
  • Pro
    • Hub and ECR are conformant
    • Clients are not surprised by otherwise-conformant registries deviating from spec behavior

@neersighted
Copy link
Author

neersighted commented Oct 17, 2023

Opt 3: leave as "MUST" and implement transactional API

  • Con
    • Time to implement, likely want to release 1.1 before this is done
    • Hub and ECR will not be 1.1-conformant and will hopefully introduce conformance for a future 1.2 release
  • Pro
    • Allows registries to enforce hash tree/graph integrity
    • Allows registries to ensure all references are to verified/uploadable content/manifests
    • Clients will be able to 'simply' ensure all referrers they care about and the content that is referred to goes live at the same time, if the registry supports this API

@dlorenc
Copy link

dlorenc commented Nov 7, 2023

Is there a next step here to closing this out? The voting seems to have stabilized in favor of option 1.

@jdolitsky
Copy link
Member

Is there a next step here to closing this out? The voting seems to have stabilized in favor of option 1.

Considering this is what we will do, just opened PR to cut 1.1 rc4: #487

@jdolitsky
Copy link
Member

@neersighted @dmcgowan @sudo-bmitch - serious question: what was the original intention of putting this up for vote?

We would like to poll for opinions on what the best way to move forward is

The poll has taken place. Can we move forward now? Are we going to just ignore the result of the vote?

@neersighted
Copy link
Author

neersighted commented Nov 13, 2023

I (read: we) opened this issue due to a lack of maintainer engagement and a perceived inability to move forward without more tangible action/making providing opinions more accessible.

I do think that at this point the result is clear; I appreciate all of those who strove to understand the issue and solicited informed opinions from others.

I would consider it presumptuous of me to bind the maintainers to a poll without prior agreement, but I hope that the results of this poll are a source of signal/major influence on whatever decision is finally made by the maintainers of the spec.

@dmcgowan
Copy link
Member

I believe Brandon wanted to get a gauge of where folks stood on possible solutions. I don't think we have a good solution proposed here that both solves the blocking change and alleviates concerns around client complexity. I am OK with closing out this issue and having the discussion in one place. The breaking change is still a release blocker. Once we have PR to discuss, it will be easier to vote on something more concrete.

@jdolitsky
Copy link
Member

Once we have PR to discuss, it will be easier to vote on something more concrete.

Given that the vote has been in favor of no change (Opt 1), there is in fact nothing left to vote on other than moving forward with a release. Am I wrong?

@jdolitsky
Copy link
Member

Closing this issue as resolved in favor of "Opt 1: no change" as described here.

Continuing the conversation in #459 with this new data point.

@sudo-bmitch
Copy link
Contributor

I was asking for it to get a signal from the community because this went back and forth between a few of us without anyone else weighing in. I was prepared to rewrite my implementation if the community went the other way and didn't see the value of pushing the metadata before the image.

The breaking change is still a release blocker.

I believe there's disagreement on whether this is a breaking change. Given that validation is a "MAY" in the spec, and this is a new field, I think it's appropriate for the spec to specify that this new field is outside of that permitted validation.

@jdolitsky
Copy link
Member

I believe there's disagreement on whether this is a breaking change.

As we heard last week, there is indeed disagreement about whether this is a breaking change. It's fairly subjective and a matter of opinion. It could be seen either way - which is why it had to come down to a vote.

The results of the vote show that most people concerned with the matter do not view this as a breaking change.

@jcarter3
Copy link

This vote was never publicized for community wide vote, and seems to be more internally shared. It was meant to initially gauge a rough sentiment, not to decide futures.

@jdolitsky
Copy link
Member

jdolitsky commented Nov 13, 2023

This vote was never publicized for community wide vote, and seems to be more internally shared. It was meant to initially gauge a rough sentiment, not to decide futures.

That's fair, but now we have "rough sentiment" and we are deciding not to do anything with it. I hope that maintainers will take this vote into account in #490.

@jcarter3
Copy link

What we have is people sharing links and asking people to vote their way 🤷‍♂️ Anything gathered from here should be completely abandoned.

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

No branches or pull requests

6 participants