Fix/manifest digest on put #3849
Conversation
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## main #3849 +/- ##
==========================================
- Coverage 56.58% 55.87% -0.71%
==========================================
Files 105 110 +5
Lines 10636 11092 +456
==========================================
+ Hits 6018 6198 +180
- Misses 3946 4204 +258
- Partials 672 690 +18
☔ View full report in Codecov by Sentry. |
|
@napalu can you rebase to |
f8afaf8 to
6b82655
Compare
Signed-off-by: Florent Heyworth <[email protected]>
Signed-off-by: Florent Heyworth <[email protected]>
Signed-off-by: Florent Heyworth <[email protected]>
6b82655 to
12325c2
Compare
|
@milosgajdos done |
There was a problem hiding this comment.
LGTM. Although from reading this comment I think the Docker-* headers are legacy so I'm not sure what influence this change will have in the future.
|
This is also related to #3793. |
|
Stripping these |
|
Since this client is used in |
|
@thaJeztah do you mind having a look at this 🙇♂️ |
| // if we don't get a digest (do we really need one?) use a HEAD request since | ||
| // the spec mandates this should return a Docker-Content-Digest | ||
| if dgstHeader == "" { | ||
| req, _ := http.NewRequestWithContext(ctx, http.MethodHead, manifestURL, nil) | ||
| r, err := ms.client.Do(req) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| dgstHeader = r.Header.Get("Docker-Content-Digest") | ||
| } |
There was a problem hiding this comment.
I'm not sure if this is a good idea to trigger a second request, as this could easily lead to a race condition with other clients pushing a version of the manifest, which may be a different digest as the one that was just pushed by this client.
I also doubt this will help in cases you described where the header would be stripped, as it it's stripped from the PUT request, it would just as well be stripped from the HEAD request?
Take into account situations (such as proxies/load-balancers outside of your control) stripping Docker-Content-Digest header from responses when storing manifests
There was a problem hiding this comment.
Background for the pull request: in a setting with complex network topology (and many different teams/agencies involved) against a Nexus (Sonatype) registry we first thought some load-balancers/proxies were stripping the headers. The digest was however returned as part of the body. It later turned out the registry was not sending the digest header at all under certain circumstances (when using postgreSQL as an external metadata database but would return the header when using the embedded database - this behaviour has been fixed since). So in our case it turned out to be a misbehaving registry.
The client we are using (oc-mirror) only validates the digest if it is returned - the absence of a header is not considered an error. The distribution library however returns an error when no digest response is received on PUT. The HEAD request fallback was added as extra insurance in case the digest was not returned as part of the original headers or as part of the response body since the spec appears to mandate that a HEAD should return the digest.
| if mt != "" { | ||
| body, err := io.ReadAll(resp.Body) | ||
| if err == nil { | ||
| _, desc, err := distribution.UnmarshalManifest(mt, body) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| dgstHeader = desc.Digest.String() | ||
| } | ||
| } |
There was a problem hiding this comment.
Is this correct? Does the PUT response contain the uploaded data that was sent by the client? I think the response body would be either empty (only a static header) or an error-response in JSON format.
There was a problem hiding this comment.
Sonatype Nexus returns the digest as part of the body as well. I think some other registries do as well - see #3792 for context.
| // The specs are ambivalent on Docker-Content-Digest header: on the one hand, the docs at https://docs.docker.com/registry/spec/api/#digest-header | ||
| // mention that "any response may include a Docker-Content-Digest header" and on the other hand, the docs at | ||
| // https://docs.docker.com/registry/spec/api/#put-manifest say that a PUT request should return a Docker-Content-Digest header. |
There was a problem hiding this comment.
Note that the docs at https://docs.docker.com/registry/spec/api/ are built from the files in this repository, and describe the "Docker Registry HTTP API V2" specification, which has been superseded by the OCI distribution spec.
From the OCI distribution spec for the PUT /v2/<name>/manifests/<reference> endpoint
<name>is the namespace of the repository, and the<reference>MUST be either a) a digest or b) a tag.
and
The
Docker-Content-Digestheader returns the canonical digest of the uploaded blob, and MUST be equal to the client provided digest. Clients MAY ignore the value but if it is used, the client SHOULD verify the value against the uploaded blob data.
So I guess there's 2 possible situations here;
- when pushing the manifest with a digest as
<reference>, the header MUST be the same as that digest - when pushing the manifest with a name as
<reference>, the header must contain the canonical digest of the data
So, I'm not sure if it's possible to validate anything on the client side if the registry does not return a Docker-Content-Digest header (at least not with the current spec), other than the 201 status code, and trusting that the data was sent correctly;
The registry MUST store the manifest in the exact byte representation provided by the client. Upon a successful upload, the registry MUST return response code 201 Created,
With the current spec describing these headers as OPTIONAL;
Because of the origins this specification, the client MAY encounter Docker-specific headers, such as
Docker-Content-Digest, orDocker-Distribution-API-Version. These headers are OPTIONAL and clients SHOULD NOT depend on them.
So the only correct option is to ignore missing Docker-Content-Digest headers, and to skip validation.
That said, it feels like an omission in the OCI spec that there's no option to validate, so perhaps this should be raised as (per this discussion) it looks like there's no real alternative to verify if the registry calculated the expected digest.
Perhaps this endpoint needs a ?digest=<digest> query-parameter, as is provided for PUT /v2/<name>/blobs/uploads/<reference>?digest=<digest>, which would allow the registry to reject the request if the calculated ("canonical") digest does not match the digest that's provided by the client.
There was a problem hiding this comment.
I agree with your analysis that the absence of a digest should not result in an error - it should be up to the client to determine if a digest is expected.
Take into account situations (such as proxies/load-balancers outside of your control) stripping Docker-Content-Digest header from responses when storing manifests. Closes #3848