launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> === modified file 'config.yaml'
> --- config.yaml	2015-04-02 12:26:26 +0000
> +++ config.yaml	2015-05-22 15:44:58 +0000
> @@ -12,4 +12,7 @@
>  virtinfo_endpoint: http://xmlrpc-private.launchpad.dev:8087/git
>  cgit_exec_path: /usr/lib/cgit/cgit.cgi
>  cgit_data_path: /usr/share/cgit
> +cgit_secret_path: null
> +openid_provider_root: https://testopenid.dev/
>  site_name: git.launchpad.dev
> +main_site_root: https://launchpad.dev/
> 
> === modified file 'httpserver.tac'
> --- httpserver.tac	2015-04-26 21:14:39 +0000
> +++ httpserver.tac	2015-05-22 15:44:58 +0000
> @@ -19,13 +19,7 @@
>  
>      config = TurnipConfig()
>      smarthttp_site = server.Site(
> -        SmartHTTPFrontendResource(b'localhost',
> -                                  config.get('pack_virt_port'),
> -                                  config.get('virtinfo_endpoint'),
> -                                  config.get('repo_store'),
> -                                  cgit_exec_path=config.get('cgit_exec_path'),
> -                                  cgit_data_path=config.get('cgit_data_path'),
> -                                  site_name=config.get('site_name')))
> +        SmartHTTPFrontendResource(b'localhost', config))
>      return internet.TCPServer(config.get('smart_http_port'), smarthttp_site)
>  
>  
> 
> === modified file 'requirements.txt'
> --- requirements.txt	2015-04-28 21:36:37 +0000
> +++ requirements.txt	2015-05-22 15:44:58 +0000
> @@ -1,13 +1,19 @@
>  contextlib2==0.4.0
>  cornice==0.19.0
>  lazr.sshserver==0.1.1
> +Paste==2.0.2
>  PasteDeploy==1.5.2
>  pyasn1==0.1.7
>  pycrypto==2.6.1
>  pyramid==1.5.4
> +# XXX: deryck 2012-08-10
> +# See lp:~deryck/python-openid/python-openid-fix1034376 which
> +# reapplied a patch from wgrant to get codehosting going again.
> +python-openid==2.2.5-fix1034376
>  PyYAML==3.11
>  repoze.lru==0.6
>  simplejson==3.6.5
> +six==1.9.0
>  translationstring==1.3
>  Twisted==15.0.0
>  venusian==1.0
> 
> === modified file 'turnip/pack/http.py'
> --- turnip/pack/http.py	2015-04-26 22:53:26 +0000
> +++ turnip/pack/http.py	2015-05-22 15:44:58 +0000
> @@ -6,16 +6,32 @@
>  
>  from cStringIO import StringIO
>  import os.path
> +import pickle
>  import tempfile
>  import textwrap
> +try:
> +    from urllib.parse import urlencode
> +except ImportError:
> +    from urllib import urlencode
>  import sys
>  import zlib
>  
> +from openid.consumer import consumer
> +from openid.extensions.sreg import (
> +    SRegRequest,
> +    SRegResponse,
> +    )
> +from paste.auth.cookie import (
> +    AuthCookieSigner,
> +    decode as decode_cookie,
> +    encode as encode_cookie,
> +    )
>  from twisted.internet import (
>      defer,
>      protocol,
>      reactor,
>      )
> +from twisted.python.urlpath import URLPath
>  from twisted.web import (
>      http,
>      resource,
> @@ -338,23 +354,175 @@
>  class CGitScriptResource(twcgi.CGIScript):
>      """HTTP resource to run cgit."""
>  
> -    def __init__(self, root):
> +    def __init__(self, root, repo_url, repo_path, trailing):
>          twcgi.CGIScript.__init__(self, root.cgit_exec_path)
>          self.root = root
> +        self.repo_url = repo_url
> +        self.repo_path = repo_path
> +        self.trailing = trailing
>          self.cgit_config = None
>  
> -    def _error(self, request, message, code=http.INTERNAL_SERVER_ERROR):
> -        request.setResponseCode(code)
> -        request.setHeader(b'Content-Type', b'text/plain')
> -        request.write(message)
> -        request.finish()
> -
>      def _finished(self, ignored):
>          if self.cgit_config is not None:
>              self.cgit_config.close()
>  
> -    def _translatePathCallback(self, translated, env, request,
> -                               *args, **kwargs):
> +    def runProcess(self, env, request, *args, **kwargs):
> +        request.notifyFinish().addBoth(self._finished)
> +        self.cgit_config = tempfile.NamedTemporaryFile(
> +            mode='w+', prefix='turnip-cgit-')
> +        os.chmod(self.cgit_config.name, 0o644)
> +        fmt = {'repo_url': self.repo_url, 'repo_path': self.repo_path}
> +        if self.root.site_name is not None:
> +            prefixes = " ".join(
> +                "{}://{}".format(scheme, self.root.site_name)
> +                for scheme in ("git", "git+ssh", "https"))
> +            print("clone-prefix={}".format(prefixes), file=self.cgit_config)
> +        print(textwrap.dedent("""\
> +            css=/static/cgit.css
> +            enable-http-clone=0
> +            enable-index-owner=0
> +            logo=/static/launchpad-logo.png
> +
> +            repo.url={repo_url}
> +            repo.path={repo_path}
> +            """).format(**fmt), file=self.cgit_config)
> +        self.cgit_config.flush()
> +        env["CGIT_CONFIG"] = self.cgit_config.name
> +        env["PATH_INFO"] = "/%s%s" % (self.repo_url, self.trailing)
> +        env["SCRIPT_NAME"] = "/"
> +        twcgi.CGIScript.runProcess(self, env, request, *args, **kwargs)
> +
> +
> +class BaseHTTPAuthResource(resource.Resource):
> +    """Base HTTP resource for OpenID authentication handling."""
> +
> +    session_var = 'turnip.session'
> +    cookie_name = 'TURNIP_COOKIE'
> +    anonymous_id = '+launchpad-anonymous'
> +
> +    def __init__(self, root):
> +        resource.Resource.__init__(self)
> +        self.root = root
> +        if root.cgit_secret is not None:
> +            self.signer = AuthCookieSigner(root.cgit_secret)
> +        else:
> +            self.signer = None
> +
> +    def _getSession(self, request):
> +        if self.signer is not None:
> +            cookie = request.getCookie(self.cookie_name)
> +            if cookie is not None:
> +                content = self.signer.auth(cookie)
> +                if content:
> +                    return pickle.loads(decode_cookie(content))

Let's not turn an impersonation vulnerability into remote code execution. JSON should be fine here.

> +        return {}
> +
> +    def _putSession(self, request, session):
> +        if self.signer is not None:
> +            content = self.signer.sign(encode_cookie(pickle.dumps(session)))
> +            cookie = '%s=%s; Path=/; secure;' % (self.cookie_name, content)
> +            request.setHeader(b'Set-Cookie', cookie.encode('UTF-8'))
> +
> +    def _error(self, request, message, code=http.INTERNAL_SERVER_ERROR):
> +        request.setResponseCode(code)
> +        request.setHeader(b'Content-Type', b'text/plain')
> +        request.write(message)
> +        request.finish()
> +
> +    def _makeConsumer(self, session):
> +        """Build an OpenID `Consumer` object with standard arguments."""
> +        # Multiple instances need to share a store or not use one at all (in
> +        # which case they will use check_authentication).  Using no store is
> +        # easier, and check_authentication is cheap.
> +        return consumer.Consumer(session, None)
> +
> +
> +class HTTPAuthLoginResource(BaseHTTPAuthResource):
> +    """HTTP resource to complete OpenID authentication."""
> +
> +    def render_GET(self, request):
> +        """Complete the OpenID authentication process.
> +
> +        Here we handle the result of the OpenID process.  If the process
> +        succeeded, we record the identity URL and username in the session
> +        and redirect the user to the page they were trying to view that
> +        triggered the login attempt.  In the various failure cases we return
> +        a 401 Unauthorized response with a brief explanation of what went
> +        wrong.
> +        """
> +        # XXX cjwatson 2015-05-21: Bring logging into sync with the similar
> +        # method in launchpad/lib/launchpad_loggerhead/app.py.
> +        session = self._getSession(request)
> +        response = self._makeConsumer(session).complete(
> +            request.args, request.args['openid.return_to'])
> +        if response.status == consumer.SUCCESS:
> +            sreg_info = SRegResponse.fromSuccessResponse(response)
> +            if not sreg_info:
> +                message = (
> +                    "You don't have a Launchpad account.  Check that you're "
> +                    "logged in as the right user, or log into Launchpad and "
> +                    "try again.")
> +                self._error(request, message, http.UNAUTHORIZED)
> +            else:
> +                session['identity_url'] = response.identity_url
> +                session['user'] = sreg_info['nickname']
> +                self._putSession(request, session)
> +                request.redirect(request.args['back_to'])
> +                request.finish()
> +        elif response.status == consumer.FAILURE:
> +            self._error(request, response.message, http.UNAUTHORIZED)
> +        elif response.status == consumer.CANCEL:
> +            self._error(
> +                request, 'Authentication cancelled.', http.UNAUTHORIZED)
> +        else:
> +            self._error(request, 'Unknown OpenID response.', http.UNAUTHORIZED)
> +
> +
> +class HTTPAuthLogoutResource(BaseHTTPAuthResource):
> +    """HTTP resource to log out of OpenID authentication."""
> +
> +    def render_GET(self, request):
> +        """Log out of turnip.
> +
> +        Clear the cookie and redirect to `next_to`.
> +        """
> +        self._putSession(request, {})
> +        next_url = request.args.get('next_to')
> +        if next_url is None:
> +            next_url = self.root.main_site_root
> +        request.redirect(next_url)
> +        return b''
> +
> +
> +class HTTPAuthRootResource(BaseHTTPAuthResource):
> +    """HTTP resource to translate a path and authenticate if necessary.
> +
> +    Requests that require further authentication are denied or sent through
> +    OpenID redirection, as appropriate.  Properly-authenticated requests are
> +    passed on to cgit.
> +    """
> +
> +    def _beginLogin(self, request, session):
> +        """Start the process of authenticating with OpenID.
> +
> +        We redirect the user to Launchpad to identify themselves.  Launchpad
> +        will then redirect them to our +login page with enough information
> +        that we can then redirect them again to the page they were looking
> +        at, with a cookie that gives us the identity URL and username.
> +        """
> +        openid_request = self._makeConsumer(session).begin(
> +            self.root.openid_provider_root)
> +        openid_request.addExtension(SRegRequest(required=['nickname']))
> +        urlpath = URLPath.fromRequest(request)
> +        back_to = str(urlpath)
> +        base_url = str(urlpath.click('/'))
> +        target = openid_request.redirectURL(
> +            base_url,
> +            base_url + '+login/?' + urlencode({'back_to': back_to}))
> +        request.redirect(target.encode('UTF-8'))
> +        request.finish()
> +
> +    def _translatePathCallback(self, translated, request):
>          if 'path' not in translated:
>              self._error(
>                  request, b'translatePath response did not include path')
> @@ -384,75 +552,69 @@
>                  return
>              repo_url = repo_url[:-len(trailing)]
>          repo_url = repo_url.strip('/')
> -        request.notifyFinish().addBoth(self._finished)
> -        self.cgit_config = tempfile.NamedTemporaryFile(
> -            mode='w+', prefix='turnip-cgit-')
> -        os.chmod(self.cgit_config.name, 0o644)
> -        fmt = {'repo_url': repo_url, 'repo_path': repo_path}
> -        if self.root.site_name is not None:
> -            prefixes = " ".join(
> -                "{}://{}".format(scheme, self.root.site_name)
> -                for scheme in ("git", "git+ssh", "https"))
> -            print("clone-prefix={}".format(prefixes), file=self.cgit_config)
> -        print(textwrap.dedent("""\
> -            css=/static/cgit.css
> -            enable-http-clone=0
> -            enable-index-owner=0
> -            logo=/static/launchpad-logo.png
> -
> -            repo.url={repo_url}
> -            repo.path={repo_path}
> -            """).format(**fmt), file=self.cgit_config)
> -        self.cgit_config.flush()
> -        env["CGIT_CONFIG"] = self.cgit_config.name
> -        env["PATH_INFO"] = "/%s%s" % (repo_url, trailing)
> -        env["SCRIPT_NAME"] = "/"
> -        twcgi.CGIScript.runProcess(self, env, request, *args, **kwargs)
> -
> -    def _translatePathErrback(self, failure, request):
> +        cgit_resource = CGitScriptResource(
> +            self.root, repo_url, repo_path, trailing)
> +        request.render(cgit_resource)
> +
> +    def _translatePathErrback(self, failure, request, session):
>          e = failure.value
> +        message = e.faultString
>          if e.faultCode in (1, 290):
>              error_code = http.NOT_FOUND
>          elif e.faultCode in (2, 310):
>              error_code = http.FORBIDDEN
>          elif e.faultCode in (3, 410):
> -            # XXX cjwatson 2015-03-30: should be UNAUTHORIZED, but we
> -            # don't implement that yet
> -            error_code = http.FORBIDDEN
> +            if 'user' in session:
> +                error_code = http.UNAUTHORIZED

401ing a web browser without sending a WWW-Authenticate header is a Bad Idea. Should be 403 instead.

> +                message = 'You are logged in as %s.' % session['user']

This probably also wants to say that you're not allowed to access the repo.

> +            else:
> +                self._beginLogin(request, session)
> +                return
>          else:
>              error_code = http.INTERNAL_SERVER_ERROR
> -        self._error(request, e.faultString, code=error_code)
> +        self._error(request, message, code=error_code)
>  
> -    def runProcess(self, env, request, *args, **kwargs):
> +    def render_GET(self, request):
> +        session = self._getSession(request)
> +        identity_url = session.get('identity_url', self.anonymous_id)
>          proxy = xmlrpc.Proxy(self.root.virtinfo_endpoint, allowNone=True)
> -        # XXX cjwatson 2015-03-30: authentication
>          d = proxy.callRemote(
> -            b'translatePath', request.path, b'read', None, False)
> -        d.addCallback(
> -            self._translatePathCallback, env, request, *args, **kwargs)
> -        d.addErrback(self._translatePathErrback, request)
> +            b'translatePath', request.path, b'read', identity_url, True)
> +        d.addCallback(self._translatePathCallback, request)
> +        d.addErrback(self._translatePathErrback, request, session)
>          return server.NOT_DONE_YET
>  
>  
> +class HTTPAuthResource(resource.Resource):
> +    """Container for the various HTTP authentication resources."""
> +
> +    def __init__(self, root):
> +        resource.Resource.__init__(self)
> +        self.putChild('', HTTPAuthRootResource(root))
> +        self.putChild('+login', HTTPAuthLoginResource(root))
> +        self.putChild('+logout', HTTPAuthLogoutResource(root))
> +
> +
>  class SmartHTTPFrontendResource(resource.Resource):
>      """HTTP resource to translate Git smart HTTP requests to pack protocol."""
>  
>      allowed_services = frozenset((b'git-upload-pack', b'git-receive-pack'))
>  
> -    def __init__(self, backend_host, backend_port, virtinfo_endpoint,
> -                 repo_store, cgit_exec_path=None, cgit_data_path=None,
> -                 site_name=None):
> +    def __init__(self, backend_host, config):
>          resource.Resource.__init__(self)
>          self.backend_host = backend_host
> -        self.backend_port = backend_port
> -        self.virtinfo_endpoint = virtinfo_endpoint
> +        self.backend_port = config.get("pack_virt_port")
> +        self.virtinfo_endpoint = config.get("virtinfo_endpoint")
>          # XXX cjwatson 2015-03-30: Knowing about the store path here
>          # violates turnip's layering and may cause scaling problems later,
>          # but for now cgit needs direct filesystem access.
> -        self.repo_store = repo_store
> -        self.cgit_exec_path = cgit_exec_path
> -        self.site_name = site_name
> +        self.repo_store = config.get("repo_store")
> +        self.cgit_exec_path = config.get("cgit_exec_path")
> +        self.openid_provider_root = config.get("openid_provider_root")
> +        self.site_name = config.get("site_name")
> +        self.main_site_root = config.get("main_site_root")
>          self.putChild('', SmartHTTPRootResource())
> +        cgit_data_path = config.get("cgit_data_path")
>          if cgit_data_path is not None:
>              static_resource = DirectoryWithoutListings(
>                  cgit_data_path, defaultType='text/plain')
> @@ -462,6 +624,12 @@
>              self.putChild('static', static_resource)
>              favicon = os.path.join(top, 'images', 'launchpad.png')
>              self.putChild('favicon.ico', static.File(favicon))
> +        cgit_secret_path = config.get("cgit_secret_path")
> +        if cgit_secret_path:
> +            with open(cgit_secret_path, 'rb') as cgit_secret_file:
> +                self.cgit_secret = cgit_secret_file.read()
> +        else:
> +            self.cgit_secret = None
>  
>      @staticmethod
>      def _isGitRequest(request):
> @@ -489,7 +657,7 @@
>              if service in self.allowed_services:
>                  return SmartHTTPCommandResource(self, service, path)
>          elif self.cgit_exec_path is not None:
> -            return CGitScriptResource(self)
> +            return HTTPAuthResource(self)
>          return resource.NoResource(b'No such resource')
>  
>      def connectToBackend(self, client_factory):
> 
> === modified file 'turnip/pack/tests/test_functional.py'
> --- turnip/pack/tests/test_functional.py	2015-04-26 16:08:24 +0000
> +++ turnip/pack/tests/test_functional.py	2015-05-22 15:44:58 +0000
> @@ -343,7 +343,11 @@
>          # virtinfo servers started by the mixin.
>          frontend_site = server.Site(
>              SmartHTTPFrontendResource(
> -                b'localhost', self.virt_port, self.virtinfo_url, self.root))
> +                b'localhost', {
> +                    "pack_virt_port": self.virt_port,
> +                    "virtinfo_endpoint": self.virtinfo_url,
> +                    "repo_store": self.root,
> +                    }))
>          self.frontend_listener = reactor.listenTCP(0, frontend_site)
>          self.port = self.frontend_listener.getHost().port
>  
> 
> === modified file 'turnipserver.py'
> --- turnipserver.py	2015-04-07 05:47:48 +0000
> +++ turnipserver.py	2015-05-22 15:44:58 +0000
> @@ -31,9 +31,6 @@
>  PACK_BACKEND_PORT = config.get('pack_backend_port')
>  REPO_STORE = config.get('repo_store')
>  VIRTINFO_ENDPOINT = config.get('virtinfo_endpoint')
> -CGIT_EXEC_PATH = config.get('cgit_exec_path')
> -CGIT_DATA_PATH = config.get('cgit_data_path')
> -SITE_NAME = config.get('site_name')
>  
>  # turnipserver.py is preserved for convenience in development, services
>  # in production are run in separate processes.
> @@ -58,11 +55,7 @@
>  reactor.listenTCP(config.get('pack_frontend_port'),
>                    PackFrontendFactory('localhost',
>                                        PACK_VIRT_PORT))
> -smarthttp_site = server.Site(
> -    SmartHTTPFrontendResource(
> -        b'localhost', PACK_VIRT_PORT, VIRTINFO_ENDPOINT, REPO_STORE,
> -        cgit_exec_path=CGIT_EXEC_PATH, cgit_data_path=CGIT_DATA_PATH,
> -        site_name=SITE_NAME))
> +smarthttp_site = server.Site(SmartHTTPFrontendResource(b'localhost', config))
>  reactor.listenTCP(config.get('smart_http_port'), smarthttp_site)
>  smartssh_service = SmartSSHService(
>      b'localhost', PACK_VIRT_PORT, config.get('authentication_endpoint'),
> 


-- 
https://code.launchpad.net/~cjwatson/turnip/cgit-openid/+merge/259948
Your team Launchpad code reviewers is subscribed to branch lp:turnip.