From 14bc664b4882c0686e7413ac23be53fa75ebc09f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 30 Mar 2016 20:31:18 +0200 Subject: [PATCH 1/4] Remove a redundant dockerImageSource.makeRequest parameter It is always computed in the same, or equivalent, way. Also remove pingResponse.needsAuth, only used in the above. --- docker.go | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/docker.go b/docker.go index 25d9efa2..0fde2c1c 100644 --- a/docker.go +++ b/docker.go @@ -87,7 +87,7 @@ func (i *dockerImage) Manifest() (types.ImageManifest, error) { func (i *dockerImage) getTags() ([]string, error) { // FIXME? Breaking the abstraction. url := fmt.Sprintf(tagsURL, i.src.scheme, i.src.registry, i.src.ref.RemoteName()) - res, err := i.src.makeRequest("GET", url, i.src.WWWAuthenticate != "", nil) + res, err := i.src.makeRequest("GET", url, nil) if err != nil { return nil, err } @@ -203,7 +203,7 @@ func (s *dockerImageSource) GetManifest() (manifest []byte, unverifiedCanonicalD url := fmt.Sprintf(manifestURL, s.scheme, s.registry, s.ref.RemoteName(), s.tag) // TODO(runcom) set manifest version header! schema1 for now - then schema2 etc etc and v1 // TODO(runcom) NO, switch on the resulter manifest like Docker is doing - res, err := s.makeRequest("GET", url, pr.needsAuth(), nil) + res, err := s.makeRequest("GET", url, nil) if err != nil { return nil, "", err } @@ -221,7 +221,7 @@ func (s *dockerImageSource) GetManifest() (manifest []byte, unverifiedCanonicalD func (s *dockerImageSource) GetLayer(digest string) (io.ReadCloser, error) { url := fmt.Sprintf(blobsURL, s.scheme, s.registry, s.ref.RemoteName(), digest) logrus.Infof("Downloading %s", url) - res, err := s.makeRequest("GET", url, s.WWWAuthenticate != "", nil) + res, err := s.makeRequest("GET", url, nil) if err != nil { return nil, err } @@ -236,7 +236,7 @@ func (s *dockerImageSource) GetSignatures() ([][]byte, error) { return [][]byte{}, nil } -func (s *dockerImageSource) makeRequest(method, url string, auth bool, headers map[string]string) (*http.Response, error) { +func (s *dockerImageSource) makeRequest(method, url string, headers map[string]string) (*http.Response, error) { req, err := http.NewRequest("GET", url, nil) if err != nil { return nil, err @@ -245,7 +245,7 @@ func (s *dockerImageSource) makeRequest(method, url string, auth bool, headers m for n, h := range headers { req.Header.Add(n, h) } - if auth { + if s.WWWAuthenticate != "" { if err := s.setupRequestAuth(req); err != nil { return nil, err } @@ -600,10 +600,6 @@ type pingResponse struct { errors []apiErr } -func (pr *pingResponse) needsAuth() bool { - return pr.WWWAuthenticate != "" -} - func (s *dockerImageSource) ping() (*pingResponse, error) { client := &http.Client{} if s.transport != nil { From 60fbdd3988c8dc8df060f7c053a457a7f5dafd20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 30 Mar 2016 20:42:22 +0200 Subject: [PATCH 2/4] Do not assume GetManifest is called before other dockerImageSource methods Call dockerImageSource.ping() in .makeRequest() if needed, instead of expecting a caller to do it (which only happened in GetManifest). This required splitting the URLs into the baseURL (dependent on .ping() result) and the suffix (independent of it), which was a simplification anyway. Also rename WWWAuthenticate to wwwAuthenticate, it is a private cache field. --- docker.go | 38 +++++++++++++++++++++----------------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/docker.go b/docker.go index 0fde2c1c..c7ad4fce 100644 --- a/docker.go +++ b/docker.go @@ -31,9 +31,9 @@ const ( dockerCfgObsolete = ".dockercfg" baseURL = "%s://%s/v2/" - tagsURL = baseURL + "%s/tags/list" - manifestURL = baseURL + "%s/manifests/%s" - blobsURL = baseURL + "%s/blobs/%s" + tagsURL = "%s/tags/list" + manifestURL = "%s/manifests/%s" + blobsURL = "%s/blobs/%s" ) var ( @@ -86,7 +86,7 @@ func (i *dockerImage) Manifest() (types.ImageManifest, error) { func (i *dockerImage) getTags() ([]string, error) { // FIXME? Breaking the abstraction. - url := fmt.Sprintf(tagsURL, i.src.scheme, i.src.registry, i.src.ref.RemoteName()) + url := fmt.Sprintf(tagsURL, i.src.ref.RemoteName()) res, err := i.src.makeRequest("GET", url, nil) if err != nil { return nil, err @@ -188,19 +188,13 @@ type dockerImageSource struct { registry string username string password string - WWWAuthenticate string // Obtained by s.ping() - scheme string // Obtained by s.ping() + wwwAuthenticate string // Cache of a value set by ping() if scheme is not empty + scheme string // Cache of a value returned by a successful ping() if not empty transport *http.Transport } func (s *dockerImageSource) GetManifest() (manifest []byte, unverifiedCanonicalDigest string, err error) { - pr, err := s.ping() - if err != nil { - return nil, "", err - } - s.WWWAuthenticate = pr.WWWAuthenticate - s.scheme = pr.scheme - url := fmt.Sprintf(manifestURL, s.scheme, s.registry, s.ref.RemoteName(), s.tag) + url := fmt.Sprintf(manifestURL, s.ref.RemoteName(), s.tag) // TODO(runcom) set manifest version header! schema1 for now - then schema2 etc etc and v1 // TODO(runcom) NO, switch on the resulter manifest like Docker is doing res, err := s.makeRequest("GET", url, nil) @@ -219,7 +213,7 @@ func (s *dockerImageSource) GetManifest() (manifest []byte, unverifiedCanonicalD } func (s *dockerImageSource) GetLayer(digest string) (io.ReadCloser, error) { - url := fmt.Sprintf(blobsURL, s.scheme, s.registry, s.ref.RemoteName(), digest) + url := fmt.Sprintf(blobsURL, s.ref.RemoteName(), digest) logrus.Infof("Downloading %s", url) res, err := s.makeRequest("GET", url, nil) if err != nil { @@ -237,6 +231,16 @@ func (s *dockerImageSource) GetSignatures() ([][]byte, error) { } func (s *dockerImageSource) makeRequest(method, url string, headers map[string]string) (*http.Response, error) { + if s.scheme == "" { + pr, err := s.ping() + if err != nil { + return nil, err + } + s.wwwAuthenticate = pr.WWWAuthenticate + s.scheme = pr.scheme + } + + url = fmt.Sprintf(baseURL, s.scheme, s.registry) + url req, err := http.NewRequest("GET", url, nil) if err != nil { return nil, err @@ -245,7 +249,7 @@ func (s *dockerImageSource) makeRequest(method, url string, headers map[string]s for n, h := range headers { req.Header.Add(n, h) } - if s.WWWAuthenticate != "" { + if s.wwwAuthenticate != "" { if err := s.setupRequestAuth(req); err != nil { return nil, err } @@ -262,9 +266,9 @@ func (s *dockerImageSource) makeRequest(method, url string, headers map[string]s } func (s *dockerImageSource) setupRequestAuth(req *http.Request) error { - tokens := strings.SplitN(strings.TrimSpace(s.WWWAuthenticate), " ", 2) + tokens := strings.SplitN(strings.TrimSpace(s.wwwAuthenticate), " ", 2) if len(tokens) != 2 { - return fmt.Errorf("expected 2 tokens in WWW-Authenticate: %d, %s", len(tokens), s.WWWAuthenticate) + return fmt.Errorf("expected 2 tokens in WWW-Authenticate: %d, %s", len(tokens), s.wwwAuthenticate) } switch tokens[0] { case "Basic": From 7bee2da169684c3590089bee15d22300287e7c09 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 2 May 2016 19:19:51 +0200 Subject: [PATCH 3/4] Use the provided method in dockerImageSource.makeRequest instead of hard-coding GET --- docker.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker.go b/docker.go index c7ad4fce..4a56ba65 100644 --- a/docker.go +++ b/docker.go @@ -241,7 +241,7 @@ func (s *dockerImageSource) makeRequest(method, url string, headers map[string]s } url = fmt.Sprintf(baseURL, s.scheme, s.registry) + url - req, err := http.NewRequest("GET", url, nil) + req, err := http.NewRequest(method, url, nil) if err != nil { return nil, err } From 654050b7e8898b8e94a974190ec57229a8599aac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 2 May 2016 19:22:52 +0200 Subject: [PATCH 4/4] Fix handling of !tlsVerify when certPath is not set --- docker.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/docker.go b/docker.go index 4a56ba65..7bc087c0 100644 --- a/docker.go +++ b/docker.go @@ -477,15 +477,16 @@ func newDockerImageSource(img, certPath string, tlsVerify bool) (*dockerImageSou return nil, err } var tr *http.Transport - if certPath != "" { + if certPath != "" || !tlsVerify { tlsc := &tls.Config{} - cert, err := tls.LoadX509KeyPair(filepath.Join(certPath, "cert.pem"), filepath.Join(certPath, "key.pem")) - if err != nil { - return nil, fmt.Errorf("Error loading x509 key pair: %s", err) + if certPath != "" { + cert, err := tls.LoadX509KeyPair(filepath.Join(certPath, "cert.pem"), filepath.Join(certPath, "key.pem")) + if err != nil { + return nil, fmt.Errorf("Error loading x509 key pair: %s", err) + } + tlsc.Certificates = append(tlsc.Certificates, cert) } - - tlsc.Certificates = append(tlsc.Certificates, cert) tlsc.InsecureSkipVerify = !tlsVerify tr = &http.Transport{ TLSClientConfig: tlsc,