deprecating min_verify_time option; doesn't meaningfully increase security, and adds code complexity

5 files changed, 57 insertions, 30 deletions

diff --git a/CHANGES b/CHANGES
index 2b94b98..d0db245 100644
--- a/CHANGES
+++ b/CHANGES
@@ -17,7 +17,7 @@ Release History
 
         * Fixed rare ``'NoneType' object has no attribute 'decode'`` error
           that sometimes occurred on platforms with a deviant implementation
-          of :func:`!os_crypt`. 
+          of :func:`!os_crypt`.
 
     CryptContext
 
@@ -63,6 +63,10 @@ Release History
         * deprecated some unused functions in :mod:`!passlib.utils`,
           they will be removed in release 1.7.
 
+        * The :class:`!CryptContext` option
+          :ref:`min_verify_time <min-verify-time>` has been deprecated,
+          will be ignored in release 1.7, and will be removed in release 1.8.
+
     Other
 
         * Passlib is now source-compatible with Python 2.5+ and Python 3,
diff --git a/docs/lib/passlib.context-options.rst b/docs/lib/passlib.context-options.rst
index 745e7cf..042b64a 100644
--- a/docs/lib/passlib.context-options.rst
+++ b/docs/lib/passlib.context-options.rst
@@ -47,8 +47,9 @@ of the :class:`!CryptContext` instance itself:
     For use in INI files, this may also be specified as a single comma-separated string
     of handler names.
 
-    This is primarily used by :meth:`CryptContext.hash_needs_update` and :meth:`CryptPolicy.handler_is_deprecated`.
-    If the application does not use these methods, this option can be ignored.
+    This is primarily used by :meth:`CryptContext.hash_needs_update` and
+    :meth:`CryptPolicy.handler_is_deprecated`. If the application does not use
+    these methods, this option can be ignored.
 
     Example: ``deprecated=["des_crypt"]``.
 
@@ -60,16 +61,18 @@ of the :class:`!CryptContext` instance itself:
 
     Example: ``default="sha256_crypt"``.
 
-``min_verify_time``
+.. _min-verify-time:
 
-    If specified, all :meth:`CryptContext.verify` calls will take at least this many seconds.
-    If set to an amount larger than the time used by the strongest hash in the system,
-    this prevents an attacker from guessing the strength of particular hashes through timing measurements.
+``min_verify_time``
 
-    Specified in integer or fractional seconds.
+    If specified, unsuccessful :meth:`CryptContext.verify` calls will take at
+    least this many seconds. Specified in integer or fractional seconds.
 
     Example: ``min_verify_time=0.1``.
 
+    .. deprecated:: 1.6 this option is not very useful, and will be removed
+                    in version 1.8.
+
 .. note::
 
     For symmetry with the format of the hash option keywords (below),
@@ -233,7 +236,6 @@ A sample policy file:
     schemes = md5_crypt, sha512_crypt, bcrypt
     deprecated = md5_crypt
     default = sha512_crypt
-    min_verify_time = 0.1
 
     #set some common options for all schemes
     all.vary_rounds = 10%%
@@ -255,7 +257,6 @@ And the equivalent as a set of python keyword options::
         schemes = ["md5_crypt", "sha512_crypt", "bcrypt" ],
         deprecated = ["md5_crypt"],
         default = "sha512_crypt",
-        min_verify_time = 0.1,
 
         #set some common options for all schemes
         all__vary_rounds = "10%",
diff --git a/docs/lib/passlib.context-usage.rst b/docs/lib/passlib.context-usage.rst
index 3df3520..9832203 100644
--- a/docs/lib/passlib.context-usage.rst
+++ b/docs/lib/passlib.context-usage.rst
@@ -130,9 +130,6 @@ applications with advanced policy requirements may want to create a hash policy
     ;   (existing md5_crypt hashes will be flagged as needs-updating)
     deprecated = md5_crypt
 
-    ;set verify to always take at least 1/10th of a second
-    min_verify_time = 0.1
-
     ;set boundaries for pbkdf2 rounds parameter
     ;   (pbkdf2 hashes outside this range will be flagged as needs-updating)
     pbkdf2_sha1.min_rounds = 10000
diff --git a/passlib/context.py b/passlib/context.py
index da9e1dd..2e99eee 100644
--- a/passlib/context.py
+++ b/passlib/context.py
@@ -421,6 +421,8 @@ class CryptPolicy(object):
                                    "in policy: %r" % (scheme,))
 
         elif key == "min_verify_time":
+            warn("'min_verify_time' is deprecated as of Passlib 1.6, will be "
+                 "ignored in 1.7, and removed in 1.8.", DeprecationWarning)
             value = float(value)
             if value < 0:
                 raise ValueError("'min_verify_time' must be >= 0")
@@ -582,7 +584,8 @@ class CryptPolicy(object):
         return bool(kwds.get("deprecated"))
 
     def get_min_verify_time(self, category=None):
