New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable hostname checks for SSL connections on RHEL 7.2 #668

Merged
merged 2 commits into from Dec 8, 2015

Conversation

Projects
None yet
2 participants
@atodorov
Contributor

atodorov commented Nov 24, 2015

In RHEL 7.2 python-libs-2.7.5-34.el7.x86_64 introduced the following change:

diff -u httplib.py /usr/lib64/python2.7/httplib.py
--- httplib.py  2014-02-11 14:46:38.000000000 +0200
+++ /usr/lib64/python2.7/httplib.py 2015-11-24 16:33:57.106008991 +0200
@@ -215,6 +215,10 @@
 # maximal line length when calling readline().
 _MAXLINE = 65536

+# maximum amount of headers accepted
+_MAXHEADERS = 100
+
+
 class HTTPMessage(mimetools.Message):

     def addheader(self, key, value):
@@ -271,6 +275,8 @@
         elif self.seekable:
             tell = self.fp.tell
         while True:
+            if len(hlist) > _MAXHEADERS:
+                raise HTTPException("got more than %d headers" % _MAXHEADERS)
             if tell:
                 try:
                     startofline = tell()
@@ -1159,21 +1165,44 @@

         def __init__(self, host, port=None, key_file=None, cert_file=None,
                      strict=None, timeout=socket._GLOBAL_DEFAULT_TIMEOUT,
-                     source_address=None):
+                     source_address=None, context=None, check_hostname=None):
             HTTPConnection.__init__(self, host, port, strict, timeout,
                                     source_address)
             self.key_file = key_file
             self.cert_file = cert_file
+            if context is None:
+                context = ssl._create_default_https_context()
+            will_verify = context.verify_mode != ssl.CERT_NONE
+            if check_hostname is None:
+                check_hostname = will_verify
+            elif check_hostname and not will_verify:
+                raise ValueError("check_hostname needs a SSL context with "
+                                 "either CERT_OPTIONAL or CERT_REQUIRED")
+            if key_file or cert_file:
+                context.load_cert_chain(cert_file, key_file)
+            self._context = context
+            self._check_hostname = check_hostname

         def connect(self):
             "Connect to a host on a given (SSL) port."

-            sock = socket.create_connection((self.host, self.port),
-                                            self.timeout, self.source_address)
+            HTTPConnection.connect(self)
+
             if self._tunnel_host:
-                self.sock = sock
-                self._tunnel()
-            self.sock = ssl.wrap_socket(sock, self.key_file, self.cert_file)
+                server_hostname = self._tunnel_host
+            else:
+                server_hostname = self.host
+            sni_hostname = server_hostname if ssl.HAS_SNI else None
+
+            self.sock = self._context.wrap_socket(self.sock,
+                                                  server_hostname=sni_hostname)
+            if not self._context.check_hostname and self._check_hostname:
+                try:
+                    ssl.match_hostname(self.sock.getpeercert(), server_hostname)
+                except Exception:
+                    self.sock.shutdown(socket.SHUT_RDWR)
+                    self.sock.close()
+                    raise

     __all__.append("HTTPSConnection")

which causes SSL connections to attempt verification of hostname, which in turn fails and causes #647. With this patch both context.check_hostname and check_hostname are set to False which successfully disables this check.

Please build an updated RPM for EPEL once this is included in master. Thanks!

Btw without this patch test 68 List all was failing on RHEL 7.2, now everything is PASS.

@mdomsch

This comment has been minimized.

Show comment
Hide comment
@mdomsch

mdomsch Nov 24, 2015

Member

Arrggghhh! I hate backwards incompatible changes like this. After this
change, s3cmd running on < RHEL7 will fail because
httplib.HTTPSConnection.init()
doesn't take the context or check_hostname arguments. Odd though - we've
been passing in context= already. Oh, I see it...

   try:
        context = http_connection._ssl_context()
        # S3's wildcart certificate doesn't work with DNS-style named

