@chris
Hey Chris -
I’ve got some spare cycles so I figured I’d check out openhab2 and the state of the zwave security. I’ve been running my old OH1 binding for a while here at home, there are couple things I need to clean up but it’s obviously time to move on to the OH2 codebase
I see the security code is the OH2 addons git repos (great!).
Couple of questions as I get setup:
In terms of the changes I make, what’s the best way to simplify merges and pull requests? Should I set origin to your addons fork? Any particular branch?
Do you know if any security testing has been done in the OH2 codebase?
Are you aware of any security specific issues in the OH2 codebase that I should work on? If not, I’ll just be by my list and do some testing here
Saw some HUGE threads around OH2 zwave and habmin, I admit I didn’t read through them
Hey Dave,
It would be great if you have some time… Currently, I’m in the middle of a major rewrite of the transaction code. This was spurred on by the security classes, although it’s always been an issue with transactions not being handled very well (IMHO). The new code will allow multiple ‘transactions’ to be outstanding at once, and this should make it easier to handle the nonce requests in the middle of the secure transactions…
So, I would probably avoid major changes to the current codebase. Every command class, and every serial class has significantly changed - as has the controller and every test class. I hope to have this working in the next day or two as it was mostly working last night, but let’s see! .
As above, I would probably avoid this right now, but if you want to work on something, then it should be against the main branch. Once the new transaction code is ready though, the plan is to remove the ZWave binding from the OH2 repository and it will have it’s own repo (which is already started).
Yes - I used it for a while and it was working fine. Since I’m now using a version that has security disabled while I write the transaction code, it’s not working now.
Well, I would really like to see if ‘tidied up’ (please don’t take that negatively - I suspect you have some similar thoughts as I saw some of your comments in the original code suggesting you were looking for better ways to do things)…
I plan to remove the dependancy on the static network key setter in the security class. This shouldn’t be necessary with the new inclusion code but I’ve not yet done it.
I would like to look at why there is a separate secure thread required? Can’t this be eliminated?
I would like to consolidate the security code. Currently it is scattered throughout the layers which isn’t too nice (eg in the command handler, the security classes and the controller). I would like to think we can consolidate this to make it easier to manage, but maybe I’m wrong as I’ve not spent a lot of time looking at it yet.
I’m not sure I like the current initialisation/include code. I wonder if it wouldn’t be better to add the code directly into the init stage advancer? It just seems a bit hard to follow and I wonder if you took this route to avoid too many changes with the original codebase?
There are probably some smaller issues, but I think the above are my main thoughts…
What do you think? You know the code better than me so it would be good to get your thoughts - we have an opportunity to improve the architecture (hopefully!).
Good move! . The ZWave test thread is just enormous now.
Totally agree. I remember keeping track of which responses go with which requests seemed more difficult than it should have been. Some of that was just me learning the zwave code. If I recall correctly, the protocol doesn’t do a whole lot to help out in this area, which always surprised me.
That’s fine, I will hold off for now. I’ve been working on getting my eclipse OH2 environment setup. With my OH1 dev work, I was never able to run from eclipse which slowed down my development work. I’ll wait the new code and repos is ready.
I assume you will be the owner of the new repos? That would be good,as I’ve always struggled with keeping your fork in sync with mine vs master (although this is probably due to my lack of github experience). That would make the PR process easier too.
Glad you’ve been putting it to the test and that it’s been working. Been running the OH1 version here, overall it’s been working well but, as I mentioned, I had a few things I wanted to look at
I am all for refactoring and cleaning things up. I really did try to keep things simple, although you’d never know if from looking at the code
Totally agree here. I do have some test cases I want to add back in to the OH2 codebase that require setting the key, but I’m sure I can find some way to accomplish that without a static setter.
Also worth a look. I remember cringing when I had to add that. Hopefully the refactoring will give us a good way to get rid of it. I think it’s a serial (as opposed to parallel) process, so we should be able to have something else do it.
This is a good goal as well. I think the openzwave guys did some consolidation in their 2nd pass at security, perhaps we can look at that for some ideas. Part of the problem is that the inbound messages have to be decrypted before being processed by the controller. Maybe we can add a preprocessing method to the command classes interface to abstract this out of the controller (just thinking out loud)
I don’t like it etiher Just for background…I split it out so I could work on it in isolation. By the time I was done, there was so much of it, I thought it more understandable to be in it’s own class. Both the init stage advancer and the security inclusion code are rather complex, I’m little afraid to put them together.
To be frank, there’s part of me that thinks since it’s working, it should be left alone. Has flashbacks to endless hours in front of door lock I remember being very excited when I could finally work on the code to actually lock/unlock the door!
Not saying it shouldn’t / couldn’t be integrated, I just don’t see myself volunteering to do that work and test it
I’ve always been a bit TDD guy, theoretically there should be a way to add some test cases to simulate inclusion. You mentioned the test cases are changing, I can take a look at that once the new code base is ready.
From a developer perspective, I think these are good goals. The only one I’m hesitant about is 4 (inclusion code).
Lastly, I got OH2 running in eclipse today, WOW it looks awesome! Love the new UI and the fact that the encryption key is auto-generated. I paired a couple of non-security devices to get a feel for the inclusion process, it’s really nice! Great work!
Well, it will still be under openhab, so similar situation to what it is now, but just split out of the main repository. This will make issue management easier, and avoid ‘polluting’ the main OH2 repo with lots of ZWave updates .
I know - I’ve seen you made some comments in the code . I can imagine it’s one of those things that takes some work to get working, and then you find that once it is, it’s grown a little and could be restructured with lessons learnt… This goes for the whole binding really.
I’ve already restructured the code here. Inclusion is now done differently - you added this setter (if I understand correctly) since you didn’t know if the device supported security until later on in the initialisation. However, there is a way to know - the NIF is effectively sent during inclusion, so we know right at the beginning if this is a secure device - the classes are added at inclusion time so we can also set the key. It also allows me to skip a number of steps in the initialisation which speeds things up quite a bit.
Yep - that’s also my feeling - sounds good.[quote=“dbadia, post:3, topic:11564”]
Part of the problem is that the inbound messages have to be decrypted before being processed by the controller.
[/quote]
I need to look closer at this as I’m not sure why this is the case - it’s not the case for other encapsulation classes. For example, in the CRC, or MultiCommand (and others) classes we process the encapsulation in the command class and then pass the command class on for processing… Why is it different here?
Same here .
That’s what I thought, and probably what I would have done as well! As per my comments above, I guess we should see if there’s a better structure from lessons learnt.
While I can appreciate that, at the moment, I’m more inclined to push to get a tidy(ish!) codebase so that it can then be left alone. OH2 is a ‘good opportunity’ to refactor and I think it would be good to improve if we can. I remember when I first started working on the code the initialisation code was scattered throughout the code - in nearly every command class. It was hard to follow, and impossible to change. I prefer, if possible to try and consolidate - I know it’s not always possible, but I think it would make it easier to understand…
No probs at all (he says boldly - without thinking it through ).
I started writing a test to do this, but haven’t completed it. I would really like to see this though.
Well, in OH2 there are now a hundred plus test cases - far (very far!) from complete, but it’s a start. I added a lot before I started the refactor to make sure that the packet generation didn’t break - this tests all command classes and serial command classes - at least the packet generation side.
The changes aren’t so much that if you wrote a test to test the secure inclusion against the current codebase that it couldn’t be refactored in 10 minutes so if you wanted to come up with some secure test cases it would really appreciated.
A few days ago I started investigating about zwave security as I had some errors with several devices in OH 2 and I found this thread, so before I started digging into the code I was wondering what’s the current status of the changes you comment here.
As I see the new repo is up and running but I’m not sure if the transactions new code or some of the improvements you talk about here have been commited.
No - the transaction code is still not included. This still needs some work to iron out a corner case issue - I’ll try and work on this in the coming week or so.
.
Just for background…I split it out so I could work on it in isolation. By the time I was done, there was so much of it, I thought it more understandable to be in it’s own class. Both the init stage advancer and the security inclusion code are rather complex, I’m little afraid to put them together.
I remember being very excited when I could finally work on the code to actually lock/unlock the door!
).