File/environment-based secrets

[markormesher]

Fixes #31

First draft of a way to provide some config values from a file or environment variable rather than hardcoding them in config.json that is fully backwards compatible. It works by replacing the constant values with nested objects as follows:

"authentication": {
  "api_token": "my_secret_value"
}

// becomes...

"authentication": {
  "api_token": {
    "file": "/path/to/my/secret/file"
  }
}

// or...

"authentication": {
  "api_token": {
    "env": "MY_SECRET_ENV_VAR_NAME"
  }
}

I went with replacing the values rather than adding new keys like api_token_file because a) it lead to cleaner code and b) it avoids the problem of "what if a user enters both?".

Right now this is implemented for the different auth configs and the zone ID - I think those make the most sense but happy to take feedback. I've added some brief docs in the readme but I could also add example docker-compose files or similar.

[timothymiller]

Thank you for putting this together. I have some feedback for some changes I'd like to see in this pr.

I do not want to see changes to the current config.json format. For supporting environment variables, we should add documentation for the various field names.

Using the config.json approach, today you can run multiple zones + multiple subdomains within each zone with a single container. I want to keep this feature parity with the environment variables approach. We will have to use a numbered naming schema such as _2, _3, _4, etc probably up to 10. These environment variables will be loaded into a python object with the same format as config.json

That way, the user can write a single docker-compose file and manage all their DNS by way of environment variables.

[markormesher]

Was this meant to be closed? I've not had time to look at your suggestions yet but I would still like to work on this

[timothymiller]

Absolutely not. I am looking forward to your forthcoming commit.

I'll restate this again, because we have multiple PRs trying to add file/environment based secrets, and so far they have came short of the community expectations.

A PR to support file/environment based secrets cannot break backwards compatibility with passing in a config.json file, which is the preferred method for Docker-compose & the legacy Python script method.

If we have an environment variable set, then we should short circuit and not read in the config.json file.

Is there anything else I can help explain to help you?

[markormesher]

Hey @timothymiller - sorry for the delay here, finally getting time to sit down and properly read through your comments.

100% with you on not breaking any backwards compatibility. The PR as it stands now doesn't do that because it only extends the config.json format, so any previously-valid config would still work against this version. That includes configs for multiple domains with multiple sets of credentials, any of which can be a mix-and-match between written directly in config.json, specified as env vars or read from a file. E.g. the following would work:

{
  "cloudflare": [
    {
      "authentication": {
          "api_token": "my_hard_coded_value", 
          "api_key": {
              "api_key": { "file": "/my/secret/file" },
              "account_email": { "env": "MY_ENV_VAR" }
          }
      },
      ...
    },
    {
      "authentication": {
          "api_token": { "file": "/my/other/secret/file" }, 
          "api_key": {
              "api_key": "my_other_hardcoded_value",
              "account_email": { "env": "MY_OTHER_ENV_VAR" }
          }
      },
      ...
    }
  ]
}

However, all of that does go against your preference of "I do not want to see changes to the current config.json format". Personally I like the idea of extending the format and keeping all of the config in one place, but it's not my project 😃 So, that leads to having environment variables only and ignoring values in config.json if the env vars are set (and in fact presumably the corresponding keys in config.json would be absent).

When you say having vars named _2, _3, ... _10, do you mean to correspond to the multiple domains that could be specified? If so I don't think we need to cap it at 10: for each config i we can just look for env vars ending with _$i (or +1 depending whether you want the env vars to be zero-indexed).

Have I understood your preference correctly above? (Or given you cause to reconsider extending the format? 😉)

[timothymiller]

Thanks for your work so far.

This PR needs to be synced with the latest master branch to be approved.

[dhrrgn]

I think this being over complicated, and overthought.

File Support In my opinion, file support is of little real world value. If you are already copying files, exporting those values to env vars is trivial (export FOO=$(cat val.txt), export $(cat .env | xargs), use jq, etc). So supporting env vars should make this unnecessary.

Environment Variables Instead of predefined var names, with unhelpful indexes, I propose that it support any env var with a predefined prefix (e.g. CF_DDNS_). Then it's just a simple matter of using template substitution when reading config values that support env vars.

Example:

import os
from collections import Collection, Mapping
from string import Template

# Read in all environment variables that have the correct prefix
ENV_VARS = {key: value for (key, value) in os.environ.items() if key.startswith('CF_DDNS_')}

# Stole this from StackOverflow for demo purposes
def recursive_map(data, func):
    apply = lambda x: recursive_map(x)
    if isinstance(data, Mapping):
        return type(data)({k: apply(v) for k, v in data.items()})
    elif isinstance(data, Collection):
        return type(data)(apply(v) for v in data)
    else:
        return func(data)

# Substitutes env variables in a template string with their values 
def resolve_env(val):
  return Template(val).safe_substitute(**ENV_VARS)

# Resolve env vars in all values
config = recursive_map(config, resolve_env)

This way, you can just do something like:

{
  "cloudflare": [
    {
      "authentication": {
          "api_token": "my_hard_coded_value", 
          "api_key": {
              "account_email": "$CF_DDNS_EMAIL"
          }
      },
      ...
  ]
}

[timothymiller]

I think this being over complicated, and overthought.

File Support In my opinion, file support is of little real world value. If you are already copying files, exporting those values to env vars is trivial (export FOO=$(cat val.txt), export $(cat .env | xargs), use jq, etc). So supporting env vars should make this unnecessary.

Environment Variables Instead of predefined var names, with unhelpful indexes, I propose that it support any env var with a predefined prefix (e.g. CF_DDNS_). Then it's just a simple matter of using template substitution when reading config values that support env vars.

Example:

import os
from collections import Collection, Mapping
from string import Template

# Read in all environment variables that have the correct prefix
ENV_VARS = {key: value for (key, value) in os.environ.items() if key.startswith('CF_DDNS_')}

# Stole this from StackOverflow for demo purposes
def recursive_map(data, func):
    apply = lambda x: recursive_map(x)
    if isinstance(data, Mapping):
        return type(data)({k: apply(v) for k, v in data.items()})
    elif isinstance(data, Collection):
        return type(data)(apply(v) for v in data)
    else:
        return func(data)

# Substitutes env variables in a template string with their values 
def resolve_env(val):
  return Template(val).safe_substitute(**ENV_VARS)

# Resolve env vars in all values
config = recursive_map(config, resolve_env)

This way, you can just do something like:

{
  "cloudflare": [
    {
      "authentication": {
          "api_token": "my_hard_coded_value", 
          "api_key": {
              "account_email": "$CF_DDNS_EMAIL"
          }
      },
      ...
  ]
}

I like your suggestion. I opened a call to action in the README for anyone interested in opening a PR to add this functionality.

[timothymiller]

This discussion is being carried on in this thread https://github.com/timothymiller/cloudflare-ddns/pull/117