backwards incompatible change to RFC822Name (#3953)

* backwards incompatible change to RFC822Name

During this release cycle we decided to officially deprecate passing
U-labels to our GeneralName constructors. At first we tried changing
this in a purely backwards compatible way but get_values_for_type made
that untenable. This PR modifies RFC822Name to accept two types:
U-label strings (which raises a deprecation warning) and A-label strings
(the new preferred type). There is also a constructor for RFC822Name
that bypasses validation so we can parse garbage out of certificates
(and round trip it if necessary)

* whoops

5 files changed, 79 insertions, 85 deletions

diff --git a/src/cryptography/hazmat/backends/openssl/decode_asn1.py b/src/cryptography/hazmat/backends/openssl/decode_asn1.py
index aefb2422..86f8f8d4 100644
--- a/src/cryptography/hazmat/backends/openssl/decode_asn1.py
+++ b/src/cryptography/hazmat/backends/openssl/decode_asn1.py
@@ -134,8 +134,14 @@ def _decode_general_name(backend, gn):
             _decode_x509_name(backend, gn.d.directoryName)
         )
     elif gn.type == backend._lib.GEN_EMAIL:
-        data = _asn1_string_to_bytes(backend, gn.d.rfc822Name)
-        return x509.RFC822Name(data)
+        # Convert to bytes and then decode to utf8. We don't use
+        # asn1_string_to_utf8 here because it doesn't properly convert
+        # utf8 from ia5strings.
+        data = _asn1_string_to_bytes(backend, gn.d.rfc822Name).decode("utf8")
+        # We don't use the constructor for RFC822Name so we can bypass
+        # validation. This allows us to create RFC822Name objects that have
+        # unicode chars when a certificate (against the RFC) contains them.
+        return x509.RFC822Name._init_without_validation(data)
     elif gn.type == backend._lib.GEN_OTHERNAME:
         type_id = _obj2txt(backend, gn.d.otherName.type_id)
         value = _asn1_to_der(backend, gn.d.otherName.value)
diff --git a/src/cryptography/hazmat/backends/openssl/encode_asn1.py b/src/cryptography/hazmat/backends/openssl/encode_asn1.py
index 3177cf96..3f0a4c8c 100644
--- a/src/cryptography/hazmat/backends/openssl/encode_asn1.py
+++ b/src/cryptography/hazmat/backends/openssl/encode_asn1.py
@@ -434,9 +434,10 @@ def _encode_general_name(backend, name):
     elif isinstance(name, x509.RFC822Name):
         gn = backend._lib.GENERAL_NAME_new()
         backend.openssl_assert(gn != backend._ffi.NULL)
-        asn1_str = _encode_asn1_str(
-            backend, name.bytes_value, len(name.bytes_value)
-        )
+        # ia5strings are supposed to be ITU T.50 but to allow round-tripping
+        # of broken certs that encode utf8 we'll encode utf8 here too.
+        data = name.value.encode("utf8")
+        asn1_str = _encode_asn1_str(backend, data, len(data))
         gn.type = backend._lib.GEN_EMAIL
         gn.d.rfc822Name = asn1_str
     elif isinstance(name, x509.UniformResourceIdentifier):
diff --git a/src/cryptography/x509/general_name.py b/src/cryptography/x509/general_name.py
index d4d92c88..6f7fa7a7 100644
--- a/src/cryptography/x509/general_name.py
+++ b/src/cryptography/x509/general_name.py
@@ -53,77 +53,55 @@ class RFC822Name(object):
     def __init__(self, value):
         if isinstance(value, six.text_type):
             try:
-                value = value.encode("ascii")
+                value.encode("ascii")
             except UnicodeEncodeError:
                 value = self._idna_encode(value)
                 warnings.warn(
-                    "RFC822Name values should be passed as bytes, not strings."
-                    " Support for passing unicode strings will be removed in a"
-                    " future version.",
-                    utils.DeprecatedIn21,
-                    stacklevel=2,
-                )
-            else:
-                warnings.warn(
-                    "RFC822Name values should be passed as bytes, not strings."
-                    " Support for passing unicode strings will be removed in a"
-                    " future version.",
+                    "RFC822Name values should be passed as an A-label string. "
+                    "This means unicode characters should be encoded via "
+                    "idna. Support for passing unicode strings (aka U-label) "
+                    " will be removed in a future version.",
                     utils.DeprecatedIn21,
                     stacklevel=2,
                 )
