neilenns / node-deepstackai-trigger

Detects motion using Deepstack AI and calls registered triggers based on trigger rules.
MIT License
165 stars 28 forks source link

Allow secrets in settings #383

Closed TonyBrobston closed 3 years ago

TonyBrobston commented 3 years ago

Fixes https://github.com/danecreekphotography/node-deepstackai-trigger/issues/371

Description of changes

Adds a secrets.json file and use mustache to render those secrets into settings.

Checklist

neilenns commented 3 years ago

Cool! I'll take a look at it this weekend.

TonyBrobston commented 3 years ago

Cool! I'll take a look at it this weekend.

Sounds good.

TonyBrobston commented 3 years ago

@danecreekphotography I updated the sampleConfiguration. I think secrets should be used by default. I realize it is slightly more complicated, however I think it's not worth compromising security.

neilenns commented 3 years ago

The docker-compose.yaml change looks fine.

The settings.json file changes are a big hurdle for new users. Based on questions I've had here and elsewhere (ipcamtalk, other Facebook groups), most people using this don't understand Docker at all and just want to have their security cameras have AI motion. Even making minor changes to settings.json was a hurdle for some.

Adding another layer of indirection out of the box and having to explain all that in the documentation is probably overkill for the core userbase: people installing this at home on a home computer.

TonyBrobston commented 3 years ago

The docker-compose.yaml change looks fine.

The settings.json file changes are a big hurdle for new users. Based on questions I've had here and elsewhere (ipcamtalk, other Facebook groups), most people using this don't understand Docker at all and just want to have their security cameras have AI motion. Even making minor changes to settings.json was a hurdle for some.

Adding another layer of indirection out of the box and having to explain all that in the documentation is probably overkill for the core userbase: people installing this at home on a home computer.

Gotcha. That makes sense and is an unfortunate reality.

I guess my use case is slightly different, I'm pushing my configuration to github (without secrets.json) so I have a copy, as well as source control. I try to set all my docker stuff up so I can re-image my ubuntu server if something goes haywire and just pull some configurations from github. It seems like users might want the ability to back their configuration up, however it sounds like the average user probably isn't familiar with github. I could see us building a UI that gets all this setup correctly on the backend, then follow home assistant's recommendation; I think they recommend backing things up to github. But like you said, this is probably a big hurdle for most.

I guess I'll just reap the benefits of this change; seems like others may not think to use this. I'll look around the docs and try to identify the right spot to add this information.

neilenns commented 3 years ago

Best place to add it to the docs would be as a whole new page called "Managing secrets". Then we can link to it from a bunch of places, like the overview documentation etc.

If you do write something up make sure to run markdownlint on the .md and resolve any errors that get thrown. Thanks!

TonyBrobston commented 3 years ago

@danecreekphotography That sounds good to me. I'm not familiar with github's wiki, is that where you're wanting to put it? Is this just something you're able to edit or is there a way for me to fork it and push up a PR?

neilenns commented 3 years ago

