MagicMirrorOrg / MagicMirror

MagicMirror² is an open source modular smart mirror platform. With a growing list of installable modules, the MagicMirror² allows you to convert your hallway or bathroom mirror into your personal assistant.
http://magicmirror.builders
MIT License
19.75k stars 4.2k forks source link

Feature Request: Easier Ability to use Environment Variables in Configuration #1756

Closed tillig closed 1 year ago

tillig commented 5 years ago

I've been using Magic Mirror 2.8.0 and want to store secrets (e.g., OpenWeather API keys) outside of my configuration file so I can commit the file to source control. This would also allow for easier Docker image creation where you could poke in some environment variables rather than full config files.

Doing this has been a challenge.

There is a discussion where I raised this over in the forum.

A working config.js with environment variables looks like this:

const owApiKeyName = "OPENWEATHER_API_KEY";
var remote = null;
if (typeof window !== "undefined") {
  remote = window.require("electron").remote;
}

var readEnvironment = function (name) {
  if (typeof process !== "undefined" && process.env[name]) {
    // process is undefined in the Electron app.
    return process.env[name];
  }
  if (remote && remote.process.env[name]) {
    // remote is null if the Electron nodeIntegration value isn't set to true.
    return remote.process.env[name];
  }
};

var config = {
  address: "localhost",
  port: 8080,
  ipWhitelist: ["127.0.0.1", "::ffff:127.0.0.1", "::1"],
  language: "en",
  timeFormat: 24,
  units: "imperial",

  electronOptions: {
    webPreferences: {
      nodeIntegration: true
    }
  },

  modules: [
    {
      module: "alert"
    },
    {
      module: "updatenotification",
      position: "top_bar"
    },
    {
      module: "clock",
      position: "top_left"
    },
    {
      module: "calendar",
      header: "US Holidays",
      position: "top_left",
      config: {
        calendars: [{
          symbol: "calendar-check",
          url: "webcal://www.calendarlabs.com/ical-calendar/ics/76/US_Holidays.ics"
        }]
      }
    },
    {
      module: "compliments",
      position: "lower_third"
    },
    {
      module: "currentweather",
      position: "top_right",
      config: {
        location: "Hillsboro",
        locationID: "5731371", //ID from http://bulk.openweathermap.org/sample/city.list.json.gz; unzip the gz file and find your city
        appid: readEnvironment(owApiKeyName)
      }
    },
    {
      module: "weatherforecast",
      position: "top_right",
      header: "Weather Forecast",
      config: {
        location: "Hillsboro",
        locationID: "5731371", //ID from http://bulk.openweathermap.org/sample/city.list.json.gz; unzip the gz file and find your city
        appid: readEnvironment(owApiKeyName)
      }
    },
    {
      module: "newsfeed",
      position: "bottom_bar",
      config: {
        feeds: [{
          title: "New York Times",
          url: "http://www.nytimes.com/services/xml/rss/nyt/HomePage.xml"
        }],
        showSourceTitle: true,
        showPublishDate: true,
        broadcastNewsFeeds: true,
        broadcastNewsUpdates: true
      }
    }
  ]
};

/*************** DO NOT EDIT THE LINE BELOW ***************/
if (typeof module !== "undefined") {
  module.exports = config;
}

The idea is that you have to fall back from process to remote.process before you can get to the environment.

I'd like to propose a couple things that would make environment variable usage easier:

From a backwards compatibility perspective, you could treat config.js as something that both use and the difference would be that the module settings on the Electron side that are read from config.js would be ignored in favor of retrieved settings.

tillig commented 5 years ago

I'm finding that the nodeIntegration: true bit really hoses up modules. The default clock module, for example, stops loading things that have time zones set because the node_modules path for the server isn't found by the client. Or something. I'll have to find a different workaround.

Regardless, having environment variable support would be huge. I think the client getting the configuration from the server /config endpoint would address a lot.

tillig commented 5 years ago

I found that the clientonly code does try to retrieve configuration from the server but only if the server isn't local. If I remove the constraint for a non-local server (so it always reads remote config), I can see that the configuration gets read there and does have the environment variables from the server... but once it hits the modules, that configuration isn't actually used.