buckets.
if (hostname.endswith('.amazonaws.com') or hostname.endswith('.
amazonaws.com.cn')) and context:
# this merely delays running the hostname check until
# after the connection is made and we get control
# back. We then run the same check, relaxed for S3's
# wildcard certificates.
context.check_hostname = False
conn = httplib.HTTPSConnection(hostname, port, context=context)
except TypeError:
conn = httplib.HTTPSConnection(hostname, port)

If we make the change as this patch suggests, we could get a TypeError for
two different reasons: one where context= is not present (as we catch now),
and one where check_hostname is not present (which we would catch, but
handle improperly). Which we would get depends on the version of
python-libs installed on the system, and 7.0 >= RHEL < 7.2 we would fail.
:-(

Can you add the corresponding try/except to catch missing check_hostname
also?

Thanks,
Matt

On Tue, Nov 24, 2015 at 8:43 AM, Alexander Todorov <notifications@github.com

wrote:

In RHEL 7.2 python-libs-2.7.5-34.el7.x86_64 introduced the following
change:

diff -u httplib.py /usr/lib64/python2.7/httplib.py
--- httplib.py 2014-02-11 14:46:38.000000000 +0200
+++ /usr/lib64/python2.7/httplib.py 2015-11-24 16:33:57.106008991 +0200
@@ -215,6 +215,10 @@

maximal line length when calling readline().

_MAXLINE = 65536

+# maximum amount of headers accepted
+_MAXHEADERS = 100
+
+
class HTTPMessage(mimetools.Message):

 def addheader(self, key, value):

@@ -271,6 +275,8 @@
elif self.seekable:
tell = self.fp.tell
while True:

  •        if len(hlist) > _MAXHEADERS:
    
  •            raise HTTPException("got more than %d headers" % _MAXHEADERS)
         if tell:
             try:
                 startofline = tell()
    

    @@ -1159,21 +1165,44 @@

     def __init__(self, host, port=None, key_file=None, cert_file=None,
                  strict=None, timeout=socket._GLOBAL_DEFAULT_TIMEOUT,
    
  •                 source_address=None):
    
  •                 source_address=None, context=None, check_hostname=None):
         HTTPConnection.**init**(self, host, port, strict, timeout,
                                 source_address)
         self.key_file = key_file
         self.cert_file = cert_file
    
  •        if context is None:
    
  •            context = ssl._create_default_https_context()
    
  •        will_verify = context.verify_mode != ssl.CERT_NONE
    
  •        if check_hostname is None:
    
  •            check_hostname = will_verify
    
  •        elif check_hostname and not will_verify:
    
  •            raise ValueError("check_hostname needs a SSL context with "
    
  •                             "either CERT_OPTIONAL or CERT_REQUIRED")
    
  •        if key_file or cert_file:
    
  •            context.load_cert_chain(cert_file, key_file)
    
  •        self._context = context
    
  •        self._check_hostname = check_hostname
    
     def connect(self):
         "Connect to a host on a given (SSL) port."
    
  •        sock = socket.create_connection((self.host, self.port),
    
  •                                        self.timeout, self.source_address)
    
  •        HTTPConnection.connect(self)
    
    •    if self._tunnel_host:
      
  •            self.sock = sock
    
  •            self._tunnel()
    
  •        self.sock = ssl.wrap_socket(sock, self.key_file, self.cert_file)
    
  •            server_hostname = self._tunnel_host
    
  •        else:
    
  •            server_hostname = self.host
    
  •        sni_hostname = server_hostname if ssl.HAS_SNI else None
    
  •        self.sock = self._context.wrap_socket(self.sock,
    
  •                                              server_hostname=sni_hostname)
    
  •        if not self._context.check_hostname and self._check_hostname:
    
  •            try:
    
  •                ssl.match_hostname(self.sock.getpeercert(), server_hostname)
    
  •            except Exception:
    
  •                self.sock.shutdown(socket.SHUT_RDWR)
    
  •                self.sock.close()
    
  •                raise
    

    all.append("HTTPSConnection")

which causes SSL connections to attempt verification of hostname, which in
turn fails and causes #647 #647.
With this patch both context.check_hostname and check_hostname are set to
False which successfully disables this check.

Please build an updated RPM for EPEL once this is included in master.

Thanks!

You can view, comment on, or merge this pull request online at:

#668
Commit Summary

  • Disable hostname checks for SSL connections on RHEL 7.2

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#668.

Member

mdomsch commented Nov 24, 2015

Arrggghhh! I hate backwards incompatible changes like this. After this
change, s3cmd running on < RHEL7 will fail because
httplib.HTTPSConnection.init()
doesn't take the context or check_hostname arguments. Odd though - we've
been passing in context= already. Oh, I see it...

   try:
        context = http_connection._ssl_context()
        # S3's wildcart certificate doesn't work with DNS-style named

buckets.
if (hostname.endswith('.amazonaws.com') or hostname.endswith('.
amazonaws.com.cn')) and context:
# this merely delays running the hostname check until
# after the connection is made and we get control
# back. We then run the same check, relaxed for S3's
# wildcard certificates.
context.check_hostname = False
conn = httplib.HTTPSConnection(hostname, port, context=context)
except TypeError:
conn = httplib.HTTPSConnection(hostname, port)

If we make the change as this patch suggests, we could get a TypeError for
two different reasons: one where context= is not present (as we catch now),
and one where check_hostname is not present (which we would catch, but
handle improperly). Which we would get depends on the version of
python-libs installed on the system, and 7.0 >= RHEL < 7.2 we would fail.
:-(

Can you add the corresponding try/except to catch missing check_hostname
also?

Thanks,
Matt

On Tue, Nov 24, 2015 at 8:43 AM, Alexander Todorov <notifications@github.com

wrote:

In RHEL 7.2 python-libs-2.7.5-34.el7.x86_64 introduced the following
change:

diff -u httplib.py /usr/lib64/python2.7/httplib.py
--- httplib.py 2014-02-11 14:46:38.000000000 +0200
+++ /usr/lib64/python2.7/httplib.py 2015-11-24 16:33:57.106008991 +0200
@@ -215,6 +215,10 @@

maximal line length when calling readline().

_MAXLINE = 65536

+# maximum amount of headers accepted
+_MAXHEADERS = 100
+
+
class HTTPMessage(mimetools.Message):

 def addheader(self, key, value):

@@ -271,6 +275,8 @@
elif self.seekable:
tell = self.fp.tell
while True:

  •        if len(hlist) > _MAXHEADERS:
    
  •            raise HTTPException("got more than %d headers" % _MAXHEADERS)
         if tell:
             try:
                 startofline = tell()
    

    @@ -1159,21 +1165,44 @@

     def __init__(self, host, port=None, key_file=None, cert_file=None,
                  strict=None, timeout=socket._GLOBAL_DEFAULT_TIMEOUT,
    
  •                 source_address=None):
    
  •                 source_address=None, context=None, check_hostname=None):
         HTTPConnection.**init**(self, host, port, strict, timeout,
                                 source_address)
         self.key_file = key_file
         self.cert_file = cert_file
    
  •        if context is None:
    
  •            context = ssl._create_default_https_context()
    
  •        will_verify = context.verify_mode != ssl.CERT_NONE
    
  •        if check_hostname is None:
    
  •            check_hostname = will_verify
    
  •        elif check_hostname and not will_verify:
    
  •            raise ValueError("check_hostname needs a SSL context with "
    
  •                             "either CERT_OPTIONAL or CERT_REQUIRED")
    
  •        if key_file or cert_file:
    
  •            context.load_cert_chain(cert_file, key_file)
    
  •        self._context = context
    
  •        self._check_hostname = check_hostname
    
     def connect(self):
         "Connect to a host on a given (SSL) port."
    
  •        sock = socket.create_connection((self.host, self.port),
    
  •                                        self.timeout, self.source_address)
    
  •        HTTPConnection.connect(self)
    
    •    if self._tunnel_host:
      
  •            self.sock = sock
    
  •            self._tunnel()
    
  •        self.sock = ssl.wrap_socket(sock, self.key_file, self.cert_file)
    
  •            server_hostname = self._tunnel_host
    
  •        else:
    
  •            server_hostname = self.host
    
  •        sni_hostname = server_hostname if ssl.HAS_SNI else None
    
  •        self.sock = self._context.wrap_socket(self.sock,
    
  •                                              server_hostname=sni_hostname)
    
  •        if not self._context.check_hostname and self._check_hostname:
    
  •            try:
    
  •                ssl.match_hostname(self.sock.getpeercert(), server_hostname)
    
  •            except Exception:
    
  •                self.sock.shutdown(socket.SHUT_RDWR)
    
  •                self.sock.close()
    
  •                raise
    

    all.append("HTTPSConnection")

which causes SSL connections to attempt verification of hostname, which in
turn fails and causes #647 #647.
With this patch both context.check_hostname and check_hostname are set to
False which successfully disables this check.

Please build an updated RPM for EPEL once this is included in master.

Thanks!

You can view, comment on, or merge this pull request online at:

#668
Commit Summary

  • Disable hostname checks for SSL connections on RHEL 7.2

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#668.

@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Nov 24, 2015

Contributor

I've created another commit for this case although I don't think you need it. The httplib.py diff above suggests that both context and check_hostname are added together. I've not seen the case where only context exists but not check_hostname. Anyway you can apply both commits or only the first one.

Contributor

atodorov commented Nov 24, 2015

I've created another commit for this case although I don't think you need it. The httplib.py diff above suggests that both context and check_hostname are added together. I've not seen the case where only context exists but not check_hostname. Anyway you can apply both commits or only the first one.

@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Nov 24, 2015

Contributor

Btw I've found a way to inspect what arguments are there in the constructor. From the inspect module:

class H:
    def __init__(self, context=None):
        pass

pprint(dict(inspect.getmembers(H.__init__.__func__.__code__)))

Look at the 'co_varnames' attribute. It is ('self', 'context'). If you'd like to go this way we can add ifs for context and check_hostname, build the argument list and pass it to the constructor in one statement. Doesn't look so much nicer than the double try-except though.

Contributor

atodorov commented Nov 24, 2015

Btw I've found a way to inspect what arguments are there in the constructor. From the inspect module:

class H:
    def __init__(self, context=None):
        pass

pprint(dict(inspect.getmembers(H.__init__.__func__.__code__)))

Look at the 'co_varnames' attribute. It is ('self', 'context'). If you'd like to go this way we can add ifs for context and check_hostname, build the argument list and pass it to the constructor in one statement. Doesn't look so much nicer than the double try-except though.

@mdomsch

This comment has been minimized.

Show comment
Hide comment
@mdomsch

mdomsch Nov 24, 2015

Member

double-try with a good comment will be fine thanks.

On Tue, Nov 24, 2015 at 1:42 PM, Alexander Todorov <notifications@github.com

wrote:

Btw I've found a way to inspect what arguments are there in the
constructor. From the inspect module:

class H:
def init(self, context=None):
pass

pprint(dict(inspect.getmembers(H.init.func.code)))

Look at the 'co_varnames' attribute. It is ('self', 'context'). If you'd
like to go this way we can add ifs for context and check_hostname, build
the argument list and pass it to the constructor in one statement. Doesn't
look so much nicer than the double try-except though.


Reply to this email directly or view it on GitHub
#668 (comment).

Member

mdomsch commented Nov 24, 2015

double-try with a good comment will be fine thanks.

On Tue, Nov 24, 2015 at 1:42 PM, Alexander Todorov <notifications@github.com

wrote:

Btw I've found a way to inspect what arguments are there in the
constructor. From the inspect module:

class H:
def init(self, context=None):
pass

pprint(dict(inspect.getmembers(H.init.func.code)))

Look at the 'co_varnames' attribute. It is ('self', 'context'). If you'd
like to go this way we can add ifs for context and check_hostname, build
the argument list and pass it to the constructor in one statement. Doesn't
look so much nicer than the double try-except though.


Reply to this email directly or view it on GitHub
#668 (comment).

@atodorov

This comment has been minimized.

Show comment
Hide comment
@atodorov

atodorov Nov 29, 2015

Contributor

ping, how does the latest look ?

Contributor

atodorov commented Nov 29, 2015

ping, how does the latest look ?

@mdomsch

This comment has been minimized.

Show comment
Hide comment
@mdomsch

mdomsch Dec 7, 2015

Member

Almost. This unconditionally disables all checking, which I don't think is what we want.
See https://github.com/s3tools/s3cmd/tree/atodorov-ssl_wildcard_fix branch with one more patch beyond yours.

Member

mdomsch commented Dec 7, 2015

Almost. This unconditionally disables all checking, which I don't think is what we want.
See https://github.com/s3tools/s3cmd/tree/atodorov-ssl_wildcard_fix branch with one more patch beyond yours.

@mdomsch

This comment has been minimized.

Show comment
Hide comment
@mdomsch

mdomsch Dec 7, 2015

Member

With this patch, it seems to work on RHEL 6.7, RHEL 7.0, RHEL 7.2, and Fedora 23. Can someone else try other OSs?

Member

mdomsch commented Dec 7, 2015

With this patch, it seems to work on RHEL 6.7, RHEL 7.0, RHEL 7.2, and Fedora 23. Can someone else try other OSs?

@mdomsch

This comment has been minimized.

Show comment
Hide comment
@mdomsch

mdomsch Dec 7, 2015

Member

Centos 7.x hasn't caught up with that RHEL 7.2 change yet. It has:
python-libs-2.7.5-18.el7_1.1.x86_64
which doesn't have either context= or check_hostname=.

Fedora 23 has
python-libs-2.7.10-8.fc23.x86_64
which has context= but not check_hostname=.

I find it odd and annoying that Fedora would not have a patch that was already into RHEL 7.2 though.

Member

mdomsch commented Dec 7, 2015

Centos 7.x hasn't caught up with that RHEL 7.2 change yet. It has:
python-libs-2.7.5-18.el7_1.1.x86_64
which doesn't have either context= or check_hostname=.

Fedora 23 has
python-libs-2.7.10-8.fc23.x86_64
which has context= but not check_hostname=.

I find it odd and annoying that Fedora would not have a patch that was already into RHEL 7.2 though.

@mdomsch

This comment has been minimized.

Show comment
Hide comment
@mdomsch

mdomsch Dec 8, 2015

Member

The Centos 7 cr repository has the newer python-libs library. I've tested with and without the fix branch and have seen it fail without, and succeed with, the fix branch.

Member

mdomsch commented Dec 8, 2015

The Centos 7 cr repository has the newer python-libs library. I've tested with and without the fix branch and have seen it fail without, and succeed with, the fix branch.

@mdomsch mdomsch referenced this pull request Dec 8, 2015

Merged

Atodorov ssl wildcard fix #673

@mdomsch mdomsch merged commit 341749e into s3tools:master Dec 8, 2015

@mdomsch

This comment has been minimized.

Show comment
Hide comment
@mdomsch

mdomsch Jan 20, 2016

Member

http://koji.fedoraproject.org/koji/taskinfo?taskID=12622836 now has an EL7
build with 1.6.1 that includes this fix.

On Sun, Nov 29, 2015 at 7:56 AM, Alexander Todorov <notifications@github.com

wrote:

ping, how does the latest look ?


Reply to this email directly or view it on GitHub
#668 (comment).

Member

mdomsch commented Jan 20, 2016

http://koji.fedoraproject.org/koji/taskinfo?taskID=12622836 now has an EL7
build with 1.6.1 that includes this fix.

On Sun, Nov 29, 2015 at 7:56 AM, Alexander Todorov <notifications@github.com

wrote:

ping, how does the latest look ?


Reply to this email directly or view it on GitHub
#668 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment