ssl deproxy client and server #9

vladtcvs · 2018-05-11T14:05:26Z

Fixing message parsing

work on https client

vladtcvs · 2018-05-15T12:45:30Z

This PR is for tempesta-tech/tempesta#737

vankoven

As addition, there still lack of tests documentation, which was one of the most important requirements.

vankoven · 2018-05-15T13:30:40Z

tls/test_https.py

+__license__ = 'GPL2'
+
+rootCA = """-----BEGIN CERTIFICATE-----
+MIIDZjCCAk6gAwIBAgIJAPP794tWF2qsMA0GCSqGSIb3DQEBCwUAMEgxCzAJBgNV


The certificates and keys are defined twice: in tls/ directory and in this file. Why the repetition is required?

It's not required. They should be deleted ftom tls/

May be we should store them in tls directory? We will also need the certificates for stress tests.

vankoven · 2018-05-15T14:00:29Z

helpers/tf_cfg.py

-                                           'log_file': 'tests_log.log'},
+                                           'log_file': 'tests_log.log',
+                                           'tmpdir': '/tmp/host',
+                                           'ip': '127.0.0.1'},


Configuration is confusing and mixed up. It was assumed that framework and client - are the same node and can't exist without each other, especially if deproxy-based tests are used. But I see the reason why it's important to run stress tester on other node: TfwBomber. Since it kernel module which heavily relies on Tempesta code, system will crash on bugs and no tests results will be saved.

I'm not happy about splitting framework and client nodes: when developing tests, you have to always remember, what is the type of current client and which node to use. But I don't see a good solution here.

Since the splitting is required, then explicit sections framework, stress_tester, tempesta, backendsare required. Putting everything ingeneral` section is worst way possible.

Also the splitting is half-done. Client node is still hardcoded to be localhost only. See the check() function here.

Good solution is to find good way to run deproxy client and servers on remote nodes.

vankoven · 2018-05-15T14:02:44Z

helpers/tf_cfg.py

@@ -47,7 +47,9 @@ def defaults(self):
        self.config.read_dict({'General': {'verbose': '0',
                                           'duration': '10',
                                           'concurrent_connections': '10',
-                                           'log_file': 'tests_log.log'},
+                                           'log_file': 'tests_log.log',
+                                           'tmpdir': '/tmp/host',


tmpdir: configuration key with absolutely the same meaning in other nodes is workdir. Please keep same names for options with the same meaning.

vankoven · 2018-05-15T14:13:04Z

tls/test_https.py

+        },
+    ]
+
+    def test_1(self):


Don't see usage of Tempesta itself. Client connects directly to nginx.

Yes, this tests checks only client and server. Tests for tempesta will be added later

vankoven · 2018-05-15T14:17:25Z

helpers/remote.py

@@ -15,7 +15,7 @@
 __license__ = 'GPL2'

 # Don't remove files from remote node. Helpful for tests development.
-DEBUG_FILES = False
+DEBUG_FILES = True


See this change again and again in your PRs and evey time it's dirty commit.

vankoven · 2018-05-15T14:36:22Z

framework/deproxy_server.py

                           '<<<<<\n%s>>>>>'
                           % self.request_buffer))
+            self.send_response(self.response_bad_request)


Connection must be closed after parsing error. It's bad idea to keep connection open and parse further requests.

May be handle_parsing_error() would be handy helpful function? it'll send and error (if asked) and close the connection. We have more tests, where the behaviour is going to be pretty useful.

vankoven · 2018-05-15T14:40:10Z

framework/deproxy_server.py

+        raise NotImplementedError("Not implemented 'wait_for_connections'")
+
+    @abc.abstractmethod
+    def recieve_request(self, request, connection):


I'm confused but I rather often misspell receive as recieve. It's copy-paste, but the spelling should be fixed :-]

vankoven · 2018-05-15T14:44:14Z

framework/deproxy_server.py

+class SSLStaticDeproxyServer(BaseAsyncoreDeproxyServer):
+
+    def __init__(self, cert, cert_key, *args, **kwargs):
+        self.response = kwargs['response']


Why response is defined for SSL servver, but not defined for non-SSL socket. The only differens between them must be init_socket finction

? both ssl and non-ssl have 'response'

vankoven · 2018-05-15T14:47:55Z

framework/deproxy_server.py

+
+    return srv
+
+def create_srv_deproxy_ssl(server):


I think, there should be only one helper function to create appropriate server, since all required variables are taken from server dictionary.

this is different functions because they creates different classes. I've used 1 class - 1 creation function method

I just asked because the functions look very close. I was expecting to see fabric pattern here: single function which creates the right class according to it's description. But it's a part of __create_client() function, which is the fabric actually, i missed that, sorry.

vankoven · 2018-05-15T14:53:04Z

framework/deproxy_server.py

+
+def create_srv_deproxy(server):
+    ka = "close"
+    if server.has_key('connection_header'):


What is connection_header? Server class must not add it's own headers when sending responses. The exact message to be send must be defined in test. No modifications inside server or client are allowed.

Oh, I've confused. Of course, there should be not 'close' or 'keep_alive', but amount of responses before closing connection

vankoven · 2018-05-18T14:26:34Z

framework/deproxy_client.py

+
+    def init_socket(self):
+        self.s = socket.socket()
+        #self.ssl_socket = ssl.SSLSocket(sock=self.s,


Seems like the code was commented out for the troubleshooting, but the right version wasn't returned back. Need to fix it.

fixed, commented code is removed

vankoven · 2018-05-18T14:31:15Z

framework/deproxy_client.py

+        BaseAsyncoreClient.recieve_response(self, response)
+
+    def init_socket(self):
+        self.s = socket.socket()


asyncore.dispatcher uses self.socket for the interaction, why self.s is needed?

self.s - raw socket. Later, in make_connect, it is wrapped with ssl socket, and this ssl socket is setted as self.socket

vankoven · 2018-05-18T14:34:36Z

framework/deproxy_server.py

                           '<<<<<\n%s>>>>>'
                           % self.request_buffer))
+            self.send_response(self.response_bad_request)


May be handle_parsing_error() would be handy helpful function? it'll send and error (if asked) and close the connection. We have more tests, where the behaviour is going to be pretty useful.

vankoven · 2018-05-18T14:39:35Z

framework/deproxy_server.py

+
+    return srv
+
+def create_srv_deproxy_ssl(server):


I just asked because the functions look very close. I was expecting to see fabric pattern here: single function which creates the right class according to it's description. But it's a part of __create_client() function, which is the fabric actually, i missed that, sorry.

vankoven · 2018-05-18T14:44:43Z

helpers/remote.py

@@ -220,6 +220,9 @@ def create_node(host):
        return RemoteNode(host, hostname, workdir, username, port)
    return LocalNode(host, hostname, workdir)

+def create_host_node():


See client. It also has no real hostname field, it's hardcoded to be localhost in tf_cfg.py.

vankoven · 2018-05-18T14:47:29Z

tls/test_https.py

+__license__ = 'GPL2'
+
+rootCA = """-----BEGIN CERTIFICATE-----
+MIIDZjCCAk6gAwIBAgIJAPP794tWF2qsMA0GCSqGSIb3DQEBCwUAMEgxCzAJBgNV