I added some logging to look at configuration and it seems all the way through electron.js the values are correct, but when the modules are running, they somehow have the wrong config - config they've read from config.js rather than the /config remote endpoint.

As a side note, when running in clientonly mode it doesn't appear there's a way to get dev tools running. I'll file a separate issue for that.

tillig commented 5 years ago

I think the problem is server.js - instead of including the JSON from the /config endpoint, it replaces a #CONFIG_FILE# token with the config.js location and assumes it can just be read directly.

This bypasses all the environment and loads config.js directly.

You can fix that in a kind of hacky way by creating a "module" in a different location using the configuration in server.js:

  app.get("/config-module/config.js", function (req, res) {
    var moduleConfig = "var config = " + JSON.stringify(config) + ";";
    moduleConfig += "if (typeof module !== \"undefined\") { module.exports = config; }";
    res.type('text/javascript').send(moduleConfig);
  });

Then in app.get("/"...) where the HTML is replaced with the location of config.js, use that generated module instead:

html = html.replace("#CONFIG_FILE#", "/config-module/config.js");

Or, better, just put the generated module in index.html directly and don't read the config.js at all.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

tillig commented 4 years ago

It appears this is still something that would be helpful.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

buxxi commented 4 years ago

Would also like this functionality for the same reason as @tillig Haven't really looked into how the core of the MagicMirror works but if it just includes the config-file in both the client and server it feels kind of wrong.

Could the notification-system be used for this where a LOAD_MODULE-notification is sent on startup that triggers the start()-method on that module. Then the payload could be that modules configuration only with environment-variables populated.

And wouldn't it be better with having some kind of templating instead of functions in the config, like: config: { appid: "${env.OPENWEATHER_API_KEY}" }

daxiang28 commented 3 years ago

In case anyone runs into this and happens to be using @khassel awesome Docker installation, environment variables are supported.

.env

SECRET_1=secret_value
SECRET_2=secret_value_2

docker-compose.yml

services:
  magicmirror:
    environment:
      SECRET_1: ${SECRET_1}
      SECRET_2: ${SECRET_2}

config.js.template

var config = {
  // The variables are replaced when the template is converted to config.js
  // so you'll need to wrap them with quotes as if they were declared as strings
  secret1: '${SECRET_1}'
}
rejas commented 3 years ago

Thx for the info @daxiang28 Might this be something that should be added to the documentation of the docker project of @khassel ?

khassel commented 3 years ago

sorry for the late reply ...

The construction using variables defined in .env in your docker-compose.yml is normal behaviour of docker-compose, see https://docs.docker.com/compose/environment-variables/.

For replacing variables in config.js the container uses envsubst to replace variables in a config.js.template which is documented here.

Thanks @daxiang28 for the hint that we can combine those 2 features to keep all secrets seperated, I will add a hint to the documentation.

20k-ultra commented 1 year ago

Maybe OP should re-open this issue since it is not stale. This is still a seriously embarrassing issue to have. As a new magic mirror user I was shocked to see the lack of config sharing and I understand why now...because your config would contain all your secrets!

tillig commented 1 year ago

Unfortunately, I don't have the ability to re-open the issue, just comment. You'd have to file a new issue.

rejas commented 1 year ago

Re-opening since some people hav einterest in a solution. Lets discuss :-)

khassel commented 1 year ago

There was already a solution here https://github.com/MichMich/MagicMirror/pull/1947 which was not accepted by the repo owner.

A very simple solution without breaking changes would be to call envsubst within mm to create the config.js from a e.g. config.js.template (as mentioned here but not doing the envsubst call with a bash script but within mm using child_process).

sdetweil commented 1 year ago

environment variables suck as a data passing mechanism under linux. and we don't have a launch wrapper like docker compose and how we encode the 'variables' could/would affect the checking and data structure creation and data types expected by the modules (who don't check now when user does interval:"60000")

$var doesn't work,  require and checking fail
`var`  doesn't work, same problem
"$var"  doesn't work , sometimes.. gets strings ok, messes up numbers
20k-ultra commented 1 year ago

environment variables suck as a data passing mechanism under linux.

interested to learn about what alternatives there are and why environment variables are not good.

tillig commented 1 year ago

environment variables suck as a data passing mechanism under linux

