search

auth0 / auth0-deploy-cli

The Auth0 Deploy CLI is a tool that helps you manage your Auth0 tenant configuration. It integrates into your development workflows as a standalone CLI or as a node module.
MIT License
242 stars 147 forks source link

Potentially dangerous and inconsistent behavior for secrets #689

Open jcerjak opened 1 year ago

jcerjak commented 1 year ago

Checklist

Description

When running export, the exported files contain some secret values by default. This is dangerous, since users might accidentally push secrets to a code repository.

Additionally, behavior is inconsistent between resource types. Some resource types provide sanitized values, but they are also sanitized differently.

I haven't done extensive testing, for now I've noticed the following in exported tenant.yaml:

Expectation

I would expect the following:

I think it would be best to not expose secrets by default. Currently, there is a way to exclude secret fields by using EXCLUDED_PROPS, however you need to carefully examine the exported data and manually check for potential secret values to exclude. Instead of blacklisting approach, I propose to sanitize all secrets by default. Then users can whitelist some properties if they want to get actual secret values. For example, instead of using EXCLUDED_PROPS, add something like INCLUDED_SECRETS.

Regarding sanitization, secret values should be sanitized consistently, instead of using keywords (e.g. '##SMTP_PASS##') and _VALUE_NOT_SHOWN_. Keywords are probably more useful, since users could use https://github.com/auth0/auth0-deploy-cli/blob/master/docs/keyword-replacement.md to load secrets from environment variables. On other hand, this could be dangerous, if "reserved" Auth0 keywords (such as '##SMTP_PASS##') would collide with existing keyword mappings. Though this could probably be prevented by doing additional validation on AUTH0_KEYWORD_REPLACE_MAPPINGS.

Reproduction

  1. Given Auth0 tenant with:
    • a social connection (e.g. Google)
    • Custom STMP Email Provider
    • Custom Webhook event stream, with Authorization token set
  2. When exporting the tenant config with a0deploy export --format=yaml --output_folder=config
  3. Then config/tenant.yaml contains:
    • connections.options.client_secret for social connection contains secret value
    • emailProvider.smtp_pass contains sanitized value '##SMTP_PASS##'
    • logStreams.sink.httpAuthorization contains sanitized value _VALUE_NOT_SHOWN_

Deploy CLI version

7.15.1

Node version

v18.12.1

jcerjak commented 1 year ago

Related also to #72 and #601.

willvedd commented 1 year ago

