launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

Thiago F. Pappacena has proposed merging ~pappacena/launchpad:dockerhub-registry-push into launchpad:master with ~pappacena/launchpad:refactor-oci-push-auth as a prerequisite.

Commit message:
Adding support for DockerHub image push and it's authentication protocol.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/384192
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:dockerhub-registry-push into launchpad:master.

diff --git a/lib/lp/oci/model/ociregistryclient.py b/lib/lp/oci/model/ociregistryclient.py
index c16e1a0..a935e05 100644
--- a/lib/lp/oci/model/ociregistryclient.py
+++ b/lib/lp/oci/model/ociregistryclient.py
@@ -15,6 +15,7 @@ import hashlib
 from io import BytesIO
 import json
 import logging
+import re
 import tarfile
 
 from requests.exceptions import (
@@ -304,27 +305,60 @@ class RegistryHTTPClient:
 
 
 class DockerHubHTTPClient(RegistryHTTPClient):
+    """Special case of RegistryHTTPClient for DockerHub.
+
+    This client type is prepared to deal with DockerHub's authorization
+    cycle, which involves fetching the appropriate authorization token
+    instead of using HTTP's basic auth.
+    """
+
     def __init__(self, push_rule):
         super(DockerHubHTTPClient, self).__init__(push_rule)
         self.auth_token = None
 
     @property
     def api_url(self):
-        """Get the base API URL for Dockerhub projects."""
+        """Get the base API URL for Dockerhub projects, including both the
+        image and user name."""
         push_rule = self.push_rule
         return "{}/v2/{}/{}".format(
             push_rule.registry_url, push_rule.username, push_rule.image_name)
 
-    def authenticate(self, last_failure):
-        """Tries to authenticate, considering the last HTTP 401 error."""
-        raise NotImplementedError("DockerHub auth is not implemented yet.")
+    def parseAuthInstructions(self, request):
+        """Parse the Www-Authenticate response header.
+
+        This method parses the appropriate header from the request and returns
+        the token type and the key-value pairs that should be used as query
+        parameters of the token GET request."""
+        instructions = request.headers['Www-Authenticate']
+        token_type, values = instructions.split(' ', 1)
+        dict_values = dict(re.compile(r'(.*?)="(.*?)"\,?').findall(values))
+        return token_type, dict_values
+
+    def authenticate(self, last_failed_request):
+        """Tries to authenticate, considering the last HTTP 401 failed
+        request."""
+        token_type, values = self.parseAuthInstructions(last_failed_request)
+        url = values.pop("realm")
+        # We should use the basic auth version for this request.
+        response = super(DockerHubHTTPClient, self).request(
+            url, params=values, method="GET", auth=self.credentials)
+        response.raise_for_status()
+        self.auth_token = response.json()["token"]
 
     def request(self, url, auth_retry=True, *args, **request_kwargs):
+        """Does a request, handling authentication cycle in case of 401
+        response.
+
+        :param auth_retry: Should we authentication and retry the request if
+                           it fails with HTTP 401 code?"""
         try:
-            return super(DockerHubHTTPClient, self).request(
-                url, *args, **request_kwargs)
+            headers = request_kwargs.pop("headers", {})
+            if self.auth_token is not None:
+                headers["Authorization"] = "Bearer %s" % self.auth_token
+            return urlfetch(url, headers=headers, **request_kwargs)
         except HTTPError as e:
-            if e.response.status_code == 401:
+            if auth_retry and e.response.status_code == 401:
                 self.authenticate(e.response)
                 return self.request(url, auth_retry=False, **request_kwargs)
             raise
diff --git a/lib/lp/oci/tests/test_ociregistryclient.py b/lib/lp/oci/tests/test_ociregistryclient.py
index 6ba25c0..90a8302 100644
--- a/lib/lp/oci/tests/test_ociregistryclient.py
+++ b/lib/lp/oci/tests/test_ociregistryclient.py
@@ -25,14 +25,20 @@ from testtools.matchers import (
     MatchesListwise,
     )
 import transaction
+from zope.security.proxy import removeSecurityProxy
 
 from lp.oci.model.ociregistryclient import (
+    DockerHubHTTPClient,
     OCIRegistryClient,
     RegistryHTTPClient,
     )
 from lp.oci.tests.helpers import OCIConfigHelperMixin
+from lp.services.compat import mock
 from lp.testing import TestCaseWithFactory
-from lp.testing.layers import LaunchpadZopelessLayer
+from lp.testing.layers import (
+    DatabaseFunctionalLayer,
+    LaunchpadZopelessLayer,
+    )
 
 
 class TestOCIRegistryClient(OCIConfigHelperMixin, TestCaseWithFactory):
@@ -302,3 +308,134 @@ class TestOCIRegistryClient(OCIConfigHelperMixin, TestCaseWithFactory):
         self.assertEqual(
             5, self.client._upload.retry.statistics["attempt_number"])
         self.assertEqual(5, self.retry_count)
+
+
+class TestDockerHubHTTPClient(OCIConfigHelperMixin, TestCaseWithFactory):
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestDockerHubHTTPClient, self).setUp()
+        self.setConfig()
+
+    def makeOCIPushRule(self):
+        credentials = self.factory.makeOCIRegistryCredentials(
+            url="https://registry.hub.docker.com";,
+            credentials={'username': 'the-user', 'password': "the-passwd"})
+        return self.factory.makeOCIPushRule(
+            registry_credentials=credentials,
+            image_name="test-image")
+
+    def test_api_url(self):
+        push_rule = self.makeOCIPushRule()
+        client = DockerHubHTTPClient(push_rule)
+        self.assertEqual(
+            "https://registry.hub.docker.com/v2/the-user/test-image";,
+            client.api_url)
+
+    def test_parse_instructions(self):
+        auth_header_content = (
+            'Bearer realm="https://auth.docker.io/token";,'
+            'service="registry.docker.io",'
+            'scope="repository:the-user/test-image:pull,push"')
+
+        request = mock.Mock()
+        request.headers = {'Www-Authenticate': auth_header_content}
+
+        push_rule = self.makeOCIPushRule()
+        client = DockerHubHTTPClient(push_rule)
+
+        self.assertEqual(
+            client.parseAuthInstructions(request), ("Bearer", {
+            "realm": "https://auth.docker.io/token";,
+            "service": "registry.docker.io",
+            "scope": "repository:the-user/test-image:pull,push"
+        }))
+
+    @responses.activate
+    def test_unauthorized_request_retries_fetching_token(self):
+        token_url = "https://auth.docker.io/token";
+        auth_header_content = (
+            'Bearer realm="%s",'
+            'service="registry.docker.io",'
+            'scope="repository:the-user/test-image:pull,push"') % token_url
+
+        url = "http://fake.launchpad.test/foo";
+        responses.add("GET", url, status=401, headers={
+            'Www-Authenticate': auth_header_content})
+        responses.add("GET", token_url, status=200, json={"token": "123abc"})
+        responses.add("GET", url, status=201, json={"success": True})
+
+        push_rule = removeSecurityProxy(self.makeOCIPushRule())
+        client = DockerHubHTTPClient(push_rule)
+        response = client.request(url)
+        self.assertEquals(201, response.status_code)
+        self.assertEquals(response.json(), {"success": True})
+
+        # Check that the 3 requests were made in order.
+        self.assertEquals(3, len(responses.calls))
+        failed_call, auth_call, success_call = responses.calls
+
+        self.assertEqual(url, failed_call.request.url)
+        self.assertEqual(401, failed_call.response.status_code)
+
+        self.assertStartsWith(auth_call.request.url, token_url)
+        self.assertEqual(200, auth_call.response.status_code)
+
+        self.assertEqual(url, success_call.request.url)
+        self.assertEqual(201, success_call.response.status_code)
+
+    @responses.activate
+    def test_unauthorized_request_retries_only_once(self):
+        token_url = "https://auth.docker.io/token";
+        auth_header_content = (
+            'Bearer realm="%s",'
+            'service="registry.docker.io",'
+            'scope="repository:the-user/test-image:pull,push"') % token_url
+
+        url = "http://fake.launchpad.test/foo";
+        responses.add("GET", url, status=401, headers={
+            'Www-Authenticate': auth_header_content})
+        responses.add("GET", token_url, status=200, json={"token": "123abc"})
+
+        push_rule = removeSecurityProxy(self.makeOCIPushRule())
+        client = DockerHubHTTPClient(push_rule)
+        self.assertRaises(HTTPError, client.request, url)
+
+        # Check that the 3 requests were made in order.
+        self.assertEquals(3, len(responses.calls))
+        failed_call, auth_call, second_failed_call = responses.calls
+
+        self.assertEqual(url, failed_call.request.url)
+        self.assertEqual(401, failed_call.response.status_code)
+
+        self.assertStartsWith(auth_call.request.url, token_url)
+        self.assertEqual(200, auth_call.response.status_code)
+
+        self.assertEqual(url, second_failed_call.request.url)
+        self.assertEqual(401, second_failed_call.response.status_code)
+
+    @responses.activate
+    def test_unauthorized_request_fails_to_get_token(self):
+        token_url = "https://auth.docker.io/token";
+        auth_header_content = (
+            'Bearer realm="%s",'
+            'service="registry.docker.io",'
+            'scope="repository:the-user/test-image:pull,push"') % token_url
+
+        url = "http://fake.launchpad.test/foo";
+        responses.add("GET", url, status=401, headers={
+            'Www-Authenticate': auth_header_content})
+        responses.add("GET", token_url, status=400)
+
+        push_rule = removeSecurityProxy(self.makeOCIPushRule())
+        client = DockerHubHTTPClient(push_rule)
+        self.assertRaises(HTTPError, client.request, url)
+
+        self.assertEquals(2, len(responses.calls))
+        failed_call, auth_call = responses.calls
+
+        self.assertEqual(url, failed_call.request.url)
+        self.assertEqual(401, failed_call.response.status_code)
+
+        self.assertStartsWith(auth_call.request.url, token_url)
+        self.assertEqual(400, auth_call.response.status_code)