This seems fairly hyperbolic considering there are lots of systems that do this, from deploying containers in Kubernetes (where you might pass credentials, etc. in environment) to Java apps that read system settings from environment to simple Node apps that might start listening to a port based on environment.

I think it's valid to say that environment variables are painful if they need to be treated as anything other than a string by the consuming code, for sure. Based on context it may be that the environment variable needs to be treated like a number rather than a string, and knowing when to parse that and how to err out or fall back if it fails is a challenge.

But I don't know that it means "environment variable suck," it just means there can be complexity.

sdetweil commented 1 year ago

in linux, env variables are not passed from x to started app.. UNLESS explicitly 'exported'..

current is

pm2 launches
   mm.sh  --- if we export here, it works
       which launches npm start 
             which launches electron

where do you define the variables?

for pi0 (only currently) we have a wrapper shell script run-start.sh (to get the server running so u can access via chrome as electron is dead on armv6l).. now is there ANOTHER place to set this up?

sdetweil commented 1 year ago

sucks from a user (support) perspective.. mgmt, spelling, placement, syntax

don't get me wrong, externalizing some credentials repo should be a useful thing

sdetweil commented 1 year ago

my MMM-Config module builds a form for non-editing of config.js , and auto detecting the module intent of the variables (and author trickiness) is a challenge

20k-ultra commented 1 year ago

what alternative is there ? I am with @tillig in that I am containerizing my magic mirror runtime and using https://www.balena.io which allows me to ship containers to my devices. From the balena dashboard I can update service variables which are injected as environment variables within the container. This would allow me to call process.env.THING and get secrets without hard coding them.

Better yet, I can finally publish my config file. Is anyone else confused why no one shares configs ??? I feel like this is the biggest deterrent.

khassel commented 1 year ago

if you are using https://hub.docker.com/r/karsten13/magicmirror/ you can use env variables as described here

20k-ultra commented 1 year ago

I was using https://hub.docker.com/r/bastilimbach/docker-magicmirror/ but found that image as well when I read this issue originally and saw https://github.com/MichMich/MagicMirror/issues/1756#issuecomment-864292252.

It works for deployments but for development I'd have to stop and start the container. I am running magic mirror in docker on my development machine with bind mounts so that when I edit my config.js I just have to reload my tab to see the results.

docker run --rm \
    --publish 80:8080 \
    --mount "type=bind,source=$(pwd)/magic_mirror/config/config.js,target=/opt/magic_mirror/config/config.js" \
    --mount "type=bind,source=$(pwd)/magic_mirror/css/custom.css,target=/opt/magic_mirror/css/custom.css" \
    --mount "type=bind,source=$(pwd)/magic_mirror/modules/MMM-CalendarExt2,target=/opt/magic_mirror/modules/MMM-CalendarExt2" \
    --mount "type=bind,source=$(pwd)/magic_mirror/modules/MMM-SimpleLogo,target=/opt/magic_mirror/modules/MMM-SimpleLogo" \
    --volume /etc/localtime:/etc/localtime:ro \
    --name magic_mirror \
    bastilimbach/docker-magicmirror

I'm grateful that image at-least has a workaround and restarting my container when developing is not the end of the world.

khassel commented 1 year ago

Reloading the browser after changes in config.js means only the browser side gets the new config, not the server side, so this will not work for all modules.

In karsten13/magicmirror the ensubst command is executed in the entrypoint of the container before starting mm so when changing config.js.template you have to restart the container for getting the new config.js, I see no other way ...

sdetweil commented 1 year ago

and restarting is no different than non-docker

I must restart mm a cople hundred times a week w development, testing and support

don't use pm2, dont use docker.

use npm start./ctrl-q only node and npm dependencies on dev machine.

sdetweil commented 1 year ago

Is anyone else confused why no one shares configs

why share config?, it isn't that hard. maybe individual module config..(what people ask for most) but u don't need the whole file for that

sdetweil commented 1 year ago

so, I found a bash script I wrote over a year ago to do this variable replacement approach {latitude}

from the dictionary "{latitude}" : 40

so the thing after replacement is as written in the dictionary.. number, string.. whatever

dictionary.txt config.model.js.txt copy-values.sh.txt