-        # XXX: deprecate this function ?
+        warn("get_min_verify_time is deprecated, and will be removed in "
+             "Passlib 1.8", DeprecationWarning)
         kwds = self._get_handler_options("all", category)[0]
         return kwds.get("min_verify_time") or 0
 
@@ -1031,19 +1034,22 @@ class _CryptRecord(object):
     def verify(self, secret, hash, **context):
         "verify helper - adds min_verify_time delay"
         mvt = self._min_verify_time
-        assert mvt
+        assert mvt > 0
         start = tick()
         ok = self.handler.verify(secret, hash, **context)
+        if ok:
+            return True
         end = tick()
         delta = mvt + start - end
         if delta > 0:
             sleep(delta)
         elif delta < 0:
-            #warn app they aren't being protected against timing attacks...
+            #warn app they exceeded bounds (this might reveal
+            #relative costs of different hashes if under migration)
             warn("CryptContext: verify exceeded min_verify_time: "
                  "scheme=%r min_verify_time=%r elapsed=%r" %
                  (self.scheme, mvt, end-start), PasslibContextWarning)
-        return ok
+        return False
 
     #================================================================
     # hash_needs_update()
diff --git a/passlib/tests/test_context.py b/passlib/tests/test_context.py
index 72ee39b..a7d4caf 100644
--- a/passlib/tests/test_context.py
+++ b/passlib/tests/test_context.py
@@ -466,6 +466,13 @@ admin__context__deprecated = des_crypt, bsdi_crypt
 
     def test_15_min_verify_time(self):
         "test get_min_verify_time() method"
+        # silence deprecation warnings for min verify time
+        with catch_warnings():
+            warnings.filterwarnings("ignore", category=DeprecationWarning)
+            self._test_15()
+
+    def _test_15(self):
+
         pa = CryptPolicy()
         self.assertEqual(pa.get_min_verify_time(), 0)
         self.assertEqual(pa.get_min_verify_time('admin'), 0)
@@ -920,10 +927,10 @@ class CryptContextTest(TestCase):
         "test verify() honors min_verify_time"
         #NOTE: this whole test assumes time.sleep() and tick()
         #      have better than 100ms accuracy - set via delta.
-        delta = .1
-        min_delay = delta
-        min_verify_time = min_delay + 2*delta
-        max_delay = min_verify_time + 2*delta
+        delta = .05
+        min_delay = 2*delta
+        min_verify_time = 5*delta
+        max_delay = 8*delta
 
         class TimedHash(uh.StaticHandler):
             "psuedo hash that takes specified amount of time"
@@ -937,9 +944,14 @@ class CryptContextTest(TestCase):
             @classmethod
             def genhash(cls, secret, hash):
                 time.sleep(cls.delay)
-                return hash or 'x'
+                return secret + 'x'
 
-        cc = CryptContext([TimedHash], min_verify_time=min_verify_time)
+        # silence deprecation warnings for min verify time
+        with catch_warnings(record=True) as wlog:
+            warnings.filterwarnings("always", category=DeprecationWarning)
+            cc = CryptContext([TimedHash], min_verify_time=min_verify_time)
+        self.assertWarningMatches(wlog.pop(0), category=DeprecationWarning)
+        self.assertFalse(wlog)
 
         def timecall(func, *args, **kwds):
             start = tick()
@@ -947,24 +959,31 @@ class CryptContextTest(TestCase):
             end = tick()
             return end-start, result
 
-        #verify hashing works
+        #verify genhash delay works
         TimedHash.delay = min_delay
-        elapsed, _ = timecall(TimedHash.genhash, 'stub', 'stub')
+        elapsed, result = timecall(TimedHash.genhash, 'stub', None)
+        self.assertEqual(result, 'stubx')
         self.assertAlmostEqual(elapsed, min_delay, delta=delta)
 
         #ensure min verify time is honored
-        elapsed, _ = timecall(cc.verify, "stub", "stub")
+        elapsed, result = timecall(cc.verify, "stub", "stubx")
+        self.assertTrue(result)
+        self.assertAlmostEqual(elapsed, min_delay, delta=delta)
+
+        elapsed, result = timecall(cc.verify, "blob", "stubx")
+        self.assertFalse(result)
         self.assertAlmostEqual(elapsed, min_verify_time, delta=delta)
 
         #ensure taking longer emits a warning.
         TimedHash.delay = max_delay
         with catch_warnings(record=True) as wlog:
-            warnings.simplefilter("always")
-            elapsed, _ = timecall(cc.verify, "stub", "stub")
+            warnings.filterwarnings("always")
+            elapsed, result = timecall(cc.verify, "blob", "stubx")
+        self.assertFalse(result)
         self.assertAlmostEqual(elapsed, max_delay, delta=delta)
-        self.assertEqual(len(wlog), 1)
-        self.assertWarningMatches(wlog[0],
+        self.assertWarningMatches(wlog.pop(0),
             message_re="CryptContext: verify exceeded min_verify_time")
+        self.assertFalse(wlog)
 
     def test_25_verify_and_update(self):
         "test verify_and_update()"