Skip to content

Commit

Permalink
Modify makeRequestToResolvedURL and makeRequestToResolvedURLOnce to a…
Browse files Browse the repository at this point in the history 
…ccept an *url.URL

This is, sadly, wasteful, because NewRequestWithContext() only accepts
a string and parses it again, but it gives us more type safety, and simplifies
at least some callers.

Most importantly, this will also allow us to call url.Redacted() for logging.

Signed-off-by: Miloslav Trmač <[email protected]>
  • Loading branch information
@mtrmac
mtrmac committed Mar 17, 2022
1 parent f0d818b commit fa54b28
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 17 deletions.
34 changes: 22 additions & 12 deletions docker/docker_client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -463,7 +463,11 @@ func (c *dockerClient) makeRequest(ctx context.Context, method, path string, hea
return nil, err
}

url := fmt.Sprintf("%s://%s%s", c.scheme, c.registry, path)
urlString := fmt.Sprintf("%s://%s%s", c.scheme, c.registry, path)
url, err := url.Parse(urlString)
if err != nil {
return nil, err
}
return c.makeRequestToResolvedURL(ctx, method, url, headers, stream, -1, auth, extraScope)
}

Expand Down Expand Up @@ -500,7 +504,7 @@ func parseRetryAfter(res *http.Response, fallbackDelay time.Duration) time.Durat
// makeRequest should generally be preferred.
// In case of an HTTP 429 status code in the response, it may automatically retry a few times.
// TODO(runcom): too many arguments here, use a struct
func (c *dockerClient) makeRequestToResolvedURL(ctx context.Context, method, url string, headers map[string][]string, stream io.Reader, streamLen int64, auth sendAuth, extraScope *authScope) (*http.Response, error) {
func (c *dockerClient) makeRequestToResolvedURL(ctx context.Context, method string, url *url.URL, headers map[string][]string, stream io.Reader, streamLen int64, auth sendAuth, extraScope *authScope) (*http.Response, error) {
delay := backoffInitialDelay
attempts := 0
for {
Expand All @@ -518,7 +522,7 @@ func (c *dockerClient) makeRequestToResolvedURL(ctx context.Context, method, url
if delay > backoffMaxDelay {
delay = backoffMaxDelay
}
logrus.Debugf("Too many requests to %s: sleeping for %f seconds before next attempt", url, delay.Seconds())
logrus.Debugf("Too many requests to %s: sleeping for %f seconds before next attempt", url.String(), delay.Seconds())
select {
case <-ctx.Done():
return nil, ctx.Err()
Expand All @@ -533,8 +537,8 @@ func (c *dockerClient) makeRequestToResolvedURL(ctx context.Context, method, url
// streamLen, if not -1, specifies the length of the data expected on stream.
// makeRequest should generally be preferred.
// Note that no exponential back off is performed when receiving an http 429 status code.
func (c *dockerClient) makeRequestToResolvedURLOnce(ctx context.Context, method, url string, headers map[string][]string, stream io.Reader, streamLen int64, auth sendAuth, extraScope *authScope) (*http.Response, error) {
req, err := http.NewRequestWithContext(ctx, method, url, stream)
func (c *dockerClient) makeRequestToResolvedURLOnce(ctx context.Context, method string, url *url.URL, headers map[string][]string, stream io.Reader, streamLen int64, auth sendAuth, extraScope *authScope) (*http.Response, error) {
req, err := http.NewRequestWithContext(ctx, method, url.String(), stream)
if err != nil {
return nil, err
}
Expand All @@ -553,7 +557,7 @@ func (c *dockerClient) makeRequestToResolvedURLOnce(ctx context.Context, method,
return nil, err
}
}
logrus.Debugf("%s %s", method, url)
logrus.Debugf("%s %s", method, url.String())
res, err := c.client.Do(req)
if err != nil {
return nil, err
Expand Down Expand Up @@ -735,14 +739,17 @@ func (c *dockerClient) detectPropertiesHelper(ctx context.Context) error {
c.client = &http.Client{Transport: tr}

ping := func(scheme string) error {
url := fmt.Sprintf(resolvedPingV2URL, scheme, c.registry)
url, err := url.Parse(fmt.Sprintf(resolvedPingV2URL, scheme, c.registry))
if err != nil {
return err
}
resp, err := c.makeRequestToResolvedURL(ctx, http.MethodGet, url, nil, nil, -1, noAuth, nil)
if err != nil {
logrus.Debugf("Ping %s err %s (%#v)", url, err.Error(), err)
logrus.Debugf("Ping %s err %s (%#v)", url.String(), err.Error(), err)
return err
}
defer resp.Body.Close()
logrus.Debugf("Ping %s status %d", url, resp.StatusCode)
logrus.Debugf("Ping %s status %d", url.String(), resp.StatusCode)
if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusUnauthorized {
return httpResponseToError(resp, "")
}
Expand All @@ -762,14 +769,17 @@ func (c *dockerClient) detectPropertiesHelper(ctx context.Context) error {
}
// best effort to understand if we're talking to a V1 registry
pingV1 := func(scheme string) bool {
url := fmt.Sprintf(resolvedPingV1URL, scheme, c.registry)
url, err := url.Parse(fmt.Sprintf(resolvedPingV1URL, scheme, c.registry))
if err != nil {
return false
}
resp, err := c.makeRequestToResolvedURL(ctx, http.MethodGet, url, nil, nil, -1, noAuth, nil)
if err != nil {
logrus.Debugf("Ping %s err %s (%#v)", url, err.Error(), err)
logrus.Debugf("Ping %s err %s (%#v)", url.String(), err.Error(), err)
return false
}
defer resp.Body.Close()
logrus.Debugf("Ping %s status %d", url, resp.StatusCode)
logrus.Debugf("Ping %s status %d", url.String(), resp.StatusCode)
if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusUnauthorized {
return false
}
Expand Down
6 changes: 3 additions & 3 deletions docker/docker_image_dest.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,7 @@ func (d *dockerImageDestination) PutBlob(ctx context.Context, stream io.Reader,
// This error text should never be user-visible, we terminate only after makeRequestToResolvedURL
// returns, so there isn’t a way for the error text to be provided to any of our callers.
defer uploadReader.Terminate(errors.New("Reading data from an already terminated upload"))
res, err = d.c.makeRequestToResolvedURL(ctx, http.MethodPatch, uploadLocation.String(), map[string][]string{"Content-Type": {"application/octet-stream"}}, uploadReader, inputInfo.Size, v2Auth, nil)
res, err = d.c.makeRequestToResolvedURL(ctx, http.MethodPatch, uploadLocation, map[string][]string{"Content-Type": {"application/octet-stream"}}, uploadReader, inputInfo.Size, v2Auth, nil)
if err != nil {
logrus.Debugf("Error uploading layer chunked %v", err)
return nil, err
Expand All @@ -207,7 +207,7 @@ func (d *dockerImageDestination) PutBlob(ctx context.Context, stream io.Reader,
locationQuery := uploadLocation.Query()
locationQuery.Set("digest", blobDigest.String())
uploadLocation.RawQuery = locationQuery.Encode()
res, err = d.c.makeRequestToResolvedURL(ctx, http.MethodPut, uploadLocation.String(), map[string][]string{"Content-Type": {"application/octet-stream"}}, nil, -1, v2Auth, nil)
res, err = d.c.makeRequestToResolvedURL(ctx, http.MethodPut, uploadLocation, map[string][]string{"Content-Type": {"application/octet-stream"}}, nil, -1, v2Auth, nil)
if err != nil {
return types.BlobInfo{}, err
}
Expand Down Expand Up @@ -277,7 +277,7 @@ func (d *dockerImageDestination) mountBlob(ctx context.Context, srcRepo referenc
return errors.Wrap(err, "determining upload URL after a mount attempt")
}
logrus.Debugf("... started an upload instead of mounting, trying to cancel at %s", uploadLocation.String())
res2, err := d.c.makeRequestToResolvedURL(ctx, http.MethodDelete, uploadLocation.String(), nil, nil, -1, v2Auth, extraScope)
res2, err := d.c.makeRequestToResolvedURL(ctx, http.MethodDelete, uploadLocation, nil, nil, -1, v2Auth, extraScope)
if err != nil {
logrus.Debugf("Error trying to cancel an inadvertent upload: %s", err)
} else {
Expand Down
5 changes: 3 additions & 2 deletions docker/docker_image_src.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -253,13 +253,14 @@ func (s *dockerImageSource) getExternalBlob(ctx context.Context, urls []string)
return nil, 0, errors.New("internal error: getExternalBlob called with no URLs")
}
for _, u := range urls {
if u, err := url.Parse(u); err != nil || (u.Scheme != "http" && u.Scheme != "https") {
url, err := url.Parse(u)
if err != nil || (url.Scheme != "http" && url.Scheme != "https") {
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, u, nil, nil, -1, noAuth, nil)
resp, err = s.c.makeRequestToResolvedURL(ctx, http.MethodGet, url, nil, nil, -1, noAuth, nil)
if err == nil {
if resp.StatusCode != http.StatusOK {
err = errors.Errorf("error fetching external blob from %q: %d (%s)", u, resp.StatusCode, http.StatusText(resp.StatusCode))
Expand Down

0 comments on commit fa54b28

Please sign in to comment.