I certainly agree that there are inconsistencies with how secret obfuscation is (or isn't) handled. I'm mostly concerned about the Google social connection exposing the client secret, I'll certainly look into that.

jcerjak commented 1 year ago

Thanks. Maybe also consider some automated way of sanitizing secrets, since with manual whitelisting approach this could happen again (and it happened before, e.g. in #601).

And please check if emailProvider.smtp_user could be sanitized as well (similar to ##SMTP_PASS##). This would make the export/import roundtrip easier for us, because the value (we use ##SMTP_USER##) will not be replaced on each export . Unless this will already be handled in #688.

jo-al-ra commented 1 year ago

I have also encountered the same issue and accidentally pushed the secrets of several social connections (like Google Enterprise Connections or SIWE) to our repository. I then tried to replace the secrets using AUTH0_KEYWORD_REPLACE_MAPPINGS in the config.json.

We usually push the config.json file for each tenant (dev, staging, prod) to our repository. Thus, we would like to replace those plain text secrets using environment variables, which will then be injected via bitbucket pipelines. But replacing specific values of the AUTH0_KEYWORD_REPLACE_MAPPINGS with environment variables is unfortunately not as easy as replacing top-level values such as the AUTH0_CLIENT_SECRET. Is there a way to replace single values of the AUTH0_KEYWORD_REPLACE_MAPPINGS without exporting the complete object as an environment variable as in the examples? https://github.com/auth0/auth0-deploy-cli/blob/master/docs/configuring-the-deploy-cli.md#examples

# Non-primitive configuration values
export AUTH0_EXCLUDED='["actions","organizations"]'
export AUTH0_KEYWORD_REPLACE_MAPPINGS='{"ENVIRONMENT":"dev"}'
a0deploy export -c=config.json --format=yaml --output_folder=local
jo-al-ra commented 1 year ago

But replacing specific values of the AUTH0_KEYWORD_REPLACE_MAPPINGS with environment variables is unfortunately not as easy as replacing top-level values such as the AUTH0_CLIENT_SECRET.

My bad. I did not set up my environment variables correctly. Using "##MY_SECRET##" also works in the AUTH0_KEYWORD_REPLACE_MAPPINGS.

I am sorry for hijacking this issue. Please be aware, that multiple connections with secrets can exist. Thus, sanitizing them all the same way using a constant like '##CUSTOM_CONNECTION_SECRET## could result in problems as well. As users might forget to replace it and will end up with the same secret for all connections.

ghenry commented 1 year ago

Hi all,

I've come to this issue as I exported my whole config from a dev UK tenant into a new EU tenant. Export has no client ids or secrets and import created all new ones on the new tenant.

Is a0deploy supposed to be able to backup a whole auth0 tenant as-is?

Thanks.

ghenry commented 1 year ago

Nevermind, I've enabled the export of these fields using INCLUDED_PROPS

Thanks!

philippefutureboy commented 2 weeks ago

Here's how I solved the issue on my end:

  1. tenant.yaml created by dump should not be under svc
  2. tenant.yaml managed by your team should be under svc

Assming you have pulled the remote tenant.yaml into tenant-remote.yaml, you can automatically replace secrets in the remote file with the keyword mappings using the following script:


const fs = require("node:fs")
const path = require("node:path")
const YAML = require('yaml')

function walkMutateMany(objects, callback) {
  // Function to walk through the object
  function walk(path, key, nodes, parents) {
    // If all nodes are arrays, map over all indexes and reassign values
    if (nodes.every(Array.isArray)) {
      const maxLength = nodes.reduce(
        (maxLength, node) =>
          node.length > maxLength ? node.length : maxLength,
        0
      );
      // If the value is an array, iterate through each element
      let resultNodes = nodes;
      for (let i = 0; i < maxLength; i += 1) {
        const values = walk(
          `${path}[${i}]`,
          i,
          nodes.map((node) => node[i]),
          nodes
        );
        resultNodes.forEach((node, nodeIndex) => {
          if (node.length > i) {
            node[i] = values[nodeIndex];
          }
        });
      }
      return resultNodes;
      // If all nodes are basic javascript objects, map over all keys and reassign values
    } else if (
      nodes.every(
        (node) =>
          typeof node === "object" &&
          node !== null &&
          node.constructor === Object
      )
    ) {
      // If the value is an object, iterate through its key-value pairs
      const allKeys = Object.keys(nodes.reduce(
        (keys, node) => {
          Object.keys(node).forEach((k) => keys[k] = true)
          return keys
        },
        {}
      ));
      let resultNodes = nodes;
      for (const key of allKeys) {
        const values = walk(
          `${path}.${key}`,
          key,
          nodes.map((node) => node[key]),
          nodes
        );
        resultNodes.forEach((node, nodeIndex) => {
          if (key in node && node.hasOwnProperty(key))
            node[key] = values[nodeIndex];
        });
      }
      return resultNodes;
      // If the value is neither an array nor an object, invoke the callback
      // This covers scalars as well as complex objects such as Date or objects created using a class
    } else {
      return callback(path, key, nodes, parents);
    }
  }

  // Start the walk from the root object
  return walk(
    "$",
    null,
    objects,
    objects.map(() => null)
  );
}

const localTenantManifest = YAML.parse(fs.readFileSync(path.join(__dirname, "../tenants/my-tenant", "tenant.yaml")).toString('utf8'))
const remoteTenantManifest = YAML.parse(fs.readFileSync(path.join(__dirname, "../tenants/my-tenant", "tenant-remote.yaml")).toString('utf8'))

function isKeyWordMapping(value) {
  return typeof value === "string" && /(##|@@)[A-Za-z0-9_]+(##|@@)/.test(value)
}

const results = walkMutateMany(
  [
    localTenantManifest,
    remoteTenantManifest
  ],
  (path, key, [local, remote], parents) => {
    console.log(path, key, [local, remote])
    return [local, isKeyWordMapping(local) ? local : remote]
  }
)

fs.writeFileSync(
  path.join(__dirname, "../tenants/my-tenant", "tenant-result.yaml"),
  YAML.stringify(results[1])
)

It basically walks through the tenant-remote.yaml and the tenant.yaml and rewrites any value in the remote that has a mapping in the tenant.yaml. That way you keep the updates in structure that can come from remote, but you also strip any secrets from the file. You can then overwrite your local tenant.yaml with the result.

WARNING: This assumes that the remote hasn't introduced secrets that aren't managed by the tooling in the repo. You should always check if the resulting file contains any new secrets before committing changes to svc.