Skip to content

Commit

Permalink
ociclient: require digests for descriptors
Browse files Browse the repository at this point in the history
In PR #29, I suggested that we should verify that the
tests fail when the fix was not applied, but the tests
did not fail in that case.

It turns out that's because the client was being a bit
too slack about requiring digests in responses, meaning
that the client succeeded even when it should have failed.

This change makes `ociclient.descriptorFromResponse`
a little more stringent about digests, and I've explicitly
verified that when the changes in #29 are reverted,
the tests do in fact fail as expected.

Signed-off-by: Roger Peppe <rogpeppe@gmail.com>
Change-Id: Ic7352b572593a60dc2209193fdd64bf2e4235af9
Dispatch-Trailer: {"type":"trybot","CL":1191111,"patchset":4,"ref":"refs/changes/11/1191111/4","targetBranch":"main"}
  • Loading branch information
rogpeppe authored and porcuepine committed Apr 3, 2024
1 parent 3a31e47 commit fee694a
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 10 deletions.
14 changes: 12 additions & 2 deletions ociregistry/ociclient/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -124,18 +124,25 @@ type client struct {
listPageSize int
}

type descriptorRequired byte

const (
requireSize descriptorRequired = 1 << iota
requireDigest
)

// descriptorFromResponse tries to form a descriptor from an HTTP response,
// filling in the Digest field using knownDigest if it's not present.
//
// Note: this implies that the Digest field will be empty if there is no
// digest in the response and knownDigest is empty.
func descriptorFromResponse(resp *http.Response, knownDigest digest.Digest, requireSize bool) (ociregistry.Descriptor, error) {
func descriptorFromResponse(resp *http.Response, knownDigest digest.Digest, require descriptorRequired) (ociregistry.Descriptor, error) {
contentType := resp.Header.Get("Content-Type")
if contentType == "" {
contentType = "application/octet-stream"
}
size := int64(0)
if requireSize {
if (require & requireSize) != 0 {
if resp.StatusCode == http.StatusPartialContent {
contentRange := resp.Header.Get("Content-Range")
if contentRange == "" {
Expand Down Expand Up @@ -165,6 +172,9 @@ func descriptorFromResponse(resp *http.Response, knownDigest digest.Digest, requ
} else {
digest = knownDigest
}
if (require&requireDigest) != 0 && digest == "" {
return ociregistry.Descriptor{}, fmt.Errorf("no digest found in response")
}
return ociregistry.Descriptor{
Digest: digest,
MediaType: contentType,
Expand Down
11 changes: 4 additions & 7 deletions ociregistry/ociclient/reader.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ func (c *client) GetBlobRange(ctx context.Context, repo string, digest ociregist
// Fix that either by returning ErrUnsupported or by reading the whole
// blob and returning only the required portion.
defer closeOnError(&_err, resp.Body)
desc, err := descriptorFromResponse(resp, ociregistry.Digest(rreq.Digest), true)
desc, err := descriptorFromResponse(resp, ociregistry.Digest(rreq.Digest), requireSize)
if err != nil {
return nil, fmt.Errorf("invalid descriptor in response: %v", err)
}
Expand Down Expand Up @@ -97,7 +97,7 @@ func (c *client) resolve(ctx context.Context, rreq *ocirequest.Request) (ociregi
return ociregistry.Descriptor{}, err
}
resp.Body.Close()
desc, err := descriptorFromResponse(resp, ociregistry.Digest(rreq.Digest), true)
desc, err := descriptorFromResponse(resp, ociregistry.Digest(rreq.Digest), requireSize|requireDigest)
if err != nil {
return ociregistry.Descriptor{}, fmt.Errorf("invalid descriptor in response: %v", err)
}
Expand Down Expand Up @@ -138,7 +138,7 @@ func (c *client) read(ctx context.Context, rreq *ocirequest.Request) (_ ociregis
return nil, err
}
defer closeOnError(&_err, resp.Body)
desc, err := descriptorFromResponse(resp, ociregistry.Digest(rreq.Digest), true)
desc, err := descriptorFromResponse(resp, ociregistry.Digest(rreq.Digest), requireSize)
if err != nil {
return nil, fmt.Errorf("invalid descriptor in response: %v", err)
}
Expand Down Expand Up @@ -176,13 +176,10 @@ func (c *client) read(ctx context.Context, rreq *ocirequest.Request) (_ ociregis
return nil, err
}
resp1.Body.Close()
desc, err = descriptorFromResponse(resp1, ociregistry.Digest(rreq1.Digest), true)
desc, err = descriptorFromResponse(resp1, ociregistry.Digest(rreq1.Digest), requireSize|requireDigest)
if err != nil {
return nil, err
}
if desc.Digest == "" {
return nil, fmt.Errorf("no digest header found in response")
}
}
}
return newBlobReader(resp.Body, desc), nil
Expand Down
3 changes: 2 additions & 1 deletion ociregistry/ociclient/writer.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ func (c *client) MountBlob(ctx context.Context, fromRepo, toRepo string, dig oci
// return Unsupported.
return ociregistry.Descriptor{}, fmt.Errorf("registry does not support mounts: %w", ociregistry.ErrUnsupported)
}
return descriptorFromResponse(resp, dig, false)
// TODO: is it OK to omit the size from the returned descriptor here?
return descriptorFromResponse(resp, dig, requireDigest)
}

func (c *client) PushBlob(ctx context.Context, repo string, desc ociregistry.Descriptor, r io.Reader) (_ ociregistry.Descriptor, _err error) {
Expand Down

0 comments on commit fee694a

Please sign in to comment.