-        elif not isinstance(value, bytes):
-            raise TypeError("value must be bytes")
+        else:
+            raise TypeError("value must be string")
 
-        name, address = parseaddr(value.decode("ascii"))
+        name, address = parseaddr(value)
         if name or not address:
             # parseaddr has found a name (e.g. Name <email>) or the entire
             # value is an empty string.
             raise ValueError("Invalid rfc822name value")
 
-        self._bytes_value = value
+        self._value = value
 
-    bytes_value = utils.read_only_property("_bytes_value")
+    value = utils.read_only_property("_value")
+
+    @classmethod
+    def _init_without_validation(cls, value):
+        instance = cls.__new__(cls)
+        instance._value = value
+        return instance
 
     def _idna_encode(self, value):
         _, address = parseaddr(value)
         parts = address.split(u"@")
-        return parts[0].encode("ascii") + b"@" + idna.encode(parts[1])
-
-    @property
-    def value(self):
-        warnings.warn(
-            "RFC822Name.bytes_value should be used instead of RFC822Name.value"
-            "; it contains the name as raw bytes, instead of as an idna-"
-            "decoded unicode string. RFC822Name.value will be removed in a "
-            "future version.",
-            utils.DeprecatedIn21,
-            stacklevel=2
-        )
-        _, address = parseaddr(self.bytes_value.decode("ascii"))
-        parts = address.split(u"@")
-        if len(parts) == 1:
-            # Single label email name. This is valid for local delivery.
-            # No IDNA decoding needed since there is no domain component.
-            return address
-        else:
-            # A normal email of the form user@domain.com. Let's attempt to
-            # encode the domain component and reconstruct the address.
-            return parts[0] + u"@" + idna.decode(parts[1])
+        return parts[0] + "@" + idna.encode(parts[1]).decode("ascii")
 
     def __repr__(self):
-        return "<RFC822Name(bytes_value={0!r})>".format(self.bytes_value)
+        return "<RFC822Name(value={0!r})>".format(self.value)
 
     def __eq__(self, other):
         if not isinstance(other, RFC822Name):
             return NotImplemented
 
-        return self.bytes_value == other.bytes_value
+        return self.value == other.value
 
     def __ne__(self, other):
         return not self == other
 
     def __hash__(self):
-        return hash(self.bytes_value)
+        return hash(self.value)
 
 
 def _idna_encode(value):
