MF-651 - X509 Mutual TLS authentication #676

dborovcanin · 2019-03-28T12:09:00Z

What does this do?

This pull request introduces a new authorization method using x.509 certificates.

Which issue(s) does this PR fix/relate to?

This pull request resolves #651.

List any changes that modify/break current functionality

In order to establish a connection and send a message, Thing needs to provide a valid client certificate. The default configuration is unchanged and by default, running Docker composition does not include mutual authentication.

Have you included tests for your changes?

I have tested all the features manually. However, there are no automated tests provided.

Did you document any new/modified functionality?

Yes, I have.

codecov-io · 2019-03-28T12:24:11Z

Codecov Report

Merging #676 into master will increase coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #676      +/-   ##
==========================================
+ Coverage   86.29%   86.35%   +0.05%     
==========================================
  Files          62       62              
  Lines        3722     3722              
==========================================
+ Hits         3212     3214       +2     
+ Misses        353      352       -1     
+ Partials      157      156       -1

Impacted Files	Coverage Δ
writers/cassandra/init.go	`80% <0%> (+20%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc49f6a...e539f2c. Read the comment docs.

drasko · 2019-03-28T22:52:38Z

docker/nginx/nginx-key.conf

        listen 443 ssl http2 default_server;
        listen [::]:443 ssl http2 default_server;

-        #


Not sure that I like removing useful info

drasko · 2019-03-28T22:53:13Z

docker/nginx/nginx-key.conf

@@ -201,29 +147,6 @@ http {
            }
        }

-        # Proxy pass to mainflux-mqtt-adapter
-        location /mqtt {


Are you removing MQTT-over-WS support?

drasko · 2019-03-28T22:53:48Z

docker/nginx/nginx-key.conf

@@ -242,23 +165,17 @@ http {
    }
 }

-# MQTT


Why removing this?

drasko · 2019-03-28T22:55:20Z

docker/ssl/Makefile

+
+ca:
+	openssl req -newkey rsa:2048 -x509 -nodes -sha512 -days 1095 \
+				-keyout $(CRT_LOCATION)/ca.key -out $(CRT_LOCATION)/ca.crt -subj "/CN=localhost/O=Mainflux/OU=IoT/emailAddress=info@mainflux.com"


Please use global vars here if you defined them.

All of this should not go into Makefile IMHO.

Rather make a spearate script that can be used independently of Makefile, than call it from Makefile with appropriate parameter.

Makefiles are not very readable and can be easily polluted.

I don't like this Makefile, either. However, I find it extremely simple to use it as a wrapper around OpenSSL. It's easier and more convenient than a simple shell script. Do you have any suggestion? We can completely remove it and describe openssl commands in docs, but for the user, it's easier to use make something than to run complex openssl commands.

OK to keep it.

drasko · 2019-03-28T22:59:17Z

docker/ssl/access.js

+
+function access(s) {
+    s.on('upload', function (data) {
+        while (data == '') {


All of this should happen only if the cert has been verified/valid.

Since we use ssl_verify_client on;, NginX will put a validity flag in a certain internal var, and this must be checked.

I still do not see where you check this var for MQTT. This should be probably checked somewhere in the script (at least it is like this in Lua).

docker/ssl/access.js

drasko · 2019-03-28T23:02:14Z

docker/ssl/access.js

+    return '';
+}
+
+function setKey(r) {


Add info that this is for HTTP and WS only.

mkdocs.yml

docker/ssl/access.js

docker/docker-compose.yml

docker/nginx/nginx-key.conf

drasko · 2019-03-29T18:15:54Z

docker/ssl/authorization.js

+	    |                        PROTOCOL NAME                        |    
+	    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-|
+	    |    VERSION   |     FLAGS     |          KEEP ALIVE          | 
+        +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-|


PLease align

docker/ssl/authorization.js

drasko · 2019-03-29T18:25:37Z

docker/ssl/authorization.js

+        This method extracts Password field.
+	*/
+
+    var packet_type_flags_byte = data.codePointAt(0);


Looks like you started parsing packet flags fields without even checking if the message type is CONNECT. If it is not CONNECT, we should exit right after this check of a first byte.

drasko · 2019-03-30T01:56:50Z

docker/nginx/nginx-x509.conf

@@ -0,0 +1,256 @@
+##


No need for double ##. One is enough.

drasko · 2019-03-30T02:10:18Z

docker/ssl/Makefile

+
+ca:
+	openssl req -newkey rsa:2048 -x509 -nodes -sha512 -days 1095 \
+				-keyout $(CRT_LOCATION)/ca.key -out $(CRT_LOCATION)/ca.crt -subj "/CN=localhost/O=Mainflux/OU=IoT/emailAddress=info@mainflux.com"


OK to keep it.

drasko · 2019-03-30T02:10:57Z

docker/ssl/authorization.js

+
+    s.on('upload', function (data) {
+        while (data == '') {
+            return s.AGAIN


Do we really need this s.AGAIN - nginx/njs#111 (comment)

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

nmarcetic · 2019-04-02T13:44:56Z

docker/nginx/nginx-key.conf

-    # SSL Settings
-    ##
-    ssl_protocols TLSv1 TLSv1.1 TLSv1.2; # Dropping SSLv3, ref: POODLE
+    ssl_protocols TLSv1 TLSv1.1 TLSv1.2 TLSv1.3;


Maybe is a good idea to disable TLS 1.0 and 1.1, there are few well-known vulnerabilities, it's not secure and even newer versions of browsers removed support. TLS 1.3 is preferred but we can keep TLS 1.2 also (a lot of peoples still use it).

nmarcetic · 2019-04-02T13:45:29Z

docker/nginx/nginx-key.conf

-        # and https://raymii.org/s/tutorials/Strong_SSL_Security_On_nginx.html
-
-        ssl_protocols TLSv1 TLSv1.1 TLSv1.2;
+        ssl_protocols TLSv1 TLSv1.1 TLSv1.2 TLSv1.3;


Same question for TLS 1.0 1.1

nmarcetic · 2019-04-02T13:45:49Z

docker/nginx/nginx-key.conf

-        # and https://raymii.org/s/tutorials/Strong_SSL_Security_On_nginx.html
-
-        ssl_protocols TLSv1 TLSv1.1 TLSv1.2;
+        ssl_protocols TLSv1 TLSv1.1 TLSv1.2 TLSv1.3;


Same for TLS version

nmarcetic · 2019-04-02T13:46:14Z

docker/nginx/nginx-x509.conf

+    include /etc/nginx/mime.types;
+    default_type application/octet-stream;
+
+    ssl_protocols TLSv1 TLSv1.1 TLSv1.2 TLSv1.3;


TLS version

docker/nginx/nginx-x509.conf

nmarcetic · 2019-04-02T13:49:42Z

docker/nginx/nginx-x509.conf

+        add_header Access-Control-Allow-Methods '*';
+        add_header Access-Control-Allow-Headers '*';
+
+        server_name localhost;


Maybe also think to put this as ENV var so I can use custom domain easy.

nmarcetic · 2019-04-02T13:53:13Z

client.csr

@@ -0,0 +1,28 @@
+-----BEGIN CERTIFICATE REQUEST-----


Where this file is located? client.csr looks like it's in the project root?

nmarcetic · 2019-04-02T13:53:26Z

client.key

@@ -0,0 +1,51 @@
+-----BEGIN RSA PRIVATE KEY-----


Same question for location of the file.

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

docker/nginx/nginx-x509.conf

drasko

LGTM

drasko · 2019-04-02T15:54:46Z

Merged! Great work @dusanb94, very important feature!

* Use NginX njs module for mutual authentication Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Add Makefile for cert management Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Move certificates make context to scripts dir Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Move nginx.conf to separate directory Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Choose between two NginX configurations Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Move certs Makefile to docker/ssl/ Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Use default key-based authentication Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Add mTLS docs Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Update Makefile Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Add check if Authorization is present Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Add check if Will Flag is 1 Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Return MQTT over WS Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Fix docker-compose.yml volume mapping Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Rename security section in docs Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Add message type check before message parsing Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Remove double comments Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Remove s.AGAIN in return Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Update Makefile Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Remove CSR and key from the root Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Drop TLS version below 1.2 Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com> * Add comments for cert and key paths Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

dborovcanin force-pushed the MF-651 branch from 6c0c8bc to 44c7f63 Compare March 28, 2019 12:10

dborovcanin force-pushed the MF-651 branch 2 times, most recently from 86f04a8 to 4cc8ac7 Compare March 28, 2019 16:14

drasko requested changes Mar 28, 2019

View reviewed changes

drasko requested changes Mar 29, 2019

View reviewed changes

dborovcanin force-pushed the MF-651 branch 4 times, most recently from 2e8865b to 4a966e9 Compare March 29, 2019 20:07

dborovcanin marked this pull request as ready for review March 29, 2019 20:08

dborovcanin requested a review from drasko March 29, 2019 20:08

drasko requested changes Mar 30, 2019

View reviewed changes

docker/nginx/nginx-x509.conf Outdated

@@ -0,0 +1,256 @@

##

Copy link

Contributor

drasko Mar 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for double ##. One is enough.

drasko requested changes Mar 30, 2019

View reviewed changes

drasko mentioned this pull request Mar 30, 2019

MF-388 Helm Charts for Mainflux #550

Closed

dborovcanin force-pushed the MF-651 branch from dc25935 to 881544e Compare April 1, 2019 09:28

dborovcanin added 14 commits April 2, 2019 09:39

Use NginX njs module for mutual authentication

031c4f8

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Add Makefile for cert management

cb14f59

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Move certificates make context to scripts dir

7a6f770

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Move nginx.conf to separate directory

2668bd4

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Choose between two NginX configurations

a8720d8

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Move certs Makefile to docker/ssl/

8ed567b

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Use default key-based authentication

3afe5ae

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Add mTLS docs

779def9

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Update Makefile

6699bec

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Add check if Authorization is present

5f38919

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Add check if Will Flag is 1

c7437c0

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Return MQTT over WS

ecd1b8b

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Fix docker-compose.yml volume mapping

ece2f48

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Rename security section in docs

e8cefe6

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

dborovcanin added 4 commits April 2, 2019 09:39

Add message type check before message parsing

5a6fba6

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Remove double comments

83b56b5

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Remove s.AGAIN in return

5cf7bb4

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Update Makefile

36346dd

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

dborovcanin force-pushed the MF-651 branch from 881544e to 36346dd Compare April 2, 2019 07:39

nmarcetic requested changes Apr 2, 2019

View reviewed changes

dborovcanin added 3 commits April 2, 2019 16:16

Remove CSR and key from the root

de0aa4e

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Drop TLS version below 1.2

0599250

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

Add comments for cert and key paths

e539f2c

Signed-off-by: Dušan Borovčanin <dusan.borovcanin@mainflux.com>

nmarcetic approved these changes Apr 2, 2019

View reviewed changes

docker/nginx/nginx-x509.conf Show resolved Hide resolved

drasko approved these changes Apr 2, 2019

View reviewed changes

drasko merged commit f9b17d5 into absmach:master Apr 2, 2019

dborovcanin deleted the MF-651 branch September 30, 2019 12:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MF-651 - X509 Mutual TLS authentication #676

MF-651 - X509 Mutual TLS authentication #676

dborovcanin commented Mar 28, 2019

codecov-io commented Mar 28, 2019 •

edited

Loading

drasko Mar 28, 2019

drasko Mar 28, 2019

drasko Mar 28, 2019

drasko Mar 28, 2019

drasko Mar 28, 2019

dborovcanin Mar 29, 2019

drasko Mar 30, 2019

drasko Mar 28, 2019

drasko Mar 30, 2019

drasko Mar 28, 2019

drasko Mar 29, 2019

drasko Mar 29, 2019 •

edited

Loading

drasko Mar 30, 2019

drasko Mar 30, 2019

drasko Mar 30, 2019

nmarcetic Apr 2, 2019

nmarcetic Apr 2, 2019

nmarcetic Apr 2, 2019

nmarcetic Apr 2, 2019

nmarcetic Apr 2, 2019

nmarcetic Apr 2, 2019

nmarcetic Apr 2, 2019

drasko left a comment

drasko commented Apr 2, 2019

@@ @@ -242,23 +165,17 @@ http { @@
                   }
               }
-              # MQTT

MF-651 - X509 Mutual TLS authentication #676

MF-651 - X509 Mutual TLS authentication #676

Conversation

dborovcanin commented Mar 28, 2019

What does this do?

Which issue(s) does this PR fix/relate to?

List any changes that modify/break current functionality

Have you included tests for your changes?

Did you document any new/modified functionality?

codecov-io commented Mar 28, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drasko Mar 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drasko left a comment

Choose a reason for hiding this comment

drasko commented Apr 2, 2019

codecov-io commented Mar 28, 2019 •

edited

Loading

drasko Mar 29, 2019 •

edited

Loading