[CloudSync] Adding TLS to secure MQTT Client Broker Communication #465

nitu-s-gupta · 2022-02-02T10:19:03Z

Description

Certificates can be generated using openssl library and copied to /var/edge-orchestration/mqtt/certs folder. This will aloow the secure communication between mqtt broker and client.

Fixes # (#382)

Type of change

New feature (non-breaking change which adds functionality)

How Has This Been Tested?

1.MQTT mosquitto broker is configured to be running in the AWS endpoint. The certificates are copied to /etc/mosquitto/certs folder and corresponding path is specified in the configuration file
In the client side the root CA certificates are copied in the /var/edge-orchestration/mqtt/certs
2. The edge orchestration is build and run using following command with option CLOUD_SYNC set to true

docker run -it -d --privileged --network="host" --name edge-orchestration -e CLOUD_SYNC=true -v /var/edge-orchestration/:/var/edge-orchestration/:rw -v /var/run/docker.sock:/var/run/docker.sock:rw -v /proc/:/process/:ro  lfedge/edge-home-orchestration-go:latest

From the another terminal/post make a curl command as follows to publish data using home edge to the broker running on AWS endpoint

curl --location --request POST 'http://192.168.1.107:56001/api/v1/orchestration/cloudsyncmgr' \
--header 'Content-Type: text/plain' \
--data-raw '{
    "appid": "com.nokia.homeedge1",
    "payload": "{Another data from TV1 and testdata}",
    "topic": "home1/livingroom",
    "url" : "ec2-54-205-12-98.compute-1.amazonaws.com"
}'

Test Configuration:

OS type & version: (Ubuntu 20.04)
Hardware: (x86)
Edge Orchestration Release: (v1.1.0)

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings

Signed-off-by: Nitu Gupta <[email protected]>

[Cloudsync] Added testcases to improve the Code coverage for MQTT

tiokim

Thanks for the contribution! Please take a look at my comments.

internal/common/mqtt/mqttconfig.go

internal/controller/cloudsyncmgr/cloudsync.go

tiokim

PTAL!

internal/common/mqtt/mqttconfig.go

tiokim

LGTM Thanks!

MoonkiHong

+1

tdrozdovsky · 2022-02-08T06:34:43Z

internal/controller/cloudsyncmgr/cloudsync.go

