Skip to content

Commit

Permalink
Remove fallback on commonName in match_hostname()
Browse files Browse the repository at this point in the history
  • Loading branch information
hramezani committed Dec 24, 2020
1 parent c09b25e commit fd0c475
Show file tree
Hide file tree
Showing 8 changed files with 26 additions and 83 deletions.
2 changes: 0 additions & 2 deletions src/urllib3/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,6 @@ def add_stderr_logger(level=logging.DEBUG):
# mechanisms to silence them.
# SecurityWarning's always go off by default.
warnings.simplefilter("always", exceptions.SecurityWarning, append=True)
# SubjectAltNameWarning's should go off once per host
warnings.simplefilter("default", exceptions.SubjectAltNameWarning, append=True)
# InsecurePlatformWarning's don't vary between requests, so we keep it default.
warnings.simplefilter("default", exceptions.InsecurePlatformWarning, append=True)
# SNIMissingWarnings should go off only once.
Expand Down
17 changes: 1 addition & 16 deletions src/urllib3/connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,7 @@ class BaseSSLError(BaseException):


from ._version import __version__
from .exceptions import (
ConnectTimeoutError,
NewConnectionError,
SubjectAltNameWarning,
SystemTimeWarning,
)
from .exceptions import ConnectTimeoutError, NewConnectionError, SystemTimeWarning
from .packages.ssl_match_hostname import CertificateError, match_hostname
from .util import SKIP_HEADER, SKIPPABLE_HEADERS, connection
from .util.ssl_ import (
Expand Down Expand Up @@ -420,16 +415,6 @@ def connect(self):
# the TLS library, this cannot always be done. So we check whether
# the TLS Library still thinks it's matching hostnames.
cert = self.sock.getpeercert()
if not cert.get("subjectAltName", ()):
warnings.warn(
(
f"Certificate for {hostname} has no `subjectAltName`, falling back to check for a "
"`commonName` for now. This feature is being removed by major browsers and "
"deprecated by RFC 2818. (See https://github.com/urllib3/urllib3/issues/497 "
"for details.)"
),
SubjectAltNameWarning,
)
_match_hostname(cert, self.assert_hostname or server_hostname)

self.is_verified = (
Expand Down
6 changes: 0 additions & 6 deletions src/urllib3/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -186,12 +186,6 @@ class SecurityWarning(HTTPWarning):
pass


class SubjectAltNameWarning(SecurityWarning):
"""Warned when connecting to a host with a certificate missing a SAN."""

pass


class InsecureRequestWarning(SecurityWarning):
"""Warned when making an unverified HTTPS request."""

Expand Down
15 changes: 1 addition & 14 deletions src/urllib3/packages/ssl_match_hostname/_implementation.py
Original file line number Diff line number Diff line change
Expand Up @@ -125,17 +125,6 @@ def match_hostname(cert, hostname):
if host_ip is not None and _ipaddress_match(value, host_ip):
return
dnsnames.append(value)
if not dnsnames:
# The subject is only checked when there is no dNSName entry
# in subjectAltName
for sub in cert.get("subject", ()):
for key, value in sub:
# XXX according to RFC 2818, the most specific Common Name
# must be used.
if key == "commonName":
if _dnsname_match(value, hostname):
return
dnsnames.append(value)
if len(dnsnames) > 1:
raise CertificateError(
"hostname %r "
Expand All @@ -144,6 +133,4 @@ def match_hostname(cert, hostname):
elif len(dnsnames) == 1:
raise CertificateError(f"hostname {hostname!r} doesn't match {dnsnames[0]!r}")
else:
raise CertificateError(
"no appropriate commonName or subjectAltName fields were found"
)
raise CertificateError("no appropriate subjectAltName fields were found")
14 changes: 0 additions & 14 deletions test/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,20 +75,6 @@ def ip_san_server(tmp_path_factory):
yield cfg


@pytest.fixture
def ipv6_addr_server(tmp_path_factory):
if not HAS_IPV6:
pytest.skip("Only runs on IPv6 systems")

tmpdir = tmp_path_factory.mktemp("certs")
ca = trustme.CA()
# IP address in Common Name
server_cert = ca.issue_cert(common_name="::1")

with run_server_in_thread("https", "::1", tmpdir, ca, server_cert) as cfg:
yield cfg


@pytest.fixture
def ipv6_san_server(tmp_path_factory):
if not HAS_IPV6:
Expand Down
2 changes: 0 additions & 2 deletions test/contrib/test_pyopenssl.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,7 @@ def teardown_module():
from ..with_dummyserver.test_https import ( # noqa: E402, F401
TestHTTPS,
TestHTTPS_IPSAN,
TestHTTPS_IPv6Addr,
TestHTTPS_IPV6SAN,
TestHTTPS_NoSAN,
TestHTTPS_TLSv1,
TestHTTPS_TLSv1_1,
TestHTTPS_TLSv1_2,
Expand Down
13 changes: 13 additions & 0 deletions test/test_connection.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@
import pytest

from urllib3.connection import RECENT_DATE, CertificateError, _match_hostname
from urllib3.packages.ssl_match_hostname._implementation import (
CertificateError as ImplementationCertificateError,
)
from urllib3.packages.ssl_match_hostname._implementation import match_hostname


class TestConnection:
Expand Down Expand Up @@ -43,6 +47,15 @@ def test_match_hostname_mismatch(self):
)
assert e._peer_cert == cert

def test_match_hostname_ignore_common_name(self):
cert = {"subject": [("commonName", "foo")]}
asserted_hostname = "foo"
with pytest.raises(
ImplementationCertificateError,
match="no appropriate subjectAltName fields were found",
):
match_hostname(cert, asserted_hostname)

def test_recent_date(self):
# This test is to make sure that the RECENT_DATE value
# doesn't get too far behind what the current date is.
Expand Down
40 changes: 11 additions & 29 deletions test/with_dummyserver/test_https.py
Original file line number Diff line number Diff line change
Expand Up @@ -782,22 +782,6 @@ class TestHTTPS_TLSv1_3(TestHTTPS):
certs = TLSv1_3_CERTS


class TestHTTPS_NoSAN:
def test_warning_for_certs_without_a_san(self, no_san_server):
"""Ensure that a warning is raised when the cert from the server has
no Subject Alternative Name."""
with mock.patch("warnings.warn") as warn:
with HTTPSConnectionPool(
no_san_server.host,
no_san_server.port,
cert_reqs="CERT_REQUIRED",
ca_certs=no_san_server.ca_certs,
) as https_pool:
r = https_pool.request("GET", "/")
assert r.status == 200
assert warn.called


class TestHTTPS_IPSAN:
def test_can_validate_ip_san(self, ip_san_server):
"""Ensure that urllib3 can validate SANs with IP addresses in them."""
Expand All @@ -816,19 +800,6 @@ def test_can_validate_ip_san(self, ip_san_server):
assert r.status == 200


class TestHTTPS_IPv6Addr:
def test_strip_square_brackets_before_validating(self, ipv6_addr_server):
"""Test that the fix for #760 works."""
with HTTPSConnectionPool(
"[::1]",
ipv6_addr_server.port,
cert_reqs="CERT_REQUIRED",
ca_certs=ipv6_addr_server.ca_certs,
) as https_pool:
r = https_pool.request("GET", "/")
assert r.status == 200


class TestHTTPS_IPV6SAN:
def test_can_validate_ipv6_san(self, ipv6_san_server):
"""Ensure that urllib3 can validate SANs with IPv6 addresses in them."""
Expand All @@ -845,3 +816,14 @@ def test_can_validate_ipv6_san(self, ipv6_san_server):
) as https_pool:
r = https_pool.request("GET", "/")
assert r.status == 200

def test_strip_square_brackets_before_validating(self, ipv6_san_server):
"""Test that the fix for #760 works."""
with HTTPSConnectionPool(
"[::1]",
ipv6_san_server.port,
cert_reqs="CERT_REQUIRED",
ca_certs=ipv6_san_server.ca_certs,
) as https_pool:
r = https_pool.request("GET", "/")
assert r.status == 200

0 comments on commit fd0c475

Please sign in to comment.