Support MQTT certificate and the key for the gateway backend #201

moznion · 2018-01-17T10:38:35Z

brocaar/chirpstack-network-server#284 is the same motivation.

brocaar

Thanks for your work!

I have a couple of comments to make these config options consistent with the other TLS related config options.

Could you update these and apply these changes to to lora-gateway-bridge and loraserver? Then these changes should be good to merge :-)

brocaar · 2018-01-22T11:15:56Z

docs/content/install/config.md

@@ -20,6 +20,8 @@ GLOBAL OPTIONS:
   --mqtt-username value            mqtt server username (optional) [$MQTT_USERNAME]
   --mqtt-password value            mqtt server password (optional) [$MQTT_PASSWORD]
   --mqtt-ca-cert value             mqtt CA certificate file used by the gateway backend (optional) [$MQTT_CA_CERT]
+   --mqtt-cert value                mqtt certificate file used by the gateway backend (optional) [$MQTT_CERT]


See previous comment (--mqtt-tls-cert and --mqtt-tls-key)

brocaar · 2018-01-22T11:18:01Z

internal/handler/mqtthandler/mqtt_handler.go

-		}
+	tlsconfig, err := newTLSConfig(cafile, certFile, certKeyFile)
+	if err != nil {
+		log.Fatalf("Error with the mqtt CA certificate: %s", err)


Maybe this would print more debugging information:

log.WithError(err).WithFields(log.Fields{ "ca_cert": cafile, "tls_cert": certFile, "tls_key": certKeyFile, }).Fatal("error loading mqtt certificate files")

brocaar · 2018-01-22T11:21:37Z

internal/handler/mqtthandler/mqtt_handler.go

@@ -70,23 +69,47 @@ func NewHandler(server, username, password, cafile string) (handler.Handler, err
 	return &h, nil
 }

-func newTLSConfig(cafile string) (*tls.Config, error) {
+func newTLSConfig(cafile, certFile, certKeyFile string) (*tls.Config, error) {
+	if cafile == "" && (certFile == "" || certKeyFile == "") {


I think there are three valid options:

Only CA

TLS cert + key

CA, TLS + key

If in this case the CA cert is set + only the TLS cert or TLS key, it would pass this check, but the config is invalid I believe.

brocaar · 2018-01-22T11:24:12Z

internal/handler/mqtthandler/mqtt_handler.go

+			return nil, err
+		}
+
+		kp, err := tls.X509KeyPair(cert, certKey)


Please use https://golang.org/pkg/crypto/tls/#LoadX509KeyPair. It allows you to directly load the files by passing the paths to these files. Saves a couple of lines ;-)

Fix brocaar#201 (comment) Fix brocaar#201 (comment)

Fix brocaar#201 (comment)

moznion · 2018-01-23T08:33:56Z

@brocaar
Thank you for your reviewing! I've just corrected the pointed out.
If you seem this change is good, I will apply the same patch to other components.

But Travis CI is failing... I wonder why. Can it be rebuilt?

moznion · 2018-01-23T13:41:12Z

After rebuilding, fixed it :)

moznion · 2018-01-26T09:13:00Z

@brocaar How about this?

brocaar

Thanks for also adding tests 👍

I've added some small comments (I think you might have missed some renames). Let me know what you think.

brocaar · 2018-01-26T13:25:17Z

cmd/lora-app-server/main.go

@@ -174,7 +174,7 @@ func setRedisPool(c *cli.Context) error {
 }

 func setHandler(c *cli.Context) error {
-	h, err := mqtthandler.NewHandler(c.String("mqtt-server"), c.String("mqtt-username"), c.String("mqtt-password"), c.String("mqtt-ca-cert"))
+	h, err := mqtthandler.NewHandler(c.String("mqtt-server"), c.String("mqtt-username"), c.String("mqtt-password"), c.String("mqtt-ca-cert"), c.String("mqtt-cert"), c.String("mqtt-cert-key"))


c.String("mqtt-cert"), c.String("mqtt-cert-key")

That should be mqtt-tls-cert an mqtt-tls-key I think?

Uh oh, I missed... Sorry.

brocaar · 2018-01-26T13:26:14Z

docs/content/install/config.md

@@ -20,6 +20,8 @@ GLOBAL OPTIONS:
   --mqtt-username value            mqtt server username (optional) [$MQTT_USERNAME]
   --mqtt-password value            mqtt server password (optional) [$MQTT_PASSWORD]
   --mqtt-ca-cert value             mqtt CA certificate file used by the gateway backend (optional) [$MQTT_CA_CERT]
+   --mqtt-tls-cert value            mqtt certificate file used by the gateway backend (optional) [$MQTT_CERT]


$MQTT_CERT --> $MQTT_TLS_CERT, same for $MQTT_CERT_KEY.

brocaar · 2018-01-26T13:33:21Z

internal/handler/mqtthandler/mqtt_handler.go

@@ -70,23 +75,57 @@ func NewHandler(server, username, password, cafile string) (handler.Handler, err
 	return &h, nil
 }

-func newTLSConfig(cafile string) (*tls.Config, error) {
+func newTLSConfig(cafile, certFile, certKeyFile string) (*tls.Config, error) {
+	// Here are three valid options:


Sorry for commenting on this again, but maybe it is better to remove the whole check at all. E.g. when the user does only provide a tls cert or only a tls key, tls.LoadX509KeyPair(certFile, certKeyFile) will also handle this as an error.

Fix: brocaar#201 (comment) Fix: brocaar#201 (comment)

Fix brocaar#201 (comment)

moznion · 2018-01-28T15:27:02Z

I'm sorry for my late reply (I was lost the Internet connection...).

I fixed some typos and redundant MQTT TLS argument validation. Could you review this patch again?

brocaar · 2018-01-29T07:42:29Z

Thanks for your contribution! I've merged your pull-request :-)

moznion · 2018-01-29T07:53:45Z

Thank you so much for your merging!
May I apply the same patch to loraserver and gateway-bridge?

brocaar · 2018-01-29T07:54:33Z

Yes, please do! That would be much appreciated :-)

moznion · 2018-01-29T07:55:33Z

Sure, I'll do that!

Support MQTT certificate and the key for the gateway backend

e7937d8

moznion force-pushed the support_mqtt_cert branch from ebbb626 to e7937d8 Compare January 17, 2018 10:58

brocaar reviewed Jan 22, 2018

View reviewed changes

moznion added 4 commits January 23, 2018 15:39

Rename option name of MQTT cert and the key to keeping consistency

a3d9e5d

Fix brocaar#201 (comment) Fix brocaar#201 (comment)

Change MQTT TLS configuration error message to be more debuggable

fd40a56

Fix brocaar#201 (comment)

Use LoadX509KeyPair instead of manually procedure, simplify

0ebc1f4

Fix brocaar#201 (comment)

Fix weird TLS config validation condition

2f57c4b

Fix brocaar#201 (comment)

Refactor MQTT TLS configuration validation

aa84f53

brocaar reviewed Jan 26, 2018

View reviewed changes

moznion added 2 commits January 29, 2018 00:16

Fix typos

35e026e

Fix: brocaar#201 (comment) Fix: brocaar#201 (comment)

Remove MQTT TLS argument validation

0fd156c

Fix brocaar#201 (comment)

brocaar merged commit cce7403 into brocaar:master Jan 29, 2018

moznion deleted the support_mqtt_cert branch January 30, 2018 01:47

This was referenced Jan 30, 2018

Remove garbage files that are no longer used #207

Merged

Fix a discord between CLI arg and envvar of MQTT cert/key #209

Closed

brocaar added a commit that referenced this pull request Feb 8, 2018

Update changelog with #201.

708d68a

brocaar added a commit that referenced this pull request Feb 8, 2018

Update changelog with #201.

0df3e84

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support MQTT certificate and the key for the gateway backend #201

Support MQTT certificate and the key for the gateway backend #201

moznion commented Jan 17, 2018

brocaar left a comment

brocaar Jan 22, 2018

brocaar Jan 22, 2018

brocaar Jan 22, 2018

brocaar Jan 22, 2018

moznion commented Jan 23, 2018 •

edited

moznion commented Jan 23, 2018

moznion commented Jan 26, 2018

brocaar left a comment

brocaar Jan 26, 2018

moznion Jan 28, 2018

brocaar Jan 26, 2018

brocaar Jan 26, 2018 •

edited

moznion commented Jan 28, 2018

brocaar commented Jan 29, 2018

moznion commented Jan 29, 2018

brocaar commented Jan 29, 2018

moznion commented Jan 29, 2018

Support MQTT certificate and the key for the gateway backend #201

Support MQTT certificate and the key for the gateway backend #201

Conversation

moznion commented Jan 17, 2018

brocaar left a comment

Choose a reason for hiding this comment

brocaar Jan 22, 2018

Choose a reason for hiding this comment

brocaar Jan 22, 2018

Choose a reason for hiding this comment

brocaar Jan 22, 2018

Choose a reason for hiding this comment

brocaar Jan 22, 2018

Choose a reason for hiding this comment

moznion commented Jan 23, 2018 • edited

moznion commented Jan 23, 2018

moznion commented Jan 26, 2018

brocaar left a comment

Choose a reason for hiding this comment

brocaar Jan 26, 2018

Choose a reason for hiding this comment

moznion Jan 28, 2018

Choose a reason for hiding this comment

brocaar Jan 26, 2018

Choose a reason for hiding this comment

brocaar Jan 26, 2018 • edited

Choose a reason for hiding this comment

moznion commented Jan 28, 2018

brocaar commented Jan 29, 2018

moznion commented Jan 29, 2018

brocaar commented Jan 29, 2018

moznion commented Jan 29, 2018

moznion commented Jan 23, 2018 •

edited

brocaar Jan 26, 2018 •

edited