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

Fix c/image fails to pull OCI image with non-http(s):// urls #1403

Merged
merged 1 commit into from Nov 16, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 18 additions & 4 deletions docker/docker_image_src.go
Expand Up @@ -236,6 +236,9 @@ func (s *dockerImageSource) ensureManifestIsLoaded(ctx context.Context) error {
return nil
}

// getExternalBlob returns the reader of the first available blob URL from urls, which must not be empty.
// This function can return nil reader when no url is supported by this function. In this case, the caller
// should fallback to fetch the non-external blob (i.e. pull from the registry).
func (s *dockerImageSource) getExternalBlob(ctx context.Context, urls []string) (io.ReadCloser, int64, error) {
var (
resp *http.Response
Expand All @@ -244,21 +247,27 @@ func (s *dockerImageSource) getExternalBlob(ctx context.Context, urls []string)
if len(urls) == 0 {
return nil, 0, errors.New("internal error: getExternalBlob called with no URLs")
}
for _, url := range urls {
for _, u := range urls {
if u, err := url.Parse(u); err != nil || (u.Scheme != "http" && u.Scheme != "https") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely non-blocking: Do we want to silently ignore err here? The combination of “a failing HTTP fetch is a hard error” and “a completely invalid URL is silently accepted” is a bit weird.

OTOH ignoring them is consistent with the idea of allowing IPFS or whatever else, so this is fine as is — just raising this design point so that others speak up if they feel strongly about it.

continue // unsupported url. skip this url.
}
// NOTE: we must not authenticate on additional URLs as those
// can be abused to leak credentials or tokens. Please
// refer to CVE-2020-15157 for more information.
resp, err = s.c.makeRequestToResolvedURL(ctx, http.MethodGet, url, nil, nil, -1, noAuth, nil)
resp, err = s.c.makeRequestToResolvedURL(ctx, http.MethodGet, u, nil, nil, -1, noAuth, nil)
if err == nil {
if resp.StatusCode != http.StatusOK {
err = errors.Errorf("error fetching external blob from %q: %d (%s)", url, resp.StatusCode, http.StatusText(resp.StatusCode))
err = errors.Errorf("error fetching external blob from %q: %d (%s)", u, resp.StatusCode, http.StatusText(resp.StatusCode))
logrus.Debug(err)
resp.Body.Close()
continue
}
break
}
}
if resp == nil && err == nil {
return nil, 0, nil // fallback to non-external blob
}
if err != nil {
return nil, 0, err
}
Expand Down Expand Up @@ -408,7 +417,12 @@ func (s *dockerImageSource) GetBlobAt(ctx context.Context, info types.BlobInfo,
// May update BlobInfoCache, preferably after it knows for certain that a blob truly exists at a specific location.
func (s *dockerImageSource) GetBlob(ctx context.Context, info types.BlobInfo, cache types.BlobInfoCache) (io.ReadCloser, int64, error) {
if len(info.URLs) != 0 {
return s.getExternalBlob(ctx, info.URLs)
r, s, err := s.getExternalBlob(ctx, info.URLs)
if err != nil {
return nil, 0, err
} else if r != nil {
return r, s, nil
}
}

path := fmt.Sprintf(blobsPath, reference.Path(s.physicalRef.ref), info.Digest.String())
Expand Down
30 changes: 23 additions & 7 deletions oci/layout/oci_src.go
Expand Up @@ -5,6 +5,7 @@ import (
"io"
"io/ioutil"
"net/http"
"net/url"
"os"
"strconv"

Expand Down Expand Up @@ -113,7 +114,12 @@ func (s *ociImageSource) HasThreadSafeGetBlob() bool {
// May update BlobInfoCache, preferably after it knows for certain that a blob truly exists at a specific location.
func (s *ociImageSource) GetBlob(ctx context.Context, info types.BlobInfo, cache types.BlobInfoCache) (io.ReadCloser, int64, error) {
if len(info.URLs) != 0 {
return s.getExternalBlob(ctx, info.URLs)
r, s, err := s.getExternalBlob(ctx, info.URLs)
if err != nil {
return nil, 0, err
} else if r != nil {
return r, s, nil
}
}

path, err := s.ref.blobPath(info.Digest, s.sharedBlobDir)
Expand All @@ -140,34 +146,44 @@ func (s *ociImageSource) GetSignatures(ctx context.Context, instanceDigest *dige
return [][]byte{}, nil
}

// getExternalBlob returns the reader of the first available blob URL from urls, which must not be empty.
// This function can return nil reader when no url is supported by this function. In this case, the caller
// should fallback to fetch the non-external blob (i.e. pull from the registry).
func (s *ociImageSource) getExternalBlob(ctx context.Context, urls []string) (io.ReadCloser, int64, error) {
if len(urls) == 0 {
return nil, 0, errors.New("internal error: getExternalBlob called with no URLs")
}

errWrap := errors.New("failed fetching external blob from all urls")
for _, url := range urls {

req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil)
hasSupportedURL := false
for _, u := range urls {
if u, err := url.Parse(u); err != nil || (u.Scheme != "http" && u.Scheme != "https") {
continue // unsupported url. skip this url.
}
hasSupportedURL = true
req, err := http.NewRequestWithContext(ctx, http.MethodGet, u, nil)
if err != nil {
errWrap = errors.Wrapf(errWrap, "fetching %s failed %s", url, err.Error())
errWrap = errors.Wrapf(errWrap, "fetching %s failed %s", u, err.Error())
continue
}

resp, err := s.client.Do(req)
if err != nil {
errWrap = errors.Wrapf(errWrap, "fetching %s failed %s", url, err.Error())
errWrap = errors.Wrapf(errWrap, "fetching %s failed %s", u, err.Error())
continue
}

if resp.StatusCode != http.StatusOK {
resp.Body.Close()
errWrap = errors.Wrapf(errWrap, "fetching %s failed, response code not 200", url)
errWrap = errors.Wrapf(errWrap, "fetching %s failed, response code not 200", u)
continue
}

return resp.Body, getBlobSize(resp), nil
}
if !hasSupportedURL {
return nil, 0, nil // fallback to non-external blob
}

return nil, 0, errWrap
}
Expand Down