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

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

Closed
tillig opened this issue Aug 23, 2019 · 49 comments
Closed

Comments

@tillig
Copy link

tillig commented Aug 23, 2019

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.

  • The config.js is used in both a server and a client context. That is, instead of the Electron client app retrieving configuration from the /config endpoint of the server, the config.js is used directly. This means any references to process.env don't work because...
  • Electron doesn't provide the process environment on the server to the running client. [This issue shows an example of that.] process doesn't exist by default, and even if you enable nodeIntegration, the process that Electron is running under isn't the same as the process the server started under. Instead, you need to require('electron') and access the electron.remote.process.env to get those variables marshaled over. But...
  • Electron nodeIntegration is off by default. It's also turned off in the main electron.js. This can be overridden by config.

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:

  • Separate client and server configuration such that...
    • The client only needs the endpoint to query and the Electron options.
    • The server has the endpoint to serve on and the module configuration.
  • Change the Electron app to query module configurations from the server rather than directly using config.js.

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
Copy link
Author

tillig commented Aug 26, 2019

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
Copy link
Author

tillig commented Aug 26, 2019

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
Copy link
Author

tillig commented Aug 26, 2019

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
Copy link

stale bot commented Dec 3, 2019

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.

@stale stale bot added the wontfix label Dec 3, 2019
@tillig
Copy link
Author

tillig commented Dec 3, 2019

It appears this is still something that would be helpful.

@stale stale bot removed the wontfix label Dec 3, 2019
@stale
Copy link

stale bot commented Feb 1, 2020

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.

@stale stale bot added the wontfix label Feb 1, 2020
@stale stale bot closed this as completed Feb 8, 2020
@buxxi
Copy link
Contributor

buxxi commented Feb 9, 2020

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
Copy link

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
Copy link
Collaborator

rejas commented Jun 4, 2021

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

@khassel
Copy link
Collaborator

khassel commented Jun 18, 2021

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
Copy link

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
Copy link
Author

tillig commented Oct 26, 2022

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

@rejas rejas reopened this Oct 26, 2022
@rejas
Copy link
Collaborator

rejas commented Oct 26, 2022

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

@rejas rejas added enhancement and removed wontfix labels Oct 26, 2022
@khassel
Copy link
Collaborator

khassel commented Oct 26, 2022

There was already a solution here #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
Copy link
Collaborator

sdetweil commented Oct 26, 2022

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
Copy link

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
Copy link
Author

tillig commented Oct 26, 2022

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
Copy link
Collaborator

sdetweil commented Oct 26, 2022

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?

@khassel
Copy link
Collaborator

khassel commented Nov 2, 2022

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
Copy link
Collaborator

MichMich commented Nov 3, 2022

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
Copy link

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
Copy link
Contributor

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
Copy link
Collaborator

sdetweil commented Nov 3, 2022

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
Copy link
Collaborator

sdetweil commented Nov 3, 2022

@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
Copy link

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
Copy link

20k-ultra commented Nov 3, 2022

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
Copy link
Collaborator

sdetweil commented Nov 3, 2022

@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
Copy link
Collaborator

sdetweil commented Nov 3, 2022

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

@khassel
Copy link
Collaborator

khassel commented Nov 3, 2022

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
Copy link
Collaborator

sdetweil commented Nov 4, 2022

@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
Copy link
Collaborator

MichMich commented Nov 4, 2022

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
Copy link
Collaborator

sdetweil commented Nov 4, 2022

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
Copy link
Collaborator

khassel commented Nov 4, 2022

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):

  • browser and server are reading config.js
  • browser cannot handle config.js with env vars
  • first step for env vars in config.js would be to read config.js only from server and client retrieves config (with resolved env vars) from server, but this would break all modules without a node_helper (so we cannot do this)

Possible minimal solution would be the pseudo-envsubst:

  • if config.js.template (file with env vars) exists
  • then do var replacements with js stuff before starting the app
  • and create a config.js with resolved variables
  • this would not change anything for all current users
  • people who want var substitution could use the new feature

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

@sdetweil
Copy link
Collaborator

sdetweil commented Nov 4, 2022

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
Copy link
Collaborator

rejas commented Nov 4, 2022

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:

  • about security:

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?

  • about config / config sharing

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
Copy link
Collaborator

sdetweil commented Nov 4, 2022

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

@rejas
Copy link
Collaborator

rejas commented Nov 4, 2022

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

yes, worked

rejas pushed a commit that referenced this issue Feb 12, 2023
This is the implemenation of envsubst discussed in #1756 

Documentation update will follow after merge.
@rejas rejas added this to the 2.23 milestone Feb 18, 2023
MichMich added a commit that referenced this issue Apr 4, 2023
## [2.23.0] - 2023-04-04

Thanks to: @angeldeejay, @buxxi, @CarJem, @dariom, @DaveChild, @dWoolridge, @grenagit, @Hirschberger, @KristjanESPERANTO, @MagMar94, @naveensrinivasan, @nfogal, @psieg, @rajniszp, @retroflex, @SkySails and @tomzt.

Special thanks to @khassel, @rejas and @sdetweil for taking over most (if not all) of the work on this release as project collaborators. This version would not be there without their effort. Thank you guys! You are awesome!

### Added

- Added increments for hourly forecasts in weather module (#2996)
- Added tests for hourly weather forecast
- Added possibility to ignore MagicMirror repo in updatenotification module
- Added Pirate Weather as new weather provider (#3005)
- Added possibility to use your own templates in Alert module
- Added error message if `<modulename>.js` file is missing in module folder to get a hint in the logs (#2403)
- Added possibility to use environment variables in `config.js` (#1756)
- Added option `pastDaysCount` to default calendar module to control of how many days past events should be displayed
- Added thai language to alert module
- Added option `sendNotifications` in clock module (#3056)

### Removed

- Removed darksky weather provider
- Removed unneeded (and unwanted) '.' after the year in calendar repeatingCountTitle (#2896)

### Updated

- Use develop as target branch for dependabot
- Update issue template, contributing doc and sample config
- The weather modules clearly separates precipitation amount and probability (risk of rain/snow)
  - This requires all providers that only supports probability to change the config from `showPrecipitationAmount` to `showPrecipitationProbability`.
- Update tests for weather and calendar module
- Changed updatenotification module for MagicMirror repo only: Send only notifications for `master` if there is a tag on a newer commit
- Update dates in Calendar widgets every minute
- Cleanup jest coverage for patches
- Update `stylelint` dependencies, switch to `stylelint-config-standard` and handle `stylelint` issues, update `main.css` matching new rules
- Update Eslint config, add new rule and handle issue
- Convert lots of callbacks to async/await
- Revise require imports (#3071 and #3072)

### Fixed

- Fix wrong day labels in envcanada forecast (#2987)
- Fix for missing default class name prefix for customEvents in calendar
- Fix electron flashing white screen on startup (#1919)
- Fix weathergov provider hourly forecast (#3008)
- Fix message display with HTML code into alert module (#2828)
- Fix typo in french translation
- Yr wind direction is no longer inverted
- Fix async node_helper stopping electron start (#2487)
- The wind direction arrow now points in the direction the wind is flowing, not into the wind (#3019)
- Fix precipitation css styles and rounding value
- Fix wrong vertical alignment of calendar title column when wrapEvents is true (#3053)
- Fix empty news feed stopping the reload forever
- Fix e2e tests (failed after async changes) by running calendar and newsfeed tests last
- Lint: Use template literals instead of string concatenation
- Fix default alert module to render HTML for title and message
- Fix Open-Meteo wind speed units
@khassel khassel closed this as completed Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants