Adding SSL support to server and command line clients. #899

dkrupp · 2017-09-11T20:05:00Z

No description provided.

Xazax-hun

According to codacy: ssl.wrap_socket call with no SSL/TLS protocol version specified, the default SSLv23 could be insecure, possible security issue.

Maybe we should fix this problem.

Xazax-hun · 2017-09-12T07:47:18Z

libcodechecker/libclient/authentication_helper.py

        self.__host = host
        self.__port = port
-        self.transport = THttpClient.THttpClient(self.__host, self.__port, uri)
+        url = protocol+"://"+host+":"+str(port)+uri
+        self.transport = THttpClient.THttpClient(url)
        self.protocol = TJSONProtocol.TJSONProtocol(self.transport)
        self.client = codeCheckerAuthentication.Client(self.protocol)


The protocol and self.protocol are two distinct things. Maybe we should name them separately.

Xazax-hun · 2017-09-12T07:49:02Z

libcodechecker/libclient/product_helper.py

        self.__host = host
        self.__port = port
-        self.transport = THttpClient.THttpClient(self.__host, self.__port, uri)
+        url = protocol+"://"+host+":"+str(port)+uri


This url creating code is there multiple times, maybe worth to create a function?

Same comment about the name of protocol as above.

Xazax-hun · 2017-09-12T07:51:32Z

libcodechecker/server/client_db_access_server.py

+                                              certfile=ssl_cert_file)
+
+            else:
+                LOG.info("SSL key or cert files not found. cert:" +


Do we want to display this message even if the user never configured this? It is just an ergonomic change in the future.

Xazax-hun · 2017-09-12T07:53:53Z

libcodechecker/util.py

@@ -266,7 +266,7 @@ def split_server_url(url):
    LOG.debug("Parsing server url '{0}'".format(url))

    protocol = 'http'
-    if url.startswith('http'):
+    if url.startswith('http') or url.startswith('https'):


If url starts with https it also starts with http right? This check might be redundant.

Xazax-hun · 2017-09-12T07:56:34Z

tests/libtest/thrift_client_to_db.py

-            host, port, '/' + product + '/v' + VERSION + endpoint)
+        url = protocol+"://"+host+":"+str(port) +\
+            '/' + product + '/v' + VERSION + endpoint
+        print ("setup viewer client:"+url)


Is this print intended? If so, maybe having a space after colon and starting with capital letter would be great.

Xazax-hun

Few nits otherwise LGTM!

Xazax-hun · 2017-09-12T08:56:24Z

tests/libtest/codechecker.py


+    url = protocol + "://"+server_data['viewer_host'] +\


Maybe you could use the just introduced util here?

Please format the code better so + is wrapped in spaces around the operand.

Xazax-hun · 2017-09-12T08:56:47Z

tests/libtest/thrift_client_to_db.py

                 auto_handle_connection=True, session_token=None):
        # Import only if necessary; some tests may not add this to PYTHONPATH.
        from libcodechecker import session_manager
        from codeCheckerDBAccess_v6 import codeCheckerDBAccess
        from codeCheckerDBAccess_v6.constants import MAX_QUERY_SIZE

        self.max_query_size = MAX_QUERY_SIZE
-        transport = THttpClient.THttpClient(
-            host, port, '/' + product + '/v' + VERSION + endpoint)
+        url = protocol+"://"+host+":"+str(port) +\


Is the introduced util applicable here?

Xazax-hun · 2017-09-12T08:57:04Z

tests/libtest/thrift_client_to_db.py

        full_uri = '/v' + VERSION + uri
        if product:
            full_uri = '/' + product + full_uri
-
-        transport = THttpClient.THttpClient(host, port, full_uri)
+        url = proto+"://"+host+":"+str(port)+full_uri


Is the introduced util applicable here?

Xazax-hun · 2017-09-12T08:57:10Z

tests/libtest/thrift_client_to_db.py

                 session_token=None):
        # Import only if necessary; some tests may not add this to PYTHONPATH.
        from libcodechecker import session_manager
        from Authentication_v6 import codeCheckerAuthentication
-
-        transport = THttpClient.THttpClient(host, port, '/v' + VERSION + uri)
+        url = proto+"://"+host+":"+str(port)+'/v' + VERSION + uri


Is the introduced util applicable here?

whisperity

Minor stuff. Mostly docs and stuff. Thank you, @dkrupp, for implementing this.

whisperity · 2017-09-12T09:38:39Z

docs/products.md

+PRODUCT_URL` parameter, which specifies the access protocol, 
+server host, port, and the product's unique endpoint 
+in the following format: `http[s]://localhost:8001/Default`,
+where `Default` is the product's endpoint. See the [User Guide](/docs/user_guide.md#product_url-format ) 


There is a space before the )

whisperity · 2017-09-12T09:39:03Z

docs/usage.md

 ```
- CodeChecker store ./reports --host localhost --port 8555 --name tmux
+ CodeChecker store ./reports --name tmux --url http://loclahost:8555/Default 


localhost has a typo in it

whisperity · 2017-09-12T09:39:17Z

docs/products.md

-details.
+PRODUCT_URL` parameter, which specifies the access protocol, 
+server host, port, and the product's unique endpoint 
+in the following format: `http[s]://localhost:8001/Default`,


Is http mandatory even if we are using only HTTP?

whisperity · 2017-09-12T09:40:02Z

docs/user_guide.md

+
+To enable SSL simply place ssl certificate to `<CONFIG_DIRECTORY>/cert.pem`
+and the corresponding private key to `<CONFIG_DIRECTORY>/key.pem`.
+When the server finds these files upon start-up. SSL will be automatically enabled. 


Sentences are broken wrongly, perhaps you meant a , instead of .?

whisperity · 2017-09-12T09:40:13Z

docs/user_guide.md

+You can enforce SSL security on your listening socket. In this case all clients must
+access your server using the `https://host:port` URL format.
+
+To enable SSL simply place ssl certificate to `<CONFIG_DIRECTORY>/cert.pem`


SSL is uppercase, and then lowercase.

whisperity · 2017-09-12T09:42:22Z

tests/libtest/codechecker.py

@@ -87,7 +88,7 @@ def logout(codechecker_cfg, test_project_path):
    logout_cmd = ['CodeChecker', 'cmd', 'login',
                  '--logout',
                  '--verbose', 'debug',
-                  '--url', 'localhost:' + port]
+                  '--url', protocol+'://'+'localhost:' + port]


operator format

whisperity · 2017-09-12T09:42:28Z

tests/libtest/codechecker.py


+    url = protocol + "://"+server_data['viewer_host'] +\


operator format

whisperity · 2017-09-12T09:43:19Z

tests/ssl_example_cert/cert.pem

@@ -0,0 +1,32 @@
+-----BEGIN CERTIFICATE-----


Is this a self-signed certificate? Are we sure we are not leaking anything here?

whisperity · 2017-09-12T09:44:04Z

docs/user_guide.md

@@ -940,6 +967,15 @@ root account arguments:
                        not require authentication otherwise.
 ~~~~~~~~~~~~~~~~~~~~~

+### Enfore secure socket (SSL)


Perhaps we should link to some generally available guide which explains how to make an SSL certificate?

whisperity · 2017-09-12T09:44:40Z

tests/ssl_example_cert/cert.pem

@@ -0,0 +1,32 @@
+-----BEGIN CERTIFICATE-----


The shell actions that were used to make these test files, I think, should be saved to this folder in an .sh file too.

dkrupp requested review from Xazax-hun and csordasmarton September 11, 2017 20:05

dkrupp force-pushed the add_ssl_support branch from fd67988 to eeb4d81 Compare September 12, 2017 07:14

Xazax-hun suggested changes Sep 12, 2017

View reviewed changes

dkrupp force-pushed the add_ssl_support branch 3 times, most recently from 38b8933 to 3db75c1 Compare September 12, 2017 08:51

Xazax-hun approved these changes Sep 12, 2017

View reviewed changes

dkrupp force-pushed the add_ssl_support branch from 3db75c1 to bf23cc0 Compare September 12, 2017 09:02

whisperity added API change 📄 Content of patch changes API! GUI 🎨 labels Sep 12, 2017

dkrupp force-pushed the add_ssl_support branch from bf23cc0 to 870221a Compare September 12, 2017 09:44

whisperity requested changes Sep 12, 2017

View reviewed changes

dkrupp force-pushed the add_ssl_support branch 4 times, most recently from 3a661b2 to e05e2af Compare September 12, 2017 12:34

dkrupp requested a review from whisperity September 12, 2017 12:52

dkrupp force-pushed the add_ssl_support branch 2 times, most recently from 10ec4b4 to 4bae654 Compare September 12, 2017 13:53

Adding SSL support to server and command line clients.

e143d12

dkrupp force-pushed the add_ssl_support branch from 4bae654 to e143d12 Compare September 12, 2017 15:22

csordasmarton merged commit d471e20 into Ericsson:master Sep 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Adding SSL support to server and command line clients. #899

Adding SSL support to server and command line clients. #899

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Adding SSL support to server and command line clients. #899

Adding SSL support to server and command line clients. #899

Uh oh!

Conversation

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!

Uh oh!