diff --git a/tests/x509/test_x509.py b/tests/x509/test_x509.py
index afe1c0e9..d6aafcda 100644
--- a/tests/x509/test_x509.py
+++ b/tests/x509/test_x509.py
@@ -2243,7 +2243,14 @@ class TestCertificateBuilder(object):
         "add_ext",
         [
             x509.SubjectAlternativeName(
-                [x509.DNSName._init_without_validation(u'a\xedt\xe1s.test')]
+                [
+                    # These examples exist to verify compatibility with
+                    # certificates that have utf8 encoded data in the ia5string
+                    x509.DNSName._init_without_validation(u'a\xedt\xe1s.test'),
+                    x509.RFC822Name._init_without_validation(
+                        u'test@a\xedt\xe1s.test'
+                    ),
+                ]
             ),
             x509.CertificatePolicies([
                 x509.PolicyInformation(
diff --git a/tests/x509/test_x509_ext.py b/tests/x509/test_x509_ext.py
index 6e376bb1..5ad28358 100644
--- a/tests/x509/test_x509_ext.py
+++ b/tests/x509/test_x509_ext.py
@@ -248,7 +248,7 @@ class TestCertificateIssuer(object):
             x509.DNSName(u"cryptography.io"),
             x509.DNSName(u"crypto.local"),
             x509.DNSName(u"another.local"),
-            x509.RFC822Name(b"email@another.local"),
+            x509.RFC822Name(u"email@another.local"),
             x509.UniformResourceIdentifier(b"http://another.local"),
         ])
         assert ci[-1] == ci[4]
@@ -1755,53 +1755,51 @@ class TestDirectoryName(object):
 
 class TestRFC822Name(object):
     def test_repr(self):
-        gn = x509.RFC822Name(b"string")
+        gn = x509.RFC822Name(u"string")
         if six.PY3:
-            assert repr(gn) == "<RFC822Name(bytes_value=b'string')>"
+            assert repr(gn) == "<RFC822Name(value='string')>"
         else:
-            assert repr(gn) == "<RFC822Name(bytes_value='string')>"
+            assert repr(gn) == "<RFC822Name(value=u'string')>"
 
     def test_equality(self):
-        gn = x509.RFC822Name(b"string")
-        gn2 = x509.RFC822Name(b"string2")
-        gn3 = x509.RFC822Name(b"string")
+        gn = x509.RFC822Name(u"string")
+        gn2 = x509.RFC822Name(u"string2")
+        gn3 = x509.RFC822Name(u"string")
         assert gn != gn2
         assert gn != object()
         assert gn == gn3
 
-    def test_not_text_or_bytes(self):
+    def test_not_text(self):
         with pytest.raises(TypeError):
             x509.RFC822Name(1.3)
 
+        with pytest.raises(TypeError):
+            x509.RFC822Name(b"bytes")
+
     def test_invalid_email(self):
         with pytest.raises(ValueError):
             x509.RFC822Name(u"Name <email>")
-        with pytest.raises(ValueError):
-            x509.RFC822Name(b"Name <email>")
 
         with pytest.raises(ValueError):
-            x509.RFC822Name(b"")
+            x509.RFC822Name(u"")
 
     def test_single_label(self):
-        gn = x509.RFC822Name(b"administrator")
-        with pytest.warns(utils.DeprecatedIn21):
-            assert gn.value == u"administrator"
-
-        assert gn.bytes_value == b"administrator"
+        gn = x509.RFC822Name(u"administrator")
+        assert gn.value == u"administrator"
 
     def test_idna(self):
         with pytest.warns(utils.DeprecatedIn21):
             gn = x509.RFC822Name(u"email@em\xe5\xefl.com")
 
-        with pytest.warns(utils.DeprecatedIn21):
-            assert gn.value == u"email@em\xe5\xefl.com"
+        assert gn.value == u"email@xn--eml-vla4c.com"
 
-        assert gn.bytes_value == b"email@xn--eml-vla4c.com"
+        gn2 = x509.RFC822Name(u"email@xn--eml-vla4c.com")
+        assert gn2.value == u"email@xn--eml-vla4c.com"
 
     def test_hash(self):
-        g1 = x509.RFC822Name(b"email@host.com")
-        g2 = x509.RFC822Name(b"email@host.com")
-        g3 = x509.RFC822Name(b"admin@host.com")
+        g1 = x509.RFC822Name(u"email@host.com")
+        g2 = x509.RFC822Name(u"email@host.com")
+        g3 = x509.RFC822Name(u"admin@host.com")
 
         assert hash(g1) == hash(g2)
         assert hash(g1) != hash(g3)
@@ -2046,7 +2044,7 @@ class TestGeneralNames(object):
             x509.DNSName(u"cryptography.io"),
             x509.DNSName(u"crypto.local"),
             x509.DNSName(u"another.local"),
-            x509.RFC822Name(b"email@another.local"),
+            x509.RFC822Name(u"email@another.local"),
             x509.UniformResourceIdentifier(b"http://another.local"),
         ])
         assert gn[-1] == gn[4]
@@ -2087,7 +2085,7 @@ class TestGeneralNames(object):
             [x509.DNSName(u"cryptography.io")]
         )
         gns2 = x509.GeneralNames(
-            [x509.RFC822Name(b"admin@cryptography.io")]
+            [x509.RFC822Name(u"admin@cryptography.io")]
         )
         assert gns != gns2
         assert gns != object()
@@ -2095,7 +2093,7 @@ class TestGeneralNames(object):
     def test_hash(self):
         gns = x509.GeneralNames([x509.DNSName(u"cryptography.io")])
         gns2 = x509.GeneralNames([x509.DNSName(u"cryptography.io")])
-        gns3 = x509.GeneralNames([x509.RFC822Name(b"admin@cryptography.io")])
+        gns3 = x509.GeneralNames([x509.RFC822Name(u"admin@cryptography.io")])
         assert hash(gns) == hash(gns2)
         assert hash(gns) != hash(gns3)
 
@@ -2124,7 +2122,7 @@ class TestIssuerAlternativeName(object):
             x509.DNSName(u"cryptography.io"),
             x509.DNSName(u"crypto.local"),
             x509.DNSName(u"another.local"),