you u customize the config.model put var values in the dictionary.txt file

then run installers/copy-values.sh, which generates the execution config.js before npm start

functionally the same as envsubst.. without the environment

20k-ultra commented 1 year ago

Reloading the browser after changes in config.js means only the browser side gets the new config, not the server side, so this will not work for all modules.

In karsten13/magicmirror the ensubst command is executed in the entrypoint of the container before starting mm so when changing config.js.template you have to restart the container for getting the new config.js, I see no other way ...

Thanks for clarifying that, definitely changes things. A watcher would be necessary than but that at that point... this watcher could run some config.template parser like in https://hub.docker.com/r/karsten13/magicmirror/

Is anyone else confused why no one shares configs

why share config?, it isn't that hard. maybe individual module config..(what people ask for most) but u don't need the whole file for that

This makes sense that people would share just individual module configs. I'm learning the magicmirror-isms still so excuse my ignorance. I think it's still valuable to be able to clone someones repo with their config so you can start from something. I for example have a config with custom css to make a calendar layout.

sdetweil commented 1 year ago

cool, you could add a package.json to your clonable if not already and add a postinstall to append your css to custom.css

20k-ultra commented 1 year ago

It sounds like we have workarounds but they are all different than what I've used in my experience of writing/using software.

Can we reach a consensus about the value of this feature ? Does MagicMirror want it ?

khassel commented 1 year ago

I still think we should have this feature, but have you read the conversation of the not accepted solution?

If you can convince @MichMich to accept a solution someone can implement it.

MichMich commented 1 year ago

I still stand by my last comment:

The problem with this, is that I have the feeling that we are overcomplicating things for just some edge-cases. Maybe it's better to write a separate module that fetches ENV variables does some magic with it? This way, the login stays outside of the core.

Thoughts?

20k-ultra commented 1 year ago

The main use case for loading environment variables for configuration is secrets/keys.

With this use case in mind I ask the follow; Does the client need to know the api keys ? Is this question module specific ?

My understanding of why this feature doesn't exist is because the config.js file is used by the server and client. If the client doesn't need secrets, than I feel this is the problem to be solving since sending secrets that aren't used by the client seems like a security flaw (least privilege) which should be enough motivation to differently implement this config.js file.

tl;dr if modules use secrets on the client side I would say this change will require more work than it's worth given the workarounds.

KristjanESPERANTO commented 1 year ago

I want to support @20k-ultra's argument. If I hang a MM in a network, everyone in this network who can access the mirror with a browser can also access the secrets in the config.js.

sdetweil commented 1 year ago

client means modulename.js, right.

it depends. a lot of modules use fetch in browser component (modulename.js) and don't use a node helper. in that case, 'client' needs whatever is required to make the API work. keys, passwords. ....

if you use chrome from your pc to access your mirror, then modulename.js is loaded there

weather is a perfect example. all client side based calendar is not. BUT 90% of the config settings are client side (output formatting)

and system design sends the config parms to the client side only. the client forwards them on to server side helper

sdetweil commented 1 year ago

@KristjanESPERANTO well, it's not 'that' easy. someone would have to know how to find the mm instance, know about dev env, mm structure and where to look.

you let people like that on your network?

no one I know, that has access to my network, would be able to do that.

and there is nothing useful in there. weather calendar, and news. and some other random API keys

20k-ultra commented 1 year ago

client means modulename.js, right.

I'm not sure of the exact mechanisms that do this but I refer to client as anything running in a browser.

weather is a perfect example. all client side based calendar is not. BUT 90% of the config settings are client side (output formatting)

This is what I mean. Calendar does not need keys in the frontend. This example has access to sensitive information so sending the secrets to anyone on the network that asks for the webpage seems like a flaw in current implementation. For weather, if you get pwned you lose weather API access if someone misuses your tokens but for other modules the risks can be significant.

Reminder, I think there is some UX to gain for developers since they wouldn't have to maintain postinstall scripts as suggested above:

you could add a package.json to your clonable if not already and add a postinstall to append your css to custom.css

If the solution is big enough of a breaking change I understand the hesitation. But, given what I've mentioned (developer UX and security) would MM still have implemented this file in this way ? If the answer is no than we could begin to discuss the best way to fix it.

