-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mosquitto Example #76
Conversation
Signed-off-by: fdeantoni <[email protected]>
Signed-off-by: fdeantoni <[email protected]>
Thanks @fdeantoni for submitting this. Im not familiar with mosquitto, but i will review for other correctness. |
examples/mosquitto/README.md
Outdated
|
||
## Guide | ||
|
||
### Preqrequisites |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
### Preqrequisites | |
### Prerequisites |
examples/mosquitto/README.md
Outdated
UpstreamAuthority "disk" { | ||
plugin_data { | ||
key_file_path = "./conf/server/dummy_upstream_ca.key" | ||
cert_file_path = "./conf/server/dummy_upstream_ca.crt" | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the upstream authority necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it is necessary, but this is the default that comes with the package. I just left is as is. I figured it is better to leave as much as possible the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Still think its better to remove because then we need instructions on how to create those certs and they are not relevant to this example.
examples/mosquitto/README.md
Outdated
Once installed, make sure to stop it as we will be running it with the spiffe | ||
helper instead: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once installed, make sure to stop it as we will be running it with the spiffe | |
helper instead: | |
Once installed, make sure to stop it as we will be running it with the spiffe-helper instead: |
-spiffeID spiffe://example.org/mosquitto-server \ | ||
-parentID spiffe://example.org/node1 \ | ||
-selector unix:user:mosquitto \ | ||
-ttl 600 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the ttl is not strictly necessary i would leave it out and let it be default value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is similar to the PostgreSQL sample that also adjusts the ttl to show the certificate expiry. I think it is a nice touch to clearly show the benefit of using spiffe. Maybe I should make it clearer that the ttl is purely for testing and in production you would want to keep it the default or some larger sensible value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, i think its fine to leave.
sudo chown mosquitto-client:mosquitto-client /tmp/mosquitto/svids | ||
``` | ||
|
||
Copy over the script [./connect.sh] to `node2` and run it with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the connect.sh you just reference the current directory but above you use ./examples/moquitto/..
. Just want to make sure we are consistent with the what directory we are running these commands from
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! I will double check the references to the files.
Thanks for the review @faisal-memon ! I have added some comments to a few of your recommendations. Let me know what you think. After that I will push the corrections... |
@fdeantoni Put in my follow up replies |
Hi @faisal-memon, pushed the changes as suggested. Let me know if ok! |
Added an example using Mosquitto broker.