-            x509.RFC822Name(b"email@another.local"),
+            x509.RFC822Name(u"email@another.local"),
             x509.UniformResourceIdentifier(b"http://another.local"),
         ])
         assert ian[-1] == ian[4]
@@ -2167,7 +2165,7 @@ class TestIssuerAlternativeName(object):
             [x509.DNSName(u"cryptography.io")]
         )
         san2 = x509.IssuerAlternativeName(
-            [x509.RFC822Name(b"admin@cryptography.io")]
+            [x509.RFC822Name(u"admin@cryptography.io")]
         )
         assert san != san2
         assert san != object()
@@ -2176,7 +2174,7 @@ class TestIssuerAlternativeName(object):
         ian = x509.IssuerAlternativeName([x509.DNSName(u"cryptography.io")])
         ian2 = x509.IssuerAlternativeName([x509.DNSName(u"cryptography.io")])
         ian3 = x509.IssuerAlternativeName(
-            [x509.RFC822Name(b"admin@cryptography.io")]
+            [x509.RFC822Name(u"admin@cryptography.io")]
         )
         assert hash(ian) == hash(ian2)
         assert hash(ian) != hash(ian3)
@@ -2249,7 +2247,7 @@ class TestSubjectAlternativeName(object):
             x509.DNSName(u"cryptography.io"),
             x509.DNSName(u"crypto.local"),
             x509.DNSName(u"another.local"),
-            x509.RFC822Name(b"email@another.local"),
+            x509.RFC822Name(u"email@another.local"),
             x509.UniformResourceIdentifier(b"http://another.local"),
         ])
         assert san[-1] == san[4]
@@ -2292,7 +2290,7 @@ class TestSubjectAlternativeName(object):
             [x509.DNSName(u"cryptography.io")]
         )
         san2 = x509.SubjectAlternativeName(
-            [x509.RFC822Name(b"admin@cryptography.io")]
+            [x509.RFC822Name(u"admin@cryptography.io")]
         )
         assert san != san2
         assert san != object()
@@ -2301,7 +2299,7 @@ class TestSubjectAlternativeName(object):
         san = x509.SubjectAlternativeName([x509.DNSName(u"cryptography.io")])
         san2 = x509.SubjectAlternativeName([x509.DNSName(u"cryptography.io")])
         san3 = x509.SubjectAlternativeName(
-            [x509.RFC822Name(b"admin@cryptography.io")]
+            [x509.RFC822Name(u"admin@cryptography.io")]
         )
         assert hash(san) == hash(san2)
         assert hash(san) != hash(san3)
@@ -2487,7 +2485,7 @@ class TestRSASubjectAlternativeNameExtension(object):
         san = ext.value
 
         rfc822name = san.get_values_for_type(x509.RFC822Name)
-        assert [u"email@em\xe5\xefl.com"] == rfc822name
+        assert [u"email@xn--eml-vla4c.com"] == rfc822name
 
     def test_idna2003_invalid(self, backend):
         cert = _load_cert(
@@ -2520,7 +2518,7 @@ class TestRSASubjectAlternativeNameExtension(object):
         rfc822_name = ext.value.get_values_for_type(x509.RFC822Name)
         dns_name = ext.value.get_values_for_type(x509.DNSName)
         uri = ext.value.get_values_for_type(x509.UniformResourceIdentifier)
-        assert rfc822_name == [u"email@\u043f\u044b\u043a\u0430.cryptography"]
+        assert rfc822_name == [u"email@xn--80ato2c.cryptography"]
         assert dns_name == [u"xn--80ato2c.cryptography"]
         assert uri == [u"https://www.\u043f\u044b\u043a\u0430.cryptography"]
 
@@ -2569,10 +2567,14 @@ class TestRSASubjectAlternativeNameExtension(object):
             x509.load_pem_x509_certificate,
             backend
         )
-        with pytest.raises(ValueError) as exc:
-            cert.extensions
-
-        assert 'Invalid rfc822name value' in str(exc.value)
+        san = cert.extensions.get_extension_for_class(
+            x509.SubjectAlternativeName
+        ).value
+        values = san.get_values_for_type(x509.RFC822Name)
+        assert values == [
+            u'email', u'email <email>', u'email <email@email>',
+            u'email <email@xn--eml-vla4c.com>', u'myemail:'
+        ]
 
     def test_other_name(self, backend):
         cert = _load_cert(