chirpstack / chirpstack-gateway-bridge

ChirpStack Gateway Bridge abstracts Packet Forwarder protocols into Protobuf or JSON over MQTT.
https://www.chirpstack.io
MIT License
425 stars 274 forks source link

Location configuration for the TLS CA certificate #37

Closed minggi closed 7 years ago

minggi commented 7 years ago

I try to install the lora-gateway-bridge on the gateway including MQTT TLS.
How can I configure the location configuration for the TLS CA certificate?

siscia commented 7 years ago

Unfortunately is not possible at the moment.

There is a quite interesting workaround. Set up a private MQTT server between your bridges and your backend. This doesn't quite solve the problem but it may be enough for your use case.

I believe it can be added, if you really need it either implement it or ping me.

minggi commented 7 years ago

Is there another way do open the secure connection?

But it makes definitly sense for us to implement it. Is it a big deal?

siscia commented 7 years ago

Not that I am aware.

I guess that you could set up a secure tunnel between your MQTT server and your client... But it kinda a hack around...

No, it is not a big deal, but it takes time.

Feel free to submit a pull request.

I guess Brocaar himself could provide you with some consulting time, but I don't know if he is busy already. If he can't I could do it for you, but again it takes time.

(disclaimer, I am just a random stranger on the internet that happens to use this software, I am not a maintainer of the project)

----- Messaggio originale ----- Da: "minggi" notifications@github.com Inviato: ‎14/‎06/‎2017 10:11 A: "brocaar/lora-gateway-bridge" lora-gateway-bridge@noreply.github.com Cc: "Simone Mosciatti" sisciaprivate@gmail.com; "Comment" comment@noreply.github.com Oggetto: Re: [brocaar/lora-gateway-bridge] Location configuration for the TLSCA certificate (#37)

Is there another way do open the secure connection? But it makes definitly sense for us to implement it. Is it a big deal? — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

minggi commented 7 years ago

I will add a global option like --cafile and try to program it.

minggi commented 7 years ago

Question: Can I not only add manually the CA certificates to default openssl CA bundle?

siscia commented 7 years ago

@minggi , didin't quite understood what you are asking.

Overall your goal should be to provide a tls.Config at the MQTT config. https://godoc.org/github.com/eclipse/paho.mqtt.golang#ClientOptions.SetTLSConfig

That should be added somewhere here: https://github.com/brocaar/lora-gateway-bridge/blob/master/backend/mqttpubsub/backend.go#L30

Cheers,

minggi commented 7 years ago

I have the code for the TLS / CA file and it works fine. Next step: create a branche. I will try... but it's my first attempt.

siscia commented 7 years ago

@minggi , first of all, congratulation for your first pull request :tada:

Now, let's analyze a little bit your 2 pull request.

The first issue is that are two different pull requests, a PR usually add a functionality and should be an atomic change.

Then if you look carefully you can see, below your pull request a messages indicating that all the check fails. Most likely this is due to the fact that your functionality breaks the previous interface. NewBackend now required 4 arguments, not 3 anymore.

What you should do is to modify the tests that use the old interface, the one with 3 arguments, to accept your extra argument.

Finally a well done full request should include unit test so that the project maintain a reasonable test coverage.

Then a side note on your golang style. In most cases functions returns two arguments, one is the actual value created, *tls.Config in our case and the other is an error. If everything goes well the error is set to nil and the object returned is good to use. If something goes wrong the error itself should be set to something different to nil and indicate what was the problem; the object returned may or may not be good.

Also, wait for some feedback from @brocaar , at the end of the day he is the one merging or not your request. Apart from the problem that I highlighted above your pull request seems fine to me.

Good job :+1:

minggi commented 7 years ago

@siscia tanks for your input

I'm working now on a better change. One problem: How can I test the TLS stuff. :-( This unit test is not really easy to programming. Any examples or ideas?

The rest of the code is ready and the unit test was sucessfull.

siscia commented 7 years ago

I do agree that it is quite complex.

Personally, in this case, I wouldn't bother with a test, but that is me so you should wait for feedback from @brocaar

You could generate the certificate in the makefile, but this assume that the machine running the test has the actual software to generate the certificates and that the tests are run using the Makefile.

I am not really into the TLS and certificate stuff so I may be completely off.

However, I will start making a pull request, so that we can be sure that the code is ok.

Then we will move into figure out how to test the feature you add.

On github, if you make a pull request from a branch and then you commit again on that same branch the new commit will be included in the pull request.

minggi commented 7 years ago

42 Pull request done

brocaar commented 7 years ago

Thanks for your work. I've merged your pull-request. This will be included in the next release :-)