Yeah you can fork the wiki, but I honestly don't know if you can open PRs against it. I'm not entirely sure how to handle this :(

TonyBrobston commented 3 years ago

@danecreekphotography I googled and there are some haphazard solutions. I personally think ditching GitHub wiki's and use some other simple static rendered doc tool, then host that using GitHub Pages; which would be https://danecreekphotography.github.io/node-deepstackai-trigger. I'd have to look around to see what other sites use, but here is one example: React Bootstrap

For now I'll just update a quick doc write-up I did below (feel free to modify this as you see fit):

How to manage secrets

The idea of this section is to separate your configuration from your sensitive information, which we refer to as "secrets". These secrets are mainly passwords or tokens, but you may use them for other information you may consider sensitive or want to keep obscure; like ip addresses, URIs, etc. The approach is to create a file of key/value pairs, which we represent as a json object (i.e. {"secretKey": "secretValue"}). node-deepstackai-trigger uses mustache rendering to search for a secret's key inside the settings.json file and replace it with the secret's value. In order to keep your secrets from being checked into GitHub we will add the file name secrets.json to the .gitignore file. Below are the steps to make this happen:

  1. Add a reference to your secrets.json file by adding to docker-compose secrets here:

    secrets:
    # This should point to the location of the secrets.json configuration file
    file: ./secrets.json
  2. Add a reference to your newly added secret by adding to the docker-compose container's secrets here:

    - secrets
  3. Add a settings.json file, which can use mustache templating. The value inside the double curly-brace ({{}}) which is the secret's key, will be swapped for the secret's value from secrets.json.

    {
    "deepstackUri": "http://deepstack-ai:5000/",
    "enableAnnotations": false,
    "enableWebServer": false,
    "verbose": true,
    "awaitWriteFinish": false,
    "mqtt": {
      "uri": "mqtt://mqtt:1883",
      "username": "{{mqttUsername}}",
      "password": "{{mqttPassword}}",
      "enabled": false
    },
    "telegram": {
      "botToken": "{{telegramBotToken}}",
      "enabled": false
    },
    "pushbullet": {
      "accessToken": "{{pushbulletAccessToken}}",
      "enabled": false
    },
    "pushover": {
      "apiKey": "{{pushoverApiKey}}",
      "userKey": "{{pushoverUserKey}}",
      "enabled": false
    }
    }
  4. Add a secrets.json file, which will be used for mustache templating in settings.json. The string value on the left, "mqttUsername" for example, is the secret's key. The string value on the right, "mqttPassword" for example, is the secret's value. It's as simple as it sounds.

    {
    "mqttUsername": "user",
    "mqttPassword": "pass",
    "telegramBotToken": "insert bot token here",
    "pushbulletAccessToken": "access token here",
    "pushoverApiKey": "api key here",
    "pushoverUserKey": "user key here"
    }
  5. Add a .gitignore. This enables you to backup this setup to GitHub without exposing your secrets.

    secrets.json
neilenns commented 3 years ago

I actually went down the path once of trying GitHub pages which is why there's a docs directory: https://github.com/danecreekphotography/node-deepstackai-trigger/tree/main/docs.

I didn't get very far 😂

Thanks for the doc. I'll take it and add it in to the wiki as part of the release process when this all merges.

TonyBrobston commented 3 years ago

I actually went down the path once of trying GitHub pages which is why there's a docs directory: https://github.com/danecreekphotography/node-deepstackai-trigger/tree/main/docs.

I didn't get very far 😂

Thanks for the doc. I'll take it and add it in to the wiki as part of the release process when this all merges.

I could potentially help with this at some point (no promises). I have played with github pages a bit, currently one of my demo sites is hosted here: https://tonybrobston.github.io/jpegasus-demo/

neilenns commented 3 years ago

Sure, if you ever have time and dig into it. I'm not going to look at it any time soon.

What's the overall status of this PR? Is there anything else you want to add before you consider it done?

TonyBrobston commented 3 years ago

I've looked the PR over a few times and I can't think of anything else to add. Lmk if you're able to think of anything else.

I plan to follow this up with another PR which will handle secrets in triggers.json. Also, a side note so I don't forget; it's possible for secrets to be printed in logs accidentally, we'll want to be cognizant of this, since logging a secret will defeat the purpose of this work. I have some ideas on how to handle this or at a minimum, how to write a test that identifies the issue.

TonyBrobston commented 3 years ago

Actually, it looks like I have something that will break triggers here: https://github.com/TonyBrobston/node-deepstackai-trigger/blob/93244dc5914965a7b49cb211bf961f003913daef/src/TriggerManager.ts#L53

So I need to do some work on that.

TonyBrobston commented 3 years ago

I wonder if we should first put this out as a dev tag docker image. I'd hate to break the main docker image if I missed something.

TonyBrobston commented 3 years ago

Actually, looking a bit closer, I don't think https://github.com/TonyBrobston/node-deepstackai-trigger/blob/93244dc5914965a7b49cb211bf961f003913daef/src/TriggerManager.ts#L53 is broken.

neilenns commented 3 years ago

When this eventually merges to main it will automatically become a dev tag image. The latest image is a manual release by me so there's no risk of having people auto-update until it's really published as stable.

What I've done in the past is produce a separate tagged image for a specific branch. That might be the best thing to do here too: merge this into another branch instead of main, then run it for a bit as that image off Docker Hub before actually merging to main as dev.

TonyBrobston commented 3 years ago

I'm good with a separate tag for a specific branch or going to dev, totally up to you.

I was looking into allowing secrets in triggers, it looks super similar to allowing secrets in settings. I think the only thing I'm wondering is, some naming on the object that holds the settingsFilePath and secretsFilePath. I currently have IConfiguration. However I now need another object that holds triggersFilePath and secretsFilePath. So I'm thinking of moving IConfiguration to ISettingsConfiguration and then adding a ITriggersConfiguration. However, we have some other types that are very close to those names: ISettingsConfigJson and ITriggersConfigJson. What are your thoughts? I think I'll go ahed and push my changes up. So my previous comment is slightly incorrect; I'll follow this PR up with another PR that handles removing secrets from a few areas that log, I'm not sure what that looks like yet.

neilenns commented 3 years ago

Since IConfiguration isn't really the configuration, it's the paths to the configuration files, maybe do ISettingsFiles? Also why does it have to be a different interface for settings vs triggers? Just call it IConfiguration and have the properties be baseFile and secretsFile?

TonyBrobston commented 3 years ago

Alright, I updated to what you mentioned.

TonyBrobston commented 3 years ago

Looking into how to handle redacting secrets from logs and I realized I had broken triggers.json hot reloading. I made some changes to hopefully fix that.

neilenns commented 3 years ago

I created a new branch called issue371. Can you either change this PR to point at that (does GitHub allow that?) or close this PR and open a new one requesting to merge into that branch instead of main?

That'll give us a place to merge this and produce the Docker tagged image for testing without having it show up in main until we're confident this is all working and ready to go out.

TonyBrobston commented 3 years ago

@danecreekphotography I swapped the branch that it's merging into.

davkav commented 3 years ago

Just got caught about by this. Can a sample secrets.json be included in the sampleConfiguration directory?