Repository: scrapy/scrapy
Pull request #3550 (2018-12-26 15:35:59)

Backport security fix #3415
Backport #3415 to 1.5 branch.

codecov[bot]: # [Codecov](https://codecov.io/gh/scrapy/scrapy/pull/3550?src=pr&el=h1) Report
> Merging [#3550](https://codecov.io/gh/scrapy/scrapy/pull/3550?src=pr&el=desc) into [1.5](https://codecov.io/gh/scrapy/scrapy/commit/50caaf5b4235a47c04ec26d2cedf32413ef89559?src=pr&el=desc) will **decrease** coverage by `0.81%`.
> The diff coverage is `94.11%`.


```diff
@@            Coverage Diff             @@
##              1.5    #3550      +/-   ##
==========================================
- Coverage   82.08%   81.27%   -0.82%     
==========================================
  Files         228      228              
  Lines        9574     9589      +15     
  Branches     1382     1335      -47     
==========================================
- Hits         7859     7793      -66     
- Misses       1456     1521      +65     
- Partials      259      275      +16
```

| [Impacted Files](https://codecov.io/gh/scrapy/scrapy/pull/3550?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [scrapy/settings/default\_settings.py](https://codecov.io/gh/scrapy/scrapy/pull/3550/diff?src=pr&el=tree#diff-c2NyYXB5L3NldHRpbmdzL2RlZmF1bHRfc2V0dGluZ3MucHk=) | `98.62% <100%> (+0.01%)` | :arrow_up: |
| [scrapy/extensions/telnet.py](https://codecov.io/gh/scrapy/scrapy/pull/3550/diff?src=pr&el=tree#diff-c2NyYXB5L2V4dGVuc2lvbnMvdGVsbmV0LnB5) | `81.96% <93.33%> (+6.96%)` | :arrow_up: |
| [scrapy/core/downloader/handlers/s3.py](https://codecov.io/gh/scrapy/scrapy/pull/3550/diff?src=pr&el=tree#diff-c2NyYXB5L2NvcmUvZG93bmxvYWRlci9oYW5kbGVycy9zMy5weQ==) | `62.9% <0%> (-32.26%)` | :arrow_down: |
| [scrapy/utils/boto.py](https://codecov.io/gh/scrapy/scrapy/pull/3550/diff?src=pr&el=tree#diff-c2NyYXB5L3V0aWxzL2JvdG8ucHk=) | `46.66% <0%> (-26.67%)` | :arrow_down: |
| [scrapy/utils/gz.py](https://codecov.io/gh/scrapy/scrapy/pull/3550/diff?src=pr&el=tree#diff-c2NyYXB5L3V0aWxzL2d6LnB5) | `86.11% <0%> (-13.89%)` | :arrow_down: |
| [scrapy/link.py](https://codecov.io/gh/scrapy/scrapy/pull/3550/diff?src=pr&el=tree#diff-c2NyYXB5L2xpbmsucHk=) | `86.36% <0%> (-13.64%)` | :arrow_down: |
| [scrapy/core/downloader/tls.py](https://codecov.io/gh/scrapy/scrapy/pull/3550/diff?src=pr&el=tree#diff-c2NyYXB5L2NvcmUvZG93bmxvYWRlci90bHMucHk=) | `77.5% <0%> (-12.5%)` | :arrow_down: |
| [scrapy/\_monkeypatches.py](https://codecov.io/gh/scrapy/scrapy/pull/3550/diff?src=pr&el=tree#diff-c2NyYXB5L19tb25rZXlwYXRjaGVzLnB5) | `50% <0%> (-7.15%)` | :arrow_down: |
| [scrapy/mail.py](https://codecov.io/gh/scrapy/scrapy/pull/3550/diff?src=pr&el=tree#diff-c2NyYXB5L21haWwucHk=) | `70.23% <0%> (-5.96%)` | :arrow_down: |
| [scrapy/utils/iterators.py](https://codecov.io/gh/scrapy/scrapy/pull/3550/diff?src=pr&el=tree#diff-c2NyYXB5L3V0aWxzL2l0ZXJhdG9ycy5weQ==) | `95.45% <0%> (-4.55%)` | :arrow_down: |
| ... and [12 more](https://codecov.io/gh/scrapy/scrapy/pull/3550/diff?src=pr&el=tree-more) | |

codecov[bot]: # [Codecov](https://codecov.io/gh/scrapy/scrapy/pull/3550?src=pr&el=h1) Report
> Merging [#3550](https://codecov.io/gh/scrapy/scrapy/pull/3550?src=pr&el=desc) into [1.5](https://codecov.io/gh/scrapy/scrapy/commit/50caaf5b4235a47c04ec26d2cedf32413ef89559?src=pr&el=desc) will **decrease** coverage by `0.81%`.
> The diff coverage is `94.11%`.


```diff
@@            Coverage Diff             @@
##              1.5    #3550      +/-   ##
==========================================
- Coverage   82.08%   81.27%   -0.82%     
==========================================
  Files         228      228              
  Lines        9574     9589      +15     
  Branches     1382     1335      -47     
==========================================
- Hits         7859     7793      -66     
- Misses       1456     1521      +65     
- Partials      259      275      +16
```

| [Impacted Files](https://codecov.io/gh/scrapy/scrapy/pull/3550?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [scrapy/settings/default\_settings.py](https://codecov.io/gh/scrapy/scrapy/pull/3550/diff?src=pr&el=tree#diff-c2NyYXB5L3NldHRpbmdzL2RlZmF1bHRfc2V0dGluZ3MucHk=) | `98.62% <100%> (+0.01%)` | :arrow_up: |
| [scrapy/extensions/telnet.py](https://codecov.io/gh/scrapy/scrapy/pull/3550/diff?src=pr&el=tree#diff-c2NyYXB5L2V4dGVuc2lvbnMvdGVsbmV0LnB5) | `81.96% <93.33%> (+6.96%)` | :arrow_up: |
| [scrapy/core/downloader/handlers/s3.py](https://codecov.io/gh/scrapy/scrapy/pull/3550/diff?src=pr&el=tree#diff-c2NyYXB5L2NvcmUvZG93bmxvYWRlci9oYW5kbGVycy9zMy5weQ==) | `62.9% <0%> (-32.26%)` | :arrow_down: |
| [scrapy/utils/boto.py](https://codecov.io/gh/scrapy/scrapy/pull/3550/diff?src=pr&el=tree#diff-c2NyYXB5L3V0aWxzL2JvdG8ucHk=) | `46.66% <0%> (-26.67%)` | :arrow_down: |
| [scrapy/utils/gz.py](https://codecov.io/gh/scrapy/scrapy/pull/3550/diff?src=pr&el=tree#diff-c2NyYXB5L3V0aWxzL2d6LnB5) | `86.11% <0%> (-13.89%)` | :arrow_down: |
| [scrapy/link.py](https://codecov.io/gh/scrapy/scrapy/pull/3550/diff?src=pr&el=tree#diff-c2NyYXB5L2xpbmsucHk=) | `86.36% <0%> (-13.64%)` | :arrow_down: |
| [scrapy/core/downloader/tls.py](https://codecov.io/gh/scrapy/scrapy/pull/3550/diff?src=pr&el=tree#diff-c2NyYXB5L2NvcmUvZG93bmxvYWRlci90bHMucHk=) | `77.5% <0%> (-12.5%)` | :arrow_down: |
| [scrapy/\_monkeypatches.py](https://codecov.io/gh/scrapy/scrapy/pull/3550/diff?src=pr&el=tree#diff-c2NyYXB5L19tb25rZXlwYXRjaGVzLnB5) | `50% <0%> (-7.15%)` | :arrow_down: |
| [scrapy/mail.py](https://codecov.io/gh/scrapy/scrapy/pull/3550/diff?src=pr&el=tree#diff-c2NyYXB5L21haWwucHk=) | `70.23% <0%> (-5.96%)` | :arrow_down: |
| [scrapy/utils/iterators.py](https://codecov.io/gh/scrapy/scrapy/pull/3550/diff?src=pr&el=tree#diff-c2NyYXB5L3V0aWxzL2l0ZXJhdG9ycy5weQ==) | `95.45% <0%> (-4.55%)` | :arrow_down: |
| ... and [12 more](https://codecov.io/gh/scrapy/scrapy/pull/3550/diff?src=pr&el=tree-more) | |


Patch:
diff --git a/docs/topics/telnetconsole.rst b/docs/topics/telnetconsole.rst
index ce79c9f35..bf2ffa443 100644
--- a/docs/topics/telnetconsole.rst
+++ b/docs/topics/telnetconsole.rst
@@ -16,6 +16,17 @@ The telnet console is a :ref:`built-in Scrapy extension
 disable it if you want. For more information about the extension itself see
 :ref:`topics-extensions-ref-telnetconsole`.
 
+.. warning::
+    It is not secure to use telnet console via public networks, as telnet
+    doesn't provide any transport-layer security. Having username/password
+    authentication doesn't change that.
+
+    Intended usage is connecting to a running Scrapy spider locally
+    (spider process and telnet client are on the same machine)
+    or over a secure connection (VPN, SSH tunnel).
+    Please avoid using telnet console over insecure connections,
+    or disable it completely using :setting:`TELNETCONSOLE_ENABLED` option.
+
 .. highlight:: none
 
 How to access the telnet console
@@ -26,8 +37,26 @@ The telnet console listens in the TCP port defined in the
 the console you need to type::
 
     telnet localhost 6023
+    Trying localhost...
+    Connected to localhost.
+    Escape character is '^]'.
+    Username:
+    Password:  
     >>>
-    
+
+By default Username is ``scrapy`` and Password is autogenerated. The 
+autogenerated Password can be seen on scrapy logs like the example bellow::
+
+    2018-10-16 14:35:21 [scrapy.extensions.telnet] INFO: Telnet Password: 16f92501e8a59326
+
+Default Username and Password can be overriden by the settings 
+:setting:`TELNETCONSOLE_USERNAME` and :setting:`TELNETCONSOLE_PASSWORD`.
+
+.. warning::
+    Username and password provide only a limited protection, as telnet
+    is not using secure transport - by default traffic is not encrypted
+    even if username and password are set.
+
 You need the telnet program which comes installed by default in Windows, and
 most Linux distros.
 
@@ -160,3 +189,24 @@ Default: ``'127.0.0.1'``
 
 The interface the telnet console should listen on
 
+
+.. setting:: TELNETCONSOLE_USERNAME
+
+TELNETCONSOLE_USERNAME
+----------------------
+
+Default: ``'scrapy'``
+
+The username used for the telnet console
+
+
+.. setting:: TELNETCONSOLE_PASSWORD
+
+TELNETCONSOLE_PASSWORD
+----------------------
+
+Default: ``None``
+
+The password used for the telnet console, default behaviour is to have it
+autogenerated
+
diff --git a/scrapy/extensions/telnet.py b/scrapy/extensions/telnet.py
index 5ca0d19a0..1df8692a2 100644
--- a/scrapy/extensions/telnet.py
+++ b/scrapy/extensions/telnet.py
@@ -4,8 +4,10 @@ Scrapy Telnet Console extension
 See documentation in docs/topics/telnetconsole.rst
 """
 
+import os
 import pprint
 import logging
+import binascii
 
 from twisted.internet import protocol
 try:
@@ -20,6 +22,7 @@ from scrapy import signals
 from scrapy.utils.trackref import print_live_refs
 from scrapy.utils.engine import print_engine_status
 from scrapy.utils.reactor import listen_tcp
+from scrapy.utils.decorators import defers
 
 try:
     import guppy
@@ -45,6 +48,13 @@ class TelnetConsole(protocol.ServerFactory):
         self.noisy = False
         self.portrange = [int(x) for x in crawler.settings.getlist('TELNETCONSOLE_PORT')]
         self.host = crawler.settings['TELNETCONSOLE_HOST']
+        self.username = crawler.settings['TELNETCONSOLE_USERNAME']
+        self.password = crawler.settings['TELNETCONSOLE_PASSWORD']
+
+        if not self.password:
+            self.password = binascii.hexlify(os.urandom(8)).decode('utf8')
+            logger.info('Telnet Password: %s', self.password)
+
         self.crawler.signals.connect(self.start_listening, signals.engine_started)
         self.crawler.signals.connect(self.stop_listening, signals.engine_stopped)
 
@@ -63,9 +73,25 @@ class TelnetConsole(protocol.ServerFactory):
         self.port.stopListening()
 
     def protocol(self):
-        telnet_vars = self._get_telnet_vars()
-        return telnet.TelnetTransport(telnet.TelnetBootstrapProtocol,
-            insults.ServerProtocol, manhole.Manhole, telnet_vars)
+        class Portal:
+            """An implementation of IPortal"""
+            @defers
+            def login(self_, credentials, mind, *interfaces):
+                if not (credentials.username == self.username.encode('utf8') and
+                        credentials.checkPassword(self.password.encode('utf8'))):
+                    raise ValueError("Invalid credentials")
+
+                protocol = telnet.TelnetBootstrapProtocol(
+                    insults.ServerProtocol,
+                    manhole.Manhole,
+                    self._get_telnet_vars()
+                )
+                return (interfaces[0], protocol, lambda: None)
+
+        return telnet.TelnetTransport(
+            telnet.AuthenticatingTelnetProtocol,
+            Portal()
+        )
 
     def _get_telnet_vars(self):
         # Note: if you add entries here also update topics/telnetconsole.rst
@@ -81,8 +107,8 @@ class TelnetConsole(protocol.ServerFactory):
             'p': pprint.pprint,
             'prefs': print_live_refs,
             'hpy': hpy,
-            'help': "This is Scrapy telnet console. For more info see: " \
-                "https://doc.scrapy.org/en/latest/topics/telnetconsole.html",
+            'help': "This is Scrapy telnet console. For more info see: "
+                    "https://doc.scrapy.org/en/latest/topics/telnetconsole.html",
         }
         self.crawler.signals.send_catch_log(update_telnet_vars, telnet_vars=telnet_vars)
         return telnet_vars
diff --git a/scrapy/settings/default_settings.py b/scrapy/settings/default_settings.py
index ead511473..b264b4e25 100644
--- a/scrapy/settings/default_settings.py
+++ b/scrapy/settings/default_settings.py
@@ -275,6 +275,8 @@ USER_AGENT = 'Scrapy/%s (+https://scrapy.org)' % import_module('scrapy').__versi
 TELNETCONSOLE_ENABLED = 1
 TELNETCONSOLE_PORT = [6023, 6073]
 TELNETCONSOLE_HOST = '127.0.0.1'
+TELNETCONSOLE_USERNAME = 'scrapy'
+TELNETCONSOLE_PASSWORD = None
 
 SPIDER_CONTRACTS = {}
 SPIDER_CONTRACTS_BASE = {


Test patch:
diff --git a/tests/test_extension_telnet.py b/tests/test_extension_telnet.py
new file mode 100644
index 000000000..4f389e5cb
--- /dev/null
+++ b/tests/test_extension_telnet.py
@@ -0,0 +1,60 @@
+try:
+    import unittest.mock as mock
+except ImportError:
+    import mock
+
+from twisted.trial import unittest
+from twisted.conch.telnet import ITelnetProtocol
+from twisted.cred import credentials
+from twisted.internet import defer
+
+from scrapy.extensions.telnet import TelnetConsole, logger
+from scrapy.utils.test import get_crawler
+
+
+class TelnetExtensionTest(unittest.TestCase):
+    def _get_console_and_portal(self, settings=None):
+        crawler = get_crawler(settings_dict=settings)
+        console = TelnetConsole(crawler)
+        username = console.username
+        password = console.password
+
+        # This function has some side effects we don't need for this test
+        console._get_telnet_vars = lambda: {}
+
+        console.start_listening()
+        protocol = console.protocol()
+        portal = protocol.protocolArgs[0]
+
+        return console, portal
+
+    @defer.inlineCallbacks
+    def test_bad_credentials(self):
+        console, portal = self._get_console_and_portal()
+        creds = credentials.UsernamePassword(b'username', b'password')
+        d = portal.login(creds, None, ITelnetProtocol)
+        yield self.assertFailure(d, ValueError)
+        console.stop_listening()
+
+    @defer.inlineCallbacks
+    def test_good_credentials(self):
+        console, portal = self._get_console_and_portal()
+        creds = credentials.UsernamePassword(
+            console.username.encode('utf8'),
+            console.password.encode('utf8')
+        )
+        d = portal.login(creds, None, ITelnetProtocol)
+        yield d
+        console.stop_listening()
+
+    @defer.inlineCallbacks
+    def test_custom_credentials(self):
+        settings = {
+            'TELNETCONSOLE_USERNAME': 'user',
+            'TELNETCONSOLE_PASSWORD': 'pass',
+        }
+        console, portal = self._get_console_and_portal(settings=settings)
+        creds = credentials.UsernamePassword(b'user', b'pass')
+        d = portal.login(creds, None, ITelnetProtocol)
+        yield d
+        console.stop_listening()