HIDEs the network key in the logs #1086
Conversation
We do in when calling Zigbee-herdsman from zigbee2mqtt, but not here
src/controller/controller.ts
Outdated
| logger.debug(`Starting with options '${JSON.stringify(this.options)}'`, NS); | ||
|
|
||
| let optionsLog = objectAssignDeep({}, this.options, {network: {networkKey: 'HIDDEN'}}); | ||
| logger.debug(`Starting with options '${JSON.stringify(optionsLog)}'`, NS); |
There was a problem hiding this comment.
To prevent adding a new dependency, can we just do this here?
| logger.debug(`Starting with options '${JSON.stringify(optionsLog)}'`, NS); | |
| logger.debug(`Starting with options '${JSON.stringify(this.options).replace(this.options.network.networkKey, 'HIDDEN')}'`, NS); |
There was a problem hiding this comment.
Probably best to do that here too, instead of messing with objects.
There was a problem hiding this comment.
Note: Probably this would be better as a feature of nurikk/zigbee2mqtt-frontend#2011 "Download sanitized". It could go through security (network key, IEEE...) and privacy (friendly names...) checks, on an existing file, specifically for sharing. That way local logs could retain all values for local debugging.
There was a problem hiding this comment.
But that doesn't apply to the log files in the log directory, I think it's good to keep it hidden in the logs as well.
There was a problem hiding this comment.
It all comes down to the security of the machine in question, and the handling of the logs/config/backup (kept local vs shared/uploaded), since everything is kept in plain in the same folder.
If frontend provides a button to download any log (current or archived) with the sanitization logic applied when downloaded, it can provide any log "ready for sharing". With the option of manually accessing the files (directly), to see the plain versions.
There was a problem hiding this comment.
Logs are usually in a less safe place than the config, we definitively need to hide the secrets there...
Of course it does not preclude from having an option to download a sanitized version with anonymization
There was a problem hiding this comment.
Probably best to do that here too, instead of messing with objects.
That's the line I used, for consistency indeed ...
There was a problem hiding this comment.
To prevent adding a new dependency, can we just do this here?
I was worried with networkKey as an array, but it seems to work. I'm JSON.stringify() it rather than toString() just in case and array.toString and JSON stringify don't behave the same for some reason.
One other thing I used replaceAll so that it wouldn't miss if you set your networkKey to an existing key for some weird reason... and instead make for a fun "guess the HIDDEN key" game.
So yeah it is technically more fragile and could still leak those type of weird keys
slightly more fragile, please don't use a networkKey "network" or any other option key name...
package.json
Outdated
| @@ -34,6 +34,7 @@ | |||
| "fast-deep-equal": "^3.1.3", | |||
| "mixin-deep": "^2.0.1", | |||
| "mz": "^2.7.0", | |||
| "object-assign-deep": "^0.4.0", | |||
There was a problem hiding this comment.
Could you remove the added dependencies here? After that this can be merged.
There was a problem hiding this comment.
Forgot to git add ...
Done
|
Thanks! |
We do it when calling Zigbee-herdsman from zigbee2mqtt, but not when receiving the config.
This has security implications and might require a communication as any user who shared their logs
also shared their network secret