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

Improve warning messages for insecure inclusion #41

Open
marcelveldt opened this issue Jul 4, 2024 · 5 comments
Open

Improve warning messages for insecure inclusion #41

marcelveldt opened this issue Jul 4, 2024 · 5 comments
Assignees
Labels
frontend This bug or request (primary) involves the Home Assistant frontend zwave-certification Required for Z-Wave certification

Comments

@marcelveldt
Copy link
Collaborator

Issues/improvements

  1. Show right after the node was added, not at the end of the interview --> Z-Wave JS: Notification for error during secure inclusion is only shown after the interview home-assistant/core#117636

  2. Distinguish between the different reasons for insecure inclusion instead of "an error". See feat: include reason for low security in "node added" parameter node-zwave-js#5570 for details

  3. If a node has been security bootstrapped with the S0 Command Class in an S2 capable network, the SIS / Primary controller MUST issue a warning message to the user informing that the node has not been included securely. Requires 2. on the HA side

TODO Home Assistant (frontend):

  1. Show big fat warning that the node is being included insecurely at the start of the process.

  2. Show specific error message when inclusion failed, so show as much info as possible. The Driver has been adjusted to deliver this info.

  3. Show warning that the node was NOT added securely (despite the fact that the user choose secure inclusion).

@marcelveldt marcelveldt converted this from a draft issue Jul 4, 2024
@marcelveldt marcelveldt added backend This bug or request (primary) involves the Home Assistant backend/integration (and its dependencies) frontend This bug or request (primary) involves the Home Assistant frontend and removed backend This bug or request (primary) involves the Home Assistant backend/integration (and its dependencies) labels Jul 4, 2024
@AlCalzone AlCalzone added the zwave-certification Required for Z-Wave certification label Sep 5, 2024
@MindFreeze MindFreeze self-assigned this Oct 21, 2024
@MindFreeze
Copy link
Collaborator

@marcelveldt I don't understand point 3. We already show a warning when the interview is finished. Is this about showing the specific error? Then it would just part of point 2, no?

@marcelveldt
Copy link
Collaborator Author

@marcelveldt I don't understand point 3. We already show a warning when the interview is finished. Is this about showing the specific error? Then it would just part of point 2, no?

@AlCalzone can explain this better than me but apparently it can be true that you choose secure inclusion but in the end the controller did not do that and the node got included without security.

@MindFreeze
Copy link
Collaborator

Sounds like displaying the reason after the node is added covers this, so if 1 & 2 are done, 3 is also covered

@AlCalzone
Copy link
Member

I'll have to double check 3. - don't remember right now whether that required additional work.

@AlCalzone
Copy link
Member

I'm getting this error now when a device was granted the S0 key in an S2-capable network:
Image

node-zwave-js SHOULD expose the reason SecurityBootstrapFailure.S0Downgrade, so we should be able to warn that a possible S0 downgrade attack has happened.

Will need to investigate to see where this info got lost.

@AlCalzone AlCalzone reopened this Dec 4, 2024
@AlCalzone AlCalzone moved this from Done/verify to In Progress in Z-Wave JS: Road to Certification Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend This bug or request (primary) involves the Home Assistant frontend zwave-certification Required for Z-Wave certification
Projects
Status: In Progress
Development

No branches or pull requests

3 participants