impexpd: verify remote socket against actual host name #1699
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Hi @anarcat, there is an overlong line which currently blocks the tests from passing: |
|
hi!
i believe i have fixed this particular warning, but i must say my local linters scream at Ganeti all the time, so I can't tell if I really fixed everything here. Emacs's Flycheck is flagging 161 warnings in that particular file here... those are: i do not believe the three E501 errors above are mine, so let's hope for the best? also, do you know how to silence those CodeQL errors? they seem rather harmless as the IP addresses in question are server-side so not really PII, same for the (public) TLS cert... |
|
There is actually a Python Test failing now: You should be able to reproduce this by running |
ah, well I'll treat a i couldn't figure out how to run this container in docker, for some reason, podman is failing to pull docker images right now: go figure. |
i'm not sure what you mean here. why can't we just put those IPs in there? it would seem tremendously useful for debugging, and in fact it's why I explicitly added those... |
|
also, i think that codeQL is just giving us a false positive here: |
|
ugh, of course the PR still fails because we still need to resolve the IP, duh. this makes no sense to me, how can this be None? that would mean the CommandBuilder gets setup without a |
In 7bb0351 (impexpd: fix certificate verification with new socat versions, 2017-12-20), a hostname verification was introduced to fix socat's new (and proper) behavior of actually checking the remote hostname during OpenSSL-protected transfers. The problem, however, is that the hostname used was the default `X509_CERT_CN` constant (`x509CertCn` in Haskell) which is hardcoded to `ganeti.example.com`. In a real-world deployment, it seems like the remote CommonName (CN) of the certificate used by the export daemon is actually the target node name. In my case, it meant I was getting the following error from socat during transfers: Disk 0 failed to send data: Exited with status 1 (recent output: socat: E certificate is valid but its commonName does not match hostname "ganeti.example.com") At first I thought socat might be doing us some trouble, but no: socat works properly. An example is this: 1. generate a self-signed certificate: openssl req -x509 -newkey rsa:4096 -keyout key.pem -out cert.pem -sha256 -nodes -subj '/CN=ganeti.example.com' -days 1 2. start a socat server: socat -ls -d -d STDIO OPENSSL-LISTEN:8443,reuseaddr,forever,key=key.pem,cert=cert.pem,cafile=cert.pem 3. connect to it with a client: socat -ls -d -d STDIO OPENSSL:localhost:8443,openssl-commonname=ganeti.example.com,verify=1,key=key.pem,cert=cert.pem,cafile=cert.pem ... which actually works, which means the `openssl-commonname` argument actually works, and works properly. If it's changed, for example, to `ganeti.example.net`, the above fails with the aforementioned error message. We fix this by doing a reverse name resolution on the provided IP address. Now, we don't *assume* it's an IP address: this code kicks in only if the impexpd is passed an actual IP address, but in my experience it seems to always be the case (which is probably a separate problem to fix). This is rather brittle and assumes DNS will not lie, which is quite a stretch. In our environment, however, we have end-to-end DNSSEC so we can trust the DNS. And this beats hardcoding verify=0, which is the other workaround that can be done to fix this issue. Closes: ganeti#1681
|
i tried to fix the None host in https://github.com/ganeti/ganeti/compare/3d98c297ec7aa6a65afffd5773c3800ea43f79e6..a270666546c73ac1c03989c23969f7782395f194 ... it still doesn't work, would love some help on this. |
|
I spent some time on this and found something with regards to def testOptionLengthError(self):
testopts = [
CmdBuilderConfig(bind="0.0.0.0" + ("A" * impexpd.SOCAT_OPTION_MAXLEN),
port=1234, ca="/tmp/ca"),
CmdBuilderConfig(host="localhost", port=1234,
ca="/tmp/ca" + ("B" * impexpd.SOCAT_OPTION_MAXLEN)),
CmdBuilderConfig(host="localhost", port=1234,
key="/tmp/key" + ("B" * impexpd.SOCAT_OPTION_MAXLEN)),
]I found the error regarding the Not tampering with the Regarding the failing ...to actually check for the hostname instead of the ganeti.example.com constant: self.assertTrue("openssl-commonname=%s" % host in ssl_addr)However, that still fails. Looking at the three test cases
The above assumption is backed by the fact that removing both IP addresses from the list lets the test pass. For a more complete test we need to work with a list of dictionaries instead of a plain list, e.g. something like: testcases = [
{ 'target': 'localhost', 'expected_hostname': 'localhost' },
{ 'target': '198.51.100.4', 'expected_hostname': constants.X509_CERT_CN },
{ 'target': '192.0.2.99', 'expected_hostname': constants.X509_CERT_CN }
]
for testcase in testcases:
...However, this would still rely on those ip addresses not resolving to anything in the test environment (and it also does not cover the case where the IP actually is supposed to resolve to test all code paths). I'd say the only way to fix this would be to mock away the DNS resolving part so that the tests do not depend on anything uncontrollable. Other tests make use of the Python Mock module which should be able to fake the results of the Hope that helps a bit, I currently lack the time to come up with an implementation of the above myself, sorry :-( |
i also feel a bit out of my depth here but:
i think the proper way to do this would be to use some reserved IP space. This is probably what
It does sound like mocking would be the better approach here, but i'm not sure how to implement that tooling... |
|
Hi @anarcat, I finally found some time to look into this and created a patch against the current master based on your PR with working tests. The test code now mocks the return value of diff --git a/lib/impexpd/__init__.py b/lib/impexpd/__init__.py
index a643aac16..b6fcaee99 100644
--- a/lib/impexpd/__init__.py
+++ b/lib/impexpd/__init__.py
@@ -225,8 +225,30 @@ class CommandBuilder(object):
# For socat versions >= 1.7.3, we need to also specify
# openssl-commonname, otherwise server certificate verification will
# fail.
+ x509_cert_cn = host
+ # we were previously hardcoding constants.X509_CERT_CN here, but
+ # that's always ganeti.example.com while the other end generates
+ # an actual cert that matches the hostname. unfortunately here
+ # we are typically given an IP address, so let's try to find a
+ # real hostname for the certificate check
+ if netutils.IPAddress.IsValid(host):
+ # this looks like an IP address, override based on the reverse
+ # DNS
+ try:
+ x509_cert_cn, _, _ = socket.gethostbyaddr(host)
+ logging.warning("overriden IP address %s to reverse hostname %s",
+ host,
+ x509_cert_cn)
+ except OSError as e:
+ logging.error(
+ "failed to resolve IP address %s, reverting to default %s: %s",
+ host,
+ constants.X509_CERT_CN,
+ e,
+ )
+ x509_cert_cn = constants.X509_CERT_CN
if self._GetSocatVersion() >= (1, 7, 3):
- addr2 += ["openssl-commonname=%s" % constants.X509_CERT_CN]
+ addr2 += ["openssl-commonname=%s" % x509_cert_cn]
else:
raise errors.GenericError("Invalid mode '%s'" % self._mode)
diff --git a/test/py/ganeti.impexpd_unittest.py b/test/py/ganeti.impexpd_unittest.py
index 7e54f09cf..62d6849d7 100755
--- a/test/py/ganeti.impexpd_unittest.py
+++ b/test/py/ganeti.impexpd_unittest.py
@@ -44,7 +44,7 @@ from ganeti import errors
from ganeti import impexpd
import testutils
-
+import unittest.mock
class CmdBuilderConfig(objects.ConfigObject):
__slots__ = [
@@ -108,13 +108,20 @@ class TestCommandBuilder(unittest.TestCase):
else:
self.assertFalse(magic_cmd)
- for host in ["localhost", "198.51.100.4", "192.0.2.99"]:
+ testcases = [
+ { "target": "localhost", "expected_hostname": "localhost" },
+ { "target": "198.51.100.4", "expected_hostname": "my.ganeti.org" },
+ ]
+
+ for host in testcases:
+ socket_patcher = unittest.mock.patch("socket.gethostbyaddr", return_value=(host["expected_hostname"], [], []))
+ socket_patcher.start()
for port in [0, 1, 1234, 7856, 45452]:
for cmd_prefix in [None, "PrefixCommandGoesHere|",
"dd if=/dev/hda bs=1048576 |"]:
for cmd_suffix in [None, "< /some/file/name",
"| dd of=/dev/null"]:
- opts = CmdBuilderConfig(host=host, port=port, compress=compress,
+ opts = CmdBuilderConfig(host=host["target"], port=port, compress=compress,
cmd_prefix=cmd_prefix,
cmd_suffix=cmd_suffix)
@@ -141,15 +148,16 @@ class TestCommandBuilder(unittest.TestCase):
self.assertTrue(("OPENSSL-LISTEN:%s" % port) in ssl_addr)
elif mode == constants.IEM_EXPORT:
ssl_addr = socat_cmd[-1].split(",")
- self.assertTrue(("OPENSSL:%s:%s" % (host, port)) in ssl_addr)
+ self.assertTrue(("OPENSSL:%s:%s" % (host["target"], port)) in ssl_addr)
if impexpd.CommandBuilder._GetSocatVersion() >= (1, 7, 3):
self.assertTrue("openssl-commonname=%s" %
- constants.X509_CERT_CN in ssl_addr)
+ host["expected_hostname"] in ssl_addr)
else:
self.assertTrue("openssl-commonname=%s" %
constants.X509_CERT_CN not in ssl_addr)
self.assertTrue("verify=1" in ssl_addr)
+ socket_patcher.stop()
@testutils.RequiresIPv6()
def testIPv6(self):
@@ -190,8 +198,6 @@ class TestCommandBuilder(unittest.TestCase):
def testOptionLengthError(self):
testopts = [
- CmdBuilderConfig(bind="0.0.0.0" + ("A" * impexpd.SOCAT_OPTION_MAXLEN),
- port=1234, ca="/tmp/ca"),
CmdBuilderConfig(host="localhost", port=1234,
ca="/tmp/ca" + ("B" * impexpd.SOCAT_OPTION_MAXLEN)),
CmdBuilderConfig(host="localhost", port=1234,
|
nice, thanks! LGTM! |
In 7bb0351 (impexpd: fix certificate verification with new socat versions, 2017-12-20), a hostname verification was introduced to fix socat's new (and proper) behavior of actually checking the remote hostname during OpenSSL-protected transfers.
The problem, however, is that the hostname used was the default
X509_CERT_CNconstant (x509CertCnin Haskell) which is hardcoded toganeti.example.com. In a real-world deployment, it seems like the remote CommonName (CN) of the certificate used by the export daemon is actually the target node name.In my case, it meant I was getting the following error from socat during transfers:
At first I thought socat might be doing us some trouble, but no: socat works properly. An example is this:
generate a self-signed certificate:
start a socat server:
connect to it with a client:
... which actually works, which means the
openssl-commonnameargument actually works, and works properly. If it's changed, for example, toganeti.example.net, the above fails with the aforementioned error message.We fix this by doing a reverse name resolution on the provided IP address. Now, we don't assume it's an IP address: this code kicks in only if the impexpd is passed an actual IP address, but in my experience it seems to always be the case (which is probably a separate problem to fix).
This is rather brittle and assumes DNS will not lie, which is quite a stretch. In our environment, however, we have end-to-end DNSSEC so we can trust the DNS. And this beats hardcoding verify=0, which is the other workaround that can be done to fix this issue.
Closes: #1681