Merge pull request #883 from reaperhulk/deprecation-dance

move salt_length from MGF1 to PSS and start deprecation cycle

8 files changed, 183 insertions, 78 deletions

diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index 34c544d9..9e89e563 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -6,6 +6,11 @@ Changelog
 
 .. note:: This version is not yet released and is under active development.
 
+* Deprecated ``salt_length`` on
+  :class:`~cryptography.hazmat.primitives.asymmetric.padding.MGF1` and added it
+  to :class:`~cryptography.hazmat.primitives.asymmetric.padding.PSS`. It will be
+  removed from ``MGF1`` in two releases per our :doc:`/api-stability` policy.
+
 0.3 - 2014-03-27
 ~~~~~~~~~~~~~~~~
 
diff --git a/cryptography/hazmat/backends/openssl/backend.py b/cryptography/hazmat/backends/openssl/backend.py
index 3293741c..0c632aee 100644
--- a/cryptography/hazmat/backends/openssl/backend.py
+++ b/cryptography/hazmat/backends/openssl/backend.py
@@ -701,15 +701,20 @@ class _HMACContext(object):
         return self._backend._ffi.buffer(buf)[:outlen[0]]
 
 
-def _get_rsa_pss_salt_length(mgf, key_size, digest_size):
-    if mgf._salt_length is MGF1.MAX_LENGTH:
+def _get_rsa_pss_salt_length(pss, key_size, digest_size):
+    if pss._mgf._salt_length is not None:
+        salt = pss._mgf._salt_length
+    else:
+        salt = pss._salt_length
+
+    if salt is MGF1.MAX_LENGTH or salt is PSS.MAX_LENGTH:
         # bit length - 1 per RFC 3447
         emlen = int(math.ceil((key_size - 1) / 8.0))
         salt_length = emlen - digest_size - 2
         assert salt_length >= 0
         return salt_length
     else:
-        return mgf._salt_length
+        return salt
 
 
 @utils.register_interface(interfaces.AsymmetricSignatureContext)
