Allow secrets in settings

[TonyBrobston]

Fixes #371

Description of changes

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

Checklist

• User-facing change description added to unreleased section of CHANGELOG.md
• Update sampleConfiguration
• Write higher level tests? Settings and main
• Update Wiki?

[neilenns]

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

[TonyBrobston]

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

Sounds good.

[neilenns]

Looks pretty good! Just a few things here and there I noticed.

[TonyBrobston]

@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]

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]

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]

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]

@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]

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]

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]

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]

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]

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]

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]

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]

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]

Alright, I updated to what you mentioned.

[TonyBrobston]

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]

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]

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

[davkav]

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