I'm hoping to separate the why and how of this feature so we can more easily discuss in a productive manner. So far I've been talking only about why with responses from those more knowledgeable providing workaround hows.

20k-ultra commented 1 year ago

you let people like that on your network?

I don't think it's a stretch to think people will use magic mirror in a public space. Besides, justifying a security hole because "just don't get pwned" seems silly.

and there is nothing useful in there. weather calendar, and news. and some other random API keys

dangerous precedent to set here. How about Alexa integrations ?

Not trying to be confrontational btw all! Appreciating the time you've given this.

sdetweil commented 1 year ago

@20k-ultra

the how is a complete rewrite of the core config.

the client component would have to ask some server component for 'their' config. if there is no (today optional) helper, this would have to be pushed back into the core.

today communications is not exposed socket.io communications between front and back. core is not involved other than setup.

the helper would have to get info from the core, instead of from the client.

this does not protect each modules handling of the config data.

every module would be broken.

there are also some modules that 'know' that the total config is available in both helper and client, and use that info(my MMM-Config for example, getting the address/ipWhitelist settngs in helper, also count instances of modules, which is not officially available outside my module config)

sdetweil commented 1 year ago

side comment, Alexa integrations are dead, thank Amazon... but I know what u meant

khassel commented 1 year ago

May I write this more than once: I still think we should have this feature ...

But I can also understand avoiding breaking changes. May we have to talk about MagicMirror v3 where we do things better and we accept breaking changes. And e.g. realize the idea to load the config from the server. And I want to share my mm with a public url to others without publishing all secrets.

sdetweil commented 1 year ago

@khassel I think it would be interesting..

how would you propose to DO it?

envsubst and my script do not address your problem. they protect the file system file. which must be resolved to load the system.

MichMich commented 1 year ago

Correct me if I'm wrong, but we can limit access to the MagicMirror² server to only local host. So in that case no sensitive data is available on the local network. Server only mode is a bit different, but that's what I consider an edge case. If there is a simple non breaking way of solving this, I'm all for it. But we should not complicate things for situations that are considered an edge case and can be solved with an external module.

sdetweil commented 1 year ago

the browser side loads the entire config, and separately the portions for each module. in clear text

one can use an https connection and then I can transport over the internet to have you view my mirror. but any actor that has access to your browser can see my config.

khassel commented 1 year ago

Last post was a wish list for a future mm version, its clear that this is not solvable without breaking changes (and therefore I didn't think how I would implement this).

Back to current mm version and sum up of the discussion (correct me if I'm wrong):

Possible minimal solution would be the pseudo-envsubst:

As long we have no better idea we can decide to implement such a pseudo-envsubst or close this ticket as not planned.

sdetweil commented 1 year ago

correct

but point 3, 1st steps this still exposes a modules config in browser, and so aggregated, all modules configs are exposed same as today. push or pull doesn't solve. we would need some keyed/encrypted protected storage facility and methods to get vars from it

rejas commented 1 year ago

As long we have no better idea we can decide to implement such a pseudo-envsubst or close this ticket as not planned.

Got to agree here with the plan, but myabe also some other TODOs I read from here:

The mm2 wasn't build around the idea of protecting senstitive infos in your config (correct me if I am wrong here @MichMich). With the premise of "do not break existing stuff" we cannot change the config-handling for the modules to increase security in v2.

There might be modules out there in the wild where you put your banking credentials into the config (or other sensitvie info) but even if this is a "beginner friendly project" people should know a little on security topics.

--> so a TODO might be: add a section on security in the documentation website?

Editing the config by hand / copying from the website is a error prone process that might be up for improvement. Thats why I was interested to learn about @sdetweil MMM-Config module. Wouldnt it be nice to have something like that as a new default module? A module for importing / exporting / editing your config? Not something a new user has to download from somewhere else and figure out, but which might help him directly from the beginning with editing his config/mm2 ?

--> just a thought, not directly a TODO (only TODO would be opening a new discussion ticket for this :-)

sdetweil commented 1 year ago

@rejas did u try the proper url for MMM-Config?

rejas commented 1 year ago

@rejas did u try the proper url for MMM-Config?

yes, worked