@@ -65,6 +67,13 @@ func (c *CloudSyncImpl) InitiateCloudSync(isCloudSet string) (err error) {
 		if strings.Compare(strings.ToLower(isCloudSet), "true") == 0 {
 			log.Println("CloudSync init set")


Suggested change

log.Println("CloudSync init set")

log.Info(logPrefix, "CloudSync init set")

tdrozdovsky · 2022-02-08T06:35:16Z

internal/controller/cloudsyncmgr/cloudsync.go

+			secure := os.Getenv("SECURE")
+			if len(secure) > 0 {
+				if strings.Compare(strings.ToLower(secure), "true") == 0 {
+					log.Println(logPrefix, "Orchestration init with secure option")


Suggested change

log.Println(logPrefix, "Orchestration init with secure option")

log.Info(logPrefix, "Orchestration init with secure option")

tdrozdovsky · 2022-02-08T06:37:39Z

internal/controller/cloudsyncmgr/cloudsync.go

+			secure := os.Getenv("SECURE")
+			if len(secure) > 0 {
+				if strings.Compare(strings.ToLower(secure), "true") == 0 {
+					log.Println(logPrefix, "Orchestration init with secure option")


logPrefix = "[cloudsyncmgr]" change to logPrefix = "[cloudsyncmgr] "

tdrozdovsky · 2022-02-08T06:46:18Z

internal/common/mqtt/mqttconnection.go

@@ -115,7 +115,8 @@ func StartMQTTClient(brokerURL string, clientID string) string {
 		return err.Error()
 	}
 	clientConfig.ClientOptions.SetOnConnectHandler(clientConfig.onConnect())
-	URL := clientConfig.SetBrokerURL("tcp")
+	clientConfig.setProtocol()
+	URL := clientConfig.SetBrokerURL()
 	log.Info(logPrefix, " The broker is", URL)


Before logPrefix = "[MQTTConnectionMgr]" should be change to 1logPrefix = "[MQTTConnectionMgr] "`

Suggested change

log.Info(logPrefix, " The broker is", URL)

log.Info(logPrefix, "The broker is", URL)

tdrozdovsky · 2022-02-08T06:49:13Z

internal/common/mqtt/mqttconfig.go

+	certpool := x509.NewCertPool()
+	ca, err := ioutil.ReadFile(caCertConfig)
+	if err != nil {
+		log.Warn(err.Error())


Suggested change

log.Warn(err.Error())

log.Warn(logPrefix, err.Error())

tdrozdovsky · 2022-02-08T06:50:58Z

internal/common/mqtt/mqttconfig.go

+				return nil, err
+			}
+			copts.SetTLSConfig(tlsconfig)
+			// TODO Use Username and password to provide authorization to MQTT Broker zuncomment the below two lines to enable authorization after creating password file for each user


Suggested change

// TODO Use Username and password to provide authorization to MQTT Broker zuncomment the below two lines to enable authorization after creating password file for each user

// TODO: Use Username and Password to provide authorization to MQTT Broker uncomment the below two lines to enable authorization after creating password file for each user. This is temporary solution!

This is not a temporary solution. Its the solution as suggested by mosquitto broker. So its user dependent and hence commented. I can uncomment and put it but then what will be the password set by the user and username hence it is user dependent and also its one of the secure modes of communication as provided by Mosquitto broker as mentioned in the documentation

login and password cannot be stored in the program and must be exported from a secure storage

more suitable option is a signed certificate with a fixed SAN (we had difficulties with this, but I will investigate this issue in more detail later)
So I think this is a temporary solution.

Yes , it should be ideally but you have to pass the password through copts only thats the architecture design of Mosquitto broker . Secondly Certificates through SAN can give you secure communication channel which is already done. The issue was resolved by me. There is no issue in it. So I think there is some confusion from your side. SAN certificates were fine and are working fine now. As already mentioned in previous PR also the issue was resolved and it wasnot certificates issue.
Storing certificate at any location wouldnt change passing of password through copts only the path will change. SO i disagree with your point of this being a temporary solution.

As I wrote above, this is a temporary solution, since the password cannot be stored in the program.

We can take it step by step

while we consider the option of communication with only one broker

we have two options: a) add the phrase that this is a temporary solution, accept this request, and urgently prepare to read the login and password from an external file; b) we finish reading the login and password from an external file or from another place

If accepted in the current version: we need remove the OpenSSF "passing" and "silver" badge from our project (because it violates security requirements ), and also the container that is downloaded from dockerhub will not work fully.

I'm more concerned about the unprotected storage password in the code (hardcoded). And as I said earlier, we can implement the issues of secure interaction with the broker later

We need to fix the secure location for password file storage . We can discuss that separately. And this file reading should not take much time. So once we fix the file location we can raise PR for that. @nitu-s-gupta : Please remove the lines..

@nitu-s-gupta Please create an issue to discuss about cert and password.

@nitu-s-gupta Please create an issue to discuss about cert and password.

#471

suresh-lc

+1

Signed-off-by: Nitu Gupta <[email protected]>

sonarcloud · 2022-02-08T10:46:21Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

tdrozdovsky

LGTM

nitu-s-gupta and others added 3 commits January 31, 2022 11:57

[Cloudsync] Added testcases to improve the Code coverage

123cf4b

Signed-off-by: Nitu Gupta <[email protected]>

Merge pull request lf-edge#458 from nitu-s-gupta/coverage

5f3b330

[Cloudsync] Added testcases to improve the Code coverage for MQTT

Merge branch 'lf-edge:master' into master

0600306

nitu-s-gupta requested review from MoonkiHong, suresh-lc, tiokim and tdrozdovsky as code owners February 2, 2022 10:19

boring-cyborg bot added base code mqtt labels Feb 2, 2022

nitu-s-gupta changed the title ~~Secure~~ [CloudSync] Adding TLS to secure MQTT Client Broker Communication Feb 2, 2022

tiokim reviewed Feb 3, 2022

View reviewed changes

internal/common/mqtt/mqttconfig.go Outdated Show resolved Hide resolved

internal/controller/cloudsyncmgr/cloudsync.go Outdated Show resolved Hide resolved

nitu-s-gupta force-pushed the secure branch from 29cb4cc to 3c7bf28 Compare February 3, 2022 09:06

tiokim reviewed Feb 3, 2022

View reviewed changes

internal/common/mqtt/mqttconfig.go Show resolved Hide resolved

internal/common/mqtt/mqttconfig.go Outdated Show resolved Hide resolved

internal/common/mqtt/mqttconfig.go Outdated Show resolved Hide resolved

nitu-s-gupta force-pushed the secure branch 2 times, most recently from b756d1a to 28ab1f0 Compare February 4, 2022 07:12

tiokim approved these changes Feb 8, 2022

View reviewed changes

MoonkiHong approved these changes Feb 8, 2022

View reviewed changes

tdrozdovsky requested changes Feb 8, 2022

View reviewed changes

nitu-s-gupta force-pushed the secure branch from 25d7701 to b9dd1d7 Compare February 8, 2022 08:15

suresh-lc approved these changes Feb 8, 2022

View reviewed changes

[CloudSync] Adding TLS for Secure MQTT Broker Client Communication

e0e9e7d

Signed-off-by: Nitu Gupta <[email protected]>

nitu-s-gupta force-pushed the secure branch from b9dd1d7 to e0e9e7d Compare February 8, 2022 10:45

tdrozdovsky approved these changes Feb 8, 2022

View reviewed changes

MoonkiHong merged commit b71bb98 into lf-edge:master Feb 8, 2022

nitu-s-gupta deleted the secure branch February 24, 2022 08:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CloudSync] Adding TLS to secure MQTT Client Broker Communication #465

[CloudSync] Adding TLS to secure MQTT Client Broker Communication #465

nitu-s-gupta commented Feb 2, 2022

tiokim left a comment

tiokim left a comment

tiokim left a comment

MoonkiHong left a comment

tdrozdovsky Feb 8, 2022

nitu-s-gupta Feb 8, 2022

tdrozdovsky Feb 8, 2022

nitu-s-gupta Feb 8, 2022

tdrozdovsky Feb 8, 2022

nitu-s-gupta Feb 8, 2022

tdrozdovsky Feb 8, 2022

nitu-s-gupta Feb 8, 2022

tdrozdovsky Feb 8, 2022

nitu-s-gupta Feb 8, 2022

tdrozdovsky Feb 8, 2022

nitu-s-gupta Feb 8, 2022 •

edited

Loading

tdrozdovsky Feb 8, 2022

nitu-s-gupta Feb 8, 2022 •

edited

Loading

tdrozdovsky Feb 8, 2022

tdrozdovsky Feb 8, 2022

tdrozdovsky Feb 8, 2022

suresh-lc Feb 8, 2022

tiokim Feb 8, 2022

suresh-lc Feb 8, 2022

suresh-lc left a comment

sonarcloud bot commented Feb 8, 2022

tdrozdovsky left a comment

		@@ -65,6 +67,13 @@ func (c *CloudSyncImpl) InitiateCloudSync(isCloudSet string) (err error) {
		if strings.Compare(strings.ToLower(isCloudSet), "true") == 0 {
		log.Println("CloudSync init set")

	log.Println("CloudSync init set")
	log.Info(logPrefix, "CloudSync init set")

	log.Println(logPrefix, "Orchestration init with secure option")
	log.Info(logPrefix, "Orchestration init with secure option")

	log.Info(logPrefix, " The broker is", URL)
	log.Info(logPrefix, "The broker is", URL)

	// TODO Use Username and password to provide authorization to MQTT Broker zuncomment the below two lines to enable authorization after creating password file for each user
	// TODO: Use Username and Password to provide authorization to MQTT Broker uncomment the below two lines to enable authorization after creating password file for each user. This is temporary solution!

[CloudSync] Adding TLS to secure MQTT Client Broker Communication #465

[CloudSync] Adding TLS to secure MQTT Client Broker Communication #465

Conversation

nitu-s-gupta commented Feb 2, 2022

Description

Type of change

How Has This Been Tested?

Checklist:

tiokim left a comment

Choose a reason for hiding this comment

tiokim left a comment

Choose a reason for hiding this comment

tiokim left a comment

Choose a reason for hiding this comment

MoonkiHong left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitu-s-gupta Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitu-s-gupta Feb 8, 2022 • 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

suresh-lc left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Feb 8, 2022

tdrozdovsky left a comment

Choose a reason for hiding this comment

nitu-s-gupta Feb 8, 2022 •

edited

Loading

nitu-s-gupta Feb 8, 2022 •

edited

Loading