@@ -803,7 +808,7 @@ class _RSASignatureContext(object):
             res = self._backend._lib.EVP_PKEY_CTX_set_rsa_pss_saltlen(
                 pkey_ctx,
                 _get_rsa_pss_salt_length(
-                    self._padding._mgf,
+                    self._padding,
                     self._private_key.key_size,
                     self._hash_ctx.algorithm.digest_size
                 )
@@ -871,7 +876,7 @@ class _RSASignatureContext(object):
             data_to_sign,
             evp_md,
             _get_rsa_pss_salt_length(
-                self._padding._mgf,
+                self._padding,
                 self._private_key.key_size,
                 len(data_to_sign)
             )
@@ -988,7 +993,7 @@ class _RSAVerificationContext(object):
             res = self._backend._lib.EVP_PKEY_CTX_set_rsa_pss_saltlen(
                 pkey_ctx,
                 _get_rsa_pss_salt_length(
-                    self._padding._mgf,
+                    self._padding,
                     self._public_key.key_size,
                     self._hash_ctx.algorithm.digest_size
                 )
@@ -1068,7 +1073,7 @@ class _RSAVerificationContext(object):
             evp_md,
             buf,
             _get_rsa_pss_salt_length(
-                self._padding._mgf,
+                self._padding,
                 self._public_key.key_size,
                 len(data_to_verify)
             )
diff --git a/cryptography/hazmat/primitives/asymmetric/padding.py b/cryptography/hazmat/primitives/asymmetric/padding.py
index 02aff280..72806a61 100644
--- a/cryptography/hazmat/primitives/asymmetric/padding.py
+++ b/cryptography/hazmat/primitives/asymmetric/padding.py
@@ -13,6 +13,8 @@
 
 from __future__ import absolute_import, division, print_function
 
+import warnings
+
 import six
 
 from cryptography import utils
@@ -26,26 +28,52 @@ class PKCS1v15(object):
 
 @utils.register_interface(interfaces.AsymmetricPadding)
 class PSS(object):
+    MAX_LENGTH = object()
     name = "EMSA-PSS"
 
-    def __init__(self, mgf):
+    def __init__(self, mgf, salt_length=None):
         self._mgf = mgf
 
+        if salt_length is None:
+            warnings.warn(
+                "salt_length is deprecated on MGF1 and should be added via the"
+                " PSS constructor.",
+                utils.DeprecatedIn04
+            )
+        else:
+            if (not isinstance(salt_length, six.integer_types) and
+                    salt_length is not self.MAX_LENGTH):
+                raise TypeError("salt_length must be an integer")
+
+            if salt_length is not self.MAX_LENGTH and salt_length < 0:
+                raise ValueError("salt_length must be zero or greater")
+
+        if salt_length is None and self._mgf._salt_length is None:
+            raise ValueError("You must supply salt_length")
+
+        self._salt_length = salt_length
+
 
 class MGF1(object):
     MAX_LENGTH = object()
 
-    def __init__(self, algorithm, salt_length):
+    def __init__(self, algorithm, salt_length=None):
         if not isinstance(algorithm, interfaces.HashAlgorithm):
             raise TypeError("Expected instance of interfaces.HashAlgorithm.")
 
         self._algorithm = algorithm
 
-        if (not isinstance(salt_length, six.integer_types) and
-                salt_length is not self.MAX_LENGTH):
-            raise TypeError("salt_length must be an integer")
+        if salt_length is not None:
+            warnings.warn(
+                "salt_length is deprecated on MGF1 and should be passed to "
+                "the PSS constructor instead.",
+                utils.DeprecatedIn04
+            )
+            if (not isinstance(salt_length, six.integer_types) and
+                    salt_length is not self.MAX_LENGTH):
+                raise TypeError("salt_length must be an integer")
 
-        if salt_length is not self.MAX_LENGTH and salt_length < 0:
-            raise ValueError("salt_length must be zero or greater")
+            if salt_length is not self.MAX_LENGTH and salt_length < 0:
+                raise ValueError("salt_length must be zero or greater")
 
         self._salt_length = salt_length
diff --git a/cryptography/utils.py b/cryptography/utils.py
index eac833b6..5566d123 100644
--- a/cryptography/utils.py
+++ b/cryptography/utils.py
@@ -16,6 +16,9 @@ from __future__ import absolute_import, division, print_function
 import sys
 
 
+DeprecatedIn04 = PendingDeprecationWarning
+
+
 def register_interface(iface):
     def register_decorator(klass):
         iface.register(klass)
diff --git a/docs/doing-a-release.rst b/docs/doing-a-release.rst
index 5571e0f3..ad3b4791 100644
--- a/docs/doing-a-release.rst
+++ b/docs/doing-a-release.rst
@@ -49,3 +49,6 @@ Post-release tasks
 * Add new :doc:`/changelog` entry with next version and note that it is under
   active development
 * Send a pull request with these items
+* Check for any outstanding code undergoing a deprecation cycle by looking in
+  ``cryptography.utils`` for ``DeprecatedIn**`` definitions. If any exist open
+  a ticket to increment them for the next release.
diff --git a/docs/hazmat/primitives/asymmetric/padding.rst b/docs/hazmat/primitives/asymmetric/padding.rst
index 2a5de3c7..89af7eaa 100644
--- a/docs/hazmat/primitives/asymmetric/padding.rst
+++ b/docs/hazmat/primitives/asymmetric/padding.rst
@@ -10,10 +10,13 @@ Padding
     correct padding signatures can be forged, messages decrypted, and private
     keys compromised.
 
-.. class:: PSS(mgf)
+.. class:: PSS(mgf, salt_length)
 
     .. versionadded:: 0.3
 
+    .. versionchanged:: 0.4
+        Added ``salt_length`` parameter.
+
     PSS (Probabilistic Signature Scheme) is a signature scheme defined in
     :rfc:`3447`. It is more complex than PKCS1 but possesses a `security proof`_.
     This is the `recommended padding algorithm`_ for RSA signatures.
@@ -21,6 +24,14 @@ Padding
     :param mgf: A mask generation function object. At this time the only
         supported MGF is :class:`MGF1`.
 
+    :param int salt_length: The length of the salt. It is recommended that this
+        be set to ``PSS.MAX_LENGTH``.
+
+    .. attribute:: MAX_LENGTH
+
+        Pass this attribute to ``salt_length`` to get the maximum salt length
+        available.
+
 .. class:: PKCS1v15()
 
     .. versionadded:: 0.3
@@ -31,10 +42,13 @@ Padding
 Mask generation functions
 ~~~~~~~~~~~~~~~~~~~~~~~~~
 
-.. class:: MGF1(algorithm, salt_length)
+.. class:: MGF1(algorithm)
 
     .. versionadded:: 0.3
 
+    .. versionchanged:: 0.4
+        Deprecated the ``salt_length`` parameter.
+
     MGF1 (Mask Generation Function 1) is used as the mask generation function
     in :class:`PSS` padding. It takes a hash algorithm and a salt length.
 
@@ -42,14 +56,6 @@ Mask generation functions
         :class:`~cryptography.hazmat.primitives.interfaces.HashAlgorithm`
         provider.
 
-    :param int salt_length: The length of the salt. It is recommended that this
-        be set to ``MGF1.MAX_LENGTH``.
-
-    .. attribute:: MAX_LENGTH
-
-        Pass this attribute to ``salt_length`` to get the maximum salt length
-        available.
-
 
 .. _`Padding is critical`: http://rdist.root.org/2009/10/06/why-rsa-encryption-padding-is-critical/
 .. _`security proof`: http://eprint.iacr.org/2001/062.pdf
diff --git a/docs/hazmat/primitives/asymmetric/rsa.rst b/docs/hazmat/primitives/asymmetric/rsa.rst
index 5c554abc..5074f1c5 100644
--- a/docs/hazmat/primitives/asymmetric/rsa.rst
+++ b/docs/hazmat/primitives/asymmetric/rsa.rst
@@ -73,10 +73,8 @@ RSA
             ... )
             >>> signer = private_key.signer(
             ...     padding.PSS(
-            ...         mgf=padding.MGF1(
-            ...             algorithm=hashes.SHA256(),
-            ...             salt_length=padding.MGF1.MAX_LENGTH
-            ...         )
+            ...         mgf=padding.MGF1(hashes.SHA256()),
+            ...         salt_length=padding.PSS.MAX_LENGTH
             ...     ),
             ...     hashes.SHA256(),
             ...     default_backend()
@@ -158,10 +156,8 @@ RSA
             ... )
             >>> signer = private_key.signer(
             ...     padding.PSS(
-            ...         mgf=padding.MGF1(
-            ...             algorithm=hashes.SHA256(),
-            ...             salt_length=padding.MGF1.MAX_LENGTH
-            ...         )
+            ...         mgf=padding.MGF1(hashes.SHA256()),
+            ...         salt_length=padding.PSS.MAX_LENGTH
             ...     ),
             ...     hashes.SHA256(),
             ...     default_backend()
@@ -173,10 +169,8 @@ RSA
             >>> verifier = public_key.verifier(
             ...     signature,
             ...     padding.PSS(
-            ...         mgf=padding.MGF1(
-            ...             algorithm=hashes.SHA256(),
-            ...             salt_length=padding.MGF1.MAX_LENGTH
-            ...         )
+            ...         mgf=padding.MGF1(hashes.SHA256()),
+            ...         salt_length=padding.PSS.MAX_LENGTH
             ...     ),
             ...     hashes.SHA256(),
             ...     default_backend()
diff --git a/tests/hazmat/primitives/test_rsa.py b/tests/hazmat/primitives/test_rsa.py
index c458a662..cc87d981 100644
--- a/tests/hazmat/primitives/test_rsa.py
+++ b/tests/hazmat/primitives/test_rsa.py
@@ -39,7 +39,7 @@ class DummyPadding(object):
 
 
 class DummyMGF(object):
-    pass
+    _salt_length = 0
 
 
 def _modinv(e, m):
@@ -454,10 +454,8 @@ class TestRSASignature(object):
         )
         signer = private_key.signer(
             padding.PSS(
-                mgf=padding.MGF1(
-                    algorithm=hashes.SHA1(),
-                    salt_length=padding.MGF1.MAX_LENGTH
-                )
+                mgf=padding.MGF1(algorithm=hashes.SHA1()),
+                salt_length=padding.PSS.MAX_LENGTH
             ),
             hashes.SHA1(),
             backend
@@ -471,6 +469,23 @@ class TestRSASignature(object):
         verifier = public_key.verifier(
             signature,
             padding.PSS(
+                mgf=padding.MGF1(algorithm=hashes.SHA1()),
+                salt_length=padding.PSS.MAX_LENGTH
+            ),
+            hashes.SHA1(),
+            backend
+        )
+        verifier.update(binascii.unhexlify(example["message"]))
+        verifier.verify()
+
+    def test_deprecated_pss_mgf1_salt_length(self, backend):
+        private_key = rsa.RSAPrivateKey.generate(
+            public_exponent=65537,
+            key_size=512,
+            backend=backend
+        )
+        signer = private_key.signer(
+            padding.PSS(
                 mgf=padding.MGF1(
                     algorithm=hashes.SHA1(),
                     salt_length=padding.MGF1.MAX_LENGTH
@@ -479,7 +494,21 @@ class TestRSASignature(object):
             hashes.SHA1(),
             backend
         )
-        verifier.update(binascii.unhexlify(example["message"]))
+        signer.update(b"so deprecated")
+        signature = signer.finalize()
+        assert len(signature) == math.ceil(private_key.key_size / 8.0)
+        verifier = private_key.public_key().verifier(
+            signature,
+            padding.PSS(
+                mgf=padding.MGF1(
+                    algorithm=hashes.SHA1(),
+                    salt_length=padding.MGF1.MAX_LENGTH
+                )
+            ),
+            hashes.SHA1(),
+            backend
+        )
+        verifier.update(b"so deprecated")
         verifier.verify()
 
     @pytest.mark.parametrize(
@@ -498,10 +527,8 @@ class TestRSASignature(object):
         )
         public_key = private_key.public_key()
         pss = padding.PSS(
-            mgf=padding.MGF1(
-                algorithm=hash_alg,
-                salt_length=padding.MGF1.MAX_LENGTH
-            )
+            mgf=padding.MGF1(hash_alg),
+            salt_length=padding.PSS.MAX_LENGTH
         )
         signer = private_key.signer(
             pss,
@@ -531,10 +558,8 @@ class TestRSASignature(object):
         )
         signer = private_key.signer(
             padding.PSS(
-                mgf=padding.MGF1(
-                    algorithm=hashes.SHA1(),
-                    salt_length=padding.MGF1.MAX_LENGTH
-                )
+                mgf=padding.MGF1(hashes.SHA1()),
+                salt_length=padding.PSS.MAX_LENGTH
             ),
             hashes.SHA512(),
             backend
@@ -555,10 +580,8 @@ class TestRSASignature(object):
         with pytest.raises(ValueError):
             private_key.signer(
                 padding.PSS(
-                    mgf=padding.MGF1(
-                        algorithm=hashes.SHA1(),
-                        salt_length=padding.MGF1.MAX_LENGTH
-                    )
+                    mgf=padding.MGF1(hashes.SHA1()),
+                    salt_length=padding.PSS.MAX_LENGTH
                 ),
                 hashes.SHA512(),
                 backend
@@ -572,10 +595,8 @@ class TestRSASignature(object):
         )
         signer = private_key.signer(
             padding.PSS(
-                mgf=padding.MGF1(
-                    algorithm=hashes.SHA1(),
-                    salt_length=1000000
-                )
+                mgf=padding.MGF1(hashes.SHA1()),
+                salt_length=1000000
             ),
             hashes.SHA1(),
             backend
@@ -722,10 +743,8 @@ class TestRSAVerification(object):
         verifier = public_key.verifier(
             binascii.unhexlify(example["signature"]),
             padding.PSS(
-                mgf=padding.MGF1(
-                    algorithm=hashes.SHA1(),
-                    salt_length=20
-                )
+                mgf=padding.MGF1(algorithm=hashes.SHA1()),
+                salt_length=20
             ),
             hashes.SHA1(),
             backend
@@ -749,10 +768,8 @@ class TestRSAVerification(object):
         verifier = public_key.verifier(
             signature,
             padding.PSS(
-                mgf=padding.MGF1(
-                    algorithm=hashes.SHA1(),
-                    salt_length=padding.MGF1.MAX_LENGTH
-                )
+                mgf=padding.MGF1(algorithm=hashes.SHA1()),
+                salt_length=padding.PSS.MAX_LENGTH
             ),
             hashes.SHA1(),
             backend
@@ -779,10 +796,8 @@ class TestRSAVerification(object):
         verifier = public_key.verifier(
             signature,
             padding.PSS(
-                mgf=padding.MGF1(
-                    algorithm=hashes.SHA1(),
-                    salt_length=padding.MGF1.MAX_LENGTH
-                )
+                mgf=padding.MGF1(algorithm=hashes.SHA1()),
+                salt_length=padding.PSS.MAX_LENGTH
             ),
             hashes.SHA1(),
             backend
@@ -809,10 +824,8 @@ class TestRSAVerification(object):
         verifier = public_key.verifier(
             signature,
             padding.PSS(
-                mgf=padding.MGF1(
-                    algorithm=hashes.SHA1(),
-                    salt_length=padding.MGF1.MAX_LENGTH
-                )
+                mgf=padding.MGF1(algorithm=hashes.SHA1()),
+                salt_length=padding.PSS.MAX_LENGTH
             ),
             hashes.SHA1(),
             backend
@@ -904,10 +917,8 @@ class TestRSAVerification(object):
             public_key.verifier(
                 signature,
                 padding.PSS(
-                    mgf=padding.MGF1(
-                        algorithm=hashes.SHA1(),
-                        salt_length=padding.MGF1.MAX_LENGTH
-                    )
+                    mgf=padding.MGF1(algorithm=hashes.SHA1()),
+                    salt_length=padding.PSS.MAX_LENGTH
                 ),
                 hashes.SHA512(),
                 backend
@@ -1113,7 +1124,57 @@ class TestRSAPKCS1Verification(object):
     ))
 
 
+class TestPSS(object):
+    def test_deprecation_warning(self):
+        pytest.deprecated_call(
+            padding.PSS,
+            mgf=padding.MGF1(hashes.SHA1(), 20)
+        )
+
+    def test_invalid_salt_length_not_integer(self):
+        with pytest.raises(TypeError):
+            padding.PSS(
+                mgf=padding.MGF1(
+                    hashes.SHA1()
+                ),
+                salt_length=b"not_a_length"
+            )
+
+    def test_invalid_salt_length_negative_integer(self):
+        with pytest.raises(ValueError):
+            padding.PSS(
+                mgf=padding.MGF1(
+                    hashes.SHA1()
+                ),
+                salt_length=-1
+            )
+
+    def test_no_salt_length_supplied_pss_or_mgf1(self):
+        with pytest.raises(ValueError):
+            padding.PSS(mgf=padding.MGF1(hashes.SHA1()))
+
+    def test_valid_pss_parameters(self):
+        algorithm = hashes.SHA1()
+        salt_length = algorithm.digest_size
+        mgf = padding.MGF1(algorithm)
+        pss = padding.PSS(mgf=mgf, salt_length=salt_length)
+        assert pss._mgf == mgf
+        assert pss._salt_length == salt_length
+
+    def test_valid_pss_parameters_maximum(self):
+        algorithm = hashes.SHA1()
+        mgf = padding.MGF1(algorithm)
+        pss = padding.PSS(mgf=mgf, salt_length=padding.PSS.MAX_LENGTH)
+        assert pss._mgf == mgf
+        assert pss._salt_length == padding.PSS.MAX_LENGTH
+
+
 class TestMGF1(object):
+    def test_deprecation_warning(self):
+        pytest.deprecated_call(
+            padding.MGF1, algorithm=hashes.SHA1(), salt_length=20
+        )
+
     def test_invalid_hash_algorithm(self):
         with pytest.raises(TypeError):
             padding.MGF1(b"not_a_hash", 0)