Add CA file option for TLS connection #42
Conversation
|
🎉 🎉 🎉 🎉 🎉 🎉 Congrats for your pull request :) I believe the code it is very reasonable. Let's wait for feedback from brocaar and we will move from there :) |
backend/mqttpubsub/backend.go
Outdated
| opts.SetConnectionLostHandler(b.onConnectionLost) | ||
|
|
||
|
|
||
| if len(cafile) != 0 { |
There was a problem hiding this comment.
Alternatively you could also put if cafile != ""
backend/mqttpubsub/backend.go
Outdated
|
|
||
| if len(cafile) != 0 { | ||
| tlsconfig, err := NewTLSConfig(cafile) | ||
| if(err == nil) { |
There was a problem hiding this comment.
No need to put () around the err == nil
backend/mqttpubsub/backend.go
Outdated
| if len(cafile) != 0 { | ||
| tlsconfig, err := NewTLSConfig(cafile) | ||
| if(err == nil) { | ||
| opts.SetClientID("ssl-client").SetTLSConfig(tlsconfig) |
There was a problem hiding this comment.
Is there a specific reason to use the SetClientID? You might run into trouble when you have multiple LoRa Gateway Bridge instances. I think only one connection per client ID is allowed.
There was a problem hiding this comment.
@brocaar, you are perfectly right on this.
However just for let you know some MQTT server allow load balacing based on the clientID, it is quite a nice feature.
cmd/lora-gateway-bridge/main.go
Outdated
| EnvVar: "MQTT_PASSWORD", | ||
| }, | ||
| cli.StringFlag{ | ||
| Name: "cafile", |
There was a problem hiding this comment.
Maybe it would be better to keep this specific to MQTT, thus mqtt-cafile MQTT_CAFILE? Or mqtt-ca-cert and MQTT_CA_CERT (to keep the naming consistent with the LoRa Server / LoRa App Server config flags).
|
Thanks! I have added a few comments (just a few consistency related comments). Would you be able make these changes? Then I think it is good to merge :-) |
cmd/lora-gateway-bridge/doc.go
Outdated
| --mqtt-username MQTT username [$MQTT_USERNAME] | ||
| --mqtt-password MQTT password [$MQTT_PASSWORD] | ||
| --cafile CA certificate file [$CAFILE] | ||
| --mqtt-ca-cert CA certificate file [$CAFILE] |
| EnvVar: "CAFILE", | ||
| Name: "mqtt-ca-cert", | ||
| Usage: "mqtt CA certificate file (optional)", | ||
| EnvVar: "MQTT_CA_CERT", |
|
OT |
|
@brocaar , do you want me to add this change to the other loraserver and the lora-app-server too? |
|
If you would like to do so, that would be great! |
|
This is working on my Conduit Gateways. Thank you! It would be great if lora-gateway-bridge could support client certificate authentication with MQTT as well. |
|
@nickumbe , it doesn't look too difficult, why you don't try to submit a pull request? Look at the previous thread about this issues: #37 You see how minggi was supported? If you are interested in the feature the simplest thing you can do is to implement it yourself, ask feedback, fix whatever need to be fixed and finally get it merged. Bonus point, after the first PR is accepted it is simpler to implement it in other parts of the architecture: brocaar/chirpstack-network-server#190 |
4 participants
No description provided.