Skip to content
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

Mixed signing 2 #208

Merged
merged 2 commits into from Sep 22, 2015

Conversation

Projects
None yet
3 participants
@fleinze
Copy link
Contributor

commented Sep 22, 2015

to match the description in http://forum.mysensors.org/topic/1021/security-introducing-signing-support-to-mysensors :

However, the difference is that the gateway will only require signed messages from nodes it knows in turn require signed messages.

fleinze added some commits Sep 22, 2015

Update MySensor.cpp
only verify signature from nodes that require signing
Update MySensor.cpp
Gateway only expects signing only from nodes it knows in turn require signed messages.
@mysensors-jenkins

This comment has been minimized.

Copy link

commented Sep 22, 2015

As the author of this pull request is not in my whitelist, I may not build this PR automatically.
A member of the MySensors core team can ask me to verify this pull request by telling me "jenkins, build this please".
The author can be whitelisted to get future PR:s automatically verified by telling me "jenkins, add author to whitelist please".
But remember that I will only listen if you name me with a capital 'J'.
If you would like to submit a PR and not have Jenkins build it for you, you can add "[skip ci]" to your PR "body".

@fallberg

This comment has been minimized.

Copy link

commented on libraries/MySensors/MySensor.cpp in 7858825 Sep 22, 2015

This change will make nodes that don't sign messages to the sender skip verifying messages from that sender. That is not good. A node that require signatures, must verify ALL messages. Not just messages from nodes that also require signatures.
The GW should not behave like this though. I have to look up what has happened to the special case of the GW but this change would affect both GW and nodes.

@fallberg

This comment has been minimized.

Copy link

commented on libraries/MySensors/MySensor.cpp in 518ed6a Sep 22, 2015

Ok, now I am more in sync. Did not see this change before. I will still see where my original change went to handle this because it was supposed to be like this already.

This comment has been minimized.

Copy link

replied Sep 22, 2015

On this line: https://github.com/mysensors/Arduino/blob/development/libraries/MySensors/MySensor.cpp#L673
the logic to handle GW signing requirements is handled.
It works like this:
When the GW receives signing request form a node (sent at node startup) it will reply with it's own requirements.
If the GW require signing, it will tell the node that it does so and that should cause the node to sign messages to the GW.
However, I see now that in the GW case, it would still require signed messages from all nodes it seem. This is the problem you see, right?
Your fix should solve that case I believe.
The change would mean that now, when a node receives a message, and it require signatures, it will do signature verification if it is not a gateway or it signs messages to the sender, and that is in line with the documentation.

@fallberg

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2015

@fleinze Have you tested this change with a GW that require signatures, and two nodes (on that is "secure" and one that is "insecure")?
Expected behavior:
GW<->"secure node": all messages are signed in both directions
GW<->"insecure node": no messages are signed at all

I cannot currently verify the change myself I am afraid. But this is at least how it is supposed to work.

Also, you should be able to set the GW to not require signatures, then the behavior should be like this:
GW<->"secure node": all messages to the node is signed, no messages to the GW is signed
GW<->"insecure node": no messages are signed at all
It would be great if you could test this usecase as well.

@fallberg

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2015

Jenkins, build this please

@fleinze

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2015

thank you fallberg, I did test the usecases:
"insecure" GW -> "secure" node: messages to gateway are not signed, messages to node are signed
"insecure" GW -> "insecure" node: no signing at all

@mysensors-jenkins

This comment has been minimized.

Copy link

commented Sep 22, 2015

Congratulations! I found no problems building this pull request for any of the supported boards or examples.
You can see the result of the build(s) here: http://ci.mysensors.org/job/MySensorsArduinoPR/103/

@fallberg

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2015

@fleinze Ok, so all testcases did pass as expected?

@fleinze

This comment has been minimized.

Copy link
Contributor Author

commented Sep 22, 2015

I just did final tests with a clean copy: (true means requires signing, false means doesn't require signing, as in the MySigning constructor)
GW(true) -> node(true): all messages signed
GW(true) -> node(false): no messages signed
GW(false) -> node(false): no messages signed
GW(false) -> node(true): messages to the gw are not signed, messages to the node are signed
So, all testcases as expected.

fallberg added a commit that referenced this pull request Sep 22, 2015

Merge pull request #208 from fleinze/mixed-signing-2
Gateway now correctly handles message signing

@fallberg fallberg merged commit 3ce3c6e into mysensors:development Sep 22, 2015

1 check passed

Jenkins No test results found.
Details
@fallberg

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2015

Thanks @fleinze for finding and fixing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.