May be we should store them in tls directory? We will also need the certificates for stress tests.

vladtcvs · 2018-05-21T13:59:46Z

See client. It also has no real hostname field, it's hardcoded to be localhost in tf_cfg.py.

It's wrong idea.

Daemonizing proxy Stopping

Add test for new deproxy Fixes of client_proxy.py

vladtcvs force-pushed the vlad-https branch 2 times, most recently from b42c3aa to 1cd179d Compare May 15, 2018 12:44

vladtcvs changed the title ~~In progress: ssl client~~ In progress: ssl deproxy client and server May 15, 2018

krizhanovsky requested review from krizhanovsky and vankoven May 15, 2018 12:47

krizhanovsky assigned krizhanovsky and vankoven May 15, 2018

vankoven suggested changes May 15, 2018

View reviewed changes

vladtcvs changed the title ~~In progress: ssl deproxy client and server~~ ssl deproxy client and server May 17, 2018

vladtcvs force-pushed the vlad-https branch 3 times, most recently from cd30b23 to 3fee6ce Compare May 18, 2018 12:55

vankoven reviewed May 18, 2018

View reviewed changes

vladtcvs added 15 commits June 29, 2018 16:51

New deproxy client

2f0342d

New deproxy client

99da6c2

Daemonizing proxy Stopping

Add new deproxy to framework

ad0666a

Add test for new deproxy Fixes of client_proxy.py

Build deproxy_v2 client without explicit command port specification

cae6585

Use new deproxy client

81c7456

Separate http rules test to separate tests

1e066e9

Fix client

5a2be5a

Work on client

81bf743

Fixes errors

8cd130d

Wait for fork and pidfile creation

a80b565

Fixes

4f30d42

Additional checks

364658d

SSL client

f74b046

Set rootCA

193772f

Use ssl socket

24c4d00

vladtcvs added 4 commits July 5, 2018 17:06

fix

517f8f6

Making requests

3447f93

Use rootCA

444bfdf

Use another method for socket creation

39c14e3

vladtcvs force-pushed the vlad-https branch from 4e494d6 to 39c14e3 Compare July 5, 2018 20:15

vladtcvs added 2 commits July 6, 2018 15:56

SSL deproxy client

5228ef4

Use ssl deproxy client

3129eec

vladtcvs closed this Jul 19, 2018

krizhanovsky deleted the vlad-https branch July 1, 2019 14:26

ssl deproxy client and server #9

ssl deproxy client and server #9

Uh oh!

Conversation

vladtcvs commented May 11, 2018

Uh oh!

vladtcvs commented May 15, 2018

Uh oh!

vankoven left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vladtcvs commented May 21, 2018

Uh oh!

Uh oh!